fix midi tx fifo overflow cause data corruption

rename

examples/device/dynamic_configuration/src/main.c

@@ -172,7 +172,7 @@ void midi_task(void)
   // regardless of these being used or not. Therefore incoming traffic should be read
   // (possibly just discarded) to avoid the sender blocking in IO
   uint8_t packet[4];
-  while(tud_midi_available()) tud_midi_receive(packet);
+  while( tud_midi_available() ) tud_midi_packet_read(packet);
 
   // send note every 1000 ms
   if (board_millis() - start_ms < 286) return; // not enough time

examples/device/midi_test/src/main.c

@@ -132,7 +132,7 @@ void midi_task(void)
   // regardless of these being used or not. Therefore incoming traffic should be read
   // (possibly just discarded) to avoid the sender blocking in IO
   uint8_t packet[4];
-  while(tud_midi_available()) tud_midi_receive(packet);
+  while ( tud_midi_available() ) tud_midi_packet_read(packet);
 
   // send note every 1000 ms
   if (board_millis() - start_ms < 286) return; // not enough time
@@ -146,10 +146,12 @@ void midi_task(void)
   if (previous < 0) previous = sizeof(note_sequence) - 1;
 
   // Send Note On for current position at full velocity (127) on channel 1.
-  tud_midi_write24(cable_num, 0x90 | channel, note_sequence[note_pos], 127);
+  uint8_t note_on[3] = { 0x90 | channel, note_sequence[note_pos], 127 };
+  tud_midi_write(cable_num, note_on, 3);
 
   // Send Note Off for previous note.
-  tud_midi_write24(cable_num, 0x80 | channel, note_sequence[previous], 0);
+  uint8_t note_off[3] = { 0x80 | channel, note_sequence[previous], 0};
+  tud_midi_write(cable_num, note_off, 3);
 
   // Increment position
   note_pos++;

examples/make.mk

@@ -103,7 +103,7 @@ CFLAGS += \
 
 # Debugging/Optimization
 ifeq ($(DEBUG), 1)
-  CFLAGS += -Og
+  CFLAGS += -O0
 else
   CFLAGS += -Os
 endif

src/class/midi/midi.h

@@ -61,6 +61,51 @@ typedef enum
   MIDI_JACK_EXTERNAL = 0x02
 } midi_jack_type_t;
 
+typedef enum
+{
+  MIDI_CIN_MISC              = 0,
+  MIDI_CIN_CABLE_EVENT       = 1,
+  MIDI_CIN_SYSCOM_2BYTE      = 2, // 2 byte system common message e.g MTC, SongSelect
+  MIDI_CIN_SYSCOM_3BYTE      = 3, // 3 byte system common message e.g SPP
+  MIDI_CIN_SYSEX_START       = 4, // SysEx starts or continue
+  MIDI_CIN_SYSEX_END_1BYTE   = 5, // SysEx ends with 1 data, or 1 byte system common message
+  MIDI_CIN_SYSEX_END_2BYTE   = 6, // SysEx ends with 2 data
+  MIDI_CIN_SYSEX_END_3BYTE   = 7, // SysEx ends with 3 data
+  MIDI_CIN_NOTE_ON           = 8,
+  MIDI_CIN_NOTE_OFF          = 9,
+  MIDI_CIN_POLY_KEYPRESS     = 10,
+  MIDI_CIN_CONTROL_CHANGE    = 11,
+  MIDI_CIN_PROGRAM_CHANGE    = 12,
+  MIDI_CIN_CHANNEL_PRESSURE  = 13,
+  MIDI_CIN_PITCH_BEND_CHANGE = 14,
+  MIDI_CIN_1BYTE_DATA = 15
+} midi_code_index_number_t;
+
+// MIDI 1.0 status byte
+enum
+{
+  //------------- System Exclusive -------------//
+  MIDI_STATUS_SYSEX_START                    = 0xF0,
+  MIDI_STATUS_SYSEX_END                      = 0xF7,
+
+  //------------- System Common -------------//
+  MIDI_STATUS_SYSCOM_TIME_CODE_QUARTER_FRAME = 0xF1,
+  MIDI_STATUS_SYSCOM_SONG_POSITION_POINTER   = 0xF2,
+  MIDI_STATUS_SYSCOM_SONG_SELECT             = 0xF3,
+  // F4, F5 is undefined
+  MIDI_STATUS_SYSCOM_TUNE_REQUEST            = 0xF6,
+
+  //------------- System RealTime  -------------//
+  MIDI_STATUS_SYSREAL_TIMING_CLOCK           = 0xF8,
+  // 0xF9 is undefined
+  MIDI_STATUS_SYSREAL_START                  = 0xFA,
+  MIDI_STATUS_SYSREAL_CONTINUE               = 0xFB,
+  MIDI_STATUS_SYSREAL_STOP                   = 0xFC,
+  // 0xFD is undefined
+  MIDI_STATUS_SYSREAL_ACTIVE_SENSING         = 0xFE,
+  MIDI_STATUS_SYSREAL_SYSTEM_RESET           = 0xFF,
+};
+
 /// MIDI Interface Header Descriptor
 typedef struct TU_ATTR_PACKED
 {

src/class/midi/midi_device.c

@@ -38,6 +38,7 @@
 //--------------------------------------------------------------------+
 // MACRO CONSTANT TYPEDEF
 //--------------------------------------------------------------------+
+
 typedef struct
 {
   uint8_t itf_num;
@@ -126,16 +127,19 @@ uint32_t tud_midi_n_read(uint8_t itf, uint8_t cable_num, void* buffer, uint32_t
   midid_interface_t* midi = &_midid_itf[itf];
 
   // Fill empty buffer
-  if (midi->read_buffer_length == 0) {
+  if ( midi->read_buffer_length == 0 )
-    if (!tud_midi_n_packet_read(itf, midi->read_buffer)) return 0;
+  {
+    if ( !tud_midi_n_packet_read(itf, midi->read_buffer) ) return 0;
 
     uint8_t code_index = midi->read_buffer[0] & 0x0f;
     // We always copy over the first byte.
     uint8_t count = 1;
     // Ignore subsequent bytes based on the code.
-    if (code_index != 0x5 && code_index != 0xf) {
+    if ( code_index != 0x5 && code_index != 0xf )
+    {
       count = 2;
-      if (code_index != 0x2 && code_index != 0x6 && code_index != 0xc && code_index != 0xd) {
+      if ( code_index != 0x2 && code_index != 0x6 && code_index != 0xc && code_index != 0xd )
+      {
         count = 3;
       }
     }
@@ -150,7 +154,8 @@ uint32_t tud_midi_n_read(uint8_t itf, uint8_t cable_num, void* buffer, uint32_t
   memcpy(buffer, midi->read_buffer + 1 + midi->read_target_length, n);
   midi->read_target_length += n;
 
-  if (midi->read_target_length == midi->read_buffer_length) {
+  if ( midi->read_target_length == midi->read_buffer_length )
+  {
     midi->read_buffer_length = 0;
     midi->read_target_length = 0;
   }
@@ -166,10 +171,6 @@ bool tud_midi_n_packet_read (uint8_t itf, uint8_t packet[4])
   return (num_read == 4);
 }
 
-void midi_rx_done_cb(midid_interface_t* midi, uint8_t const* buffer, uint32_t bufsize) {
-  tu_fifo_write_n(&midi->rx_ff, buffer, bufsize);
-}
-
 //--------------------------------------------------------------------+
 // WRITE API
 //--------------------------------------------------------------------+
@@ -185,7 +186,8 @@ static uint32_t write_flush(midid_interface_t* midi)
   TU_VERIFY( usbd_edpt_claim(rhport, midi->ep_in), 0 );
 
   uint16_t count = tu_fifo_read_n(&midi->tx_ff, midi->epin_buf, CFG_TUD_MIDI_EP_BUFSIZE);
-  if (count > 0)
+
+  if (count)
   {
     TU_ASSERT( usbd_edpt_xfer(rhport, midi->ep_in, midi->epin_buf, count), 0 );
     return count;
@@ -202,21 +204,26 @@ uint32_t tud_midi_n_write(uint8_t itf, uint8_t cable_num, uint8_t const* buffer,
   midid_interface_t* midi = &_midid_itf[itf];
   TU_VERIFY(midi->itf_num, 0);
 
+  uint32_t written = 0;
   uint32_t i = 0;
   while ( i < bufsize )
   {
-    uint8_t data = buffer[i];
+    uint8_t const data = buffer[i];
+
     if ( midi->write_buffer_length == 0 )
     {
-      uint8_t msg = data >> 4;
+      // new packet
+
+      uint8_t const msg = data >> 4;
       midi->write_buffer[1] = data;
       midi->write_buffer_length = 2;
+
       // Check to see if we're still in a SysEx transmit.
-      if ( midi->write_buffer[0] == 0x4 )
+      if ( midi->write_buffer[0] == MIDI_CIN_SYSEX_START )
       {
-        if ( data == 0xf7 )
+        if ( data == MIDI_STATUS_SYSEX_END )
         {
-          midi->write_buffer[0] = 0x5;
+          midi->write_buffer[0] = MIDI_CIN_SYSEX_END_1BYTE;
           midi->write_target_length = 2;
         }
         else
@@ -226,29 +233,31 @@ uint32_t tud_midi_n_write(uint8_t itf, uint8_t cable_num, uint8_t const* buffer,
       }
       else if ( (msg >= 0x8 && msg <= 0xB) || msg == 0xE )
       {
-        midi->write_buffer[0] = cable_num << 4 | msg;
+        // Channel Voice Messages
+        midi->write_buffer[0] = (cable_num << 4) | msg;
         midi->write_target_length = 4;
       }
       else if ( msg == 0xf )
       {
-        if ( data == 0xf0 )
+        // System message
+        if ( data == MIDI_STATUS_SYSEX_START )
         {
-          midi->write_buffer[0] = 0x4;
+          midi->write_buffer[0] = MIDI_CIN_SYSEX_START;
           midi->write_target_length = 4;
         }
-        else if ( data == 0xf1 || data == 0xf3 )
+        else if ( data == MIDI_STATUS_SYSCOM_TIME_CODE_QUARTER_FRAME || data == MIDI_STATUS_SYSCOM_SONG_SELECT )
         {
-          midi->write_buffer[0] = 0x2;
+          midi->write_buffer[0] = MIDI_CIN_SYSCOM_2BYTE;
           midi->write_target_length = 3;
         }
-        else if ( data == 0xf2 )
+        else if ( data == MIDI_STATUS_SYSCOM_SONG_POSITION_POINTER )
         {
-          midi->write_buffer[0] = 0x3;
+          midi->write_buffer[0] = MIDI_CIN_SYSCOM_3BYTE;
           midi->write_target_length = 4;
         }
         else
         {
-          midi->write_buffer[0] = 0x5;
+          midi->write_buffer[0] = MIDI_CIN_SYSEX_END_1BYTE;
           midi->write_target_length = 2;
         }
       }
@@ -264,33 +273,44 @@ uint32_t tud_midi_n_write(uint8_t itf, uint8_t cable_num, uint8_t const* buffer,
     }
     else
     {
+      // On-going packet
+
       TU_ASSERT(midi->write_buffer_length < 4, 0);
       midi->write_buffer[midi->write_buffer_length] = data;
-      midi->write_buffer_length += 1;
+      midi->write_buffer_length++;
+
       // See if this byte ends a SysEx.
-      if ( midi->write_buffer[0] == 0x4 && data == 0xf7 )
+      if ( midi->write_buffer[0] == MIDI_CIN_SYSEX_START && data == MIDI_STATUS_SYSEX_END )
       {
-        midi->write_buffer[0] = 0x4 + (midi->write_buffer_length - 1);
+        midi->write_buffer[0] = MIDI_CIN_SYSEX_START + (midi->write_buffer_length - 1);
         midi->write_target_length = midi->write_buffer_length;
       }
     }
 
+    // Send out packet
     if ( midi->write_buffer_length == midi->write_target_length )
     {
-      uint16_t written = tu_fifo_write_n(&midi->tx_ff, midi->write_buffer, 4);
+      // zeroes unused bytes
-      if ( written < 4 )
+      for(uint8_t idx = midi->write_target_length; idx < 4; idx++) midi->write_buffer[idx] = 0;
-      {
+
-        TU_ASSERT(written == 0);
+      uint16_t const count = tu_fifo_write_n(&midi->tx_ff, midi->write_buffer, 4);
-        break;
+
-      }
+      // reset buffer
-      midi->write_buffer_length = 0;
+      midi->write_buffer_length = midi->write_target_length = 0;
+
+      // fifo overflow, here we assume FIFO is multiple of 4 and didn't check remaining before writing
+      if ( count != 4 ) break;
+
+      // updated written if succeeded
+      written = i;
     }
+
     i++;
   }
 
   write_flush(midi);
 
-  return i;
+  return written;
 }
 
 bool tud_midi_n_packet_write (uint8_t itf, uint8_t const packet[4])
@@ -300,8 +320,7 @@ bool tud_midi_n_packet_write (uint8_t itf, uint8_t const packet[4])
     return 0;
   }
 
-  if (tu_fifo_remaining(&midi->tx_ff) < 4)
+  if (tu_fifo_remaining(&midi->tx_ff) < 4) return false;
-    return false;
 
   tu_fifo_write_n(&midi->tx_ff, packet, 4);
   write_flush(midi);
@@ -347,9 +366,9 @@ void midid_reset(uint8_t rhport)
 uint16_t midid_open(uint8_t rhport, tusb_desc_interface_t const * desc_itf, uint16_t max_len)
 {
   // 1st Interface is Audio Control v1
-  TU_VERIFY(TUSB_CLASS_AUDIO                      == desc_itf->bInterfaceClass    &&
+  TU_VERIFY(TUSB_CLASS_AUDIO               == desc_itf->bInterfaceClass    &&
-            AUDIO_SUBCLASS_CONTROL                == desc_itf->bInterfaceSubClass &&
+            AUDIO_SUBCLASS_CONTROL         == desc_itf->bInterfaceSubClass &&
-            AUDIO_FUNC_PROTOCOL_CODE_UNDEF        == desc_itf->bInterfaceProtocol, 0);
+            AUDIO_FUNC_PROTOCOL_CODE_UNDEF == desc_itf->bInterfaceProtocol, 0);
 
   uint16_t drv_len = tu_desc_len(desc_itf);
   uint8_t const * p_desc = tu_desc_next(desc_itf);
@@ -365,9 +384,9 @@ uint16_t midid_open(uint8_t rhport, tusb_desc_interface_t const * desc_itf, uint
   TU_VERIFY(TUSB_DESC_INTERFACE == tu_desc_type(p_desc), 0);
   tusb_desc_interface_t const * desc_midi = (tusb_desc_interface_t const *) p_desc;
 
-  TU_VERIFY(TUSB_CLASS_AUDIO                      == desc_midi->bInterfaceClass    &&
+  TU_VERIFY(TUSB_CLASS_AUDIO               == desc_midi->bInterfaceClass    &&
-            AUDIO_SUBCLASS_MIDI_STREAMING         == desc_midi->bInterfaceSubClass &&
+            AUDIO_SUBCLASS_MIDI_STREAMING  == desc_midi->bInterfaceSubClass &&
-            AUDIO_FUNC_PROTOCOL_CODE_UNDEF        == desc_midi->bInterfaceProtocol, 0);
+            AUDIO_FUNC_PROTOCOL_CODE_UNDEF == desc_midi->bInterfaceProtocol, 0);
 
   // Find available interface
   midid_interface_t * p_midi = NULL;

src/class/midi/midi_device.h

@@ -59,16 +59,28 @@
 // Application API (Multiple Interfaces)
 // CFG_TUD_MIDI > 1
 //--------------------------------------------------------------------+
+
+// Check if midi interface is mounted
 bool     tud_midi_n_mounted    (uint8_t itf);
+
+// Get the number of bytes available for reading
 uint32_t tud_midi_n_available  (uint8_t itf, uint8_t cable_num);
+
+// Read byte stream (legacy)
 uint32_t tud_midi_n_read       (uint8_t itf, uint8_t cable_num, void* buffer, uint32_t bufsize);
+
+// Write byte Stream (legacy)
 uint32_t tud_midi_n_write      (uint8_t itf, uint8_t cable_num, uint8_t const* buffer, uint32_t bufsize);
 
+// Write good-old 3 bytes data, this use write packet
 static inline
 uint32_t tud_midi_n_write24    (uint8_t itf, uint8_t cable_num, uint8_t b1, uint8_t b2, uint8_t b3);
 
-bool tud_midi_n_packet_read    (uint8_t itf, uint8_t packet[4]);
+// Read event packet (4 bytes)
-bool tud_midi_n_packet_write   (uint8_t itf, uint8_t const packet[4]);
+bool     tud_midi_n_packet_read    (uint8_t itf, uint8_t packet[4]);
+
+// Write event packet (4 bytes)
+bool     tud_midi_n_packet_write   (uint8_t itf, uint8_t const packet[4]);
 
 //--------------------------------------------------------------------+
 // Application API (Single Interface)