From 23716a260b981f76fe666d87a9b3b2afdf556449 Mon Sep 17 00:00:00 2001
From: Bastian de Byl <bastian@debyl.io>
Date: Fri, 15 May 2026 11:30:56 -0400
Subject: [PATCH] =?UTF-8?q?fix:=20unbrick=20boot=20=E2=80=94=20revert=20-S?=
 =?UTF-8?q?6=20Clear(),=20add=20reentrancy=20guard,=20S7?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
---
 include/param_prj.h |  7 ++++++-
 src/main.cpp        | 31 ++++++++++++++++++++++++++-----
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/include/param_prj.h b/include/param_prj.h
index d35551a..80972a5 100644
--- a/include/param_prj.h
+++ b/include/param_prj.h
@@ -129,7 +129,12 @@
 // SKUDAK customization suffix: bump on every Skudak-side change so the OpenInverter
 // web UI shows a distinct version (e.g. "4=1.20.R-S4") and we can visually confirm
 // the right firmware is flashed. Match the stm32-sine -S<N> convention.
-#define VERSTR STRINGIFY(4=VER-S5)
+// -S6 was withdrawn (PR #8 closed): the canary-rebuild attempt called
+// canMap->Clear() inside MapChargerMessages, which recursed through the
+// HandleClear callback and stack-overflowed at boot before the terminal
+// came up. Jumping straight to -S7 so any bench that still reports -S6
+// in the openinverter UI is visibly the broken build and gets re-flashed.
+#define VERSTR STRINGIFY(4=VER-S7)
 
 /***** enums ******/
 
diff --git a/src/main.cpp b/src/main.cpp
index 790efe8..1f6ff69 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -108,20 +108,41 @@ static void Ms100Task(void)
 
 static void MapChargerMessages()
 {
+   // SKUDAK: Reentrancy guard. canMap->Clear() (libopeninv canmap.cpp) ends
+   // with canHardware->ClearUserMessages(), which fires the HandleClear
+   // callback below — which calls back into THIS function. If this function
+   // ever calls Clear() itself, the chain becomes infinite recursion → stack
+   // overflow → HardFault before the terminal/web UI come up (this is what
+   // bricked -S6 on Kyle's bench, see PR #8 postmortem). The static guard
+   // makes any future "accidental Clear() inside the rebuild path" a no-op
+   // re-entry rather than a chip-bricking fault. Safe because this function
+   // is only invoked from main() and from the HandleClear callback, both
+   // single-threaded contexts; never from an ISR.
+   static bool in_progress = false;
+   if (in_progress) return;
+
    uint32_t dummyId;
    uint8_t dummyOfs;
    int8_t dummyAdd, dummyLen;
    float dummyGain;
    bool dummyrx;
 
-   //check sample value, if it is mapped assume valid CAN map
-   if (canMap->FindMap(Param::hwaclim, dummyId, dummyOfs, dummyLen, dummyGain, dummyAdd, dummyrx)) return;
-
-   //canMap->Clear();
+   // SKUDAK: tmpobcmax is the canary for the -S5+ CAN-map schema. If it's
+   // already mapped, the persistent flash map is up-to-date — preserve it
+   // and any user customizations. If it's NOT mapped, the saved map is
+   // from an older firmware that didn't broadcast OBC temp on 0x211 byte 7.
+   //
+   // We deliberately do NOT call canMap->Clear() here. Clear() triggers
+   // HandleClear, and the recursion is fatal (see guard comment above). The
+   // upgrade path is: user issues "canclear" via the openinverter terminal
+   // or web UI to nuke the stale map manually. That single Clear fires
+   // HandleClear once, this function re-runs additively, and Save() persists.
+   if (canMap->FindMap(Param::tmpobcmax, dummyId, dummyOfs, dummyLen, dummyGain, dummyAdd, dummyrx)) return;
 
+   in_progress = true;
    ChargerCAN::MapMessages(canMap);
-
    canMap->Save();
+   in_progress = false;
 }
 
 /** This function is called when the user changes a parameter */