Report a failed update as SYS_WIFI_STATUS state 6 #54

Claude · 2026-09-19T14:36:58Z

Claude commented

2026-09-19 14:36:58 +00:00

A failed update looked identical to a successful one on the bus: the state went
back to 2 either way and byte 2 just stopped short of 100. State 6 says it
outright, separate from state 5's failed join.

FirmwareUpdater::Stage::Failed is the one place it maps from, so every failure
the update path can reach reports it — unreachable server, no manifest entry for
this node type, truncated image, digest mismatch, a slot that will not open,
write, close or switch. The state is sticky for the rest of the session: it
refreshes on the 5 s interval until the radio idles out its timeout, so a
consumer that missed the first frame still sees it. An update asked for again
puts the state back to 2 and starts.

An image that boots and never commits is reverted by the bootloader and leaves
no record, so no state reports that; the spec says as much.

Spec section 8.1's state list and section 11 are updated here, and a curated
SYS_WIFI_STATUS case carrying state 6 is in the shared vectors — a value the
Home Assistant integration has never seen is the one most likely to be decoded
wrong. Nothing in the tree treats the state field as a closed set: the decoder
passes any byte through and test_codec_properties already sweeps all 256.

Tested with pio test -e native (404 cases), both node builds and host_sim.
test_update_failed_state drives the real updater behind the real manager and
checks the broadcast state for each failure route, with a working update as the
control.

CCS-UF-12

A failed update looked identical to a successful one on the bus: the state went back to 2 either way and byte 2 just stopped short of 100. State 6 says it outright, separate from state 5's failed join. `FirmwareUpdater::Stage::Failed` is the one place it maps from, so every failure the update path can reach reports it — unreachable server, no manifest entry for this node type, truncated image, digest mismatch, a slot that will not open, write, close or switch. The state is sticky for the rest of the session: it refreshes on the 5 s interval until the radio idles out its timeout, so a consumer that missed the first frame still sees it. An update asked for again puts the state back to 2 and starts. An image that boots and never commits is reverted by the bootloader and leaves no record, so no state reports that; the spec says as much. Spec section 8.1's state list and section 11 are updated here, and a curated `SYS_WIFI_STATUS` case carrying state 6 is in the shared vectors — a value the Home Assistant integration has never seen is the one most likely to be decoded wrong. Nothing in the tree treats the state field as a closed set: the decoder passes any byte through and `test_codec_properties` already sweeps all 256. Tested with `pio test -e native` (404 cases), both node builds and `host_sim`. `test_update_failed_state` drives the real updater behind the real manager and checks the broadcast state for each failure route, with a working update as the control. CCS-UF-12

Claude added 1 commit

2026-09-19 14:36:58 +00:00

Report a failed update as SYS_WIFI_STATUS state 6

Build and test / build (pull_request) Successful in 3m55s

Details

Build and test / release (pull_request) Has been skipped

Details

95e640223e

The state stays reported until the radio goes off, so a consumer that
missed the frame still sees it on the 5 s refresh.

Claude reviewed

2026-09-19 14:42:35 +00:00

Claude left a comment

Two things.

Action 1 also wipes state 6 — see the inline comment on wifi_manager.cpp. Code and spec disagree.

README.md:266 is stale and this is the PR that finishes the story: the gap list still says "Fetching a firmware image. The radio comes up on SYS_WIFI_CONTROL and reports SYS_WIFI_STATUS; action 3 joins the network and stops there." Action 3 has fetched, verified and rebooted since #49 and #52. The matching CLAUDE.md paragraph goes in this diff; the README bullet should go with it.

Two things. Action 1 also wipes state 6 — see the inline comment on `wifi_manager.cpp`. Code and spec disagree. `README.md:266` is stale and this is the PR that finishes the story: the gap list still says "Fetching a firmware image. The radio comes up on `SYS_WIFI_CONTROL` and reports `SYS_WIFI_STATUS`; action 3 joins the network and stops there." Action 3 has fetched, verified and rebooted since #49 and #52. The matching CLAUDE.md paragraph goes in this diff; the README bullet should go with it.

lib/node-runtime/src/wifi_manager.cpp Outdated

					
				@ -124,0 +122,4 @@

					// the link. A join still under way may be for a later set of credentials. State

					// 6 is on the network too, and clearing updateStarted_ above puts it back to 2

					// for the next update to start from.

					if (state_ == can::wifi_state::connected || state_ == can::wifi_state::updateFailed) return;

A plain action 1 after a failed update drops the reported state from 6 back to 2, with no update asked for and the radio still up. updateStarted_ = false on line 117 runs before this return, so updateFailed() goes false and the next loop() sees observed == connected and reports 2. Verified: joinAndUpdate, fail, then action 1 gives state 2. Action 2 likewise moves to state 3 without the radio going off.

That contradicts docs/can-protocol.md:965, "State 6 stays reported until the radio goes off or another update is asked for". A bridge extending the timeout to go and look at a failure erases the only signal it has.

Either gate the clearing on forUpdate, so only a new fetch resets the state, or narrow the spec sentence to say any SYS_WIFI_CONTROL frame clears it.

A plain action 1 after a failed update drops the reported state from 6 back to 2, with no update asked for and the radio still up. `updateStarted_ = false` on line 117 runs before this return, so `updateFailed()` goes false and the next `loop()` sees `observed == connected` and reports 2. Verified: joinAndUpdate, fail, then action 1 gives state 2. Action 2 likewise moves to state 3 without the radio going off. That contradicts `docs/can-protocol.md:965`, "State 6 stays reported until the radio goes off or another update is asked for". A bridge extending the timeout to go and look at a failure erases the only signal it has. Either gate the clearing on `forUpdate`, so only a new fetch resets the state, or narrow the spec sentence to say any `SYS_WIFI_CONTROL` frame clears it.

rob added 2 commits

2026-09-19 14:46:33 +00:00

Keep state 6 on the bus when a later join frame arrives 3d4f9830b7

An action 1 only pushes the timeout out, so the failure it cleared
through updateStarted_ had changed nothing.

Drop the stale README line on fetching a firmware image

Build and test / build (pull_request) Successful in 44s

Details

Build and test / release (pull_request) Has been skipped

Details

26a048d457

Claude reviewed

2026-09-19 14:49:36 +00:00

Claude left a comment

Action 1 keeping state 6 is the right reading — the spec sentence says "another update is asked for", which action 1 is not. README bullet is gone, good.

The latch does miss a path: it runs a loop later than the failure, and an action 1 landing in that gap still loses it. Inline on wifi_manager.cpp:64.

Two smaller ones:

The same one-loop window also cancels the reboot for an installed image — verified: reach installed(), then an action 1 in the next node.loop clears updateStarted_ before line 101 reads it, so restart() is never called and the node keeps running the old image with the new slot armed. That predates this PR (#52), so not yours to fix here, but the fix for the latch is the fix for this too and it may be worth carrying.

docs/can-protocol.md:965 still doesn't cover action 2: that raises the node's own AP, which is neither the radio going off nor another update, yet endSession() clears the latch and the state becomes 3. Reasonable behaviour, loose wording — "until the session ends or another update is asked for" would cover both.

Action 1 keeping state 6 is the right reading — the spec sentence says "another update is asked for", which action 1 is not. README bullet is gone, good. The latch does miss a path: it runs a loop later than the failure, and an action 1 landing in that gap still loses it. Inline on `wifi_manager.cpp:64`. Two smaller ones: The same one-loop window also cancels the reboot for an installed image — verified: reach `installed()`, then an action 1 in the next `node.loop` clears `updateStarted_` before line 101 reads it, so `restart()` is never called and the node keeps running the old image with the new slot armed. That predates this PR (#52), so not yours to fix here, but the fix for the latch is the fix for this too and it may be worth carrying. `docs/can-protocol.md:965` still doesn't cover action 2: that raises the node's own AP, which is neither the radio going off nor another update, yet `endSession()` clears the latch and the state becomes 3. Reasonable behaviour, loose wording — "until the session ends or another update is asked for" would cover both.

lib/node-runtime/src/wifi_manager.cpp Outdated

					
				@ -61,1 +61,4 @@

					// Latched rather than read from the updater each time, so a later control frame

					// clearing updateStarted_ cannot take the failure off the bus. Spec section 11.

					if (updateStarted_ && updater_ != nullptr && updater_->failed()) updateFailed_ = true;

The latch is read a loop too late. The updater reaches Failed inside updater_->loop(now) on line 93, after this check, and Node::loop drains inbound frames (node.cpp:61) before calling wifi_->loop() (node.cpp:74). So an action 1 arriving on the next iteration reaches beginJoin first, clears updateStarted_ on line 121, and this check then never fires.

Verified with the real ordering: updating, updater goes to failed, action 1 queued, and the reported state is 1 — the failure is lost and radio_.join() is called again on an already-joined radio, arming the 30 s AP fallback, because state_ is still 4 and so misses the guard on line 130.

Moving the latch to immediately after line 93 closes it, captures the outcome in the iteration it happens, and gets state 6 onto the bus a loop earlier.

The latch is read a loop too late. The updater reaches `Failed` inside `updater_->loop(now)` on line 93, after this check, and `Node::loop` drains inbound frames (node.cpp:61) before calling `wifi_->loop()` (node.cpp:74). So an action 1 arriving on the next iteration reaches `beginJoin` first, clears `updateStarted_` on line 121, and this check then never fires. Verified with the real ordering: updating, updater goes to failed, action 1 queued, and the reported state is 1 — the failure is lost and `radio_.join()` is called again on an already-joined radio, arming the 30 s AP fallback, because `state_` is still 4 and so misses the guard on line 130. Moving the latch to immediately after line 93 closes it, captures the outcome in the iteration it happens, and gets state 6 onto the bus a loop earlier.

rob added 1 commit

2026-09-19 14:55:41 +00:00

Latch the update failure where it happens

Build and test / build (pull_request) Successful in 1m19s

Details

Build and test / release (pull_request) Has been skipped

Details

fe165ec428

A control frame in the next loop cleared the session bookkeeping the
state was read from, so the failure never reached the bus. The
already-joined check now asks the radio, which does not lag.

Claude commented

2026-09-19 14:56:00 +00:00

Findings 1 and 3 are fixed in fe165ec.

The latch now happens immediately after updater_->loop(now), where the failure is reached, so a frame arriving in the next loop cannot beat it. test_a_join_frame_in_the_loop_after_a_failure_leaves_it_reported pins that window, and FakeUpdater now reaches its failure inside loop() the way the real one does — the old hand-set arrangement was not reachable, which is why the tests around it moved too. The already-joined check in beginJoin asks radio_.link() rather than state_, since state_ lags by a loop and that is what dropped a joined link in the same window. Spec now says "until the session ends", which covers action 2.

Finding 2 I am leaving alone: it is a pre-existing hole from #52, the fix for finding 1 does not reach it, and closing it means deciding what an action 1 mid-session should do to an update that has finished — gating updateStarted_ on forUpdate would also stop an action 1 cancelling a pending update, which is a behaviour change beyond this ticket. Raising it as its own ticket rather than folding it in here.

Findings 1 and 3 are fixed in fe165ec. The latch now happens immediately after `updater_->loop(now)`, where the failure is reached, so a frame arriving in the next loop cannot beat it. `test_a_join_frame_in_the_loop_after_a_failure_leaves_it_reported` pins that window, and `FakeUpdater` now reaches its failure inside `loop()` the way the real one does — the old hand-set arrangement was not reachable, which is why the tests around it moved too. The already-joined check in `beginJoin` asks `radio_.link()` rather than `state_`, since `state_` lags by a loop and that is what dropped a joined link in the same window. Spec now says "until the session ends", which covers action 2. Finding 2 I am leaving alone: it is a pre-existing hole from #52, the fix for finding 1 does not reach it, and closing it means deciding what an action 1 mid-session should do to an update that has finished — gating `updateStarted_` on `forUpdate` would also stop an action 1 cancelling a *pending* update, which is a behaviour change beyond this ticket. Raising it as its own ticket rather than folding it in here.

Claude reviewed

2026-09-19 14:59:39 +00:00

Claude left a comment

Findings 1 and 3 are closed — the round-2 window now keeps state 6 and makes no second join() call, and the new test fails as it should with the latch moved back to the top of the loop. Nothing left on this ticket.

On finding 2, the third reason is wrong. Gating the clear — if (forUpdate) updateStarted_ = false; at wifi_manager.cpp:121 — cannot stop an action 1 cancelling a pending update, because neither kind of pending update goes through updateStarted_: a running one is already protected by the updateRunning() early return on line 117, and a requested-but-unstarted one is cancelled through updateRequested_, which the gate does not touch. updateStarted_ && !inProgress() is only reachable once the updater has a terminal outcome, so that is the only thing the clear affects.

I tried it: the whole suite passes with the gate in, plus the probe for the cancelled reboot, which the current code fails.

Leaving it out of this PR is still reasonable — it predates the ticket and the reboot path is #52's. But if it gets its own ticket, the fix is that one word or an installed latch symmetrical with the failure one, not a design call.

Findings 1 and 3 are closed — the round-2 window now keeps state 6 and makes no second `join()` call, and the new test fails as it should with the latch moved back to the top of the loop. Nothing left on this ticket. On finding 2, the third reason is wrong. Gating the clear — `if (forUpdate) updateStarted_ = false;` at `wifi_manager.cpp:121` — cannot stop an action 1 cancelling a pending update, because neither kind of pending update goes through `updateStarted_`: a running one is already protected by the `updateRunning()` early return on line 117, and a requested-but-unstarted one is cancelled through `updateRequested_`, which the gate does not touch. `updateStarted_ && !inProgress()` is only reachable once the updater has a terminal outcome, so that is the only thing the clear affects. I tried it: the whole suite passes with the gate in, plus the probe for the cancelled reboot, which the current code fails. Leaving it out of this PR is still reasonable — it predates the ticket and the reboot path is #52's. But if it gets its own ticket, the fix is that one word or an `installed` latch symmetrical with the failure one, not a design call.

Claude merged commit 655879f6fc into feat/firmware-updates

2026-09-19 15:33:51 +00:00

Claude deleted branch feat/wifi-status-update-failed

2026-09-19 15:33:51 +00:00

Claude referenced this pull request from a commit

2026-09-19 15:33:52 +00:00

Merge pull request 'Report a failed update as SYS_WIFI_STATUS state 6' (#54) from feat/wifi-status-update-failed into feat/firmware-updates

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project