MIDI System Exclusive handling is inconsistent (and BeOS incompatible)

[Pete]

Under BeOS, when one outputs System Exclusive MIDI data with SpraySystemExclusive(), or receives it via the SystemExclusive method, one deals with only the actual data portion of the sequence -- the '0xF0' start byte and '0xF7' trailer are omitted (supplied and removed as necessary by the midi_server).

With Haiku, SpraySystemExclusive seems to behave the same, sending a properly formatted SysEx sequence to the output device, but SystemExclusive passes the whole received sequence, including the start and end bytes.

Not only is this bad for BeOS apps, but it's inconsistent with the handling of other MIDI events, where the status byte is stripped and processed before the appropriate method is invoked. It's also internally inconsistent, because if a SysEx sequence is recorded and resent, it ends up with two start and two end bytes!

[Pete]

I should amend the above slightly. Received SysEx data only actually contains the start byte (0xF0). It is terminated by the 0xF7 end byte, but this doesn't get added to the SysEx data itself; it appears as a separate following System Common event. (It should be swallowed rather than passed on.)

[phoudoin]

Try to modify src/servers/midi/PortDrivers.cpp that way (changes between >>> & <<<)

!cpp
int32 MidiPortProducer::GetData()
{
[...]

    while (keepRunning)
    {
        if (read(fd, &next, 1) != 1)
        {
            perror("Error reading data from driver");
            if (haveSysEx)
            {
                free(sysexBuf);
            }
            return B_ERROR;
        }

>>>     if (haveSysEx && next < 0x80)  // System Exclusive data byte
        {
             sysexBuf[sysexSize++] = next;
             if (sysexSize == sysexAlloc)
             {
                 sysexAlloc *= 2;
                 sysexBuf = (uint8*) realloc(sysexBuf, sysexAlloc);
             }
             continue;
        }
<<<
        if ((next & 0xF8) == 0xF8)  // System Realtime
        {
             SpraySystemRealTime(next);
        }
        else if ((next & 0xF0) == 0xF0)  // System Common
        {
             runningStatus = 0;
             msgBuf[0] = next;
             msgPtr = msgBuf + 1;
             switch (next)
             {
                 case B_SYS_EX_START:
                     sysexAlloc = 4096;
                     sysexBuf = (uint8*) malloc(sysexAlloc);
>>>
                     // sysexBuf[0] = next;
                     sysexSize = 0; // 1;
<<<
                     haveSysEx = true;
                     break;

                 case B_SONG_POSITION:
                     needed = 2;
                     msgSize = 3;
                     break;

                 case B_MIDI_TIME_CODE:
                 case B_SONG_SELECT:
                 case B_CABLE_MESSAGE:
                     needed = 1;
                     msgSize = 2;
                     break;
                      
>>>              case B_SYS_EX_END:
                     if (haveSysEx)
                     {
                         SpraySystemExclusive(sysexBuf, sysexSize);
	                 haveSysEx = false;
                     }
                     break;

                 case B_TUNE_REQUEST:
<<<                  SpraySystemCommon(next, 0, 0);
                     break;
             }    
[...]

[phoudoin]

I've made some change along the proposed one above in hrev33127. Please test.

[Pete]

Replying to phoudoin:

I've made some change along the proposed one above in hrev33127. Please test.

Passes what I threw at it. Couldn't check if System Realtime messages can be interspersed (can't think of a way to generate them), but System Exclusives now get sprayed with the correct content, and the 'end' byte no longer appears as a separate message. Tested both through external USB MIDI, and with internal producer and consumer nodes through PatchBay.

[phoudoin]

Okay, thanks for testing. One major change I've done too regarding SysEx is that now *only* F7 byte can end it, and except F8 all bytes above 0x80 interleaved in F0 ... F7 sequence are discarded plain and simple. Before, any byte in [0x81, 0xF7] range was ending a SysEx. According to MIDI spec, that was wrong, but maybe we're too strict now on this regard.

I'll closed this ticket now, to be reopened if needed.

[Pete]

Replying to phoudoin:

Okay, thanks for testing. One major change I've done too regarding SysEx is that now *only* F7 byte can end it, and except F8 all bytes above 0x80 interleaved in F0 ... F7 sequence are discarded plain and simple. Before, any byte in [0x81, 0xF7] range was ending a SysEx. According to MIDI spec, that was wrong, but maybe we're too strict now on this regard.

I'll closed this ticket now, to be reopened if needed.

Sorry -- reopened it again... I realized that I could test interleaved events, because I can generate any sequence of bytes I like to send as a 'SysEx' (except that the F0/F7 will always be added). So I was able to verify that f7 ends the event properly, and interleaved RealTime events get passed on. [Unfortunately there's a (known) bug in the UNO firmware that screws this up if the byte-count is not a multiple of 4, and it took me all afternoon to realize it!]

However, the behaviour was still not quite according to the MIDI Spec as I understand it. Any non-SysEx-data byte, except RealTime, is supposed to terminate the SysEx sequence, and if it is a valid status byte it should become the first of a new event. So I've made a further slight modification as follows:

--- haiku/trunk/src/servers/midi/PortDrivers.cpp	2009-09-14 04:34:20.000000000 +0000
+++ haiku/src/servers/midi/PortDrivers.cpp	2009-09-16 17:04:20.000000000 +0000
@@ -114,14 +114,21 @@
 					sysexAlloc *= 2;
 					sysexBuf = (uint8*) realloc(sysexBuf, sysexAlloc);
 				}
+				continue;
 			} else if (next == B_SYS_EX_END) {
 				SpraySystemExclusive(sysexBuf, sysexSize);
 				haveSysEx = false;
+				continue;
 			} else if ((next & 0xF8) == 0xF8) {
 				// System Realtime interleaved in System Exclusive byte(s)
 				SpraySystemRealTime(next);
+				continue;
+			} else {	// must be a new status byte
+				// Terminate the sequence
+				SpraySystemExclusive(sysexBuf, sysexSize);
+				haveSysEx = false;
+				// and *don't* go back for another byte
 			}
-			continue;
 		}
 		
 		if ((next & 0xF8) == 0xF8)  // System Realtime

Umm -- not sure if that's easy to extract, so I'll attach the diff as well...

Tested, and it seems to work, aside from the UNO bug.

[phoudoin]

Replying to Pete:

However, the behaviour was still not quite according to the MIDI Spec as I understand it. Any non-SysEx-data byte, except RealTime, is supposed to terminate the SysEx sequence, and if it is a valid status byte it should become the first of a new event.

That what I was fearing, indeed.

So I've made a further slight modification as follows:

Sounds good. Will comit it tonight.

[Pete]

Correcting myself for the sake of correctness...Pete:

[Unfortunately there's a (known) bug in the UNO firmware that screws this up if the byte-count is not a multiple of 4...]

I meant a multiple of 3 (The contents of a single USB MIDI packet).

Also, you may notice that the revised code has a bit of redundancy. I went for straightforwardness rather than compactness, but I think this slightly smaller code should work as well (untested):

	if (haveSysEx) { // System Exclusive mode
		if (next < 0x80) { // System Exclusive data byte
			sysexBuf[sysexSize++] = next;
			if (sysexSize == sysexAlloc) {
				sysexAlloc *= 2;
				sysexBuf = (uint8*) realloc(sysexBuf, sysexAlloc);
			}
			continue;
		} else if ((next & 0xF8) == 0xF8) {
			// System Realtime interleaved in System Exclusive byte(s)
			SpraySystemRealTime(next);
			continue;
		} else {
			// Terminate the sequence
			SpraySystemExclusive(sysexBuf, sysexSize);
			haveSysEx = false;
			if (next == B_SYS_EX_END) continue;	// discard this byte
		}
	}

Change or not as you like...

[phoudoin]

Could I close this ticket? After hrev33159, it should now works as expected...

[Pete]

Replying to phoudoin:

Could I close this ticket? After hrev33159, it should now works as expected...

Yup -- I think we're done here...

[phoudoin]

Except if I screw it in hrev33265, which is supposed to be only coding style cleanup, it's done. We can always reopen it.