microsoft / MIDI

Windows MIDI Services
MIT License
300 stars 24 forks source link

[BUG] Messages following undefined System Common/Realtime messages are corrupt. #361

Open m-komo opened 1 month ago

m-komo commented 1 month ago

Describe the bug Sending undefined System Common/Realtime messages (0xF4, 0xF5, 0xF9, 0xFD) to the USB MIDI device which is working with the USB MIDI 1.0 driver (USBAUDIO.sys) corrupts following messages.

To Reproduce

  1. Set the Roland UM-ONE mk2 to class-compliant mode and short INPUPT and OUTPUT to loop back messages.
  2. Attach the UM-ONE to PC.
  3. Make sure the device is working with the USB MIDI 1.0 driver (USBAUDIO.sys).
  4. Open midi.exe in two windows.
  5. From one, monitor UM-ONE.
  6. From the other, send the attached "TestMT1.txt" to UM-ONE. Received messages on the monitor are corrupt (received_messages.txt).

Expected behavior All MT2 messages in the file are received as they are.

Screenshots I have attached the captured USB log file (USBLog.zip). It shows that USB packets sent to the device are already corrupt.

image

Installer Name or Version

Desktop (please complete the following information):

Device information, if this is with an external MIDI device:

Application Information

Psychlist1972 commented 1 month ago

Those undefined system common messages do not have a defined number of data bytes in the spec. The behavior ends up being undefined.

That said, I've reported this to Andrew because the translation library would end up in a state where it may never recover if something like the UM-One returns back only the status byte and no data bytes for those undefined messages.

Psychlist1972 commented 1 month ago

Note that whatever change is made here to accommodate this must also be made in the USB MIDI 2 Driver @AmeNote-Michael

AmeNote-Michael commented 1 month ago

For the USB MIDI 2 Driver (usbmidi2.sys) it is actually not an issue cause in USB MIDI 1.0 it is a single 32bit word always regardless if message is defined or not. When an undefined message, then I just ignore rest of bytes in the 32 bit data.

For the UMP to BS or BS to UMP, should just pass the undefined byte I think when going from UMP to BS or in case of BS to UMP, should be a UMP message with just the one real time message byte, rest NULL.

Psychlist1972 commented 1 month ago

Also discussing here: https://github.com/midi2-dev/AM_MIDI2.0Lib/issues/16

We may need to decline this one, because these are undefined MIDI 1.0 messages. It pains me to see it mess up the entire stream of data, but no one should be using undefined MIDI 1.0 messages.

We're much more forgiving when it comes to MIDI messages that stay in UMP. But when translating to bytes, we're at the mercy of what devices do with that data.

My recommendation to Andrew for libmidi2 is to just drop these undefined messages when translating to MIDI 1.0 byte data format. But that may not be simple to do.

Psychlist1972 commented 1 month ago

Update: Andrew is looking at options in libmidi2. Rejecting the status byte as well as any data bytes which come after the undefined message is looking to be a reasonable option. This is really about hardening the translator against bad data, because no one should be using these opcodes.

Psychlist1972 commented 1 month ago

Decision: Michael and Andrew will change their code to drop these messages in both directions.