fix: re-run startup sequence when controller reconnects after power cycle#787
fix: re-run startup sequence when controller reconnects after power cycle#787peter-takacs wants to merge 3 commits into
Conversation
The config burst (PID/pressure/pump coeffs) sent on a reconnect can be lost in the unstable BLE window right after the link comes up, and a spurious ACK then stops the reliable layer from retrying -- leaving the controller on firmware- default gains (Kff 0.000) so the boiler never heats after a controller reboot. Re-send the config burst for ~8s after each (re)connect at a 1s cadence; a copy lands once the link settles, and coalescing keeps it to one queued payload per type. HW-validated on bare master: controller-reset config delivery goes from ~78% fail to 30/30 pass, the re-send delivering config on the cycles that would otherwise fail. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MUmX6ZL8t269p38f24GSqp
…ycle When the display head unit runs headless (always-on) and the machine is power-cycled, the controller board reboots and reconnects via BLE. On reconnect, onSystemInfo() was a no-op for startup because `loaded` was already true: setMode(startupMode) and controller:ready never fired again, leaving the display stuck in STANDBY with no way back to BREW without manually rebooting the head unit. Reset `loaded` to false on BLE disconnect so the next onSystemInfo() re-runs the full startup sequence: applies the configured startup mode, fires controller:ready (re-initialising BoilerFillPlugin, LedControl, OTA), and resends PID/pressure/pump config. Intended to sit on top of jniebuhr#785 which addresses reliable config delivery in the same reconnect window. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesBLE Reconnect State Reset and Config Resend
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
🧹 Nitpick comments (1)
src/display/core/Controller.cpp (1)
524-531: 💤 Low valueOptional: use subtraction pattern for
millis()overflow safety.The check
now < configResendUntilcan misbehave ifmillis()wraps around (~49.7 days uptime) during the 8-second window. The standard overflow-safe idiom is to store the start time and compare elapsed duration:- unsigned long configResendUntil = 0; + unsigned long configResendStart = 0;- configResendUntil = millis() + CONFIG_RESEND_WINDOW_MS; + configResendStart = millis();- if (comms.isConnected() && now < configResendUntil && (now - lastConfigResend) >= CONFIG_RESEND_INTERVAL_MS) { + if (comms.isConnected() && (now - configResendStart) < CONFIG_RESEND_WINDOW_MS && (now - lastConfigResend) >= CONFIG_RESEND_INTERVAL_MS) {This is a very rare edge case—the initial config send in
onSystemInfo()still happens regardless—so not urgent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/display/core/Controller.cpp` around lines 524 - 531, The comparison `now < configResendUntil` is vulnerable to millis() overflow issues that can occur after ~49.7 days of uptime. Replace this direct deadline comparison with an overflow-safe subtraction pattern by storing a reference time (such as the reconnect time or configuration window start time) and checking if the elapsed duration `(now - referenceTime) < WINDOW_DURATION_MS` remains within the allowed window. This aligns with the overflow-safe subtraction pattern already used in the `(now - lastConfigResend) >= CONFIG_RESEND_INTERVAL_MS` check on the same line.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/display/core/Controller.cpp`:
- Around line 524-531: The comparison `now < configResendUntil` is vulnerable to
millis() overflow issues that can occur after ~49.7 days of uptime. Replace this
direct deadline comparison with an overflow-safe subtraction pattern by storing
a reference time (such as the reconnect time or configuration window start time)
and checking if the elapsed duration `(now - referenceTime) <
WINDOW_DURATION_MS` remains within the allowed window. This aligns with the
overflow-safe subtraction pattern already used in the `(now - lastConfigResend)
>= CONFIG_RESEND_INTERVAL_MS` check on the same line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43467c55-5362-44f2-b9f4-de5dde3d2f86
📒 Files selected for processing (2)
src/display/core/Controller.cppsrc/display/core/Controller.h
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
|
3 participants
Summary
Fixes the head unit getting permanently stuck in
STANDBYafter the machine controller power-cycles or its BLE link drops — without requiring a head-unit reboot.This PR contains two commits:
bf47f53— not our work — this is Dave's config-resend fix from fix(comms): re-send controller config after BLE reconnect #785, included here because our fix is intended to sit on top of it. If fix(comms): re-send controller config after BLE reconnect #785 lands first, that commit can be dropped from this branch.d17f198— our fix — resetsloaded = falseon BLE disconnect soonSystemInfo()re-runs the full startup sequence on every reconnect, not just the first boot.Root cause
loadedis the gate that decides whetheronSystemInfo()runs the startup sequence:Before this fix, a disconnect left
loaded = true. On the next connect the gate was already open —setMode(startupMode)was never called and the head unit stayed locked inMODE_STANDBYwith no way out short of a manual reboot.State machine
stateDiagram-v2 direction TB [*] --> Boot Boot --> Connecting : screenReady=true\nconnect() called state "BLE Lifecycle" as BLE { Connecting --> WaitingForController : scan > 10 s\n(controller:bluetooth:waiting) WaitingForController --> Connecting : scan continues Connecting --> StartupLoading : link up / loaded=false WaitingForController --> StartupLoading : link up / loaded=false StartupLoading --> Operational : onSystemInfo() rcvd\nloaded=true · setMode(startupMode)\ncontroller:ready fired\nconfig resent for 8 s (PR 785) } state "Operational" as Operational { direction LR [*] --> Standby : startupMode=STANDBY [*] --> BrewIdle : startupMode=BREW (default) Standby --> BrewIdle : deactivateStandby() BrewIdle --> Standby : activateStandby() / timeout BrewIdle --> BrewActive : activate() BrewActive --> BrewIdle : deactivate() BrewIdle --> Steam : steam button Standby --> Steam : steam button Steam --> BrewIdle : brew button / release Steam --> SteamReady : boiler at target SteamReady --> BrewIdle : deactivate() BrewIdle --> Water : water button Water --> WaterActive : activate() WaterActive --> Water : deactivate() Water --> BrewIdle : setMode(BREW) } Operational --> Connecting : BLE disconnect\nloaded=false [THIS FIX]\nsetMode(STANDBY)The fix
Reconnect sequence (both fixes applied)
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit