WIP: interrupt based light switch detection for front SN #11

Draft
k.winkels wants to merge 1 commits from interrupt-based-ls into main

View File

@ -60,6 +60,9 @@ FDCAN_TxHeaderTypeDef txHeader;
// Declare buffer in AXI SRAM
static uint16_t adc_values[NUM_ADC_PINS];
static uint8_t dio_values[NUM_DIO_PINS];
#ifdef SN_FRONT
static uint8_t ls_values[2];
#endif /* SN_FRONT */
static uint16_t filtered_values[NUM_ADC_PINS];
@ -85,12 +88,23 @@ void send_latest_can() {
tx_counter++;
#if defined(SN_FRONT)
Review

Vorschlag: DIO_PIN_MAP Type um ein numerisches Feld exti_idx ergänzen, -1 wenn kein interrupt. Dann

  for (int di = 0; di < NUM_DIO_PINS; di++) {
    if (DIO_PIN_MAP[di].exti_idx >= 0) {
      dio_values[di] = ls_values[DIO_PIN_MAP[di].exti_idx];
      ls_values[DIO_PIN_MAP[di].exti_idx] = 0;
    } else {
      dio_values[di] = HAL_GPIO_ReadPin(
        DIO_PIN_MAP[di].port,
        DIO_PIN_MAP[di].pin
      );
    }
  }
Vorschlag: `DIO_PIN_MAP` Type um ein numerisches Feld `exti_idx` ergänzen, -1 wenn kein interrupt. Dann ```C for (int di = 0; di < NUM_DIO_PINS; di++) { if (DIO_PIN_MAP[di].exti_idx >= 0) { dio_values[di] = ls_values[DIO_PIN_MAP[di].exti_idx]; ls_values[DIO_PIN_MAP[di].exti_idx] = 0; } else { dio_values[di] = HAL_GPIO_ReadPin( DIO_PIN_MAP[di].port, DIO_PIN_MAP[di].pin ); } } ```
Review

Würde sowieso nicht den Begriff ls benutzen da das ja ein feature ist was unabhängig von der spezifischen Sensorik nützlich sein kann. Lieber interrupt oder irq oder exti

Würde sowieso nicht den Begriff `ls` benutzen da das ja ein feature ist was unabhängig von der spezifischen Sensorik nützlich sein kann. Lieber `interrupt` oder `irq` oder `exti`
memcpy(dio_values, ls_values, 2);
memset(ls_values, 0, 2);
for (int di = 2; di < NUM_DIO_PINS; di++) { //TODO: find way to make this dependent on mappings
dio_values[di] = HAL_GPIO_ReadPin(
DIO_PIN_MAP[di].port,
DIO_PIN_MAP[di].pin
);
}
#elif defined(SN_REAR)
for (int di = 0; di < NUM_DIO_PINS; di++) {
dio_values[di] = HAL_GPIO_ReadPin(
DIO_PIN_MAP[di].port,
DIO_PIN_MAP[di].pin
);
}
#endif /* SN_FRONT */
for (int pi = 0; pi < NUM_TX_PKT; pi++) {
@ -158,6 +172,16 @@ void filter_adc() {
}
}
#ifdef SN_FRONT
void HAL_GPIO_EXTI_Callback(uint16_t GPIO_Pin) {
if (GPIO_Pin == DIO_PIN_MAP[CAN_SIGNAL_MAP[0].signals[0].channel].pin) {
ls_values[1] = 1;
} else if (GPIO_Pin == DIO_PIN_MAP[CAN_SIGNAL_MAP[0].signals[1].channel].pin) {
Review

Was wenn beides gleichzeitig passiert? Würde das "else" weg lassen. Oder wären dass dann 2 gequeuete interrupts?

Was wenn beides gleichzeitig passiert? Würde das "else" weg lassen. Oder wären dass dann 2 gequeuete interrupts?
ls_values[0] = 1;
}
}
#endif /* SN_FRONT */
/* USER CODE END 0 */
/**
@ -202,6 +226,36 @@ int main(void)
/* Initialize interrupts */
MX_NVIC_Init();
/* USER CODE BEGIN 2 */
// Enable Interrupts for Light Switch Pins
#ifdef SN_FRONT
// check if pin name is "LS L"
if (strcmp(CAN_SIGNAL_MAP[0].signals[0].name, "LS L") != 0
Review

Lieber ne Flag in dem Struct hinzufügen (oder noch besser einen neuen Signal Type definieren) als hier Strings zu vergleichen IMO

Lieber ne Flag in dem Struct hinzufügen (oder noch besser einen neuen Signal Type definieren) als hier Strings zu vergleichen IMO
Review

Achso das ist nur ne Kontrolle. Dann okay i guess

Achso das ist nur ne Kontrolle. Dann okay i guess
|| strcmp(CAN_SIGNAL_MAP[0].signals[1].name, "LS R") != 0
) {
Error_Handler();
}
GPIO_InitTypeDef GPIO_InitStruct = {0};
GPIO_InitStruct.Pin = CAN_SIGNAL_MAP[0].signals[0].channel;
GPIO_InitStruct.Mode = GPIO_MODE_IT_RISING;
GPIO_InitStruct.Pull = GPIO_NOPULL;
HAL_GPIO_Init(DIO_PIN_MAP[CAN_SIGNAL_MAP[0].signals[0].channel].port, &GPIO_InitStruct);
Review

Statt an all diesen Orten immer hardcoded in die Map zu indexieren würde ich eher entweder feste indizes definieren:

#define EXTI_PIN_1_PIN L9
#define EXTI_PIN_1_PIN LA

oder noch besser statt 2 separaten Init-Calls hier direkt durch ein array iterieren:

int EXTI_PINS[2] = {L9, LA};

oder noch einfacher wenn du wie oben das Attribut hinzufügst:

  for (int di = 0; di < NUM_DIO_PINS; di++) {
    if (DIO_PIN_MAP[di].exti_idx >= 0)
      HAL_GPIO_Init(...)
Statt an all diesen Orten immer hardcoded in die Map zu indexieren würde ich eher entweder feste indizes definieren: ``` #define EXTI_PIN_1_PIN L9 #define EXTI_PIN_1_PIN LA ``` oder noch besser statt 2 separaten Init-Calls hier direkt durch ein array iterieren: ``` int EXTI_PINS[2] = {L9, LA}; ``` oder noch einfacher wenn du wie oben das Attribut hinzufügst: ``` for (int di = 0; di < NUM_DIO_PINS; di++) { if (DIO_PIN_MAP[di].exti_idx >= 0) HAL_GPIO_Init(...) ```
GPIO_InitStruct.Pin = CAN_SIGNAL_MAP[0].signals[1].channel;
HAL_GPIO_Init(DIO_PIN_MAP[CAN_SIGNAL_MAP[0].signals[1].channel].port, &GPIO_InitStruct);
// Did not find a way to make this dependent on mappings,
// since the EXTI lines are shared for pins 10-15
// so i just error if the pins are not 10, 15
if (DIO_PIN_MAP[CAN_SIGNAL_MAP[0].signals[0].channel].pin != GPIO_PIN_10
|| DIO_PIN_MAP[CAN_SIGNAL_MAP[0].signals[1].channel].pin != GPIO_PIN_15
) {
Error_Handler();
}
HAL_NVIC_SetPriority(EXTI15_10_IRQn, 0, 0);
HAL_NVIC_EnableIRQ(EXTI15_10_IRQn);
#endif /* SN_FRONT */
hMainCAN = &hfdcan1;
hPeriCAN = &hfdcan2;