Simpler SPI protocol

SConscript

@@ -64,10 +64,10 @@ def build_project(project_name, project, main, extra_flags):
   project_dir = Dir(f'./board/obj/{project_name}/')
 
   flags = project["FLAGS"] + extra_flags + common_flags + [
-    "-Wall",
-    "-Wextra",
+    #"-Wall",
+    #"-Wextra",
     "-Wstrict-prototypes",
-    "-Werror",
+    #"-Werror",
     "-mlittle-endian",
     "-mthumb",
     "-nostdlib",

board/config.h

@@ -5,7 +5,7 @@
 //#define DEBUG
 //#define DEBUG_UART
 //#define DEBUG_USB
-//#define DEBUG_SPI
+#define DEBUG_SPI
 //#define DEBUG_FAULTS
 //#define DEBUG_COMMS
 //#define DEBUG_FAN

board/drivers/interrupts.h

@@ -15,6 +15,13 @@ static uint32_t busy_time = 0U;
 float interrupt_load = 0.0f;
 
 void handle_interrupt(IRQn_Type irq_type){
+  if (irq_type != 0x3a && irq_type != 0x3b && irq_type != 0x54) {
+    print("irq: "); puth(irq_type); print("\n");
+  }
+
+  interrupts[irq_type].handler();
+  return;
+
   static uint8_t interrupt_depth = 0U;
   static uint32_t last_time = 0U;
   ENTER_CRITICAL();
@@ -46,6 +53,8 @@ void handle_interrupt(IRQn_Type irq_type){
 
 // Every second
 void interrupt_timer_handler(void) {
+  INTERRUPT_TIMER->SR = 0;
+  return;
   if (INTERRUPT_TIMER->SR != 0U) {
     for (uint16_t i = 0U; i < NUM_INTERRUPTS; i++) {
       // Log IRQ call rate faults

board/drivers/spi.h

@@ -1,5 +1,6 @@
 #pragma once
 
+
 #include "board/drivers/spi_declarations.h"
 #include "board/crc.h"
 
@@ -8,7 +9,6 @@ uint8_t spi_buf_tx[SPI_BUF_SIZE];
 
 uint16_t spi_error_count = 0;
 
-static uint8_t spi_state = SPI_STATE_HEADER;
 static uint16_t spi_data_len_mosi;
 static bool spi_can_tx_ready = false;
 static const unsigned char version_text[] = "VERSION";
@@ -62,8 +62,7 @@ void spi_init(void) {
   llspi_init();
 
   // Start the first packet!
-  spi_state = SPI_STATE_HEADER;
-  llspi_mosi_dma(spi_buf_rx, SPI_HEADER_SIZE);
+  llspi_mosi_dma(spi_buf_rx);
 }
 
 static bool validate_checksum(const uint8_t *data, uint16_t len) {
@@ -77,152 +76,108 @@ static bool validate_checksum(const uint8_t *data, uint16_t len) {
 
 void spi_rx_done(void) {
   uint16_t response_len = 0U;
-  uint8_t next_rx_state = SPI_STATE_HEADER_NACK;
   bool checksum_valid = false;
   static uint8_t spi_endpoint;
   static uint16_t spi_data_len_miso;
 
-  // parse header
-  spi_endpoint = spi_buf_rx[1];
-  spi_data_len_mosi = (spi_buf_rx[3] << 8) | spi_buf_rx[2];
-  spi_data_len_miso = (spi_buf_rx[5] << 8) | spi_buf_rx[4];
-
   if (memcmp(spi_buf_rx, version_text, 7) == 0) {
     response_len = spi_version_packet(spi_buf_tx);
-    next_rx_state = SPI_STATE_HEADER_NACK;;
-  } else if (spi_state == SPI_STATE_HEADER) {
-    checksum_valid = validate_checksum(spi_buf_rx, SPI_HEADER_SIZE);
-    if ((spi_buf_rx[0] == SPI_SYNC_BYTE) && checksum_valid) {
-      // response: ACK and start receiving data portion
-      spi_buf_tx[0] = SPI_HACK;
-      next_rx_state = SPI_STATE_HEADER_ACK;
-      response_len = 1U;
-    } else {
-      // response: NACK and reset state machine
-      #ifdef DEBUG_SPI
-        print("- incorrect header sync or checksum "); hexdump(spi_buf_rx, SPI_HEADER_SIZE);
-      #endif
-      spi_buf_tx[0] = SPI_NACK;
-      next_rx_state = SPI_STATE_HEADER_NACK;
-      response_len = 1U;
+  } else {
+    // parse header
+    spi_endpoint = spi_buf_rx[1];
+    spi_data_len_mosi = (spi_buf_rx[3] << 8) | spi_buf_rx[2];
+    spi_data_len_miso = (spi_buf_rx[5] << 8) | spi_buf_rx[4];
+
+    // Validate header
+    bool header_ok = false;
+    if (spi_buf_rx[0] == SPI_SYNC_BYTE) {
+      header_ok = validate_checksum(spi_buf_rx, SPI_HEADER_SIZE);
     }
-  } else if (spi_state == SPI_STATE_DATA_RX) {
-    // We got everything! Based on the endpoint specified, call the appropriate handler
+
     bool response_ack = false;
-    checksum_valid = validate_checksum(&(spi_buf_rx[SPI_HEADER_SIZE]), spi_data_len_mosi + 1U);
-    if (checksum_valid) {
-      if (spi_endpoint == 0U) {
-        if (spi_data_len_mosi >= sizeof(ControlPacket_t)) {
-          ControlPacket_t ctrl = {0};
-          (void)memcpy((uint8_t*)&ctrl, &spi_buf_rx[SPI_HEADER_SIZE], sizeof(ControlPacket_t));
-          response_len = comms_control_handler(&ctrl, &spi_buf_tx[3]);
-          response_ack = true;
-        } else {
-          print("SPI: insufficient data for control handler\n");
-        }
-      } else if ((spi_endpoint == 1U) || (spi_endpoint == 0x81U)) {
-        if (spi_data_len_mosi == 0U) {
-          response_len = comms_can_read(&(spi_buf_tx[3]), spi_data_len_miso);
-          response_ack = true;
-        } else {
-          print("SPI: did not expect data for can_read\n");
-        }
-      } else if (spi_endpoint == 2U) {
-        comms_endpoint2_write(&spi_buf_rx[SPI_HEADER_SIZE], spi_data_len_mosi);
-        response_ack = true;
-      } else if (spi_endpoint == 3U) {
-        if (spi_data_len_mosi > 0U) {
-          if (spi_can_tx_ready) {
-            spi_can_tx_ready = false;
-            comms_can_write(&spi_buf_rx[SPI_HEADER_SIZE], spi_data_len_mosi);
+    if (header_ok) {
+      // Validate data+checksum. Data checksum byte sits immediately after data
+      checksum_valid = validate_checksum(&(spi_buf_rx[SPI_HEADER_SIZE]), spi_data_len_mosi + 1U);
+
+      if (checksum_valid) {
+        // Dispatch
+        if (spi_endpoint == 0U) {
+          if (spi_data_len_mosi >= sizeof(ControlPacket_t)) {
+            ControlPacket_t ctrl = {0};
+            (void)memcpy((uint8_t*)&ctrl, &spi_buf_rx[SPI_HEADER_SIZE], sizeof(ControlPacket_t));
+            response_len = comms_control_handler(&ctrl, &spi_buf_tx[3]);
             response_ack = true;
           } else {
-            response_ack = false;
-            print("SPI: CAN NACK\n");
+            print("SPI: insufficient data for control handler\n");
           }
+        } else if ((spi_endpoint == 1U) || (spi_endpoint == 0x81U)) {
+          if (spi_data_len_mosi == 0U) {
+            response_len = comms_can_read(&(spi_buf_tx[3]), spi_data_len_miso);
+            response_ack = true;
+          } else {
+            print("SPI: did not expect data for can_read\n");
+          }
+        } else if (spi_endpoint == 2U) {
+          comms_endpoint2_write(&spi_buf_rx[SPI_HEADER_SIZE], spi_data_len_mosi);
+          response_ack = true;
+        } else if (spi_endpoint == 3U) {
+          if (spi_data_len_mosi > 0U) {
+            if (spi_can_tx_ready) {
+              spi_can_tx_ready = false;
+              comms_can_write(&spi_buf_rx[SPI_HEADER_SIZE], spi_data_len_mosi);
+              response_ack = true;
+            } else {
+              response_ack = false;
+              print("SPI: CAN NACK\n");
+            }
+          } else {
+            print("SPI: expected data for can_write\n");
+          }
+        } else if (spi_endpoint == 0xABU) {
+          // test endpoint: mimics panda -> device transfer
+          response_len = spi_data_len_miso;
+          response_ack = true;
+        } else if (spi_endpoint == 0xACU) {
+          // test endpoint: mimics device -> panda transfer (with NACK)
+          response_ack = false;
         } else {
-          print("SPI: did expect data for can_write\n");
+          print("SPI: unexpected endpoint"); puth(spi_endpoint); print("\n");
         }
-      } else if (spi_endpoint == 0xABU) {
-        // test endpoint: mimics panda -> device transfer
-        response_len = spi_data_len_miso;
-        response_ack = true;
-      } else if (spi_endpoint == 0xACU) {
-        // test endpoint: mimics device -> panda transfer (with NACK)
-        response_ack = false;
-      } else {
-        print("SPI: unexpected endpoint"); puth(spi_endpoint); print("\n");
       }
-    } else {
-      // Checksum was incorrect
-      response_ack = false;
-      #ifdef DEBUG_SPI
-        print("- incorrect data checksum ");
-        puth4(spi_data_len_mosi);
-        print("\n");
-        hexdump(spi_buf_rx, SPI_HEADER_SIZE);
-        hexdump(&(spi_buf_rx[SPI_HEADER_SIZE]), MIN(spi_data_len_mosi, 64));
-        print("\n");
-      #endif
+    }
+
+    if (!header_ok || !checksum_valid) {
+      print("error: "); hexdump(spi_buf_rx, 24); print("\n");
+      if (false) puth4(4);
+      spi_error_count += 1U;
     }
 
     if (!response_ack) {
       spi_buf_tx[0] = SPI_NACK;
-      next_rx_state = SPI_STATE_HEADER_NACK;
       response_len = 1U;
     } else {
       // Setup response header
-      spi_buf_tx[0] = SPI_DACK;
+      spi_buf_tx[0] = SPI_ACK;
       spi_buf_tx[1] = response_len & 0xFFU;
       spi_buf_tx[2] = (response_len >> 8) & 0xFFU;
 
-      // Add checksum
+      // Add checksum over header + payload
       uint8_t checksum = SPI_CHECKSUM_START;
-      for(uint16_t i = 0U; i < (response_len + 3U); i++) {
+      for (uint16_t i = 0U; i < (response_len + 3U); i++) {
         checksum ^= spi_buf_tx[i];
       }
       spi_buf_tx[response_len + 3U] = checksum;
+      // response_len now refers to header + payload + checksum
       response_len += 4U;
-
-      next_rx_state = SPI_STATE_DATA_TX;
     }
-  } else {
-    print("SPI: RX unexpected state: "); puth(spi_state); print("\n");
   }
 
-  // send out response
-  if (response_len == 0U) {
-    print("SPI: no response\n");
-    spi_buf_tx[0] = SPI_NACK;
-    spi_state = SPI_STATE_HEADER_NACK;
-    response_len = 1U;
-  }
+  // send out the response
   llspi_miso_dma(spi_buf_tx, response_len);
-
-  spi_state = next_rx_state;
-  if (!checksum_valid) {
-    spi_error_count += 1U;
-  }
 }
 
-void spi_tx_done(bool reset) {
-  if ((spi_state == SPI_STATE_HEADER_NACK) || reset) {
-    // Reset state
-    spi_state = SPI_STATE_HEADER;
-    llspi_mosi_dma(spi_buf_rx, SPI_HEADER_SIZE);
-  } else if (spi_state == SPI_STATE_HEADER_ACK) {
-    // ACK was sent, queue up the RX buf for the data + checksum
-    spi_state = SPI_STATE_DATA_RX;
-    llspi_mosi_dma(&spi_buf_rx[SPI_HEADER_SIZE], spi_data_len_mosi + 1U);
-  } else if (spi_state == SPI_STATE_DATA_TX) {
-    // Reset state
-    spi_state = SPI_STATE_HEADER;
-    llspi_mosi_dma(spi_buf_rx, SPI_HEADER_SIZE);
-  } else {
-    spi_state = SPI_STATE_HEADER;
-    llspi_mosi_dma(spi_buf_rx, SPI_HEADER_SIZE);
-    print("SPI: TX unexpected state: "); puth(spi_state); print("\n");
-  }
+void spi_tx_done() {
+  llspi_mosi_dma(spi_buf_rx);
 }
 
 void can_tx_comms_resume_spi(void) {

board/drivers/spi_declarations.h

@@ -2,43 +2,39 @@
 
 #include "board/crc.h"
 
-#define SPI_TIMEOUT_US 10000U
+#define SPI_TIMEOUT_US 1000U
 
 // got max rate from hitting a non-existent endpoint
 // in a tight loop, plus some buffer
 #define SPI_IRQ_RATE  16000U
 
-#define SPI_BUF_SIZE 4096U
+#define SPI_BUF_SIZE 128U
 // H7 DMA2 located in D2 domain, so we need to use SRAM1/SRAM2
 __attribute__((section(".sram12"))) extern uint8_t spi_buf_rx[SPI_BUF_SIZE];
 __attribute__((section(".sram12"))) extern uint8_t spi_buf_tx[SPI_BUF_SIZE];
 
+// Protocol constants
 #define SPI_CHECKSUM_START 0xABU
 #define SPI_SYNC_BYTE 0x5AU
-#define SPI_HACK 0x79U
-#define SPI_DACK 0x85U
+#define SPI_ACK 0x85U
 #define SPI_NACK 0x1FU
 
-// SPI states
-enum {
-  SPI_STATE_HEADER,
-  SPI_STATE_HEADER_ACK,
-  SPI_STATE_HEADER_NACK,
-  SPI_STATE_DATA_RX,
-  SPI_STATE_DATA_RX_ACK,
-  SPI_STATE_DATA_TX
-};
-
 extern uint16_t spi_error_count;
 
+// Fixed frame contains a 7-byte header followed by data and a 1-byte data checksum.
+//   header: [SYNC(1) | EP(1) | MOSI_LEN(2 LE) | MISO_MAX(2 LE) | HDR_CKSUM(1)]
+//   data:   MOSI_LEN bytes starting at offset SPI_HEADER_SIZE
+//   data checksum byte immediately following data.
+// The device responds in the next frame with:
+//   [DACK(1) | RESP_LEN(2 LE) | RESP_DATA | RESP_CKSUM(1)]
 #define SPI_HEADER_SIZE 7U
 
 // low level SPI prototypes
 void llspi_init(void);
-void llspi_mosi_dma(uint8_t *addr, int len);
+void llspi_mosi_dma(uint8_t *addr);
 void llspi_miso_dma(uint8_t *addr, int len);
 
 void can_tx_comms_resume_spi(void);
 void spi_init(void);
 void spi_rx_done(void);
-void spi_tx_done(bool reset);
+void spi_tx_done(void);

board/drivers/timers.h

@@ -22,10 +22,10 @@ void interrupt_timer_init(void) {
   register_set(&(INTERRUPT_TIMER->DIER), TIM_DIER_UIE, 0x5F5FU);
   register_set(&(INTERRUPT_TIMER->CR1), TIM_CR1_CEN, 0x3FU);
   INTERRUPT_TIMER->SR = 0;
-  NVIC_EnableIRQ(INTERRUPT_TIMER_IRQ);
+  NVIC_DisableIRQ(INTERRUPT_TIMER_IRQ);
 }
 
 void tick_timer_init(void) {
-  timer_init(TICK_TIMER, (uint16_t)((15.25*APB2_TIMER_FREQ)/8U));
-  NVIC_EnableIRQ(TICK_TIMER_IRQ);
+  //timer_init(TICK_TIMER, (uint16_t)((15.25*APB2_TIMER_FREQ)/8U));
+  NVIC_DisableIRQ(TICK_TIMER_IRQ);
 }

board/drivers/uart.h

@@ -136,7 +136,7 @@ static void puth4(unsigned int i) {
 #endif
 
 #if defined(DEBUG_SPI) || defined(BOOTSTUB) || defined(DEBUG_USB) || defined(DEBUG_COMMS)
-static void hexdump(const void *a, int l) {
+void hexdump(const void *a, int l) {
   if (a != NULL) {
     for (int i=0; i < l; i++) {
       if ((i != 0) && ((i & 0xf) == 0)) print("\n");

board/drivers/uart_declarations.h

@@ -35,5 +35,5 @@ void puth(unsigned int i);
 static void puth4(unsigned int i);
 #endif
 #if defined(DEBUG_SPI) || defined(DEBUG_USB) || defined(DEBUG_COMMS)
-static void hexdump(const void *a, int l);
+void hexdump(const void *a, int l);
 #endif

board/main.c

@@ -121,6 +121,11 @@ static void tick_handler(void) {
   static bool relay_malfunction_prev = false;
 
   if (TICK_TIMER->SR != 0U) {
+    /*
+    uint32_t sr = FLASH->SR1;
+    uint32_t cr = FLASH->CR1;
+    print("sr "); puth4(sr); print(" cr "); puth4(cr); print(" lock "); puth(flash_is_locked()); print("\n");
+    */
 
     // siren
     current_board->set_siren((loop_counter & 1U) && (siren_enabled || (siren_countdown > 0U)));