Opened 10 years ago

Closed 10 years ago

#4445 closed bug (fixed)

MIDI System Exclusive handling is inconsistent (and BeOS incompatible)

Reported by: Pete Owned by: phoudoin
Priority: normal Milestone: R1
Component: Servers/midi_server Version: R1/pre-alpha1
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

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!

Attachments (1)

PortDrivers.diff (834 bytes ) - added by Pete 10 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 by korli, 10 years ago

Component: ServersServers/midi_server
Owner: changed from axeld to korli

comment:2 by Pete, 10 years ago

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.)

comment:3 by phoudoin, 10 years ago

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;
             }    
[...]

comment:4 by phoudoin, 10 years ago

Owner: changed from korli to phoudoin
Status: newassigned

comment:5 by phoudoin, 10 years ago

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

in reply to:  5 comment:6 by Pete, 10 years ago

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.

comment:7 by phoudoin, 10 years ago

Resolution: fixed
Status: assignedclosed

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.

by Pete, 10 years ago

Attachment: PortDrivers.diff added

in reply to:  7 ; comment:8 by Pete, 10 years ago

Resolution: fixed
Status: closedreopened

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.

in reply to:  8 comment:9 by phoudoin, 10 years ago

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.

in reply to:  8 ; comment:10 by Pete, 10 years ago

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...

in reply to:  10 ; comment:11 by phoudoin, 10 years ago

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

in reply to:  11 comment:12 by Pete, 10 years ago

Replying to phoudoin:

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

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

comment:13 by phoudoin, 10 years ago

Resolution: fixed
Status: reopenedclosed

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

Note: See TracTickets for help on using tickets.