Register readback on most modules. Still need to convert the other on…

…es (#396) * Added an initial working implementation of the register readback and applied it to the LLCAN registers. Also fixed an init bug in CAN. * Locally disable pylint import error * Reverted change to CAN-obj->FA1R setting * Fixed misra compliance * Changed hash function name * Fixed CAN filter setting * Added voltage health test * Converted ADC to register functions * Converted clock to use register setters * Added check to see if fault status is zero after running test * Converted DAC to use register setters * Converted fan to use register setters * Converted gmlan bitbanging to use register setters * Changed over interrupt driver to use register setters. Also moved some includes and definition places for critical sections * Fixed build * Converted LLGPIO to use register setters * Converted pwm to use register setters * Fixed cold boot * Fixed health fault check in automated tests * Added comment for the future (issue #367) * Converted RTC driver to use register setters * Converted SPI to use register setters * Converted timer driver to use register setters * Fixed register fault on white pandas and disabled showing of fault for release * Bump version
commaai · Dec 5, 2019 · 893e486 · 893e486
1 parent 6bbae7b
commit 893e486
Show file tree

Hide file tree

Showing 25 changed files with 306 additions and 153 deletions.
diff --git a/VERSION b/VERSION
@@ -1 +1 @@
-v1.6.8
+v1.6.9
diff --git a/board/bootstub.c b/board/bootstub.c
@@ -29,8 +29,10 @@ const board *current_board;
 // ********************* Includes *********************
 #include "libc.h"
 #include "provision.h"
+#include "critical.h"
 #include "faults.h"
 
+#include "drivers/registers.h"
 #include "drivers/interrupts.h"
 #include "drivers/clock.h"
 #include "drivers/llgpio.h"
@@ -67,7 +69,8 @@ extern void *_app_start[];
 // BOUNTY: $200 coupon on shop.comma.ai or $100 check.
 
 int main(void) {
-  init_interrupts(false);
+  // Init interrupt table
+  init_interrupts(true);
 
   disable_interrupts();
   clock_init();

diff --git a/board/critical.h b/board/critical.h
@@ -0,0 +1,23 @@
+// ********************* Critical section helpers *********************
+volatile bool interrupts_enabled = false;
+
+void enable_interrupts(void) {
+  interrupts_enabled = true;
+  __enable_irq();
+}
+
+void disable_interrupts(void) {
+  interrupts_enabled = false;
+  __disable_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();                                           \
+  }
diff --git a/board/drivers/adc.h b/board/drivers/adc.h
@@ -9,26 +9,16 @@
 #define ADCCHAN_CURRENT 13
 
 void adc_init(void) {
-  // global setup
-  ADC->CCR = ADC_CCR_TSVREFE | ADC_CCR_VBATE;
-  //ADC1->CR2 = ADC_CR2_ADON | ADC_CR2_EOCS | ADC_CR2_DDS;
-  ADC1->CR2 = ADC_CR2_ADON;
-
-  // long
-  //ADC1->SMPR1 = ADC_SMPR1_SMP10 | ADC_SMPR1_SMP11 | ADC_SMPR1_SMP12 | ADC_SMPR1_SMP13;
-  ADC1->SMPR1 = ADC_SMPR1_SMP12 | ADC_SMPR1_SMP13;
+  register_set(&(ADC->CCR), ADC_CCR_TSVREFE | ADC_CCR_VBATE, 0xC30000U);
+  register_set(&(ADC1->CR2), ADC_CR2_ADON, 0xFF7F0F03U);
+  register_set(&(ADC1->SMPR1), ADC_SMPR1_SMP12 | ADC_SMPR1_SMP13, 0x7FFFFFFU);
 }
 
 uint32_t adc_get(unsigned int channel) {
-  // includes length
-  //ADC1->SQR1 = 0;
-
-  // select channel
-  ADC1->JSQR = channel << 15;
-
-  //ADC1->CR1 = ADC_CR1_DISCNUM_0;
-  //ADC1->CR1 = ADC_CR1_EOCIE;
+  // Select channel
+  register_set(&(ADC1->JSQR), (channel << 15U), 0x3FFFFFU);
 
+  // Start conversion
   ADC1->SR &= ~(ADC_SR_JEOC);
   ADC1->CR2 |= ADC_CR2_JSWSTART;
   while (!(ADC1->SR & ADC_SR_JEOC));

diff --git a/board/drivers/clock.h b/board/drivers/clock.h
@@ -1,40 +1,40 @@
 void clock_init(void) {
   // enable external oscillator
-  RCC->CR |= RCC_CR_HSEON;
+  register_set_bits(&(RCC->CR), RCC_CR_HSEON);
   while ((RCC->CR & RCC_CR_HSERDY) == 0);
 
   // divide things
-  RCC->CFGR = RCC_CFGR_HPRE_DIV1 | RCC_CFGR_PPRE2_DIV2 | RCC_CFGR_PPRE1_DIV4;
+  register_set(&(RCC->CFGR), RCC_CFGR_HPRE_DIV1 | RCC_CFGR_PPRE2_DIV2 | RCC_CFGR_PPRE1_DIV4, 0xFF7FFCF3U);
 
   // 16mhz crystal
-  RCC->PLLCFGR = RCC_PLLCFGR_PLLQ_2 | RCC_PLLCFGR_PLLM_3 |
-                 RCC_PLLCFGR_PLLN_6 | RCC_PLLCFGR_PLLN_5 | RCC_PLLCFGR_PLLSRC_HSE;
+  register_set(&(RCC->PLLCFGR), RCC_PLLCFGR_PLLQ_2 | RCC_PLLCFGR_PLLM_3 | RCC_PLLCFGR_PLLN_6 | RCC_PLLCFGR_PLLN_5 | RCC_PLLCFGR_PLLSRC_HSE, 0x7F437FFFU);
 
   // start PLL
-  RCC->CR |= RCC_CR_PLLON;
+  register_set_bits(&(RCC->CR), RCC_CR_PLLON);
   while ((RCC->CR & RCC_CR_PLLRDY) == 0);
 
   // Configure Flash prefetch, Instruction cache, Data cache and wait state
   // *** without this, it breaks ***
-  FLASH->ACR = FLASH_ACR_ICEN | FLASH_ACR_DCEN | FLASH_ACR_LATENCY_5WS;
+  register_set(&(FLASH->ACR), FLASH_ACR_ICEN | FLASH_ACR_DCEN | FLASH_ACR_LATENCY_5WS, 0x1F0FU);
 
   // switch to PLL
-  RCC->CFGR |= RCC_CFGR_SW_PLL;
+  register_set_bits(&(RCC->CFGR), RCC_CFGR_SW_PLL);
   while ((RCC->CFGR & RCC_CFGR_SWS) != RCC_CFGR_SWS_PLL);
 
   // *** running on PLL ***
 }
 
 void watchdog_init(void) {
   // setup watchdog
-  IWDG->KR = 0x5555;
-  IWDG->PR = 0;          // divider /4
+  IWDG->KR = 0x5555U;
+  register_set(&(IWDG->PR), 0x0U, 0x7U);  // divider/4
+
   // 0 = 0.125 ms, let's have a 50ms watchdog
-  IWDG->RLR = 400 - 1;
-  IWDG->KR = 0xCCCC;
+  register_set(&(IWDG->RLR), (400U-1U), 0xFFFU);
+  IWDG->KR = 0xCCCCU;
 }
 
 void watchdog_feed(void) {
-  IWDG->KR = 0xAAAA;
+  IWDG->KR = 0xAAAAU;
 }
 
diff --git a/board/drivers/dac.h b/board/drivers/dac.h
@@ -2,22 +2,19 @@ void puth(unsigned int i);
 void puts(const char *a);
 
 void dac_init(void) {
-  // no buffers required since we have an opamp
-  //DAC->CR = DAC_CR_EN1 | DAC_CR_BOFF1 | DAC_CR_EN2 | DAC_CR_BOFF2;
-  DAC->DHR12R1 = 0;
-  DAC->DHR12R2 = 0;
-  DAC->CR = DAC_CR_EN1 | DAC_CR_EN2;
+  // No buffers required since we have an opamp
+  register_set(&(DAC->DHR12R1), 0U, 0xFFFU);
+  register_set(&(DAC->DHR12R2), 0U, 0xFFFU);
+  register_set(&(DAC->CR), DAC_CR_EN1 | DAC_CR_EN2, 0x3FFF3FFFU);
 }
 
 void dac_set(int channel, uint32_t value) {
   if (channel == 0) {
-    DAC->DHR12R1 = value;
+    register_set(&(DAC->DHR12R1), value, 0xFFFU);
   } else if (channel == 1) {
-    DAC->DHR12R2 = value;
+    register_set(&(DAC->DHR12R2), value, 0xFFFU);
   } else {
-    puts("Failed to set DAC: invalid channel value: ");
-    puth(value);
-    puts("\n");
+    puts("Failed to set DAC: invalid channel value: 0x"); puth(value); puts("\n");
   }
 }
 
diff --git a/board/drivers/fan.h b/board/drivers/fan.h
@@ -31,9 +31,9 @@ void fan_init(void){
     pwm_init(TIM3, 3);
 
     // Init TACH interrupt
-    SYSCFG->EXTICR[0] = SYSCFG_EXTICR1_EXTI2_PD;
-    EXTI->IMR |= (1U << 2);
-    EXTI->RTSR |= (1U << 2);
-    EXTI->FTSR |= (1U << 2);
+    register_set(&(SYSCFG->EXTICR[0]), SYSCFG_EXTICR1_EXTI2_PD, 0xF00U);
+    register_set_bits(&(EXTI->IMR), (1U << 2));
+    register_set_bits(&(EXTI->RTSR), (1U << 2));
+    register_set_bits(&(EXTI->FTSR), (1U << 2));
     NVIC_EnableIRQ(EXTI2_IRQn);
 }
diff --git a/board/drivers/gmlan_alt.h b/board/drivers/gmlan_alt.h
@@ -124,15 +124,15 @@ int get_bit_message(char *out, CAN_FIFOMailBox_TypeDef *to_bang) {
 
 void setup_timer4(void) {
   // setup
-  TIM4->PSC = 48-1;          // tick on 1 us
-  TIM4->CR1 = TIM_CR1_CEN;   // enable
-  TIM4->ARR = 30-1;          // 33.3 kbps
+  register_set(&(TIM4->PSC), (48-1), 0xFFFFU);    // Tick on 1 us
+  register_set(&(TIM4->CR1), TIM_CR1_CEN, 0x3FU); // Enable
+  register_set(&(TIM4->ARR), (30-1), 0xFFFFU);   // 33.3 kbps
 
   // in case it's disabled
   NVIC_EnableIRQ(TIM4_IRQn);
 
   // run the interrupt
-  TIM4->DIER = TIM_DIER_UIE; // update interrupt
+  register_set(&(TIM4->DIER), TIM_DIER_UIE, 0x5F5FU); // Update interrupt
   TIM4->SR = 0;
 }
 
@@ -171,9 +171,9 @@ void reset_gmlan_switch_timeout(void) {
 
 void set_bitbanged_gmlan(int val) {
   if (val != 0) {
-    GPIOB->ODR |= (1U << 13);
+    register_set_bits(&(GPIOB->ODR), (1U << 13));
   } else {
-    GPIOB->ODR &= ~(1U << 13);
+    register_clear_bits(&(GPIOB->ODR), (1U << 13));
   }
 }
 
@@ -231,8 +231,8 @@ void TIM4_IRQ_Handler(void) {
       if ((gmlan_sending == gmlan_sendmax) || (gmlan_fail_count == MAX_FAIL_COUNT)) {
         set_bitbanged_gmlan(1); // recessive
         set_gpio_mode(GPIOB, 13, MODE_INPUT);
-        TIM4->DIER = 0;  // no update interrupt
-        TIM4->CR1 = 0;   // disable timer
+        register_clear_bits(&(TIM4->DIER), TIM_DIER_UIE); // No update interrupt
+        register_set(&(TIM4->CR1), 0U, 0x3FU); // Disable timer
         gmlan_sendmax = -1;   // exit
       }
     }

diff --git a/board/drivers/interrupts.h b/board/drivers/interrupts.h
@@ -1,29 +1,3 @@
-// ********************* Interrupt helpers *********************
-volatile bool interrupts_enabled = false;
-
-void enable_interrupts(void) {
-  interrupts_enabled = true;
-  __enable_irq();
-}
-
-void disable_interrupts(void) {
-  interrupts_enabled = false;
-  __disable_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();                                           \
-  }
-
-// ********************* Interrupt handling *********************
-
 typedef struct interrupt {
   IRQn_Type irq_type;
   void (*handler)(void);
@@ -79,11 +53,11 @@ void init_interrupts(bool check_rate_limit){
   }
 
   // Init timer 10 for a 1s interval
-  RCC->APB1ENR |= RCC_APB1ENR_TIM6EN;  // enable interrupt timer peripheral
+  register_set_bits(&(RCC->APB1ENR), RCC_APB1ENR_TIM6EN);  // Enable interrupt timer peripheral
   REGISTER_INTERRUPT(TIM6_DAC_IRQn, TIM6_DAC_IRQ_Handler, 1, FAULT_INTERRUPT_RATE_INTERRUPTS)
-  TIM6->PSC = 732-1;
-  TIM6->DIER = TIM_DIER_UIE;
-  TIM6->CR1 = TIM_CR1_CEN;
+  register_set(&(TIM6->PSC), (732-1), 0xFFFFU);
+  register_set(&(TIM6->DIER), TIM_DIER_UIE, 0x5F5FU);
+  register_set(&(TIM6->CR1), TIM_CR1_CEN, 0x3FU);
   TIM6->SR = 0;
   NVIC_EnableIRQ(TIM6_DAC_IRQn);
 }

diff --git a/board/drivers/llcan.h b/board/drivers/llcan.h
@@ -19,25 +19,24 @@ void puts(const char *a);
 
 bool llcan_set_speed(CAN_TypeDef *CAN_obj, uint32_t speed, bool loopback, bool silent) {
   // initialization mode
-  CAN_obj->MCR = CAN_MCR_TTCM | CAN_MCR_INRQ;
+  register_set(&(CAN_obj->MCR), CAN_MCR_TTCM | CAN_MCR_INRQ, 0x180FFU);
   while((CAN_obj->MSR & CAN_MSR_INAK) != CAN_MSR_INAK);
 
   // set time quanta from defines
-  CAN_obj->BTR = (CAN_BTR_TS1_0 * (CAN_SEQ1-1)) |
+  register_set(&(CAN_obj->BTR), ((CAN_BTR_TS1_0 * (CAN_SEQ1-1)) |
             (CAN_BTR_TS2_0 * (CAN_SEQ2-1)) |
-            (can_speed_to_prescaler(speed) - 1U);
+            (can_speed_to_prescaler(speed) - 1U)), 0xC37F03FFU);
 
   // silent loopback mode for debugging
   if (loopback) {
-    CAN_obj->BTR |= CAN_BTR_SILM | CAN_BTR_LBKM;
+    register_set_bits(&(CAN_obj->BTR), CAN_BTR_SILM | CAN_BTR_LBKM);
   }
   if (silent) {
-    CAN_obj->BTR |= CAN_BTR_SILM;
+    register_set_bits(&(CAN_obj->BTR), CAN_BTR_SILM);
   }
 
   // reset
-  // cppcheck-suppress redundantAssignment ; it's a register
-  CAN_obj->MCR = CAN_MCR_TTCM | CAN_MCR_ABOM;
+  register_set(&(CAN_obj->MCR), CAN_MCR_TTCM | CAN_MCR_ABOM, 0x180FFU);
 
   #define CAN_TIMEOUT 1000000
   int tmp = 0;
@@ -51,20 +50,25 @@ bool llcan_set_speed(CAN_TypeDef *CAN_obj, uint32_t speed, bool loopback, bool s
 }
 
 void llcan_init(CAN_TypeDef *CAN_obj) {
-  // accept all filter
-  CAN_obj->FMR |= CAN_FMR_FINIT;
+  // Enter init mode
+  register_set_bits(&(CAN_obj->FMR), CAN_FMR_FINIT);
+
+  // Wait for INAK bit to be set
+  while(((CAN_obj->MSR & CAN_MSR_INAK) == CAN_MSR_INAK)) {}
 
   // no mask
-  CAN_obj->sFilterRegister[0].FR1 = 0;
-  CAN_obj->sFilterRegister[0].FR2 = 0;
-  CAN_obj->sFilterRegister[14].FR1 = 0;
-  CAN_obj->sFilterRegister[14].FR2 = 0;
+  // For some weird reason some of these registers do not want to set properly on CAN2 and CAN3. Probably something to do with the single/dual mode and their different filters.
+  CAN_obj->sFilterRegister[0].FR1 = 0U;
+  CAN_obj->sFilterRegister[0].FR2 = 0U;
+  CAN_obj->sFilterRegister[14].FR1 = 0U;
+  CAN_obj->sFilterRegister[14].FR2 = 0U;
   CAN_obj->FA1R |= 1U | (1U << 14);
 
-  CAN_obj->FMR &= ~(CAN_FMR_FINIT);
+  // Exit init mode, do not wait
+  register_clear_bits(&(CAN_obj->FMR), CAN_FMR_FINIT);
 
   // enable certain CAN interrupts
-  CAN_obj->IER |= CAN_IER_TMEIE | CAN_IER_FMPIE0 |  CAN_IER_WKUIE;
+  register_set_bits(&(CAN_obj->IER), CAN_IER_TMEIE | CAN_IER_FMPIE0 |  CAN_IER_WKUIE);
 
   if (CAN_obj == CAN1) {
     NVIC_EnableIRQ(CAN1_TX_IRQn);
@@ -87,7 +91,7 @@ void llcan_init(CAN_TypeDef *CAN_obj) {
 
 void llcan_clear_send(CAN_TypeDef *CAN_obj) {
   CAN_obj->TSR |= CAN_TSR_ABRQ0;
-  CAN_obj->MSR &= ~(CAN_MSR_ERRI);
+  register_clear_bits(&(CAN_obj->MSR), CAN_MSR_ERRI);
   // cppcheck-suppress selfAssignment ; needed to clear the register
   CAN_obj->MSR = CAN_obj->MSR;
 }

diff --git a/board/drivers/llgpio.h b/board/drivers/llgpio.h
@@ -15,16 +15,16 @@ void set_gpio_mode(GPIO_TypeDef *GPIO, unsigned int pin, unsigned int mode) {
   uint32_t tmp = GPIO->MODER;
   tmp &= ~(3U << (pin * 2U));
   tmp |= (mode << (pin * 2U));
-  GPIO->MODER = tmp;
+  register_set(&(GPIO->MODER), tmp, 0xFFFFFFFFU);
   EXIT_CRITICAL();
 }
 
 void set_gpio_output(GPIO_TypeDef *GPIO, unsigned int pin, bool enabled) {
   ENTER_CRITICAL();
   if (enabled) {
-    GPIO->ODR |= (1U << pin);
+    register_set_bits(&(GPIO->ODR), (1U << pin));
   } else {
-    GPIO->ODR &= ~(1U << pin);
+    register_clear_bits(&(GPIO->ODR), (1U << pin));
   }
   set_gpio_mode(GPIO, pin, MODE_OUTPUT);
   EXIT_CRITICAL();
@@ -33,9 +33,9 @@ void set_gpio_output(GPIO_TypeDef *GPIO, unsigned int pin, bool enabled) {
 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);
+    register_set_bits(&(GPIO->OTYPER), (1U << pin));
   } else {
-    GPIO->OTYPER &= ~(1U << pin);
+    register_clear_bits(&(GPIO->OTYPER), (1U << pin));
   }
   EXIT_CRITICAL();
 }
@@ -45,7 +45,7 @@ void set_gpio_alternate(GPIO_TypeDef *GPIO, unsigned int pin, unsigned int mode)
   uint32_t tmp = GPIO->AFR[pin >> 3U];
   tmp &= ~(0xFU << ((pin & 7U) * 4U));
   tmp |= mode << ((pin & 7U) * 4U);
-  GPIO->AFR[pin >> 3] = tmp;
+  register_set(&(GPIO->AFR[pin >> 3]), tmp, 0xFFFFFFFFU);
   set_gpio_mode(GPIO, pin, MODE_ALTERNATE);
   EXIT_CRITICAL();
 }
@@ -55,7 +55,7 @@ void set_gpio_pullup(GPIO_TypeDef *GPIO, unsigned int pin, unsigned int mode) {
   uint32_t tmp = GPIO->PUPDR;
   tmp &= ~(3U << (pin * 2U));
   tmp |= (mode << (pin * 2U));
-  GPIO->PUPDR = tmp;
+  register_set(&(GPIO->PUPDR), tmp, 0xFFFFFFFFU);
   EXIT_CRITICAL();
 }