diff --git a/03-Architecture-Hardware/firmware-review-v2.3-EinarBoard.md b/03-Architecture-Hardware/firmware-review-v2.3-EinarBoard.md new file mode 100644 index 0000000..85486cd --- /dev/null +++ b/03-Architecture-Hardware/firmware-review-v2.3-EinarBoard.md @@ -0,0 +1,203 @@ +--- +title: Firmware Review — blackpillrtkcode_v2 v2.3 → EinarBoard 1.0 production +tags: [project-drogheda, firmware, watchdog, review, einarboard, production] +project: Project Drogheda +author: Helgen CTO (review run 2026-07-24) +date: 2026-07-24 +status: Active — review of v2.3 as production candidate + v3.0 build spec +supersedes: extends firmware-review.md (v1→v2, 2026-06-19); does not replace it +--- + +# Firmware Review — `blackpillrtkcode_v2.ino` v2.3 as the EinarBoard 1.0 production firmware + +**Question under review:** v2.3 (2026-06-22) was written for the Blackpill *proto* wiring. +EinarBoard 1.0 is fabbed and in service. Is v2.3 correct, and what must change to make it the +production ("on-PCB") software? + +**Bottom line:** v2.3's recovery doctrine is sound and was re-verified programmatically +(backoff ladder, SPS-thrash guard, RTK-loss grace all behave exactly as specified). But it has +**one verified field-impacting bug** (millis()-rollover collapses the recovery backoff every +49.7 days of uptime), **one wrong-for-the-shipped-hardware subsystem** (3-wire fan tach on a +2-wire fan → permanent false `fan_fault` + floating EXTI), and **it drives none of the +EinarBoard-only silicon** (LTC4040 charge management, Modo-B relay, NTC, gauge). Flashed today +it is *safe but blind*: the hardware defaults (PA8 100 k pull-down = charging enabled with the +charger's own NTC guard; PB5 low = K1 NC = corrections direct) were designed to tolerate +exactly this firmware. The production build is a **v3.0** — scope in §4. + +Severity: 🔴 field-impacting · 🟠 robustness · 🟡 cleanup. Authority for hardware facts: +[[watchdog-electrical-design-state]] (canonical) → [[watchdog-production-schematic-delta]] §2/§4/§5 +→ EinarBoard harness truth 2026-07-23 (DFR1015 removed pre-fab, audit H3). + +--- + +## 1. Findings — bugs in v2.3 as written + +### 🔴 F1. `millis()` rollover collapses the recovery gate (verified, uint32 model) + +`beginPowerCycle()` arms the backoff as an **absolute deadline**, and RUNNING checks it with an +**absolute compare**: + +```cpp +nextRecoveryAfter = millis() + POWER_CYCLE_TIME + delayMs; // wraps at 2^32 +... +if (millis() >= nextRecoveryAfter) beginPowerCycle(...); // not rollover-safe +``` + +On the target `unsigned long` is 32-bit → `millis()` wraps every **49.7 days**. These units run +24/7; every unit reaches rollover around day 50. Any wait window that straddles the wrap +overflows to a *small* deadline, so the gate reads open — and every re-arm inside the pre-wrap +window overflows again. A LIMP unit (always inside a 30-min window) is hit **every** rollover. + +**Modelled** (`tools/hostsim/rollover_model.cpp`, uint32 arithmetic, LIMP unit, wedged stack): +gaps go `1806 s → 91 s ×~20 → 1806 s` — i.e. ~20 back-to-back power-cycles (rail-off 6 s + +boot 25 s + NMEA-timeout 60 s apart) in the last ~30 min before wrap. Each one is a tracker +disconnect: this is precisely the flapping v2 was built to kill, resurrected two times per +quarter per unit. Mid-ladder (non-LIMP) units get the same collapse if a backoff window +straddles the wrap, and `recoveryCount` inflates into a false LIMP latch. + +**Fix (small):** store the window start + length and compare elapsed time — subtraction is +wrap-safe: `waiting && (millis() - recoveryWaitStart) >= recoveryWaitLen`. All other timers in +the file already use the subtraction idiom correctly; this is the only absolute compare. +(The `linkLostSince == 0` / `pwrEdgeMs == 0` sentinels are benign — 0 recurs for 1 ms every +49.7 days at worst.) + +### 🔴 F2. Fan-fault machinery is wrong for the shipped fans (2-wire, no tach) + +Canonical open item (2026-06-24): **fans are 2-wire, PB10 tach = DNP provision** — "drop the +PB10 tach health check and infer fan-fault from temperature." v2.3 still ships the 3-wire +logic, so on EinarBoard: + +- `fanHealth()` boosts to 100 % for 300 ms every 30 s whenever the fan is commanded, counts + **0 edges** (no tach exists) → `fanFault` **latches true on every unit, forever** → + `fan_fault=1` in every TLM/$PHLG → alarm fatigue that discredits the very telemetry channel + the fleet-health effort depends on. Plus an audible periodic full-speed blip. +- Worse, PB10 is configured `INPUT` + EXTI FALLING assuming the board's 10 k pull-up — which is + **DNP** on EinarBoard. A floating EXTI pin can fire on noise → garbage `fanRpm` values and a + randomly *masked* fault. Either failure mode corrupts the field. + +**Fix:** gate the tach path behind a build flag (off for EinarBoard), report fan fault by +temperature trend (PWM ≥ threshold for N min while control temp still rising), and if the tach +path is ever kept, `INPUT_PULLDOWN`. + +### 🔴 F3. The EinarBoard-only silicon is not driven at all (the actual "production software" gap) + +Not bugs — missing scope. But flashing v2.3 leaves half the new board inert, including the two +levers that answer the RCA's #1 failure (hot battery) and the ICAVE observability goal: + +| Pin | Net (delta §2) | Hardware state under v2.3 | Consequence | +|---|---|---|---| +| PA8 | `CHRG_EN` (LTC4040 CHGOFF) | Hi-Z; 100 k pull-down → **charging enabled** | Safe (charger's own cell-NTC still guards) but: **no temp-qualified charge cutoff** (the firmware half of the RCA #1 answer), and **no safety-timer restart** — a deep-discharged 12 Ah pack needs ≈5 h @ 2.42 A, which exceeds the LTC4040's internal timer, so without the documented `CHRG_EN` toggle-when-cool logic the pack stays **chronically undercharged** after any deep discharge (delta §4 checklist item). | +| PB3 / PB4 | `CHRG_STAT` / `CHRG_FAULT` | unread | Charging / NTC-fault state invisible in TLM/$PHLG. A unit just died in service (23-Jul short incident); charge-path observability is exactly what the fleet screen needs. | +| PB5 | `BYP_CTRL` (K1 driver) | low → K1 NC = direct uplink | Safe by design — but the MCU **never inserts itself**, so `$PHLG` never reaches the server. Modo B is the *permanent* uplink architecture since the 2026-07-02 carrier decision; the v2.3 header note "v3 keeps the modem on the F9P socket" and `ENABLE_PHLG_RELAY 0` are **stale**. Live fleet health — the point of the board — is off. | +| PB1 | `NTC_TEMP` (ADC1_IN9) | unread | No hot-spot (battery/modem) temperature; it is specified to feed the charge cutoff and fan control. | +| PB12–15 | `BATLED1–4` | unconfigured | Battery gauge dark. Hardware populated; the install/operation manual documents it; installers will read "dead battery." | + +### 🟠 F4. Blocking sensor read stalls the loop — UART overrun risk, and a Modo-B blocker + +`readSHT40()` sits in `delay(10)` (plus ~1 ms of blocking I²C traffic at 100 kHz) every +`THERMAL_PERIOD` (2 s). At 115 200 baud, 10 ms ≈ **115 bytes**; the STM32duino default UART RX +ring is **64 bytes** unless built with `-DSERIAL_RX_BUFFER_SIZE`. A GNSS epoch burst colliding +with the read overflows the ring → lost/garbled sentences. For supervision this is survivable +(next epoch refreshes `lastNmeaMs`), but once the Modo-B relay is enabled the **uplink GGA to +the caster** shares this loop — dropped bytes there corrupt the NTRIP VRS feed. + +**Fix (prerequisite for enabling the relay):** make SHT40 a two-phase non-blocking read +(trigger measurement, harvest ≥10 ms later on the next service pass) and build with +`SERIAL_RX_BUFFER_SIZE 256` (and TX likewise for the relay). I²C stuck-bus: STM32duino ops have +internal timeouts (won't trip the 8 s IWDG), but add 9-clock bus recovery at init as hygiene. + +### 🟠 F5. Thermal derate ignores the battery + +`thermalDerate` derives from **MCU die temp only** (≥75 °C), while the fan curve correctly uses +`thermalCtlTemp()` = hottest of air/probe/MCU. The RCA #1 failure is a **hot battery** — derate +(and the F3 charge cutoff) should key off the control temperature including `tBat`/NTC, with +per-sensor limits (a 75 °C die is fine; a 60 °C cell is not). + +### 🟡 F6. GGA parser: empty-quality field and no checksum validation (verified) + +- Quality field empty (`...,W,,08,...`) → `fixQuality = ',' - '0' = `**−4** (verified in sim). + Semantics survive (−4 < 4 → unhealthy) but telemetry prints −4. Clamp to 0–8. +- A GGA with a **bad checksum is accepted** (verified) — line noise during power transitions can + set bogus fix/sat for one epoch. The XOR is already implemented for `$PHLG`; validate inbound too. + +### 🟡 F7. Docs/comments contradict the fabbed board + +`docs/PINOUT.md` still describes PA5 as "conditioned via DFR1015" — **DFR1015 was removed +pre-fab** (audit H3); the real path is raw 12–24 V → Rig 22 k / Rigb 10 k divider + BAT54 clamp +(harness truth 2026-07-23). PA4 is described as "PowerBoost rail" (it taps **VEH5V**). The v2.3 +header's Modo-B note is stale (F3). Also note: `INPUT_PULLDOWN` on PA5 puts the internal +~30–50 k in parallel with Rigb 10 k → divider sits ≈3.2 V at 12 V ignition — still a clean +logic HIGH, but document it and don't "clean up" the pulldown without redoing that math. + +### 🟡 F8. SLEEP is not low-power + +Brownout "SLEEP" keeps the MCU spinning at full clock (~tens of mA). On EinarBoard the MCU runs +from the *backed-up* rail, so a parked machine drains the 12 Ah pack in roughly two weeks of +firmware overhead alone. Intent (stay alive to wake on vehicle power) is served today; an STM32 +STOP-mode + EXTI/RTC-wake pass would turn weeks into months. Later rev, not a v3.0 blocker. + +### 🟡 F9. Minor + +`newDataReceived` is written, never read (ANSI-TUI leftover). `$PHLG` `nmea_age` grows +monotonically in SLEEP (cosmetic). `PIN_5V_SENSE` name predates the VEH5V rename. + +--- + +## 2. What v2.3 gets right (verified — keep all of it) + +- **Recovery doctrine intact and exact** (sim): backoff gaps 66/126/246/486/966 s → LIMP + retries every 1806 s; **SPS-only unit is never power-cycled** (guard 5a — 0 cycles in 30 min + of healthy fix=1); RTK-achieved-then-lost recovers at lost+120 s grace. +- Non-blocking main loop with IWDG petted once per pass; power-off is the safe state; PA2 + floated on rail-off so GPIO can't back-power the stack. +- No `String`/heap; fixed 110-byte NMEA buffer with overflow guard; `$PHLG` checksum correct + (21 fields, verified) and append-only field discipline maintained COLS↔TLM↔$PHLG. +- EMA-filtered ADC, hysteresis + debounce on power/ignition, 5 s brownout confirmation, + reset-cause captured before flags clear, sensors-absent fallbacks throughout. +- The relay pump design (forward verbatim, inject only after ≥8 ms line idle) is the right + shape for Modo B — it just needs F4's prerequisites before it can be enabled. + +## 3. Verification performed (2026-07-24) + +Harness committed at `04-Source-Code/firmware/tools/hostsim/` (run `./run.sh`): + +- **Stub-compile** of the unmodified sketch: `g++ -std=c++14 -Wall -Wextra` — clean, zero warnings. +- **Scenario sims** (fake millis/ADC/UART): `ladder`, `sps`, `rtkloss`, `gga`, `phlg` — results + as cited in §1/§2. +- **Rollover** modelled separately in **uint32_t** (`rollover_model.cpp`) because host + `unsigned long` is 64-bit and cannot reproduce target wrap: as-written gate → 91 s cycle + storm before wrap; wrap-safe rewrite → uniform 1806 s across it. +- Host sim validates **logic only**, not the STM32 target — the standing rule is unchanged: + **bench-flash before field.** + +## 4. v3.0 — the actual production build (proposed scope, in order) + +1. **Safe fixes on the current doctrine** — F1 wrap-safe recovery window; F2 fan-fault by + temperature trend + tach behind a flag; F6 checksum + quality clamp; F9 cleanups. +2. **Board-target split** — one source, `TARGET_EINARBOARD` / `TARGET_PROTO` flag: + charge-control polarity (**PROD `CHRG_EN` HIGH = charge OFF** vs proto `CHRG_RUN` inverted), + SHT40+TMP102 vs AHT10, relay on/off, tach off. Kills the class of "flashed the proto build + on a production unit" errors. +3. **Charge management block** (the RCA #1 firmware half): read PB1 NTC (β≈3950/Steinhart) + + TMP102; drive PA8 cutoff from the hottest cell-relevant temp; safety-timer restart + (`CHRG_EN` toggle when cool ∧ `BAT_SENSE` low ∧ not charging); PB3/PB4 into EVT + new + TLM/$PHLG fields (append-only); derate from control temp (F5). +4. **Battery gauge** PB12–15 from `vBatF` with hysteresis (suggest 3.55/3.70/3.85/3.95 V for + the 4.00 V-charged 1S pack — bench-tune; also sanity-check BAT_CRITICAL/RECOVERY on the + Hyvel cell). +5. **Modo-B enablement** — after F4 prerequisites (non-blocking SHT40, 256-byte UART rings): + `BYP_CTRL` HIGH only in RUNNING (K1 inserts the MCU), LOW + PA9 floated in + SLEEP/POWER_CYCLE/BOOTING; `ENABLE_PHLG_RELAY 1`; verify GGA-uplink integrity on the bench + (byte-exact forward, injection only in idle gaps). K1's NC path remains the hardware + guarantee if firmware dies — do not weaken it. +6. **Docs**: rewrite `docs/PINOUT.md` for EinarBoard truth (F7); extend LOGGING-SPEC + backend + `PHLG_FIELDS` for the new columns. + +Gate: host-sim regressions green (`tools/hostsim`) → bench-flash on a spare EinarBoard → +soak with the LTC4040 backup-transition + charge-cutoff test (the standing fab-gate test) → +then fleet. + +--- +*Related: [[firmware-review]] (v1→v2) · [[watchdog-electrical-design-state]] · +[[watchdog-production-schematic-delta]] · `docs/LOGGING-SPEC.md` · CHANGELOG.md · +`tools/hostsim/` (this review's harness).* diff --git a/04-Source-Code/firmware b/04-Source-Code/firmware index 1eab7b9..ea1d84c 160000 --- a/04-Source-Code/firmware +++ b/04-Source-Code/firmware @@ -1 +1 @@ -Subproject commit 1eab7b92ff5a9421f0e358936ce038dbee8e39e1 +Subproject commit ea1d84c9f14e63324cd6b09351b4a537e29d2e11