fix: unbrick boot — revert -S6 Clear(), add reentrancy guard, -S7 #9

Open

bastian wants to merge 4 commits from fix/canmap-revert-clear-add-reentrancy-guard into main

pull from: fix/canmap-revert-clear-add-reentrancy-guard

merge into: Skudak:main

Skudak:main

Skudak:feat/s12-vcucmd-3state

Skudak:fix/canmap-rebuild-on-upgrade

Conversation 0 Commits 4 Files Changed 4

+144 -60

bastian commented 2026-05-15 11:31:33 -04:00

Owner

Copy Link

Copy Source

Summary

Replaces and supersedes #8 (closed). PR #8 (-S6) caused the teslacharger to stop responding to the ESP8266 web UI after flashing. Kyle had to re-flash an older firmware 3-5 attempts later to recover.

Root cause: infinite recursion via the HandleClear callback

PR #8 uncommented canMap->Clear() inside MapChargerMessages(). Clear() (libopeninv canmap.cpp:145-150) ends with canHardware->ClearUserMessages(), which (canhardware.cpp:82-91) fires the HandleClear() callback registered in main.cpp:170-173 — and HandleClear() calls MapChargerMessages(). The chain:

main() → MapChargerMessages()
       → canMap->Clear()
       → canHardware->ClearUserMessages()
       → HandleClear() callback
       → MapChargerMessages()       ← re-entry
       → canMap->Clear()
       → canHardware->ClearUserMessages()
       → HandleClear() callback
       → ... stack overflow → HardFault → chip dead, web UI silent

The canary check (FindMap(Param::tmpobcmax)) doesn't break the recursion because tmpobcmax has already been wiped by step 2 of the first Clear; the second call sees the empty map and recurses again.

Hard fault fires before main() reaches the terminal loop, so neither USART terminal nor the ESP8266 web UI ever come up. Recovery requires re-flashing a firmware that doesn't have this code path (older -S5 or earlier).

Fix

1. Re-comment canMap->Clear() inside MapChargerMessages(). The safe behavior is back: on a stale-saved-map upgrade, this function early-returns and the chip boots normally with the old map. The actual rebuild now requires the user to manually issue canclear via the openinverter terminal or web UI — a single Clear fires HandleClear once, MapMessages adds the current set additively, Save persists.

2. Added a static in_progress reentrancy guard. Any future accidental Clear() inside this function or its callees becomes a no-op re-entry rather than a chip-bricking fault. Safe because MapChargerMessages is only invoked from main() and from the HandleClear callback, both single-threaded contexts.

3. Kept the canary as Param::tmpobcmax — still useful as a schema marker (so a future fix that can safely auto-rebuild without recursion has the right canary). Just no longer auto-triggers a destructive rebuild.

4. VERSTR S5 → S7 (skips S6 deliberately). Any bench reporting 1.20.R-S6 in the openinverter UI is the broken build and needs to be re-flashed. -S7 is visibly distinct.

What this does NOT fix (separate follow-up needed)

src/chargercan.cpp has 73 AddRecv+AddSend calls trying to fit into libopeninv's 50-slot CANPOS pool (MAX_ITEMS=50 in canmap.h:31). When a rebuild does run (manual canclear after this is flashed), canmap.cpp:483 silently returns CAN_ERR_MAXITEMS for the last 23 entries — including the SKUDAK 0x211 broadcasts at the tail.

So even after this PR ships and the user issues canclear, the app will still show 0 A / 0 °C in the STATUS screen until a Step-2 PR either:

• Option A: drops unused entries (0x368 idle frame, CHAdeMO TX/RX, duplicate hwaclim mappings) — fits within 50 with single-OBC support.
• Option B: forks libopeninv to bump MAX_ITEMS to 80 — accommodates the firmware as-is plus headroom.

Recommend Option A for the immediate Step-2 PR, with Option B revisited if/when multi-OBC support is needed.

Test plan

• make clean && make — zero new warnings. Size: 25,428 B text / 2,072 B data / 328 B bss (+12 / +0 / +4 vs -S5 baseline, accounting for the static guard).
• Flash to the bench teslacharger (currently running -S5 post-recovery).
• Confirm the web UI comes up within ~5 s of reset. This is the critical regression check vs. -S6 brick.
• Confirm openinverter web UI shows 1.20.R-S7 in the version field.
• Confirm the canMap state is unchanged from before the -S7 flash (same params still mapped — the canary check passes if -S5 had hwaclim mapped without tmpobcmax, so MapChargerMessages enters the in_progress path, runs MapMessages once with overflow-truncation, and saves; the bench may or may not gain SKUDAK 0x211 entries depending on what fits in the pool first).
• Optionally test the recovery flow: issue canclear via the openinverter web UI, power-cycle. Confirm the chip comes back up cleanly (no boot fault, no recursion). It may have a partially-populated map due to the still-unresolved pool overflow — that's Step 2's job.

Postmortem links

• Closed PR #8 with the same scope — DO NOT MERGE #8.
• Internal plan: /Users/bastian/.claude/plans/okay-now-that-we-re-abundant-hartmanis.md

🤖 Generated with Claude Code

## Summary **Replaces and supersedes #8 (closed).** PR #8 (-S6) caused the teslacharger to stop responding to the ESP8266 web UI after flashing. Kyle had to re-flash an older firmware 3-5 attempts later to recover. ### Root cause: infinite recursion via the HandleClear callback PR #8 uncommented `canMap->Clear()` inside `MapChargerMessages()`. `Clear()` (libopeninv canmap.cpp:145-150) ends with `canHardware->ClearUserMessages()`, which (canhardware.cpp:82-91) fires the `HandleClear()` callback registered in main.cpp:170-173 — and `HandleClear()` calls `MapChargerMessages()`. The chain: ``` main() → MapChargerMessages() → canMap->Clear() → canHardware->ClearUserMessages() → HandleClear() callback → MapChargerMessages() ← re-entry → canMap->Clear() → canHardware->ClearUserMessages() → HandleClear() callback → ... stack overflow → HardFault → chip dead, web UI silent ``` The canary check (`FindMap(Param::tmpobcmax)`) doesn't break the recursion because tmpobcmax has already been wiped by step 2 of the first Clear; the second call sees the empty map and recurses again. Hard fault fires before main() reaches the terminal loop, so neither USART terminal nor the ESP8266 web UI ever come up. Recovery requires re-flashing a firmware that doesn't have this code path (older -S5 or earlier). ## Fix 1. **Re-comment `canMap->Clear()`** inside `MapChargerMessages()`. The safe behavior is back: on a stale-saved-map upgrade, this function early-returns and the chip boots normally with the old map. The actual rebuild now requires the user to manually issue `canclear` via the openinverter terminal or web UI — a single Clear fires HandleClear once, MapMessages adds the current set additively, Save persists. 2. **Added a static `in_progress` reentrancy guard.** Any future accidental Clear() inside this function or its callees becomes a no-op re-entry rather than a chip-bricking fault. Safe because `MapChargerMessages` is only invoked from `main()` and from the `HandleClear` callback, both single-threaded contexts. 3. **Kept the canary as `Param::tmpobcmax`** — still useful as a schema marker (so a future fix that can safely auto-rebuild without recursion has the right canary). Just no longer auto-triggers a destructive rebuild. 4. **`VERSTR` S5 → S7** (skips S6 deliberately). Any bench reporting `1.20.R-S6` in the openinverter UI is the broken build and needs to be re-flashed. -S7 is visibly distinct. ## What this does NOT fix (separate follow-up needed) `src/chargercan.cpp` has 73 AddRecv+AddSend calls trying to fit into `libopeninv`'s 50-slot CANPOS pool (`MAX_ITEMS=50` in canmap.h:31). When a rebuild does run (manual `canclear` after this is flashed), `canmap.cpp:483` silently returns `CAN_ERR_MAXITEMS` for the last 23 entries — including the SKUDAK 0x211 broadcasts at the tail. So even after this PR ships and the user issues `canclear`, the app will still show 0 A / 0 °C in the STATUS screen until a Step-2 PR either: - **Option A**: drops unused entries (0x368 idle frame, CHAdeMO TX/RX, duplicate hwaclim mappings) — fits within 50 with single-OBC support. - **Option B**: forks `libopeninv` to bump `MAX_ITEMS` to 80 — accommodates the firmware as-is plus headroom. Recommend Option A for the immediate Step-2 PR, with Option B revisited if/when multi-OBC support is needed. ## Test plan - [x] `make clean && make` — zero new warnings. Size: 25,428 B text / 2,072 B data / 328 B bss (+12 / +0 / +4 vs -S5 baseline, accounting for the static guard). - [ ] Flash to the bench teslacharger (currently running -S5 post-recovery). - [ ] **Confirm the web UI comes up within ~5 s of reset.** This is the critical regression check vs. -S6 brick. - [ ] Confirm openinverter web UI shows `1.20.R-S7` in the version field. - [ ] Confirm the canMap state is unchanged from before the -S7 flash (same params still mapped — the canary check passes if -S5 had hwaclim mapped without tmpobcmax, so MapChargerMessages enters the `in_progress` path, runs `MapMessages` once with overflow-truncation, and saves; the bench may or may not gain SKUDAK 0x211 entries depending on what fits in the pool first). - [ ] Optionally test the recovery flow: issue `canclear` via the openinverter web UI, power-cycle. Confirm the chip comes back up cleanly (no boot fault, no recursion). It may have a partially-populated map due to the still-unresolved pool overflow — that's Step 2's job. ## Postmortem links - Closed PR #8 with the same scope — DO NOT MERGE #8. - Internal plan: `/Users/bastian/.claude/plans/okay-now-that-we-re-abundant-hartmanis.md` 🤖 Generated with [Claude Code](https://claude.com/claude-code)

bastian added 1 commit 2026-05-15 11:31:34 -04:00

fix: unbrick boot — revert -S6 Clear(), add reentrancy guard, S7 ... 23716a260b

PR #8 (-S6) caused chip boot fault: MapChargerMessages() uncommented
canMap->Clear(). Clear() ends with canHardware->ClearUserMessages(),
which fires the HandleClear callback (main.cpp:170-173) — and
HandleClear calls MapChargerMessages(). Recursive chain:

  main() -> MapChargerMessages
         -> Clear() -> ClearUserMessages -> HandleClear
                                         -> MapChargerMessages (re-entry)
                                            -> Clear() (recurse...)
                                            -> ... stack overflow -> HardFault

Bench symptom (Kyle): ESP8266 web UI stops responding after flashing
-S6. Recovery required re-flashing an older firmware (3-5 attempts).

This commit:
1. Re-commented canMap->Clear() inside MapChargerMessages — the safe
   path. The upgrade rebuild now requires a manual "canclear" via the
   openinverter terminal/web UI, which fires HandleClear once and
   triggers an additive MapMessages + Save.
2. Added a static `in_progress` reentrancy guard so any future
   accidental Clear() inside this function or its callees turns into
   a no-op re-entry instead of a chip-bricking fault.
3. Kept the canary as Param::tmpobcmax (from PR #8) — useful as a
   schema marker, just no longer auto-triggers a destructive rebuild.
4. Bumped VERSTR S5 -> S7 (skips S6 so any bench reporting "S6" in
   the openinverter UI is visibly the broken build and gets flashed).

Note: this does NOT solve the underlying "73 entries trying to fit in
50-slot CANPOS pool" issue — chargercan.cpp still has more AddRecv +
AddSend calls than MAX_ITEMS. Step 2 (reduce entries) is a separate
follow-up PR.

#patch

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

bastian added 1 commit 2026-05-18 14:07:22 -04:00

feat: S8/S9 - trim chargercan.cpp + restore c2/c3 RX for multi-module Tesla OBC ... c905e1236b

S8 trimmed the 73-entry chargercan.cpp down to 37 entries to fit
libopeninv's 50-slot CANPOS pool — needed because the upstream firmware
silently overflowed the pool and dropped the SKUDAK 0x211 broadcast at
the tail. Without that broadcast the VCU couldn't see idc/udc/tmpobcmax.

S9 restores 8 of the entries S8 dropped: c2/c3 uac, flag, idc, udc.
These turn out to be NECESSARY for 3-module Tesla OBC operation:

- CalcTotals (charger.cpp:41/48) sums c1+c2+c3 idc and takes the max
  udc. Without c2/c3 idc mapped, idc reports only module 1's
  contribution. Bench observed 2.7 kW reported vs 9.7 kW actual EVSE
  draw — exactly the 1/3 ratio expected.
- CheckChargerFaults (charger.cpp:226-261) gates active2/active3
  detection on c2uac/c3uac > 70V. Without those mapped, the active
  bits short-circuit to false — which masked the c2flag/c3flag
  CheckAlive timeout. We were getting away with the trim by accident.

Kept dropped (still need pool headroom for future):
- c2/c3 iac (AC current — c1iac × 3 is a fine proxy, not used in
  aggregation anywhere)
- c2/c3 stt (state — all modules report same value for healthy operation)
- c2/c3 tmp1/2/in (temps — tmpobcmax from c1's 3 probes is sufficient;
  full 9-probe worst-case is a nice-to-have)
- CHAdeMO RX/TX (0x102/0x108/0x109) — Polarity VCU doesn't use CHAdeMO
- 0x368 idle frame — cosmetic only
- hwaclim duplicates on 0x209/0x20B — all modules report identical limits

Pool math: 42 / 50 slots used, 8-slot headroom for future additions.

VERSTR S8 → S9 so any bench reporting -S8 in the openinverter UI is
visibly the regressed build and needs canclear + reflash.

After flash: user must issue `canclear` from the openinverter Commands
tab so the persisted map gets rebuilt from the new chargercan.cpp.
Power-cycle, verify `can p` shows the 8 new c2/c3 lines, then EVSE
test should report ~9.5 kW in the app (was 2.7 kW).

#patch

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

bastian added 1 commit 2026-05-18 14:39:16 -04:00

feat: S10 - udclim RX on 0x212 bytes 1-2 for VCU-driven force-stop ... 4c1f2e1991

Fix B on the VCU side (b24864e) opens HV contactors when BMS reports
cell voltage above the user's ChargeLimit_pct target. That isolates
the pack but doesn't tell the OBC modules to stop drawing AC. Result:
~0.55 kW continuous EVSE draw dissipated as conversion loss in the OBC,
water pumps stay engaged, app reports "Charging <1 kW" indefinitely.

Root cause: ChargerStateMachine's EVSEACTIVATE state has only four
exits (CheckVoltage udc>udclim, CheckTimeout, CheckUnplugged,
CheckChargerFaults). With contactors open the VCU can't drive pack
voltage above udclim, so none of the exits fire — state machine wedges.

Fix: make udclim CAN-driven on 0x212 bytes 1-2 (uint16 LE, range 50-420V).
The VCU normally sends 398 V (no change in behavior). When VCU is in
CHARGE_COMPLETE state it drops to 200 V — well below any realistic
pack voltage — which fires the existing CheckVoltage() path within
1 second (10 × 100 ms ticks) and transitions EVSEACTIVATE → STOP → OFF.
OBC modules disable, AC draw drops to zero.

Elegant because we reuse the existing CheckVoltage() logic instead of
adding new state-machine code, new params, or new exit paths. Pre-S10
firmware silently ignored bytes 1-2; pre-S10 VCU with S10 teslacharger
leaves udclim at its persistent flash default (398V). Safe upgrade in
both directions.

Pool: 42 → 43 / 50 slots used, still comfortable headroom.

Pairs with stm32-hal-vcu PR #47's matching dynamic-udclim TX commit.

CAUTION: udclim updates here are RAM-only; running `save` while VCU
is forcing 200V would persist that value and require canclear-and-
rebuild to recover. Documented in chargercan.cpp comment block.

#patch

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

bastian added 1 commit 2026-05-18 15:39:40 -04:00

feat: S11 - explicit VCU stop command on 0x212 byte 3 (replaces udclim hack) ... d75027b17c

The -S10 udclim=200V mechanism was a hack — VCU lowered udclim below
pack to fire CheckVoltage() udc>udclim and force STOP. It didn't work
reliably because once VCU opens HV contactors, the OBC per-module
voltage probes (c1udc/c2udc/c3udc) collapse below 200V and udc =
MAX(c1,c2,c3) never accumulates 10 ticks above udclim. Bench symptom:
VCU reaches CHARGE_COMPLETE, opens contactors, but teslacharger stays
in EVSEACTIVATE, OBC modules keep drawing ~0.5kW AC, water pumps run,
CP signal stays charging, EVSE doesn't shut down.

S11 adds an explicit vcustop byte on 0x212 byte 3 — VCU sets it to 1
when it wants charging stopped. ChargerStateMachine checks vcustop
FIRST in the EVSEACTIVATE case and transitions to STOP immediately.
Mirrors how the Dilong OBC obeys OBC_ControlCMD = Stopped.

Wire format (additive, no breaking change to S10):
  Byte 0:    vcuchglim_pct  (existing)
  Bytes 1-2: udclim_V       (existing, DEPRECATED — VCU sends 398V no-op)
  Byte 3:    vcustop        (NEW, 0=normal / 1=force stop)
  Bytes 4-7: reserved

Defensive: Param::Change(udclim) now clamps received <50V values to the
398V flash default. Protects against a pre-PR47 VCU sending all-zeros
in bytes 1-2 (which would otherwise make CheckVoltage() fire constantly
and brick charging).

After this VCU also flashed: the WPUMP issue resolves naturally because
VCU_IsCharging() reads OIOBC.State < ACTIVATE once teslacharger goes
to STOP. The VCU/teslacharger ownership becomes symmetric with the
Dilong path — VCU is authoritative for "should we charge."

Pool: 43 → 44 / 50 slots used.

#patch

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Some checks are pending

Hide all checks

Build Firmware / Build stm32-teslacharger (pull_request) Successful in 58s

Details

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/canmap-revert-clear-add-reentrancy-guard:fix/canmap-revert-clear-add-reentrancy-guard

git checkout fix/canmap-revert-clear-add-reentrancy-guard

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

No items

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Kyle bastian

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Skudak/stm32-openinverter-teslacharger#9