From 5ebc8bc1b1114821daf7ab7d2847b3578922eb12 Mon Sep 17 00:00:00 2001
From: James Smith <james@loopj.com>
Date: Tue, 5 Aug 2025 15:14:20 -0700
Subject: [PATCH 1/5] Refactor command handling

---
 firmware/libsi/CMakeLists.txt                 |  2 +-
 .../libsi/include/si/{ => device}/commands.h  | 26 +-----
 firmware/libsi/include/si/si.h                | 24 ++++--
 firmware/libsi/src/{ => device}/commands.c    | 81 +++++++++++--------
 firmware/libsi/src/device/gc_controller.c     | 18 ++---
 .../libsi/src/platform/efr32/si_efr32_emlib.c | 68 +++++++---------
 firmware/libsi/test/test_commands.c           |  6 +-
 firmware/libsi/test/test_gc_controller.c      |  6 +-
 firmware/receiver/src/main.c                  |  2 +-
 9 files changed, 112 insertions(+), 121 deletions(-)
 rename firmware/libsi/include/si/{ => device}/commands.h (68%)
 rename firmware/libsi/src/{ => device}/commands.c (59%)

diff --git a/firmware/libsi/CMakeLists.txt b/firmware/libsi/CMakeLists.txt
index 879244e..3b4d74e 100644
--- a/firmware/libsi/CMakeLists.txt
+++ b/firmware/libsi/CMakeLists.txt
@@ -5,7 +5,7 @@ cmake_minimum_required(VERSION "3.21")
 project(si LANGUAGES C)
 
 # Define the library
-add_library(si STATIC "src/crc8.c" "src/commands.c" "src/device/gc_controller.c")
+add_library(si STATIC "src/crc8.c" "src/device/commands.c" "src/device/gc_controller.c")
 
 # Configure SI peripheral selection
 if(DEFINED SI_RX_TIMER_IDX)
diff --git a/firmware/libsi/include/si/commands.h b/firmware/libsi/include/si/device/commands.h
similarity index 68%
rename from firmware/libsi/include/si/commands.h
rename to firmware/libsi/include/si/device/commands.h
index 2c46ccb..79e5860 100644
--- a/firmware/libsi/include/si/commands.h
+++ b/firmware/libsi/include/si/device/commands.h
@@ -3,7 +3,7 @@
 #include <stdbool.h>
 #include <stdint.h>
 
-#include "si.h"
+#include "si/si.h"
 
 /**
  * Function type for command handlers.
@@ -14,7 +14,7 @@
  *
  * @return 0 on success, negative error code on failure
  */
-typedef int (*si_command_handler_fn)(const uint8_t *command, si_callback_fn callback, void *context);
+typedef int (*si_command_handler_fn)(const uint8_t *command, si_complete_cb_t callback, void *context);
 
 /**
  * Register a command handler for commands from an SI host.
@@ -27,27 +27,9 @@ typedef int (*si_command_handler_fn)(const uint8_t *command, si_callback_fn call
 void si_command_register(uint8_t command, uint8_t length, si_command_handler_fn handler, void *context);
 
 /**
- * Get the expected length of an SI command.
- *
- * @param command the command to check
- *
- * @return the expected length of the command, in bytes, or 0 if the command is unknown
- */
-uint8_t si_command_get_length(uint8_t command);
-
-/**
- * Get the command handler for an SI command.
- *
- * @param command the command to check
- *
- * @return the command handler function, or NULL if the command is unknown
- */
-si_command_handler_fn si_command_get_handler(uint8_t command);
-
-/**
- * Process an SI command.
- *
+ * Process a single SI command on the bus.
  *
+ * This will read a command from the SI bus and call the registered handler.
  */
 void si_command_process();
 
diff --git a/firmware/libsi/include/si/si.h b/firmware/libsi/include/si/si.h
index 7233f2e..2eead74 100644
--- a/firmware/libsi/include/si/si.h
+++ b/firmware/libsi/include/si/si.h
@@ -41,6 +41,7 @@
 
 #pragma once
 
+#include <stdbool.h>
 #include <stddef.h>
 #include <stdint.h>
 
@@ -118,12 +119,21 @@ enum {
   SI_ERR_TRANSFER_TIMEOUT,
 };
 
+/**
+ * Function type for per-byte callbacks during receive operations.
+ *
+ * @param byte the received byte
+ * @param byte_index the zero-based index of the byte (0 for first byte)
+ * @return true to continue the transfer, false to stop it
+ */
+typedef bool (*si_byte_cb_t)(uint8_t byte, uint8_t byte_index);
+
 /**
  * Function type for transfer completion callbacks.
  *
  * @param result 0 on success, negative error code on failure
  */
-typedef void (*si_callback_fn)(int result);
+typedef void (*si_complete_cb_t)(int result_or_bytes_read);
 
 /**
  * Initialize the SI bus.
@@ -143,24 +153,26 @@ void si_init(uint8_t port, uint8_t pin, uint8_t mode, uint32_t rx_freq, uint32_t
  * @param length the length of the data
  * @param callback function to call when the transfer is complete
  */
-void si_write_bytes(const uint8_t *data, uint8_t length, si_callback_fn callback);
+void si_write_bytes(const uint8_t *data, uint8_t length, si_complete_cb_t callback);
 
 /**
  * Read data from the SI bus.
  *
  * @param buffer the buffer to read into
- * @param length the number of bytes expected
- * @param callback function to call when the transfer is complete
+ * @param max_length the maximum number of bytes to read (buffer size)
+ * @param byte_callback optional function to call for each received byte (can be NULL)
+ * @param complete_callback function to call when the transfer is complete
  */
-void si_read_bytes(uint8_t *buffer, uint8_t length, si_callback_fn callback);
+void si_read_bytes(uint8_t *buffer, uint8_t max_length, si_byte_cb_t byte_callback, si_complete_cb_t complete_callback);
 
 /**
  * Read a single command from the SI bus.
  *
  * @param buffer the buffer to read into
+ * @param max_length the maximum buffer size
  * @param callback function to call when the command has been read
  */
-void si_read_command(uint8_t *buffer, si_callback_fn callback);
+void si_read_command(uint8_t *buffer, uint8_t max_length, si_complete_cb_t callback);
 
 /**
  * Wait for the SI bus to be idle.
diff --git a/firmware/libsi/src/commands.c b/firmware/libsi/src/device/commands.c
similarity index 59%
rename from firmware/libsi/src/commands.c
rename to firmware/libsi/src/device/commands.c
index eddb5da..dfc6f04 100644
--- a/firmware/libsi/src/commands.c
+++ b/firmware/libsi/src/device/commands.c
@@ -1,4 +1,4 @@
-#include "si/commands.h"
+#include "si/device/commands.h"
 
 #define COMMAND_TABLE_SIZE 18
 
@@ -16,12 +16,21 @@ struct command_entry {
   void *context;
 };
 
-static uint8_t bus_state                                      = BUS_STATE_UNKNOWN;
+// Command table for registered SI commands
 static struct command_entry command_table[COMMAND_TABLE_SIZE] = {0};
+static struct command_entry *current_command                  = NULL;
+
+// Current bus state
+static uint8_t bus_state = BUS_STATE_UNKNOWN;
+
+// Buffer for reading/writing commands
 static uint8_t command_buffer[SI_BLOCK_SIZE];
+
+// Automatically transition back to reading commands
 static bool auto_tx_rx_transition = false;
 
 // Vaguely context-aware hash function
+// We're hashing to reduce memory usage while keeping O(1) lookup time in most cases
 static inline uint8_t hash_command(uint8_t command)
 {
   // Reserve hash slots for universal commands
@@ -62,18 +71,13 @@ static void on_tx_complete(int result)
 // Command handler RX completion callback
 static void on_rx_complete(int result)
 {
-  if (result == 0) {
-    // Look up the command in the table
-    struct command_entry *command = find_command(command_buffer[0]);
-    if (command && command->handler) {
-      // Call the command handler
-      bus_state = BUS_STATE_BUSY;
-      command->handler(command_buffer, on_tx_complete, command->context);
-      return;
-    }
+  // If we successfully read a command and have a handler, call it
+  if (result >= 0 && current_command && current_command->handler) {
+    current_command->handler(command_buffer, on_tx_complete, current_command->context);
+    return;
   }
 
-  // Error during command read or handler not found
+  // Otherwise, there was either an error during the read, or handler not found
   bus_state = BUS_STATE_ERROR;
 
   // Transition back to RX mode if auto transition is enabled
@@ -81,37 +85,41 @@ static void on_rx_complete(int result)
     si_command_process();
 }
 
-void si_command_register(uint8_t command, uint8_t length, si_command_handler_fn handler, void *context)
+static bool command_byte_cb(uint8_t byte, uint8_t byte_index)
 {
-  uint8_t index = hash_command(command);
+  // If this is the first byte, look up the command entry
+  if (byte_index == 0) {
+    current_command = find_command(byte);
+    if (current_command == NULL) {
+      return false; // Stop reading on unknown command
+    }
+  }
 
-  // Linear probing for collision resolution
-  while (command_table[index].handler != NULL && command_table[index].command != command) {
-    index = (index + 1) % COMMAND_TABLE_SIZE;
+  if (byte_index == current_command->length - 1) {
+    return false; // Stop reading after the expected length
   }
 
-  command_table[index].command = command;
-  command_table[index].length  = length;
-  command_table[index].handler = handler;
-  command_table[index].context = context;
+  return true;
 }
 
-uint8_t si_command_get_length(uint8_t command)
+void si_command_register(uint8_t command, uint8_t length, si_command_handler_fn handler, void *context)
 {
-  struct command_entry *entry = find_command(command);
-  if (entry == NULL)
-    return 0;
-
-  return entry->length;
-}
+  uint8_t index = hash_command(command);
 
-si_command_handler_fn si_command_get_handler(uint8_t command)
-{
-  struct command_entry *entry = find_command(command);
-  if (entry == NULL)
-    return NULL;
+  // Linear probing for collision resolution
+  // If the slot is already occupied, find the next available slot
+  // This has high clustering, but is simple and effective for our use case
+  while (command_table[index].handler != NULL && command_table[index].command != command) {
+    index = (index + 1) % COMMAND_TABLE_SIZE;
+  }
 
-  return entry->handler;
+  // Store the command entry
+  command_table[index] = (struct command_entry){
+      .command = command,
+      .length  = length,
+      .handler = handler,
+      .context = context,
+  };
 }
 
 void si_command_process()
@@ -119,8 +127,11 @@ void si_command_process()
   if (bus_state != BUS_STATE_IDLE)
     si_await_bus_idle();
 
+  // Initialize command reading context
+  current_command = NULL;
+
   bus_state = BUS_STATE_BUSY;
-  si_read_command(command_buffer, on_rx_complete);
+  si_read_bytes(command_buffer, SI_BLOCK_SIZE, command_byte_cb, on_rx_complete);
 }
 
 void si_command_processing_enable()
diff --git a/firmware/libsi/src/device/gc_controller.c b/firmware/libsi/src/device/gc_controller.c
index e4f7374..62b34b3 100644
--- a/firmware/libsi/src/device/gc_controller.c
+++ b/firmware/libsi/src/device/gc_controller.c
@@ -1,6 +1,6 @@
 #include <string.h>
 
-#include "si/commands.h"
+#include "si/device/commands.h"
 #include "si/device/gc_controller.h"
 
 /*
@@ -74,7 +74,7 @@ static uint8_t *pack_input_state(struct si_device_gc_input_state *src, uint8_t a
  * Command:         {0x00}
  * Response:        A 3-byte device info.
  */
-static int handle_info(const uint8_t *command, si_callback_fn callback, void *context)
+static int handle_info(const uint8_t *command, si_complete_cb_t callback, void *context)
 {
   struct si_device_gc_controller *device = (struct si_device_gc_controller *)context;
 
@@ -90,7 +90,7 @@ static int handle_info(const uint8_t *command, si_callback_fn callback, void *co
  * Command:         {0xFF}
  * Response:        A 3-byte device info.
  */
-static int handle_reset(const uint8_t *command, si_callback_fn callback, void *context)
+static int handle_reset(const uint8_t *command, si_complete_cb_t callback, void *context)
 {
   struct si_device_gc_controller *device = (struct si_device_gc_controller *)context;
 
@@ -108,7 +108,7 @@ static int handle_reset(const uint8_t *command, si_callback_fn callback, void *c
  * Command:         {0x40, analog_mode, motor_state}
  * Response:        An 8-byte packed input state, see `pack_input_state` for details
  */
-static int handle_short_poll(const uint8_t *command, si_callback_fn callback, void *context)
+static int handle_short_poll(const uint8_t *command, si_complete_cb_t callback, void *context)
 {
   struct si_device_gc_controller *device = (struct si_device_gc_controller *)context;
 
@@ -150,7 +150,7 @@ static int handle_short_poll(const uint8_t *command, si_callback_fn callback, vo
  * Command:         {0x41}
  * Response:        A 10-byte input state representing the current origin.
  */
-static int handle_read_origin(const uint8_t *command, si_callback_fn callback, void *context)
+static int handle_read_origin(const uint8_t *command, si_complete_cb_t callback, void *context)
 {
   struct si_device_gc_controller *device = (struct si_device_gc_controller *)context;
 
@@ -174,7 +174,7 @@ static int handle_read_origin(const uint8_t *command, si_callback_fn callback, v
  * Command:         {0x42, 0x00, 0x00}
  * Response:        A 10-byte input state representing the current origin.
  */
-static int handle_calibrate(const uint8_t *command, si_callback_fn callback, void *context)
+static int handle_calibrate(const uint8_t *command, si_complete_cb_t callback, void *context)
 {
   struct si_device_gc_controller *device = (struct si_device_gc_controller *)context;
 
@@ -205,7 +205,7 @@ static int handle_calibrate(const uint8_t *command, si_callback_fn callback, voi
  *
  * NOTE: This command is not used by any games, but is included for completeness.
  */
-static int handle_long_poll(const uint8_t *command, si_callback_fn callback, void *context)
+static int handle_long_poll(const uint8_t *command, si_complete_cb_t callback, void *context)
 {
   struct si_device_gc_controller *device = (struct si_device_gc_controller *)context;
 
@@ -238,7 +238,7 @@ static int handle_long_poll(const uint8_t *command, si_callback_fn callback, voi
  * Command:         {0x4D, 0x??, 0x??} - 2nd and 3rd bytes seem to differ every time
  * Response:        8 bytes of zeroes.
  */
-static int handle_probe_device(const uint8_t *command, si_callback_fn callback, void *context)
+static int handle_probe_device(const uint8_t *command, si_complete_cb_t callback, void *context)
 {
   // Respond with 8 bytes of zeroes
   uint8_t response[8] = {0};
@@ -255,7 +255,7 @@ static int handle_probe_device(const uint8_t *command, si_callback_fn callback,
  * Command:         {0x4E, wireless_id_h | SI_WIRELESS_FIX_ID, wireless_id_l}
  * Response:        A 3-byte device info.
  */
-static int handle_fix_device(const uint8_t *command, si_callback_fn callback, void *context)
+static int handle_fix_device(const uint8_t *command, si_complete_cb_t callback, void *context)
 {
   struct si_device_gc_controller *device = (struct si_device_gc_controller *)context;
 
diff --git a/firmware/libsi/src/platform/efr32/si_efr32_emlib.c b/firmware/libsi/src/platform/efr32/si_efr32_emlib.c
index a4f92c5..49bc999 100644
--- a/firmware/libsi/src/platform/efr32/si_efr32_emlib.c
+++ b/firmware/libsi/src/platform/efr32/si_efr32_emlib.c
@@ -8,7 +8,7 @@
 
 #include "dmadrv.h"
 
-#include "si/commands.h"
+#include "si/device/commands.h"
 
 #ifdef __ZEPHYR__
 #include <zephyr/irq.h>
@@ -96,8 +96,9 @@ static unsigned int tx_dma_channel;
 // Transfer state
 static struct {
   uint8_t *data;
-  uint8_t length;
-  si_callback_fn callback;
+  uint8_t max_length;
+  si_byte_cb_t byte_callback;
+  si_complete_cb_t complete_callback;
 } si_xfer;
 
 // RX LDMA configuration
@@ -147,12 +148,13 @@ void si_init(uint8_t port, uint8_t pin, uint8_t mode, uint32_t rx_freq, uint32_t
   si_mode      = mode;
 }
 
-void si_write_bytes(const uint8_t *bytes, uint8_t length, si_callback_fn callback)
+void si_write_bytes(const uint8_t *bytes, uint8_t length, si_complete_cb_t callback)
 {
   // Save the transfer state
-  si_xfer.data     = (uint8_t *)bytes;
-  si_xfer.length   = length;
-  si_xfer.callback = callback;
+  si_xfer.data              = (uint8_t *)bytes;
+  si_xfer.max_length        = length;
+  si_xfer.byte_callback     = NULL;
+  si_xfer.complete_callback = callback;
 
   // Convert the bytes to appropriate line coding and add to the buffer
   uint8_t *buf_ptr = tx_buffer;
@@ -169,12 +171,13 @@ void si_write_bytes(const uint8_t *bytes, uint8_t length, si_callback_fn callbac
   DMADRV_LdmaStartTransfer(tx_dma_channel, &ldma_tx_config, ldma_tx_descriptors, NULL, NULL);
 }
 
-void si_read_bytes(uint8_t *buffer, uint8_t length, si_callback_fn callback)
+void si_read_bytes(uint8_t *buffer, uint8_t max_length, si_byte_cb_t byte_callback, si_complete_cb_t complete_callback)
 {
   // Save the transfer state
-  si_xfer.data     = buffer;
-  si_xfer.length   = length;
-  si_xfer.callback = callback;
+  si_xfer.data              = buffer;
+  si_xfer.max_length        = max_length;
+  si_xfer.byte_callback     = byte_callback;
+  si_xfer.complete_callback = complete_callback;
 
   // Clear the RX buffer
   while (TIMER_CaptureGet(SI_RX_TIMER, 0))
@@ -187,11 +190,6 @@ void si_read_bytes(uint8_t *buffer, uint8_t length, si_callback_fn callback)
   DMADRV_LdmaStartTransfer(rx_dma_channel, &ldma_rx_config, ldma_rx_descriptors, ldma_callback_rx, NULL);
 }
 
-void si_read_command(uint8_t *buffer, si_callback_fn callback)
-{
-  si_read_bytes(buffer, 0, callback);
-}
-
 void si_await_bus_idle(void)
 {
   // Start the timer
@@ -340,32 +338,20 @@ static bool ldma_callback_rx(unsigned int chan, unsigned int iteration, void *us
   // Process the received pulses into the byte buffer
   decode_edge_timings(&si_xfer.data[byte_idx], rx_edge_timings[byte_idx % 2]);
 
-  // If this is the first byte, determine how many bytes are expected
-  if (si_xfer.length == 0 && iteration == 1) {
-    si_xfer.length = si_command_get_length(si_xfer.data[0]);
-
-    // Unknown command, stop the transfer
-    if (si_xfer.length == 0) {
-      // Don't clock in any more data
-      TIMER_Enable(SI_RX_TIMER, false);
-
-      // Call the transfer callback if one is set
-      if (si_xfer.callback)
-        si_xfer.callback(-SI_ERR_UNKNOWN_COMMAND);
-
-      // Stop the LDMA chain
-      return false;
-    }
+  // Call the byte callback if provided
+  bool continue_transfer = true;
+  if (si_xfer.byte_callback) {
+    continue_transfer = si_xfer.byte_callback(si_xfer.data[byte_idx], byte_idx);
   }
 
-  // We have all the bytes we expected
-  if (iteration == si_xfer.length) {
-    // Don't clock in any more data
+  // If we have reached the maximum length, stop the transfer
+  if (!continue_transfer || byte_idx == si_xfer.max_length - 1) {
+    // Stop the timer
     TIMER_Enable(SI_RX_TIMER, false);
 
-    // Call the transfer callback if one is set
-    if (si_xfer.callback)
-      si_xfer.callback(0);
+    // Call the complete callback if provided
+    if (si_xfer.complete_callback)
+      si_xfer.complete_callback(byte_idx + 1);
 
     // Stop the LDMA chain
     return false;
@@ -382,7 +368,7 @@ void SI_TX_USART_IRQHandler()
   uint32_t flags = USART_IntGet(SI_TX_USART);
   USART_IntClear(SI_TX_USART, flags);
 
-  // Call the transfer callback if one is set
-  if (si_xfer.callback)
-    si_xfer.callback(0);
+  // Call the transfer callback with the number of bytes written
+  if (si_xfer.complete_callback)
+    si_xfer.complete_callback(si_xfer.max_length);
 }
\ No newline at end of file
diff --git a/firmware/libsi/test/test_commands.c b/firmware/libsi/test/test_commands.c
index 107301e..7634404 100644
--- a/firmware/libsi/test/test_commands.c
+++ b/firmware/libsi/test/test_commands.c
@@ -1,6 +1,6 @@
 #include "unity.h"
 
-#include "si/commands.h"
+#include "si/device/commands.h"
 #include "si/si.h"
 
 // Stub functions
@@ -8,12 +8,12 @@ void si_await_bus_idle(void)
 {
 }
 
-static int handle_info(const uint8_t *command, si_callback_fn callback, void *context)
+static int handle_info(const uint8_t *command, si_complete_cb_t callback, void *context)
 {
   return 3;
 }
 
-static int handle_reset(const uint8_t *command, si_callback_fn callback, void *context)
+static int handle_reset(const uint8_t *command, si_complete_cb_t callback, void *context)
 {
   return 3;
 }
diff --git a/firmware/libsi/test/test_gc_controller.c b/firmware/libsi/test/test_gc_controller.c
index 2cfc691..34c74b7 100644
--- a/firmware/libsi/test/test_gc_controller.c
+++ b/firmware/libsi/test/test_gc_controller.c
@@ -3,7 +3,7 @@
 
 #include "unity.h"
 
-#include "si/commands.h"
+#include "si/device/commands.h"
 #include "si/device/gc_controller.h"
 #include "si/si.h"
 
@@ -11,13 +11,13 @@
 static uint8_t response_buf[SI_BLOCK_SIZE] = {0};
 static uint8_t response_len                = 0;
 
-void si_write_bytes(const uint8_t *data, uint8_t length, si_callback_fn callback)
+void si_write_bytes(const uint8_t *data, uint8_t length, si_complete_cb_t callback)
 {
   memcpy(response_buf, data, length);
   response_len = length;
 }
 
-void si_read_command(uint8_t *data, si_callback_fn callback)
+void si_read_command(uint8_t *data, si_complete_cb_t callback)
 {
 }
 
diff --git a/firmware/receiver/src/main.c b/firmware/receiver/src/main.c
index f0375a9..fb69a2b 100644
--- a/firmware/receiver/src/main.c
+++ b/firmware/receiver/src/main.c
@@ -5,7 +5,7 @@
 #include "em_cmu.h"
 #include "em_gpio.h"
 
-#include "si/commands.h"
+#include "si/device/commands.h"
 #include "si/device/gc_controller.h"
 #include "wavebird/message.h"
 #include "wavebird/packet.h"

From 5355bfd020164b9cc100b69bcab60889faebcf37 Mon Sep 17 00:00:00 2001
From: James Smith <james@loopj.com>
Date: Tue, 5 Aug 2025 16:10:07 -0700
Subject: [PATCH 2/5] Fix tests

---
 firmware/libsi/include/si/device/commands.h | 21 ++++++-
 firmware/libsi/src/device/commands.c        | 69 +++++++++------------
 firmware/libsi/test/test_commands.c         | 18 ++++--
 firmware/libsi/test/test_gc_controller.c    |  6 +-
 4 files changed, 65 insertions(+), 49 deletions(-)

diff --git a/firmware/libsi/include/si/device/commands.h b/firmware/libsi/include/si/device/commands.h
index 79e5860..c1537ee 100644
--- a/firmware/libsi/include/si/device/commands.h
+++ b/firmware/libsi/include/si/device/commands.h
@@ -16,16 +16,35 @@
  */
 typedef int (*si_command_handler_fn)(const uint8_t *command, si_complete_cb_t callback, void *context);
 
+/**
+ * Command structure representing a registered command.
+ */
+struct si_command {
+  uint8_t command;
+  uint8_t length;
+  si_command_handler_fn handler;
+  void *user_data;
+};
+
 /**
  * Register a command handler for commands from an SI host.
  *
  * @param command the command to handle
- * @param command_length the length of the command
+ * @param command_length the length of the command in bytes
  * @param handler the command handler function
  *
  */
 void si_command_register(uint8_t command, uint8_t length, si_command_handler_fn handler, void *context);
 
+/**
+ * Look up a command structure by command ID.
+ *
+ * @param command the command ID to look up
+ *
+ * @return pointer to the command structure, or NULL if not found
+ */
+struct si_command *si_command_find_by_id(uint8_t command);
+
 /**
  * Process a single SI command on the bus.
  *
diff --git a/firmware/libsi/src/device/commands.c b/firmware/libsi/src/device/commands.c
index dfc6f04..12b4098 100644
--- a/firmware/libsi/src/device/commands.c
+++ b/firmware/libsi/src/device/commands.c
@@ -9,16 +9,9 @@ enum {
   BUS_STATE_ERROR,
 };
 
-struct command_entry {
-  uint8_t command;
-  uint8_t length;
-  si_command_handler_fn handler;
-  void *context;
-};
-
 // Command table for registered SI commands
-static struct command_entry command_table[COMMAND_TABLE_SIZE] = {0};
-static struct command_entry *current_command                  = NULL;
+static struct si_command command_table[COMMAND_TABLE_SIZE] = {0};
+static struct si_command *current_command                  = NULL;
 
 // Current bus state
 static uint8_t bus_state = BUS_STATE_UNKNOWN;
@@ -43,20 +36,6 @@ static inline uint8_t hash_command(uint8_t command)
   return (command % (COMMAND_TABLE_SIZE - 2)) + 2;
 }
 
-// Find a command entry by command id
-static struct command_entry *find_command(uint8_t command)
-{
-  uint8_t index = hash_command(command);
-
-  while (command_table[index].handler != NULL) {
-    if (command_table[index].command == command) {
-      return &command_table[index];
-    }
-    index = (index + 1) % COMMAND_TABLE_SIZE;
-  }
-  return NULL;
-}
-
 // Command handler TX completion callback
 static void on_tx_complete(int result)
 {
@@ -73,7 +52,7 @@ static void on_rx_complete(int result)
 {
   // If we successfully read a command and have a handler, call it
   if (result >= 0 && current_command && current_command->handler) {
-    current_command->handler(command_buffer, on_tx_complete, current_command->context);
+    current_command->handler(command_buffer, on_tx_complete, current_command->user_data);
     return;
   }
 
@@ -87,22 +66,21 @@ static void on_rx_complete(int result)
 
 static bool command_byte_cb(uint8_t byte, uint8_t byte_index)
 {
-  // If this is the first byte, look up the command entry
+  // If this is the first byte, check if there is a registered command
   if (byte_index == 0) {
-    current_command = find_command(byte);
-    if (current_command == NULL) {
-      return false; // Stop reading on unknown command
-    }
+    current_command = si_command_find_by_id(byte);
+    if (current_command == NULL)
+      return false;
   }
 
-  if (byte_index == current_command->length - 1) {
-    return false; // Stop reading after the expected length
-  }
+  // Stop reading when we reach the expected length
+  if (byte_index == current_command->length - 1)
+    return false;
 
   return true;
 }
 
-void si_command_register(uint8_t command, uint8_t length, si_command_handler_fn handler, void *context)
+void si_command_register(uint8_t command, uint8_t length, si_command_handler_fn handler, void *user_data)
 {
   uint8_t index = hash_command(command);
 
@@ -113,15 +91,28 @@ void si_command_register(uint8_t command, uint8_t length, si_command_handler_fn
     index = (index + 1) % COMMAND_TABLE_SIZE;
   }
 
-  // Store the command entry
-  command_table[index] = (struct command_entry){
-      .command = command,
-      .length  = length,
-      .handler = handler,
-      .context = context,
+  // Store the command
+  command_table[index] = (struct si_command){
+      .command   = command,
+      .length    = length,
+      .handler   = handler,
+      .user_data = user_data,
   };
 }
 
+struct si_command *si_command_find_by_id(uint8_t command)
+{
+  uint8_t index = hash_command(command);
+
+  while (command_table[index].handler != NULL) {
+    if (command_table[index].command == command) {
+      return &command_table[index];
+    }
+    index = (index + 1) % COMMAND_TABLE_SIZE;
+  }
+  return NULL;
+}
+
 void si_command_process()
 {
   if (bus_state != BUS_STATE_IDLE)
diff --git a/firmware/libsi/test/test_commands.c b/firmware/libsi/test/test_commands.c
index 7634404..9de632a 100644
--- a/firmware/libsi/test/test_commands.c
+++ b/firmware/libsi/test/test_commands.c
@@ -20,19 +20,25 @@ static int handle_reset(const uint8_t *command, si_complete_cb_t callback, void
 
 static void test_register_command()
 {
+  struct si_command *cmd = NULL;
+
   si_command_register(0x00, 1, handle_info, NULL);
-  TEST_ASSERT_EQUAL(1, si_command_get_length(0x00));
-  TEST_ASSERT_EQUAL(handle_info, si_command_get_handler(0x00));
+  cmd = si_command_find_by_id(0x00);
+  TEST_ASSERT_NOT_NULL(cmd);
+  TEST_ASSERT_EQUAL(1, cmd->length);
+  TEST_ASSERT_EQUAL(handle_info, cmd->handler);
 
   si_command_register(0xFF, 3, handle_reset, NULL);
-  TEST_ASSERT_EQUAL(3, si_command_get_length(0xFF));
-  TEST_ASSERT_EQUAL(handle_reset, si_command_get_handler(0xFF));
+  cmd = si_command_find_by_id(0xFF);
+  TEST_ASSERT_NOT_NULL(cmd);
+  TEST_ASSERT_EQUAL(3, cmd->length);
+  TEST_ASSERT_EQUAL(handle_reset, cmd->handler);
 }
 
 static void test_register_command_missing()
 {
-  TEST_ASSERT_EQUAL(0, si_command_get_length(0x69));
-  TEST_ASSERT_NULL(si_command_get_handler(0x69));
+  struct si_command *cmd = si_command_find_by_id(0x69);
+  TEST_ASSERT_NULL(cmd);
 }
 
 void test_commands(void)
diff --git a/firmware/libsi/test/test_gc_controller.c b/firmware/libsi/test/test_gc_controller.c
index 34c74b7..882cfc7 100644
--- a/firmware/libsi/test/test_gc_controller.c
+++ b/firmware/libsi/test/test_gc_controller.c
@@ -17,15 +17,15 @@ void si_write_bytes(const uint8_t *data, uint8_t length, si_complete_cb_t callba
   response_len = length;
 }
 
-void si_read_command(uint8_t *data, si_complete_cb_t callback)
+void si_read_bytes(uint8_t *buffer, uint8_t max_length, si_byte_cb_t byte_callback, si_complete_cb_t complete_callback)
 {
 }
 
 // Simulate receiving a command
 static int simulate_command(struct si_device_gc_controller *device, uint8_t *command)
 {
-  si_command_handler_fn handler = si_command_get_handler(command[0]);
-  return handler(command, NULL, device);
+  struct si_command *cmd = si_command_find_by_id(command[0]);
+  return cmd->handler(command, NULL, device);
 }
 
 // Test that the device info response is correct for a standard GameCube controller

From ec4d98f010fc111e6477b9040489ba8e2570a8f4 Mon Sep 17 00:00:00 2001
From: James Smith <james@loopj.com>
Date: Tue, 5 Aug 2025 16:42:52 -0700
Subject: [PATCH 3/5] Fix auto tx->rx transition

---
 firmware/libsi/src/device/commands.c |  6 ++++++
 firmware/receiver/src/main.c         | 18 +++++-------------
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/firmware/libsi/src/device/commands.c b/firmware/libsi/src/device/commands.c
index 12b4098..ceccf35 100644
--- a/firmware/libsi/src/device/commands.c
+++ b/firmware/libsi/src/device/commands.c
@@ -127,12 +127,18 @@ void si_command_process()
 
 void si_command_processing_enable()
 {
+  if (auto_tx_rx_transition)
+    return;
+
   auto_tx_rx_transition = true;
   si_command_process();
 }
 
 void si_command_processing_disable()
 {
+  if (!auto_tx_rx_transition)
+    return;
+
   auto_tx_rx_transition = false;
   bus_state             = BUS_STATE_UNKNOWN;
 }
\ No newline at end of file
diff --git a/firmware/receiver/src/main.c b/firmware/receiver/src/main.c
index fb69a2b..d5d140b 100644
--- a/firmware/receiver/src/main.c
+++ b/firmware/receiver/src/main.c
@@ -78,7 +78,6 @@ struct {
 
 // SI state
 static struct si_device_gc_controller si_device = {0};
-static bool enable_si_command_handling          = true;
 
 // Buttons, switches, and LEDs
 static struct led *status_led              = NULL;
@@ -107,7 +106,7 @@ static void initialize_controller(uint8_t controller_type)
   if (controller_type == WP_CONT_TYPE_GC_WAVEBIRD) {
     // Present as an OEM WaveBird receiver
     si_device_gc_init(&si_device, SI_TYPE_GC | SI_GC_WIRELESS | SI_GC_NOMOTOR);
-    enable_si_command_handling = true;
+    si_command_processing_enable();
   } else if (controller_type == WP_CONT_TYPE_GC_WIRED_NOMOTOR) {
     // Present as a wired GameCube controller without rumble
     si_device_gc_init(&si_device, SI_TYPE_GC | SI_GC_STANDARD | SI_GC_NOMOTOR);
@@ -208,7 +207,7 @@ static void handle_wavebird_packet(const uint8_t *packet)
     memcpy(&si_device.input.stick_x, &message[4], 6);
 
     // We have a good input state, enable SI command handling if it was disabled
-    enable_si_command_handling = true;
+    si_command_processing_enable();
 
     // Set the input state as valid
     input_valid_until = millis + INPUT_VALID_MS;
@@ -246,7 +245,7 @@ static void handle_pairing_started(void)
   pairing_active = true;
 
   // Disable SI command handling during pairing
-  enable_si_command_handling = false;
+  si_command_processing_disable();
 
   // Set the LED effect to indicate pairing mode
   if (status_led)
@@ -281,7 +280,7 @@ static void handle_pairing_finished(uint8_t status, uint8_t channel)
       led_effect_blink(status_led, 500, 3);
 
     // Immediately reenable SI command handling
-    enable_si_command_handling = true;
+    si_command_processing_enable();
   } else {
     DEBUG_PRINT("Pairing cancelled\n");
 
@@ -290,7 +289,7 @@ static void handle_pairing_finished(uint8_t status, uint8_t channel)
       led_off(status_led);
 
     // Immediately reenable SI command handling
-    enable_si_command_handling = true;
+    si_command_processing_enable();
   }
 }
 
@@ -421,15 +420,8 @@ int main(void)
                                                                                            : "Wired (no motor)");
   DEBUG_PRINT("\n");
 
-  // Wait for the SI bus to be idle before starting the main loop
-  si_await_bus_idle();
-
   // Main loop
   while (1) {
-    // Check if we need to initiate the next SI transfer
-    if (enable_si_command_handling)
-      si_command_process();
-
     // Check for new wavebird packets
     wavebird_radio_process();
 

From bc5bc439b83b1af0218fc70abc3d17e29a827aa0 Mon Sep 17 00:00:00 2001
From: James Smith <james@loopj.com>
Date: Thu, 7 Aug 2025 16:20:54 -0700
Subject: [PATCH 4/5] libsi: move gc specific si commands to gc_controller

---
 .../libsi/include/si/device/gc_controller.h   | 26 ++++++++++++-
 firmware/libsi/include/si/si.h                | 38 +------------------
 firmware/libsi/src/device/gc_controller.c     |  2 +-
 3 files changed, 28 insertions(+), 38 deletions(-)

diff --git a/firmware/libsi/include/si/device/gc_controller.h b/firmware/libsi/include/si/device/gc_controller.h
index d8a8ea6..eeee110 100644
--- a/firmware/libsi/include/si/device/gc_controller.h
+++ b/firmware/libsi/include/si/device/gc_controller.h
@@ -7,6 +7,31 @@
 #include <stdbool.h>
 #include <stdint.h>
 
+// GameCube controller commands
+#define SI_CMD_GC_SHORT_POLL        0x40
+#define SI_CMD_GC_SHORT_POLL_LEN    3
+#define SI_CMD_GC_SHORT_POLL_RESP   8
+
+#define SI_CMD_GC_READ_ORIGIN       0x41
+#define SI_CMD_GC_READ_ORIGIN_LEN   1
+#define SI_CMD_GC_READ_ORIGIN_RESP  10
+
+#define SI_CMD_GC_CALIBRATE         0x42
+#define SI_CMD_GC_CALIBRATE_LEN     3
+#define SI_CMD_GC_CALIBRATE_RESP    10
+
+#define SI_CMD_GC_LONG_POLL         0x43
+#define SI_CMD_GC_LONG_POLL_LEN     3
+#define SI_CMD_GC_LONG_POLL_RESP    10
+
+#define SI_CMD_GC_PROBE_DEVICE      0x4D
+#define SI_CMD_GC_PROBE_DEVICE_LEN  3
+#define SI_CMD_GC_PROBE_DEVICE_RESP 8
+
+#define SI_CMD_GC_FIX_DEVICE        0x4E
+#define SI_CMD_GC_FIX_DEVICE_LEN    3
+#define SI_CMD_GC_FIX_DEVICE_RESP   3
+
 /**
  * Rumble motor states.
  */
@@ -88,7 +113,6 @@ struct si_device_gc_controller {
  *
  * @param device the device to initialize
  * @param type the device type flags
- * @param input_state pointer to an input state buffer
  */
 void si_device_gc_init(struct si_device_gc_controller *device, uint8_t type);
 
diff --git a/firmware/libsi/include/si/si.h b/firmware/libsi/include/si/si.h
index 2eead74..2b83ec7 100644
--- a/firmware/libsi/include/si/si.h
+++ b/firmware/libsi/include/si/si.h
@@ -57,31 +57,6 @@
 #define SI_CMD_INFO_LEN             1
 #define SI_CMD_INFO_RESP            3
 
-// GameCube controller commands
-#define SI_CMD_GC_SHORT_POLL        0x40
-#define SI_CMD_GC_SHORT_POLL_LEN    3
-#define SI_CMD_GC_SHORT_POLL_RESP   8
-
-#define SI_CMD_GC_READ_ORIGIN       0x41
-#define SI_CMD_GC_READ_ORIGIN_LEN   1
-#define SI_CMD_GC_READ_ORIGIN_RESP  10
-
-#define SI_CMD_GC_CALIBRATE         0x42
-#define SI_CMD_GC_CALIBRATE_LEN     3
-#define SI_CMD_GC_CALIBRATE_RESP    10
-
-#define SI_CMD_GC_LONG_POLL         0x43
-#define SI_CMD_GC_LONG_POLL_LEN     3
-#define SI_CMD_GC_LONG_POLL_RESP    10
-
-#define SI_CMD_GC_PROBE_DEVICE      0x4D
-#define SI_CMD_GC_PROBE_DEVICE_LEN  3
-#define SI_CMD_GC_PROBE_DEVICE_RESP 8
-
-#define SI_CMD_GC_FIX_DEVICE        0x4E
-#define SI_CMD_GC_FIX_DEVICE_LEN    3
-#define SI_CMD_GC_FIX_DEVICE_RESP   3
-
 // SI device info flags
 // On wireless controllers 0x00C0FF is reserved for the controller ID
 
@@ -131,9 +106,9 @@ typedef bool (*si_byte_cb_t)(uint8_t byte, uint8_t byte_index);
 /**
  * Function type for transfer completion callbacks.
  *
- * @param result 0 on success, negative error code on failure
+ * @param result positive number of bytes read on success, negative error code on failure
  */
-typedef void (*si_complete_cb_t)(int result_or_bytes_read);
+typedef void (*si_complete_cb_t)(int result);
 
 /**
  * Initialize the SI bus.
@@ -165,15 +140,6 @@ void si_write_bytes(const uint8_t *data, uint8_t length, si_complete_cb_t callba
  */
 void si_read_bytes(uint8_t *buffer, uint8_t max_length, si_byte_cb_t byte_callback, si_complete_cb_t complete_callback);
 
-/**
- * Read a single command from the SI bus.
- *
- * @param buffer the buffer to read into
- * @param max_length the maximum buffer size
- * @param callback function to call when the command has been read
- */
-void si_read_command(uint8_t *buffer, uint8_t max_length, si_complete_cb_t callback);
-
 /**
  * Wait for the SI bus to be idle.
  *
diff --git a/firmware/libsi/src/device/gc_controller.c b/firmware/libsi/src/device/gc_controller.c
index 62b34b3..fa98c44 100644
--- a/firmware/libsi/src/device/gc_controller.c
+++ b/firmware/libsi/src/device/gc_controller.c
@@ -302,11 +302,11 @@ void si_device_gc_init(struct si_device_gc_controller *device, uint8_t type)
 
   // Register the SI commands handled by GameCube controllers
   si_command_register(SI_CMD_INFO, SI_CMD_INFO_LEN, handle_info, device);
+  si_command_register(SI_CMD_RESET, SI_CMD_RESET_LEN, handle_reset, device);
   si_command_register(SI_CMD_GC_SHORT_POLL, SI_CMD_GC_SHORT_POLL_LEN, handle_short_poll, device);
   si_command_register(SI_CMD_GC_READ_ORIGIN, SI_CMD_GC_READ_ORIGIN_LEN, handle_read_origin, device);
   si_command_register(SI_CMD_GC_CALIBRATE, SI_CMD_GC_CALIBRATE_LEN, handle_calibrate, device);
   si_command_register(SI_CMD_GC_LONG_POLL, SI_CMD_GC_LONG_POLL_LEN, handle_long_poll, device);
-  si_command_register(SI_CMD_RESET, SI_CMD_RESET_LEN, handle_reset, device);
 
   // Register additional commands handled by WaveBird receivers
   if (type & SI_GC_WIRELESS) {

From ef40fffcdd1eaff7d931df970bbdfc3a9a51bc72 Mon Sep 17 00:00:00 2001
From: James Smith <james@loopj.com>
Date: Thu, 7 Aug 2025 16:43:08 -0700
Subject: [PATCH 5/5] Remove complex bus state logic

---
 firmware/libsi/include/si/device/commands.h   |  4 +-
 firmware/libsi/src/device/commands.c          | 56 +++++++------------
 .../libsi/src/platform/efr32/si_efr32_emlib.c |  2 +
 3 files changed, 26 insertions(+), 36 deletions(-)

diff --git a/firmware/libsi/include/si/device/commands.h b/firmware/libsi/include/si/device/commands.h
index c1537ee..ea5cdb8 100644
--- a/firmware/libsi/include/si/device/commands.h
+++ b/firmware/libsi/include/si/device/commands.h
@@ -49,8 +49,10 @@ struct si_command *si_command_find_by_id(uint8_t command);
  * Process a single SI command on the bus.
  *
  * This will read a command from the SI bus and call the registered handler.
+ *
+ * @param await_bus_idle if true, will wait for the SI bus to be idle before reading commands
  */
-void si_command_process();
+void si_command_process(bool await_bus_idle);
 
 /**
  * Enable automatic command processing.
diff --git a/firmware/libsi/src/device/commands.c b/firmware/libsi/src/device/commands.c
index ceccf35..27cc915 100644
--- a/firmware/libsi/src/device/commands.c
+++ b/firmware/libsi/src/device/commands.c
@@ -2,25 +2,15 @@
 
 #define COMMAND_TABLE_SIZE 18
 
-enum {
-  BUS_STATE_UNKNOWN = 0,
-  BUS_STATE_IDLE,
-  BUS_STATE_BUSY,
-  BUS_STATE_ERROR,
-};
-
 // Command table for registered SI commands
 static struct si_command command_table[COMMAND_TABLE_SIZE] = {0};
 static struct si_command *current_command                  = NULL;
 
-// Current bus state
-static uint8_t bus_state = BUS_STATE_UNKNOWN;
-
 // Buffer for reading/writing commands
 static uint8_t command_buffer[SI_BLOCK_SIZE];
 
 // Automatically transition back to reading commands
-static bool auto_tx_rx_transition = false;
+static bool command_processing_enabled = false;
 
 // Vaguely context-aware hash function
 // We're hashing to reduce memory usage while keeping O(1) lookup time in most cases
@@ -39,12 +29,10 @@ static inline uint8_t hash_command(uint8_t command)
 // Command handler TX completion callback
 static void on_tx_complete(int result)
 {
-  // Update bus state based on result
-  bus_state = (result < 0) ? BUS_STATE_ERROR : BUS_STATE_IDLE;
-
-  // Transition back to RX mode if auto transition is enabled
-  if (auto_tx_rx_transition)
-    si_command_process();
+  // Begin listening for the next command if command processing is enabled
+  // Wait for bus idle if there was an error
+  if (command_processing_enabled)
+    si_command_process(result < 0);
 }
 
 // Command handler RX completion callback
@@ -56,12 +44,9 @@ static void on_rx_complete(int result)
     return;
   }
 
-  // Otherwise, there was either an error during the read, or handler not found
-  bus_state = BUS_STATE_ERROR;
-
-  // Transition back to RX mode if auto transition is enabled
-  if (auto_tx_rx_transition)
-    si_command_process();
+  // Begin listening for the next command if command processing is enabled
+  if (command_processing_enabled)
+    si_command_process(true);
 }
 
 static bool command_byte_cb(uint8_t byte, uint8_t byte_index)
@@ -113,32 +98,33 @@ struct si_command *si_command_find_by_id(uint8_t command)
   return NULL;
 }
 
-void si_command_process()
+void si_command_process(bool await_bus_idle)
 {
-  if (bus_state != BUS_STATE_IDLE)
+  // Await for the bus to be idle if requested
+  if (await_bus_idle)
     si_await_bus_idle();
 
-  // Initialize command reading context
-  current_command = NULL;
-
-  bus_state = BUS_STATE_BUSY;
+  // Begin reading the next command from the SI bus
   si_read_bytes(command_buffer, SI_BLOCK_SIZE, command_byte_cb, on_rx_complete);
 }
 
 void si_command_processing_enable()
 {
-  if (auto_tx_rx_transition)
+  if (command_processing_enabled)
     return;
 
-  auto_tx_rx_transition = true;
-  si_command_process();
+  // Mark command processing as enabled
+  command_processing_enabled = true;
+
+  // Start processing commands, waiting for the bus to be idle
+  si_command_process(true);
 }
 
 void si_command_processing_disable()
 {
-  if (!auto_tx_rx_transition)
+  if (!command_processing_enabled)
     return;
 
-  auto_tx_rx_transition = false;
-  bus_state             = BUS_STATE_UNKNOWN;
+  // Mark command processing as disabled
+  command_processing_enabled = false;
 }
\ No newline at end of file
diff --git a/firmware/libsi/src/platform/efr32/si_efr32_emlib.c b/firmware/libsi/src/platform/efr32/si_efr32_emlib.c
index 49bc999..3168e87 100644
--- a/firmware/libsi/src/platform/efr32/si_efr32_emlib.c
+++ b/firmware/libsi/src/platform/efr32/si_efr32_emlib.c
@@ -197,6 +197,7 @@ void si_await_bus_idle(void)
 
   while (1) {
     // Wait for the line to go high
+    // TODO: Add a timeout
     while (GPIO_PinInGet(si_data_port, si_data_pin) == 0)
       ;
 
@@ -204,6 +205,7 @@ void si_await_bus_idle(void)
     TIMER_CounterSet(SI_RX_TIMER, 0);
 
     // Wait for either the bus idle period to elapse or line to go low
+    // TODO: Add a timeout
     while (GPIO_PinInGet(si_data_port, si_data_pin) == 1) {
       if (TIMER_CounterGet(SI_RX_TIMER) >= rx_bus_idle_period)
         goto idle_detected;