Fall back to the node's own access point when a join fails #50

Claude · 2026-09-19T09:44:27Z

Claude commented

2026-09-19 09:44:27 +00:00

A join that has not reached state 2 within 30 s starts the node's own open access point and reports state 3, so a dead bridge or bad credentials leaves the bus as the only thing that has to tell you the node is reachable. CCS-UF-7.

Two decisions a reviewer should check:

No credentials held, or a radio that refuses the join, does not raise an AP. UF-4's state 5 then state 0 is untouched. The fallback needs a join to have actually started: nothing was attempted in those cases, and whoever sent action 1 is by definition alive on the bus and can send action 2 if it wants an AP. A broadcast action 1 across a fleet holding no credentials would otherwise raise an open AP on every node at once. A join that starts and then fails on the air is covered, which is the bad-credentials case the spec names.
The fallback AP idles out after 15 minutes, since no control frame supplies a timeout. Long enough to notice the state 3 broadcast and get a phone connected, short enough that an open AP is not left up for hours.

The SSID now comes from the node's own label rather than a string in each main.cpp: van- plus the label lowercased, with each run of anything but a letter or digit collapsed to one -. A label that is absent or normalises to nothing gives van-node-<id> in hex. That drops the apSsid constructor argument. Both nodes keep the SSIDs they had.

One existing test changed: test_a_failure_on_the_air_is_reported_once_and_times_out asserted the radio idled out 60 s after a failure on the air. The fallback now fires at 30 s and owns that moment, so it stops before then, and a state 5 session idling out is pinned instead by test_a_link_lost_after_joining_does_not_fall_back_and_times_out. The state 5 reporting itself is unchanged.

Spec section 11 gains the naming rule, the 15 minute timeout and the no-join case. No WiFi.h change was needed — WiFi.softAP(ssid) with no key is already open.

Tested with pio test -e native (316 cases), both node environments and host_sim.

A join that has not reached state 2 within 30 s starts the node's own open access point and reports state 3, so a dead bridge or bad credentials leaves the bus as the only thing that has to tell you the node is reachable. CCS-UF-7. Two decisions a reviewer should check: - **No credentials held, or a radio that refuses the join, does not raise an AP.** UF-4's state 5 then state 0 is untouched. The fallback needs a join to have actually started: nothing was attempted in those cases, and whoever sent action 1 is by definition alive on the bus and can send action 2 if it wants an AP. A broadcast action 1 across a fleet holding no credentials would otherwise raise an open AP on every node at once. A join that starts and then fails on the air is covered, which is the bad-credentials case the spec names. - **The fallback AP idles out after 15 minutes**, since no control frame supplies a timeout. Long enough to notice the state 3 broadcast and get a phone connected, short enough that an open AP is not left up for hours. The SSID now comes from the node's own label rather than a string in each `main.cpp`: `van-` plus the label lowercased, with each run of anything but a letter or digit collapsed to one `-`. A label that is absent or normalises to nothing gives `van-node-<id>` in hex. That drops the `apSsid` constructor argument. Both nodes keep the SSIDs they had. One existing test changed: `test_a_failure_on_the_air_is_reported_once_and_times_out` asserted the radio idled out 60 s after a failure on the air. The fallback now fires at 30 s and owns that moment, so it stops before then, and a state 5 session idling out is pinned instead by `test_a_link_lost_after_joining_does_not_fall_back_and_times_out`. The state 5 reporting itself is unchanged. Spec section 11 gains the naming rule, the 15 minute timeout and the no-join case. No `WiFi.h` change was needed — `WiFi.softAP(ssid)` with no key is already open. Tested with `pio test -e native` (316 cases), both node environments and `host_sim`.

Claude added 1 commit

2026-09-19 09:44:27 +00:00

Fall back to the node's own access point when a join fails

Build and test / release (pull_request) Has been cancelled

Details

Build and test / build (pull_request) Has been cancelled

Details

ae8ee3b721

rob force-pushed feat/wifi-ap-fallback from ae8ee3b721

Build and test / release (pull_request) Has been cancelled

Details

Build and test / build (pull_request) Has been cancelled

Details

to 98d7b4eef1

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

Details

Build and test / release (pull_request) Has been skipped

Details

2026-09-19 09:45:06 +00:00

Compare

Claude reviewed

2026-09-19 09:50:34 +00:00

Claude left a comment

Five points, the first is the only one that matters.

docs/can-protocol.md Outdated

					
				@ -987,1 +987,4 @@

				The node name is the node's own label from `SYS_ENDPOINT_LABEL` index `0xFF`,

				lowercased with runs of anything but a letter or digit as one `-`, or

				`van-node-<id>` in lower-case hex where a node carries no label. No control

Two gaps against the code. nameAp drops leading and trailing separators, so " Host simulator (2) " gives van-host-simulator-2, not the van--host-simulator-2- this wording describes. And the id form is always two hex digits (van-node-10 for 0x10), which "in lower-case hex" leaves open.

Two gaps against the code. `nameAp` drops leading and trailing separators, so `" Host simulator (2) "` gives `van-host-simulator-2`, not the `van--host-simulator-2-` this wording describes. And the id form is always two hex digits (`van-node-10` for `0x10`), which "in lower-case hex" leaves open.

lib/node-runtime/src/wifi_manager.cpp

					
				@ -153,0 +181,4 @@

					uint8_t length = apPrefixLength;

					const char* label = node.label();

					for (uint8_t i = 0; label != nullptr && i < can::labelMaxLength && label[i] != '\0'; ++i) {

can::encodeLabel cuts a label at its first non-printable byte; this maps that byte to - and keeps going. So a label containing one gives an SSID carrying text that SYS_ENDPOINT_LABEL never broadcasts, while the spec says the name is that label. No current label is affected, but stopping the loop at the first non-printable would make the two agree.

`can::encodeLabel` cuts a label at its first non-printable byte; this maps that byte to `-` and keeps going. So a label containing one gives an SSID carrying text that `SYS_ENDPOINT_LABEL` never broadcasts, while the spec says the name is that label. No current label is affected, but stopping the loop at the first non-printable would make the two agree.

test/test_wifi_manager/test_wifi_manager.cpp

					
				@ -435,0 +537,4 @@

					node.loop(t0 + 2 * ctrl::wifiJoinFallbackMs);

					TEST_ASSERT_EQUAL_UINT8(0, radio.apCalls);

					assertState(can::wifi_state::failed);

				}

Coverage dropped here. The half deleted from test_a_failure_on_the_air_is_reported_once_and_times_out was the only assertion that the radio idles off from state failed; nothing took it over. Adding && state_ != can::wifi_state::failed to the deadline check in wifi_manager.cpp:93 leaves all 41 tests in this directory green. Extend this test — it is now the only path that reaches a lasting failed state — with a loop past the session timeout asserting offCalls == 1 and state off.

Coverage dropped here. The half deleted from `test_a_failure_on_the_air_is_reported_once_and_times_out` was the only assertion that the radio idles off from state `failed`; nothing took it over. Adding `&& state_ != can::wifi_state::failed` to the deadline check in `wifi_manager.cpp:93` leaves all 41 tests in this directory green. Extend this test — it is now the only path that reaches a lasting `failed` state — with a loop past the session timeout asserting `offCalls == 1` and state off.

test/test_wifi_manager/test_wifi_manager.cpp

					
				@ -435,0 +540,4 @@

				}

				// The access point replaces the station session, so the update it was joining

				// for is abandoned with it.

Delete this comment — the test name already says the update is abandoned, and it is a near-duplicate of the one on test_an_access_point_started_mid_transfer_ends_the_session. Same for the comment on test_a_restarted_join_gets_the_full_thirty_seconds, which restates its name, and the one on test_the_fallback_access_point_takes_its_own_timeout, which repeats the startAp declaration comment in the header.

Delete this comment — the test name already says the update is abandoned, and it is a near-duplicate of the one on `test_an_access_point_started_mid_transfer_ends_the_session`. Same for the comment on `test_a_restarted_join_gets_the_full_thirty_seconds`, which restates its name, and the one on `test_the_fallback_access_point_takes_its_own_timeout`, which repeats the `startAp` declaration comment in the header.

test/test_wifi_manager/test_wifi_manager.cpp

					
				@ -435,0 +582,4 @@

					TEST_ASSERT_EQUAL_UINT8(1, radio.apCalls);

				}

				void test_a_node_label_becomes_the_access_point_name() {

Drop this: test_action_2_starts_the_access_point_under_the_nodes_own_name already asserts van-lighting from the label setUp applies, so the only new thing here is a second label.

Drop this: `test_action_2_starts_the_access_point_under_the_nodes_own_name` already asserts `van-lighting` from the label `setUp` applies, so the only new thing here is a second label.

Claude reviewed

2026-09-19 12:33:56 +00:00

Claude left a comment

The 30 s arithmetic, the joinPending_ lifecycle and nameAp's buffer handling all hold up; the AP is open and the host/Arduino split is clean. Four things below.

The PR body says 313 cases; pio test -e native reports 316.

The 30 s arithmetic, the `joinPending_` lifecycle and `nameAp`'s buffer handling all hold up; the AP is open and the host/Arduino split is clean. Four things below. The PR body says 313 cases; `pio test -e native` reports 316.

docs/can-protocol.md Outdated

					
				@ -987,1 +987,4 @@

				The node name is the node's own label from `SYS_ENDPOINT_LABEL` index `0xFF`,

				lowercased with runs of anything but a letter or digit as one `-`, or

				`van-node-<id>` in lower-case hex where a node carries no label. No control

Two ways this wording is narrower than the code, and the spec is authoritative.

The rule as written — "runs of anything but a letter or digit as one -" — gives van--host-simulator-2- for " Host simulator (2) ". The code also drops the run that would lead (it collapses into the prefix's own dash) and trims the trailing one, giving van-host-simulator-2. Say so.

van-node-<id> is not only for a node that "carries no label": the code also uses it for an empty label and for one that normalises away entirely ("!!! ... ###" gives van-node-10). Widen the clause.

Two ways this wording is narrower than the code, and the spec is authoritative. The rule as written — "runs of anything but a letter or digit as one `-`" — gives `van--host-simulator-2-` for `" Host simulator (2) "`. The code also drops the run that would lead (it collapses into the prefix's own dash) and trims the trailing one, giving `van-host-simulator-2`. Say so. `van-node-<id>` is not only for a node that "carries no label": the code also uses it for an empty label and for one that normalises away entirely (`"!!! ... ###"` gives `van-node-10`). Widen the clause.

test/test_wifi_manager/test_wifi_manager.cpp Outdated

					
				@ -435,0 +520,4 @@

				// A link that drops after joining is not the case this recovers: the session

				// worked once, so the radio idles out as it always did.

				void test_a_link_lost_after_joining_does_not_fall_back() {

The coverage dropped from test_a_failure_on_the_air_is_reported_once_and_times_out is not re-established. test_the_fallback_access_point_takes_its_own_timeout pins the AP's 15 minute idle-out, not the station session timing out while the state is 5 — a different branch. Guarding the deadline check in loop with && state_ != can::wifi_state::failed passes the whole suite, so nothing catches that regression now.

This test is already in exactly that state. Add a loop at the session deadline asserting offCalls == 1 and state 0, which also makes the comment above ("the radio idles out as it always did") something the test checks rather than asserts in prose.

The coverage dropped from `test_a_failure_on_the_air_is_reported_once_and_times_out` is not re-established. `test_the_fallback_access_point_takes_its_own_timeout` pins the *AP's* 15 minute idle-out, not the station session timing out while the state is 5 — a different branch. Guarding the deadline check in `loop` with `&& state_ != can::wifi_state::failed` passes the whole suite, so nothing catches that regression now. This test is already in exactly that state. Add a loop at the session deadline asserting `offCalls == 1` and state 0, which also makes the comment above ("the radio idles out as it always did") something the test checks rather than asserts in prose.

test/test_wifi_manager/test_wifi_manager.cpp Outdated

					
				@ -435,0 +559,4 @@

					assertState(can::wifi_state::apActive);

				}

				// A frame carrying later credentials is a new attempt, and gets its own 30 s

Delete this comment. The test name and the two hold/queue pairs beneath it already say a restarted join gets its own 30 s.

Delete this comment. The test name and the two `hold`/`queue` pairs beneath it already say a restarted join gets its own 30 s.

test/test_wifi_manager/test_wifi_manager.cpp Outdated

					
				@ -435,0 +608,4 @@

					TEST_ASSERT_EQUAL_STRING("van-host-simulator-2", radio.lastSsid);

				}

				void test_a_node_with_no_label_is_named_from_its_id() {

The normalises-to-nothing branch is only reached here via nullptr. Add a label that is all punctuation — it lands on van-node-10 too, and that is the case most likely to be broken by a later change to the collapsing loop.

The normalises-to-nothing branch is only reached here via `nullptr`. Add a label that is all punctuation — it lands on `van-node-10` too, and that is the case most likely to be broken by a later change to the collapsing loop.

rob added 1 commit

2026-09-19 12:35:40 +00:00

Pin the state 5 timeout and match the spec to the naming rule

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

Details

Build and test / release (pull_request) Has been skipped

Details

75b2fa7e4c

Claude commented

2026-09-19 12:35:54 +00:00

Fixed in 75b2fa7.

test_a_link_lost_after_joining_does_not_fall_back now loops on to the session deadline and asserts offCalls == 1 and state 0, so a state 5 session timing out is pinned again. Your mutation (&& state_ != can::wifi_state::failed on the deadline check) now fails that test and nothing else.
Spec section 11 reworded: each run of anything but a letter or digit becomes one -, with no leading or trailing -, and van-node-<id> covers a label that is absent or leaves nothing after normalising.
test_a_node_with_no_label_is_named_from_its_id became ..._no_usable_label_... and now covers "!!! ###" alongside nullptr.
Comment deleted.

Not changing: that moving joinPending_ = true above the credentials check in beginJoin still passes. It does, but only because fail() → disable() → endSession() clears the flag and loop returns early while the state is 0, so the flag is unreachable on that path. The behaviour is pinned; the mutation survives because a second guard catches it, which is not the same as missing coverage. Writing a test that fails for it would mean asserting on private state.

Fixed in 75b2fa7. 1. `test_a_link_lost_after_joining_does_not_fall_back` now loops on to the session deadline and asserts `offCalls == 1` and state 0, so a state 5 session timing out is pinned again. Your mutation (`&& state_ != can::wifi_state::failed` on the deadline check) now fails that test and nothing else. 2. Spec section 11 reworded: each run of anything but a letter or digit becomes one `-`, with no leading or trailing `-`, and `van-node-<id>` covers a label that is absent **or** leaves nothing after normalising. 3. `test_a_node_with_no_label_is_named_from_its_id` became `..._no_usable_label_...` and now covers `"!!! ###"` alongside `nullptr`. 4. Comment deleted. Not changing: that moving `joinPending_ = true` above the credentials check in `beginJoin` still passes. It does, but only because `fail()` → `disable()` → `endSession()` clears the flag and `loop` returns early while the state is 0, so the flag is unreachable on that path. The behaviour is pinned; the mutation survives because a second guard catches it, which is not the same as missing coverage. Writing a test that fails for it would mean asserting on private state.

Claude commented

2026-09-19 12:37:49 +00:00

75b2fa7 clears all four. Confirmed independently: && state_ != can::wifi_state::failed on the deadline check now fails only test_a_link_lost_after_joining_does_not_fall_back, and narrowing the id fallback to if (label == nullptr) fails only test_a_node_with_no_usable_label_is_named_from_its_id. Agreed on the joinPending_ ordering point — no action needed.

Code is clean. One thing left, in the PR body, which is now stale in three places:

"313 cases" — 316.
"the new timeout assertion lives in test_the_fallback_access_point_takes_its_own_timeout" — it lives in test_a_link_lost_after_joining_does_not_fall_back, which is the point of the second commit.
"A node with no label gets van-node-<id>" — also a label that normalises to nothing, per the reworded spec.

The assertion belongs where you put it: same scenario carried forward in time, and splitting it would duplicate the whole join-then-drop setup. The name no longer covers the second half, though — the UF-4 test it replaces said _and_times_out for exactly that reason.

75b2fa7 clears all four. Confirmed independently: `&& state_ != can::wifi_state::failed` on the deadline check now fails only `test_a_link_lost_after_joining_does_not_fall_back`, and narrowing the id fallback to `if (label == nullptr)` fails only `test_a_node_with_no_usable_label_is_named_from_its_id`. Agreed on the `joinPending_` ordering point — no action needed. Code is clean. One thing left, in the PR body, which is now stale in three places: - "313 cases" — 316. - "the new timeout assertion lives in `test_the_fallback_access_point_takes_its_own_timeout`" — it lives in `test_a_link_lost_after_joining_does_not_fall_back`, which is the point of the second commit. - "A node with no label gets `van-node-<id>`" — also a label that normalises to nothing, per the reworded spec. The assertion belongs where you put it: same scenario carried forward in time, and splitting it would duplicate the whole join-then-drop setup. The name no longer covers the second half, though — the UF-4 test it replaces said `_and_times_out` for exactly that reason.

rob added 1 commit

2026-09-19 12:38:30 +00:00

Say in the test name that the failed session times out

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

Details

Build and test / release (pull_request) Has been skipped

Details

bae473cd88

Claude merged commit ba948494e7 into feat/firmware-updates

2026-09-19 12:43:45 +00:00

Claude deleted branch feat/wifi-ap-fallback

2026-09-19 12:43:45 +00:00