fix(usb): tether follows the USB bus state; WiFi failover off the PSRAM stack; distinct netif MAC - #151
fix(usb): tether follows the USB bus state; WiFi failover off the PSRAM stack; distinct netif MAC#151iliabaranov wants to merge 2 commits into
Conversation
… MAC - esp_tinyusb device events: attached/resumed -> netif link up, detached/suspended -> link down. Until now the synthetic 'connected' at start was never undone, so a host that de-enumerated us or suspended the bus left a dead tether ranked as the best uplink and the supervisor never failed over (bench 2026-09-18: port de-authorised, device kept its lease, no WiFi for minutes). Enables CONFIG_TINYUSB_SUSPEND/RESUME_CALLBACK. - The MAC given to tinyusb_net_init() is the HOST's (CDC-NCM iMACAddress); the lwIP netif now uses a distinct locally-administered address instead of the same one (spec violation that worked on Linux by accident).
dcs_wifi_set_enabled() reaches flash (wifi_list NVS blob, PHY calibration NVS) and the supervisor's stack lives in PSRAM, so the first time the failover path ever ran — tether link-down from the previous commit — it died in spi_flash_disable_interrupts_caches_and_other_cpu's esp_task_stack_is_sane_cache_disabled assert (coredump: net_sup -> dcs_wifi_set_enabled -> ml_config_get_wifi_list -> nvs_get_blob). The toggle now runs on a transient 4 KB internal-stack worker; the supervisor waits on a dedicated semaphore so its link-down task notification is not consumed. Any Ethernet unit losing Eth+USB would have hit the same abort.
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.
Graphify review — findings
Drives the tether netif link state from USB bus events: on_usb_event marks the link down on detach/suspend and up on attach/resume, so a host that de-enumerates, suspends, or disappears drops the DHCP lease and lets the net supervisor fail over instead of ranking a dead tether as the best uplink (gated on new CONFIG_TINYUSB_SUSPEND_CALLBACK/CONFIG_TINYUSB_RESUME_CALLBACK, taking the same rx lock so it can't race esp_netif_destroy). Gives the lwIP netif its own MAC (host MAC with bit 0x02 flipped) rather than sharing the NCM endpoint's, fixing a spec violation where the host learned the device IP against its own MAC. Runs the WiFi failover toggle in supervisor_task on a short-lived internal-stack worker (wifi_set_enabled_on_internal_stack) synchronised via a dedicated semaphore, since dcs_wifi_set_enabled touches flash/NVS and the supervisor's PSRAM stack asserts on flash access; if the worker can't get internal RAM the toggle is skipped and retried next tick.
Worth a look
- Multiple supervisor tasks can race on shared WiFi-toggle request —
firmware/components/dcs_support/src/dcs_net_supervisor.c:290· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- USB tether DHCP/ARP MAC changed incompatibly —
components/ml_dev_tether/src/ml_dev_tether.c:357· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 26 functions depend on the 18 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dcs_support_init()— 2 callers, 30 callees - new:
page_state()— 0 callers, 38 callees - new:
supervisor_task()— 0 callers, 8 callees
Verification — 26 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 26 function(s) in the blast radius were not formally verified this run
· 3 more finding(s) on lines outside this diff (see the check run).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes USB link-state failover behavior, introduces a new cross-task WiFi-toggle worker synchronized by a dedicated semaphore, and reaches NVS/flash off a PSRAM-backed task in the failover path, a human look would still be worthwhile.
What was reviewed: the on_usb_event USB-bus-state -> netif link-state mirroring under s_rx_lock; the wifi_toggle_task/s_wifi_toggle_done worker replacing direct dcs_wifi_set_enabled() calls from the PSRAM-stacked supervisor; the device-side MAC now diverging from the host MAC (dev_mac[5] ^= 0x02); and the two sdkconfig.defaults additions that gate the new suspend/resume code paths. Also checked: whether the worker's 4096 B stack is adequate given a since-removed comment cited 4608 B, and a pre-existing (now more frequently triggered) unlocked write/locked-read race on s_netif between ml_dev_tether_try_start() and on_usb_event — both flagged as candidates by the automated review but not treated as blocking.
Extended reasoning...
Overview
The diff touches three related areas in the USB-tether/net-supervisor failover stack: (1) components/ml_dev_tether/src/ml_dev_tether.c adds a TinyUSB event callback (on_usb_event) that mirrors USB attach/detach/suspend/resume onto the lwIP netif's link state, switches to TINYUSB_CONFIG_EVENT, and changes the device-side netif MAC to differ from the host-adopted NCM MAC by one bit; (2) firmware/components/dcs_support/src/dcs_net_supervisor.c moves the WiFi enable/disable toggle off the PSRAM-stacked supervisor task onto a short-lived internal-RAM worker task synchronized via a dedicated binary semaphore, to avoid an NVS-from-PSRAM-stack assert; and (3) two sdkconfig.defaults files add the Kconfig options needed for the new suspend/resume callbacks to compile. I read the full diff and the surrounding code (netif lifecycle, s_rx_lock usage, ml_dev_tether_stop(), supervisor task creation) to confirm the changes are internally consistent with the PR's stated rationale, and traced the MAC derivation math and the new task/semaphore lifecycle by hand since the underlying esp_tinyusb/TinyUSB headers are not vendored in this checkout and could not be directly inspected.
Security risks
No injection, auth-bypass, or data-exposure surface is introduced. The main risk class here is concurrency/lifecycle correctness (a stale or torn pointer read, a semaphore mis-synchronization, or an under-sized stack causing a crash) rather than a traditional security vulnerability. A crash-on-failover bug in this path is operationally significant, though, since this is a robot's connectivity failover mechanism.
Level of scrutiny
This warrants more than a rubber-stamp look even though the automated bug hunt reported no findings. It is not a mechanical/config-only change: it adds a new interrupt/callback-driven link-state path guarded by a pre-existing mutex, introduces a brand-new cross-task synchronization primitive (a worker task plus binary semaphore) to work around a real observed boot-loop assert, and changes an addressing property (MAC) of a network interface. The PR author's own description documents a bench abort caused by this exact area formerly, which underscores that these paths are subtle enough to have already produced a hardware failure once during development. That combination (state-machine change + new concurrency primitive + prior real incident in the same code) is enough that I would not personally sign off without a second set of eyes, even though I did not find a concrete bug.
Other factors
The automated review's own ruled-out list flags two items I found credible to double check by hand — a potential undersized worker stack (4096 B vs. a since-removed comment's 4608 B figure) and a pre-existing, now more frequently exercised, unlocked-write/locked-read race on s_netif — and I did not find a way to independently prove either is safe (the former depends on runtime memory layout the diff doesn't change and the latter is inherited pre-existing behavior, so I did not treat these as new blocking findings, consistent with the bug hunter's own disposition). There is no CODEOWNERS/CLAUDE.md constraint in play, and the timeline shows no outstanding third-party CHANGES_REQUESTED review to weigh against approval. Given the zero-findings result but the change's real complexity and safety-adjacent purpose, deferring to a human rather than approving is the appropriate call.
|
Graphify escalations, answered:
|
|
Superseded by #156 (consolidated remote-connectivity PR, same commits cherry-picked linearly; all review findings from this thread are addressed there). Closing. |
…rd re-establishment, coord deadline (#156) Consolidates #149, #152, #151, #155 and #153 (closed as superseded; their review threads and bot findings were addressed there and the fixes are included here). 12 linear commits, 409+/29−. Every commit was in the build that passed the 3.7 h soak below. ## What is fixed | area | commit(s) | defect | |---|---|---| | USB tether TX | `de335e8` `1b5fe82` | `tinyusb_net_send_sync(…, 0)` reclaimed the packet before the equal-priority TinyUSB task ran the deferred send — ~95 % of device→host frames silently dropped (regression from #135). Now a 20 ms wait; TIMEOUT-after-send duplicate accepted and documented. | | USB tether ring | `3a8a527` | 16-slot ring lost ~1 safety frame/min on a hub-chain host (burst-induced). 64 slots (96 KB PSRAM), 400 ms lifetime. | | Tether link state | `f9ec4cb` | netif never went link-down when the host detached/suspended, so a dead tether stayed the preferred uplink. esp_tinyusb events now drive it; device netif gets its own MAC. | | WiFi failover crash | `9823785` | first time the failover path ever ran it aborted: `dcs_wifi_set_enabled` reaches flash from the supervisor's PSRAM stack. Toggle moved to an internal-stack worker. | | WireGuard re-establishment | `0440ad2` `5b86d21` `e020c86` `cae7e94` `54149eb` `d7a335f` | **root cause of the multi-minute/indefinite bond drops**: on direct regain with no keypair left, the live endpoint stayed 0.0.0.0 (relay-only) and a safety peer fell into the bulk-peer "one-shot, wait for them to initiate" — the machine host's wireguard-go never initiates without data. Safety peers now connect with paced retries; initiations get a second leg to the peer's authenticated direct disco source while DERP-only; disco pings fan out to it. Per-peer WG diagnostics in `/admin/api/peers`. | | Coord partial frame | `8cf5104` | `coord_recv` bounded by a retry count × 2 s socket timeout (≈10 min); now a 10 s wall-clock deadline, `ETIMEDOUT` so callers reconnect (#127). 88-check host test. | ## Evidence - Deterministic WG repro (block direct+DERP, reboot remote mid-outage, unblock direct only, no host help): **v1.3-14 never recovers (>300 s); this branch bonds in 22 s.** - Soak 2026-09-19 12:06–15:47 PDT, 3.7 h, this code + #148/#150/#154, two Ethernet remotes + one USB remote bonded to a ROS 2 machine on the tether host, DUT E-stop cycled every 5 min (42/42, stop ≤0.5 s, re-arm ≤0.75 s): **zero bond drops / rebonds / resets, 100 % bonded samples on all three** (12 638 × 1 Hz each); reply age p99.9 361–391 ms; USB ring 566 916 frames, 0 expired, 0 dropped; machine-side 194 207 requests, 194 170 answered (the rest late DERP-mirror copies, correctly rejected). One 1.07 s receive-side excursion on an Ethernet remote (W5500 RX class, tracked separately). - Baseline for the same class of USB remote on v1.3: 252 ring drops + 55 expiries per 4 h, and 2–7 min bond outages. ## Not in this PR #148 (release guard), #150 (name scrub), #154 (lockstep mismatch diagnostics — SIL comparator, reviewed separately). DERP-mirror latency (1.3 s median) and the W5500 RX stall are follow-ups.
1 participant
Three small fixes found on the bench while re-testing the tether after #149. Independent of #148/#149/#150 (disjoint files).
1. Tether netif never went link-down (item 4)
The tether netif was brought up with a synthetic
esp_netif_action_connectedand nothing ever undid it. A host that de-enumerated us, suspended the bus or went away left the netif up with its lease, so the net supervisor kept ranking a dead tether as the best uplink and never failed over — bench: port de-authorised, device kept10.42.0.x, no WiFi for minutes. Now the esp_tinyusb device events drive the link: attached/resumed → up, detached/suspended → down (CONFIG_TINYUSB_SUSPEND/RESUME_CALLBACKenabled for both roles).2. WiFi failover crashed on the supervisor's PSRAM stack (found by fix 1)
The first time the failover path ever ran on a USB unit it aborted:
dcs_wifi_set_enabled()reaches flash (wifi_listNVS blob, PHY-cal NVS) andnet_suphas a PSRAM stack, sospi_flash_disable_interrupts_caches_and_other_cpuassertsesp_task_stack_is_sane_cache_disabled(coredump:net_sup → dcs_wifi_set_enabled → ml_config_get_wifi_list → nvs_get_blob). Any Ethernet unit losing Eth+USB would have hit the same abort. The toggle now runs on a transient 4 KB internal-stack worker; the supervisor waits on a dedicated semaphore so its link-down task notification is not consumed. The old comment claiming the supervisor "does no flash/NVS" is corrected.3. Device netif MAC == host MAC (item 8)
The MAC given to
tinyusb_net_init()is the host's (CDC-NCM iMACAddress); the lwIP netif used the same one. Worked on Linux by accident, spec violation, indistinguishable frames in captures. The device side now flips one more bit (still locally administered, derived from the unit's WiFi MAC).Verification (bench, fresh ESP32-S3 + field remote, Linux host)
…:d5, device DHCP lease at…:d7; tether 10/10 pings, HTTP 17 ms.USB detached -> tether link down→ supervisorno Eth/USB for 20s — bringing up WiFi→ v1.3-10 aborted here (fix 2); with fix 2 the unit came up on WiFi and stayed reachable via the tailnet; re-authorise → lease back in 4 s.96+/11−.