HSS control#12
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a high-side switch (HSS) control system for the Power Distribution Unit, including a generic SPI shift register driver and logic for managing output states and fault handling. The review identified several critical issues: a pin definition conflict in LSOM_S_Pins.h, a missing SPI interrupt callback that would lead to deadlocks, and potential race conditions on the global state variable. Feedback also highlighted endianness concerns during SPI transmission, the non-standard use of emojis in enums, and the need for more precise hardware timing instead of using RTOS task delays.
| #define LSOM_13_PIN GPIO_PIN_2 | ||
| // differs from sheet? | ||
| #define LSOM_13_PORT GPIOC | ||
| #define LSOM_13_PIN GPIO_PIN_12 |
There was a problem hiding this comment.
Assigning LSOM_13_PIN to GPIO_PIN_12 on GPIOC creates a conflict, as this pin is already defined for LSOM_15_PIN (line 39). This will lead to hardware contention if both are initialized or used simultaneously.
|
|
||
| ShiftRegister_SPI_HandleTypeDef hsscontrol_sr; | ||
|
|
||
| uint32_t HSS_state = HSSCONTROL_MASK_LATCH; // initialize with all HSS off, all HSS fault latching on |
There was a problem hiding this comment.
HSS_state is a global variable modified by multiple functions (e.g., PDU_Mk1_HSSControl_WriteHSSEnField, PDU_Mk1_HSSControl_AllOn). If these functions are called from different RTOS tasks, a race condition occurs. Access to HSS_state should be protected by a mutex or performed using atomic operations.
There was a problem hiding this comment.
should be chill if you do all the HSS stuff in one task
if u plan on not doing that fsr i think it's better to have a modified hss state that u only write to the global state once when you intend on performing the spi write, u can wrap that in a semaphore and release after the write
|
|
||
| #include "stm32xx_hal.h" | ||
|
|
||
| #define SR_SPI_MUTEX_DELAY_TICKS portMAX_DELAY |
There was a problem hiding this comment.
why portMAX_DELAY here? try to use deterministic delays so you know for sure your mutexes will return and not just hang
if u dont hv an exact value in mind can just choose a larger delay
| typedef enum { | ||
| SR_SPI_😢, // ShiftRegister_SPI sad | ||
| SR_SPI_🙂, // ShiftRegister_SPI happy | ||
| SR_SPI_🕷️, // ShiftRegister_SPI SPI mutex timeout | ||
| SR_SPI_🕸️, // ShiftRegister_SPI_ SPI done semaphore timeout |
| SR_SPI_🙂, // ShiftRegister_SPI happy | ||
| SR_SPI_🕷️, // ShiftRegister_SPI SPI mutex timeout | ||
| SR_SPI_🕸️, // ShiftRegister_SPI_ SPI done semaphore timeout | ||
| } ShiftRegister_SPI_Status_t; |
|
|
||
| // FUNCTION DEFINITIONS ------------------------------------------------------- | ||
|
|
||
| ShiftRegister_SPI_Status_t SR_SPI_Init(ShiftRegister_SPI_HandleTypeDef* sr) |
There was a problem hiding this comment.
gpio init the MCU pins that connect to the SR here
| @@ -0,0 +1,44 @@ | |||
| // ShiftRegister_SPI.h | |||
| // ---------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
can u put the p/n of the shift register somewhere
| bool PDU_Mk1_HSSControl_AllOn() | ||
| { | ||
| HSS_state |= HSSCONTROL_MASK_EN; | ||
| printf("HSS State\n"); |
|
|
||
| ShiftRegister_SPI_HandleTypeDef hsscontrol_sr; | ||
|
|
||
| uint32_t HSS_state = HSSCONTROL_MASK_LATCH; // initialize with all HSS off, all HSS fault latching on |
There was a problem hiding this comment.
should be chill if you do all the HSS stuff in one task
if u plan on not doing that fsr i think it's better to have a modified hss state that u only write to the global state once when you intend on performing the spi write, u can wrap that in a semaphore and release after the write
| // DEFINES -------------------------------------------------------------------- | ||
|
|
||
| #define TASKPRIORITY_INIT tskIDLE_PRIORITY + 2 | ||
| #define TASKSTACKSIZE_INIT configMINIMAL_STACK_SIZE+1500 |
|
|
||
| void Error_Handler(void) | ||
| { | ||
| __disable_irq(); |
There was a problem hiding this comment.
dont need to disable interrupts here, i think stm hal does it by default in their error handlers because they assume the worst case and intend to completely shut off unexpected state changes
| return false; | ||
| } | ||
|
|
||
| PDU_Mk1_HSSControl_FilterFaultedOutputENs(); |
There was a problem hiding this comment.
what does this do? i can't find it anywhere in this pr, only the definition in the header
2 participants
No description provided.