Zb/bms fixes#43
Open
ZacharyBer wants to merge 9 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved legacy low-level ADBMS drivers, command/CRC and hardware-abstraction headers; added a common register-level ADBMS library with platform hooks, STM32 HAL platform integration, a scheduled acquisition layer, a processing/app layer with new tasks/APIs, and migrated console/task call sites to the new interfaces. Changes
Sequence Diagram(s)sequenceDiagram
participant Task as BMSProcessingTask
participant Proc as BMS_Proc
participant App as BMS_App
participant Drv as ADBMS_Driver
participant Plat as Platform
participant HAL as STM32_HAL
Task->>+App: BMS_App_Init()
App->>+Plat: FEB_ADBMS_Platform_Init()
Plat->>+HAL: enable DWT / configure CS GPIO
HAL-->>-Plat: ready
Plat-->>-App: platform ready
App->>+Drv: ADBMS_Init(num_ics)
Drv->>+Plat: CS low + SPI write (wake/config)
Plat->>+HAL: HAL_SPI_Transmit
HAL-->>-Plat: tx done
Plat-->>-Drv: cs high
Drv->>Drv: parse registers, PEC verify -> update cache
Drv-->>-App: init success
rect rgba(200,150,255,0.5)
Task->>+Proc: BMS_Proc_RunFrame()
Proc->>+Drv: ADBMS_StartCellADC(...)
Drv->>+Plat: SPI write (start ADC)
Plat->>+HAL: HAL_SPI_Transmit
HAL-->>-Plat: done
Plat-->>-Drv: command sent
Proc->>+Drv: ADBMS_PollADC(timeout)
Drv->>+Plat: Poll (DelayMs / SPI read)
Plat->>+HAL: HAL_SPI_Receive
HAL-->>-Plat: rx data
Plat-->>-Drv: data
Drv->>Drv: verify PEC -> update g_adbms cache
Drv-->>-Proc: parsed voltages
Proc->>Proc: aggregate -> update g_bms_pack
end
rect rgba(150,200,150,0.5)
Proc->>+Drv: ADBMS_SetDischarge(mask)
Drv->>Drv: update cached CFGB
Proc->>+Drv: ADBMS_WriteRegister(REG_CFGB)
Drv->>+Plat: SPI write (config write)
Plat->>+HAL: HAL_SPI_Transmit
HAL-->>-Plat: done
Plat-->>-Drv: ack
Drv-->>-Proc: write complete
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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: 17
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@BMS/Core/User/Inc/ADBMS6830B_Commands.h`:
- Around line 139-142: The ADAX macro currently discards the channel's upper
select bit because ((ch & 0x10) << 6) merges into the 0x0410 base; update ADAX
so the ch bit4 is moved into the low byte (matching ADBMS6830B_adax()) by
replacing ((ch & 0x10) << 6) with ((ch & 0x10) >> 4) (i.e., ADAX becomes 0x0410
| ((ow & 0x01) << 8) | ((pup & 0x01) << 7) | ((ch & 0x10) >> 4) | ((ch & 0x0F)
<< 0)); also review ADAX2 to ensure it encodes the upper channel bit similarly
if needed.
In `@BMS/Core/User/Inc/ADBMS6830B_Registers.h`:
- Around line 113-221: The unions decode the wrong register map; update the
structures to match the datasheet: change ADBMS_STATA_t to only contain VREF2
and ITMP (raw size matching RDSTATA), update ADBMS_STATB_t to contain VD, VA and
VRES fields (not OV/UV cell flags), move the per-cell OV/UV flag bytes into
ADBMS_STATD_t (where RDSTATD holds OV/UV flags), revise ADBMS_STATE_t to expose
reserved bytes plus GPI and REV only, and modify ADBMS_PWM_t so PWMA holds
PWM1–PWM12 and PWMB contains only PWM13–PWM16 with remaining bytes reserved;
adjust packing and raw[] sizes accordingly and rename or keep field names
(VREF2, ITMP, VD, VA, VRES, C_OV_LO/C_OV_HI/C_UV_LO/C_UV_HI -> fields inside
ADBMS_STATD_t, GPI, REV, PWM13–PWM16) to match the datasheet mapping.
- Around line 49-111: The CFGA/CFGB bitfield layouts are incorrect vs the
datasheet; update ADBMS_CFGA_t and ADBMS_CFGB_t to match Table 55/56: in
ADBMS_CFGA_t make Byte0 keep CTH and REFON, make Byte1 entirely FLAG_D (8 bits),
move FC to the correct location (only FC[2:0] in Byte5 along with the status
bits) and adjust SOAKON/OWRNG/reserved/GPO fields so bytes map exactly as
datasheet specifies; in ADBMS_CFGB_t add the missing DTMEN and DTRNG bits,
expand DCTO to 6 bits, and ensure VUV/VOV packing (VUV_LO, VUV_HI, VOV_LO,
VOV_HI) and DCC_LO/DCC_HI align to the specified bytes with proper bit widths
and packing attributes so serialization produces the exact register image.
- Around line 19-24: The macro ADBMS_TOTAL_CELLS is incorrectly set to 18 for
the ADBMS6830B; update ADBMS_TOTAL_CELLS from 18 to 16 in ADBMS6830B_Registers.h
and search for any code that uses ADBMS_TOTAL_CELLS (e.g., buffer allocations,
loops, index calculations) to ensure they now iterate to 16 cells (not 18) and
properly handle the two reserved bytes in the final cell/PWM register group;
keep ADBMS_CELLS_PER_REG as 3 but ensure any logic that maps registers to cell
indices clamps/limits using the new ADBMS_TOTAL_CELLS constant.
- Around line 305-315: ADBMS_VoltageToThreshold currently casts and subtracts
which underflows for voltages <2.4mV and can overflow the 12-bit field; change
the implementation to compute the raw code as floor(voltage_mV / 2.4f) - 1
(using a signed/int intermediate), then clamp the result to the valid 12-bit
range 0..0xFFF before casting to uint16_t and returning; ensure the function
uses a signed temp variable to avoid wraparound and returns 0 when the computed
code is negative and 0x0FFF when it exceeds the 12-bit max.
In `@BMS/Core/User/Inc/FEB_ADBMS6830B_Driver.h`:
- Around line 49-98: Run clang-format on the updated header block to fix spacing
and comment alignment so the macros and doxygen blocks conform to project style;
reformat the block containing the ADBMS_Driver_Constants, ADBMS_Register_Types
and ADBMS_Bit_Masks sections (symbols like NUM_RX_BYT, REG_DATA_SIZE,
REG_WITH_PEC_SIZE, MAX_WRITE_BUFFER_SIZE, ADC_POLL_TIMEOUT, ADC_POLL_INCREMENT,
CFGRA, CFGRB, CELL, AUX, STAT, REFON_MASK, REFON_CLEAR_MASK, DCC_LOW_MASK,
DCC_HIGH_MASK, BYTE_MASK, NIBBLE_MASK, HALF_BYTE_SHIFT) and ensure consistent
indentation, single-space comment prefixes, aligned macro definitions, and
trailing comment spacing to satisfy the repository's clang-format configuration
so scripts/format.sh --check passes.
In `@BMS/Core/User/Inc/FEB_ADBMS6830B.h`:
- Around line 89-109: The AUX_CH enum values after GPIO10 are incorrectly
numbered (they become 11..17) and do not match the wire encoding used by the
low-level command layer (0x10..0x16), which will cause ADBMS6830B_adax() to
select incorrect AUX sources; fix by updating the AUX_CH definition to use the
explicit encoded values for VREF2..VRES (set to 0x10..0x16) or stop defining a
local enum and instead reuse the canonical channel enum from
ADBMS6830B_Commands.h so the values never drift (update the typedef enum AUX_CH
or replace its usage with the ADBMS6830B_Commands.h enum where ADBMS6830B_adax()
is called).
In `@BMS/Core/User/Inc/FEB_Const.h`:
- Around line 79-81: The block of macro definitions
(FEB_CELL_BALANCE_THRESHOLD_MV, FEB_CELL_BALANCE_INTERVAL_MS,
FEB_CELL_BALANCE_ALL_AT_ONCE) needs to be reformatted with the project's
clang-format style; run the repository formatter (e.g., ./scripts/format.sh or
clang-format) and apply the suggested changes so the header conforms to CI
formatting rules and ./scripts/format.sh --check passes.
In `@BMS/Core/User/Src/ADBMS6830B_Cmd.c`:
- Around line 173-190: ADBMS_PollADC currently polls by repeatedly calling
FEB_spi_read_byte(0xFF), which neither keeps CSB low after conversion start nor
issues a documented PLA* poll command, so it can return false completion; change
ADBMS_PollADC to perform a proper poll transaction: assert CS low (or follow
sequence used when ADC start was issued), send the appropriate poll opcode
(e.g., PLADC/PLCADC/PLSADC/PLAUX as required by the conversion started), clock
out the status bit via FEB_spi_transfer/txrx until the status indicates
completion or timeout, and then deassert CS; reuse/replace FEB_spi_read_byte
usages with the SPI transfer function that lets you control CS and clock cycles
and ensure the timeout math still uses milliseconds consistently in
ADBMS_PollADC.
- Around line 264-279: ADBMS_ReadAux currently only issues RDAUXA and fills
ic[].aux_codes[0..2], leaving channels 4–10 stale; update the function to read
all four AUX registers (RDAUXA, RDAUXB, RDAUXC, RDAUXD) by calling transmitCMDR
for each 8*total_ic block (or a single multi-block read if the device supports
it), append/assign the subsequent bytes into ic[i].aux_codes[3..9] in the
correct order for each IC, and preserve/return pec_errors (checking PEC for each
read if applicable); locate the logic around ADBMS_ReadAux, transmitCMDR,
read_buffer and aux_codes to implement the additional reads and mappings.
- Around line 73-90: The code in ADBMS_WriteConfig uses write_buffer sized by
total_ic but calls transmitCMDW which always serializes FEB_NUM_IC blocks, so
mismatch can overrun or truncate; fix by either validating at the top of
ADBMS_WriteConfig that total_ic == FEB_NUM_IC and return an error (or assert) if
not, or replace the transmitCMDW calls with a lower-level writer that accepts
the actual length (e.g., use/write a variant of transmitCMDW or write_68 that
takes total_ic) and pass total_ic so the write uses total_ic*6 bytes for both
WRCFGA and WRCFGB.
- Around line 218-224: The PEC check only validates the first IC because
pec10_calc and rx_pec are computed from read_buffer[0..7] once; fix by iterating
over all ICs in the returned frame (each IC uses an 8-byte block) and perform
pec10_calc and rx_pec per-block: for each index i from 0 to total_ic-1 compute
calc_pec = pec10_calc(6, &read_buffer[i*8]) and rx_pec from read_buffer[i*8 + 6]
| (read_buffer[i*8 + 7] << 8), increment pec_errors on mismatch; make the same
change for the other occurrence referenced (lines 252-258) using the same per-IC
8-byte offset logic.
In `@BMS/Core/User/Src/ADBMS6830B_Registers.c`:
- Around line 188-203: The read-indexing is reversed in ADBMS_ReadReg: instead
of using (FEB_NUM_IC - 1 - ic) to pick bytes from rx_buf returned by
transmitCMDR, compute the offset so IC 0 maps to the first device returned by
the chain (i.e., offset = ic * 8) and then memcpy 6 bytes from &rx_buf[offset]
into data; also add a defensive check that ic < FEB_NUM_IC before computing the
offset to avoid out-of-bounds reads and keep the existing TODO for PEC
validation.
- Around line 208-220: ADBMS_WriteReg currently builds tx_buf with only the
target IC slot populated, leaving other slots uninitialized and clobbering the
daisy chain; fix ADBMS_WriteReg by initializing the full tx_buf before memcpy —
either require/accept a full-chain 6*FEB_NUM_IC buffer from the caller or
implement a read-modify-write: call the register-read helper (e.g.,
ADBMS_ReadRegs or equivalent) to populate tx_buf with the current register image
for all FEB_NUM_IC devices, then overwrite the target slot using the existing
offset calculation and memcpy, validate ic is in range, and only then call
transmitCMDW(cmd, tx_buf). Ensure tx_buf is fully initialized to avoid sending
stack garbage to other ICs.
In `@BMS/Core/User/Src/FEB_ADBMS6830B.c`:
- Around line 712-715: The code is reading an unpopulated accumulator field
FEB_ACC.min_voltage_V causing min to stay zero; change the usage to the
populated field FEB_ACC.pack_min_voltage_V (both for the float assignment and
the LOG_D call) so the balancing computation and log use the actual minimum
computed by store_cell_voltages(); verify references to balancing_cycle,
FEB_ACC.pack_max_voltage_V and balancing_mask remain unchanged.
- Around line 818-820: The code currently clears balancing_mask to 0x0000 which
prevents any cells from being selected when FEB_CELL_BALANCE_ALL_AT_ONCE == 0;
change the reset logic so that balancing_mask is either restored to the initial
alternating pattern (e.g., the same value used at startup) instead of 0x0000, or
only reset balancing_mask when FEB_CELL_BALANCE_ALL_AT_ONCE is enabled while
still resetting balancing_cycle to 0; update the reset around balancing_mask and
balancing_cycle accordingly (refer to balancing_mask, balancing_cycle, and
FEB_CELL_BALANCE_ALL_AT_ONCE in the diff) so applied = bits & balancing_mask
continues to work after a restart.
In `@BMS/Core/User/Src/FEB_SM.c`:
- Around line 176-180: The shutdown relay must be opened before performing ADBMS
traffic; move the FEB_HW_BMS_Shutdown_Set(false) call to execute before
FEB_Stop_Balance(), or alternatively wrap the FEB_Stop_Balance() call with the
ADBMS mutex to ensure no isoSPI writes occur before the HV path is disabled.
Update the sequence around the FEB_Stop_Balance and FEB_HW_BMS_Shutdown_Set
calls in FEB_SM.c (and ensure callers in FEB_ADBMS6830B.c respect the mutex), so
the relay-open is guaranteed first and any ADBMS config writes happen only after
the relay is opened or under the ADBMS mutex.
🪄 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: 38c5f62c-9d62-48ac-baf3-93a424472e2d
📒 Files selected for processing (14)
BMS/Core/User/Inc/ADBMS6830B_Cmd.hBMS/Core/User/Inc/ADBMS6830B_Commands.hBMS/Core/User/Inc/ADBMS6830B_Registers.hBMS/Core/User/Inc/FEB_ADBMS6830B.hBMS/Core/User/Inc/FEB_ADBMS6830B_Driver.hBMS/Core/User/Inc/FEB_CMDCODES.hBMS/Core/User/Inc/FEB_Const.hBMS/Core/User/Src/ADBMS6830B_Cmd.cBMS/Core/User/Src/ADBMS6830B_Registers.cBMS/Core/User/Src/FEB_ADBMS6830B.cBMS/Core/User/Src/FEB_ADBMS6830B_Driver.cBMS/Core/User/Src/FEB_Commands.cBMS/Core/User/Src/FEB_SM.cDASH/Core/User/Src/FEB_Main.c
💤 Files with no reviewable changes (2)
- DASH/Core/User/Src/FEB_Main.c
- BMS/Core/User/Inc/FEB_CMDCODES.h
Comment on lines
+139
to
+142
| #define ADAX(ow, pup, ch) (0x0410 | ((ow & 0x01) << 8) | ((pup & 0x01) << 7) | ((ch & 0x10) << 6) | ((ch & 0x0F) << 0)) | ||
|
|
||
| // Start AUX2 ADC Conversions and Poll Status | ||
| #define ADAX2(ch) (0x0400 | ((ch & 0x0F) << 0)) |
Contributor
There was a problem hiding this comment.
ADAX drops the upper channel-select bit.
((ch & 0x10) << 6) folds into the base 0x0410, so channels like VREF2/VD/VA encode the same as their low-nibble counterparts. If this is meant to match ADBMS6830B_adax() in the driver, the upper bit needs to be shifted into the low byte instead.
Suggested fix
-#define ADAX(ow, pup, ch) (0x0410 | ((ow & 0x01) << 8) | ((pup & 0x01) << 7) | ((ch & 0x10) << 6) | ((ch & 0x0F) << 0))
+#define ADAX(ow, pup, ch) (0x0410 | ((ow & 0x01) << 8) | ((pup & 0x01) << 7) | ((ch & 0x10) << 2) | ((ch & 0x0F) << 0))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@BMS/Core/User/Inc/ADBMS6830B_Commands.h` around lines 139 - 142, The ADAX
macro currently discards the channel's upper select bit because ((ch & 0x10) <<
6) merges into the 0x0410 base; update ADAX so the ch bit4 is moved into the low
byte (matching ADBMS6830B_adax()) by replacing ((ch & 0x10) << 6) with ((ch &
0x10) >> 4) (i.e., ADAX becomes 0x0410 | ((ow & 0x01) << 8) | ((pup & 0x01) <<
7) | ((ch & 0x10) >> 4) | ((ch & 0x0F) << 0)); also review ADAX2 to ensure it
encodes the upper channel bit similarly if needed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
BMS/Core/Src/main.c (1)
111-111:⚠️ Potential issue | 🔴 CriticalPrintf retargeting to USART2 is not implemented; boot diagnostic output will be lost.
The printf call executes safely before the FreeRTOS scheduler starts, but the
__io_putchar()function has no implementation in the codebase. Whilesyscalls.cdeclares it as a weak extern and_write()calls it for each character, there is no corresponding definition that targets USART2. Without this implementation, the boot message will compile but produce no output at runtime. Add a__io_putchar()function that callsHAL_UART_Transmit(&huart2, ...)to properly retarget printf output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BMS/Core/Src/main.c` at line 111, Implement a proper __io_putchar() that forwards characters to USART2 so printf output is not lost: add a definition of __io_putchar(int ch) which uses HAL_UART_Transmit(&huart2, (uint8_t *)&ch, 1, HAL_MAX_DELAY) (or a short timeout) and returns the character or EOF on error; this will satisfy the weak extern used by syscalls.c/_write() and ensure the boot printf("[BOOT] UART ready `@115200` (USART2)\r\n") is transmitted over huart2.
🧹 Nitpick comments (2)
BMS/Core/User/Inc/FEB_Const.h (1)
57-68: LGTM on commented-out code removal.Commenting out the isoSPI configuration is appropriate during the architectural restructuring. Consider removing these lines entirely in a follow-up if redundant mode is not planned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BMS/Core/User/Inc/FEB_Const.h` around lines 57 - 68, Remove the entire commented-out isoSPI configuration block to avoid dead code clutter: delete the commented definitions for ISOSPI_MODE_REDUNDANT, ISOSPI_MODE_SPI1_ONLY, ISOSPI_MODE_SPI2_ONLY, the placeholder ISOSPI_MODE selection, and the redundant-mode settings ISOSPI_FAILOVER_PEC_THRESHOLD, ISOSPI_FAILOVER_LOCKOUT_MS, and ISOSPI_PRIMARY_CHANNEL from FEB_Const.h (or keep them only if you explicitly plan to reintroduce redundant mode, in which case replace with a short TODO comment referencing those symbols).BMS/Core/User/Src/FEB_ADBMS_Platform.c (1)
35-55: Consider adding SPI error handling for robustness.The HAL_SPI_Transmit/Receive functions return
HAL_StatusTypeDefwhich is currently ignored. While the ADBMS driver has its own PEC-based error detection, silent SPI failures could complicate debugging.🔧 Optional: Add error logging for debug builds
void ADBMS_Platform_SPI_Write(const uint8_t *data, uint16_t len) { - HAL_SPI_Transmit(ADBMS_SPI_HANDLE, (uint8_t *)data, len, ADBMS_SPI_TIMEOUT_MS); + HAL_StatusTypeDef status = HAL_SPI_Transmit(ADBMS_SPI_HANDLE, (uint8_t *)data, len, ADBMS_SPI_TIMEOUT_MS); + (void)status; /* PEC errors handled at driver level */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BMS/Core/User/Src/FEB_ADBMS_Platform.c` around lines 35 - 55, The SPI helper functions ADBMS_Platform_SPI_Write, ADBMS_Platform_SPI_Read and ADBMS_Platform_SPI_WriteRead currently ignore the HAL_SPI_Transmit/Receive return value; capture the HAL_StatusTypeDef result from each HAL_SPI_* call, and on non-HAL_OK call a new platform error handler (e.g., ADBMS_Platform_HandleSPIError(HAL_StatusTypeDef status)) to centralize retry/cleanup behavior; additionally add conditional debug logging in that handler (or inline) that prints the status and which function failed (use symbols ADBMS_SPI_HANDLE and ADBMS_SPI_TIMEOUT_MS when composing log messages). Ensure the new handler is declared/implemented in the same module so callers remain void and behavior stays consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@BMS/Core/User/Inc/FEB_Const.h`:
- Around line 27-28: The macro FEB_NUM_TEMP_SENSORS (defined as
FEB_NUM_TEMP_SENSE_PER_IC * FEB_NUM_ICPBANK) is unused—either remove this define
or add a clear comment explaining its intended purpose and relation to the
active sensor count (BMS_TEMP_TOTAL_SENSORS). If removing, delete the
FEB_NUM_TEMP_SENSORS definition and run a build to confirm no references remain;
if keeping, add a one-line comment next to FEB_NUM_TEMP_SENSORS describing why
it exists, how it differs from BMS_TEMP_TOTAL_SENSORS (e.g. per-FEB theoretical
vs. global configured total), and when it should be used so future reviewers
understand its role.
In `@BMS/Core/User/Src/FEB_ADBMS_App.c`:
- Around line 173-242: Reset g_bms_pack.last_error at the start of each new
voltage processing pass so stale errors don't persist: inside
BMS_App_ProcessVoltage (and the similar block at lines 359-491), set
g_bms_pack.last_error = BMS_APP_OK (or the success enum) before starting
ADBMS_StartCellADC/_Poll/_Read sequences (or right after the initialized check)
so subsequent successful polls can overwrite it only if new errors occur; ensure
later error paths still assign g_bms_pack.last_error when a new fault is
detected.
- Around line 468-490: The code currently sets g_bms_pack.temp_valid = true and
increments g_bms_pack.temp_read_count even when temp_count == 0; change the flow
so that only when temp_count > 0 (i.e., after you update
pack_min_temp_C/pack_max_temp_C/pack_avg_temp_C and per-bank aggregates) you
increment temp_read_count and set g_bms_pack.temp_valid = true; otherwise ensure
g_bms_pack.temp_valid is set to false and per-bank temp_valid flags are cleared
(or left false) before calling _validate_temperatures() so stale temperatures
are not treated as fresh — adjust logic around the temp_count conditional and
references to g_bms_pack.temp_read_count, g_bms_pack.temp_valid, and
banks[].temp_valid accordingly.
- Around line 494-510: _validate_temperatures currently faults on a single
out-of-range sample; change it to honor the consecutive-violation counters by
checking the per-sensor temp_violations against BMS_TEMP_ERROR_THRESHOLD before
setting g_bms_pack.last_error or logging. Specifically, in
_validate_temperatures(), instead of immediately assigning
BMS_APP_ERR_TEMP_HIGH/LOW when g_bms_pack.pack_max_temp_C > max_temp_C or
pack_min_temp_C < min_temp_C, verify the corresponding temp_violations counter
(the struct populated in the earlier loop) has reached or exceeded
BMS_TEMP_ERROR_THRESHOLD; only then set BMS_APP_ERR_TEMP_HIGH /
BMS_APP_ERR_TEMP_LOW and call LOG_E. Ensure you reference the same
temp_violations fields used in the code that increments them (so the check
matches the counters incremented in lines ~433-449).
- Around line 713-806: The getters (e.g., BMS_App_GetPackVoltage,
BMS_App_GetMinCellVoltage, BMS_App_GetCellVoltage, BMS_App_GetCellVoltageS,
BMS_App_GetMinTemp, BMS_App_GetMaxTemp, BMS_App_GetAvgTemp,
BMS_App_GetTempSensor, BMS_App_GetICTemp,
BMS_App_IsInitialized/IsVoltageValid/IsTempValid, BMS_App_GetLastError,
BMS_App_ClearError, BMS_App_GetPackData) read g_bms_pack without synchronization
and can observe torn updates from
BMS_App_ProcessVoltage/BMS_App_ProcessTemperature; fix by acquiring the same
pack mutex used by the processing code around each read (or copy) and releasing
it before return—either lock/unlock in each getter or provide a short, locked
snapshot copy of g_bms_pack to return safely; ensure you reuse the existing
mutex used by the processors (the pack mutex) rather than creating a new one so
reads and writes are mutually exclusive.
- Around line 896-899: FEB_Stop_Balance() calls BMS_App_StopBalancing() without
taking the same mutex used by the task path, which can race SPI/config writes
and corrupt hardware state or g_bms_pack; modify FEB_Stop_Balance to take
ADBMSMutexHandle (use the same lock/unlock primitives as the monitoring task),
call BMS_App_StopBalancing() while holding that mutex, then release the mutex,
ensuring the mutex acquisition is non-blocking or handles errors consistently
with the task path.
In `@BMS/Core/User/Src/FEB_Task_ADBMS.c`:
- Around line 171-178: When transitioning out of BMS_STATE_BALANCE the current
code silently skips BMS_App_StopBalancing() if osMutexAcquire(ADBMSMutexHandle,
pdMS_TO_TICKS(50)) times out; change this so the path cannot leave balancing
with discharge still enabled: modify the block guarded by s_prev_state ==
BMS_STATE_BALANCE to retry acquiring ADBMSMutexHandle (e.g. loop with small
sleep/backoff) until the mutex is obtained and then call BMS_App_StopBalancing()
and BMS_App_SetMode(BMS_MODE_NORMAL) and release the mutex, and if the mutex
cannot be acquired after a reasonable cumulative timeout escalate by invoking
the system fault/cleanup path (e.g. call the existing fault handler or set an
error flag) so discharge cannot remain enabled; update references around
s_prev_state, BMS_STATE_BALANCE, ADBMSMutexHandle, BMS_App_StopBalancing, and
BMS_App_SetMode accordingly.
- Around line 136-150: The initial voltage/temperature reads call
BMS_App_ProcessVoltage() and BMS_App_ProcessTemperature() while discarding their
return codes, allowing the task to proceed with stale g_bms_pack data; modify
the code to capture and check the return values from BMS_App_ProcessVoltage()
and BMS_App_ProcessTemperature(), and on any error (e.g. communication or pack
fault) log the specific error, release the ADBMSMutexHandle, set an appropriate
error/state flag in g_bms_pack or a task-local status, and abort or defer
further processing (e.g. return from the task or enter a safe wait) so the task
does not continue using invalid pack values. Ensure all paths still release
ADBMSMutexHandle before exiting.
In `@common/FEB_ADBMS_Library/Src/ADBMS6830B_Registers.c`:
- Around line 218-247: The loop that unpacks multi-IC read payloads stores data
into g_adbms.ics[ic] in forward order, mismatching the write (wire) order;
change the unpacking in the function containing the loop that manipulates
g_adbms.ics, tx_count, received_pec, calc_pec (uses ADBMS_CalcPEC10 and
ADBMS_PEC10_SIZE) so it iterates the chain in reverse (start at last IC and
decrement to 0), update the pointer arithmetic accordingly (advance ptr the same
way per-IC but assign into g_adbms.ics[ic_rev] or use an index var like ic =
g_adbms.num_ics-1 - i), and preserve the existing PEC check and memcpy to
dest_offsets[ic] so each device's payload lands under the correct g_adbms.ics[]
entry.
- Around line 2076-2087: The threshold helper functions are using a 1500 mV
offset and therefore are not inverses of the setter math; update
ADBMS_ThresholdCodeToMV and ADBMS_MVToThresholdCode to match the same formulas
used by ADBMS_SetUVThreshold()/ADBMS_SetOVThreshold() by removing the 1500 mV
offset: compute millivolts as code * 2.4 mV (i.e., (code * 24) / 10) in
ADBMS_ThresholdCodeToMV and compute code as voltage_mV * 10 / 24 (with
appropriate uint32_t casts and clamping to valid range) in
ADBMS_MVToThresholdCode so the two functions are true inverses of each other.
- Around line 1890-1918: The completion check in ADBMS_PollAux2ADC is inverted:
instead of returning success when rx_byte != 0xFF, change the condition to
return ADBMS_OK when rx_byte == 0xFF (matching
_poll_adc_internal()/ADBMS_PollAuxADC behavior). Locate ADBMS_PollAux2ADC (which
builds cmd_buf with PLAUX2, calculates PEC via ADBMS_CalcPEC15, sends via
ADBMS_Platform_SPI_WriteRead and reads rx_byte) and replace the if (rx_byte !=
0xFF) branch with if (rx_byte == 0xFF) return ADBMS_OK; so the function reports
ready only when the poll byte indicates completion.
- Around line 976-984: The GPO mask in ADBMS_SetGPO is currently masking 11 bits
(0x7FF) but the API exposes only GPIO1-10, so change the mask to 10 bits (0x3FF)
when assigning cfg.GPO to prevent bleeding into adjacent CFGA fields; update the
assignment in ADBMS_SetGPO (the line using CFGARA_DECODE/ENCODE and cfg.GPO) to
use gpio_mask & 0x3FF.
---
Outside diff comments:
In `@BMS/Core/Src/main.c`:
- Line 111: Implement a proper __io_putchar() that forwards characters to USART2
so printf output is not lost: add a definition of __io_putchar(int ch) which
uses HAL_UART_Transmit(&huart2, (uint8_t *)&ch, 1, HAL_MAX_DELAY) (or a short
timeout) and returns the character or EOF on error; this will satisfy the weak
extern used by syscalls.c/_write() and ensure the boot printf("[BOOT] UART ready
`@115200` (USART2)\r\n") is transmitted over huart2.
---
Nitpick comments:
In `@BMS/Core/User/Inc/FEB_Const.h`:
- Around line 57-68: Remove the entire commented-out isoSPI configuration block
to avoid dead code clutter: delete the commented definitions for
ISOSPI_MODE_REDUNDANT, ISOSPI_MODE_SPI1_ONLY, ISOSPI_MODE_SPI2_ONLY, the
placeholder ISOSPI_MODE selection, and the redundant-mode settings
ISOSPI_FAILOVER_PEC_THRESHOLD, ISOSPI_FAILOVER_LOCKOUT_MS, and
ISOSPI_PRIMARY_CHANNEL from FEB_Const.h (or keep them only if you explicitly
plan to reintroduce redundant mode, in which case replace with a short TODO
comment referencing those symbols).
In `@BMS/Core/User/Src/FEB_ADBMS_Platform.c`:
- Around line 35-55: The SPI helper functions ADBMS_Platform_SPI_Write,
ADBMS_Platform_SPI_Read and ADBMS_Platform_SPI_WriteRead currently ignore the
HAL_SPI_Transmit/Receive return value; capture the HAL_StatusTypeDef result from
each HAL_SPI_* call, and on non-HAL_OK call a new platform error handler (e.g.,
ADBMS_Platform_HandleSPIError(HAL_StatusTypeDef status)) to centralize
retry/cleanup behavior; additionally add conditional debug logging in that
handler (or inline) that prints the status and which function failed (use
symbols ADBMS_SPI_HANDLE and ADBMS_SPI_TIMEOUT_MS when composing log messages).
Ensure the new handler is declared/implemented in the same module so callers
remain void and behavior stays consistent.
🪄 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: 400a6ea4-6300-4449-ab34-fb299c955b83
📒 Files selected for processing (29)
.cubemx-manifest.json.vscode/settings.jsonBMS/CMakeLists.txtBMS/Core/Src/main.cBMS/Core/User/Inc/BMS_HW_Config.hBMS/Core/User/Inc/FEB_AD68xx_Interface.hBMS/Core/User/Inc/FEB_ADBMS6830B.hBMS/Core/User/Inc/FEB_ADBMS6830B_Driver.hBMS/Core/User/Inc/FEB_ADBMS_App.hBMS/Core/User/Inc/FEB_ADBMS_Platform.hBMS/Core/User/Inc/FEB_Commands.hBMS/Core/User/Inc/FEB_Config.hBMS/Core/User/Inc/FEB_Const.hBMS/Core/User/Inc/FEB_HW.hBMS/Core/User/Src/FEB_AD68xx_Interface.cBMS/Core/User/Src/FEB_ADBMS6830B.cBMS/Core/User/Src/FEB_ADBMS6830B_Driver.cBMS/Core/User/Src/FEB_ADBMS_App.cBMS/Core/User/Src/FEB_ADBMS_Platform.cBMS/Core/User/Src/FEB_Commands.cBMS/Core/User/Src/FEB_HW.cBMS/Core/User/Src/FEB_SM.cBMS/Core/User/Src/FEB_Task_ADBMS.ccommon/CMakeLists.txtcommon/FEB_ADBMS_Library/CMakeLists.txtcommon/FEB_ADBMS_Library/Inc/ADBMS6830B_Commands.hcommon/FEB_ADBMS_Library/Inc/ADBMS6830B_Memory_Map.hcommon/FEB_ADBMS_Library/Inc/ADBMS6830B_Registers.hcommon/FEB_ADBMS_Library/Src/ADBMS6830B_Registers.c
💤 Files with no reviewable changes (10)
- BMS/Core/User/Inc/FEB_Commands.h
- BMS/Core/User/Inc/FEB_Config.h
- BMS/Core/User/Src/FEB_ADBMS6830B_Driver.c
- BMS/Core/User/Inc/FEB_HW.h
- BMS/Core/User/Src/FEB_HW.c
- BMS/Core/User/Inc/FEB_AD68xx_Interface.h
- BMS/Core/User/Src/FEB_AD68xx_Interface.c
- BMS/Core/User/Src/FEB_ADBMS6830B.c
- BMS/Core/User/Inc/FEB_ADBMS6830B.h
- BMS/Core/User/Inc/FEB_ADBMS6830B_Driver.h
✅ Files skipped from review due to trivial changes (3)
- .vscode/settings.json
- common/FEB_ADBMS_Library/CMakeLists.txt
- common/FEB_ADBMS_Library/Inc/ADBMS6830B_Commands.h
🚧 Files skipped from review as they are similar to previous changes (1)
- BMS/Core/User/Src/FEB_SM.c
Comment on lines
+713
to
+806
| float BMS_App_GetPackVoltage(void) | ||
| { | ||
| return g_bms_pack.pack_voltage_V; | ||
| } | ||
|
|
||
| float BMS_App_GetMinCellVoltage(void) | ||
| { | ||
| return g_bms_pack.pack_min_cell_V; | ||
| } | ||
|
|
||
| float BMS_App_GetMaxCellVoltage(void) | ||
| { | ||
| return g_bms_pack.pack_max_cell_V; | ||
| } | ||
|
|
||
| float BMS_App_GetCellVoltage(uint8_t bank, uint8_t ic, uint8_t cell) | ||
| { | ||
| if (bank >= BMS_NUM_BANKS || ic >= BMS_ICS_PER_BANK || cell >= BMS_CELLS_PER_IC) | ||
| { | ||
| return 0.0f; | ||
| } | ||
| return g_bms_pack.banks[bank].ics[ic].cells[cell].voltage_C_V; | ||
| } | ||
|
|
||
| float BMS_App_GetCellVoltageS(uint8_t bank, uint8_t ic, uint8_t cell) | ||
| { | ||
| if (bank >= BMS_NUM_BANKS || ic >= BMS_ICS_PER_BANK || cell >= BMS_CELLS_PER_IC) | ||
| { | ||
| return 0.0f; | ||
| } | ||
| return g_bms_pack.banks[bank].ics[ic].cells[cell].voltage_S_V; | ||
| } | ||
|
|
||
| float BMS_App_GetMinTemp(void) | ||
| { | ||
| return g_bms_pack.pack_min_temp_C; | ||
| } | ||
|
|
||
| float BMS_App_GetMaxTemp(void) | ||
| { | ||
| return g_bms_pack.pack_max_temp_C; | ||
| } | ||
|
|
||
| float BMS_App_GetAvgTemp(void) | ||
| { | ||
| return g_bms_pack.pack_avg_temp_C; | ||
| } | ||
|
|
||
| float BMS_App_GetTempSensor(uint8_t bank, uint8_t sensor_idx) | ||
| { | ||
| if (bank >= BMS_NUM_BANKS || sensor_idx >= BMS_TEMP_TOTAL_SENSORS) | ||
| { | ||
| return 0.0f; | ||
| } | ||
| return g_bms_pack.banks[bank].temp_sensors_C[sensor_idx]; | ||
| } | ||
|
|
||
| float BMS_App_GetICTemp(uint8_t bank, uint8_t ic) | ||
| { | ||
| if (bank >= BMS_NUM_BANKS || ic >= BMS_ICS_PER_BANK) | ||
| { | ||
| return 0.0f; | ||
| } | ||
| return g_bms_pack.banks[bank].ics[ic].internal_temp_C; | ||
| } | ||
|
|
||
| bool BMS_App_IsInitialized(void) | ||
| { | ||
| return g_bms_pack.initialized; | ||
| } | ||
|
|
||
| bool BMS_App_IsVoltageValid(void) | ||
| { | ||
| return g_bms_pack.voltage_valid; | ||
| } | ||
|
|
||
| bool BMS_App_IsTempValid(void) | ||
| { | ||
| return g_bms_pack.temp_valid; | ||
| } | ||
|
|
||
| BMS_AppError_t BMS_App_GetLastError(void) | ||
| { | ||
| return g_bms_pack.last_error; | ||
| } | ||
|
|
||
| void BMS_App_ClearError(void) | ||
| { | ||
| g_bms_pack.last_error = BMS_APP_OK; | ||
| } | ||
|
|
||
| const BMS_PackData_t *BMS_App_GetPackData(void) | ||
| { | ||
| return &g_bms_pack; |
Contributor
There was a problem hiding this comment.
These getters are not actually thread-safe.
The public API documents this block as "acquire mutex internally", but every function here reads g_bms_pack directly. Callers using the documented contract can observe half-updated pack state while BMS_App_ProcessVoltage() or BMS_App_ProcessTemperature() is mutating the same structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@BMS/Core/User/Src/FEB_ADBMS_App.c` around lines 713 - 806, The getters (e.g.,
BMS_App_GetPackVoltage, BMS_App_GetMinCellVoltage, BMS_App_GetCellVoltage,
BMS_App_GetCellVoltageS, BMS_App_GetMinTemp, BMS_App_GetMaxTemp,
BMS_App_GetAvgTemp, BMS_App_GetTempSensor, BMS_App_GetICTemp,
BMS_App_IsInitialized/IsVoltageValid/IsTempValid, BMS_App_GetLastError,
BMS_App_ClearError, BMS_App_GetPackData) read g_bms_pack without synchronization
and can observe torn updates from
BMS_App_ProcessVoltage/BMS_App_ProcessTemperature; fix by acquiring the same
pack mutex used by the processing code around each read (or copy) and releasing
it before return—either lock/unlock in each getter or provide a short, locked
snapshot copy of g_bms_pack to return safely; ensure you reuse the existing
mutex used by the processors (the pack mutex) rather than creating a new one so
reads and writes are mutually exclusive.
Comment on lines
+136
to
+150
| /* Perform initial full read */ | ||
| if (osMutexAcquire(ADBMSMutexHandle, osWaitForever) == osOK) | ||
| { | ||
| LOG_I(TAG_ADBMS, "Performing initial voltage read..."); | ||
| BMS_App_ProcessVoltage(); | ||
|
|
||
| LOG_I(TAG_ADBMS, "Performing initial temperature scan..."); | ||
| BMS_App_ProcessTemperature(); | ||
|
|
||
| LOG_I(TAG_ADBMS, "Initial read complete:"); | ||
| LOG_I(TAG_ADBMS, " Pack Voltage: %.2f V", g_bms_pack.pack_voltage_V); | ||
| LOG_I(TAG_ADBMS, " Cell Range: %.3f - %.3f V", g_bms_pack.pack_min_cell_V, g_bms_pack.pack_max_cell_V); | ||
| LOG_I(TAG_ADBMS, " Temp Range: %.1f - %.1f C", g_bms_pack.pack_min_temp_C, g_bms_pack.pack_max_temp_C); | ||
|
|
||
| osMutexRelease(ADBMSMutexHandle); |
Contributor
There was a problem hiding this comment.
Fail fast when the initial scan returns a BMS error.
Lines 140-143 discard both return codes, so the task can log stale/default pack data and continue running even if the very first voltage or temperature pass already detected a comm fault or a pack fault.
Suggested fix
if (osMutexAcquire(ADBMSMutexHandle, osWaitForever) == osOK)
{
+ BMS_AppError_t voltage_err;
+ BMS_AppError_t temp_err;
+
LOG_I(TAG_ADBMS, "Performing initial voltage read...");
- BMS_App_ProcessVoltage();
+ voltage_err = BMS_App_ProcessVoltage();
LOG_I(TAG_ADBMS, "Performing initial temperature scan...");
- BMS_App_ProcessTemperature();
+ temp_err = BMS_App_ProcessTemperature();
+
+ osMutexRelease(ADBMSMutexHandle);
+
+ if (voltage_err != BMS_APP_OK)
+ {
+ _handle_error(voltage_err);
+ }
+ if (temp_err != BMS_APP_OK)
+ {
+ _handle_error(temp_err);
+ }
+ if (voltage_err != BMS_APP_OK || temp_err != BMS_APP_OK)
+ {
+ continue;
+ }
LOG_I(TAG_ADBMS, "Initial read complete:");
LOG_I(TAG_ADBMS, " Pack Voltage: %.2f V", g_bms_pack.pack_voltage_V);
LOG_I(TAG_ADBMS, " Cell Range: %.3f - %.3f V", g_bms_pack.pack_min_cell_V, g_bms_pack.pack_max_cell_V);
LOG_I(TAG_ADBMS, " Temp Range: %.1f - %.1f C", g_bms_pack.pack_min_temp_C, g_bms_pack.pack_max_temp_C);
-
- osMutexRelease(ADBMSMutexHandle);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@BMS/Core/User/Src/FEB_Task_ADBMS.c` around lines 136 - 150, The initial
voltage/temperature reads call BMS_App_ProcessVoltage() and
BMS_App_ProcessTemperature() while discarding their return codes, allowing the
task to proceed with stale g_bms_pack data; modify the code to capture and check
the return values from BMS_App_ProcessVoltage() and
BMS_App_ProcessTemperature(), and on any error (e.g. communication or pack
fault) log the specific error, release the ADBMSMutexHandle, set an appropriate
error/state flag in g_bms_pack or a task-local status, and abort or defer
further processing (e.g. return from the task or enter a safe wait) so the task
does not continue using invalid pack values. Ensure all paths still release
ADBMSMutexHandle before exiting.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
common/FEB_ADBMS_Library/Src/ADBMS6830B_Registers.c (2)
239-266:⚠️ Potential issue | 🟠 MajorReverse the IC order when unpacking multi-IC reads.
Writes already treat the wire order as “last IC first”, but this read loop still stores the first payload into
g_adbms.ics[0]. With more than one monitor in the chain, the cached data lands under the wrong IC index.Fix idea
- for (uint8_t ic = 0; ic < g_adbms.num_ics; ic++) + for (uint8_t wire_ic = 0; wire_ic < g_adbms.num_ics; wire_ic++) { + uint8_t ic = (uint8_t)(g_adbms.num_ics - 1U - wire_ic); g_adbms.ics[ic].status.tx_count++; /* Data is bytes_per_ic long */ uint8_t *data = ptr;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/FEB_ADBMS_Library/Src/ADBMS6830B_Registers.c` around lines 239 - 266, The unpack loop stores multi-IC read payloads in forward order but the wire protocol is "last IC first", so when g_adbms.num_ics > 1 the data ends up in the wrong monitor index; fix by mapping the incoming packet chunk to the reversed target index (e.g., target_ic = g_adbms.num_ics - 1 - ic) or by iterating ic from g_adbms.num_ics-1 down to 0, and then apply all updates to g_adbms.ics[target_ic] (status.tx_count, status.pec_error_count, status.comm_ok) and write into dest_offsets[target_ic] instead of g_adbms.ics[ic]; keep the same pointer advancement, PEC calculation (ADBMS_CalcPEC10), and memcpy logic but use the reversed index when assigning.
997-1005:⚠️ Potential issue | 🟠 MajorMask
gpio_maskto 10 bits, not 11.The public API only exposes GPIO1-10, but
0x7FFpreserves an extra bit and can corrupt a neighboring CFGA field when callers pass a full 16-bit mask.Fix idea
- cfg.GPO = gpio_mask & 0x7FF; + cfg.GPO = gpio_mask & 0x03FF;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/FEB_ADBMS_Library/Src/ADBMS6830B_Registers.c` around lines 997 - 1005, In ADBMS_SetGPO, the gpio_mask is currently masked with 0x7FF (11 bits) which can overwrite adjacent CFGA fields; change the mask to 10 bits (0x3FF) before assigning to cfg.GPO so only GPIO1-10 are preserved—update the masking operation in ADBMS_SetGPO (the line that sets cfg.GPO using gpio_mask & ...) and keep using CFGARA_DECODE/CFGARA_ENCODE as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/FEB_ADBMS_Library/Src/ADBMS6830B_Registers.c`:
- Around line 72-105: The file is missing the weak platform hook
ADBMS_Platform_GetTickMs which causes link failures if a target doesn't provide
it; add a weak function definition matching the header prototype uint32_t
ADBMS_Platform_GetTickMs(void) in this C file (following the pattern of other
weak hooks like ADBMS_Platform_DelayMs) that returns a safe default (e.g., 0) so
wake/poll paths can call it when a target-specific implementation is not
supplied.
- Around line 1816-1840: The three helper functions
ADBMS_ReadAllCellAndSVoltages, ADBMS_ReadAllAvgCellAndSVoltages, and
ADBMS_ReadAllAuxAndStatus currently call two separate reads
(ADBMS_ReadAllCellVoltages/ADBMS_ReadAllAveragedVoltages plus
ADBMS_ReadAllSVoltages, and ADBMS_ReadAllAux plus ADBMS_ReadAllStatus) which
must be replaced with single combined transactions using the device opcodes
RDCSALL, RDACSALL, and RDASALL respectively; change each helper to send the
single combined opcode, perform one SPI/comm transfer to fetch the combined
payload, and parse/populate the same buffers that the split helpers filled
(reusing the existing parsing logic or refactoring it into a common parser),
ensuring atomic snapshot semantics and correct byte lengths, or alternatively
rename these helpers to make it explicit they perform two transactions if you
cannot implement the combined opcodes now.
- Around line 2100-2111: ADBMS_MVToThresholdCode can produce values > 12 bits
which then get stuffed into the 12-bit VUV/VOV fields and wrap — clamp the
result to the 12-bit range before returning (mask or saturate to 0x0FFF) so
out-of-range voltage_mV values saturate instead of wrapping; also ensure
ADBMS_ThresholdCodeToMV treats its input code as a 12-bit value (mask with
0x0FFF) before converting to mV to keep conversions consistent with the register
width.
---
Duplicate comments:
In `@common/FEB_ADBMS_Library/Src/ADBMS6830B_Registers.c`:
- Around line 239-266: The unpack loop stores multi-IC read payloads in forward
order but the wire protocol is "last IC first", so when g_adbms.num_ics > 1 the
data ends up in the wrong monitor index; fix by mapping the incoming packet
chunk to the reversed target index (e.g., target_ic = g_adbms.num_ics - 1 - ic)
or by iterating ic from g_adbms.num_ics-1 down to 0, and then apply all updates
to g_adbms.ics[target_ic] (status.tx_count, status.pec_error_count,
status.comm_ok) and write into dest_offsets[target_ic] instead of
g_adbms.ics[ic]; keep the same pointer advancement, PEC calculation
(ADBMS_CalcPEC10), and memcpy logic but use the reversed index when assigning.
- Around line 997-1005: In ADBMS_SetGPO, the gpio_mask is currently masked with
0x7FF (11 bits) which can overwrite adjacent CFGA fields; change the mask to 10
bits (0x3FF) before assigning to cfg.GPO so only GPIO1-10 are preserved—update
the masking operation in ADBMS_SetGPO (the line that sets cfg.GPO using
gpio_mask & ...) and keep using CFGARA_DECODE/CFGARA_ENCODE as-is.
🪄 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: 08033461-7170-406a-b32c-abce4d578eb6
📒 Files selected for processing (6)
.cubemx-manifest.jsonBMS/Core/User/Inc/FEB_ADBMS_App.hBMS/Core/User/Src/FEB_ADBMS_App.cBMS/Core/User/Src/FEB_ADBMS_Platform.ccommon/FEB_ADBMS_Library/Inc/ADBMS6830B_Registers.hcommon/FEB_ADBMS_Library/Src/ADBMS6830B_Registers.c
🚧 Files skipped from review as they are similar to previous changes (4)
- .cubemx-manifest.json
- BMS/Core/User/Src/FEB_ADBMS_Platform.c
- BMS/Core/User/Src/FEB_ADBMS_App.c
- BMS/Core/User/Inc/FEB_ADBMS_App.h
Comment on lines
+1816
to
+1840
| ADBMS_Error_t ADBMS_ReadAllCellAndSVoltages(void) | ||
| { | ||
| /* Read both C-voltages and S-voltages */ | ||
| ADBMS_Error_t err = ADBMS_ReadAllCellVoltages(); | ||
| if (err != ADBMS_OK) | ||
| return err; | ||
| return ADBMS_ReadAllSVoltages(); | ||
| } | ||
|
|
||
| ADBMS_Error_t ADBMS_ReadAllAvgCellAndSVoltages(void) | ||
| { | ||
| /* Read both averaged C-voltages and S-voltages */ | ||
| ADBMS_Error_t err = ADBMS_ReadAllAveragedVoltages(); | ||
| if (err != ADBMS_OK) | ||
| return err; | ||
| return ADBMS_ReadAllSVoltages(); | ||
| } | ||
|
|
||
| ADBMS_Error_t ADBMS_ReadAllAuxAndStatus(void) | ||
| { | ||
| /* Read both auxiliary and status registers */ | ||
| ADBMS_Error_t err = ADBMS_ReadAllAux(); | ||
| if (err != ADBMS_OK) | ||
| return err; | ||
| return ADBMS_ReadAllStatus(); |
Contributor
There was a problem hiding this comment.
Use the actual combined read opcodes here, or rename these helpers.
These functions are documented as RDCSALL/RDACSALL/RDASALL helpers, but they currently issue two independent transactions. That doubles bus time and can mix C/S or AUX/STAT data from different snapshots if a new conversion completes between the reads.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@common/FEB_ADBMS_Library/Src/ADBMS6830B_Registers.c` around lines 1816 -
1840, The three helper functions ADBMS_ReadAllCellAndSVoltages,
ADBMS_ReadAllAvgCellAndSVoltages, and ADBMS_ReadAllAuxAndStatus currently call
two separate reads (ADBMS_ReadAllCellVoltages/ADBMS_ReadAllAveragedVoltages plus
ADBMS_ReadAllSVoltages, and ADBMS_ReadAllAux plus ADBMS_ReadAllStatus) which
must be replaced with single combined transactions using the device opcodes
RDCSALL, RDACSALL, and RDASALL respectively; change each helper to send the
single combined opcode, perform one SPI/comm transfer to fetch the combined
payload, and parse/populate the same buffers that the split helpers filled
(reusing the existing parsing logic or refactoring it into a common parser),
ensuring atomic snapshot semantics and correct byte lengths, or alternatively
rename these helpers to make it explicit they perform two transactions if you
cannot implement the combined opcodes now.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
BMS/Core/User/Src/FEB_Commands.c (2)
57-59: Nit: reuse_mode_name()instead of the nested ternary.Lines 385-398 already define
_mode_name()with a proper default branch; the ternary here will mislabel any futureBMS_OpMode_tvalue as "BALANCING".- FEB_Console_Printf("Mode: %s\r\n", pack->mode == BMS_MODE_NORMAL ? "NORMAL" - : pack->mode == BMS_MODE_CHARGING ? "CHARGING" - : "BALANCING"); + FEB_Console_Printf("Mode: %s\r\n", _mode_name(pack->mode));(Move
_mode_nameabovecmd_bms_statusor forward-declare it.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BMS/Core/User/Src/FEB_Commands.c` around lines 57 - 59, Replace the nested ternary in the FEB_Console_Printf call that prints the mode with a call to the existing helper _mode_name() to ensure unknown BMS_OpMode_t values are labeled correctly; update cmd_bms_status (or the function that contains that FEB_Console_Printf) to call _mode_name(pack->mode) instead of the ternary, and move the _mode_name definition above cmd_bms_status or add a forward-declaration for _mode_name so the symbol is visible at compile time.
53-82: CLI readsg_bms_packwithout the seqlock helpers.The header advertises
BMS_Pack_SeqBegin/SeqRetry/BMS_Pack_Snapshot()precisely so non-processing readers can get a consistent view. These commands dereferencepack->…directly while the processing task may be mid-write, producing occasionally inconsistent displayed values (e.g., a min > max split across fields). For console diagnostics this is acceptable, but consider snapshotting once withBMS_Pack_Snapshot()at the top of each command for cleaner output.Also applies to: 100-124, 142-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BMS/Core/User/Src/FEB_Commands.c` around lines 53 - 82, The CLI prints read pack fields directly from BMS_App_GetPackData() while the pack may be mid-update; replace the direct dereferences with a consistent snapshot read using the provided helpers: call BMS_Pack_Snapshot() (or use BMS_Pack_SeqBegin/SeqRetry loop) once at the top of the command and then read all fields from that snapshot variable instead of pack returned by BMS_App_GetPackData(), updating all locations (including the other ranges noted) so the displayed values are from a single consistent snapshot.BMS/Core/User/Src/FEB_ADBMS_App.c (1)
70-82: Nit: replace hardcoded6withsizeof(serial_id).
BMS_ICData_t::serial_idisuint8_t[6]; usingsizeofhere keeps_is_valid_serial_id, thememcpy, andADBMS_GetSerialID(ic, sid)in lockstep if the field width ever changes.Also applies to: 162-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BMS/Core/User/Src/FEB_ADBMS_App.c` around lines 70 - 82, Replace the hardcoded length 6 with the actual size of the serial_id field to keep lengths in sync: inside _is_valid_serial_id(const uint8_t *sid) iterate using sizeof(((BMS_ICData_t*)0)->serial_id) (or an equivalent sizeof serial_id expression) instead of 6, and make the same change where memcpy and ADBMS_GetSerialID(ic, sid) assume a length (the other occurrences around the ADBMS_GetSerialID/memcpy usage at lines ~162-174) so all uses derive the byte count from BMS_ICData_t::serial_id's sizeof rather than a literal.BMS/Core/User/Src/FEB_BMS_Acquisition.c (1)
197-227: Minor: scheduler does not catch up on missed periods.
jp->last_due_tick = nowresets the phase to "now" whenever a job runs, so if one job (e.g., AUX_SCAN, which sleeps per mux channel) overruns another's period, the overrun is not amortized and subsequent drift accumulates. If you want strict cadence usejp->last_due_tick += pdMS_TO_TICKS(jp->period_ms)(guard against runaway catch-up with a max drift). Not blocking — current behavior is fine for "best effort" sampling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BMS/Core/User/Src/FEB_BMS_Acquisition.c` around lines 197 - 227, The scheduler in BMS_Acq_ServiceScheduler resets jp->last_due_tick to now, which prevents catching up when jobs overrun; change the update to increment the last_due_tick by the job period (use jp->last_due_tick += pdMS_TO_TICKS(jp->period_ms)) instead of assigning now, and add a guard to prevent runaway catch-up (e.g., clamp the increment so last_due_tick never lags behind now by more than a configured max drift); update the logic around Job_t/jp->last_due_tick handling (affecting jobs like BMS_ACQ_JOB_AUX_SCAN) so missed periods are amortized safely.common/FEB_ADBMS_Library/Src/ADBMS6830B_Registers.c (1)
60-77:s_pec10_tableis unused and missingstatic.
ADBMS_CalcPEC10(Lines 143–165) computes PEC-10 bit-by-bit via the polynomial; the 512-bytes_pec10_tableis never referenced. Additionally, unlikes_pec15_table(Line 39), this one has nostaticqualifier, so it leaks as an external symbol with a generic name.Either switch
CalcPEC10to use the table (faster and matches the PEC-15 style), or drop the table and make sure it's at leaststaticif kept.Minimal fix if the bit-wise impl is intentional
-const uint16_t s_pec10_table[256] = { +static const uint16_t s_pec10_table[256] = {Or delete the table entirely to reclaim ~512 bytes of
.rodata.Also applies to: 143-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/FEB_ADBMS_Library/Src/ADBMS6830B_Registers.c` around lines 60 - 77, The s_pec10_table is defined without static and is unused by ADBMS_CalcPEC10 (which currently computes PEC-10 bitwise), leaking a public symbol unlike s_pec15_table; fix by either (A) updating ADBMS_CalcPEC10 to use s_pec10_table (mirror the PEC-15 table-driven approach for performance) or (B) remove s_pec10_table if you want the bitwise implementation, or at minimum mark s_pec10_table static to avoid external symbol leakage; reference s_pec10_table, ADBMS_CalcPEC10 and s_pec15_table when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@BMS/Core/User/Src/FEB_ADBMS_App.c`:
- Around line 207-212: BMS_App_SetMode and BMS_App_ClearError currently write
fields of g_bms_pack (e.g., mode, active_error_mask, voltage_error, temp_error,
comm_error, last_error, consecutive_pec_errors) directly from CLI tasks,
violating the processing-task-only writer invariant guarded by
g_bms_pack.snapshot_seq used by BMS_Pack_Snapshot/RunFrame; change these CLI
entrypoints to set request flags (follow the BMS_Proc_RequestStopBalancing
pattern) that the processing task drains and applies atomically in its RunFrame,
or alternately move the actual multi-field updates into the processing task with
a short writer-only snapshot_seq bump so updates are applied under the same
writer protection — update cmd_bms_mode/cmd_bms_pec to post the requests and
implement the handler in the processing loop to perform the g_bms_pack
modifications.
- Around line 117-134: After the ADBMS_SoftReset() call, the subsequent
ADBMS_WakeUp() return is ignored; update the code so the second ADBMS_WakeUp()
call is checked just like the first: capture its return value, validate against
ADBMS_OK, log an error with LOG_E(TAG_APP, "ADBMS_WakeUp failed: %d", err) on
failure, and return BMS_APP_ERR_INIT to abort initialization if it fails (this
change touches the ADBMS_SoftReset/ADBMS_WakeUp sequence in FEB_ADBMS_App.c).
In `@BMS/Core/User/Src/FEB_BMS_Processing.c`:
- Around line 541-552: The freshness check mixes tick and millisecond bases: now
is in ticks (osKernelGetTickCount()) while ADBMS_GetRegisterLastTickMs (from
ADBMS_Platform_GetTickMs/HAL_GetTick) returns milliseconds, but the code
subtracts ms from ticks then compares to pdMS_TO_TICKS(...). Fix by converting
the millisecond timestamps to ticks before subtracting: for each timestamp
returned by ADBMS_GetRegisterLastTickMs (cv_tick, aux_tick, statd_tick) compute
a tick equivalent via pdMS_TO_TICKS(cv_tick) etc (or change
ADBMS_Platform_GetTickMs to return ticks), and then perform (now -
cv_tick_in_ticks) <= pdMS_TO_TICKS(fresh_v_ms) (and similarly for t_fresh) to
ensure both operands use the same time base.
In `@BMS/Core/User/Src/FEB_Task_BMSProcessing.c`:
- Line 5: The header comment in FEB_Task_BMSProcessing.c incorrectly states
"Runs at 10 Hz" while the task period constant BMS_PROC_PERIOD_MS = 50 (and the
inline comment at the constant) yields 20 Hz; update the header block to state
"Runs at 20 Hz" (or alternatively change BMS_PROC_PERIOD_MS to 100 if you intend
10 Hz) and ensure the header text and the inline comment for BMS_PROC_PERIOD_MS
are consistent with the chosen period.
- Around line 49-63: The function _surface_faults_to_sm repeatedly calls
FEB_SM_Fault(BMS_STATE_FAULT_BMS) every tick while
g_bms_pack.voltage_error/temp_error (and BMS_App_GetActiveErrorMask()) remain
set; change this to edge-trigger the fault: add a static local latch (e.g.,
static bool fault_reported or static uint32_t prev_mask) inside
_surface_faults_to_sm, compare the current mask from
BMS_App_GetActiveErrorMask() to the previous value and call
FEB_SM_Fault(BMS_STATE_FAULT_BMS) only when a new fault bit appears (and set the
latch/prev_mask), and clear/reset the latch/prev_mask when mask == 0 so faults
can be re-reported next time.
---
Nitpick comments:
In `@BMS/Core/User/Src/FEB_ADBMS_App.c`:
- Around line 70-82: Replace the hardcoded length 6 with the actual size of the
serial_id field to keep lengths in sync: inside _is_valid_serial_id(const
uint8_t *sid) iterate using sizeof(((BMS_ICData_t*)0)->serial_id) (or an
equivalent sizeof serial_id expression) instead of 6, and make the same change
where memcpy and ADBMS_GetSerialID(ic, sid) assume a length (the other
occurrences around the ADBMS_GetSerialID/memcpy usage at lines ~162-174) so all
uses derive the byte count from BMS_ICData_t::serial_id's sizeof rather than a
literal.
In `@BMS/Core/User/Src/FEB_BMS_Acquisition.c`:
- Around line 197-227: The scheduler in BMS_Acq_ServiceScheduler resets
jp->last_due_tick to now, which prevents catching up when jobs overrun; change
the update to increment the last_due_tick by the job period (use
jp->last_due_tick += pdMS_TO_TICKS(jp->period_ms)) instead of assigning now, and
add a guard to prevent runaway catch-up (e.g., clamp the increment so
last_due_tick never lags behind now by more than a configured max drift); update
the logic around Job_t/jp->last_due_tick handling (affecting jobs like
BMS_ACQ_JOB_AUX_SCAN) so missed periods are amortized safely.
In `@BMS/Core/User/Src/FEB_Commands.c`:
- Around line 57-59: Replace the nested ternary in the FEB_Console_Printf call
that prints the mode with a call to the existing helper _mode_name() to ensure
unknown BMS_OpMode_t values are labeled correctly; update cmd_bms_status (or the
function that contains that FEB_Console_Printf) to call _mode_name(pack->mode)
instead of the ternary, and move the _mode_name definition above cmd_bms_status
or add a forward-declaration for _mode_name so the symbol is visible at compile
time.
- Around line 53-82: The CLI prints read pack fields directly from
BMS_App_GetPackData() while the pack may be mid-update; replace the direct
dereferences with a consistent snapshot read using the provided helpers: call
BMS_Pack_Snapshot() (or use BMS_Pack_SeqBegin/SeqRetry loop) once at the top of
the command and then read all fields from that snapshot variable instead of pack
returned by BMS_App_GetPackData(), updating all locations (including the other
ranges noted) so the displayed values are from a single consistent snapshot.
In `@common/FEB_ADBMS_Library/Src/ADBMS6830B_Registers.c`:
- Around line 60-77: The s_pec10_table is defined without static and is unused
by ADBMS_CalcPEC10 (which currently computes PEC-10 bitwise), leaking a public
symbol unlike s_pec15_table; fix by either (A) updating ADBMS_CalcPEC10 to use
s_pec10_table (mirror the PEC-15 table-driven approach for performance) or (B)
remove s_pec10_table if you want the bitwise implementation, or at minimum mark
s_pec10_table static to avoid external symbol leakage; reference s_pec10_table,
ADBMS_CalcPEC10 and s_pec15_table when making the change.
🪄 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: b7d0447e-c280-4d47-904a-1e2d380c3d86
📒 Files selected for processing (16)
.cubemx-manifest.jsonBMS/BMS.iocBMS/Core/Src/freertos.cBMS/Core/User/Inc/FEB_ADBMS_App.hBMS/Core/User/Inc/FEB_BMS_Acquisition.hBMS/Core/User/Inc/FEB_BMS_Processing.hBMS/Core/User/Inc/FEB_Task_BMSProcessing.hBMS/Core/User/Src/FEB_ADBMS_App.cBMS/Core/User/Src/FEB_ADBMS_Platform.cBMS/Core/User/Src/FEB_BMS_Acquisition.cBMS/Core/User/Src/FEB_BMS_Processing.cBMS/Core/User/Src/FEB_Commands.cBMS/Core/User/Src/FEB_Task_ADBMS.cBMS/Core/User/Src/FEB_Task_BMSProcessing.ccommon/FEB_ADBMS_Library/Inc/ADBMS6830B_Registers.hcommon/FEB_ADBMS_Library/Src/ADBMS6830B_Registers.c
✅ Files skipped from review due to trivial changes (2)
- BMS/Core/User/Inc/FEB_Task_BMSProcessing.h
- BMS/Core/User/Inc/FEB_ADBMS_App.h
🚧 Files skipped from review as they are similar to previous changes (2)
- .cubemx-manifest.json
- BMS/Core/User/Src/FEB_ADBMS_Platform.c
Comment on lines
+49
to
+63
| static void _surface_faults_to_sm(void) | ||
| { | ||
| uint32_t mask = BMS_App_GetActiveErrorMask(); | ||
| if (mask == 0) | ||
| return; | ||
|
|
||
| if (g_bms_pack.voltage_error == BMS_APP_ERR_VOLTAGE_UV || g_bms_pack.voltage_error == BMS_APP_ERR_VOLTAGE_OV) | ||
| { | ||
| FEB_SM_Fault(BMS_STATE_FAULT_BMS); | ||
| } | ||
| if (g_bms_pack.temp_error == BMS_APP_ERR_TEMP_HIGH || g_bms_pack.temp_error == BMS_APP_ERR_TEMP_LOW) | ||
| { | ||
| FEB_SM_Fault(BMS_STATE_FAULT_BMS); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
FEB_SM_Fault() is re-invoked every frame while the error remains latched.
BMS_App_GetActiveErrorMask() and the scalar voltage_error/temp_error fields stay set until BMS_App_ClearError() is called, but nothing here clears them or gates on "already in a fault state." Consequence: once a UV/OV/temp fault trips, FEB_SM_Fault(BMS_STATE_FAULT_BMS) is called ~20 times per second for as long as the task runs, polluting logs and potentially re-entering the FAULT transition on every tick.
Either latch locally (edge-trigger) or check current SM state before re-faulting.
Proposed edge-triggered fix
static void _surface_faults_to_sm(void)
{
+ static uint32_t s_last_surfaced_mask = 0;
uint32_t mask = BMS_App_GetActiveErrorMask();
- if (mask == 0)
+ if (mask == 0)
+ {
+ s_last_surfaced_mask = 0;
return;
+ }
+
+ /* Only surface new bits; avoid re-faulting every 50ms. */
+ uint32_t new_bits = mask & ~s_last_surfaced_mask;
+ s_last_surfaced_mask = mask;
+ if (new_bits == 0)
+ return;
if (g_bms_pack.voltage_error == BMS_APP_ERR_VOLTAGE_UV || g_bms_pack.voltage_error == BMS_APP_ERR_VOLTAGE_OV)
{
FEB_SM_Fault(BMS_STATE_FAULT_BMS);
}
if (g_bms_pack.temp_error == BMS_APP_ERR_TEMP_HIGH || g_bms_pack.temp_error == BMS_APP_ERR_TEMP_LOW)
{
FEB_SM_Fault(BMS_STATE_FAULT_BMS);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@BMS/Core/User/Src/FEB_Task_BMSProcessing.c` around lines 49 - 63, The
function _surface_faults_to_sm repeatedly calls
FEB_SM_Fault(BMS_STATE_FAULT_BMS) every tick while
g_bms_pack.voltage_error/temp_error (and BMS_App_GetActiveErrorMask()) remain
set; change this to edge-trigger the fault: add a static local latch (e.g.,
static bool fault_reported or static uint32_t prev_mask) inside
_surface_faults_to_sm, compare the current mask from
BMS_App_GetActiveErrorMask() to the previous value and call
FEB_SM_Fault(BMS_STATE_FAULT_BMS) only when a new fault bit appears (and set the
latch/prev_mask), and clear/reset the latch/prev_mask when mask == 0 so faults
can be re-reported next time.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@BMS/Core/User/Src/FEB_BMS_Processing.c`:
- Around line 118-145: _staged CFGB discharge (DCC) bits remain set when leaving
BMS_MODE_BALANCING because _stage_mode_thresholds only updates VUV/VOV and
requests a CFGB write; clear the DCC mask on mode transitions out of BALANCING.
After updating g_bms_pack.mode (e.g. in _drain_pending_requests), explicitly
clear the discharge bits in the CFGB staging for every IC (remove any DCC mask
previously set by _process_balancing) and then call the existing CFGB write
request (ADBMS_RequestWrite) so the physical bleed FETs are turned off;
reference functions/places: _drain_pending_requests, g_bms_pack.mode,
_process_balancing, CFGB/DCC, ADBMS_RequestWrite (and the existing
ADBMS_SetUVThreshold/ADBMS_SetOVThreshold calls in _stage_mode_thresholds) to
locate where to apply the clear-before-write.
- Around line 497-518: The pack-level over/under-temp check raises errors
immediately on pack_max_temp/pack_min_temp without debouncing; update
FEB_BMS_Processing.c so _raise_error(BMS_ERR_SRC_TEMP,
BMS_APP_ERR_TEMP_HIGH/LOW, ...) is only called when the offending sensor has its
temp_violations[sensor_index] >= BMS_TEMP_ERROR_THRESHOLD (use pk_max_sensor and
pk_min_sensor to index), or alternatively implement a pack-level consecutive
counter that increments when pack_max_temp > max_temp_threshold or pack_min_temp
< min_temp_threshold and only raises the error when that counter reaches
BMS_TEMP_ERROR_THRESHOLD (reset on safe readings); ensure LOG_E uses the same
gating logic so logs correspond to actual raised faults.
In `@BMS/Core/User/Src/FEB_Commands.c`:
- Around line 55-82: The CLI handlers currently call BMS_App_GetPackData() and
read fields directly, risking torn reads across prints; instead call
BMS_Pack_Snapshot() at the top of each handler (e.g., cmd_bms_status,
cmd_bms_cells, cmd_bms_temps) to copy the pack into a local snapshot struct and
then use that local snapshot for all FEB_Console_Printf() calls; replace every
use of the raw pointer (pack->...) in these handlers with the snapshot fields so
all printed values come from a single consistent copy.
- Around line 193-244: The handler mixes FreeRTOS ticks (osKernelGetTickCount)
with millisecond timestamps from ADBMS_GetRegisterLastTickMs causing incorrect
age math; replace the use of osKernelGetTickCount() with
ADBMS_Platform_GetTickMs() (or consistently convert
ADBMS_GetRegisterLastTickMs() to ticks with pdMS_TO_TICKS where you intend to
stay in ticks) so every age calculation and printed value in FEB_Commands.c
(references: osKernelGetTickCount, ADBMS_GetRegisterLastTickMs,
ADBMS_Platform_GetTickMs, pdMS_TO_TICKS) uses the same time base.
In `@common/FEB_ADBMS_Library/Inc/ADBMS6830B_Registers.h`:
- Around line 413-418: The pending_writes bitmask overflows for register groups
≥32; change the ADBMS_Chain_t.pending_writes field from uint32_t to uint64_t,
update any shifts to use 1ull << (ADBMS_RegGroup_t) (e.g., in ADBMS_RequestWrite
and callers) and adjust ADBMS_ConsumePendingWrites() and
ADBMS_GetPendingWrites() return types to uint64_t so the full 0..51 range is
representable; ensure all uses/assignments and any bitwise masks are updated to
64-bit to avoid undefined shifts.
🪄 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: bb6d5ebe-9b6d-4564-9378-88597d56a4ff
📒 Files selected for processing (7)
BMS/Core/User/Inc/FEB_BMS_Processing.hBMS/Core/User/Src/FEB_ADBMS_App.cBMS/Core/User/Src/FEB_BMS_Processing.cBMS/Core/User/Src/FEB_Commands.cBMS/Core/User/Src/FEB_Task_BMSProcessing.ccommon/FEB_ADBMS_Library/Inc/ADBMS6830B_Registers.hcommon/FEB_ADBMS_Library/Src/ADBMS6830B_Registers.c
🚧 Files skipped from review as they are similar to previous changes (2)
- BMS/Core/User/Src/FEB_Task_BMSProcessing.c
- common/FEB_ADBMS_Library/Src/ADBMS6830B_Registers.c
Comment on lines
+118
to
+145
| static void _stage_mode_thresholds(BMS_OpMode_t mode) | ||
| { | ||
| uint16_t uv_mv, ov_mv; | ||
| switch (mode) | ||
| { | ||
| case BMS_MODE_CHARGING: | ||
| uv_mv = BMS_CELL_UV_CHARGING_MV; | ||
| ov_mv = BMS_CELL_OV_CHARGING_MV; | ||
| break; | ||
| case BMS_MODE_BALANCING: | ||
| uv_mv = BMS_CELL_UV_BALANCING_MV; | ||
| ov_mv = BMS_CELL_OV_BALANCING_MV; | ||
| break; | ||
| case BMS_MODE_NORMAL: | ||
| default: | ||
| uv_mv = BMS_CELL_UV_NORMAL_MV; | ||
| ov_mv = BMS_CELL_OV_NORMAL_MV; | ||
| break; | ||
| } | ||
|
|
||
| for (uint8_t ic = 0; ic < BMS_TOTAL_ICS; ic++) | ||
| { | ||
| ADBMS_SetUVThreshold(ic, uv_mv); | ||
| ADBMS_SetOVThreshold(ic, ov_mv); | ||
| } | ||
| ADBMS_RequestWrite(ADBMS_REG_CFGB); | ||
| LOG_I(TAG_PROC, "Mode %d staged: UV=%dmV OV=%dmV", mode, uv_mv, ov_mv); | ||
| } |
Contributor
There was a problem hiding this comment.
Mode change leaves physical discharge enabled (stale DCC bits in CFGB).
_stage_mode_thresholds only overwrites VUV/VOV via ADBMS_SetUV/OVThreshold and then requests a CFGB write. CFGB also holds the discharge (DCC) bits, which were last staged by _process_balancing and are never cleared on a transition out of BMS_MODE_BALANCING:
- After mode flips to
NORMAL/CHARGING,_process_balancing(Line 541) early-returns without touching discharge. - The CFGB write requested here then broadcasts the stale DCC mask to every IC, keeping the bleed FETs on.
Clear discharge on any transition that leaves balancing, e.g. in _drain_pending_requests after updating g_bms_pack.mode:
if (s_req_mode_pending)
{
BMS_OpMode_t mode = s_req_mode;
+ BMS_OpMode_t prev = g_bms_pack.mode;
g_bms_pack.mode = mode;
s_req_mode_pending = false;
+ if (prev == BMS_MODE_BALANCING && mode != BMS_MODE_BALANCING)
+ {
+ BMS_Proc_RequestStopBalancing();
+ }
_stage_mode_thresholds(mode);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@BMS/Core/User/Src/FEB_BMS_Processing.c` around lines 118 - 145, _staged CFGB
discharge (DCC) bits remain set when leaving BMS_MODE_BALANCING because
_stage_mode_thresholds only updates VUV/VOV and requests a CFGB write; clear the
DCC mask on mode transitions out of BALANCING. After updating g_bms_pack.mode
(e.g. in _drain_pending_requests), explicitly clear the discharge bits in the
CFGB staging for every IC (remove any DCC mask previously set by
_process_balancing) and then call the existing CFGB write request
(ADBMS_RequestWrite) so the physical bleed FETs are turned off; reference
functions/places: _drain_pending_requests, g_bms_pack.mode, _process_balancing,
CFGB/DCC, ADBMS_RequestWrite (and the existing
ADBMS_SetUVThreshold/ADBMS_SetOVThreshold calls in _stage_mode_thresholds) to
locate where to apply the clear-before-write.
Comment on lines
+497
to
+518
| if (pack_count > 0) | ||
| { | ||
| g_bms_pack.pack_min_temp_C = pack_min_temp; | ||
| g_bms_pack.pack_max_temp_C = pack_max_temp; | ||
| g_bms_pack.pack_avg_temp_C = pack_sum / (float)pack_count; | ||
|
|
||
| float max_temp_threshold = (float)BMS_CELL_MAX_TEMP_DC / 10.0f; | ||
| float min_temp_threshold = (float)BMS_CELL_MIN_TEMP_DC / 10.0f; | ||
|
|
||
| if (pack_max_temp > max_temp_threshold) | ||
| { | ||
| _raise_error(BMS_ERR_SRC_TEMP, BMS_APP_ERR_TEMP_HIGH, pk_max_bank, 0xFF, pk_max_sensor); | ||
| LOG_E(TAG_PROC, "Over-temp: %.1fC > %.1fC (B%d S%d)", pack_max_temp, max_temp_threshold, pk_max_bank, | ||
| pk_max_sensor); | ||
| } | ||
| if (pack_min_temp < min_temp_threshold) | ||
| { | ||
| _raise_error(BMS_ERR_SRC_TEMP, BMS_APP_ERR_TEMP_LOW, pk_min_bank, 0xFF, pk_min_sensor); | ||
| LOG_E(TAG_PROC, "Under-temp: %.1fC < %.1fC (B%d S%d)", pack_min_temp, min_temp_threshold, pk_min_bank, | ||
| pk_min_sensor); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Pack-level over/under-temp error bypasses the consecutive-violation threshold.
Per-sensor violations (Lines 435–441) correctly debounce via temp_violations[] >= BMS_TEMP_ERROR_THRESHOLD, but the pack-aggregate check here faults immediately on the first frame where pack_max_temp > max_temp_threshold or pack_min_temp < min_temp_threshold. Since readings within the sensor validity window but outside the safety window always contribute to pack_max_temp/pack_min_temp, a single transient spike on one sensor is enough to trip the pack, then trip SM via BMS_App_GetActiveErrorMask().
Consider either (a) only raising TEMP_HIGH/TEMP_LOW when the offending sensor's temp_violations[] already crossed the threshold, or (b) adding a pack-level consecutive counter. Option (a) tends to compose better with the per-sensor logic:
- if (pack_max_temp > max_temp_threshold)
+ if (pack_max_temp > max_temp_threshold &&
+ g_bms_pack.banks[pk_max_bank].temp_violations[pk_max_sensor] >= BMS_TEMP_ERROR_THRESHOLD)
{
_raise_error(BMS_ERR_SRC_TEMP, BMS_APP_ERR_TEMP_HIGH, pk_max_bank, 0xFF, pk_max_sensor);
...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@BMS/Core/User/Src/FEB_BMS_Processing.c` around lines 497 - 518, The
pack-level over/under-temp check raises errors immediately on
pack_max_temp/pack_min_temp without debouncing; update FEB_BMS_Processing.c so
_raise_error(BMS_ERR_SRC_TEMP, BMS_APP_ERR_TEMP_HIGH/LOW, ...) is only called
when the offending sensor has its temp_violations[sensor_index] >=
BMS_TEMP_ERROR_THRESHOLD (use pk_max_sensor and pk_min_sensor to index), or
alternatively implement a pack-level consecutive counter that increments when
pack_max_temp > max_temp_threshold or pack_min_temp < min_temp_threshold and
only raises the error when that counter reaches BMS_TEMP_ERROR_THRESHOLD (reset
on safe readings); ensure LOG_E uses the same gating logic so logs correspond to
actual raised faults.
Comment on lines
+55
to
+82
| const BMS_PackData_t *pack = BMS_App_GetPackData(); | ||
|
|
||
| FEB_Console_Printf("=== BMS Status ===\r\n"); | ||
| FEB_Console_Printf("Initialized: %s\r\n", pack->initialized ? "YES" : "NO"); | ||
| FEB_Console_Printf("Mode: %s\r\n", _mode_name(pack->mode)); | ||
|
|
||
| if (!pack->initialized) | ||
| { | ||
| for (int cell = 0; cell < FEB_NUM_CELLS_PER_BANK; cell++) | ||
| { | ||
| float v_c = FEB_ADBMS_GET_Cell_Voltage(bank, cell); | ||
| float v_s = FEB_ADBMS_GET_Cell_Voltage_S(bank, cell); | ||
| if (v_c > 0) | ||
| { | ||
| if (v_c < min_c) | ||
| min_c = v_c; | ||
| if (v_c > max_c) | ||
| max_c = v_c; | ||
| } | ||
| if (v_s > 0) | ||
| { | ||
| if (v_s < min_s) | ||
| min_s = v_s; | ||
| if (v_s > max_s) | ||
| max_s = v_s; | ||
| } | ||
| } | ||
| FEB_Console_Printf("System not initialized - no data available\r\n"); | ||
| return; | ||
| } | ||
|
|
||
| FEB_Console_Printf("\r\n=== BMS Status ===\r\n"); | ||
| FEB_Console_Printf("State: %s\r\n", FEB_CAN_State_GetStateName(FEB_SM_Get_Current_State())); | ||
| FEB_Console_Printf("Pack Voltage: %.2fV\r\n", FEB_ADBMS_GET_ACC_Total_Voltage()); | ||
| FEB_Console_Printf("Min Cell (C/S): %.3fV / %.3fV\r\n", min_c, min_s); | ||
| FEB_Console_Printf("Max Cell (C/S): %.3fV / %.3fV\r\n", max_c, max_s); | ||
| FEB_Console_Printf("Min Temp: %.1fC Max Temp: %.1fC Avg: %.1fC\r\n", FEB_ADBMS_GET_ACC_MIN_Temp(), | ||
| FEB_ADBMS_GET_ACC_MAX_Temp(), FEB_ADBMS_GET_ACC_AVG_Temp()); | ||
| FEB_Console_Printf("Balancing: %s\r\n", FEB_Cell_Balancing_Status() ? "ON" : "OFF"); | ||
| FEB_Console_Printf("Error Type: 0x%02X\r\n", FEB_ADBMS_Get_Error_Type()); | ||
| FEB_Console_Printf("\r\n--- Pack Voltages ---\r\n"); | ||
| FEB_Console_Printf("Total: %.2f V\r\n", (double)pack->pack_voltage_V); | ||
| FEB_Console_Printf("Min Cell: %.3f V\r\n", (double)pack->pack_min_cell_V); | ||
| FEB_Console_Printf("Max Cell: %.3f V\r\n", (double)pack->pack_max_cell_V); | ||
| FEB_Console_Printf("Delta: %.1f mV\r\n", (double)((pack->pack_max_cell_V - pack->pack_min_cell_V) * 1000.0f)); | ||
|
|
||
| FEB_Console_Printf("\r\n--- Temperatures ---\r\n"); | ||
| FEB_Console_Printf("Min: %.1f C\r\n", (double)pack->pack_min_temp_C); | ||
| FEB_Console_Printf("Max: %.1f C\r\n", (double)pack->pack_max_temp_C); | ||
| FEB_Console_Printf("Avg: %.1f C\r\n", (double)pack->pack_avg_temp_C); | ||
|
|
||
| FEB_Console_Printf("\r\n--- Errors ---\r\n"); | ||
| FEB_Console_Printf("Last Error: %d\r\n", pack->last_error); | ||
| FEB_Console_Printf("Total PEC Errors: %lu\r\n", (unsigned long)pack->total_pec_errors); | ||
| FEB_Console_Printf("Voltage Valid: %s\r\n", pack->voltage_valid ? "YES" : "NO"); | ||
| FEB_Console_Printf("Temp Valid: %s\r\n", pack->temp_valid ? "YES" : "NO"); |
Contributor
There was a problem hiding this comment.
CLI commands read g_bms_pack directly, defeating the seqlock contract.
BMS_App_GetPackData() hands back a raw pointer to the global. The status/cells/temps handlers then read many fields across multiple FEB_Console_Printf() calls while the BMS processing task is the sole writer updating the same struct under its seqlock bracket (_pack_begin_write / _pack_end_write in FEB_BMS_Processing.c). A preempting processing frame mid-handler will produce a torn mix of old/new values (e.g. pack_min_cell_V from frame N, pack_max_cell_V from frame N+1, stale validity flags, inconsistent error fields), which is exactly what BMS_Pack_Snapshot() was introduced to prevent.
Take a local snapshot at the top of each handler and read from it:
static void cmd_bms_status(int argc, char *argv[])
{
(void)argc; (void)argv;
- const BMS_PackData_t *pack = BMS_App_GetPackData();
+ static BMS_PackData_t snap;
+ BMS_Pack_Snapshot(&snap, 8);
+ const BMS_PackData_t *pack = &snap;
...Same applies to cmd_bms_cells (Lines 100–125) and cmd_bms_temps (Lines 137–161).
Also applies to: 100-125, 137-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@BMS/Core/User/Src/FEB_Commands.c` around lines 55 - 82, The CLI handlers
currently call BMS_App_GetPackData() and read fields directly, risking torn
reads across prints; instead call BMS_Pack_Snapshot() at the top of each handler
(e.g., cmd_bms_status, cmd_bms_cells, cmd_bms_temps) to copy the pack into a
local snapshot struct and then use that local snapshot for all
FEB_Console_Printf() calls; replace every use of the raw pointer (pack->...) in
these handlers with the snapshot fields so all printed values come from a single
consistent copy.
Comment on lines
+193
to
244
| uint32_t now = osKernelGetTickCount(); | ||
|
|
||
| if (argc >= 2 && FEB_strcasecmp(argv[1], "fresh") == 0) | ||
| { | ||
| FEB_Console_Printf("Balancing: %s\r\n", FEB_Cell_Balancing_Status() ? "ON" : "OFF"); | ||
| FEB_Console_Printf("State: %s\r\n", FEB_CAN_State_GetStateName(FEB_SM_Get_Current_State())); | ||
| FEB_Console_Printf("Usage: BMS|balance|on or BMS|balance|off\r\n"); | ||
| FEB_Console_Printf("Note: Balancing only allowed in BATTERY_FREE or BALANCE states\r\n"); | ||
| uint32_t max_age = (argc >= 3) ? (uint32_t)atoi(argv[2]) : 500u; | ||
| FEB_Console_Printf("Register groups fresh within %lums:\r\n", (unsigned long)max_age); | ||
| for (int i = 0; i < (int)ADBMS_REG_COUNT; i++) | ||
| { | ||
| uint32_t t = ADBMS_GetRegisterLastTickMs((ADBMS_RegGroup_t)i); | ||
| if (t == 0) | ||
| continue; | ||
| uint32_t age = now - t; | ||
| if (age <= max_age) | ||
| { | ||
| FEB_Console_Printf(" %-7s age=%lums\r\n", ADBMS_GetRegisterName((ADBMS_RegGroup_t)i), (unsigned long)age); | ||
| } | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| if (argv[1][0] == 'o' && argv[1][1] == 'n') | ||
| if (argc >= 2) | ||
| { | ||
| /* Safety check: only allow balancing in safe states */ | ||
| if (!is_balancing_allowed()) | ||
| int grp = _find_reg_group_by_name(argv[1]); | ||
| if (grp < 0) | ||
| { | ||
| FEB_Console_Printf("Error: Balancing not allowed in %s state\r\n", | ||
| FEB_CAN_State_GetStateName(FEB_SM_Get_Current_State())); | ||
| FEB_Console_Printf("Allowed states: BATTERY_FREE, BALANCE\r\n"); | ||
| FEB_Console_Printf("Unknown register group: %s\r\n", argv[1]); | ||
| return; | ||
| } | ||
| FEB_Cell_Balance_Start(); | ||
| FEB_Console_Printf("Balancing started\r\n"); | ||
| } | ||
| else if (argv[1][0] == 'o' && argv[1][1] == 'f') | ||
| { | ||
| FEB_Stop_Balance(); | ||
| FEB_Console_Printf("Balancing stopped\r\n"); | ||
| FEB_Console_Printf("=== Register %s ===\r\n", ADBMS_GetRegisterName((ADBMS_RegGroup_t)grp)); | ||
| uint32_t t = ADBMS_GetRegisterLastTickMs((ADBMS_RegGroup_t)grp); | ||
| FEB_Console_Printf("Last update: %lums (age %lums)\r\n", (unsigned long)t, t == 0 ? 0UL : (unsigned long)(now - t)); | ||
| ADBMS_DumpRegister((ADBMS_RegGroup_t)grp, (ADBMS_PrintFunc_t)FEB_Console_Printf); | ||
| return; | ||
| } | ||
| else | ||
|
|
||
| FEB_Console_Printf("=== ADBMS Register Mirror ===\r\n"); | ||
| FEB_Console_Printf("Tick now: %lums\r\n", (unsigned long)now); | ||
| FEB_Console_Printf("%-8s %-12s %s\r\n", "group", "last_tick", "age_ms"); | ||
| for (int i = 0; i < (int)ADBMS_REG_COUNT; i++) | ||
| { | ||
| FEB_Console_Printf("Unknown option: %s\r\n", argv[1]); | ||
| FEB_Console_Printf("Usage: BMS|balance|on or BMS|balance|off\r\n"); | ||
| uint32_t t = ADBMS_GetRegisterLastTickMs((ADBMS_RegGroup_t)i); | ||
| const char *name = ADBMS_GetRegisterName((ADBMS_RegGroup_t)i); | ||
| if (t == 0) | ||
| { | ||
| FEB_Console_Printf("%-8s %-12s %s\r\n", name ? name : "?", "-", "never"); | ||
| } | ||
| else | ||
| { | ||
| FEB_Console_Printf("%-8s %-12lu %lu\r\n", name ? name : "?", (unsigned long)t, (unsigned long)(now - t)); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\bconfigTICK_RATE_HZ\b' -C2
rg -nP 'ADBMS_Platform_GetTickMs\s*\(' -C3Repository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 42699
🏁 Script executed:
head -n 250 BMS/Core/User/Src/FEB_Commands.c | tail -n +180Repository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 2461
🏁 Script executed:
rg -n 'osKernelGetTickCount' BMS/ --type cRepository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 1667
🏁 Script executed:
sed -n '383,390p' BMS/Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.cRepository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 258
🏁 Script executed:
rg -n 'ADBMS_GetRegisterLastTickMs' BMS/ -A 2 | head -20Repository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 1611
🏁 Script executed:
rg -n 'ADBMS_GetRegisterLastTickMs' common/FEB_ADBMS_Library/ -A 5Repository: Formula-Electric-Berkeley/FEB_FIRMWARE_SN5
Length of output: 1125
Fix mixed time bases in regs handler.
Line 193 uses osKernelGetTickCount() (FreeRTOS ticks), while ADBMS_GetRegisterLastTickMs() returns milliseconds from HAL_GetTick(). Age calculations at lines 204, 205, 223, and 241 mix these units and silently assume configTICK_RATE_HZ == 1000. If the tick rate changes, all printed ages become incorrect.
Use ADBMS_Platform_GetTickMs() to get milliseconds consistently, or convert all values to the same time domain using pdMS_TO_TICKS().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@BMS/Core/User/Src/FEB_Commands.c` around lines 193 - 244, The handler mixes
FreeRTOS ticks (osKernelGetTickCount) with millisecond timestamps from
ADBMS_GetRegisterLastTickMs causing incorrect age math; replace the use of
osKernelGetTickCount() with ADBMS_Platform_GetTickMs() (or consistently convert
ADBMS_GetRegisterLastTickMs() to ticks with pdMS_TO_TICKS where you intend to
stay in ticks) so every age calculation and printed value in FEB_Commands.c
(references: osKernelGetTickCount, ADBMS_GetRegisterLastTickMs,
ADBMS_Platform_GetTickMs, pdMS_TO_TICKS) uses the same time base.
Comment on lines
+413
to
+418
| /* Pending register-group writes requested by non-acquisition tasks. | ||
| * Bitmask of (1u << ADBMS_RegGroup_t). Chain-wide because ADBMS writes | ||
| * broadcast to all ICs in one daisy-chain transaction, with per-IC data | ||
| * already staged in g_adbms.ics[].memory by the requester. */ | ||
| volatile uint32_t pending_writes; | ||
| } ADBMS_Chain_t; |
Contributor
There was a problem hiding this comment.
pending_writes bitmask overflows for register groups with index ≥ 32.
ADBMS_RegGroup_t has 52 values (0..51). The chain-wide pending-write API (line 623) documents (1u << ADBMS_RegGroup_t) as the encoding, and ADBMS_RequestWrite() is advertised for groups like PWMA/PWMB (43/44), COMM (45), CMCFG..CMGPIOT (46..48), and RR (51). Shifting 1u (or even 1ul on a 32-bit ABI) by ≥ 32 is undefined behavior, and the bit simply cannot be stored in a uint32_t. Today only CFGA/CFGB are requested, so nothing blows up in practice, but any future use of ADBMS_RequestWrite(ADBMS_REG_PWMA) etc. will silently drop the request or corrupt the mask.
Widen to uint64_t (and use 1ull << reg) or gate ADBMS_RequestWrite with an assertion that reg < 32.
🔧 Suggested fix
- /* Pending register-group writes requested by non-acquisition tasks.
- * Bitmask of (1u << ADBMS_RegGroup_t). Chain-wide because ADBMS writes
- * broadcast to all ICs in one daisy-chain transaction, with per-IC data
- * already staged in g_adbms.ics[].memory by the requester. */
- volatile uint32_t pending_writes;
+ /* Pending register-group writes requested by non-acquisition tasks.
+ * Bitmask of (1ull << ADBMS_RegGroup_t). Chain-wide because ADBMS writes
+ * broadcast to all ICs in one daisy-chain transaction, with per-IC data
+ * already staged in g_adbms.ics[].memory by the requester.
+ * Must be >= ADBMS_REG_COUNT bits wide (52 groups today). */
+ volatile uint64_t pending_writes;And update the return types of ADBMS_ConsumePendingWrites() / ADBMS_GetPendingWrites() to uint64_t accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@common/FEB_ADBMS_Library/Inc/ADBMS6830B_Registers.h` around lines 413 - 418,
The pending_writes bitmask overflows for register groups ≥32; change the
ADBMS_Chain_t.pending_writes field from uint32_t to uint64_t, update any shifts
to use 1ull << (ADBMS_RegGroup_t) (e.g., in ADBMS_RequestWrite and callers) and
adjust ADBMS_ConsumePendingWrites() and ADBMS_GetPendingWrites() return types to
uint64_t so the full 0..51 range is representable; ensure all uses/assignments
and any bitwise masks are updated to 64-bit to avoid undefined shifts.
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
1 participant
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
Refactor
New Features
Chores