From bf6b02ba987b3da5f055c769cd66ea190639f7d1 Mon Sep 17 00:00:00 2001 From: diegoMalagrida Date: Wed, 29 Jul 2026 08:00:25 +0000 Subject: [PATCH] MIDI: parse the hardware port as a byte stream The screenless pedal is meant to be programmed over either USB or its TRS Type-A MIDI ports, and the scene protocol now carries its schema, state and routing commands over SysEx. The transmit side learned all the USB-MIDI CIN lengths in commit db24ff61829d ("Get the USB-MIDI code index numbers right in both directions"), but uart_midi_read() still treats F0-F7 as zero-length status bytes. A SysEx request arriving at the TRS input disappears before handle_midi_packet() sees it. The same reader recognizes only channel voice messages. It drops every real-time byte, so MIDI Clock arriving at the TRS jack never reaches USB, and System Common messages disappear too. A two-byte channel message also leaves the final packet byte holding data from an earlier three-byte event. Replace that ad-hoc reader with a byte-stream parser. Keep channel running status, emit real-time without disturbing a partly read message, handle System Common, and pack SysEx into the CIN 4..7 forms USB-MIDI needs. System Reset is the real-time exception and clears the parser state. Zero-pad every packet byte that the MIDI message does not use. Keep the parser next to the CIN helpers so the hardware MIDI path has host-side coverage. Feed it channel running status, real-time interleaved with both channel and SysEx messages, System Common, System Reset, and all three SysEx ending forms. Fixes: 95881ae06f8b ("Re-organize MIDI code a bit, add UART option") Assisted-by: Codex:gpt-5 Signed-off-by: diegoMalagrida --- Firmware/midi/midi.h | 121 +++++++++++++++++++++++++++++++++++++ Firmware/midi/uart.h | 33 +--------- Validation/test-midi-cin.c | 92 ++++++++++++++++++++++++++-- 3 files changed, 210 insertions(+), 36 deletions(-) diff --git a/Firmware/midi/midi.h b/Firmware/midi/midi.h index c3af99e..a7c72df 100644 --- a/Firmware/midi/midi.h +++ b/Firmware/midi/midi.h @@ -123,6 +123,127 @@ static inline uint8_t midi_status_cin(uint8_t status) return status >> 4; } +// +// Turn a raw MIDI byte stream into USB-MIDI event packets. +// +// The UART sees the wire format, with running status and real-time bytes +// interspersed anywhere in another message. USB-MIDI wants self-contained +// packets instead. Keep the parser here with the CIN helpers: the mapping +// is part of parsing a byte stream, and putting it in a host-side test means +// the hardware MIDI path is exercised too. +// +struct midi_stream_parser { + uint8_t bytes[3]; + int nr_bytes; + int message_len; + bool in_sysex; +}; + +static inline void midi_stream_reset(struct midi_stream_parser *parser) +{ + parser->nr_bytes = 0; + parser->message_len = 0; + parser->in_sysex = false; +} + +static inline void midi_stream_packet(uint8_t packet[4], uint8_t cin, + const uint8_t bytes[3], int nr_bytes) +{ + packet[0] = cin; + packet[1] = nr_bytes > 0 ? bytes[0] : 0; + packet[2] = nr_bytes > 1 ? bytes[1] : 0; + packet[3] = nr_bytes > 2 ? bytes[2] : 0; +} + +// +// Feed one byte of a raw MIDI stream. Return true when it completes one +// USB-MIDI packet in 'packet'. Real-time messages are emitted immediately +// without disturbing a partly received channel message or SysEx stream. +// System Reset is the exception: it also clears the parser state. +// +static inline bool midi_stream_read(struct midi_stream_parser *parser, + uint8_t b, uint8_t packet[4]) +{ + if (b >= 0xF8) { + uint8_t byte[] = { b, 0, 0 }; + midi_stream_packet(packet, 0x0F, byte, 1); + if (b == 0xFF) + midi_stream_reset(parser); + return true; + } + + if (parser->in_sysex) { + if (b == 0xF7) { + parser->bytes[parser->nr_bytes++] = b; + midi_stream_packet(packet, + (uint8_t)(0x04 + parser->nr_bytes), + parser->bytes, parser->nr_bytes); + parser->in_sysex = false; + parser->nr_bytes = 0; + return true; + } + if (b < 0x80) { + parser->bytes[parser->nr_bytes++] = b; + if (parser->nr_bytes != 3) + return false; + midi_stream_packet(packet, 0x04, parser->bytes, 3); + parser->nr_bytes = 0; + return true; + } + + // A non-real-time status byte abandons an unfinished SysEx. + parser->in_sysex = false; + parser->nr_bytes = 0; + } + + if (b < 0x80) { + if (!parser->message_len) + return false; + + parser->bytes[parser->nr_bytes++] = b; + if (parser->nr_bytes != parser->message_len) + return false; + + midi_stream_packet(packet, midi_status_cin(parser->bytes[0]), + parser->bytes, parser->nr_bytes); + + // Channel messages keep their status for MIDI running status. + if (parser->bytes[0] < 0xF0) + parser->nr_bytes = 1; + else + midi_stream_reset(parser); + return true; + } + + parser->nr_bytes = 0; + parser->message_len = 0; + if (b == 0xF0) { + parser->bytes[parser->nr_bytes++] = b; + parser->in_sysex = true; + return false; + } + + parser->bytes[parser->nr_bytes++] = b; + if (b < 0xF0) + parser->message_len = (b & 0xF0) == 0xC0 || + (b & 0xF0) == 0xD0 ? 2 : 3; + else if (b == 0xF1 || b == 0xF3) + parser->message_len = 2; + else if (b == 0xF2) + parser->message_len = 3; + else if (b >= 0xF4 && b <= 0xF7) + parser->message_len = 1; + else + return false; + + if (parser->message_len != 1) + return false; + + midi_stream_packet(packet, midi_status_cin(b), parser->bytes, 1); + midi_stream_reset(parser); + return true; +} + bool handle_midi_packet(const uint8_t packet[4]); void usb_midi_poll(void); bool usb_midi_write(const uint8_t packet[4]); diff --git a/Firmware/midi/uart.h b/Firmware/midi/uart.h index 5cc7001..557c0db 100644 --- a/Firmware/midi/uart.h +++ b/Firmware/midi/uart.h @@ -29,41 +29,14 @@ void uart_midi_write(const uint8_t packet[4]) bool uart_midi_read(uint8_t packet[4]) { #if MIDI_HW - static int expected_bytes = 0; - static uint8_t parser_packet[4]; - static int parser_idx = 0; + static struct midi_stream_parser parser; while (uart_rx_head != uart_rx_tail) { uint8_t b = uart_rx_buf[uart_rx_tail]; uart_rx_tail = (uart_rx_tail + 1) % UART_RX_BUF_SIZE; - if (b >= 0xF8) { - // Real-time message - continue; - } else if (b >= 0x80) { - parser_packet[1] = b; - parser_idx = 2; - if ((b & 0xF0) == 0xC0 || (b & 0xF0) == 0xD0) { - expected_bytes = 1; - } else if (b < 0xF0) { - expected_bytes = 2; - } else { - expected_bytes = 0; - } - } else if (expected_bytes > 0 && parser_idx > 0) { - parser_packet[parser_idx++] = b; - if (parser_idx - 2 == expected_bytes) { - // CIN 0 is reserved: a host is entitled to - // ignore it, and ours was emitting nothing - // else on this path. - packet[0] = midi_status_cin(parser_packet[1]); - packet[1] = parser_packet[1]; - packet[2] = parser_packet[2]; - packet[3] = parser_packet[3]; - parser_idx = 2; - return true; - } - } + if (midi_stream_read(&parser, b, packet)) + return true; } #endif return false; diff --git a/Validation/test-midi-cin.c b/Validation/test-midi-cin.c index 65b5c04..8ecf8b2 100644 --- a/Validation/test-midi-cin.c +++ b/Validation/test-midi-cin.c @@ -1,16 +1,17 @@ // -// Check the USB-MIDI Code Index Number tables in Software/midi.h. +// Check the USB-MIDI Code Index Number helpers and byte-stream parser in +// Firmware/midi/midi.h. // -// A lookup table is exactly the sort of thing that compiles cleanly and -// is wrong, and this one is only compiled at all when MIDI_HW is on - -// which it is not by default - so a mistake here would sit unnoticed. +// A lookup table or parser is exactly the sort of thing that compiles cleanly +// and is wrong. Keep host-side coverage for the firmware's hardware MIDI +// path so mistakes do not require a pedal and TRS adapter to find. // // Build and run with 'make test-midi-cin && ./test-midi-cin'. // #include #include #include -#include "midi.h" +#include "midi/midi.h" static int fails; static void chk(const char *what, int got, int want) @@ -21,6 +22,32 @@ static void chk(const char *what, int got, int want) } } +static void chk_packet(const char *what, const uint8_t packet[4], + uint8_t cin, uint8_t b1, uint8_t b2, uint8_t b3) +{ + if (packet[0] != cin || packet[1] != b1 || packet[2] != b2 || + packet[3] != b3) { + printf("FAIL %-28s got %02x %02x %02x %02x want %02x %02x %02x %02x\n", + what, packet[0], packet[1], packet[2], packet[3], + cin, b1, b2, b3); + fails++; + } +} + +static void chk_stream_packet(struct midi_stream_parser *parser, + uint8_t b, const char *what, uint8_t cin, uint8_t b1, uint8_t b2, + uint8_t b3) +{ + uint8_t packet[4]; + + if (!midi_stream_read(parser, b, packet)) { + printf("FAIL %-28s no packet\n", what); + fails++; + return; + } + chk_packet(what, packet, cin, b1, b2, b3); +} + int main(void) { // Channel voice: CIN is the top nibble, and the lengths are the @@ -69,6 +96,59 @@ int main(void) chk("reserved 0", midi_cin_length(0x0), 0); chk("reserved 1", midi_cin_length(0x1), 0); - printf(fails ? "%d failures\n" : "all CIN checks pass\n", fails); + // The DIN port receives a byte stream, not USB-MIDI packets. Check + // that the stream parser preserves running status, emits real-time + // messages in the middle of another message, and packs SysEx endings + // with the right number of bytes. + struct midi_stream_parser parser; + midi_stream_reset(&parser); + chk("note status", midi_stream_read(&parser, 0x90, (uint8_t[4]) { 0 }), false); + chk("note data 1", midi_stream_read(&parser, 0x3C, (uint8_t[4]) { 0 }), false); + chk_stream_packet(&parser, 0xF8, "clock between note bytes", 0x0F, 0xF8, 0, 0); + chk_stream_packet(&parser, 0x40, "note after clock", 0x09, 0x90, 0x3C, 0x40); + chk("running data 1", midi_stream_read(&parser, 0x3D, (uint8_t[4]) { 0 }), false); + chk_stream_packet(&parser, 0x41, "running note", 0x09, 0x90, 0x3D, 0x41); + + midi_stream_reset(&parser); + chk("program status", midi_stream_read(&parser, 0xC0, (uint8_t[4]) { 0 }), false); + chk_stream_packet(&parser, 0x05, "program change", 0x0C, 0xC0, 0x05, 0); + chk_stream_packet(&parser, 0x06, "running program", 0x0C, 0xC0, 0x06, 0); + + // System Reset is real-time, but unlike the other real-time messages + // it resets receivers to power-up state and therefore cancels both + // a message in progress and running status. + midi_stream_reset(&parser); + chk("reset note status", midi_stream_read(&parser, 0x90, (uint8_t[4]) { 0 }), false); + chk("reset note data 1", midi_stream_read(&parser, 0x3C, (uint8_t[4]) { 0 }), false); + chk_stream_packet(&parser, 0xFF, "system reset", 0x0F, 0xFF, 0, 0); + chk("data after reset", midi_stream_read(&parser, 0x40, (uint8_t[4]) { 0 }), false); + + midi_stream_reset(&parser); + chk("song position status", midi_stream_read(&parser, 0xF2, (uint8_t[4]) { 0 }), false); + chk("song position data 1", midi_stream_read(&parser, 0x01, (uint8_t[4]) { 0 }), false); + chk_stream_packet(&parser, 0x02, "song position", 0x03, 0xF2, 0x01, 0x02); + chk_stream_packet(&parser, 0xF6, "tune request", 0x05, 0xF6, 0, 0); + chk_stream_packet(&parser, 0xF7, "standalone EOX", 0x05, 0xF7, 0, 0); + + midi_stream_reset(&parser); + chk("sysex f0", midi_stream_read(&parser, 0xF0, (uint8_t[4]) { 0 }), false); + chk("sysex byte 1", midi_stream_read(&parser, 0x7D, (uint8_t[4]) { 0 }), false); + chk_stream_packet(&parser, 0xF8, "clock inside sysex", 0x0F, 0xF8, 0, 0); + chk_stream_packet(&parser, 0x03, "sysex start", 0x04, 0xF0, 0x7D, 0x03); + chk("sysex byte 2", midi_stream_read(&parser, 0x12, (uint8_t[4]) { 0 }), false); + chk_stream_packet(&parser, 0xF7, "sysex end 2", 0x06, 0x12, 0xF7, 0); + + midi_stream_reset(&parser); + chk("short sysex f0", midi_stream_read(&parser, 0xF0, (uint8_t[4]) { 0 }), false); + chk("short sysex byte", midi_stream_read(&parser, 0x7D, (uint8_t[4]) { 0 }), false); + chk_stream_packet(&parser, 0xF7, "sysex end 3", 0x07, 0xF0, 0x7D, 0xF7); + + midi_stream_reset(&parser); + chk("long sysex f0", midi_stream_read(&parser, 0xF0, (uint8_t[4]) { 0 }), false); + chk("long sysex byte 1", midi_stream_read(&parser, 0x01, (uint8_t[4]) { 0 }), false); + chk_stream_packet(&parser, 0x02, "long sysex start", 0x04, 0xF0, 0x01, 0x02); + chk_stream_packet(&parser, 0xF7, "sysex end 1", 0x05, 0xF7, 0, 0); + + printf(fails ? "%d failures\n" : "all MIDI checks pass\n", fails); return !!fails; }