Zb/bms cc checking#52
Merged
Merged
Conversation
ADBMS6830B_rdcv() and ADBMS6830B_rdsv() previously had three coupled bugs: 1. The PEC comparison only validated IC 0's slice of the response buffer, silently skipping all other ICs in a multi-IC chain. 2. rx_pec was extracted via *(uint16_t *)(cell_data + TxSize - 2), which on ARM little-endian byte-swaps the PEC AND leaves the chip's command counter (CC[5:0]) in bits [9:2] of byte 6 untouched. The comparison was structurally guaranteed to fail once CC > 0. 3. cells.pec_match[REGGRP] was never populated. The consumer at FEB_ADBMS6830B.c:174 reads BSS-zero and treats every cell as PEC OK, so genuine PEC failures on the cell-voltage bus go undetected today. CV "appearing to work" was coincidence: rdcv's mismatch return was ignored by its caller (FEB_ADBMS6830B.c:146), and pec_match defaulted to zero. After this change, PEC is verified per-IC with the same masked extraction used by the rdaux/rdcfg/rdsid sites (((byte6 & 0x03) << 8) | byte7), and pec_match is correctly written so the consumer can fail closed. Also delete the dead #ifdef READALL branches in both functions. READALL is not defined anywhere in the project. cells.pec_match[] is shared between rdcv and rdsv (single member per register group). Since rdsv runs immediately after rdcv, the flag reflects rdsv's verdict downstream — pre-existing behavior, noted in a comment.
compute_pack_temp_stats() previously gated each sample to [-40C, +85C] (TEMP_VALID_MIN_DC / TEMP_VALID_MAX_DC). This silently dropped any reading above 85C from Min/Max/Avg, so when broken/disconnected thermistors saturated and reported ~88C the pack stats showed Max=79.4C while the temps display row above clearly showed 88.0C and 86.2C readings. The disagreement made the stats line useless and would also have hidden a genuinely overheating cell. Replace the validity window with an isfinite() gate. NaN sentinels from PEC failure (0xFFFF -> NaN at FEB_ADBMS6830B.c:321-323) are still excluded; real high/low readings are now included. validate_temps() keeps the original 85C cap by design — it serves a different purpose (sensor-health diagnostic and per-sensor violation counting that drives ERROR_TYPE_TEMP_VIOLATION). Letting hot sensor- fault sentinels through that gate would falsely flag cell over-temp faults instead of broken-thermistor faults. Comment added inline so the deliberate divergence is obvious to the next reader.
- ADBMS6830B_rdpwmga / ADBMS6830B_rdpwmgb were copying 8 bytes (6 data
+ 2 PEC) into pwm.rx_data without ever validating PEC. Apply the same
per-IC pattern used by rdcfga/rdcfgb/rdaux/rdsid:
Pec10_calc(true, 6, ic_data) ==
((ic_data[6] & 0x03) << 8) | ic_data[7]
Result is stored in pwm.rx_pec_match / pwmb.rx_pec_match (the field
was already on the struct, just never written). On allocation
failure, set rx_pec_match to -1 to match the rdcfga convention.
- Remove the STAT case from ADBMS6830B_check_pec(). It read
stat.pec_match[i] but no rdstat() exists in this firmware, so the
field is BSS-zero and the read just summed garbage. Replace with a
comment explaining the gap so a future rdstat author knows where to
reinstate the branch.
After commit 69d82ab migrated the last callers (rdcv/rdsv) to Pec10_calc(true, ...), the legacy lowercase pec10_calc() wrapper has no callers and is functionally identical to Pec10_calc(true, ...). crc10Table[256] was already dead — Pec10_calc uses a bit-shift CRC, not table lookup. Drop both: the function declaration in FEB_AD68xx_Interface.h, the function body and the table in FEB_AD68xx_Interface.c. Frees ~256B of .rodata and ~80 lines of duplicated logic.
The chip increments its 6-bit command counter on every valid command and echoes it in the upper 6 bits of byte 6 of every register-read response. Mirroring it host-side catches dropped commands and chip resets that PEC validation alone won't see (PEC will pass on a single corrupt-but-valid register read; CC drift won't). - ADBMS_CC_Advance() called from cmd_68 / cmd_68_r / write_68 — every path that actually transmits a command + PEC15 to the chips. wakeup_sleep is just a CS pulse (no command bytes), correctly skipped. - ADBMS_CC_Check(observed) called from every per-IC PEC validation block in FEB_ADBMS6830B_Driver.c (rdcfga/cfgb/aux/sid/pwm/cv/sv). observed = (ic_data[6] >> 2) & 0x3F. - Mismatches log via LOG_D(TAG_BMS, ...) and resync to the observed CC so we don't keep flagging the same drift. Rate-limited to first mismatch + every 64th to avoid log spam. - ADBMS_CC_GetMismatchCount() exposes the running counter for any future console / telemetry use. Single counter for the whole chain — every IC sees the same commands.
Contributor
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 52 minutes and 29 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughAdds a host-side 6-bit ADBMS command-counter mirror and API, updates CRC10 to optionally include CC bits for RX validation, advances the host CC on transmitted commands, integrates CC-checking into register-read PEC flows, accepts all finite temperature readings for stats, and bumps version files. ChangesCommand-Counter Validation System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
BMS/Core/User/Src/FEB_AD68xx_Interface.c (1)
100-149:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftMove the 6-iteration division loop inside the
if (bIsRxCmd)block — TX paths will otherwise compute incorrect PECs.The second modulo-2 division loop (lines 135–147) performs 6 bit shifts that exist solely to flush the 6 command-counter bits XORed into the remainder at line 132. Since the XOR only occurs when
bIsRxCmd == true, the 6 shifts must also be conditional. As written, all TX calls toPec10_calc(false, ...)(e.g.,write_68,transmitCMDW→wrcfga,wrcfgb,wrpwma,wrpwmb) execute those 6 additional shifts unconditionally, producing a final remainder that differs from the standard PEC10 algorithm. The ADBMS6830 will see a PEC mismatch and reject every write command.🛠️ Proposed fix — move the second loop inside the `if`
/* If array is from received buffer add command counter to crc calculation */ if (bIsRxCmd == true) { nRemainder ^= (uint16_t)(((uint16_t)pDataBuf[nLength] & (uint8_t)0xFC) << 2u); - } - /* Perform modulo-2 division, a bit at a time */ - for (nBitIndex = 6u; nBitIndex > 0u; --nBitIndex) - { - /* Try to divide the current data bit */ - if ((nRemainder & 0x200u) > 0u) + /* Perform modulo-2 division, a bit at a time, to flush the 6 CC bits */ + for (nBitIndex = 6u; nBitIndex > 0u; --nBitIndex) { - nRemainder = (uint16_t)((nRemainder << 1u)); - nRemainder = (uint16_t)(nRemainder ^ nPolynomial); - } - else - { - nRemainder = (uint16_t)((nRemainder << 1u)); + if ((nRemainder & 0x200u) > 0u) + { + nRemainder = (uint16_t)((nRemainder << 1u) ^ nPolynomial); + } + else + { + nRemainder = (uint16_t)(nRemainder << 1u); + } } } return ((uint16_t)(nRemainder & 0x3FFu));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@BMS/Core/User/Src/FEB_AD68xx_Interface.c` around lines 100 - 149, Pec10_calc currently performs a final 6-iteration modulo-2 division unconditionally, but those 6 shifts are only required when the command-counter bits are XORed into the remainder (the bIsRxCmd path); this causes TX calls (e.g., write_68, transmitCMDW → wrcfga/wrcfgb/wrpwma/wrpwmb) to compute an incorrect PEC. Fix by moving the second for-loop (the 6-bit modulo-2 division) entirely inside the if (bIsRxCmd == true) block immediately after the XOR that uses pDataBuf[nLength], so the extra shifts only run for rx-command calculations and not for TX paths in Pec10_calc.
🧹 Nitpick comments (5)
BMS/Core/User/Inc/FEB_AD68xx_Interface.h (1)
28-40: 💤 Low valueHeader declarations look fine; consider documenting
Pec10_calc's buffer-length contract.When
bIsRxCmd == true, the implementation readspDataBuf[nLength](i.e. one byte past the declared length) to incorporate the command-counter byte. That's an implicit precondition the docstring should call out so future callers don't pass a tightly sized buffer and trigger an OOB read. Same applies to the CC tracking block — it would help to note that these functions are not reentrant / not intended to be called from multiple tasks (see related comment in the .c file).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@BMS/Core/User/Inc/FEB_AD68xx_Interface.h` around lines 28 - 40, Document that Pec10_calc(bool bIsRxCmd, uint8_t nLength, uint8_t *pDataBuf) requires pDataBuf to be at least nLength+1 bytes when bIsRxCmd is true because the implementation reads the command-counter byte at pDataBuf[nLength]; add this precondition to the function comment and clarify ownership/validity expectations for pDataBuf. Also annotate the ADBMS_CC_Advance, ADBMS_CC_Reset, ADBMS_CC_Check, and ADBMS_CC_GetMismatchCount declarations with a short note that the CC tracking API is not reentrant / not thread-safe and must be called from a single task context (or protected by external synchronization) to match the .c file behavior.BMS/Core/User/Src/FEB_ADBMS6830B_Driver.c (3)
772-776: 💤 Low valueComment-only removal of STAT branch is fine; consider clearing
crc_count.stat_pec[]from any reporting path.The rationale in the comment is correct —
stat.pec_match[]is never written so aggregating it would just sum BSS-zero entries — but downstream telemetry / printf paths that exposecrc_count.stat_pec[i]will now report a permanently-zero "STAT PEC error count", which can be misinterpreted as "STAT is healthy". Either drop the field from the reporting surface or annotate it as "not-implemented" so operators don't trust a fake green light.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@BMS/Core/User/Src/FEB_ADBMS6830B_Driver.c` around lines 772 - 776, The STAT branch removal leaves crc_count.stat_pec[] permanently zero which can be misinterpreted as a healthy STAT; update the reporting code that exposes crc_count.stat_pec[] (where reports/telemetry are assembled or printed) to either (a) omit/statistically hide the stat_pec field when ADBMS6830B_rdstat() is not present, or (b) annotate the output for crc_count.stat_pec[] as "N/I" or "not-implemented" so operators know it is not a real measurement; keep reset_crc_count() behavior unchanged but ensure any reporting/printf path checks the absence of ADBMS6830B_rdstat() (or a feature flag) before presenting crc_count.stat_pec[].
305-316: ⚖️ Poor tradeoff
pec_match[REGGRP]is shared betweenrdcvandrdsv, so the rdcv verdict is always overwritten.The inline comment already acknowledges this, but it remains a real correctness gap for any consumer (e.g.
check_pecaggregatingcells.pec_match[i]) that wants to know "did this register group's C-ADC read pass PEC?": afterrdsvruns, the answer always reflects the S-ADC read instead. If the intent is to fail-safe report worst-of-the-two, OR them rather than overwrite; if the intent is per-ADC tracking, split intoc_pec_match[]ands_pec_match[]. Same comment block at lines 350–352 inrdsv.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@BMS/Core/User/Src/FEB_ADBMS6830B_Driver.c` around lines 305 - 316, The code currently overwrites ic[icn].cells.pec_match[REGGRP] in rdcv and rdsv, losing rdcv's result; update the assignment so it preserves prior verdict (OR the new mismatch with existing value) if you want a fail-safe "worst-of-the-two" result, or alternatively add separate per-ADC fields (e.g., c_pec_match[] and s_pec_match[]) and write rdcv to c_pec_match[REGGRP] and rdsv to s_pec_match[REGGRP]; modify the write in the rdcv/rdsv blocks (the ic[icn].cells.pec_match assignment) and update any consumers like check_pec / cells.pec_match[i] to read the chosen field or compute the combined verdict.
543-545: 💤 Low valueMemory-allocation failure path silently leaves the register's
rx_datastale.In both
rdpwmga(lines 543–545) andrdpwmgb(lines 590–592), an alloc failure flagsrx_pec_match = -1and returns without touchingpwm.rx_data/pwmb.rx_data, so any consumer that ignoresrx_pec_matchreads stale (or pre-init zero) PWM data. Consider also zeroing the rx buffer or adding an explicit "data invalid" sentinel for the PWM consumers, so a missedrx_pec_matchcheck can't silently feed stale PWM duty-cycles back into balancing decisions.Also applies to: 590-592
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@BMS/Core/User/Src/FEB_ADBMS6830B_Driver.c` around lines 543 - 545, On allocation failure paths in rdpwmga and rdpwmgb you currently set ic[bank].pwm.rx_pec_match = -1 and return but leave ic[bank].pwm.rx_data (and ic[bank].pwmb.rx_data in the GB path) containing stale bytes; update those error returns to also mark the data invalid by clearing the rx buffers (e.g., zeroing the bytes) or writing a distinct invalid sentinel into ic[bank].pwm.rx_data and ic[bank].pwmb.rx_data so consumers cannot accidentally use stale PWM duty values, keeping the existing ic[bank].pwm.rx_pec_match = -1 behavior.BMS/Core/User/Src/FEB_AD68xx_Interface.c (1)
40-48: 💤 Low valueRate-limit logic silently stops logging once
s_cc_mismatch_countsaturates.Once
s_cc_mismatch_countreaches0xFFFF, the saturation branch at line 40 stops incrementing. The next iteration evaluates0xFFFF & 0x3F == 63, never1, so no further drift is ever logged — the operator loses visibility on a system that is by then drifting badly. Two simple options: (a) keep a separate "logged" decimator that doesn't saturate, or (b) gate logging on the raw count before the saturation check.♻️ One option
- if (s_cc_mismatch_count != 0xFFFF) - s_cc_mismatch_count++; - - // Rate-limit logging: log on the first mismatch and then every 64th. - if ((s_cc_mismatch_count & 0x3F) == 1) + uint16_t prev = s_cc_mismatch_count; + if (s_cc_mismatch_count != 0xFFFF) + s_cc_mismatch_count++; + + // Rate-limit logging: log on the first mismatch and then every 64th, + // independent of saturation. + if (((prev + 1u) & 0x3Fu) == 1u)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@BMS/Core/User/Src/FEB_AD68xx_Interface.c` around lines 40 - 48, s_cc_mismatch_count currently saturates at 0xFFFF which makes the (s_cc_mismatch_count & 0x3F) == 1 logging condition stop firing; add a separate non‑saturating counter (e.g. s_cc_mismatch_total of type uint32_t) that you increment on every mismatch and use that counter for the rate‑limit check and LOG_D calls (keep s_cc_mismatch_count behavior for storage/backward compatibility). Update the mismatch path that currently touches s_cc_mismatch_count so it also increments s_cc_mismatch_total, and change the if condition to use s_cc_mismatch_total & 0x3F == 1 while keeping the same LOG_D(TAG_BMS, "CC drift: expected=%u observed=%u count=%u", (unsigned)s_expected_cc, (unsigned)observed, (unsigned)s_cc_mismatch_count) call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@BMS/Core/User/Src/FEB_ADBMS6830B_Driver.c`:
- Around line 311-312: Remove the duplicate consecutive call to ADBMS_CC_Check
where it is invoked twice with the identical observed value; keep a single call
to ADBMS_CC_Check((uint8_t)((ic_data[6] >> 2) & 0x3F)) in the function
containing that snippet, and apply the same removal to the duplicate pair in
rdsv so each location calls ADBMS_CC_Check only once (this eliminates the
redundant/no-op second invocation that follows the first call which already sets
or verifies s_expected_cc).
- Around line 286-320: The loop currently calls ADBMS_CC_Check per IC which
advances/resyncs s_expected_cc and masks subsequent mismatches; instead, call
ADBMS_CC_Check once per REGGRP (after transmitCMDR) using the first IC's CC
extracted from ic_data[6], then inside the per-IC loop compare each IC's CC byte
(ic_data[6] >> 2 & 0x3F) against that first_ic_cc and set a per-IC "CC differed"
flag/increment errorCount as needed without calling ADBMS_CC_Check again; apply
the same refactor shape to the corresponding rds* functions (rdsv, rdcfga,
rdcfgb, rdpwmga, rdpwmgb, rdaux, rdsid) so s_expected_cc is only advanced once
per command (transmitCMDR) and independent per-IC CC comparisons are recorded.
---
Outside diff comments:
In `@BMS/Core/User/Src/FEB_AD68xx_Interface.c`:
- Around line 100-149: Pec10_calc currently performs a final 6-iteration
modulo-2 division unconditionally, but those 6 shifts are only required when the
command-counter bits are XORed into the remainder (the bIsRxCmd path); this
causes TX calls (e.g., write_68, transmitCMDW → wrcfga/wrcfgb/wrpwma/wrpwmb) to
compute an incorrect PEC. Fix by moving the second for-loop (the 6-bit modulo-2
division) entirely inside the if (bIsRxCmd == true) block immediately after the
XOR that uses pDataBuf[nLength], so the extra shifts only run for rx-command
calculations and not for TX paths in Pec10_calc.
---
Nitpick comments:
In `@BMS/Core/User/Inc/FEB_AD68xx_Interface.h`:
- Around line 28-40: Document that Pec10_calc(bool bIsRxCmd, uint8_t nLength,
uint8_t *pDataBuf) requires pDataBuf to be at least nLength+1 bytes when
bIsRxCmd is true because the implementation reads the command-counter byte at
pDataBuf[nLength]; add this precondition to the function comment and clarify
ownership/validity expectations for pDataBuf. Also annotate the
ADBMS_CC_Advance, ADBMS_CC_Reset, ADBMS_CC_Check, and ADBMS_CC_GetMismatchCount
declarations with a short note that the CC tracking API is not reentrant / not
thread-safe and must be called from a single task context (or protected by
external synchronization) to match the .c file behavior.
In `@BMS/Core/User/Src/FEB_AD68xx_Interface.c`:
- Around line 40-48: s_cc_mismatch_count currently saturates at 0xFFFF which
makes the (s_cc_mismatch_count & 0x3F) == 1 logging condition stop firing; add a
separate non‑saturating counter (e.g. s_cc_mismatch_total of type uint32_t) that
you increment on every mismatch and use that counter for the rate‑limit check
and LOG_D calls (keep s_cc_mismatch_count behavior for storage/backward
compatibility). Update the mismatch path that currently touches
s_cc_mismatch_count so it also increments s_cc_mismatch_total, and change the if
condition to use s_cc_mismatch_total & 0x3F == 1 while keeping the same
LOG_D(TAG_BMS, "CC drift: expected=%u observed=%u count=%u",
(unsigned)s_expected_cc, (unsigned)observed, (unsigned)s_cc_mismatch_count)
call.
In `@BMS/Core/User/Src/FEB_ADBMS6830B_Driver.c`:
- Around line 772-776: The STAT branch removal leaves crc_count.stat_pec[]
permanently zero which can be misinterpreted as a healthy STAT; update the
reporting code that exposes crc_count.stat_pec[] (where reports/telemetry are
assembled or printed) to either (a) omit/statistically hide the stat_pec field
when ADBMS6830B_rdstat() is not present, or (b) annotate the output for
crc_count.stat_pec[] as "N/I" or "not-implemented" so operators know it is not a
real measurement; keep reset_crc_count() behavior unchanged but ensure any
reporting/printf path checks the absence of ADBMS6830B_rdstat() (or a feature
flag) before presenting crc_count.stat_pec[].
- Around line 305-316: The code currently overwrites
ic[icn].cells.pec_match[REGGRP] in rdcv and rdsv, losing rdcv's result; update
the assignment so it preserves prior verdict (OR the new mismatch with existing
value) if you want a fail-safe "worst-of-the-two" result, or alternatively add
separate per-ADC fields (e.g., c_pec_match[] and s_pec_match[]) and write rdcv
to c_pec_match[REGGRP] and rdsv to s_pec_match[REGGRP]; modify the write in the
rdcv/rdsv blocks (the ic[icn].cells.pec_match assignment) and update any
consumers like check_pec / cells.pec_match[i] to read the chosen field or
compute the combined verdict.
- Around line 543-545: On allocation failure paths in rdpwmga and rdpwmgb you
currently set ic[bank].pwm.rx_pec_match = -1 and return but leave
ic[bank].pwm.rx_data (and ic[bank].pwmb.rx_data in the GB path) containing stale
bytes; update those error returns to also mark the data invalid by clearing the
rx buffers (e.g., zeroing the bytes) or writing a distinct invalid sentinel into
ic[bank].pwm.rx_data and ic[bank].pwmb.rx_data so consumers cannot accidentally
use stale PWM duty values, keeping the existing ic[bank].pwm.rx_pec_match = -1
behavior.
🪄 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: c3aab956-6bee-40fc-971c-557346704d29
📒 Files selected for processing (6)
BMS/Core/User/Inc/FEB_AD68xx_Interface.hBMS/Core/User/Src/FEB_AD68xx_Interface.cBMS/Core/User/Src/FEB_ADBMS6830B.cBMS/Core/User/Src/FEB_ADBMS6830B_Driver.cBMS/VERSIONVERSION
ZacharyBer
force-pushed
the
zb/bms_cc_checking
branch
from
8bd74e4 to
a2b0ca5
Compare
Pec10_calc reads pDataBuf[nLength] when bIsRxCmd is true (the command- counter byte), so callers must provision nLength+1 bytes. Note this in the function comment. The ADBMS_CC_* API mutates a single shared s_expected_cc; document that it is not reentrant and must be called from one task or under external synchronization. Also align the comment with the upcoming refactor that checks the CC once per command instead of once per IC.
cells.pec_match[REGGRP] is shared between rdcv and rdsv. Today rdsv overwrites the field unconditionally, so a rdcv PEC failure followed by a rdsv pass clears the flag. The downstream consumer (FEB_ADBMS6830B.c:174 / store_cell_voltages) reads pec_match and gates use of c_codes (rdcv data) on it — meaning a corrupt cell voltage from a bad rdcv was silently passed through to FEB_ACC whenever rdsv happened to pass. OR rdsv's result into the existing value (worst-of-two). rdcv still writes unconditionally, which acts as the per-sample reset.
ADBMS_CC_Check() resyncs s_expected_cc to the observed value on a mismatch. The old per-IC loops called it N times per command (and rdcv/ rdsv called it twice — copy-paste dupe), so the first IC's mismatch silently fixed the global counter for the remaining ICs and masked the drift event. Move the call out of the per-IC loop: use the first IC's CC for the global check (one command on the wire = one CC tick), then compare each IC's CC against the first inside the loop and LOG_D if they differ. Cross-IC drift surfaces in logs instead of disappearing into a resync. Applied uniformly to rdcv, rdsv, rdcfga, rdcfgb, rdpwmga, rdpwmgb, rdaux (RDAUXA + RDAUXB), and rdsid.
Pec10_calc ran the trailing 6-iteration modulo-2 division unconditionally, outside the if (bIsRxCmd == true) block. Those 6 shifts are only there to process the 6-bit command-counter byte that RX responses append after the data — TX commands have no such byte. Running them anyway for TX (via write_68 -> Pec10_calc(false, ...)) shifts zeros through the remainder and produces a wrong CRC10 on every chip write (wrcfga, wrcfgb, wrpwma, wrpwmb), so the ADBMS6830B silently rejected those writes. Fix matches the Analog Devices LTC6813/ADBMS6830 reference: put the 6-bit loop inside the RX branch where it belongs. NOTE FOR ON-HARDWARE VERIFICATION: this is the first commit on which TX PEC is spec-compliant. wrcfga/wrcfgb (over/under-voltage thresholds, GPIO config) and wrpwma/wrpwmb (cell balancing PWM) will now actually take effect on the chip. Confirm those paths during BMS bring-up.
github-actions Bot
added a commit
that referenced
this pull request
May 23, 2026
…6.0,LVPDB=1.6.0,PCU=1.6.0,Sensor_Nodes=1.6.0,UART=1.6.0,UART_TEST=1.6.0,common=1.6.0,repo=1.6.0) [skip ci]
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
New Features
Bug Fixes
Chores