Fix: MIDI parser — System Real-Time no longer corrupts running status#693
Conversation
System Real-Time bytes (0xF8–0xFF) can legally appear anywhere in a MIDI stream, even between data bytes of a multi-byte message. The parser was handling them inside the state machine's ParserEmpty case, which meant they first triggered a state reset and overwrote incoming_message_ fields (channel, type). This caused channel corruption — e.g. a Timing Clock (0xF8) between NoteOn bytes shifted the channel to 8 (0xF8 & 0x0F). The fix intercepts RT bytes at the very top of Parse(), before any state modification, using a stack-local MidiEvent. This leaves pstate_, incoming_message_, and running_status_ completely untouched, which is the correct behavior per the MIDI 1.0 specification. This also fixes RT bytes inside SysEx — they were previously stored as SysEx data instead of being dispatched as separate events. Added 4 new unit tests: - sysRealtimeMidMessage: clock bytes interleaved within a NoteOn - sysRealtimeDuringSysEx: clock byte inside a SysEx message - allSysRealtimeTypes: all 8 RT types parse with channel=0 - runningStatusPreservedAfterRealtime: running status + RT + channel check All 154 tests pass (including the existing runningStatSysRealtime test). Closes electro-smith#666 Closes electro-smith#665
|
Nice! This looks good at a glance. Thanks for the contribution! |
|
Thanks @stephenhensley! Let me know if you need anything or if I should make any adjustments. 🌟 |
|
@sauloverissimo Everything looks good to me. I'll be testing the related system-realtime PR w/ the optional callback on top of this. Thanks again for the contribution! |
Summary
System Real-Time bytes (0xF8–0xFF) can legally appear anywhere in a MIDI byte stream — even between data bytes of a multi-byte message — and must not affect the parser's running status. The current parser handles them inside the
ParserEmptyswitch case, which means they first trigger a state reset (pstate_ = ParserEmpty) and overwriteincoming_message_fields. This causes:0xF8 & 0x0F = 8)This is the root cause of #666 and #665.
The fix
Move System Real-Time handling to the very top of
Parse(), before any state modification:Uses a stack-local
MidiEventsopstate_,incoming_message_, andrunning_status_are completely untouched. The early return means RT bytes are transparent to the rest of the parser — exactly as the MIDI 1.0 spec requires.Tests
Added 4 new unit tests covering the scenarios that the existing
runningStatSysRealtimetest does not:sysRealtimeMidMessagesysRealtimeDuringSysExallSysRealtimeTypesrunningStatusPreservedAfterRealtimeAll 154 tests pass (including the existing
runningStatSysRealtimetest).Related
MidiHandlerlevel, but the underlying parser bug still exists without this fix