Harden UART/DMA recovery and embedded input processing

CMakeLists.txt

@@ -86,8 +86,8 @@ pico_set_program_version(PIOKMbox "0.1")
 # Modify the below lines to enable/disable output over UART/USB
 # UART0 is used for KMBox serial input via RP2350 USB Bridge
 # USB CDC is disabled to avoid breaking HID enumeration
-# Debug output can be sent through the KMBox interface if needed
-pico_enable_stdio_uart(PIOKMbox 1)
+# The protocol UART is owned by the response queue; debug stdio must not write it.
+pico_enable_stdio_uart(PIOKMbox 0)
 pico_enable_stdio_usb(PIOKMbox 0)
 
 # Optimization flags for release performance at 240MHz

README.md

@@ -472,6 +472,56 @@ RaspberryKMBox/
 
 Append `clean` to force a rebuild: `./build.sh all clean`
 
+### Host Regression Checks
+
+These checks need Python 3 and a C compiler with address and undefined-behavior
+sanitizers. They run production functions against mocked peripherals; they do
+not replace firmware builds or testing on the boards.
+
+```bash
+python3 tests/test_xbox_transfers.py
+python3 tests/test_bridge_parsers.py
+python3 tests/test_uart_timing.py
+python3 tests/test_display_io.py
+python3 tests/test_bridge_protocol.py
+python3 tests/test_proxy_tx.py
+python3 tests/test_proxy_rx.py
+python3 tests/test_dma_rx.py
+```
+
+Build and flash the bridge and proxy from the same revision. Both bridge targets
+use eight-byte fast packets. Humanization configuration uses opcode `0x10`, and
+info requests use `0x11` to avoid the carriage-return opcode `0x0D`; info replies
+retain `0x0D`. The variable-length wire-protocol library is not the proxy's UART
+parser and must not be mixed into that stream.
+
+The default software TFT path advances refreshes one DMA row per main-loop pass.
+Rendering runs in the main loop, while the timer interrupt only schedules it.
+The optional `TFT_HW_ACCEL` path and explicit splash/error refreshes remain
+synchronous.
+
+The proxy's protocol UART has stdio disabled. Command replies use an explicit
+output callback into a shared record queue; debug `printf` calls do not write
+onto that UART. The queue supports seven pending records of up to 256 bytes,
+including any CRLF suffix. A full queue rejects whole records and accounts for
+their bytes in `kmbox_get_tx_dropped_bytes()`. Startup and DMA-allocation failure
+use polled RX and IRQ TX, preserving pending data when DMA starts.
+
+DMA RX on both boards uses RP2350 self-trigger mode with a decrementing count,
+so complete ring-buffer laps cannot appear empty. Copies are checked against a
+second coherent count/address snapshot before delivery. A loss discards pending
+input and partial parser state; the proxy also releases injected controls and
+cancels queued/timed movement. Reception resumes after at least 1 ms without DMA
+progress and an empty UART FIFO. The sender must provide that gap at a packet
+boundary; continuous traffic is discarded during recovery.
+
+The bridge exposes overrun events through `hw_uart_get_stats()` and discarded
+bytes/ambiguous polls through `hw_uart_get_rx_loss_stats()`. The proxy exposes
+all three through `kmbox_get_rx_loss_stats()` on Core 0. If a scheduling pause
+could hide an entire hardware count period, reception fails closed and the byte
+count is a lower bound. These counters describe discarded data, not retransmission
+or flow control. No additional DMA channels or RX interrupts are required.
+
 ### Build Configuration
 
 Presets defined in `defines.h`:

bridge/core1_translator.c

@@ -14,51 +14,53 @@
 static queued_makcu_frame_t makcu_queue[MAKCU_FRAME_QUEUE_SIZE] __attribute__((aligned(4)));
 static queued_kmbox_cmd_t kmbox_queue[KMBOX_CMD_QUEUE_SIZE] __attribute__((aligned(4)));
 
-// Ring buffer indices (volatile for inter-core visibility)
-static volatile uint8_t makcu_head = 0;
-static volatile uint8_t makcu_tail = 0;
-static volatile uint8_t kmbox_head = 0;
-static volatile uint8_t kmbox_tail = 0;
+// SPSC indices: acquire/release orders slot data and prevents early slot reuse.
+static uint8_t makcu_head = 0;
+static uint8_t makcu_tail = 0;
+static uint8_t kmbox_head = 0;
+static uint8_t kmbox_tail = 0;
 
 // Statistics
 static volatile uint32_t frames_queued = 0;
 static volatile uint32_t cmds_produced = 0;
 static volatile uint32_t queue_overflows = 0;
 
-// Core1 main translation loop
+// Only fixed-size UART commands belong in this queue. Text responses (such
+// as VERSION) must not be copied into packet slots or forwarded to the UART.
+static bool queue_translated_packet(const translated_cmd_t *result) {
+    if (result->result != TRANSLATE_OK ||
+        result->length != sizeof(kmbox_queue[0].packet)) return false;
+    uint8_t head = __atomic_load_n(&kmbox_head, __ATOMIC_RELAXED);
+    uint8_t next = (head + 1) & (KMBOX_CMD_QUEUE_SIZE - 1);
+    if (next == __atomic_load_n(&kmbox_tail, __ATOMIC_ACQUIRE)) {
+        __atomic_add_fetch(&queue_overflows, 1, __ATOMIC_RELAXED);
+        return false;
+    }
+    memcpy(kmbox_queue[head].packet, result->buffer, result->length);
+    __atomic_store_n(&kmbox_head, next, __ATOMIC_RELEASE);
+    __atomic_add_fetch(&cmds_produced, 1, __ATOMIC_RELAXED);
+    return true;
+}
+
+// Consume one input frame. Kept separate from idle waiting so wake-ups always
+// recheck the queue, including events received just before WFE.
+static bool translate_one_frame(void) {
+    uint8_t tail = __atomic_load_n(&makcu_tail, __ATOMIC_RELAXED);
+    uint8_t head = __atomic_load_n(&makcu_head, __ATOMIC_ACQUIRE);
+    if (head == tail) return false;
+
+    queued_makcu_frame_t *frame = &makcu_queue[tail];
+    translated_cmd_t result;
+    makcu_translate_command(frame->cmd, frame->payload, frame->payload_len, &result);
+    queue_translated_packet(&result);
+    __atomic_store_n(&makcu_tail, (tail + 1) & (MAKCU_FRAME_QUEUE_SIZE - 1), __ATOMIC_RELEASE);
+    return true;
+}
+
 static void __not_in_flash_func(core1_main)(void) {
-    // Run translation loop at max speed
     while (1) {
-        // Check for Makcu frames to translate
-        uint8_t head = makcu_head;
-        uint8_t tail = makcu_tail;
-
-        if (head != tail) {
-            // Get frame from queue
-            queued_makcu_frame_t* frame = &makcu_queue[tail & (MAKCU_FRAME_QUEUE_SIZE - 1)];
-
-            // Translate to KMBox command(s)
-            translated_cmd_t result;
-            makcu_translate_command(frame->cmd, frame->payload, frame->payload_len, &result);
-
-            // Queue translated wire protocol packet (max 7 bytes, fits in 8-byte slot)
-            if (result.result == TRANSLATE_OK && result.length > 0) {
-                uint8_t next_kmbox_head = (kmbox_head + 1) & (KMBOX_CMD_QUEUE_SIZE - 1);
-
-                if (next_kmbox_head != kmbox_tail) {
-                    memcpy(kmbox_queue[kmbox_head].packet, result.buffer, result.length);
-                    kmbox_head = next_kmbox_head;
-                    __atomic_add_fetch(&cmds_produced, 1, __ATOMIC_RELAXED);
-                } else {
-                    __atomic_add_fetch(&queue_overflows, 1, __ATOMIC_RELAXED);
-                }
-            }
-
-            // Advance tail (consume frame)
-            makcu_tail = (tail + 1) & (MAKCU_FRAME_QUEUE_SIZE - 1);
-        } else {
-            // No work - yield to save power
-            __wfi();  // Wait for interrupt (low power idle)
+        if (!translate_one_frame()) {
+            __wfe();  // Matches the producer's SEV; a pending event prevents sleep.
         }
     }
 }
@@ -83,47 +85,46 @@ void core1_translator_init(void) {
 
 // Queue Makcu frame for translation
 bool core1_queue_makcu_frame(uint8_t cmd, const uint8_t* payload, uint16_t len) {
-    if (len > 256) {
+    if (len > 256 || (len > 0 && !payload)) {
         return false;  // Payload too large
     }
 
-    uint8_t next_head = (makcu_head + 1) & (MAKCU_FRAME_QUEUE_SIZE - 1);
+    uint8_t head = __atomic_load_n(&makcu_head, __ATOMIC_RELAXED);
+    uint8_t next_head = (head + 1) & (MAKCU_FRAME_QUEUE_SIZE - 1);
 
-    if (next_head == makcu_tail) {
+    if (next_head == __atomic_load_n(&makcu_tail, __ATOMIC_ACQUIRE)) {
         __atomic_add_fetch(&queue_overflows, 1, __ATOMIC_RELAXED);
         return false;  // Queue full
     }
 
     // Copy frame to queue
-    queued_makcu_frame_t* frame = &makcu_queue[makcu_head];
+    queued_makcu_frame_t* frame = &makcu_queue[head];
     frame->cmd = cmd;
     frame->payload_len = len;
     if (payload && len > 0) {
         memcpy(frame->payload, payload, len);
     }
 
-    // Fix #4: Data memory barrier - ensure all writes complete before publishing index
-    __dmb();
-
-    // Advance head (publish frame)
-    makcu_head = next_head;
+    __atomic_store_n(&makcu_head, next_head, __ATOMIC_RELEASE);
     __atomic_add_fetch(&frames_queued, 1, __ATOMIC_RELAXED);
 
     // Wake Core1 if idle
-    __sev();  // Send event to wake WFI
+    __dsb();  // Complete publication before waking the other core.
+    __sev();
 
     return true;
 }
 
 // Check if KMBox commands available
 bool core1_has_kmbox_commands(void) {
-    return kmbox_head != kmbox_tail;
+    return __atomic_load_n(&kmbox_head, __ATOMIC_ACQUIRE) !=
+           __atomic_load_n(&kmbox_tail, __ATOMIC_RELAXED);
 }
 
 // Dequeue translated KMBox command
 bool core1_dequeue_kmbox_command(uint8_t* packet_out) {
-    uint8_t head = kmbox_head;
-    uint8_t tail = kmbox_tail;
+    uint8_t tail = __atomic_load_n(&kmbox_tail, __ATOMIC_RELAXED);
+    uint8_t head = __atomic_load_n(&kmbox_head, __ATOMIC_ACQUIRE);
 
     if (head == tail) {
         return false;  // Queue empty
@@ -133,7 +134,7 @@ bool core1_dequeue_kmbox_command(uint8_t* packet_out) {
     memcpy(packet_out, kmbox_queue[tail].packet, 8);
 
     // Advance tail (consume command)
-    kmbox_tail = (tail + 1) & (KMBOX_CMD_QUEUE_SIZE - 1);
+    __atomic_store_n(&kmbox_tail, (tail + 1) & (KMBOX_CMD_QUEUE_SIZE - 1), __ATOMIC_RELEASE);
 
     return true;
 }

bridge/ferrum_translator.c

@@ -19,6 +19,7 @@
 #include <string.h>
 #include <stddef.h>
 #include <ctype.h>
+#include <limits.h>
 
 // Persistent button state — buttons are sticky (press stays until release)
 static uint8_t g_button_state = 0;
@@ -41,12 +42,15 @@ static bool parse_int(const char **p, int32_t *out) {
 
     if (!isdigit((unsigned char)**p)) return false;
 
-    int32_t val = 0;
+    uint32_t val = 0;
+    uint32_t limit = (uint32_t)INT32_MAX + (neg ? 1u : 0u);
     while (isdigit((unsigned char)**p)) {
-        val = val * 10 + (**p - '0');
+        uint32_t digit = (uint32_t)(**p - '0');
+        if (val > (limit - digit) / 10u) return false;
+        val = val * 10u + digit;
         (*p)++;
     }
-    *out = neg ? -val : val;
+    *out = neg ? (val == (uint32_t)INT32_MAX + 1u ? INT32_MIN : -(int32_t)val) : (int32_t)val;
     return true;
 }
 
@@ -74,7 +78,13 @@ static void emit_button_packet(ferrum_translated_t *out, uint8_t mask, bool pres
 // ============================================================================
 
 bool ferrum_translate_line(const char *line, size_t len, ferrum_translated_t *out) {
-    if (!line || len == 0 || !out) return false;
+    if (!line || len == 0 || len >= FERRUM_MAX_LINE || !out) return false;
+
+    // Respect the supplied span; callers need not provide a trailing NUL.
+    char bounded_line[FERRUM_MAX_LINE];
+    memcpy(bounded_line, line, len);
+    bounded_line[len] = '\0';
+    line = bounded_line;
 
     memset(out, 0, sizeof(ferrum_translated_t));
 
@@ -94,7 +104,8 @@ bool ferrum_translate_line(const char *line, size_t len, ferrum_translated_t *ou
         if (!parse_int(&p, &x)) return false;
         skip_separator(&p);
         if (!parse_int(&p, &y)) return false;
-        if (*p != ')') return false;
+        if (*p != ')' || x < INT16_MIN || x > INT16_MAX ||
+            y < INT16_MIN || y > INT16_MAX) return false;
 
         // Direct accumulator (0x01) — the coalescer in main.c intercepts
         // move packets and re-routes through smooth queue (0x07) at a
@@ -113,7 +124,7 @@ bool ferrum_translate_line(const char *line, size_t len, ferrum_translated_t *ou
 
         int32_t wheel;
         if (!parse_int(&p, &wheel)) return false;
-        if (*p != ')') return false;
+        if (*p != ')' || wheel < INT8_MIN || wheel > INT8_MAX) return false;
 
         out->length = (uint16_t)fast_build_mouse_move(
             out->buffer, 0, 0, g_button_state, (int8_t)wheel);