Merge pull request #381 from ed-xmos/feature/midi_exception_fix

Test and fix the exception from FIFO when invalid packet sent to Tx

lib_xua/src/midi/midioutparse.h

@@ -1,8 +1,14 @@
-// Copyright 2011-2021 XMOS LIMITED.
+// Copyright 2011-2024 XMOS LIMITED.
 // This Software is subject to the terms of the XMOS Public Licence: Version 1.
 #ifndef MIDIOUTPARSE_XH
 #define MIDIOUTPARSE_XH
 
+// If for any reason we pop a message when not needed (should never happen) this will cause midiparse out to send a size of 0 (drops packet)
+#define MIDI_OUT_NULL_MESSAGE   0x00000000
+
+#ifdef __XC__
+// Takes a MIDI packet and decomoses it into up to 3 data bytes followed by a byte count.
 {unsigned, unsigned, unsigned, unsigned} midi_out_parse(unsigned event);
+#endif
 
 #endif

lib_xua/src/midi/queue.h

@@ -3,7 +3,9 @@
 #ifndef QUEUE_H_
 #define QUEUE_H_
 
-#define assert(x) asm("ecallf %0"::"r"(x));
+#include "midioutparse.h"
+#include "xassert.h"
+
 
 typedef struct queue_t {
     /// Read index.
@@ -21,7 +23,7 @@ inline int is_power_of_2(unsigned x) {
 }
 
 inline void queue_init(queue_t &q, unsigned size) {
-    assert(is_power_of_2(size));
+    xassert(is_power_of_2(size) && "MIDI FIFO size must be a power of 2"); // Keep this enabled as will be discovered duirng dev time
     q.rdptr = 0;
     q.wrptr = 0;
     q.size = size;
@@ -38,25 +40,26 @@ inline int queue_is_full(const queue_t &q) {
 
 inline void queue_push_word(queue_t &q, unsigned array[], unsigned data)
 {
-    assert(!queue_is_full(q));
+
+    if(queue_is_full(q)) {
+        xassert(0 && "Unexpected push to MIDI queue when full");
+        // Silently drop message if asserts not enabled
+        return;
+    }
+
     array[q.wrptr++ & q.mask] = data;
 }
 
 inline unsigned queue_pop_word(queue_t &q, unsigned array[]) {
-    assert(!queue_is_empty(q));
+    if(queue_is_empty(q)){
+        xassert(0 && "Unexpected pop from MIDI queue when empty");
+        // Return NULL messaqe if asserts not enabled
+        return MIDI_OUT_NULL_MESSAGE; 
+    }
+
     return array[q.rdptr++ & q.mask];
 }
 
-inline void queue_push_byte(queue_t &q, unsigned char array[], unsigned data)
-{
-    assert(!queue_is_full(q));
-    array[q.wrptr++ & q.mask] = data;
-}
-
-inline unsigned queue_pop_byte(queue_t &q, unsigned char array[]) {
-    assert(!queue_is_empty(q));
-    return array[q.rdptr++ & q.mask];
-}
 
 inline unsigned queue_items(const queue_t &q) {
     return q.wrptr - q.rdptr;

lib_xua/src/midi/queue.xc

@@ -1,4 +1,4 @@
-// Copyright 2013-2021 XMOS LIMITED.
+// Copyright 2013-2024 XMOS LIMITED.
 // This Software is subject to the terms of the XMOS Public Licence: Version 1.
 #include "queue.h"
 
@@ -9,7 +9,5 @@ extern inline int queue_is_empty(const queue_t &q);
 extern inline int queue_is_full(const queue_t &q);
 extern inline void queue_push_word(queue_t &q, unsigned array[], unsigned data);
 extern inline unsigned queue_pop_word(queue_t &q, unsigned array[]);
-extern inline void queue_push_byte(queue_t &q, unsigned char array[], unsigned data);
-extern inline unsigned queue_pop_byte(queue_t &q, unsigned char array[]);
 extern inline unsigned queue_space(const queue_t &q);
 extern inline unsigned queue_items(const queue_t &q);

lib_xua/src/midi/usb_midi.xc

@@ -1,4 +1,4 @@
-// Copyright 2011-2022 XMOS LIMITED.
+// Copyright 2011-2024 XMOS LIMITED.
 // This Software is subject to the terms of the XMOS Public Licence: Version 1.
 #include <xs1.h>
 #include <xclib.h>
@@ -13,7 +13,7 @@
 #include "iap_user.h"
 #include "coprocessor_user.h"
 #endif
-//#define MIDI_LOOPBACK 1
+
 int icount = 0;
 static unsigned makeSymbol(unsigned data)
 {
@@ -89,9 +89,9 @@ void usb_midi(
 
     struct midi_in_parse_state mips;
 
-    // the symbol fifo (to go out of uart)
+    // the symbol fifo (to go out of uart).
     queue_t symbol_fifo;
-    unsigned symbol_fifo_arr[USB_MIDI_DEVICE_OUT_FIFO_SIZE]; // Used for 32bit USB MIDI events
+    unsigned symbol_fifo_arr[USB_MIDI_DEVICE_OUT_FIFO_SIZE]; // Used for outgoing UART symbols (which include the start and stop bit)
 
     unsigned rxPT, txPT;
     int midi_from_host_overflow = 0;
@@ -264,7 +264,7 @@ void usb_midi(
             }
             break;
 #endif
-
+        // Received as packet from USB
         case !authenticating => midi_get_ack_or_data(c_midi, is_ack, datum):
 
             if (is_ack)
@@ -281,6 +281,7 @@ void usb_midi(
                 }
             }
             else
+            // A midi packet from the host
             {
                 unsigned midi[3];
                 unsigned size;
@@ -327,7 +328,7 @@ void usb_midi(
                     midi_from_host_overflow = 1;
                 }
                 // Drop through to the isTX guarded case
-                if (!isTX)
+                if (!isTX && size > 0) // do not start tx'ing if this packet has no size
                 {
                     t :> txT; // Should be enough to trigger the other case
                     isTX = 1;

tests/test_midi/src/app_midi_simple.xc

@@ -134,7 +134,9 @@ void test(chanend c_midi){
                 } else {
                     unsigned midi_data[3] = {0};
                     unsigned byte_count = 0;
+                    // Even though this is an Rx from MIDI we use midi_out_parse so we can decode the packet
                     {midi_data[0], midi_data[1], midi_data[2], byte_count} = midi_out_parse(byterev(rx_packet));
+                    dprintf("Got packet from MIDI: 0x%8x\n", rx_packet);
                     // Note this needs to always print for capfd in pytest to pick it up
                     printf("dut_midi_rx: %u %u %u\n", midi_data[0], midi_data[1], midi_data[2]);
                     rx_cmd_count++;
@@ -144,9 +146,10 @@ void test(chanend c_midi){
 
             case tx_cmd_count < num_to_tx => tmr when timerafter(t_tx) :> int _:
                 unsigned midi[] = {commands[tx_cmd_count][0], commands[tx_cmd_count][1], commands[tx_cmd_count][2]};
+                // Even though this is a Tx to MIDI we use midi_in_parse_helper to form the packet from bytes
                 unsigned tx_packet = midi_in_parse_helper(midi, m_state);
                 outuint(c_midi, byterev(tx_packet));
-                dprintf("Sent packet to midi: %u %u %u\n", commands[tx_cmd_count][0], commands[tx_cmd_count][1], commands[tx_cmd_count][2]);
+                dprintf("Sent packet to midi: %u %u %u (0x%8x)\n", commands[tx_cmd_count][0], commands[tx_cmd_count][1], commands[tx_cmd_count][2], tx_packet);
                 t_tx += tx_interval;
                 tx_end += max_tx_time;
             break;

tests/test_midi_tx.py

@@ -23,18 +23,12 @@ def test_tx(capfd, config, build_midi):
         copy_tree(build_midi, tmpdirname)
         xe = str(Path(tmpdirname) / f"{config}/test_midi_{config}.xe")
 
-        # midi_commands = [[0x90, 0x91, 0x90],# Invalid and should be discarded
+        midi_commands = [[0x90, 0x91, 0x90],# Invalid and should be discarded
-        #                 [0x90, 60, 81],     # Note on
-        #                 [0x80, 60, 81]]     # Note off
-
-        midi_commands = [
                         [0x90, 60, 81],     # Note on
                         [0x80, 60, 81]]     # Note off
 
-
+        # Make a 1D list from the 2D list [1:] because first is invalid and we expect to skip it
-        # midi_command_expected = midi_commands[1:] # should skip invalid first message
+        midi_command_expected = [[item for row in midi_commands[1:] for item in row]]
-        # Make a 1D list from the 2D list
-        midi_command_expected = [[item for row in midi_commands for item in row]]
 
         create_midi_tx_file(midi_commands)
         create_midi_rx_file()
@@ -47,14 +41,14 @@ def test_tx(capfd, config, build_midi):
         bpb = 8
         parity = 0 
         stop = 1
-        length_of_test = sum(len(cmd) for cmd in midi_commands)
+        length_of_test = sum(len(cmd) for cmd in midi_command_expected)
 
         simthreads = [
             UARTTxChecker(tx_port, parity, baud, length_of_test, stop, bpb, debug=False)
         ]
 
 
-        simargs = ["--max-cycles", str(MAX_CYCLES), "-o", "trace.txt"]
+        simargs = ["--max-cycles", str(MAX_CYCLES), "--trace-to", "trace.txt"]
         #This is just for local debug so we can capture the traces if needed. It slows xsim down so not needed
         # simargs.extend(["--vcd-tracing", "-tile tile[1] -ports -o trace.vcd"]) 
 
@@ -65,6 +59,7 @@ def test_tx(capfd, config, build_midi):
             timeout=120,
             simargs=simargs,   
         )
+
         capture = capfd.readouterr().out
         result = tester.run(capture.split("\n"))
 

tests/uart_tx_checker.py

@@ -94,7 +94,9 @@ class UARTTxChecker(px.SimThread):
         if self.debug: print("tx starts high: %s" % ("True" if initial_port_val else "False"))
 
         for x in range(length):
-            packet.append(chr(self.read_byte(xsi, parity)))
+            byte = self.read_byte(xsi, parity)
+            if self.debug: print(f"Checker got byte: {byte}")
+            packet.append(chr(byte))
         return packet
 
     def read_byte(self, xsi, parity):

tests/xua_unit_tests/CMakeLists.txt

@@ -69,6 +69,7 @@ foreach(TESTFILE ${TEST_SOURCES})
                                                 -DUNITY_INCLUDE_DOUBLE
                                                 -DXUD_CORE_CLOCK=600
                                                 -DXUD_SERIES_SUPPORT=4
+                                                -DXASSERT_ENABLE_ASSERTIONS=0
                                                 )
 
     # For HID tests only enable HID

tests/xua_unit_tests/src/test_midi_queue/test_midi_queue.c

@@ -2,6 +2,7 @@
 // This Software is subject to the terms of the XMOS Public Licence: Version 1.
 #include <stddef.h>
 #include <stdio.h>
+#include <string.h>
 
 #include "xua_unit_tests.h"
 #include "../../../lib_xua/src/midi/queue.h"
@@ -25,6 +26,8 @@ unsigned rndm = RANDOM_SEED;
 void test_midi_queue_init(void) {
     queue_t symbol_fifo;
     unsigned symbol_fifo_storage[USB_MIDI_DEVICE_OUT_FIFO_SIZE];
+    memset(symbol_fifo_storage, USB_MIDI_DEVICE_OUT_FIFO_SIZE * sizeof(unsigned), 0xdb); // Non zero
+
     queue_init_c_wrapper(&symbol_fifo, ARRAY_SIZE(symbol_fifo_storage));
 
     int empty = queue_is_empty_c_wrapper(&symbol_fifo);
@@ -38,6 +41,13 @@ void test_midi_queue_init(void) {
 
     unsigned space = queue_space_c_wrapper(&symbol_fifo);
     TEST_ASSERT_EQUAL_UINT32(USB_MIDI_DEVICE_OUT_FIFO_SIZE, space);
+
+    // Pop empty queue
+    unsigned entry = queue_pop_word_c_wrapper(&symbol_fifo, symbol_fifo_storage);
+    TEST_ASSERT_EQUAL_UINT32(MIDI_OUT_NULL_MESSAGE, entry);
+
+    space = queue_space_c_wrapper(&symbol_fifo);
+    TEST_ASSERT_EQUAL_UINT32(USB_MIDI_DEVICE_OUT_FIFO_SIZE, space);
 }
 
 void test_midi_queue_full(void) {
@@ -46,7 +56,7 @@ void test_midi_queue_full(void) {
     queue_init_c_wrapper(&symbol_fifo, ARRAY_SIZE(symbol_fifo_storage));
 
     for(unsigned i = 0; i < USB_MIDI_DEVICE_OUT_FIFO_SIZE; i++){
-        queue_push_word_c_wrapper(&symbol_fifo, symbol_fifo_storage, 0);
+        queue_push_word_c_wrapper(&symbol_fifo, symbol_fifo_storage, 1111);
     }
 
     int empty = queue_is_empty_c_wrapper(&symbol_fifo);
@@ -60,6 +70,12 @@ void test_midi_queue_full(void) {
 
     unsigned space = queue_space_c_wrapper(&symbol_fifo);
     TEST_ASSERT_EQUAL_UINT32(0, space);
+
+    // We want no exception here and this to be ignored
+    queue_push_word_c_wrapper(&symbol_fifo, symbol_fifo_storage, 2222);
+
+    unsigned entry = queue_pop_word_c_wrapper(&symbol_fifo, symbol_fifo_storage);
+    TEST_ASSERT_EQUAL_UINT32(1111, entry);
 }
 
 void test_midi_queue_push_pop(void) {

tests/xua_unit_tests/src/xua_unit_test_helper.xc

@@ -89,18 +89,6 @@ unsigned queue_pop_word_c_wrapper(queue_t *q, unsigned array[]){
     }
 }
 
-void queue_push_byte_c_wrapper(queue_t *q, unsigned char array[], unsigned data){
-    unsafe{
-        queue_push_byte(*q, array, data);
-    }
-}
-
-unsigned queue_pop_byte_c_wrapper(queue_t *q, unsigned char array[]){
-    unsafe{
-        return queue_pop_byte(*q, array);
-    }
-}
-
 unsigned queue_items_c_wrapper(const queue_t *q){
     unsafe{
         return queue_items(*q);