Hp/sensornode#19
Merged
Merged
Conversation
…c, it now builds Co-authored-by: Gabriel Moreno <gabedebugs@gmail.com> Co-authored-by: SamNesheiwat <SamNesheiwat@users.noreply.github.com>
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 40 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request integrates multiple new STM32 peripheral drivers and sensor interfaces into the Sensor_Nodes firmware, including ADC, dual CAN buses, dual I2C interfaces, UART4, and TIM3 encoder support, alongside implementations for LSM6DSOX IMU, LIS3MDL magnetometer, and TPS2482 power monitor sensor drivers with a CAN-based communication layer. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as main()
participant GPIO as GPIO Init
participant CAN as CAN Peripheral
participant Filter as Filter Config
participant ISR as HAL_CAN_RxFifo0MsgPendingCallback()
participant App as FEB_Main (App)
Main->>GPIO: MX_GPIO_Init()
activate GPIO
GPIO-->>Main: GPIO configured
deactivate GPIO
Main->>CAN: MX_CAN1_Init() & MX_CAN2_Init()
activate CAN
CAN-->>Main: CAN handles initialized
deactivate CAN
Main->>App: FEB_CAN_Init(callback)
activate App
App->>Filter: FEB_CAN_Filter_Config()
activate Filter
Filter-->>App: Filters configured (ID-mask mode)
deactivate Filter
App->>CAN: HAL_CAN_Start(CAN1/2)
App->>CAN: Enable CAN_IT_RX_FIFO0_MSG_PENDING
App-->>Main: CAN init complete, callback registered
deactivate App
Note over Main,App: Main loop running...
CAN->>ISR: RX message pending on FIFO0
activate ISR
ISR->>CAN: HAL_CAN_GetRxMessage()
CAN-->>ISR: Message + header
ISR->>App: Invoke FEB_CAN_Rx_Callback(bus, header, data)
activate App
App-->>ISR: Callback processed
deactivate App
ISR-->>CAN: Complete
deactivate ISR
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (11)
.pre-commit-config.yaml (1)
38-38: Extract the inlinecppcheckone-liner into a script and document suppression intent.At Line 38, the hook command is getting too dense to safely maintain. Please move it into
scripts/and keep the YAML entry minimal; also add a short rationale for the twolsm6dsox_reg.csuppressions so they don’t become permanent blind spots.Proposed refactor
- entry: bash -c 'if ! command -v cppcheck &>/dev/null; then echo "cppcheck not installed, skipping (install via brew install cppcheck)"; exit 0; fi; for board in BMS DASH DART DCU LVPDB PCU Sensor_Nodes; do if [ -d "$board/Core/User" ] && find "$board/Core/User" -name "*.c" -print -quit 2>/dev/null | grep -q .; then cppcheck --quiet --enable=warning,performance,portability --suppress=missingIncludeSystem --suppress=unmatchedSuppression --suppress=uninitvar:*lsm6dsox_reg.c* --suppress=legacyUninitvar:*lsm6dsox_reg.c* --error-exitcode=1 "$board/Core/User/"; fi; done' + entry: ./scripts/run-cppcheck.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pre-commit-config.yaml at line 38, The pre-commit hook's long inline cppcheck one-liner should be extracted into a dedicated script (e.g., create scripts/run_cppcheck.sh) and the YAML hook entry reduced to a simple call to that script; move all find/loop/cppcheck flags and the two suppressions for lsm6dsox_reg.c into that script, and add a short comment at the top of the script explaining the rationale for the lsm6dsox_reg.c suppressions (what false-positive or known issue they silence and for how long), then update the .pre-commit-config.yaml entry to simply invoke the script (keeping the existing early-exit behavior if cppcheck is missing).Sensor_Nodes/CMakeLists.txt (1)
43-44: Consider adding existence check for submodule source.The CAN library source path is hardcoded. If the submodule isn't initialized or the path changes, the build will fail with a potentially confusing error.
💡 Optional: Add existence check
# CAN library generated source (from common submodule) +set(FEB_CAN_SOURCE "${CMAKE_CURRENT_SOURCE_DIR}/../common/FEB_CAN_Library_SN4/gen/feb_can.c") +if(NOT EXISTS "${FEB_CAN_SOURCE}") + message(FATAL_ERROR "CAN library source not found at ${FEB_CAN_SOURCE}. " + "Ensure the common submodule is initialized: git submodule update --init") +endif() -list(APPEND ALL_SOURCES "${CMAKE_CURRENT_SOURCE_DIR}/../common/FEB_CAN_Library_SN4/gen/feb_can.c") +list(APPEND ALL_SOURCES "${FEB_CAN_SOURCE}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sensor_Nodes/CMakeLists.txt` around lines 43 - 44, The hardcoded CAN source path added with list(APPEND ALL_SOURCES "${CMAKE_CURRENT_SOURCE_DIR}/../common/FEB_CAN_Library_SN4/gen/feb_can.c") should be guarded: check for the file's existence (using if(EXISTS <path>) or file(EXISTS ...) with the same computed path) before appending to ALL_SOURCES and, if missing, either emit a clear fatal error message (message(FATAL_ERROR "...")) explaining the missing submodule or skip adding the source and log a message; use the CMAKE_CURRENT_SOURCE_DIR and the same relative path string to locate the file so the conditional targets the exact same resource.Sensor_Nodes/Core/User/Inc/FEB_MMeter.h (2)
11-13: Redundantextern I2C_HandleTypeDef hi2c3declaration.
FEB_IMU.h(included on line 11) already declaresextern I2C_HandleTypeDef hi2c3. The duplicate declaration on line 13 is unnecessary.♻️ Suggested cleanup
`#include` "lis3mdl_reg.h" `#include` "FEB_IMU.h" // for shared platform_read / platform_write - extern I2C_HandleTypeDef hi2c3; - /* Public initialization */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sensor_Nodes/Core/User/Inc/FEB_MMeter.h` around lines 11 - 13, Remove the redundant extern declaration of I2C_HandleTypeDef hi2c3 from FEB_MMeter.h since FEB_IMU.h (already included) exposes that symbol; locate the line that reads "extern I2C_HandleTypeDef hi2c3" in FEB_MMeter.h and delete it so the header relies on the declaration provided by FEB_IMU.h (no other changes needed).
1-2: Include guard naming is inconsistent with project convention.Other FEB headers use
FEB_*_Hpattern (e.g.,FEB_MAIN_H), but this usesMMETER_H. Consider renaming toFEB_MMETER_Hfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sensor_Nodes/Core/User/Inc/FEB_MMeter.h` around lines 1 - 2, Rename the include guard from MMETER_H to follow the project convention FEB_MMETER_H: update the `#ifndef` and `#define` macros (currently MMETER_H) to FEB_MMETER_H and also update the matching `#endif` comment or trailing macro reference if present so the header's guard consistently uses FEB_MMETER_H (search for MMETER_H in FEB_MMeter.h to locate all occurrences).Sensor_Nodes/Core/User/Inc/FEB_IMU.h (2)
1-2: Include guard naming inconsistent — useFEB_IMU_Hfor consistency.The guard uses
IMU_Hwhile other FEB headers follow theFEB_*_Hpattern. This could also collide with other libraries definingIMU_H.♻️ Suggested fix
-#ifndef IMU_H -#define IMU_H +#ifndef FEB_IMU_H +#define FEB_IMU_H ... -#endif /* IMU_H */ +#endif /* FEB_IMU_H */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sensor_Nodes/Core/User/Inc/FEB_IMU.h` around lines 1 - 2, The include guard macro in FEB_IMU.h currently uses IMU_H which is inconsistent and risks collisions; rename the guard to FEB_IMU_H by updating the `#ifndef` and `#define` macros from IMU_H to FEB_IMU_H and also update any matching `#endif` comment or references in the file (look for IMU_H and the trailing `#endif` comment) so the guard is consistent with other FEB headers.
15-18: Consider makingplatform_write/platform_readinternal.These functions have a custom signature (with
devaddressparameter) that doesn't match the standardstmdev_write_ptr/stmdev_read_ptrcallback types. They appear to be implementation details shared between IMU and magnetometer drivers rather than a true public API.If they must remain shared, document their non-standard signature. Otherwise, consider keeping them
staticin a shared internal header.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sensor_Nodes/Core/User/Inc/FEB_IMU.h` around lines 15 - 18, platform_write and platform_read are internal helper functions with a non-standard signature (they include devaddress) and should not be part of the public header; move their declarations into a shared internal header or the single C file where they’re used and mark them static (or rename to a clearly internal prefix like _platform_write/_platform_read) or, if they must remain shared, add a clear comment documenting their non-standard signature and intended internal use so callers know they differ from stmdev_write_ptr/stmdev_read_ptr; update references to platform_write/platform_read in the IMU and magnetometer drivers accordingly.Sensor_Nodes/Core/User/Inc/FEB_Main.h (1)
1-11: Consider addingextern "C"wrapper for C++ compatibility.Other headers in this PR (e.g.,
FEB_MMeter.h) includeextern "C"guards. For consistency and to support potential C++ consumers:♻️ Suggested improvement
`#ifndef` FEB_MAIN_H `#define` FEB_MAIN_H +#ifdef __cplusplus +extern "C" { +#endif + `#include` "FEB_CAN.h" void FEB_Init(void); void FEB_Update(void); void FEB_CAN_Rx_Callback(FEB_CAN_Bus_t bus, CAN_RxHeaderTypeDef *rx_header, void *data); void FEB_Main_Loop(void); +#ifdef __cplusplus +} +#endif + `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sensor_Nodes/Core/User/Inc/FEB_Main.h` around lines 1 - 11, Header lacks C++ linkage guards; wrap the public C function declarations (FEB_Init, FEB_Update, FEB_CAN_Rx_Callback, FEB_Main_Loop) with an `#ifdef` __cplusplus extern "C" { ... } `#endif` pair so C++ callers get C linkage, placing the opening guard after the includes and the closing guard before the final `#endif`.Sensor_Nodes/Core/User/Src/FEB_Main.c (2)
10-16: CAN callback discards all received messages.The
FEB_CAN_Rx_Callbackcurrently does nothing with received CAN data. This is acceptable for initial scaffolding, but ensure this is intentional and not a forgotten TODO.Would you like me to generate a basic CAN message handler skeleton that decodes message IDs and dispatches to appropriate handlers?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sensor_Nodes/Core/User/Src/FEB_Main.c` around lines 10 - 16, The FEB_CAN_Rx_Callback currently ignores all incoming CAN frames; implement a basic handler inside FEB_CAN_Rx_Callback that reads the CAN ID from rx_header, extracts payload bytes from data, switches on the CAN ID to call appropriate handlers (e.g., HandleStatusMessage, HandleCommandMessage, HandleDiagnostics — create these if missing), and perform bounds checks on rx_header->DLC before accessing payload; keep a default/logging path for unknown IDs and preserve the (void) casts removal so the parameters are used.
1-4: Unused includes:FEB_MMeter.handFEB_TPS.h.These headers are included but their functions (
read_Magnetic_Field_Data, TPS functions) are not called. If intentionally staged for future use, consider adding a comment. Otherwise, remove to reduce coupling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sensor_Nodes/Core/User/Src/FEB_Main.c` around lines 1 - 4, The two headers FEB_MMeter.h and FEB_TPS.h are included but none of their APIs (e.g., read_Magnetic_Field_Data or any TPS_* functions) are used; either remove these unused includes from the top of FEB_Main.c to reduce coupling, or if they are intentionally staged for future work add a brief comment next to the include lines explaining they are reserved for upcoming magnetic sensor (read_Magnetic_Field_Data) and TPS usage so reviewers won't remove them.Sensor_Nodes/Core/User/Inc/FEB_CAN.h (1)
17-21: Document that the RX callback runs in interrupt context.
HAL_CAN_RxFifo0MsgPendingCallback()invokes this callback directly from the CAN IRQ path, so callers need to know it must stay ISR-safe and non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sensor_Nodes/Core/User/Inc/FEB_CAN.h` around lines 17 - 21, The RX callback type FEB_CAN_Rx_Callback_t and its use in FEB_CAN_Init must be documented as running in interrupt context because HAL_CAN_RxFifo0MsgPendingCallback() invokes it directly from the CAN IRQ path; update comments to state the callback executes in ISR context and must be non-blocking and ISR-safe (no blocking RTOS calls, no malloc/free, no heavy processing, no printf), and recommend deferring work to a thread-safe queue/semaphore/flag or using HAL_FROMISR-safe APIs for any further processing.Sensor_Nodes/Core/User/Src/FEB_IMU.c (1)
47-58: Gate the scan andprintfpaths behind a debug flag.
i2c_scan()runs on every init, and both read functions print floats on every sample. With these wired intoFEB_Init()/FEB_Update(), boot time and steady-state loop time become dominated by probing and UART formatting instead of sensor acquisition.Also applies to: 60-69, 101-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sensor_Nodes/Core/User/Src/FEB_IMU.c` around lines 47 - 58, i2c_scan() and the sensor read paths that printf floats are executed unconditionally (called from FEB_Init and FEB_Update), causing long boot and loop delays; wrap these behaviors behind a debug flag so they only run when debugging: add a compile-time macro (e.g., DEBUG_I2C) or a runtime boolean (e.g., debug_enabled) and early-return from i2c_scan() and the float-printing read functions when the flag is false, and remove/guard calls to i2c_scan() from FEB_Init and periodic printf calls in FEB_Update so normal operation performs only sensor acquisition (optionally keep a lower-rate probe when runtime flag is enabled). Ensure you reference i2c_scan, FEB_Init, FEB_Update and the float-printing read functions when applying the guards.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sensor_Nodes/Core/Inc/main.h`:
- Around line 49-60: There is a duplicate macro definition for
PUTCHAR_PROTOTYPE; remove the unconditional "#define PUTCHAR_PROTOTYPE int
__io_putchar(int ch)" that appears before the exported prototypes and keep the
conditional block that defines PUTCHAR_PROTOTYPE based on __GNUC__ (the block
that selects __io_putchar or fputc). Ensure only the conditional definition
remains so PUTCHAR_PROTOTYPE, __io_putchar and fputc are defined once.
In `@Sensor_Nodes/Core/Src/adc.c`:
- Around line 54-67: The ADC is only set up to sample ADC_CHANNEL_13 because
hadc1.Init.NbrOfConversion is 1 and sConfig.Channel is set once; update the
configuration so all intended inputs from HAL_ADC_MspInit() are placed in the
regular sequence: set hadc1.Init.NbrOfConversion to the total channel count
(e.g., 4), and call HAL_ADC_ConfigChannel(&hadc1,&sConfig) once per channel,
setting sConfig.Channel to each ADC_CHANNEL_x and sConfig.Rank to 1..N for each
call (adjust sConfig.SamplingTime per channel if needed) so the thermocouple,
strain-gauge and second-wiper are included in the sequencer. Ensure any code
that expects single-channel behavior is updated or that conversions iterate
through the sequence accordingly.
In `@Sensor_Nodes/Core/Src/can.c`:
- Around line 42-46: The CAN bit timing is currently 3 TQ (Prescaler=16,
SJW=1TQ, BS1=1TQ, BS2=1TQ) which is invalid—recompute and set hcan1.Init.* (and
the same for hcan2 if present) to yield 8–25 time quanta and a standard baud
(e.g., 500 kbps or 1 Mbps) given your MCU peripheral clock: choose Prescaler,
SyncJumpWidth (SJW), TimeSeg1 (BS1) and TimeSeg2 (BS2) so that (Prescaler * Tq)
= CAN bit time and BS1+BS2+SJW+Sync = TQ count between 8 and 25; update
hcan1.Init.Prescaler, hcan1.Init.SyncJumpWidth, hcan1.Init.TimeSeg1 and
hcan1.Init.TimeSeg2 (and corresponding hcan2.Init fields) accordingly before
enabling the controllers.
In `@Sensor_Nodes/Core/Src/main.c`:
- Around line 110-118: The USER CODE markers around the infinite loop are
misplaced: move the closing marker /* USER CODE END WHILE */ to immediately
before the loop exit (i.e., just after FEB_Main_Loop() if that's the intended
user section) and place /* USER CODE BEGIN 3 */ and /* USER CODE END 3 */ inside
the while(1) body (or vice versa) so that the markers properly wrap user
additions to FEB_Main_Loop(); ensure the markers surround the user-editable
region in main() rather than splitting it across inside/outside the while loop
and keep FEB_Main_Loop() call within the same USER CODE block.
- Line 105: FEB_Init currently doesn't call FEB_CAN_Init so CAN peripherals
never have filters configured, aren't started with HAL_CAN_Start, RX interrupts
aren't enabled with HAL_CAN_ActivateNotification, nor is the RX callback
registered; update FEB_Init (the function that calls MX_CAN1_Init and
MX_CAN2_Init) to call FEB_CAN_Init() after the MX_CANx_Init calls (or at an
appropriate point during initialization) and handle/propagate any error return
from FEB_CAN_Init so initialization fails visibly if CAN setup fails.
In `@Sensor_Nodes/Core/Src/usart.c`:
- Around line 92-114: Insert the missing USER CODE END marker for the UART4 Msp
Init block: inside the HAL_UART_MspInit UART4 branch (the
if(uartHandle->Instance==UART4) block) add the comment "/* USER CODE END
UART4_MspInit 0 */" immediately after the existing "/* USER CODE BEGIN
UART4_MspInit 0 */" and before the clock enable calls
(__HAL_RCC_UART4_CLK_ENABLE(), __HAL_RCC_GPIOC_CLK_ENABLE(), etc.) so CubeMX
regeneration can correctly recognize the user section.
In `@Sensor_Nodes/Core/User/Src/FEB_CAN.c`:
- Around line 20-67: The init currently registers FEB_CAN_Rx_Callback after
starting buses and enabling CAN_IT_RX_FIFO0_MSG_PENDING and ignores HAL return
statuses; update FEB_CAN_Init to assign FEB_CAN_Rx_Callback = callback before
calling HAL_CAN_ActivateNotification so the callback is present for any early RX
interrupts, and propagate/check HAL_StatusTypeDef results from HAL_CAN_Start,
HAL_CAN_ActivateNotification and HAL_CAN_ConfigFilter (called in
FEB_CAN_Filter_Config) — e.g., return or report errors if any call != HAL_OK
instead of silently swallowing failures; search for FEB_CAN_Init,
FEB_CAN_Rx_Callback, HAL_CAN_Start, HAL_CAN_ActivateNotification and
HAL_CAN_ConfigFilter to make the changes.
- Around line 6-13: FEB_CAN_Transmit is not reentrant and can overflow/lock:
remove reliance on shared statics FEB_CAN_Tx_Header, FEB_CAN_Tx_Data and
FEB_CAN_Tx_Mailbox by making a local CAN_TxHeaderTypeDef and a local uint8_t
txData[8] inside FEB_CAN_Transmit; validate and bound the incoming length (cap
at 8 and return error if larger) and copy payload with memcpy using the bounded
length; replace the infinite spin waiting for a free mailbox with a bounded wait
(timeout loop using a tick/count and return a timeout/error code if no mailbox
frees) and use the HAL_CAN_AddTxMessage return to detect success/failure; keep
FEB_CAN_Rx_Callback handling unchanged. Ensure all references in
FEB_CAN_Transmit use the new local variables (instead of FEB_CAN_Tx_Header,
FEB_CAN_Tx_Data, FEB_CAN_Tx_Mailbox).
In `@Sensor_Nodes/Core/User/Src/FEB_IMU.c`:
- Around line 60-99: The init routine ignores return statuses from IMU driver
calls causing use of uninitialized data; update lsm6dsox_init to check the
return value of every driver call (at least lsm6dsox_device_id_get,
lsm6dsox_reset_set, lsm6dsox_reset_get, lsm6dsox_block_data_update_set,
lsm6dsox_xl_data_rate_set, lsm6dsox_xl_full_scale_set,
lsm6dsox_gy_data_rate_set, lsm6dsox_gy_full_scale_set), and on any non-success
status log an error (include the status) and abort initialization (return)
rather than continuing; ensure you do not read whoamI or rst unless the
corresponding get returned success.
In `@Sensor_Nodes/Core/User/Src/FEB_Main.c`:
- Around line 26-33: The FEB_Init function currently only initializes the IMU;
update FEB_Init to call FEB_CAN_Init() so the FEB CAN layer starts the buses and
enables interrupts (in addition to the existing lsm6dsox_init()). Locate
FEB_Init and insert a call to FEB_CAN_Init() after or before lsm6dsox_init()
(ensuring it runs once during setup) so that MX_CAN1_Init()/MX_CAN2_Init()’s
peripheral configuration is followed by the CAN layer startup and interrupt
enabling.
In `@Sensor_Nodes/Core/User/Src/FEB_MMeter.c`:
- Around line 6-7: Remove the incorrect system-include directive for FEB_MAIN.h
that breaks CI and leaves only the correct local include "FEB_Main.h"; edit the
top of FEB_MMeter.c to delete the line containing `#include` <FEB_MAIN.h> so the
compiler uses the existing "FEB_Main.h" include and resolves symbols from that
header.
- Around line 28-34: lis3mdl_init currently only sets function pointers and the
I2C handle; you must also program the sensor control registers so it produces
fresh samples: after the existing context setup in lis3mdl_init call
lis3mdl_operating_mode_set(...) to enable continuous-conversion mode, call
lis3mdl_data_rate_set(...) to select an appropriate output data rate (e.g. 10–80
Hz) and call lis3mdl_full_scale_set(...) to set the desired gauss range; follow
the same call pattern and error handling used in FEB_IMU.c (check return
statuses and log/handle failures) so read_Magnetic_Field_Data() receives live
data.
In `@Sensor_Nodes/Core/User/Src/FEB_TPS.c`:
- Around line 51-60: read_TPS currently always calls
TPS2482_Poll_Current/Bus_Voltage/Shunt_Voltage which can block if the device
wasn't initialized; change read_TPS to first check the initialization flag
tps_init_res (or return an error) and skip the three TPS2482_Poll_* calls when
init failed so polling only occurs after successful init; ensure
tps_bus_voltage/tps_shunt_voltage/tps_current are left in a safe state (or
return an error code) when skipping to avoid using stale/uninitialized values.
In `@Sensor_Nodes/Core/User/Src/TPS2482.c`:
- Around line 75-99: TPS2482_Get_Register and TPS2482_Write_Register must stop
swallowing I2C errors and blocking forever: change both helpers to return a
status (e.g., HAL_StatusTypeDef or int) instead of silently writing zeros/drop,
stop using HAL_MAX_DELAY for transactions (use a reasonable timeout parameter),
propagate HAL_I2C_Mem_Read/HAL_I2C_Mem_Write return codes to the caller and only
fill results on success (or set a clear sentinel/error flag per index) and
ensure TPS2482_Init checks those return values (and per-device read statuses)
rather than treating zero-valued reads as success; update references to
TPS2482_Get_Register, TPS2482_Write_Register, TPS2482_Init, HAL_I2C_Mem_Read,
HAL_I2C_Mem_Write and TPS2482_MAX_DEVICES when making these changes.
- Around line 68-70: The readback comparison in TPS2482_Init() currently strips
APOL and LEN by using masks_flagless_mask (variable masks_flagless_mask) which
lets the initialization report success even if alert polarity or latch mode
weren't applied; update the comparison to include TPS2482_MASK_APOL and
TPS2482_MASK_LEN in masks_flagless_mask (or remove the masking and compare full
masks[i] vs masks_res[i]) so that the res[i] assignment uses the full writable
mask bits when evaluating success (i.e., change how masks_flagless_mask is
constructed or used in the res[i] &= masks[i] == (masks_res[i] &
masks_flagless_mask) check to account for APOL and LEN).
- Around line 207-214: TPS2482_Enable currently casts a bool* (result) to
GPIO_PinState* and passes it to TPS2482_GPIO_Read, which is unsafe on targets
where GPIO_PinState is wider than bool; change TPS2482_Enable to allocate a
temporary array of GPIO_PinState of size messageCount, call
TPS2482_GPIO_Read(GPIOx, GPIO_Pin, tempStates, messageCount), then convert each
tempStates[i] to result[i] (true for GPIO_PIN_SET, false for GPIO_PIN_RESET)
before returning; alternatively, change the function signature to accept
GPIO_PinState *result if callers expect raw GPIO states (update callers
accordingly). Ensure you reference TPS2482_Enable, TPS2482_GPIO_Read,
TPS2482_GPIO_Write, and parameter names GPIOx, GPIO_Pin, en_dis, result,
messageCount when applying the fix.
In `@Sensor_Nodes/Sensor_Nodes.ioc`:
- Around line 11-18: The project enables CAN RX FIFO0 message-pending interrupts
(CAN_IT_RX_FIFO0_MSG_PENDING) in FEB_CAN.c but the CubeMX .ioc lacks NVIC
entries for CAN1_RX0_IRQn and CAN2_RX0_IRQn and stm32f4xx_it.c has no handlers,
so add NVIC entries for CAN1_RX0_IRQn and CAN2_RX0_IRQn in the CubeMX .ioc NVIC
configuration or implement the ISR handlers for those IRQs in stm32f4xx_it.c
(e.g., provide CAN1_RX0_IRQHandler and CAN2_RX0_IRQHandler that call the HAL/CAN
receive-processing functions used by FEB_CAN.c) so interrupts have registered
handlers and won’t be lost on CubeMX regeneration.
In `@UART/.mxproject`:
- Around line 5-7: UART/.mxproject contains duplicated entries and malformed
tokens in the SourceFiles and CDefines fields (see symbols "SourceFiles" and
"CDefines") causing churn; fix by either removing this file from the PR (unstage
and add to .gitignore if it should not be tracked) or regenerate/normalize it
with STM32CubeMX so that SourceFiles no longer duplicates HAL/FreeRTOS sources
and removes the ";;;" empty tokens and CDefines no longer repeats
"USE_HAL_DRIVER"; verify HeaderPath and CDefines entries are unique and clean
before committing.
---
Nitpick comments:
In @.pre-commit-config.yaml:
- Line 38: The pre-commit hook's long inline cppcheck one-liner should be
extracted into a dedicated script (e.g., create scripts/run_cppcheck.sh) and the
YAML hook entry reduced to a simple call to that script; move all
find/loop/cppcheck flags and the two suppressions for lsm6dsox_reg.c into that
script, and add a short comment at the top of the script explaining the
rationale for the lsm6dsox_reg.c suppressions (what false-positive or known
issue they silence and for how long), then update the .pre-commit-config.yaml
entry to simply invoke the script (keeping the existing early-exit behavior if
cppcheck is missing).
In `@Sensor_Nodes/CMakeLists.txt`:
- Around line 43-44: The hardcoded CAN source path added with list(APPEND
ALL_SOURCES
"${CMAKE_CURRENT_SOURCE_DIR}/../common/FEB_CAN_Library_SN4/gen/feb_can.c")
should be guarded: check for the file's existence (using if(EXISTS <path>) or
file(EXISTS ...) with the same computed path) before appending to ALL_SOURCES
and, if missing, either emit a clear fatal error message (message(FATAL_ERROR
"...")) explaining the missing submodule or skip adding the source and log a
message; use the CMAKE_CURRENT_SOURCE_DIR and the same relative path string to
locate the file so the conditional targets the exact same resource.
In `@Sensor_Nodes/Core/User/Inc/FEB_CAN.h`:
- Around line 17-21: The RX callback type FEB_CAN_Rx_Callback_t and its use in
FEB_CAN_Init must be documented as running in interrupt context because
HAL_CAN_RxFifo0MsgPendingCallback() invokes it directly from the CAN IRQ path;
update comments to state the callback executes in ISR context and must be
non-blocking and ISR-safe (no blocking RTOS calls, no malloc/free, no heavy
processing, no printf), and recommend deferring work to a thread-safe
queue/semaphore/flag or using HAL_FROMISR-safe APIs for any further processing.
In `@Sensor_Nodes/Core/User/Inc/FEB_IMU.h`:
- Around line 1-2: The include guard macro in FEB_IMU.h currently uses IMU_H
which is inconsistent and risks collisions; rename the guard to FEB_IMU_H by
updating the `#ifndef` and `#define` macros from IMU_H to FEB_IMU_H and also update
any matching `#endif` comment or references in the file (look for IMU_H and the
trailing `#endif` comment) so the guard is consistent with other FEB headers.
- Around line 15-18: platform_write and platform_read are internal helper
functions with a non-standard signature (they include devaddress) and should not
be part of the public header; move their declarations into a shared internal
header or the single C file where they’re used and mark them static (or rename
to a clearly internal prefix like _platform_write/_platform_read) or, if they
must remain shared, add a clear comment documenting their non-standard signature
and intended internal use so callers know they differ from
stmdev_write_ptr/stmdev_read_ptr; update references to
platform_write/platform_read in the IMU and magnetometer drivers accordingly.
In `@Sensor_Nodes/Core/User/Inc/FEB_Main.h`:
- Around line 1-11: Header lacks C++ linkage guards; wrap the public C function
declarations (FEB_Init, FEB_Update, FEB_CAN_Rx_Callback, FEB_Main_Loop) with an
`#ifdef` __cplusplus extern "C" { ... } `#endif` pair so C++ callers get C linkage,
placing the opening guard after the includes and the closing guard before the
final `#endif`.
In `@Sensor_Nodes/Core/User/Inc/FEB_MMeter.h`:
- Around line 11-13: Remove the redundant extern declaration of
I2C_HandleTypeDef hi2c3 from FEB_MMeter.h since FEB_IMU.h (already included)
exposes that symbol; locate the line that reads "extern I2C_HandleTypeDef hi2c3"
in FEB_MMeter.h and delete it so the header relies on the declaration provided
by FEB_IMU.h (no other changes needed).
- Around line 1-2: Rename the include guard from MMETER_H to follow the project
convention FEB_MMETER_H: update the `#ifndef` and `#define` macros (currently
MMETER_H) to FEB_MMETER_H and also update the matching `#endif` comment or
trailing macro reference if present so the header's guard consistently uses
FEB_MMETER_H (search for MMETER_H in FEB_MMeter.h to locate all occurrences).
In `@Sensor_Nodes/Core/User/Src/FEB_IMU.c`:
- Around line 47-58: i2c_scan() and the sensor read paths that printf floats are
executed unconditionally (called from FEB_Init and FEB_Update), causing long
boot and loop delays; wrap these behaviors behind a debug flag so they only run
when debugging: add a compile-time macro (e.g., DEBUG_I2C) or a runtime boolean
(e.g., debug_enabled) and early-return from i2c_scan() and the float-printing
read functions when the flag is false, and remove/guard calls to i2c_scan() from
FEB_Init and periodic printf calls in FEB_Update so normal operation performs
only sensor acquisition (optionally keep a lower-rate probe when runtime flag is
enabled). Ensure you reference i2c_scan, FEB_Init, FEB_Update and the
float-printing read functions when applying the guards.
In `@Sensor_Nodes/Core/User/Src/FEB_Main.c`:
- Around line 10-16: The FEB_CAN_Rx_Callback currently ignores all incoming CAN
frames; implement a basic handler inside FEB_CAN_Rx_Callback that reads the CAN
ID from rx_header, extracts payload bytes from data, switches on the CAN ID to
call appropriate handlers (e.g., HandleStatusMessage, HandleCommandMessage,
HandleDiagnostics — create these if missing), and perform bounds checks on
rx_header->DLC before accessing payload; keep a default/logging path for unknown
IDs and preserve the (void) casts removal so the parameters are used.
- Around line 1-4: The two headers FEB_MMeter.h and FEB_TPS.h are included but
none of their APIs (e.g., read_Magnetic_Field_Data or any TPS_* functions) are
used; either remove these unused includes from the top of FEB_Main.c to reduce
coupling, or if they are intentionally staged for future work add a brief
comment next to the include lines explaining they are reserved for upcoming
magnetic sensor (read_Magnetic_Field_Data) and TPS usage so reviewers won't
remove them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86d41551-1ab6-49fd-a2ae-826fdec906dd
📒 Files selected for processing (38)
.pre-commit-config.yamlSensor_Nodes/.mxprojectSensor_Nodes/CMakeLists.txtSensor_Nodes/Core/Inc/adc.hSensor_Nodes/Core/Inc/can.hSensor_Nodes/Core/Inc/i2c.hSensor_Nodes/Core/Inc/main.hSensor_Nodes/Core/Inc/stm32f4xx_hal_conf.hSensor_Nodes/Core/Inc/tim.hSensor_Nodes/Core/Inc/usart.hSensor_Nodes/Core/Src/adc.cSensor_Nodes/Core/Src/can.cSensor_Nodes/Core/Src/gpio.cSensor_Nodes/Core/Src/i2c.cSensor_Nodes/Core/Src/main.cSensor_Nodes/Core/Src/tim.cSensor_Nodes/Core/Src/usart.cSensor_Nodes/Core/User/Inc/FEB_CAN.hSensor_Nodes/Core/User/Inc/FEB_GPS.hSensor_Nodes/Core/User/Inc/FEB_IMU.hSensor_Nodes/Core/User/Inc/FEB_MMeter.hSensor_Nodes/Core/User/Inc/FEB_Main.hSensor_Nodes/Core/User/Inc/FEB_TPS.hSensor_Nodes/Core/User/Inc/TPS2482.hSensor_Nodes/Core/User/Inc/lis3mdl_reg.hSensor_Nodes/Core/User/Inc/lsm6dsox_reg.hSensor_Nodes/Core/User/Src/FEB_CAN.cSensor_Nodes/Core/User/Src/FEB_GPS.cSensor_Nodes/Core/User/Src/FEB_IMU.cSensor_Nodes/Core/User/Src/FEB_MMeter.cSensor_Nodes/Core/User/Src/FEB_Main.cSensor_Nodes/Core/User/Src/FEB_TPS.cSensor_Nodes/Core/User/Src/TPS2482.cSensor_Nodes/Core/User/Src/lis3mdl-pid.cSensor_Nodes/Core/User/Src/lsm6dsox_reg.cSensor_Nodes/Sensor_Nodes.iocUART/.mxprojectcommon/FEB_CAN_Library_SN4
Comment on lines
+49
to
+60
| #define PUTCHAR_PROTOTYPE int __io_putchar(int ch) | ||
| /* USER CODE END EM */ | ||
|
|
||
| /* Exported functions prototypes ---------------------------------------------*/ | ||
| void Error_Handler(void); | ||
|
|
||
| /* USER CODE BEGIN EFP */ | ||
|
|
||
| #ifdef __GNUC__ | ||
| #define PUTCHAR_PROTOTYPE int __io_putchar(int ch) | ||
| #else | ||
| #define PUTCHAR_PROTOTYPE int fputc(int ch, FILE *f) | ||
| #endif |
Contributor
There was a problem hiding this comment.
Duplicate PUTCHAR_PROTOTYPE macro definition will cause redefinition warning.
The macro is defined unconditionally on line 49 within the USER CODE BEGIN EM section, then redefined again on lines 56-60 within the USER CODE BEGIN EFP section. This causes a preprocessor redefinition warning/error.
Remove the first definition and keep only the conditional one:
🔧 Proposed fix
/* USER CODE BEGIN EM */
-#define PUTCHAR_PROTOTYPE int __io_putchar(int ch)
+
/* USER CODE END EM */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #define PUTCHAR_PROTOTYPE int __io_putchar(int ch) | |
| /* USER CODE END EM */ | |
| /* Exported functions prototypes ---------------------------------------------*/ | |
| void Error_Handler(void); | |
| /* USER CODE BEGIN EFP */ | |
| #ifdef __GNUC__ | |
| #define PUTCHAR_PROTOTYPE int __io_putchar(int ch) | |
| #else | |
| #define PUTCHAR_PROTOTYPE int fputc(int ch, FILE *f) | |
| #endif | |
| /* USER CODE BEGIN EM */ | |
| /* USER CODE END EM */ | |
| /* Exported functions prototypes ---------------------------------------------*/ | |
| void Error_Handler(void); | |
| /* USER CODE BEGIN EFP */ | |
| `#ifdef` __GNUC__ | |
| `#define` PUTCHAR_PROTOTYPE int __io_putchar(int ch) | |
| `#else` | |
| `#define` PUTCHAR_PROTOTYPE int fputc(int ch, FILE *f) | |
| `#endif` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sensor_Nodes/Core/Inc/main.h` around lines 49 - 60, There is a duplicate
macro definition for PUTCHAR_PROTOTYPE; remove the unconditional "#define
PUTCHAR_PROTOTYPE int __io_putchar(int ch)" that appears before the exported
prototypes and keep the conditional block that defines PUTCHAR_PROTOTYPE based
on __GNUC__ (the block that selects __io_putchar or fputc). Ensure only the
conditional definition remains so PUTCHAR_PROTOTYPE, __io_putchar and fputc are
defined once.
Comment on lines
+54
to
+67
| hadc1.Init.NbrOfConversion = 1; | ||
| hadc1.Init.DMAContinuousRequests = DISABLE; | ||
| hadc1.Init.EOCSelection = ADC_EOC_SINGLE_CONV; | ||
| if (HAL_ADC_Init(&hadc1) != HAL_OK) | ||
| { | ||
| Error_Handler(); | ||
| } | ||
|
|
||
| /** Configure for the selected ADC regular channel its corresponding rank in the sequencer and its sample time. | ||
| */ | ||
| sConfig.Channel = ADC_CHANNEL_13; | ||
| sConfig.Rank = 1; | ||
| sConfig.SamplingTime = ADC_SAMPLETIME_3CYCLES; | ||
| if (HAL_ADC_ConfigChannel(&hadc1, &sConfig) != HAL_OK) |
Contributor
There was a problem hiding this comment.
Only ADC_CHANNEL_13 is in the regular sequence.
NbrOfConversion = 1 and sConfig.Channel = ADC_CHANNEL_13 mean hadc1 will sample only PC3. The thermocouple, strain-gauge, and second-wiper inputs configured in HAL_ADC_MspInit() never enter the sequencer as-is, so they stay unreadable unless another layer reconfigures the channel before each conversion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sensor_Nodes/Core/Src/adc.c` around lines 54 - 67, The ADC is only set up to
sample ADC_CHANNEL_13 because hadc1.Init.NbrOfConversion is 1 and
sConfig.Channel is set once; update the configuration so all intended inputs
from HAL_ADC_MspInit() are placed in the regular sequence: set
hadc1.Init.NbrOfConversion to the total channel count (e.g., 4), and call
HAL_ADC_ConfigChannel(&hadc1,&sConfig) once per channel, setting sConfig.Channel
to each ADC_CHANNEL_x and sConfig.Rank to 1..N for each call (adjust
sConfig.SamplingTime per channel if needed) so the thermocouple, strain-gauge
and second-wiper are included in the sequencer. Ensure any code that expects
single-channel behavior is updated or that conversions iterate through the
sequence accordingly.
Comment on lines
+42
to
+46
| hcan1.Init.Prescaler = 16; | ||
| hcan1.Init.Mode = CAN_MODE_NORMAL; | ||
| hcan1.Init.SyncJumpWidth = CAN_SJW_1TQ; | ||
| hcan1.Init.TimeSeg1 = CAN_BS1_1TQ; | ||
| hcan1.Init.TimeSeg2 = CAN_BS2_1TQ; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n Sensor_Nodes/Core/Src/can.c | sed -n '35,85p'Repository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 1871
🏁 Script executed:
head -25 Sensor_Nodes/Sensor_Nodes.iocRepository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 1015
🌐 Web query:
CAN bus bit timing 3 time quanta robustness interoperability standard
💡 Result:
In Classical CAN (ISO 11898-1), the bit time consists of 8 to 25 time quanta (tq) for robustness and interoperability. Fewer tq (e.g., 3) reduces phase segments, limiting oscillator tolerance, synchronization jump width (SJW), and propagation delay compensation, causing synchronization failures, reduced bus length, and interoperability issues across controllers with varying clocks and delays. More tq (e.g., 16-20) enable larger SJW (up to 4 tq), better phase error correction, higher oscillator tolerance (up to 1.58% at low rates), and longer buses. In CAN FD, nominal (arbitration) phase uses 8-80 tq (CiA 601 recommends higher for robustness); data phase 4-49 tq minimum, but higher preferred for precision at high speeds (>1 Mbit/s needs TDC). 3 tq bit time is non-standard, lacks sufficient segments (Sync=1 tq, Prop>=1, Phase1>=1, Phase2>=max(IPT=2,SJW)), violating specs and risking poor robustness/interoperability. Use 8+ tq with sample point ~75-87.5%.
Citations:
- 1: http://www.bittiming.can-wiki.info/
- 2: https://www.nxp.com/docs/en/application-note/AN1798.pdf
- 3: https://ww1.microchip.com/downloads/en/AppNotes/00754.pdf
- 4: https://www.nxp.com/docs/en/application-note/AN1798.pdf?origin_team=T2VVBR6FR
- 5: https://kvaser.com/lesson/can-bit-timing/
- 6: https://www.can-cia.org/fileadmin/cia/documents/publications/cnlm/march_2018/18-1_p28_recommendation_for_the_canfd_bit-timing_holger_zeltwanger_cia.pdf
- 7: https://nxp.com/docs/en/application-note/AN1798.pdf
- 8: https://community.infineon.com/t5/Knowledge-Base-Articles/AURIX-MCU-CAN-bit-timing-parameter-clarification/ta-p/359882?nobounce
- 9: https://ww1.microchip.com/downloads/en/appnotes/00754.pdf
- 10: https://cdn.standards.iteh.ai/samples/86384/ca6e28c79b6b4388a5104dfda2332666/ISO-11898-1-2024.pdf
- 11: http://iso.org/standard/86384.html
- 12: https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/papers/icc14_2013_paper_hartwich_1.pdf
- 13: https://www.can-cia.org/can-knowledge/cia-601-series-can-fd-guidelines-and-recommendations
🌐 Web query:
STM32F4 bxCAN timing constraints standard baud rates 500 kbps 1 Mbps
💡 Result:
The STM32F4 series bxCAN peripheral supports standard CAN baud rates up to 1 Mbps. Bit timing configuration depends on the APB1 clock frequency (fPCLK), typically 42-45 MHz when using common system clocks like 84/168 MHz HCLK with APB1 prescaler 4. bxCAN bit timing formula (from reference manuals like RM0090): tBIT = (1/fPCLK) * (BRP+1) * (1 + TS1 + TS2), where BRP=0..1023, TS1=1..16, TS2=1..8 (total quanta 8-25), SJW=1..4. Sample points should be 75-90% (ideally ~87.5% for CANopen). Examples from sources (register values; actual segments = value+1): For 500 kbps (common fPCLK=42 MHz): - BRP=5, TS1=10, TS2=1 (sample point 85.7%) {3,8,57} - For fPCLK=45 MHz, double prescaler from 1 Mbps example: BRP=10, TS1=4, TS2=2 {2} For 1 Mbps (fPCLK=45 MHz): - BRP=4 (prescaler=5), TS1=4 (BS1=5), TS2=2 (BS2=3), SJW=? (9 tq total) {2} For fPCLK=48 MHz: - BRP=5, TS1=4, TS2=1 (sample point 75%) {16} Use http://www.bittiming.can-wiki.info/ (select bxCAN, input fPCLK) for exact values per clock. CubeMX auto-calculates based on clock tree. Deviation must be <1.5% for interoperability. No fixed "constraints" beyond ranges; must fit integer quanta.
Citations:
- 1: https://community.st.com/t5/stm32-mcus-products/has-anyone-successfully-ran-an-stm32f4xx-canbus-at-500k-baud/td-p/278623
- 2: https://stackoverflow.com/questions/59802073/how-to-set-can-baud-rate-to-500k-for-stm32-controller
- 3: http://www.bittiming.can-wiki.info/
- 4: https://techoverflow.net/2019/02/01/how-to-calculate-stm32-bxcan-bit-timings-using-python/
- 5: https://www.st.com/en/microcontrollers-microprocessors/stm32f4-series/documentation.html
- 6: http://www.bittiming.can-wiki.info/?CLK=36&calc=1&ctype=bxCAN
- 7: https://teachmemicro.com/stm32-can-bus-configuration-calculator
🏁 Script executed:
# Check if there's any documentation about the target CAN bus speed
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.rst" -o -name "*.pdf" \) | head -20 | xargs grep -l -i "can\|baud" 2>/dev/null | head -5Repository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 106
Recompute the CAN bit timing before bringing this up on the vehicle bus.
Both CAN controllers are configured with Prescaler 16, SJW 1TQ, BS1 1TQ, and BS2 1TQ—creating a 3-TQ bit time at 937500 bps. This violates ISO 11898-1, which requires 8–25 time quanta for robust CAN operation. A 3-TQ bit time lacks sufficient phase segments for proper synchronization, provides inadequate oscillator tolerance, and is incompatible with standard CAN devices configured for 500 kbps or 1 Mbps. Additionally, 937500 bps is not a standard CAN baud rate.
Also applies to: 74-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sensor_Nodes/Core/Src/can.c` around lines 42 - 46, The CAN bit timing is
currently 3 TQ (Prescaler=16, SJW=1TQ, BS1=1TQ, BS2=1TQ) which is
invalid—recompute and set hcan1.Init.* (and the same for hcan2 if present) to
yield 8–25 time quanta and a standard baud (e.g., 500 kbps or 1 Mbps) given your
MCU peripheral clock: choose Prescaler, SyncJumpWidth (SJW), TimeSeg1 (BS1) and
TimeSeg2 (BS2) so that (Prescaler * Tq) = CAN bit time and BS1+BS2+SJW+Sync = TQ
count between 8 and 25; update hcan1.Init.Prescaler, hcan1.Init.SyncJumpWidth,
hcan1.Init.TimeSeg1 and hcan1.Init.TimeSeg2 (and corresponding hcan2.Init
fields) accordingly before enabling the controllers.
| MX_I2C1_Init(); | ||
| /* USER CODE BEGIN 2 */ | ||
|
|
||
| FEB_Init(); |
Contributor
There was a problem hiding this comment.
Critical: FEB_CAN_Init() is not called — CAN buses will not function.
MX_CAN1_Init() and MX_CAN2_Init() only configure the HAL peripheral settings. Per Sensor_Nodes/Core/User/Src/FEB_CAN.c:20-45, FEB_CAN_Init() is required to:
- Configure CAN filters
- Start both CAN peripherals with
HAL_CAN_Start() - Enable RX interrupts with
HAL_CAN_ActivateNotification() - Register the RX callback
Without this call, the CAN peripherals remain stopped and no messages can be sent or received.
🐛 Proposed fix — call FEB_CAN_Init in FEB_Init()
In Sensor_Nodes/Core/User/Src/FEB_Main.c, update FEB_Init():
void FEB_Init(void)
{
// Initialize IMU only
lsm6dsox_init();
+
+ // Initialize CAN buses and register callback
+ FEB_CAN_Init(FEB_CAN_Rx_Callback);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sensor_Nodes/Core/Src/main.c` at line 105, FEB_Init currently doesn't call
FEB_CAN_Init so CAN peripherals never have filters configured, aren't started
with HAL_CAN_Start, RX interrupts aren't enabled with
HAL_CAN_ActivateNotification, nor is the RX callback registered; update FEB_Init
(the function that calls MX_CAN1_Init and MX_CAN2_Init) to call FEB_CAN_Init()
after the MX_CANx_Init calls (or at an appropriate point during initialization)
and handle/propagate any error return from FEB_CAN_Init so initialization fails
visibly if CAN setup fails.
Comment on lines
110
to
118
| while (1) | ||
| { | ||
| /* USER CODE END WHILE */ | ||
|
|
||
| /* USER CODE BEGIN 3 */ | ||
| FEB_Main_Loop(); | ||
| /* USER CODE END WHILE */ | ||
| } | ||
| /* USER CODE BEGIN 3 */ | ||
|
|
||
| /* USER CODE END 3 */ |
Contributor
There was a problem hiding this comment.
Misplaced USER CODE markers will cause issues with CubeMX regeneration.
The /* USER CODE END WHILE */ marker is inside the loop body (line 114) and /* USER CODE BEGIN 3 */ is outside the loop (line 116). This layout will cause CubeMX to potentially overwrite user code on regeneration.
🔧 Proposed fix
while (1)
{
-
FEB_Main_Loop();
- /* USER CODE END WHILE */
- }
- /* USER CODE BEGIN 3 */
-
+ /* USER CODE END WHILE */
+
+ /* USER CODE BEGIN 3 */
+
+ /* USER CODE END 3 */
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| while (1) | |
| { | |
| /* USER CODE END WHILE */ | |
| /* USER CODE BEGIN 3 */ | |
| FEB_Main_Loop(); | |
| /* USER CODE END WHILE */ | |
| } | |
| /* USER CODE BEGIN 3 */ | |
| /* USER CODE END 3 */ | |
| while (1) | |
| { | |
| FEB_Main_Loop(); | |
| /* USER CODE END WHILE */ | |
| /* USER CODE BEGIN 3 */ | |
| /* USER CODE END 3 */ | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sensor_Nodes/Core/Src/main.c` around lines 110 - 118, The USER CODE markers
around the infinite loop are misplaced: move the closing marker /* USER CODE END
WHILE */ to immediately before the loop exit (i.e., just after FEB_Main_Loop()
if that's the intended user section) and place /* USER CODE BEGIN 3 */ and /*
USER CODE END 3 */ inside the while(1) body (or vice versa) so that the markers
properly wrap user additions to FEB_Main_Loop(); ensure the markers surround the
user-editable region in main() rather than splitting it across inside/outside
the while loop and keep FEB_Main_Loop() call within the same USER CODE block.
Comment on lines
+68
to
+70
| uint16_t masks_flagless_mask = TPS2482_MASK_SOL | TPS2482_MASK_SUL | TPS2482_MASK_BOL | TPS2482_MASK_BUL | | ||
| TPS2482_MASK_CNVR | TPS2482_MASK_POL; | ||
| res[i] &= masks[i] == (masks_res[i] & masks_flagless_mask); |
Contributor
There was a problem hiding this comment.
Include APOL and LEN in the mask readback check.
Those bits are writable configuration in the mask register, but masks_flagless_mask strips them before comparison. TPS2482_Init() can therefore report success even when alert polarity or latch mode were not applied.
Minimal fix
- uint16_t masks_flagless_mask = TPS2482_MASK_SOL | TPS2482_MASK_SUL | TPS2482_MASK_BOL | TPS2482_MASK_BUL |
- TPS2482_MASK_CNVR | TPS2482_MASK_POL;
+ uint16_t masks_flagless_mask = TPS2482_MASK_SOL | TPS2482_MASK_SUL | TPS2482_MASK_BOL | TPS2482_MASK_BUL |
+ TPS2482_MASK_CNVR | TPS2482_MASK_POL | TPS2482_MASK_APOL |
+ TPS2482_MASK_LEN;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sensor_Nodes/Core/User/Src/TPS2482.c` around lines 68 - 70, The readback
comparison in TPS2482_Init() currently strips APOL and LEN by using
masks_flagless_mask (variable masks_flagless_mask) which lets the initialization
report success even if alert polarity or latch mode weren't applied; update the
comparison to include TPS2482_MASK_APOL and TPS2482_MASK_LEN in
masks_flagless_mask (or remove the masking and compare full masks[i] vs
masks_res[i]) so that the res[i] assignment uses the full writable mask bits
when evaluating success (i.e., change how masks_flagless_mask is constructed or
used in the res[i] &= masks[i] == (masks_res[i] & masks_flagless_mask) check to
account for APOL and LEN).
Comment on lines
+75
to
+99
| void TPS2482_Get_Register(I2C_HandleTypeDef *hi2c, uint8_t *addresses, uint8_t reg, uint16_t *results, | ||
| uint8_t messageCount) | ||
| { | ||
| /* Fixed-size array to avoid VLA - each register is 2 bytes */ | ||
| uint8_t res[2 * TPS2482_MAX_DEVICES]; | ||
|
|
||
| /* Bounds check to prevent buffer overflow */ | ||
| if (messageCount > TPS2482_MAX_DEVICES) | ||
| { | ||
| messageCount = TPS2482_MAX_DEVICES; | ||
| } | ||
|
|
||
| for (uint8_t i = 0; i < messageCount; i++) | ||
| { | ||
| if (HAL_I2C_Mem_Read(hi2c, addresses[i] << 1, reg, I2C_MEMADD_SIZE_8BIT, &res[2 * i], sizeof(*results), | ||
| HAL_MAX_DELAY) != HAL_OK) | ||
| { | ||
| results[i] = 0; // ERROR | ||
| } | ||
| else | ||
| { | ||
| results[i] = ((uint16_t)(res[2 * i]) << 8) | (uint16_t)(res[2 * i + 1]); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Propagate I2C failures instead of turning them into zeros or infinite waits.
TPS2482_Get_Register() writes 0 on read failure, and TPS2482_Write_Register() drops write errors entirely while both helpers block with HAL_MAX_DELAY. Because TPS2482_Init() only compares read-back values, zero-valued registers can be reported as successfully initialized even when the bus transaction failed.
Also applies to: 146-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sensor_Nodes/Core/User/Src/TPS2482.c` around lines 75 - 99,
TPS2482_Get_Register and TPS2482_Write_Register must stop swallowing I2C errors
and blocking forever: change both helpers to return a status (e.g.,
HAL_StatusTypeDef or int) instead of silently writing zeros/drop, stop using
HAL_MAX_DELAY for transactions (use a reasonable timeout parameter), propagate
HAL_I2C_Mem_Read/HAL_I2C_Mem_Write return codes to the caller and only fill
results on success (or set a clear sentinel/error flag per index) and ensure
TPS2482_Init checks those return values (and per-device read statuses) rather
than treating zero-valued reads as success; update references to
TPS2482_Get_Register, TPS2482_Write_Register, TPS2482_Init, HAL_I2C_Mem_Read,
HAL_I2C_Mem_Write and TPS2482_MAX_DEVICES when making these changes.
Comment on lines
+207
to
+214
| void TPS2482_Enable(GPIO_TypeDef **GPIOx, uint16_t *GPIO_Pin, uint8_t *en_dis, bool *result, uint8_t messageCount) | ||
| { | ||
| TPS2482_GPIO_Write(GPIOx, GPIO_Pin, en_dis, messageCount); | ||
|
|
||
| // HAL_Delay(100); | ||
|
|
||
| TPS2482_GPIO_Read(GPIOx, GPIO_Pin, (GPIO_PinState *)result, messageCount); | ||
| } |
Contributor
There was a problem hiding this comment.
Stop casting a bool * buffer to GPIO_PinState *.
TPS2482_Enable() is declared with bool *result, and the current callers pass a bool. Line 213 then writes GPIO_PinState elements through that pointer, which can overwrite adjacent memory on STM32 builds where the enum is wider than bool.
Safer implementation
void TPS2482_Enable(GPIO_TypeDef **GPIOx, uint16_t *GPIO_Pin, uint8_t *en_dis, bool *result, uint8_t messageCount)
{
TPS2482_GPIO_Write(GPIOx, GPIO_Pin, en_dis, messageCount);
- // HAL_Delay(100);
-
- TPS2482_GPIO_Read(GPIOx, GPIO_Pin, (GPIO_PinState *)result, messageCount);
+ for (uint8_t i = 0; i < messageCount; i++)
+ {
+ result[i] = (HAL_GPIO_ReadPin(GPIOx[i], GPIO_Pin[i]) == GPIO_PIN_SET);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sensor_Nodes/Core/User/Src/TPS2482.c` around lines 207 - 214, TPS2482_Enable
currently casts a bool* (result) to GPIO_PinState* and passes it to
TPS2482_GPIO_Read, which is unsafe on targets where GPIO_PinState is wider than
bool; change TPS2482_Enable to allocate a temporary array of GPIO_PinState of
size messageCount, call TPS2482_GPIO_Read(GPIOx, GPIO_Pin, tempStates,
messageCount), then convert each tempStates[i] to result[i] (true for
GPIO_PIN_SET, false for GPIO_PIN_RESET) before returning; alternatively, change
the function signature to accept GPIO_PinState *result if callers expect raw
GPIO states (update callers accordingly). Ensure you reference TPS2482_Enable,
TPS2482_GPIO_Read, TPS2482_GPIO_Write, and parameter names GPIOx, GPIO_Pin,
en_dis, result, messageCount when applying the fix.
Comment on lines
+11
to
+18
| CAN1.CalculateBaudRate=937500 | ||
| CAN1.CalculateTimeBit=1066 | ||
| CAN1.CalculateTimeQuantum=355.55555555555554 | ||
| CAN1.IPParameters=CalculateTimeQuantum,CalculateTimeBit,CalculateBaudRate | ||
| CAN2.CalculateBaudRate=937500 | ||
| CAN2.CalculateTimeBit=1066 | ||
| CAN2.CalculateTimeQuantum=355.55555555555554 | ||
| CAN2.IPParameters=CalculateTimeQuantum,CalculateTimeBit,CalculateBaudRate |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'CAN1_RX0_IRQn|CAN2_RX0_IRQn|HAL_CAN_IRQHandler|HAL_NVIC_EnableIRQ\(CAN|HAL_NVIC_SetPriority\(CAN' Sensor_NodesRepository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 34841
🏁 Script executed:
fd -type f -name "stm32f4xx_it.c"Repository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 256
🏁 Script executed:
fd -type file -name "stm32f4xx_it.c"Repository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 256
🏁 Script executed:
fd -t f -name "stm32f4xx_it.c"Repository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 316
🏁 Script executed:
find . -name "stm32f4xx_it.c"Repository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 339
🏁 Script executed:
cat -n Sensor_Nodes/Core/Src/stm32f4xx_it.c | head -150Repository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 5129
🏁 Script executed:
wc -l Sensor_Nodes/Core/Src/stm32f4xx_it.cRepository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 126
🏁 Script executed:
tail -100 Sensor_Nodes/Core/Src/stm32f4xx_it.cRepository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 2506
🏁 Script executed:
grep -n "CAN1_RX0_IRQHandler\|CAN2_RX0_IRQHandler\|CAN1_RX\|CAN2_RX" Sensor_Nodes/Core/Src/stm32f4xx_it.cRepository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 68
🏁 Script executed:
cat Sensor_Nodes/Sensor_Nodes.ioc | grep -i "CAN.*IRQ\|NVIC"Repository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 824
🏁 Script executed:
grep -A5 "HAL_CAN_ActivateNotification\|CAN_IT_RX_FIFO0_MSG_PENDING" Sensor_Nodes/Core/User/Src/FEB_CAN.cRepository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 366
Add CAN1_RX0_IRQn and CAN2_RX0_IRQn to the CubeMX .ioc NVIC configuration, or implement their handlers in stm32f4xx_it.c.
FEB_CAN.c activates CAN_IT_RX_FIFO0_MSG_PENDING for both CAN buses, but the .ioc file contains no NVIC entries for CAN1_RX0_IRQn or CAN2_RX0_IRQn, and stm32f4xx_it.c has no corresponding interrupt handlers. The interrupt will fire but have no registered handler. If the handlers were added manually elsewhere, they will be lost on the next CubeMX regeneration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sensor_Nodes/Sensor_Nodes.ioc` around lines 11 - 18, The project enables CAN
RX FIFO0 message-pending interrupts (CAN_IT_RX_FIFO0_MSG_PENDING) in FEB_CAN.c
but the CubeMX .ioc lacks NVIC entries for CAN1_RX0_IRQn and CAN2_RX0_IRQn and
stm32f4xx_it.c has no handlers, so add NVIC entries for CAN1_RX0_IRQn and
CAN2_RX0_IRQn in the CubeMX .ioc NVIC configuration or implement the ISR
handlers for those IRQs in stm32f4xx_it.c (e.g., provide CAN1_RX0_IRQHandler and
CAN2_RX0_IRQHandler that call the HAL/CAN receive-processing functions used by
FEB_CAN.c) so interrupts have registered handlers and won’t be lost on CubeMX
regeneration.
Comment on lines
+5
to
+7
| SourceFiles=Core/Src/main.c;Core/Src/gpio.c;Core/Src/freertos.c;Core/Src/dma.c;Core/Src/rtc.c;Core/Src/usart.c;Core/Src/stm32f4xx_it.c;Core/Src/stm32f4xx_hal_msp.c;Core/Src/stm32f4xx_hal_timebase_tim.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_tim.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_tim_ex.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_rcc.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_rcc_ex.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_flash.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_flash_ex.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_flash_ramfunc.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_gpio.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_dma_ex.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_dma.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_pwr.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_pwr_ex.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_cortex.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_exti.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_rtc.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_rtc_ex.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_uart.c;Middlewares/Third_Party/FreeRTOS/Source/croutine.c;Middlewares/Third_Party/FreeRTOS/Source/event_groups.c;Middlewares/Third_Party/FreeRTOS/Source/list.c;Middlewares/Third_Party/FreeRTOS/Source/queue.c;Middlewares/Third_Party/FreeRTOS/Source/stream_buffer.c;Middlewares/Third_Party/FreeRTOS/Source/tasks.c;Middlewares/Third_Party/FreeRTOS/Source/timers.c;Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.c;Middlewares/Third_Party/FreeRTOS/Source/portable/MemMang/heap_4.c;Middlewares/Third_Party/FreeRTOS/Source/portable/GCC/ARM_CM4F/port.c;Drivers/CMSIS/Device/ST/STM32F4xx/Source/Templates/system_stm32f4xx.c;Core/Src/system_stm32f4xx.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_tim.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_tim_ex.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_rcc.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_rcc_ex.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_flash.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_flash_ex.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_flash_ramfunc.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_gpio.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_dma_ex.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_dma.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_pwr.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_pwr_ex.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_cortex.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_exti.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_rtc.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_rtc_ex.c;Drivers/STM32F4xx_HAL_Driver/Src/stm32f4xx_hal_uart.c;Middlewares/Third_Party/FreeRTOS/Source/croutine.c;Middlewares/Third_Party/FreeRTOS/Source/event_groups.c;Middlewares/Third_Party/FreeRTOS/Source/list.c;Middlewares/Third_Party/FreeRTOS/Source/queue.c;Middlewares/Third_Party/FreeRTOS/Source/stream_buffer.c;Middlewares/Third_Party/FreeRTOS/Source/tasks.c;Middlewares/Third_Party/FreeRTOS/Source/timers.c;Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.c;Middlewares/Third_Party/FreeRTOS/Source/portable/MemMang/heap_4.c;Middlewares/Third_Party/FreeRTOS/Source/portable/GCC/ARM_CM4F/port.c;Drivers/CMSIS/Device/ST/STM32F4xx/Source/Templates/system_stm32f4xx.c;Core/Src/system_stm32f4xx.c;;;Middlewares/Third_Party/FreeRTOS/Source/croutine.c;Middlewares/Third_Party/FreeRTOS/Source/event_groups.c;Middlewares/Third_Party/FreeRTOS/Source/list.c;Middlewares/Third_Party/FreeRTOS/Source/queue.c;Middlewares/Third_Party/FreeRTOS/Source/stream_buffer.c;Middlewares/Third_Party/FreeRTOS/Source/tasks.c;Middlewares/Third_Party/FreeRTOS/Source/timers.c;Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.c;Middlewares/Third_Party/FreeRTOS/Source/portable/MemMang/heap_4.c;Middlewares/Third_Party/FreeRTOS/Source/portable/GCC/ARM_CM4F/port.c; | ||
| HeaderPath=Drivers/STM32F4xx_HAL_Driver/Inc;Drivers/STM32F4xx_HAL_Driver/Inc/Legacy;Middlewares/Third_Party/FreeRTOS/Source/include;Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2;Middlewares/Third_Party/FreeRTOS/Source/portable/GCC/ARM_CM4F;Drivers/CMSIS/Device/ST/STM32F4xx/Include;Drivers/CMSIS/Include;Core/Inc; | ||
| CDefines=USE_HAL_DRIVER;STM32F446xx;USE_HAL_DRIVER;USE_HAL_DRIVER; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification for duplicate and malformed tokens in .mxproject files.
set -euo pipefail
fd -a '\.mxproject$' | while read -r f; do
echo "=== $f ==="
rg -n ';;;|USE_HAL_DRIVER;USE_HAL_DRIVER|system_stm32f4xx\.h;.*system_stm32f4xx\.h' "$f" || true
rg -n '^(LibFiles|SourceFiles|CDefines)=' "$f" | sed -n '1,5p'
echo
doneRepository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 68
🏁 Script executed:
#!/bin/bash
# Find and examine the .mxproject file
fd -a '\.mxproject$' -type f | head -20Repository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 256
🏁 Script executed:
#!/bin/bash
# If file exists, read it and check specific lines
if [ -f "UART/.mxproject" ]; then
echo "=== UART/.mxproject exists ==="
wc -l "UART/.mxproject"
echo ""
echo "=== First 10 lines ==="
head -10 "UART/.mxproject"
echo ""
echo "=== Lines 5-7 specifically ==="
sed -n '5,7p' "UART/.mxproject"
else
echo "File UART/.mxproject does not exist"
# Search for any .mxproject file
find . -name "*.mxproject" 2>/dev/null
fiRepository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 18545
Remove or regenerate UART/.mxproject—contains duplicate source/header entries and malformed empty tokens.
Line 5 (SourceFiles) lists multiple HAL driver and FreeRTOS files twice, and contains the empty-token pattern ;;; between consecutive entries. Line 7 (CDefines) repeats USE_HAL_DRIVER three times. This is typical auto-generated tool residue that causes persistent diff churn. Either exclude this file from the PR scope or regenerate it via STM32CubeMX to normalize the metadata.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@UART/.mxproject` around lines 5 - 7, UART/.mxproject contains duplicated
entries and malformed tokens in the SourceFiles and CDefines fields (see symbols
"SourceFiles" and "CDefines") causing churn; fix by either removing this file
from the PR (unstage and add to .gitignore if it should not be tracked) or
regenerate/normalize it with STM32CubeMX so that SourceFiles no longer
duplicates HAL/FreeRTOS sources and removes the ";;;" empty tokens and CDefines
no longer repeats "USE_HAL_DRIVER"; verify HeaderPath and CDefines entries are
unique and clean before committing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Sensor_Nodes/cmake/stm32cubemx/CMakeLists.txt (1)
62-62: Typo in comment: "Midllewares" → "Middlewares".Minor typo that doesn't affect functionality.
✏️ Suggested fix
-# Drivers Midllewares +# Drivers Middlewares🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sensor_Nodes/cmake/stm32cubemx/CMakeLists.txt` at line 62, Fix the typo in the comment string "Drivers Midllewares" by changing it to "Drivers Middlewares" (locate the comment that currently reads "Drivers Midllewares" in the CMakeLists comment block and update the spelling to "Middlewares")..cubemx-manifest.json (1)
7-7: Reduce timestamp-only manifest churn.These
generated_atedits are the only changes for boards outside this PR’s Sensor_Nodes scope, so they only add review noise and merge-conflict surface. If the CubeMX sync can avoid rewriting untouched boards, future manifest diffs will be much cleaner.Also applies to: 78-78, 180-180, 200-200, 220-220, 248-248, 306-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.cubemx-manifest.json at line 7, The manifest churn is caused by always writing a fresh "generated_at" timestamp for every board on sync; update the CubeMX manifest-generation/sync routine (e.g., the function that writes the .cubemx-manifest.json or the sync_cubemx_manifests/generate_cubemx_manifest codepath) to only set or update the "generated_at" field when the board's manifest content actually changed, otherwise preserve the existing "generated_at" value; implement a content-diff check (or compare hashes) per-board before mutating the entry so untouched boards are not rewritten.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.cubemx-manifest.json:
- Around line 277-299: The MSP file lacks peripheral-specific initialization for
the newly enabled peripherals; implement or regenerate MSP hooks so each
peripheral configures its clocks, GPIO AFs, DMA and NVIC: add definitions for
HAL_ADC_MspInit, HAL_CAN_MspInit, HAL_I2C_MspInit, HAL_TIM_Encoder_MspInit, and
HAL_UART_MspInit (and matching DeInit functions as needed) to enable the
peripheral RCC clocks, initialize GPIO pins to the correct alternate functions,
configure/enable DMA channels and set interrupt priorities, or regenerate the
code from the .ioc so CubeMX emits these functions automatically.
---
Nitpick comments:
In @.cubemx-manifest.json:
- Line 7: The manifest churn is caused by always writing a fresh "generated_at"
timestamp for every board on sync; update the CubeMX manifest-generation/sync
routine (e.g., the function that writes the .cubemx-manifest.json or the
sync_cubemx_manifests/generate_cubemx_manifest codepath) to only set or update
the "generated_at" field when the board's manifest content actually changed,
otherwise preserve the existing "generated_at" value; implement a content-diff
check (or compare hashes) per-board before mutating the entry so untouched
boards are not rewritten.
In `@Sensor_Nodes/cmake/stm32cubemx/CMakeLists.txt`:
- Line 62: Fix the typo in the comment string "Drivers Midllewares" by changing
it to "Drivers Middlewares" (locate the comment that currently reads "Drivers
Midllewares" in the CMakeLists comment block and update the spelling to
"Middlewares").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6e83f64-5ee1-4f78-be9c-a146a47ddb3e
📒 Files selected for processing (8)
.cubemx-manifest.jsonSensor_Nodes/Core/Src/adc.cSensor_Nodes/Core/Src/can.cSensor_Nodes/Core/Src/i2c.cSensor_Nodes/Core/Src/main.cSensor_Nodes/Core/Src/tim.cSensor_Nodes/Core/Src/usart.cSensor_Nodes/cmake/stm32cubemx/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- Sensor_Nodes/Core/Src/main.c
- Sensor_Nodes/Core/Src/i2c.c
Comment on lines
+277
to
+299
| "ioc_checksum": "1c63ba671240fd83c275813d0e27d1e7c68dc928d8f6e8c96eccdc4dbeb84590", | ||
| "generated_at": "2026-04-03T00:13:43Z", | ||
| "files": { | ||
| "Core/Inc/adc.h": "0ac97025a4065f07b7b65a906bd0c45ea800215434ed43c287c1685507f3f612", | ||
| "Core/Inc/can.h": "98925d7f4010f054e17d240116244377377c15248a5413a09ff811cdc23084e5", | ||
| "Core/Inc/gpio.h": "e2d62f964a0b7f238384ed6a20a181d09df9647240a268dc547acf5b35099f74", | ||
| "Core/Inc/main.h": "e07a43ebc634ce451e2f80884378d112e048e06235fb49fdc2d9a2b5057d28c5", | ||
| "Core/Inc/stm32f4xx_hal_conf.h": "a7456a03b7c563da74771c7f179d5787d760b0f5508f4d739213e07f8604b0d8", | ||
| "Core/Inc/i2c.h": "f8ccbb92e950fd1d84e3851b46a5e327f04797b3596af4ad306373eee85840a4", | ||
| "Core/Inc/main.h": "fb5d6e26770d865375a8b39896b9f17f9d268cf0747ef52c697372b58461c546", | ||
| "Core/Inc/stm32f4xx_hal_conf.h": "73d247d57dfb67fbdd50432f98f71c6fe9c18d23a705828178ca7a3589f9e709", | ||
| "Core/Inc/stm32f4xx_it.h": "a96c6c7347c880644aeab9f9fe3ca9be2d61f123389fe8c2ce7dc4321501993a", | ||
| "Core/Inc/usart.h": "2ddd1eaf81e3750b46df7222f4c34ae295bcdaaaa1152e4be4933d3a0ca90181", | ||
| "Core/Src/gpio.c": "e797753346f487c8ad1643608c3378e52c266e4d633d9ce3b1ffb7b9c92388bc", | ||
| "Core/Src/main.c": "576f4c91af0744bbc496f35c823555416e6dbe7d4288643872b73c21562b6367", | ||
| "Core/Inc/tim.h": "b060d62de6437c2e1be10eb11aa9c07afba7588e10491d67739df5bacda5d4bd", | ||
| "Core/Inc/usart.h": "8c7ef615ce73773c33fdce85e6a9809f9eef65226b59e67e53e72c902a6a70a9", | ||
| "Core/Src/adc.c": "2635c7fe171aa8514bfb03abf5cab1b247a16e48cb8a6044f0c6aac56a4c3a3a", | ||
| "Core/Src/can.c": "589dc8b641d52fdd91ac9df2efabb17e604228248601e5017359fd25c7778525", | ||
| "Core/Src/gpio.c": "223a7c7c67a608e0063f54807fb18707ad2513bd7973947554d7619aea58642d", | ||
| "Core/Src/i2c.c": "ad7c6a964844941003cc19eac436fd531c82106f18ef2e2e24afddd6363c3425", | ||
| "Core/Src/main.c": "421f1f8493ca388aa030556fe275e99bc4dff4204da219402cdcc053f5e2fe13", | ||
| "Core/Src/stm32f4xx_hal_msp.c": "3217d99dc7006dc46ac384717641fe84386f05fc71db0a3340fa0eae810bc1b7", | ||
| "Core/Src/stm32f4xx_it.c": "6cdfba6e76754e5838ae60c659793a49f4f635f7aa5d7451c0bbd0bbc502e1c7", | ||
| "Core/Src/syscalls.c": "03c06c72eafb44b499814cf9234aba57dd830f345d9b8779b5d7090e78517600", | ||
| "Core/Src/sysmem.c": "ad20f7b1fa1e7c73330727747222cf2ffa3245ebf035230ee9a157545e79df95", | ||
| "Core/Src/usart.c": "f278cb5046ca6c2101b713ceddb7ac9fd5e07e4fc69f11039d48d143b13b9c0a", | ||
| "Core/Src/tim.c": "fa03c6b308e0ca91f14debadd749426ae4277d8824438332b4fca5492052ba99", | ||
| "Core/Src/usart.c": "3e131f734c7a3b21d4ab937c63fe125268383c8edd2d02c5f332c3205f361112", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -u
echo "== Sensor_Nodes/Core/Src/stm32f4xx_hal_msp.c =="
rg -nC3 'HAL_(ADC|CAN|I2C|UART)_MspInit|HAL_TIM_Encoder_MspInit' Sensor_Nodes/Core/Src/stm32f4xx_hal_msp.c || true
echo
echo "== Sensor_Nodes/Sensor_Nodes.ioc =="
rg -n 'ADC|CAN|I2C|TIM3|UART4' Sensor_Nodes/Sensor_Nodes.ioc || trueRepository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 3600
🏁 Script executed:
cat -n Sensor_Nodes/Core/Src/stm32f4xx_hal_msp.cRepository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 2880
The MSP file is missing critical peripheral initialization code.
This manifest update configures ADC, CAN, I2C, TIM, and UART peripherals in the .ioc file, but Sensor_Nodes/Core/Src/stm32f4xx_hal_msp.c contains only the generic HAL_MspInit() stub with no peripheral-specific hooks (no HAL_ADC_MspInit, HAL_CAN_MspInit, HAL_I2C_MspInit, HAL_TIM_Encoder_MspInit, or HAL_UART_MspInit functions). CubeMX should have generated these functions to configure GPIO alternate functions, RCC clocks, and DMA for the newly enabled peripherals. Regenerate the code from the .ioc file or manually add the required MSP initialization functions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.cubemx-manifest.json around lines 277 - 299, The MSP file lacks
peripheral-specific initialization for the newly enabled peripherals; implement
or regenerate MSP hooks so each peripheral configures its clocks, GPIO AFs, DMA
and NVIC: add definitions for HAL_ADC_MspInit, HAL_CAN_MspInit, HAL_I2C_MspInit,
HAL_TIM_Encoder_MspInit, and HAL_UART_MspInit (and matching DeInit functions as
needed) to enable the peripheral RCC clocks, initialize GPIO pins to the correct
alternate functions, configure/enable DMA channels and set interrupt priorities,
or regenerate the code from the .ioc so CubeMX emits these functions
automatically.
This was referenced Apr 16, 2026
Merged
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores