Testing for PCU 4/23/26#47
Conversation
📝 WalkthroughWalkthroughThree sensor and safety validation systems are modified: default accelerator pedal mode is disabled, brake pressure plausibility validation between dual sensors is deactivated, and torque control safety gating bypasses drive-state checks while forcing overall sensor plausibility true. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
PCU/Core/User/Src/FEB_ADC.c (1)
582-593:⚠️ Potential issue | 🟠 MajorMajor:
brake_data->plausibleis now never written inFEB_ADC_GetBrakeData().With the plausibility block commented out, this function no longer sets
brake_data->plausiblein any branch — the field is left holding whatever stale value the caller's struct had on entry. Two concrete downstream consequences:
FEB_CAN_Diagnostics.cpacksBrake_Data.plausibleinto the brake status CAN frame (bit 0). SinceBrake_Datais the zero-initialized global inFEB_RMS.c, the diagnostic will reportplausible=falsefrom boot untilFEB_RMS_Torque()reaches the "pedals released" branch (Line 228) and flips it true — giving vehicle observers an incorrect safety status.- Any future consumer that reads
brake_data->plausiblefrom a local (stack-allocated)Brake_DataTypeDefwill observe indeterminate data.Even while the inter-sensor check is disabled for bench testing, the function should still affirmatively initialize the flag (e.g., to
true) at entry so the contract of "this function fully populates the struct" is preserved.🛠️ Suggested minimal fix
ADC_StatusTypeDef FEB_ADC_GetBrakeData(Brake_DataTypeDef *brake_data) { if (!brake_data) return ADC_STATUS_ERROR; + /* Default to plausible; re-enable inter-sensor check below once sensor 2 is wired. */ + brake_data->plausible = true; + /* Get brake pressure readings */Separately, when you re-enable the check, note the original expression compares a percent-scale diff (
pressure1_percent - pressure2_percent, range 0–100) againstBRAKE_PRESSURE_MAX_PHYSICAL_BAR * 0.2f=40.0fbar — mixed units. If the intent was "20% tolerance", the threshold should simply be20.0f(percent), or the diff should be computed from bar values. Worth fixing before it gets re-enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PCU/Core/User/Src/FEB_ADC.c` around lines 582 - 593, FEB_ADC_GetBrakeData currently never sets brake_data->plausible because the plausibility block was commented out; restore the function's contract by explicitly initializing brake_data->plausible (e.g., set to true) at the top of FEB_ADC_GetBrakeData or before any early return so callers never see stale/indeterminate data, and when you later re-enable the inter-sensor check update the commented comparison to use consistent units (either compare percent vs percent like pressure1_percent/pressure2_percent against 20.0f, or compute diffs in bar before comparing to BRAKE_PRESSURE_MAX_PHYSICAL_BAR * 0.2f).PCU/Core/User/Src/FEB_RMS.c (1)
196-218:⚠️ Potential issue | 🔴 CriticalCritical: committed
TEMP OVERRIDEbypasses torque-gating safety checks.Line 198 unconditionally sets
sensors_plausible = truebefore it is used as the gate for both REGEN (Line 232) and ACCELERATION (Line 241) modes. Concrete impact on the signed-off code path:
- BMS drive state is no longer a gate for torque.
FEB_RMS_Process()guards enable, but with this override the torque calculation in Lines 232‑246 will still execute regardless ofbms_in_drive/DRIVE_STATE, soRMS_CONTROL_MESSAGE.torquegets computed from APPS and pushed toFEB_CAN_RMS_Transmit_UpdateTorqueon Line 254 alongside whateverenabledhappens to be. Any future change that holdsenabledhigh across a BMS drive-state drop would command torque unexpectedly.- The safety-violation log block (Lines 200‑218) becomes unreachable. During the very testing this override is intended to support, you lose the diagnostic signal that would tell you the vehicle would otherwise have cut torque.
- Semantic confusion. The identifier
sensors_plausiblesuggests a check onAPPS_Data.plausible/Brake_Data.plausible, but Line 197 computes it purely from BMS/drive state, and Line 198 then discards even that. The!APPS_Data.plausibleand!Brake_Data.plausiblebranches in the log block were never actually enforced against torque by this variable to begin with — worth fixing that gap too.Strongly recommend gating the override behind a compile-time flag (matching the
SINGLE_APPS_MODEpattern already inFEB_PINOUT.h) so it cannot accidentally ship, and make the variable actually reflect sensor plausibility before using it.🔒 Suggested compile-time-gated refactor
// Check plausibility and safety conditions (require BMS in drive state) bool bms_in_drive = FEB_CAN_BMS_InDriveState(); - bool sensors_plausible = bms_in_drive && DRIVE_STATE; - sensors_plausible = true; /* TEMP OVERRIDE - ignore BMS state for testing */ + bool sensors_plausible = bms_in_drive && DRIVE_STATE && + APPS_Data.plausible && Brake_Data.plausible; +#ifdef RMS_BYPASS_SAFETY_FOR_BENCH_TEST +#warning "RMS safety gating is bypassed — DO NOT FLASH TO VEHICLE" + sensors_plausible = true; +#endifAt minimum, please add a
#warning/#errorguard, a tracking issue, and a target date for removal so this doesn't silently reach a car.Happy to open a follow-up issue to track removal of the override and re-enable the gating, if that helps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PCU/Core/User/Src/FEB_RMS.c` around lines 196 - 218, Remove the unconditional test override that sets sensors_plausible = true; instead compute sensors_plausible from the actual checks (bms_in_drive = FEB_CAN_BMS_InDriveState(), DRIVE_STATE, APPS_Data.plausible, Brake_Data.plausible and optionally brake position vs BRAKE_POSITION_THRESHOLD) so the existing safety-log block and torque gating in FEB_RMS_Process() and RMS_CONTROL_MESSAGE.torque -> FEB_CAN_RMS_Transmit_UpdateTorque are honored; if a test override is required, wrap it in a compile-time flag (like SINGLE_APPS_MODE) and add a `#warning` or `#error` so it cannot be compiled into production, and add a short tracking comment/issue ID and target removal date adjacent to the flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@PCU/Core/User/Src/FEB_ADC.c`:
- Around line 582-593: FEB_ADC_GetBrakeData currently never sets
brake_data->plausible because the plausibility block was commented out; restore
the function's contract by explicitly initializing brake_data->plausible (e.g.,
set to true) at the top of FEB_ADC_GetBrakeData or before any early return so
callers never see stale/indeterminate data, and when you later re-enable the
inter-sensor check update the commented comparison to use consistent units
(either compare percent vs percent like pressure1_percent/pressure2_percent
against 20.0f, or compute diffs in bar before comparing to
BRAKE_PRESSURE_MAX_PHYSICAL_BAR * 0.2f).
In `@PCU/Core/User/Src/FEB_RMS.c`:
- Around line 196-218: Remove the unconditional test override that sets
sensors_plausible = true; instead compute sensors_plausible from the actual
checks (bms_in_drive = FEB_CAN_BMS_InDriveState(), DRIVE_STATE,
APPS_Data.plausible, Brake_Data.plausible and optionally brake position vs
BRAKE_POSITION_THRESHOLD) so the existing safety-log block and torque gating in
FEB_RMS_Process() and RMS_CONTROL_MESSAGE.torque ->
FEB_CAN_RMS_Transmit_UpdateTorque are honored; if a test override is required,
wrap it in a compile-time flag (like SINGLE_APPS_MODE) and add a `#warning` or
`#error` so it cannot be compiled into production, and add a short tracking
comment/issue ID and target removal date adjacent to the flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2b0b70b-a823-45f9-8a93-eb52727fa5f0
📒 Files selected for processing (3)
PCU/Core/User/Inc/FEB_PINOUT.hPCU/Core/User/Src/FEB_ADC.cPCU/Core/User/Src/FEB_RMS.c
2 participants
Summary by CodeRabbit