Skip to content

Second review pass: remediation plan and fixes for F-19…F-44 - #21

Draft
bazauto wants to merge 12 commits into
mainfrom
claude/youthful-newton-nz39yg
Draft

bazauto wants to merge 12 commits into
mainfrom
claude/youthful-newton-nz39yg

Conversation

@bazauto

@bazauto bazauto commented Sep 25, 2026 •

Copy link
Copy Markdown
Owner

Summary

A full static review of the firmware found 26 defects, recorded as F-19…F-44 in docs/REVIEW_REMEDIATION_PLAN_2.md. The numbering continues from the first pass, so an F-nn in a code comment is unambiguous across both plans. Each finding also has an issue: F-nn is #(nn+3), so #22 to #47.

This PR adds the plan and the code for every finding. Two things are left open deliberately:

Each batch is sized to one flash-and-test cycle.

Opened as a draft: it compiles, but nothing here has run on the board. It should come out of draft once the bench checks below pass.

Issues addressed

These are referenced, not closed: each issue should close when its bench check passes, not on merge.

Batch 1 — 7c980f9

  • F-19 — one bad I²C read could command full speed. The encoder HAL now drops any delta larger than one revolution per poll as a failed read. The controller also clamps before its multiply, which could overflow, and the knob's roster wrap is modular instead of a loop that could spin while holding the state mutex.
  • F-20 — taking over a moving loco on the orchestrator started from speed 0. OrchestratorClient now keeps each loco's last reported state, and acquire replays it to the display. The first click now moves from the loco's real speed, instead of sending speed 4 or a reversal. Nothing is sent on acquire.
  • F-21 / F-32 — use-after-free on returning to the main screen, and a leak of its whole LVGL tree. MainScreen is now built once and re-shown. Neither MainScreen nor JmriConfigScreen takes the WiThrottle client's connection-callback slot any more, which the knob gating depends on.

Batch 2 — 68452a5

  • F-22 — WiThrottle reconnect leaked a socket each time and lost the session without saying so. connect() now reaps the previous session first, and every new session re-acquires the locos the UI still shows as allocated.
  • F-23 — the WiThrottle heartbeat was never armed. The device now sends *+ and heartbeats at half the server's announced interval, so JMRI e-stops this device's locos if it goes silent. It also sends HU<MAC> in place of the malformed HESP32-S3.
  • F-31 — the JMRI ping task was deleted from outside, possibly mid-send. It now exits cooperatively: it signals, suspends itself, and is only deleted after that.

Batch 3 — b35b84c

  • F-24 — no automatic recovery after an outage.
    • WiFi now keeps retrying with backoff after its immediate retries.
    • A permanent orch_connect task supervises the orchestrator login and socket.
    • JMRI now waits for WiFi however long it takes.
  • F-34 — JMRI connection faults. One jmri_conn task now carries out every JMRI connect and disconnect. Disconnect no longer blocks the LVGL task and no longer gets undone. The JSON port is learned from PW without the old race. A manual connect now updates the address used for reconnects.
  • F-40 — WiFi credential handling. Credentials are saved only once they have produced an IP address, and a refused esp_wifi_set_config no longer reboots the device.

Batch 4 — 1b8767b

  • F-25 — the physical knobs were not gated, and a refused command left the display ahead of the loco. The gate moved into ThrottleController, so it now covers the encoders as well as the touch path. A failed send rolls the model back, unless a transport report has landed since. An orchestrator ERROR re-seeds every assigned throttle from the layout's last reported state.
  • F-26 — a re-login could free the WebSocket handle under a sender. OrchestratorClient now guards its handle with its own mutex, held across send and across stop and destroy. Sends wait a bounded time, and only once the link is up.
  • F-27 — no emergency stop, and the power button failed the wrong way.
    • emergencyStop() is now part of the backend port: EMERGENCY_STOP on the orchestrator (layout-wide), and X on each held loco under WiThrottle.
    • A new E-STOP button in the main screen's bottom row fires on press. It is the one saturated button.
    • A press on an unknown power state now turns power off.
    • Under WiThrottle, power OFF falls back to PPA0 when the JSON link is down.
  • F-28 — every function was momentary under the orchestrator. A new port capability says whether the transport takes button events or absolute states. For the orchestrator, the controller toggles from the known state on press and ignores the release.
  • F-29 — the documented lock order was the reverse of the code's. CLAUDE.md, THREADING_MODEL.md, the firmware-review agent and the controller docs now all say LVGL lock before m_stateMutex, which is what the code does.

Batch 5 — 29b54d2, c472b03, bf65c15

  • F-30 — callbacks set on one task and called on another could tear. Every slot on the clients, both backends and the controller is now a CallbackSlot. It is set under its own lock, and the callback runs as a copy with that lock released.
  • F-33 — stack headroom was never measured. Setting CONFIG_THROTTLE_STACK_REPORT (off by default) makes every task log its stack high-water mark. THREADING_MODEL.md now has a table for the figures, and describes how to take them.
  • F-35 — the orchestrator roster was refused above about 35 locos. The roster is now streamed, one record at a time. It is capped at 128 locos and 4 KB per record, and an oversized record costs only that loco.
  • F-37 — a late WiThrottle reply could set the wrong loco's baseline. A report is now applied only to the throttle holding the loco it names. Speeds over 126 and functions over F28 are dropped.
  • F-38 — hot-path logging and repaints. Per-line and per-tick logs are now DEBUG. The power bar refreshes once per repaint instead of four times, and unchanged widgets are not restyled.
  • F-39 — NVS writes on the LVGL task. A new SettingsWriter, with one task and a queue, now carries out every NVS write the UI asks for, in order. It replaces the one-shot jmri_save task.
  • F-41 — Seesaw reads lost rotation. Each read is now a request, a 500 µs wait, then the read, done once. It was a combined write-read done twice, and rotation between the two reads was lost.
  • F-42 — protocol hygiene.
    • OrchestratorClient keeps its link, system and power states as atomics, and writes no shared string unlocked.
    • WebSocket messages split across frames are reassembled before parsing.
    • The layout id is re-fetched on every login.
    • requestPowerList is gone, and the JMRI power-manager name is now locked.
  • F-43 — CI hardening.
    • ci.yml now has permissions: contents: read.
    • All three actions are pinned by commit. esp-idf-ci-action@v1 turned out to be a branch; it is pinned at the head @v1 was running.
    • esp_websocket_client is pinned to ~1.8.0, with the lock's manifest_hash recomputed so CI does not re-solve.
  • F-44 — the task table had drifted. It now matches the code.
  • F-36 — the cleartext question. The measured facts are in the plan, and CLAUDE.md's Open limits names the question.

Docs — 9ddbb4f, 83007d4, 13408f5, 1537db5, 081d4e4

  • The plan itself, with an Issue column in its tracker, and a pointer to it from CLAUDE.md.
  • Component, flow, threading and NVS docs updated to match each batch.
  • Two new Traps in CLAUDE.md, with the Copilot instructions changed to match: MainScreen is never rebuilt, and UI code never takes a client's connection callback.

Decision needed: F-36 (#39)

The login, the 30-day session cookie and the control plane all cross the layout WiFi in cleartext. What the numbers say:

  • Flash is not the obstacle. mbedTLS is already linked, about 117 KB of it, pulled in by esp_websocket_client. A build with HTTPS turned off in esp_http_client was only 528 bytes smaller. So TLS would cost little flash, and staying cleartext saves none.
  • The costs are heap and the server. TLS would take roughly 20–40 KB of heap per connection, and the orchestrator would have to serve TLS.
  • WiThrottle is no better. It has no authentication at all, so the WiFi password is already the real boundary.

The options:

  1. Accept cleartext on the layout WiFi as the boundary, as F-18 did for NVS.
  2. Shorten the operator session lifetime on the orchestrator side.
  3. Add TLS on both ends.

Behaviour changes to expect on the bench

  • JMRI will now e-stop this device's locos if its heartbeat stops. That includes reflashing or rebooting the ESP while a train is running. This is intended: it is the dead-man switch.
  • E-STOP under the orchestrator stops the whole layout, not only this device's locos. Under WiThrottle it stops only the locos this device holds.
  • The encoders are read differently (F-41). If the knobs feel different, or go dead, this is the change to look at first.
  • The power button turns power off when it does not know the current state.
  • Under the orchestrator, a function button now latches: tap once for on, tap again for off.
  • A knob turned while the transport is down does nothing, physical or on-screen.
  • A WiThrottle disconnect keeps the acquisition record, and the next connect re-acquires those locos. Only Release removes one.
  • JMRI Disconnect now stays disconnected until the next Connect or a reboot.
  • WiFi credentials that never connect are not kept after a reboot.
  • With the orchestrator selected, Connect on the JMRI screen only saves the settings.
  • Settings are saved a moment after the button press, on their own task, rather than during it.
  • The serial console is quieter: received WiThrottle lines and knob ticks are now DEBUG.

Verification

  • Compiled only. Both the app and the CONFIG_THROTTLE_TESTS configurations build with ESP-IDF v5.5.2 and no new warnings. So does the app with CONFIG_THROTTLE_STACK_REPORT=y. The Espressif component registry was unreachable from the build environment, so the local build used GitHub copies of the pinned component versions (LVGL 8.4.0, esp_websocket_client 1.8.0, esp_lcd_touch and GT911 1.2.1). CI on this PR is the registry-based build, and it passes with the pinned actions, taking the lock file as it is.
  • Image size: 1,364,448 → 1,381,152 bytes locally (+16,704 across all five batches; Batch 5 added 7,008). 154,848 bytes (10%) of the 1500 K partition remain. CI's registry build comes out at 1,377,424 bytes, leaving 158,576.
  • Not run: the Unity suite, which needs the board. New tests cover:
    • the encoder delta bound and the roster wrap
    • acquire seeding, the loco state cache, and re-seeding on a refusal
    • heartbeat parsing
    • knob gating, rollback of a failed send, and the emergency stop
    • function press and release, and toggling
    • the roster splitter (seven cases), stale and out-of-range transport reports, and the callback slot

Bench checks (full criteria in the plan and each issue)

  • Batch 1: take over a loco another operator is running, then click once each way.
  • Batch 1: go Settings → Back twenty times while turning a knob; internal heap should stay stable.
  • Batch 1: unplug an encoder mid-spin; no out-of-range delta should reach the controller.
  • Batch 2: restart JMRI with a loco running, then check that the knob and the stop press still control it.
  • Batch 2: pull the ESP's power; JMRI should e-stop the loco.
  • Batch 2: restart JMRI ten times; the socket count should stay stable.
  • Batch 3: power-cycle the router.
  • Batch 3: boot the ESP before the orchestrator host, and again before JMRI.
  • Batch 3: enter a wrong WiFi password, then reboot.
  • Batch 3: JMRI Disconnect stays down; Connect to a new address sticks.
  • Batch 4: with the orchestrator offline, turn a knob; the display should return to the layout's speed.
  • Batch 4: pull the WiFi and turn a physical knob; nothing should move on screen.
  • Batch 4: press Connect on the orchestrator screen repeatedly while spinning a knob; no crash.
  • Batch 4: E-STOP under each transport. Also check the button's placement against the carousel and the icon buttons: it was laid out without a screen to look at.
  • Batch 4: stop JMRI's web server, then press the power button; power should go off over WiThrottle.
  • Batch 4: under the orchestrator, tap F0; the headlight should stay on until tapped again.
  • Batch 5 (before merging): knobs. Slow single clicks should move exactly one step each, a fast spin should lose no steps, and the button should still press and release.
  • Batch 5: under the orchestrator, a roster of more than 35 locos should load whole.
  • Batch 5: change the speed steps and leave the screen at once; the new value should apply. Save the orchestrator settings, then Connect; the login should use them. Forget WiFi.
  • Batch 5: spin a knob hard with the console attached; the display should keep up.
  • Batch 5: build with CONFIG_THROTTLE_STACK_REPORT=y, drive under load, and record the figures in THREADING_MODEL.md (F-33: Task stack headroom unmeasured #36).
  • Unity suite passes on the board.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Sphir3ie1RKbPNUWePK49n

Records the findings of a full static review of the firmware, numbered on
from the first pass so an F-nn reference in a comment stays unambiguous.
Each finding carries a severity weighed by whether a train can move
uncommanded, an operator can lose the ability to stop one, or the device
can crash while one is moving; a batch sized to one flash-and-test cycle;
and bench criteria that stay unticked until run on the board.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sphir3ie1RKbPNUWePK49n
…creen (F-19, F-20, F-21)

F-19: one corrupted I2C read reached the controller as a delta and was
clamped to full speed in one direction or the other. The encoder HAL now
drops anything beyond a full revolution per poll as a failed read; the
controller clamps before multiplying (that multiply overflowed), and the
knob's roster wrap is modular rather than a loop that could spin for
millions of iterations holding the state mutex.

F-20: taking over a loco on the orchestrator zeroed the model and the
shadow, so the first click on a loco doing 60 commanded speed 4, or a
reversal. OrchestratorClient now keeps each loco's last reported state
(snapshot replaces it; LOCO_STATE updates it; link loss clears it), and
acquire replays it to the new throttle as a LOCO_STATE would arrive.
Display path only: nothing is sent on acquire.

F-21 (and F-32): showMainScreen() destroyed the old MainScreen inside an
LVGL handler while an encoder or network task could already hold its
pointer and be waiting on the LVGL lock to repaint it. The main screen is
now built once and re-shown, which also stops the whole LVGL tree leaking
on every return. That made the screen's registration on the WiThrottle
client's single connection slot a lasting problem -- JmriConfigScreen
nulled it on Back and nothing restored it -- so neither screen takes that
slot any more: MainScreen gets link changes through the controller, and
JmriConfigScreen polls on a timer like the other config screens.

Compiled for the app and the test configuration; the new Unity tests and
the bench checks in docs/REVIEW_REMEDIATION_PLAN_2.md still need the board.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sphir3ie1RKbPNUWePK49n
… the JMRI ping task (F-22, F-23, F-31)

F-22: when JMRI dropped the link the receive task exited but nothing
closed its socket, and the reconnect task's connect() overwrote it -- one
of lwIP's ten sockets lost per JMRI restart or long WiFi drop. connect()
now reaps any previous session first. Teardown clears m_socket under the
send mutex before closing and the receive task reads its own copy, so no
send or recv can land on a descriptor lwIP has handed to another socket.

Worse, the new session held no locos while the UI still showed every
throttle live, so JMRI ignored the knob and the stop press alike. The
acquisition record now mirrors the UI rather than the TCP session: it
survives a disconnect, every connect() re-acquires from it (JMRI's reply
re-seeds the display), and only a release removes an entry.

F-23: the client never sent "*+", so JMRI's heartbeat monitoring -- its
dead-man stop for a crashed or half-open device -- was never on. It now
arms it when the server announces a non-zero interval and sends "*" at
half that from the receive task, which wakes every second anyway. It also
sends HU<station MAC> in place of the malformed "HESP32-S3".

F-31: stopHeartbeat() vTaskDelete()d the ping task from outside, possibly
mid-send holding the WebSocket client's lock. The task is now woken to
exit, signals and suspends, and is deleted only after that signal, under
a mutex shared with startHeartbeat(). Stack 2 KB -> 3 KB.

Compiled for the app and the test configuration; bench checks are listed
in docs/REVIEW_REMEDIATION_PLAN_2.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sphir3ie1RKbPNUWePK49n
…(F-24, F-34, F-40)

F-24: after an ordinary outage the device stayed offline, every throttle
dead, until someone went into settings.
- WiFi gave up for good after five immediate retries -- shorter than a
  router reboot. It now reports FAILED but keeps retrying on a one-shot
  esp_timer (5 s doubling to 60 s) until Disconnect or Forget, which the
  WiFi screen now offers in that state. The timer only calls
  esp_wifi_connect(), which does not block the esp_timer task.
- The orchestrator login was tried once at boot. orch_connect now
  supervises the link for the life of the app: waits for WiFi however
  long, retries logins with backoff (under the server's 5/min limit),
  logs in afresh when the socket's own cookie-reusing reconnect has not
  recovered in 30 s, and retries a failed roster read. The config
  screen's Connect wakes it instead of racing it with a task of its own.
- JMRI auto-connect waited 30 s for WiFi, then never reconnected.

F-34: JMRI connects happened on three tasks and disconnect on the LVGL
task (blocking it for seconds), the JSON client was connected from the
WiThrottle receive task, a manual disconnect was undone within five
seconds, and a manual connect never updated the address reconnects used.
JmriConnectionController now owns one jmri_conn task that does every
connect, reconnect and disconnect, saves the settings, and learns the
JSON port from PW via the receive task without connecting anything
there. The screen only asks. Under the orchestrator, Connect just saves.

F-40: WiFi credentials are saved once they produce an IP, not before, so
a typo no longer replaces a good password; and a refused
esp_wifi_set_config reports FAILED instead of rebooting the device.

Compiled for the app and the test configuration; bench checks are listed
in docs/REVIEW_REMEDIATION_PLAN_2.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sphir3ie1RKbPNUWePK49n
…a client's slot

Both are rules the batch 1 fixes depend on and that look like oversights
to a later reader: rebuilding the main screen reintroduces F-21's
use-after-free, and a screen registering on a client's connection
callback cuts the backend's knob gating off from the link (F-30). The
Copilot instructions' example showed exactly the rebuild pattern, so it
changes with CLAUDE.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sphir3ie1RKbPNUWePK49n
F-19…F-44 now have issues #22…#47 as a permanent record outside the
tree; the tracker gains an Issue column. Also brings three plans (F-22,
F-23, F-24) and F-44's description into line with what was built.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sphir3ie1RKbPNUWePK49n
…(F-25…F-29)

F-25: the physical encoders reached ThrottleController without passing
the UI's link gate, and a speed the transport refused stayed on the
display, so the first click afterwards jumped from it. The controller now
refuses knob input while the link is down and rolls a failed send back
(unless a transport report landed since). The orchestrator refuses with
an ERROR rather than a failed send, so any ERROR now re-seeds every
assigned throttle from the layout's last reported state.

F-26: a re-login stopped and destroyed the WebSocket handle while a
sender could be inside it. The handle now has its own mutex, held for
each send and across stop/destroy; senders only take it once the link is
known up, so they never queue behind a re-login.

F-27: there was no emergency stop, and the power toggle turned power ON
from an unknown state. The port gains emergencyStop() -- EMERGENCY_STOP
halts the layout; WiThrottle sends X to every held loco -- behind an
E-STOP button that fires on press, in the one saturated colour. Unknown
power now turns power off, and under WiThrottle an OFF goes over PPA0
when the JSON link is down.

F-28: FUNCTION_COMMAND sets state outright, so press-on/release-off made
every orchestrator function momentary. A port capability now tells the
controller to toggle on press there, and keep press/release for JMRI.

F-29: the lock order was documented backwards in CLAUDE.md, the threading
model and the review agent. Corrected to what the code has always done:
the LVGL lock before m_stateMutex.

Compiled for the app and the test configuration; the E-STOP layout and
the bench checks in docs/REVIEW_REMEDIATION_PLAN_2.md need the board.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sphir3ie1RKbPNUWePK49n
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sphir3ie1RKbPNUWePK49n
@bazauto bazauto changed the title Second review pass: remediation plan and fixes for every HIGH finding Second review pass: remediation plan and fixes for every HIGH and operator-facing MEDIUM finding Sep 25, 2026
… task (F-30…F-42)

Batch 5 of the second review pass: the remaining MEDIUM and LOW findings.

- F-30: every callback slot on the clients, both backends and the
  controller is a CallbackSlot, set under its own lock and invoked as a
  copy with the lock released. The controller's function-and-userData
  pairs become one closure each, so neither half can be replaced alone.
- F-33: CONFIG_THROTTLE_STACK_REPORT (off by default) logs each task's
  stack high-water mark for sizing on the bench. Nothing resized blind.
- F-35: the orchestrator roster is streamed through JsonArraySplitter and
  parsed a record at a time, so an 8 KB buffer no longer refuses any
  roster above about 35 locos. Capped at 128 locos and 4 KB a record.
- F-37: a transport report is applied only to the throttle that holds
  the loco it names; speeds over 126 and functions over F28 are dropped.
- F-38: per-line and per-tick logs are DEBUG; the power bar refreshes
  once per repaint, and unchanged widgets are not restyled.
- F-39: SettingsWriter, one task and a queue, carries out every NVS write
  the UI asks for, in order. It replaces the one-shot jmri_save task.
- F-41: Seesaw reads are request, 500 us wait, read, done once, instead
  of a combined write-read done twice, which lost rotation between reads.
- F-42: OrchestratorClient keeps its link, system and power states as
  atomics and writes no shared string unlocked; WebSocket continuation
  frames are reassembled; the layout id is dropped on every login.
  requestPowerList is gone, the JMRI power name is locked, and the
  WiThrottle roster warning no longer reads past the end.
- F-44: the threading model's task table is brought up to date.

Compiled for the app and test configurations; nothing here has run on
the board. The Seesaw change in particular needs a bench check before
merging.

Refs #33, #36, #38, #40, #41, #42, #44, #45, #47

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sphir3ie1RKbPNUWePK49n
…d (F-43)

- permissions: contents: read. The job checks out, builds and uploads an
  artifact; it needs nothing more, and the repository default may grant
  write.
- actions/checkout, espressif/esp-idf-ci-action and actions/upload-artifact
  are pinned by commit, each with its release in a comment. A tag can be
  moved to other code after review. esp-idf-ci-action's v1 is a branch,
  not a tag, and is pinned at the head that @v1 was running.
- espressif/esp_websocket_client goes from '*' to '~1.8.0', the series
  dependencies.lock holds. The lock's manifest_hash is recomputed with the
  component manager so CI does not re-solve.

Refs #46

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sphir3ie1RKbPNUWePK49n
…3, F-36)

- The plan records batch 5: acceptance criteria for every finding in it,
  and a tracker in which every finding is now at least code done. F-33's
  measurements and F-36's decision are the two left open.
- F-36: the measured facts for the cleartext decision. mbedTLS (about
  117 KB) is already linked by esp_websocket_client, so TLS would cost
  little flash, and staying cleartext saves none; the cost is heap per
  connection and a certificate the orchestrator would have to serve.
  CLAUDE.md's Open limits names the question; the decision is the
  owner's.
- CLAUDE.md, the Copilot instructions and main/utils/README.md list
  utils/ and SettingsWriter.

Refs #36, #39

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sphir3ie1RKbPNUWePK49n
CI on bf65c15 downloaded the three pinned commits and took
dependencies.lock as it is, without re-solving.

Refs #46

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sphir3ie1RKbPNUWePK49n
@bazauto bazauto mentioned this pull request Sep 25, 2026
4 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants