Gpio race condition fix (#263)

* Fixed pedal not initializing

* Interrupt changes

* More changes

Author: robbederks

board/board.h

@@ -48,8 +48,12 @@ void detect_configuration(void) {
   has_external_debug_serial = detect_with_pull(GPIOA, 3, PULL_DOWN);
 
   #ifdef PANDA
-    // check if the ESP is trying to put me in boot mode
-    is_entering_bootmode = !detect_with_pull(GPIOB, 0, PULL_UP);
+    if(hw_type == HW_TYPE_WHITE_PANDA) {
+      // check if the ESP is trying to put me in boot mode
+      is_entering_bootmode = !detect_with_pull(GPIOB, 0, PULL_UP);
+    } else {
+      is_entering_bootmode = 0;
+    }
   #else
     is_entering_bootmode = 0;
   #endif
@@ -58,4 +62,4 @@ void detect_configuration(void) {
 // ///// Board functions ///// //
 bool board_has_gps(void) {
   return ((hw_type == HW_TYPE_GREY_PANDA) || (hw_type == HW_TYPE_BLACK_PANDA));
-}
+}

board/bootstub.c

@@ -63,7 +63,7 @@ extern void *_app_start[];
 // BOUNTY: $200 coupon on shop.comma.ai or $100 check.
 
 int main(void) {
-  __disable_irq();
+  disable_interrupts();
   clock_init();
   detect_configuration();
   detect_board_type();

board/drivers/can.h

@@ -62,7 +62,7 @@ int can_overflow_cnt = 0;
 bool can_pop(can_ring *q, CAN_FIFOMailBox_TypeDef *elem) {
   bool ret = 0;
 
-  enter_critical_section();
+  ENTER_CRITICAL();
   if (q->w_ptr != q->r_ptr) {
     *elem = q->elems[q->r_ptr];
     if ((q->r_ptr + 1U) == q->fifo_size) {
@@ -72,7 +72,7 @@ bool can_pop(can_ring *q, CAN_FIFOMailBox_TypeDef *elem) {
     }
     ret = 1;
   }
-  exit_critical_section();
+  EXIT_CRITICAL();
 
   return ret;
 }
@@ -81,7 +81,7 @@ bool can_push(can_ring *q, CAN_FIFOMailBox_TypeDef *elem) {
   bool ret = false;
   uint32_t next_w_ptr;
 
-  enter_critical_section();
+  ENTER_CRITICAL();
   if ((q->w_ptr + 1U) == q->fifo_size) {
     next_w_ptr = 0;
   } else {
@@ -92,7 +92,7 @@ bool can_push(can_ring *q, CAN_FIFOMailBox_TypeDef *elem) {
     q->w_ptr = next_w_ptr;
     ret = true;
   }
-  exit_critical_section();
+  EXIT_CRITICAL();
   if (!ret) {
     can_overflow_cnt++;
     #ifdef DEBUG
@@ -103,10 +103,10 @@ bool can_push(can_ring *q, CAN_FIFOMailBox_TypeDef *elem) {
 }
 
 void can_clear(can_ring *q) {
-  enter_critical_section();
+  ENTER_CRITICAL();
   q->w_ptr = 0;
   q->r_ptr = 0;
-  exit_critical_section();
+  EXIT_CRITICAL();
 }
 
 // assign CAN numbering
@@ -124,7 +124,7 @@ uint8_t bus_lookup[] = {0,1,2};
 uint8_t can_num_lookup[] = {0,1,2,-1};
 int8_t can_forwarding[] = {-1,-1,-1,-1};
 uint32_t can_speed[] = {5000, 5000, 5000, 333};
-#define CAN_MAX 3
+#define CAN_MAX 3U
 
 #define CANIF_FROM_CAN_NUM(num) (cans[num])
 #define CAN_NUM_FROM_CANIF(CAN) ((CAN)==CAN1 ? 0 : ((CAN) == CAN2 ? 1 : 2))
@@ -158,9 +158,10 @@ void can_init(uint8_t can_number) {
 }
 
 void can_init_all(void) {
-  for (int i=0; i < CAN_MAX; i++) {
+  for (uint8_t i=0U; i < CAN_MAX; i++) {
     can_init(i);
   }
+  current_board->enable_can_transcievers(true);
 }
 
 void can_flip_buses(uint8_t bus1, uint8_t bus2){
@@ -248,7 +249,7 @@ void can_set_obd(uint8_t harness_orientation, bool obd){
 
 // CAN error
 void can_sce(CAN_TypeDef *CAN) {
-  enter_critical_section();
+  ENTER_CRITICAL();
 
   #ifdef DEBUG
     if (CAN==CAN1) puts("CAN1:  ");
@@ -271,15 +272,15 @@ void can_sce(CAN_TypeDef *CAN) {
 
   can_err_cnt += 1;
   llcan_clear_send(CAN);
-  exit_critical_section();
+  EXIT_CRITICAL();
 }
 
 // ***************************** CAN *****************************
 
 void process_can(uint8_t can_number) {
   if (can_number != 0xffU) {
 
-    enter_critical_section();
+    ENTER_CRITICAL();
 
     CAN_TypeDef *CAN = CANIF_FROM_CAN_NUM(can_number);
     uint8_t bus_number = BUS_NUM_FROM_CAN_NUM(can_number);
@@ -327,7 +328,7 @@ void process_can(uint8_t can_number) {
       }
     }
 
-    exit_critical_section();
+    EXIT_CRITICAL();
   }
 }
 

board/drivers/llgpio.h

@@ -11,42 +11,52 @@
 #define OUTPUT_TYPE_OPEN_DRAIN 1U
 
 void set_gpio_mode(GPIO_TypeDef *GPIO, unsigned int pin, unsigned int mode) {
+  ENTER_CRITICAL();
   uint32_t tmp = GPIO->MODER;
   tmp &= ~(3U << (pin * 2U));
   tmp |= (mode << (pin * 2U));
   GPIO->MODER = tmp;
+  EXIT_CRITICAL();
 }
 
 void set_gpio_output(GPIO_TypeDef *GPIO, unsigned int pin, bool enabled) {
+  ENTER_CRITICAL();
   if (enabled) {
     GPIO->ODR |= (1U << pin);
   } else {
     GPIO->ODR &= ~(1U << pin);
   }
   set_gpio_mode(GPIO, pin, MODE_OUTPUT);
+  EXIT_CRITICAL();
 }
 
 void set_gpio_output_type(GPIO_TypeDef *GPIO, unsigned int pin, unsigned int output_type){
+  ENTER_CRITICAL();
   if(output_type == OUTPUT_TYPE_OPEN_DRAIN) {
     GPIO->OTYPER |= (1U << pin);
   } else {
     GPIO->OTYPER &= ~(1U << pin);
   }
+  EXIT_CRITICAL();
 }
 
 void set_gpio_alternate(GPIO_TypeDef *GPIO, unsigned int pin, unsigned int mode) {
+  ENTER_CRITICAL();
   uint32_t tmp = GPIO->AFR[pin >> 3U];
   tmp &= ~(0xFU << ((pin & 7U) * 4U));
   tmp |= mode << ((pin & 7U) * 4U);
   GPIO->AFR[pin >> 3] = tmp;
   set_gpio_mode(GPIO, pin, MODE_ALTERNATE);
+  EXIT_CRITICAL();
 }
 
 void set_gpio_pullup(GPIO_TypeDef *GPIO, unsigned int pin, unsigned int mode) {
+  ENTER_CRITICAL();
   uint32_t tmp = GPIO->PUPDR;
   tmp &= ~(3U << (pin * 2U));
   tmp |= (mode << (pin * 2U));
   GPIO->PUPDR = tmp;
+  EXIT_CRITICAL();
 }
 
 int get_gpio_input(GPIO_TypeDef *GPIO, unsigned int pin) {

board/drivers/uart.h

@@ -74,7 +74,7 @@ uart_ring *get_ring_by_number(int a) {
 // ***************************** serial port *****************************
 
 void uart_ring_process(uart_ring *q) {
-  enter_critical_section();
+  ENTER_CRITICAL();
   // TODO: check if external serial is connected
   int sr = q->uart->SR;
 
@@ -108,7 +108,7 @@ void uart_ring_process(uart_ring *q) {
     // set dropped packet flag?
   }
 
-  exit_critical_section();
+  EXIT_CRITICAL();
 }
 
 // interrupt boilerplate
@@ -121,13 +121,13 @@ void UART5_IRQHandler(void) { uart_ring_process(&lin1_ring); }
 bool getc(uart_ring *q, char *elem) {
   bool ret = false;
 
-  enter_critical_section();
+  ENTER_CRITICAL();
   if (q->w_ptr_rx != q->r_ptr_rx) {
     if (elem != NULL) *elem = q->elems_rx[q->r_ptr_rx];
     q->r_ptr_rx = (q->r_ptr_rx + 1U) % FIFO_SIZE;
     ret = true;
   }
-  exit_critical_section();
+  EXIT_CRITICAL();
 
   return ret;
 }
@@ -136,14 +136,14 @@ bool injectc(uart_ring *q, char elem) {
   int ret = false;
   uint16_t next_w_ptr;
 
-  enter_critical_section();
+  ENTER_CRITICAL();
   next_w_ptr = (q->w_ptr_rx + 1U) % FIFO_SIZE;
   if (next_w_ptr != q->r_ptr_rx) {
     q->elems_rx[q->w_ptr_rx] = elem;
     q->w_ptr_rx = next_w_ptr;
     ret = true;
   }
-  exit_critical_section();
+  EXIT_CRITICAL();
 
   return ret;
 }
@@ -152,14 +152,14 @@ bool putc(uart_ring *q, char elem) {
   bool ret = false;
   uint16_t next_w_ptr;
 
-  enter_critical_section();
+  ENTER_CRITICAL();
   next_w_ptr = (q->w_ptr_tx + 1U) % FIFO_SIZE;
   if (next_w_ptr != q->r_ptr_tx) {
     q->elems_tx[q->w_ptr_tx] = elem;
     q->w_ptr_tx = next_w_ptr;
     ret = true;
   }
-  exit_critical_section();
+  EXIT_CRITICAL();
 
   uart_ring_process(q);
 
@@ -185,12 +185,12 @@ void uart_send_break(uart_ring *u) {
 }
 
 void clear_uart_buff(uart_ring *q) {
-  enter_critical_section();
+  ENTER_CRITICAL();
   q->w_ptr_tx = 0;
   q->r_ptr_tx = 0;
   q->w_ptr_rx = 0;
   q->r_ptr_rx = 0;
-  exit_critical_section();
+  EXIT_CRITICAL();
 }
 
 // ***************************** start UART code *****************************
@@ -215,7 +215,7 @@ char usart1_dma[USART1_DMA_LEN];
 void uart_dma_drain(void) {
   uart_ring *q = &esp_ring;
 
-  enter_critical_section();
+  ENTER_CRITICAL();
 
   if ((DMA2->HISR & DMA_HISR_TCIF5) || (DMA2->HISR & DMA_HISR_HTIF5) || (DMA2_Stream5->NDTR != USART1_DMA_LEN)) {
     // disable DMA
@@ -245,7 +245,7 @@ void uart_dma_drain(void) {
     q->uart->CR3 |= USART_CR3_DMAR;
   }
 
-  exit_critical_section();
+  EXIT_CRITICAL();
 }
 
 void DMA2_Stream5_IRQHandler(void) {

board/gpio.h

@@ -20,6 +20,12 @@ void jump_to_bootloader(void) {
 }
 
 void early(void) {
+  // Reset global critical depth
+  global_critical_depth = 0;
+
+  // neccesary for DFU flashing on a non-power cycled white panda
+  enable_interrupts();
+
   // after it's been in the bootloader, things are initted differently, so we reset
   if ((enter_bootloader_mode != BOOT_NORMAL) &&
       (enter_bootloader_mode != ENTER_BOOTLOADER_MAGIC) &&

board/libc.h

@@ -42,24 +42,26 @@ int memcmp(const void * ptr1, const void * ptr2, unsigned int num) {
 
 // ********************* IRQ helpers *********************
 
-int interrupts_enabled = 0;
+volatile bool interrupts_enabled = false;
+
 void enable_interrupts(void) {
-  interrupts_enabled = 1;
+  interrupts_enabled = true;
   __enable_irq();
 }
 
-int critical_depth = 0;
-void enter_critical_section(void) {
+void disable_interrupts(void) {
+  interrupts_enabled = false;
   __disable_irq();
-  // this is safe because interrupts are disabled
-  critical_depth += 1;
 }
 
-void exit_critical_section(void) {
-  // this is safe because interrupts are disabled
-  critical_depth -= 1;
-  if ((critical_depth == 0) && interrupts_enabled) {
-    __enable_irq();
+uint8_t global_critical_depth = 0U;
+#define ENTER_CRITICAL()                                      \
+  __disable_irq();                                            \
+  global_critical_depth += 1U;
+
+#define EXIT_CRITICAL()                                       \
+  global_critical_depth -= 1U;                                \
+  if ((global_critical_depth == 0U) && interrupts_enabled) {  \
+    __enable_irq();                                           \
   }
-}
 

board/main.c

@@ -645,7 +645,7 @@ void TIM3_IRQHandler(void) {
 
 int main(void) {
   // shouldn't have interrupts here, but just in case
-  __disable_irq();
+  disable_interrupts();
 
   // init early devices
   clock_init();

board/pedal/main.c

@@ -55,18 +55,18 @@ void debug_ring_callback(uart_ring *ring) {
   }
 }
 
-int usb_cb_ep1_in(uint8_t *usbdata, int len, bool hardwired) {
+int usb_cb_ep1_in(void *usbdata, int len, bool hardwired) {
   UNUSED(usbdata);
   UNUSED(len);
   UNUSED(hardwired);
   return 0;
 }
-void usb_cb_ep2_out(uint8_t *usbdata, int len, bool hardwired) {
+void usb_cb_ep2_out(void *usbdata, int len, bool hardwired) {
   UNUSED(usbdata);
   UNUSED(len);
   UNUSED(hardwired);
 }
-void usb_cb_ep3_out(uint8_t *usbdata, int len, bool hardwired) {
+void usb_cb_ep3_out(void *usbdata, int len, bool hardwired) {
   UNUSED(usbdata);
   UNUSED(len);
   UNUSED(hardwired);
@@ -299,7 +299,7 @@ void pedal(void) {
 }
 
 int main(void) {
-  __disable_irq();
+  disable_interrupts();
 
   // init devices
   clock_init();
@@ -334,7 +334,7 @@ int main(void) {
   watchdog_init();
 
   puts("**** INTERRUPTS ON ****\n");
-  __enable_irq();
+  enable_interrupts();
 
   // main pedal loop
   while (1) {