Midi input becomes unresponsive after "phantom" SysEx start byte.

[submodify]

Hello,

Been having an issue that when hot swapping out TRS Midi input cables it would occasionally cause the MIDI library to freeze or become unresponsive.

It seems that hot swapping TRS cables can occasionally cause garbled or incorrect Midi data to be produced when plugging in or unplugging the cable (I'm sure DIN cables can do this to, on rare occasion, but it seems more common on TRS cables due to how they physically connect).

I've done some basic tests, but the problem seems to be that hot swapping out a TRS connector on a Midi input, can on occasion, create a "phantom" SysEx Start, or a SysEx Stop byte (also seems to be more common when there is already MIDI data being transmitted when plugging in the TRS connector). This will then seem to lock up the MIDI library causing it not to respond to regular Midi channel messages (as it thinks it's in the middle of receiving a SysEx message).

When a SysEx Start byte is received in error, there doesn't seem to be any recovery mechanism in place for when another Channel Status Byte is subsequently received, which seems to cause this issue.

I added this code snippet between lines 1035 & 1036 of MIDI.hpp (which happens when the "extracted byte" is a channel Status byte), and it does seem to help:

        // If a Channel Status byte is received after a SysEx Start byte
		if ((mPendingMessage[0] == SystemExclusiveStart)
		|| (mPendingMessage[0] == SystemExclusiveEnd))
			{
				// Something has gone wrong... Lets reset
				resetInput();
				return false;
			}

I'm a bit of a n00b at all this, so what I've done may not be suitable, or completely off, but it does seem to help (although I've not actually tested this with SysEx messages). Anyway thanks for everyone's work on this.

[submodify]

Going by this information from:

http://midi.teragonaudio.com/tech/midispec/sysex.htm#

"The 0xF7 Status byte is dedicated to marking the end of a SysEx message. It should never occur without a preceding 0xF0 Status. In the event that a device experiences such a condition (ie, maybe the MIDI cable was connected during the transmission of a SysEx message), the device should ignore the 0xF7.

Furthermore, although the 0xF7 is supposed to mark the end of a SysEx message, in fact, any status (except for Realtime Category messages) will cause a SysEx message to be considered "done" (ie, actually "aborted" is a better description since such a scenario indicates an abnormal MIDI condition). For example, if a 0x90 happened to be sent sometime after a 0xF0 (but before the 0xF7), then the SysEx message would be considered aborted at that point. It should be noted that, like all System Common messages, SysEx cancels any current running status. In other words, the next Voice Category message (after the SysEx message) must begin with a Status."

I think there should also be a check somewhere in MIDI.hpp to ignore an 0xF7 (SysEx end) status byte, if the pending message hasn't received an 0xF0 (SysEx Start) byte. After a quick look it doesn't currently seem the case.

... and athough my code snippet above does mitigate the issue by aborting the SysEx message if a Channel Status byte is received, I guess it should also start a new pending message using that new Channel Status byte, and not just discard it.

I may have a go myself at sorting this out, but my coding skills are average at best :)

[lathoub]

I may have a go myself at sorting this out, but my coding skills are average at best :)

Good opportunity to better your coding skills! 😀
If you have working and tested code, pls issue a PR.

[submodify]

I've modified parse() a fair bit. I need to test this loads more, but if anyone spots any glaring mistakes, or just feedback in general, it would be appreciated. Any Status byte (except realtime) should now cancel a SysEx message and begin a new message.... and SysEx stop should be ignored unless there has been a previous SysEx start. I've briefly tested this on my Teensy Midi Interface which seems to work OK, but will need to test the Sysex/Running Status side of things a lot more.

// Private method: MIDI parser
template<class Transport, class Settings, class Platform>
bool MidiInterface<Transport, Settings, Platform>::parse()
{

    if (mTransport.available() == 0)
        return false;

    mLastError &= ~(1UL << ErrorParse);  // Clear ErrorParse bit

    const byte extracted = mTransport.read();

    if (extracted >= 0x80) {
        // Get Status including Undefined ones so we can properly deal with them
        const MidiType pendingType = extracted < 0xF0 ? MidiType(extracted & 0xF0) : MidiType(extracted);
        switch (pendingType) {
            // One byte realtime will complete now
            case Start:
            case Continue:
            case Stop:
            case Clock:
            case Tick:  // <---- Is tick relevant anymore?? .... Not part of the MIDI spec
            case ActiveSensing:
            case SystemReset:
                // Might as well quickly deal with these now.
                mMessage.type = pendingType;
                mMessage.channel = 0;
                mMessage.data1 = 0;
                mMessage.data2 = 0;
                mMessage.length = 1;
                mMessage.valid = true;
                return true;
            case NoteOn:
            case NoteOff:
            case ControlChange:
            case PitchBend:
            case AfterTouchPoly:
            case SongPosition:
                mPendingMessageExpectedLength = 3;
                break;
            case ProgramChange:
            case AfterTouchChannel:
            case TimeCodeQuarterFrame:
            case SongSelect:
                mPendingMessageExpectedLength = 2;
                mPendingMessage[2] = 0;  // Zero unused data
                break;
            // One byte common will be completed this run (This will also reset running status as this is not realtime)
            case TuneRequest:
                mPendingMessageExpectedLength = 1;
                mPendingMessage[1] = 0;  // Zero unused data
                mPendingMessage[2] = 0;  // Zero unused data
                break;
            case SystemExclusiveStart:
                mPendingMessageExpectedLength = MidiMessage::sSysExMaxSize;
                mMessage.sysexArray[0] = SystemExclusiveStart;
                break;
            case SystemExclusiveEnd:
                if (mPendingMessage[0] == SystemExclusiveStart) {            // If were currently doing SysEx
                    mPendingMessageExpectedLength = ++mPendingMessageIndex;  // Update expected lenght as we have an EOX and within Buffer
                    break;
                }
            default:
                mLastError |= 1UL << ErrorParse;  // Error: Undefined Status or stray EOX
                if (mErrorCallback)
                    mErrorCallback(mLastError);
                if ((pendingType != Undefined_F9) && (pendingType != Undefined_FD)) {  // Dont reset for 0xFD & 0xF9 (Undefined Realtime)
                    mPendingMessageIndex = 0;                                          // Message reset for stray EOX and Undefined common F4/F5
                }
                return false;
        }
        mPendingMessage[0] = extracted;                                 // Status seems good so lets store in pending
        if (extracted != SystemExclusiveEnd) mPendingMessageIndex = 1;  // Set PendingMessageIndex to 1 (needs to be unchanged for EOX)

    } else {
        // Check Status
        if (mPendingMessageIndex == 0) {
            mLastError |= 1UL << ErrorParse;  // Error: No Status
            if (mErrorCallback)
                mErrorCallback(mLastError);
            return false;
        }
        // Add Data
        if (mPendingMessage[0] == SystemExclusive)
            mMessage.sysexArray[mPendingMessageIndex] = extracted;
        else
            mPendingMessage[mPendingMessageIndex] = extracted;
        mPendingMessageIndex++;
    }
    // Check for a complete message
    if (mPendingMessageIndex >= mPendingMessageExpectedLength) {
        // Process SysEx
        if (mPendingMessage[0] == SystemExclusiveStart || mPendingMessage[0] == SystemExclusiveEnd) {

            mMessage.type = SystemExclusive;
            mMessage.data1 = mPendingMessageIndex & 0xff;      // LSB
            mMessage.data2 = byte(mPendingMessageIndex >> 8);  // MSB
            mMessage.channel = 0;
            mMessage.length = mPendingMessageIndex;

            // SysEx can be larger than the allocated buffer size (must handle with callbacks only)
            // Split SysEx like so:
            // first:  0xF0 .... 0xF0
            // midlle: 0xF7 .... 0xF0
            // last:   0xF7 .... 0xF7

            // SysEx buffer not full...  EOX
            if (mPendingMessage[0] == SystemExclusiveEnd) {
                mMessage.sysexArray[mPendingMessageIndex - 1] = SystemExclusiveEnd;
                mPendingMessageIndex = 0;
                if (mMessage.sysexArray[0] == SystemExclusiveEnd) {  // This is the last chunk of split SysEx
                    mMessage.valid = false;
                    launchCallback();
                } else {  // Not Split SysEx
                    mMessage.valid = true;
                }
            } else {  // SysEx buffer full
                if (mMessage.sysexArray[0] == SystemExclusiveStart) {
                    if (mErrorCallback)
                        mErrorCallback(1UL << WarningSplitSysEx);  // Notify but no need to store this warning?
                }
                byte lastByte = mMessage.sysexArray[Settings::SysExMaxSize - 1];
                mMessage.sysexArray[Settings::SysExMaxSize - 1] = SystemExclusiveStart;
                mMessage.valid = false;
                launchCallback();

                // Prep next SysEx data chunk to start with 0xF7
                mMessage.sysexArray[0] = SystemExclusiveEnd;
                mMessage.sysexArray[1] = lastByte;
                mPendingMessageIndex = 2;
            }

            return mMessage.valid;
        }

        // Process message
        if (mPendingMessage[0] < 0xF0) {  // Channel message
            mMessage.type = MidiType(mPendingMessage[0] & 0xF0);
            mMessage.channel = (mPendingMessage[0] & 0x0F) + 1;
            mPendingMessageIndex = 1;  // Set to 1 for running status
        } else {                       // Common message
            mMessage.type = MidiType(mPendingMessage[0]);
            mMessage.channel = 0;
            mPendingMessageIndex = 0;
        }
        mMessage.data1 = mPendingMessage[1];
        mMessage.data2 = mPendingMessage[2];
        mMessage.length = mPendingMessageExpectedLength;
        mMessage.valid = true;

        return true;
    } else {
        // We need more input...
        return (Settings::Use1ByteParsing) ? false : parse();
    }
}

[mink99]

Just to add some „historic“ knowledge and experience.
When the trs wiring for midi was introduced, there was a good reason that it would not be part of the standard. When you plug in a trs cable, the one connector of the plug (the tip) would make connections to all 3 contacts of the sink. This would create several random bytes to be recognised by the serial receiver.
To avoid this the din connector has clearly separated the signal, pwr and gnd pins, so that only the correct connectors touch each other.
When Arturia released the beatstep they argued that using mini trs would reduce the height of the device in comparison to a proper din midi plug, this was proven an utter lie , by simply measuring. But it created just these problems as described, random garbage when hot plugging.
The same applies for example on behringer mini synths if you „hot plug“ it by pulling and pushing the mini-trs.
But there is a simple solution : just use an adapter from trs to midi-din wiring, and always keep the adapter attached to the device, and „hot plug“ the midi cable.
Even mechanical switching solutions like the Monacor midi patchbay (mme80) work like that without issues.
I doubt that it is worth the effort, to harden any library against random garbage, when a simple adapter or connection box will solve that issue much cheaper and more reliable.

[mink99]

This random garbage serial data often would not only contain 0x7F but anything from 0x00 to 0xFF , just depending on the setup, the weather, the humidity etc. It was just in this setup, that 0x7F appeared so prominently, in other setups like mine the pattern may have been different. (Stupid remark : i love the 626 crashing)
And of course the hanging notes problem, the device receiving a note on, and then being unplugged, cannot be solved in software.

[submodify]

I doubt that it is worth the effort, to harden any library against random garbage, when a simple adapter or connection box will solve that issue much cheaper and more reliable.

Currently an erroneous SysEx Start or Stop byte will cause Midi Input to lock up, needing a power cycle to remedy. You don't think that's worth fixing?

[franky47]

You could (albeit with a very low probability) catch the end of a partial SysEx stream mid-flight when hot-plugging a DIN cable, so this at least is worth handling (IIRC it already does, by ignoring orphan data bytes not associated with an initial status byte).
Same goes for un-terminated messages on hot-unplugging, but there is not a lot we can do there (aside from a timeout, but that's outside of the MIDI spec).

How do other pieces of hardware handle this "garbage" input (if they even do)?

[submodify]

Garbage data is fine, as long as it doesn't cause the "Device" to stop functioning as it currently is with the garbage SysEx Start/Stop byte.

I've tested other commercial Midi devices (Like CME) an the 'Garbage' produced is very similar, although they are obviously handling the SysEx side properly as they don't seem to lock up.

Edit: After re-testing my CME U6 interface... hot plugging TRS connectors can indeed cause similar issues on that also. It doesn't completely lock up, but the Midi stream is severely compromised.

[submodify]

You could (albeit with a very low probability) catch the end of a partial SysEx stream mid-flight when hot-plugging a DIN cable, so this at least is worth handling (IIRC it already does, by ignoring orphan data bytes not associated with an initial status byte).

I'm not sure what would happen with the data here. The more I look at the current parse() function, the more it seems broken(it doesn't look like it discards any data ?). Also... It looks to me that a SysEx Stop byte is treated exactly the same as a SysEx Start, so a lone SysEx Stop byte in the above situation (after plugging in a DiN mid SysEx) would start a new SysEx message which is totally bonkers.

My rewritten parse() function above should handle this properly and just discard the SysEx Stop..... and is a little less bloated than the original :)

[mink99]

I doubt that it is worth the effort, to harden any library against random garbage, when a simple adapter or connection box will solve that issue much cheaper and more reliable.

Currently an erroneous SysEx Start or Stop byte will cause Midi Input to lock up, needing a power cycle to remedy. You don't think that's worth fixing?

At the cost below 3 Euro for a fix, (soldering an adapter) yes, it is not worth it, absolutely. Because the problem is not just one byte being wrong…..

[submodify]

At the cost below 3 Euro for a fix, (soldering an adapter) yes, it is not worth it, absolutely. Because the problem is not just one byte being wrong…..

Myself and other developers I know that use this library on commercial products that may have TRS Midi connectors cannot really rely on end users using TRS to DIN adapters, or being overly vigilant on NOT hot swapping out connectors. I think it's prudent to fix any device malfunction caused by simply plugging in cables.

It's also pretty obvious that currently the parse() function isn't following the guidelines for dealing with SysEx status bytes properly, which is the root cause of why the Midi input locks up.

TRS Midi will always be a bit janky in my mind, but it seems that it's here to stay. Garbage Status/Data bytes are acceptable (they are in essence still valid Midi messages), and even stuck notes (which can always be handled the usual way by means of a panic button/CC123), but the current handling of receiving SysEx Status bytes is not acceptable.

I'm fairly confident my modified parse() function above should fix all this, by following the guidelines for receiving SysEx Status bytes. Once I'm satisfied that it handles SysEx & Running Status correctly (it should do), I'll create a PR, but to me it already seems to be a better option than the current method. Erroneous SysEx bytes will no longer cause my Midi input to fail.

[mink99]

Do you have a test environment ? Which trs devices do you have available ? Which devices are you using ? Maybe i could help ?

My post may have sounded a bit harsh, i apologise for that …. It looks as if the problem of dangling sysex bytes can be solved by your code, which is good.
But I am not sure whether this solved the larger problem of the foul designed trs plugs.
During my participation in the German „Synthesizer Forum“ , when those TRS plugs became famous, we did a lot of research on that, even mocked Arturia and others on that, and learned a lot on „how not to do it“. What we found out, was that it was pure random bytes what was received (and therefore may or may not contain a 0xF7). The steady 31250 baud stream was interrupted and would not recover.
We came to the solution that midi patchbays would be recommended. Very much similar why hot plugging of audio cables would not be recommended (from a musician‘s perspective)

Again, if i can providesupport for testing various configurations, feel free to contact me.

[franky47]

As for testing, feel free to use the unit tests already in place, and to add more to cover junk data cases.