midi2-dev / AM_MIDI2.0Lib

A MIDI 2.0 C++ Library
MIT License
22 stars 3 forks source link

Messages following undefined System Common/Realtime messages are corrupt. #16

Closed Psychlist1972 closed 1 month ago

Psychlist1972 commented 1 month ago

Hi @starfishmod

Please reference this issue: https://github.com/microsoft/MIDI/issues/361

We're using UMP to bytestream and bytestream to UMP from your lib to handle this conversion within the service when we're working with either our legacy MIDI 1.0 driver or with a vendor driver. (@AmeNote-Michael what do you do with these messages for a MIDI 1.0 device within the new driver?)

image

It looks like your lib creates two data bytes for the messages, and then when reading back, reads two data bytes, whether the device sent them or not (remember, the # of bytes is undefined).

The data bytes for these messages are undefined. For reasons discussed in the last API working group, we don't filter out any messages, so not sure what is expected here. I imagine different devices could handle this in different ways, some having zero data bytes, and some with two.

(Also not sure if bytestreamParse handles running status. I don't have a test case set up for that yet, but I know I will run into it eventually in USB, and likely immediately in BLE.)

Thoughts on how to universally handle this? My original thinking was the translation code needs to check the high bit to see if the next byte is a new message, but that would completely change your processing approach.

Pete

starfishmod commented 1 month ago

@Psychlist1972 this is a complex issue. This library deals with translation as defined by the UMP spec. There is no definition for the 0xF4/0xF9. Given that these are undefined... I'm not sure what the correct translation should be? I can't wait for "next high bit status" because that could cause issues as well, especially as it might never come.

As to the errors described after this data has been sent. I am adding some more test cases, but I'm having trouble replicating.

Psychlist1972 commented 1 month ago

Thanks Andrew. I need to set up a repro for this on my side as well. It's low priority, but the fact that it is completely messing up the messages afterwards is a bit of a problem. Perhaps libmidi2 should just drop these completely when translating to/from byte stream since they are not in the MIDI 1.0 spec. IMO, that would be reasonable given that we're dealing with MIDI 1.0 here. (I assume the Mackie protocol doesn't use these)

My section question above was if libmidi2 supports running status. I didn't see anything obvious in the logic which would indicate that it does. Can you advise?

starfishmod commented 1 month ago

@Psychlist1972

Perhaps libmidi2 should just drop these completely when translating to/from byte stream since they are not in the MIDI 1.0 spec.

Yeah - Mike and I are going to sit down and come up with some suggestions around this. For exactly this situation.

My section question above was if libmidi2 supports running status.

Yes it does - and there are test cases in there for checking :) Note it's support for running status on bytestream to UMP. For UMP to bytestream it does not create running status data. I have thought about adding it as an option - but I'm not sure.

Psychlist1972 commented 1 month ago

@Psychlist1972

Perhaps libmidi2 should just drop these completely when translating to/from byte stream since they are not in the MIDI 1.0 spec.

Yeah - Mike and I are going to sit down and come up with some suggestions around this. For exactly this situation.

Excellent.

My section question above was if libmidi2 supports running status.

Yes it does - and there are test cases in there for checking :) Note it's support for running status on bytestream to UMP. For UMP to bytestream it does not create running status data. I have thought about adding it as an option - but I'm not sure.

Great, thanks. I don't have the test cases yet. I just know I'll need it for BLE MIDI. Creating running status as an option could be helpful in that case.

Pete

starfishmod commented 1 month ago

@Psychlist1972 I believe this is now fixed. All undefined System messages are now dropped