Conversation
Summary of ChangesHello @adiv06, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the groundwork for the vehicle's motor control system by introducing a comprehensive Finite State Machine (FSM) architecture. It establishes the necessary CAN communication interfaces for both receiving driver inputs and sending commands to the motor controller. A critical aspect of this work is the integration of a robust fault handling system and a CAN watchdog, designed to ensure system stability and provide mechanisms for recovery or safe shutdown. The changes also include the setup of FreeRTOS tasks to orchestrate these components, forming a foundational embedded software structure. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 significant architectural change by implementing a motor control finite state machine (FSM) using FreeRTOS. The new architecture includes dedicated tasks for the FSM, fault handling, and a CAN watchdog. While this is a good direction, the code in its current state is not ready for merging. It contains several critical issues that prevent compilation, such as multiple CAN signals sharing the same ID, incorrect definitions of global variables in header files leading to linker errors, and numerous undefined variables and functions. My review focuses on these fundamental blocking issues that need to be addressed before a proper logic review can be conducted.
Firmware/apps/headers/tasks.h
Outdated
|
|
||
| StaticTask_t Task_Watchdog_Buffer; | ||
| StackType_t Task_Watchdog_Stack_Array[configMINIMAL_STACK_SIZE]; | ||
| EventGroupHandle_t xWDogEventGroup_handle; |
There was a problem hiding this comment.
A global variable xWDogEventGroup_handle is defined in this header file. Defining non-static global variables in headers leads to "multiple definition" linker errors if the header is included in more than one .c file. The variable should be declared as extern in the header and defined in exactly one source file (e.g., watchdog.c).
| EventGroupHandle_t xWDogEventGroup_handle; | |
| extern EventGroupHandle_t xWDogEventGroup_handle; |
| } | ||
|
|
||
| void init() { | ||
| assert(FSM_SIGNAL_COUNT != SEND_TRITIUM_IDS); //ensuring signal for every ID |
There was a problem hiding this comment.
This assertion assert(FSM_SIGNAL_COUNT != SEND_TRITIUM_IDS) will always fail and halt the program because both FSM_SIGNAL_COUNT and SEND_TRITIUM_IDS are defined as 8. Based on the comment "ensuring signal for every ID", it seems you intend to check for equality.
assert(FSM_SIGNAL_COUNT == SEND_TRITIUM_IDS); //ensuring signal for every ID
Firmware/apps/src/SendTritium.c
Outdated
|
|
||
| // SendCarCAN_Put( | ||
| // driveCmd); // Send the drive command to the car CAN bus for telemetry | ||
| motor_can |
There was a problem hiding this comment.
There is a dangling identifier motor_can here which will cause a compilation error. This line seems incomplete and should be removed or completed.
Firmware/apps/headers/SendTritium.h
Outdated
| static const uint16_t fsm_signal_to_can_id[FSM_SIGNAL_COUNT] = { | ||
| [FSM_PEDALS] = CAN_ID_PEDALS, | ||
| [FSM_GEARS] = CAN_ID_GEARS, | ||
| [FSM_REGEN_BUTTON] = CAN_ID_REGEN_BUTTON, | ||
| [FSM_REGEN_ENABLED] = CAN_ID_REGEN_ENABLED, | ||
| [FSM_CRUISE_CONTROL] = CAN_ID_CRUISE_CONTROL, | ||
| [FSM_BPS_OK_TO_REGEN] = CAN_ID_BPS_OK_TO_REGEN, | ||
| [FSM_BPS_TRIP] = CAN_ID_BPS_TRIP, | ||
| [FSM_IGNITION_STATE] = CAN_ID_IGNITION_STATE, | ||
| }; |
There was a problem hiding this comment.
The fsm_signal_to_can_id array is defined as static in a header file. This creates a separate instance in every translation unit that includes this header. For a constant lookup table, it's better to declare it as extern const in the header and define it once in the corresponding .c file to save memory and avoid potential issues.
extern const uint16_t fsm_signal_to_can_id[FSM_SIGNAL_COUNT];| uint8_t generateCustomBitfield(BitfieldInputs_t[] inputs) { | ||
| // Inputs are in order of most significant bit to least significant bit | ||
| uint8_t bitfield = 0; | ||
| for (int i = 0; i < sizeof(inputs) / sizeof(inputs[0]); i++) { | ||
| bitfield |= inputs[i]; | ||
| } | ||
| return bitfield; |
There was a problem hiding this comment.
In C, when an array is passed to a function, it decays to a pointer. sizeof(inputs) will therefore give you the size of a pointer, not the number of elements in the array. This will lead to incorrect behavior in the loop. You should pass the size of the array as a separate argument to the function.
uint8_t generateCustomBitfield(BitfieldInputs_t inputs[], size_t num_inputs) {
// Inputs are in order of most significant bit to least significant bit
uint8_t bitfield = 0;
for (size_t i = 0; i < num_inputs; i++) {
bitfield |= inputs[i];
}
return bitfield;
}| } | ||
|
|
||
| //disable the FSM for any fault | ||
| disableFSM(); |
There was a problem hiding this comment.
The function disableFSM() is called here, but it is defined in SendTritium.c and not declared in any header included by this file. This will cause an "implicit function declaration" warning or error during compilation. To fix this, disableFSM() and recoverFSM() (called on line 91) should be declared in SendTritium.h, and faults.c should include SendTritium.h.
| @@ -0,0 +1,58 @@ | |||
| #include "stm32xx_hal.h" | |||
There was a problem hiding this comment.
The include stm32xx_hal.h uses a placeholder xx. This needs to be replaced with the specific STM32 series being used (e.g., f4, h7) for the code to compile.
| #include "stm32xx_hal.h" | |
| #include "stm32f4xx_hal.h" // Example for STM32F4 series |
Firmware/apps/src/watchdog.c
Outdated
| }else{ | ||
| //one or more messages were missing, wake up a fault task | ||
|
|
||
| } |
There was a problem hiding this comment.
The else block for the watchdog check is empty. If not all expected CAN messages are received within the watchdog window, a fault should be triggered here. The comment indicates this is the intended purpose.
}else{
//one or more messages were missing, wake up a fault task
Faults_ThrowFault(FAULT_ID_WATCHDOG_FSM);
}
Firmware/apps/headers/SendTritium.h
Outdated
|
|
||
| #define GEAR_FAULT_THRESHOLD 3 // number of times gear fault can occur before it is considered a fault | ||
|
|
||
| #define ACCCEL_PEDAL_RESET_THRESHOLD 20 |
There was a problem hiding this comment.
There is a typo in the macro name ACCCEL_PEDAL_RESET_THRESHOLD. It should be ACCEL_PEDAL_RESET_THRESHOLD.
#define ACCEL_PEDAL_RESET_THRESHOLD 20
1 participant
DO NOT MERGE YET.
Implementation for motor FSM. I made the architecture so more things can be added later, but just wanted to make sure it looked right.
TODO: