Keep retrying STA after AP fallback, and defer mode switches off the event handler (#78) - #238
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91490594c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (err != ESP_OK) { | ||
| /* Deliberately not ESP_ERROR_CHECK: aborting the controller over a | ||
| Wi-Fi hiccup would take a firing with it. Retry on the next tick. */ | ||
| ESP_LOGE(TAG, "esp_wifi_set_mode(%s) failed: %s", s_sta_configured ? "APSTA" : "AP", esp_err_to_name(err)); | ||
| return; |
There was a problem hiding this comment.
When esp_wifi_set_mode(APSTA) returns a non-OK result, the sole WIFI_CMD_ENTER_AP_FALLBACK command has already been dequeued. Because s_ap_active remains false, every subsequent worker tick returns immediately from service_ap_fallback(), and no further event is guaranteed to enqueue another transition. A transient failure therefore leaves the controller without either STA connectivity or its provisioning AP until reboot, despite this branch explicitly intending to retry on the next tick.
Useful? React with 👍 / 👎.
| esp_err_t err = esp_wifi_set_config(WIFI_IF_AP, &ap_config); | ||
| if (err != ESP_OK) { | ||
| ESP_LOGE(TAG, "esp_wifi_set_config(AP) failed: %s", esp_err_to_name(err)); | ||
| } |
There was a problem hiding this comment.
If esp_wifi_set_config(WIFI_IF_AP, ...) fails, this function only logs the error, after which enter_ap_fallback() sets s_ap_active, publishes 192.168.4.1, and sets WIFI_CONNECTED_BIT. The transition then becomes idempotently suppressed, so the configuration is never retried and callers are told an AP is available even though it may have stale/default settings; in the no-credentials boot path there is no STA connection that can recover access either. Propagate this failure and mark the AP active only after configuration succeeds.
Useful? React with 👍 / 👎.
| s_retry_count = 0; | ||
| s_sta_connected = true; | ||
| xEventGroupSetBits(s_wifi_event_group, WIFI_CONNECTED_BIT); | ||
| post_cmd(WIFI_CMD_POLL); |
There was a problem hiding this comment.
When STA recovers while an AP client is still associated, leave_ap_fallback() keeps the AP active, but this handler resets only the fast retry counter and never resets s_retry. If STA drops again during that overlap, the fallback policy uses the timestamp and attempt count from before the successful connection; after a sufficiently long recovery it is already overdue and can reconnect immediately, or bypass AP-client suppression that should have restarted, disrupting the associated provisioning client. Notify the worker to reset the fallback policy whenever a new STA IP is obtained.
Useful? React with 👍 / 👎.
| case WIFI_EVENT_STA_DISCONNECTED: | ||
| if (s_retry_count < s_max_retries) { | ||
| s_retry_count++; | ||
| ESP_LOGI(TAG, "STA retry %d/%d", s_retry_count, s_max_retries); | ||
| esp_wifi_connect(); | ||
| } else { | ||
| ESP_LOGW(TAG, "STA connection failed, switching to AP mode"); | ||
| xEventGroupSetBits(s_wifi_event_group, WIFI_FAIL_BIT); | ||
| start_ap(); | ||
| s_sta_connected = false; | ||
| if (!s_ap_active) { |
There was a problem hiding this comment.
If an AP client keeps the fallback AP open after STA recovery and the router then drops again, this branch changes s_sta_connected to false but leaves s_ip_str containing the now-invalid STA address written by the prior IP_EVENT_STA_GOT_IP. The controller is reachable only at 192.168.4.1 and reports apMode: true, yet /api/v1/wifi and the Wi-Fi settings card display the stale LAN address. Restore the AP address when a disconnect occurs while s_ap_active is true.
Useful? React with 👍 / 👎.
| xEventGroupSetBits(s_wifi_event_group, WIFI_FAIL_BIT); | ||
| post_cmd(WIFI_CMD_ENTER_AP_FALLBACK); |
There was a problem hiding this comment.
On normal STA retry exhaustion, WIFI_FAIL_BIT is set before the deferred WIFI_CMD_ENTER_AP_FALLBACK is processed. This wakes wifi_manager_wait_connected() even though its documented condition—STA connected or AP started—has not yet occurred, so app_main can return a timeout, observe wifi_manager_is_ap_mode() as false, and skip the setup-mode boot status while the worker is still waiting to create the AP. Move the wakeup/readiness signal into the worker after the AP transition succeeds.
Useful? React with 👍 / 👎.
…event handler (#78) AP fallback was a one-way door: after five failed STA attempts the controller brought up its provisioning AP and never tried the configured network again. A router that reboots — or that is simply slower to boot than the kiln — left the controller stranded on its own AP with no remote access to a device that may be mid-firing, recoverable only by a physical power cycle. Fallback now runs APSTA when STA credentials exist, and a new worker task retries the configured network on a 30 s → 5 min backoff, forever. Bounded so the radio is not churning every few seconds during a firing, persistent so recovery costs at most one backoff interval. Retries are suppressed while a client is associated with the AP — a STA connect drags the shared radio off-channel and would yank the provisioning form out from under whoever is filling it in — and suppression is itself capped at 15 min so a phone that auto-joined and was pocketed cannot strand the kiln. Once STA reconnects and the AP is empty, the AP is dropped and we return to the plain STA steady state. The second half of the issue was start_ap() doing network work inside the Wi-Fi event handler: it called esp_wifi_stop() and, worse, esp_netif_create_default_wifi_ap() there, so a second fallback would abort on the duplicate netif. Both netifs are now created once during init before the radio starts, the event handler is reduced to bookkeeping plus a queue post, and every mode switch happens on the worker. The fallback path also drops ESP_ERROR_CHECK in favour of logged errors and a retry on the next tick — aborting the controller over a Wi-Fi hiccup would take a firing with it. The worker is a low-priority task pinned to core 0 that sleeps on its queue; it cannot delay the firing or safety tasks and never touches the SSR. The retry policy is extracted into wifi_retry_policy.c as a pure function, free of esp_wifi/esp_event/FreeRTOS, and covered by host tests — the same split safety_helpers.c uses, and for the same reason: the rest of wifi_manager.c is not host-buildable. Verified the tests fail against a policy that reproduces today's never-retry behaviour before implementing the real one. Also switches the hardcoded retry limit and AP channel to the APP_WIFI_* constants they were duplicating, and clears WIFI_CONNECTED_BIT when an established STA link drops, so the status LED's "disconnected" state works. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eadiness Review follow-ups on #238, all in the paths that only run when something already went wrong — which is why none of them showed up in normal use. Splits the transition logic out into wifi_fallback.c: it owns "should the provisioning AP be up, and is it?", takes every radio call as an injected op, and is driven by tests/host/test_wifi_fallback.c against a fake radio. The five fixes: 1. Intent is tracked separately from achieved state. A failed esp_wifi_set_mode() used to be terminal — the one queued command had been consumed and the service loop began at "is the AP up? no → nothing to do", so a transient error left the controller with neither STA nor AP until a reboot. ap_wanted now stands until the transition lands. 2. The AP is marked active only after esp_wifi_set_config() succeeds. Previously a failed config was logged and the AP advertised anyway, so callers were pointed at whatever SSID the interface happened to hold. 3. The backoff policy is re-armed on every tick the STA link is up. Only s_retry_count was reset on GOT_IP, so when an AP client held the fallback open across a recovery, a second outage fired a reconnect instantly — and being that overdue also walks through the bounded client suppression. 4. The reported address is derived from (ap_active && !sta_connected) rather than cached, so a LAN address can no longer outlive its link. It used to survive a disconnect that happened while the AP was up, leaving /api/v1/wifi advertising a dead address for a device only reachable at 192.168.4.1. 5. Readiness is signalled from the AP-up hook, not from STA retry exhaustion. The old WIFI_FAIL_BIT woke wait_connected() before either of its documented conditions held, so app_main could see is_ap_mode() as false and skip the setup-mode boot status. The bit had no other reader and is gone.
9149059 to
bb96387
Compare
Review follow-ups: all five findings fixed, with host testsRebased onto The restructureThe four transition-shaped findings all live in code paths that only run when something already failed, which is exactly why they went unnoticed — and none of them were reachable from a host test, because So the decisions moved into This is not a wrapper for its own sake — the failure paths are the whole point, and they are now the easiest thing in the file to test. Per finding1 (P1) — fallback dropped forever on a mode-switch failure. Intent is now tracked separately from achieved state. 2 (P1) — AP advertised even when its config failed. 3 (P2) — retry backoff not reset after STA recovery. The policy is re-armed on every worker tick the STA link is up, not just once on an edge — so if an AP client holds the fallback open across a recovery, a second outage starts from the 30 s base backoff instead of firing instantly and walking through the bounded client suppression. 4 (P2) — stale LAN IP reported while only the AP is reachable. Rather than patching the disconnect branch, the cached address is gone: 5 (P2) — readiness signalled before the AP exists. RED evidenceEach fix was reverted individually against the committed code and the suite re-run. Failing assertions only ( Verification
What I did not verify
Behaviour change worth a second opinionDeleting One thing I left alone: when neither STA nor AP is up, |
1 participant
Closes #78.
Both halves of the issue were still real — nothing had been fixed since filing, and the second half is worse than described.
The bug
A kiln whose router rebooted — or that powered on before the router finished booting — fell back to its provisioning AP and stayed there until someone physically power-cycled it. No remote access to a device that may be mid-firing.
The fallback path was also fragile in a way the issue didn't capture:
start_ap()calledesp_wifi_stop()andesp_netif_create_default_wifi_ap()inline on the event-loop task, so a second fallback aborted on the duplicate netif. ThreeESP_ERROR_CHECKs in that path would panic the controller on a transient Wi-Fi error.The fix
Nothing that touches the radio runs in the event handler any more. The handler does bookkeeping and a non-blocking
xQueueSend; a new worker task (priority 1, core 0) is the only place the mode changes. It cannot delay the firing or safety tasks and never touches the SSR.wifi_manager_initcreates both netifs once, before the radio starts — that is the actual fix for the duplicate-netif abort.enter_ap_fallbackis idempotent and setsWIFI_MODE_APSTAwhen credentials exist, so the STA interface stays available.ESP_ERROR_CHECK→ logged error + next-tick retry.leave_ap_fallbackreturns to plain STA once recovered, but only when no AP client is associated.wifi_manager_is_ap_mode()now means "the AP is the only way in" — false during the brief APSTA overlap, so the status LED and/api/wifidon't lie.Retry cadence: 30 s doubling to a 5 min ceiling, forever
Bounded so an unreachable router isn't 720 radio events/hour during a firing; persistent because giving up is the bug. The 5 min cap (rather than 15) is deliberate: worst-case remote-monitoring blackout after the router returns is one interval, and a connect attempt is cheap.
Suppression while a client is on the AP — but bounded
A STA connect makes the shared radio scan off-channel, which is exactly "yanking the interface out from under someone mid-provisioning". So retries are suppressed while an AP client is associated — capped at 15 min overdue, otherwise a phone that auto-joined
Bisqueand got pocketed strands the kiln permanently, trading one failure mode for another.Suppression deliberately does not advance the policy state, so the overdue attempt fires on the first poll after the client leaves rather than waiting out a fresh backoff. That same property makes "how overdue" a free suppression clock, so bounding it needs no extra field.
Tests
The policy is extracted into
wifi_retry_policy.{c,h}— pure, no esp_wifi/esp_event/FreeRTOS — following thesafety_helpers.cpattern that exists for exactly this reason. 10 host tests intests/host/test_wifi_retry_policy.c.RED verified first, with
wifi_retry_stepstubbed toreturn WIFI_RETRY_NOT_DUE(precisely today's never-retry behavior): 9 of 10 failed.test_cadence_is_not_a_hammerpassed vacuously under that stub — it is an upper bound, deliberately paired withtest_retries_are_persistent_over_hoursas the lower bound so neither can be satisfied alone.Independently spot-checked by removing the suppression bound from the merged implementation:
make firmwaremake test-hosttest_wifi_retry_policy./scripts/lint.shCoverage gap, stated plainly
The rest of
wifi_manager.c— worker task, mode transitions, netif lifecycle — has no automated coverage; it's firmware-build verification only. APSTA behavior and AP teardown are unverified without hardware. Only the retry policy is tested.Noticed, deliberately not fixed
POST /api/v1/wifionly writes NVS ("Reboot to connect"), so while in fallback the retry loop keeps trying the old SSID after a user provisions a new one. Harmless — suppression holds retries while they're on the AP — but the obvious follow-up is re-arming the worker with new credentials live.wifi_manager_is_connected()conflates "STA associated" with "AP is up" (the AP setsWIFI_CONNECTED_BIT).status_led.candmain.cdepend on that conflation, so it's preserved rather than widening the blast radius; only the clearing of the bit on a dropped STA link was corrected.esp_event_loop_create_default()is stillESP_ERROR_CHECKed and will panic if something else created the loop first. Out of scope.🤖 Generated with Claude Code