Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Psychlist1972 opened this issue Jul 18, 2024 · 5 comments

Comments

@Psychlist1972
Copy link

Hi @starfishmod

Please reference this issue: microsoft/MIDI#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
Copy link
Member

@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
Copy link
Author

Psychlist1972 commented Jul 21, 2024

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
Copy link
Member

@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
Copy link
Author

@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 added a commit that referenced this issue Jul 31, 2024
Updated the bytestreamToUMP.h to handle timing clonk inside a sysex #24 and #19 - added tests

Added a resetBuffer to bytestreamToUMP.h part of #21

Fixes for pitch conversion #18

Handling of reserved status codes is consistent - all are now dropped fixes #17 / #16
starfishmod added a commit that referenced this issue Aug 1, 2024
…ixes #17 / #16 (Now for UMP to bytestream) - now with added tests
@starfishmod
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants