monome / norns

norns is many sound instruments.
http://monome.org
GNU General Public License v3.0
614 stars 144 forks source link

MIDI SYSEX drops 1-2 bytes from incoming data depending on length #1644

Closed okyeron closed 1 year ago

okyeron commented 1 year ago

Working with 16n and a new controller I'm making I discovered that incoming SYSEX data is getting dropped by norns.

The device_midi.c code says it is grouping data into 3 byte chunks.

What appears to be happening is that if there are one or two bytes just prior to the F7 SYSEX stop byte, then those bytes are dropped. A chunk of 3 is normal, but a chunk of 1 or 2 gets lost.

As an example:
F0 7D 00 01 1F 1A F7 comes through OK ( 3bytes + 3 bytes + F7) F0 7D 00 01 1F F7 shows up as F0 7D 00 F7 ( 3bytes + 2 bytes + F7 drops the last 2 bytes before F7) F0 7D 00 01 F7 also shows up as F0 7D 00 F7 ( 3bytes + 1 bytes + F7 drops the last 1 byte before F7)

On the norns end this is observable by just sending some SYSEX data and tab.print(data) on incoming MIDI event data.

With 16n you can test this with the sines script and add a tab.print(data) to line 100 of lib/16n.lua. When 16n connects it should send 90bytes, but only 88 are received.

tehn commented 1 year ago

try doing some debugging on the c level and i suspect you'll stumble upon the fix

catfact commented 1 year ago

its not actually obvious to me from looking, the code in device_midi.c seems nominally correct?

rubber ducking here since i have not fired up the norns HW in a while:

starting from where we consume the input buffer (arbitrary byte count?): https://github.com/monome/norns/blob/main/matron/src/device/device_midi.c#L245

we only call midi_input_msg_acc if a byte is determined to be not-a-status-byte: https://github.com/monome/norns/blob/main/matron/src/device/device_midi.c#L256 https://github.com/monome/norns/blob/main/matron/src/device/device_midi.c#L117 (conformant sysex data should pass this test since byte values must be <0x7f.)

then although the comment does say sysex bytes are grouped by 3, in fact this is only the intial / default value given to the message length at the start of a message - it should be taken to mean - "pass sysex data through in 3 byte chunks until a stop byte is seen".

when 0x7f is encountered, it actually sets the message length to the current buffer position: https://github.com/monome/norns/blob/main/matron/src/device/device_midi.c#L237

the mechanism for determining whether to post a midi event, simply checks for this very condition: that the byte position is equal to the message length: https://github.com/monome/norns/blob/main/matron/src/device/device_midi.c#L242 https://github.com/monome/norns/blob/main/matron/src/device/device_midi.c#L263

... if it's in the middle of a systex stream, the length will be the default value of 3 bytes and it will fire when 3 bytes have arrived. but if it sees a sysex end message, it should truncate the buffer length and immediately post the event.

so i think the bug is elsewhere. maybe further down. @okyeron , this is where i concur with @tehn: since you have a working test case (which would be sort of a pain to put together otherwise, requiring cuystom HW/FW or etc), it would be highly effective if you could see what is happening in a debugger.

or, indeed, just adding a print statement!

at the end of midi_input_msg_post:

fprintf(stderr, "posting midi event; byte count = %d\n", state->msg_len);

(if it posts any events with byte count < 3, then this module is working as intended and the problem is further down - in lua or etc.)

meantime i'll continue rubber ducking further down the program flow if i get a moment.

this shouldn't be too hard to track down with a little more effort.

ngwese commented 1 year ago

i'm also willing to jump in if need be since this is all code that i changed/introduced as part of supporting MIDI running status (which itself was needed to support the virtual midi interface / rtpmidi work)

okyeron commented 1 year ago

Testing situation from above with the fprintf in midi_input_msg_post

sending the sysex below and the output:

F0 7D 00 01 1F 1A F7  --> posting midi event; byte count = 3
                          posting midi event; byte count = 3
                          posting midi event; byte count = 1

F0 7D 00 01 1F F7       --> posting midi event; byte count = 3
                           posting midi event; byte count = 1

F0 7D 00 01 F7           --> posting midi event; byte count = 3
                            posting midi event; byte count = 1

byte count 1 is the F7 sysex stop byte.

Throwing a load of print statements in there I can see that midi_input_msg_post isn't firing because midi_input_msg_is_complete is false when state->msg_pos is 1 or 2. Thus the last 2 bytes are not posted.

midi_input_msg_is_complete seems to be the culprit, but I'm not seeing a way to fix it.

catfact commented 1 year ago

oh i think i get it.

the stop byte is handled as a status byte (correctly)

so it hits here and calls midi_input_msg_start instead of midi_input_msg_acc! https://github.com/monome/norns/blob/main/matron/src/device/device_midi.c#L257

and that in fact resets both the buffer position and the message length (calls midi_msg_len()); sets the msg length to 1: https://github.com/monome/norns/blob/main/matron/src/device/device_midi.c#L166-L167

then in the next part of dev_midi_consume_buffer it considers the message complete and posts it with length = 1. (and it has also reset the buf position and overwritten oldest data with the stop byte.)


it's a simple fix but not a one-liner:

okyeron commented 1 year ago

Fixed by #1648