Say what a control frame during a transfer does #60

Merged
Claude merged 2 commits from docs/uf-16-action-3-mid-update into feat/firmware-updates 2026-09-19 21:43:13 +00:00
Collaborator

Section 11 said nothing about a SYS_WIFI_CONTROL frame arriving while a
transfer is running. Documents what the code already does: actions 0 and 2
abandon the transfer, actions 1 and 3 take only the timeout, and action 3's
server is ignored even when it names a different one. Before state 4 a later
action 3 still replaces the server.

No code change. The installed-and-reboot-pending case the ticket also asked for
was already documented by CCS-UF-13. Two tests added, both pinning rules nothing
held before: the different-server frame mid-transfer, and the one that lands
before the transfer starts. pio test -e native is 414 cases; both node
environments and host_sim build.

CCS-UF-16.

Section 11 said nothing about a `SYS_WIFI_CONTROL` frame arriving while a transfer is running. Documents what the code already does: actions 0 and 2 abandon the transfer, actions 1 and 3 take only the timeout, and action 3's server is ignored even when it names a different one. Before state 4 a later action 3 still replaces the server. No code change. The installed-and-reboot-pending case the ticket also asked for was already documented by CCS-UF-13. Two tests added, both pinning rules nothing held before: the different-server frame mid-transfer, and the one that lands before the transfer starts. `pio test -e native` is 414 cases; both node environments and `host_sim` build. CCS-UF-16.
Say what a control frame during a transfer does
All checks were successful
Build and test / build (pull_request) Successful in 3m56s
Build and test / release (pull_request) Has been skipped
b01c9286b6
Claude left a comment

Two of the new paragraph's claims don't match the code.

docs/can-protocol.md:968 — "redirecting a running transfer takes an action 0 first" is not enough. disable() calls endSession(), which clears the session credentials, so the action 3 that follows finds none, reports state 5 and leaves the radio off. A redirect needs a fresh session: the BULK credentials again, then action 3. test_a_session_ended_mid_transfer_can_be_started_again re-holds credentials for exactly this reason.

docs/can-protocol.md:964,969 — "state 4" isn't the boundary, in either direction, so the rule reads as more precise than it is.

  • The server stops being replaceable one loop before state 4 goes out. WifiManager::loop reports state 2 and calls beginUpdate() in the same iteration, and Node::loop drains frames before calling it — so any frame sent in response to the state-2 broadcast already hits if (updateRunning()) return;. "Before state 4 is reported, a later action 3 replaces the server" therefore over-promises; the window really closes when the node stops connecting, i.e. state 1.
  • updateRunning() also outlives state 4: drop the link mid-transfer and observedState() reports 5 while the updater keeps retrying, so actions 1 and 3 still only move the deadline there.

Both confirmed with a throwaway test against this branch. Key the rule off "while a transfer is running" and drop the "— state 4 —" gloss, then say the server is only replaceable while the node is still connecting.

test/test_wifi_manager/test_wifi_manager.cpp:895 — the name says the frame is ignored, but its timeout is taken; only the server and port are dropped. Rename to say that, e.g. test_an_update_frame_mid_transfer_keeps_the_running_server.

Two of the new paragraph's claims don't match the code. `docs/can-protocol.md:968` — "redirecting a running transfer takes an action 0 first" is not enough. `disable()` calls `endSession()`, which clears the session credentials, so the action 3 that follows finds none, reports state 5 and leaves the radio off. A redirect needs a fresh session: the `BULK` credentials again, then action 3. `test_a_session_ended_mid_transfer_can_be_started_again` re-holds credentials for exactly this reason. `docs/can-protocol.md:964,969` — "state 4" isn't the boundary, in either direction, so the rule reads as more precise than it is. - The server stops being replaceable one loop *before* state 4 goes out. `WifiManager::loop` reports state 2 and calls `beginUpdate()` in the same iteration, and `Node::loop` drains frames before calling it — so any frame sent in response to the state-2 broadcast already hits `if (updateRunning()) return;`. "Before state 4 is reported, a later action 3 replaces the server" therefore over-promises; the window really closes when the node stops connecting, i.e. state 1. - `updateRunning()` also outlives state 4: drop the link mid-transfer and `observedState()` reports 5 while the updater keeps retrying, so actions 1 and 3 still only move the deadline there. Both confirmed with a throwaway test against this branch. Key the rule off "while a transfer is running" and drop the "— state 4 —" gloss, then say the server is only replaceable while the node is still connecting. `test/test_wifi_manager/test_wifi_manager.cpp:895` — the name says the frame is ignored, but its timeout is taken; only the server and port are dropped. Rename to say that, e.g. `test_an_update_frame_mid_transfer_keeps_the_running_server`.
@ -961,6 +961,14 @@ cannot finish within five minutes, fails of its own accord. Once the boot slot
has been switched, nothing cuts step 7 short either: a `SYS_WIFI_CONTROL` frame
arriving after it is ignored, with no status frame, and the node reboots.
A `SYS_WIFI_CONTROL` frame arriving while a transfer is running — state 4 — is
Author
Collaborator

observedState() does not report 4 exactly when updateRunning() is true.

  • One loop early: loop() reports state 2 and calls beginUpdate() in the same iteration, and Node::loop drains frames before that, so a frame sent in reply to the state-2 broadcast already finds updateRunning() true.
  • One state late: a link drop mid-transfer reports state 5 with the updater still running, so actions 1 and 3 still only move the deadline.

Key the rule off the transfer running, not off state 4.

`observedState()` does not report 4 exactly when `updateRunning()` is true. - One loop early: `loop()` reports state 2 and calls `beginUpdate()` in the same iteration, and `Node::loop` drains frames before that, so a frame sent in reply to the state-2 broadcast already finds `updateRunning()` true. - One state late: a link drop mid-transfer reports state 5 with the updater still running, so actions 1 and 3 still only move the deadline. Key the rule off the transfer running, not off state 4.
@ -964,0 +965,4 @@
acted on by action. Actions 0 and 2 abandon the transfer and end the session.
Actions 1 and 3 take only the timeout from the frame and leave the transfer
running: action 3's server address and port are ignored, including when they name
a different server, so redirecting a running transfer takes an action 0 first.
Author
Collaborator

Action 0 also clears the session credentials (endSession() → credentials_.clear()), so the action 3 after it finds none, reports state 5 and leaves the radio off. Redirecting needs a whole new session — BULK credentials, then action 3 — not just an action 0.

Action 0 also clears the session credentials (`endSession()` → `credentials_.clear()`), so the action 3 after it finds none, reports state 5 and leaves the radio off. Redirecting needs a whole new session — BULK credentials, then action 3 — not just an action 0.
@ -964,0 +966,4 @@
Actions 1 and 3 take only the timeout from the frame and leave the transfer
running: action 3's server address and port are ignored, including when they name
a different server, so redirecting a running transfer takes an action 0 first.
Before state 4 is reported, a later action 3 replaces the server the node will
Author
Collaborator

Over-promises: the window closes while the node is still connecting (state 1), not when state 4 is reported. See the note above.

Over-promises: the window closes while the node is still connecting (state 1), not when state 4 is reported. See the note above.
@ -890,6 +892,55 @@ void test_a_control_frame_during_a_transfer_does_not_restart_the_join() {
TEST_ASSERT_EQUAL_UINT8(1, radio.offCalls);
}
void test_an_update_frame_naming_another_server_mid_transfer_is_ignored() {
Author
Collaborator

The frame isn't ignored — its timeout is taken, only the server and port are dropped. Name it for that, e.g. test_an_update_frame_mid_transfer_keeps_the_running_server.

The frame isn't ignored — its timeout is taken, only the server and port are dropped. Name it for that, e.g. `test_an_update_frame_mid_transfer_keeps_the_running_server`.
Take the review's corrections to the new rule
All checks were successful
Build and test / build (pull_request) Successful in 47s
Build and test / release (pull_request) Has been skipped
639c8715d6
Claude left a comment

Nothing left to act on. The new boundary matches updateRunning(): inProgress() is stage_ == Manifest || Image, so it spans the manifest fetch, the image and every retry, and ends at Installed or Failed — and on success both real nodes set BootControl, so the reboot guard takes the frame from there, which the step 7 sentence above already covers. The link-dropped case is covered too, since the rule no longer keys off a reported state. 414 cases pass, both node environments and host_sim build.

Nothing left to act on. The new boundary matches `updateRunning()`: `inProgress()` is `stage_ == Manifest || Image`, so it spans the manifest fetch, the image and every retry, and ends at `Installed` or `Failed` — and on success both real nodes set `BootControl`, so the reboot guard takes the frame from there, which the step 7 sentence above already covers. The link-dropped case is covered too, since the rule no longer keys off a reported state. 414 cases pass, both node environments and host_sim build.
Claude merged commit ee4eab6b53 into feat/firmware-updates 2026-09-19 21:43:13 +00:00
Claude deleted branch docs/uf-16-action-3-mid-update 2026-09-19 21:43:13 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
rob/CampervanControlSystems!60
No description provided.