Circuitpython: usb_midi may be introducing additional 2-byte message

[CedarGroveStudios]

@jedgarpark discovered that sending a _ProgramChange_ 2-byte message using usb_midi incorrectly produced two _ProgramChange_ messages, the first with a zero patch byte value and the second with the correct patch byte value. Regular one and three-byte MIDI messages seem to be unaffected.
The zero-value _ProgramChange_ message was interpreted by Macintosh-based MIDI sniffers as incorrectly formatted. I used Windows 10-based MIDI sniffers to view the message traffic for the test.
I tested and confirmed the issue with Adafruit CircuitPython 4.1.0 on 2019-08-02; Adafruit PyGamer with samd51j19 and the adafruit-circuitpython-bundle-4.x-mpy-20190808 library bundle.
The test code: https://github.com/CedarGroveStudios/Classic_MIDI_FeatherWing/blob/master/examples/ProgramChange_test.py
Test results using a USB MIDI sniffer (MIDI Tool). Yellow highlighted messages were incorrectly transmitted.

Switched to UART output instead of MIDI. Results show that the UART output didn't add the invalid message with a zero patch byte value:

Debugger output to Mu for both USB and UART shows that the correct data was sent from within adafruit_midi:

Also monitored the USB and UART messages with a second MIDI sniffer, the one within my DAW, to confirm that the problem wasn't caused by the sniffer itself. Same results.

[kevinjwalters]

I had a brief look at this earlier chatting with John over Discord and couldn't see what was wrong from a library or MIDI point of view. Very interesting and very bizarre. Top debugging!

[jepler]

down in tinyusb at tud_midi_n_write, it looks like the ProgramChange message (msg=0xc) is not directly supported -- instead, it uses the cin=0xf option to send it as two one-byte messages. If my reading is correct, it's possible that some receiving software could treat this as two ProgramChange messages, only without a program number attached to the first one, never mind that the document I found from usb.org says it is permitted: (https://www.usb.org/sites/default/files/midi10.pdf)

In any case, I'd be tempted to continue diagnosing this by either making ProgramChange properly supported in tinyusb's midi_device.c and check if it fixes the problem; or dropping down a level and doing some usb sniffing to check my theory; but that sends you right back to modifying tinyusb, I suspect.

Untested:

diff --git a/src/class/midi/midi_device.c b/src/class/midi/midi_device.c
index 26716772..5a9ce827 100644
--- a/src/class/midi/midi_device.c
+++ b/src/class/midi/midi_device.c
@@ -161,6 +161,9 @@ uint32_t tud_midi_n_write(uint8_t itf, uint8_t jack_id, uint8_t const* buffer, u
         } else if ((msg >= 0x8 && msg <= 0xB) || msg == 0xE) {
             midi->message_buffer[0] = jack_id << 4 | msg;
             midi->message_target_length = 4;
+        } else if (msg == 0xC || msg == 0xD) {
+            midi->message_buffer[0] = jack_id << 4 | msg;
+            midi->message_target_length = 3;
         } else if (msg == 0xf) {
             if (data == 0xf0) {
                 midi->message_buffer[0] = 0x4;

(message_target_length = 3 because the extra byte introduced for USB framing is included in the count)

[jedgarpark]

@jepler Thanks for checking into it and discovering that. In case it matters, I was on mac os 10.13.6 and seeing the issue when sending the Program Change message from a Circuit Playground Express and reading the messages with both MIDI Tools and MIDI Monitor.

[tannewt]

@jepler Looks like you found the issue! I kinda figured that word packing would have an issue or two. It's kinda crazy to require IMO. Please PR TinyUSB for the fix.

[kevinjwalters]

Are you referring to this, USB-MIDI Event Packets on page 16 of https://www.usb.org/sites/default/files/midi10.pdf

MIDI data is transferred over USB using 32-bit USB-MIDI Event Packets. These packets provide an efficient method to transfer multiple MIDI streams with fixed length messages. The 32-bit USB-MIDI Event Packet allows multiple "virtual MIDI cables" routed over the same USB endpoint. This approach minimizes the number of required endpoints. It also makes parsing MIDI events easier by packetizing the separate bytes of a MIDI event into one parsed USB-MIDI event.

Not claiming that's the _current_ spec. btw just the first thing I found.

[jepler]

Interestingly, on Linux I get ...

so it'll be tough for me to test any fixes.

[jedgarpark]

Odd. The mac os receives it as straight up broken. Windows seems to get the intended Program Change message plus a bonus unintended one. And now your Linux test show it perfectly fine.

[jepler]

@CedarGroveStudios Is the program you used in the original report available to download? I can pop over to windows for testing/debugging (no macs in the house though)

[jedgarpark]

import adafruit_midi
import time

from adafruit_midi.timing_clock            import TimingClock
from adafruit_midi.channel_pressure        import ChannelPressure
from adafruit_midi.control_change          import ControlChange
from adafruit_midi.note_off                import NoteOff
from adafruit_midi.note_on                 import NoteOn
from adafruit_midi.pitch_bend              import PitchBend
from adafruit_midi.polyphonic_key_pressure import PolyphonicKeyPressure
from adafruit_midi.program_change          import ProgramChange
from adafruit_midi.start                   import Start
from adafruit_midi.stop                    import Stop
from adafruit_midi.system_exclusive        import SystemExclusive
from adafruit_midi.midi_message            import MIDIUnknownEvent

import usb_midi
midi = adafruit_midi.MIDI(midi_in=usb_midi.ports[0], midi_out=usb_midi.ports[1], in_channel=0, out_channel=0, debug=True)
while True:
    midi.send(Stop())
    midi.send(NoteOn(60, 100))
    midi.send(ControlChange(25, 127))
    midi.send(ProgramChange(33))
    time.sleep(1) 

[jepler]

I meant the Windows program with the title "MIDI input messages"

[jedgarpark]

Ah, got it. That's MIDI Tools https://mountainutilities.eu/miditools

[jepler]

OK, I reproduced the problem with miditools, verified that my proposed change fixes it, and filed an issue and a PR with tinyusb: https://github.com/hathach/tinyusb/pull/99

Subsequent to that, we can PR "pull in updated tinyusb" which will enable us to close this issue up.

[jedgarpark]

Great work! Thanks so much for diving into this @jepler

[jepler]

Happy I could help, and learn something in the process!