Conversation
Derv6464
requested review from
a team,
batrick905 and
Copilot
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This PR introduces “startup” initialization hooks for the firmware tasks and begins standardizing task loop delays via named constants, while also removing editor-specific VS Code configuration files.
Changes:
- Add
init()methods to CAN, Logger, and Canards Controller tasks and invoke them frommain(). - Replace hardcoded
osDelay(...)values with header-defined delay constants. - Remove auto-generated
.vscodeconfiguration files from the firmware directory.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| firmware/src/tasks/logger.h | Adds LOGGER_HZ constant and Logger::init() declaration. |
| firmware/src/tasks/logger.cpp | Uses LOGGER_HZ in task loop delay; adds Logger::init() stub. |
| firmware/src/tasks/canards_controller.h | Adds CANARD_HZ constant and Canards_Controller::init() declaration. |
| firmware/src/tasks/canards_controller.cpp | Uses CANARD_HZ in task loop delay; adds Canards_Controller::init() stub. |
| firmware/src/tasks/can.h | Adds CAN_HZ constant and CAN::init() declaration. |
| firmware/src/tasks/can.cpp | Uses CAN_HZ in task loop delay; adds CAN::init() stub. |
| firmware/src/main.cpp | Calls the new init() methods and stores their results. |
| firmware/.vscode/launch.json | Removed PlatformIO debug launch configuration. |
| firmware/.vscode/extensions.json | Removed VS Code extension recommendations. |
| firmware/.vscode/c_cpp_properties.json | Removed auto-generated IntelliSense configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uint32_t msg; | ||
| osMessageQueueGet(&logger_queue_, &msg, 0, 0); | ||
| //storage_.write(&msg); | ||
| printf("Logged message: %u\n", msg); | ||
| osDelay(200); | ||
| osDelay(LOGGER_HZ); |
There was a problem hiding this comment.
Logger queue usage looks incorrect and can lead to undefined behavior:
- The logging queue is created with element size
sizeof(task::Logger::LogMessage)(see main.cpp), but here you receive into auint32_t. osMessageQueueGetexpects anosMessageQueueId_t(not the address of it) and should have its return status checked (especially with a 0 timeout) before using/printing the received value.
Consider receiving aLogMessage(or change the queue element type) and callosMessageQueueGet(logger_queue_, ...)with a blocking timeout or anosOKcheck.
| #define LOGGER_HZ 200U //ticks | ||
|
|
||
| namespace task{ |
There was a problem hiding this comment.
LOGGER_HZ is defined as a delay in RTOS ticks (//ticks) but the name suggests a frequency in Hertz. This is easy to misread and can cause incorrect timing changes later (e.g., someone might set it to 200 expecting 200 Hz when it actually delays 200 ticks). Consider renaming to something like LOGGER_DELAY_TICKS/LOGGER_PERIOD_TICKS and prefer static constexpr/inline constexpr over a global #define in a header to avoid macro pollution.
| #define LOGGER_HZ 200U //ticks | |
| namespace task{ | |
| namespace task{ | |
| static constexpr uint32_t LOGGER_PERIOD_TICKS = 200U; // ticks |
| #include "cmsis_os.h" | ||
| #include <cstdio> | ||
|
|
||
| #define CANARD_HZ 100U //ticks |
There was a problem hiding this comment.
CANARD_HZ is used as an osDelay(...) argument (ticks/period), but the name reads like a frequency in Hz. This ambiguity is likely to cause incorrect scheduling/timing adjustments. Consider renaming to CANARD_DELAY_TICKS/CANARD_PERIOD_TICKS and using a constexpr constant instead of a global #define in the header.
| #define CAN_HZ 100U //ticks | ||
|
|
||
| namespace task{ |
There was a problem hiding this comment.
CAN_HZ is passed to osDelay(...) (ticks/period), but the name suggests a frequency in Hertz. This makes the timing semantics unclear and easy to misuse. Consider renaming to CAN_DELAY_TICKS/CAN_PERIOD_TICKS and using a constexpr constant (ideally scoped to the class/translation unit) instead of a header-level #define.
| #define CAN_HZ 100U //ticks | |
| namespace task{ | |
| namespace task{ | |
| inline constexpr uint32_t CAN_DELAY_TICKS = 100U; // ticks |
| bool status[3]; | ||
| status[0] = can_task.init(); | ||
| status[1] = logger.init(); | ||
| status[2] = canards_controller.init(); | ||
|
|
||
| //send status along CAN for telemetry, unless CAN faulty, then oops | ||
|
|
There was a problem hiding this comment.
status is written but never read. If warnings are enabled this becomes an unused-variable warning, and it also obscures whether init failures are handled. Consider either acting on these return values immediately (e.g., abort/enter safe mode, log, or transmit telemetry) or remove the array until the handling is implemented (or explicitly mark it unused).
| bool status[3]; | |
| status[0] = can_task.init(); | |
| status[1] = logger.init(); | |
| status[2] = canards_controller.init(); | |
| //send status along CAN for telemetry, unless CAN faulty, then oops | |
| if (!can_task.init()) { | |
| Error_Handler(); | |
| } | |
| if (!logger.init()) { | |
| Error_Handler(); | |
| } | |
| if (!canards_controller.init()) { | |
| Error_Handler(); | |
| } |
No description provided.