Refactor command handling

Author: loopj

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)

firmware/libsi/include/si/commands.h …mware/libsi/include/si/device/commands.hfirmware/libsi/include/si/commands.h renamed to firmware/libsi/include/si/device/commands.h firmware/libsi/include/si/commands.h renamed to 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();
 

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.

firmware/libsi/src/commands.c firmware/libsi/src/device/commands.cfirmware/libsi/src/commands.c renamed to 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,65 +71,67 @@ 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
   if (auto_tx_rx_transition)
     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()
 {
   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()

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;