fix(usb): give the NCM offer a real wait — zero-tick send_sync never transmits

[iliabaranov]

Bug (regression from #135)

ml_usb_tx.c offered each frame with tinyusb_net_send_sync(…, 0). That API defers can_xmit+xmit onto the TinyUSB task and waits on an event group; with a zero wait the caller immediately takes its own semaphore back and nulls the packet, so the deferred send finds nothing. Both tasks run at priority 5, so the TinyUSB task never preempts — the frame goes out only when it happens to win a microsecond race from core 1. Retries repeat the race until the 100 ms TTL expires.

Symptom in the field: a USB-tethered remote shows a green tether and a red internet strobe; ARP/DHCP renewals/pings from the host mostly go unanswered. Device RX is intact (usbmon shows every host frame ACKed; DHCP broadcasts complete), only device→host transmit is starved.

Fix

Wait up to 20 ms for the TinyUSB task (TX_SYNC_WAIT_MS). ESP_FAIL = endpoint busy → retry; ESP_ERR_TIMEOUT → the API has already withdrawn the packet, so a retry cannot duplicate. The drain task exists precisely so this call may block.

Verification (fresh ESP32-S3, clean NVS, Linux host, NM shared NCM)

	v1.3	this fix
ping over tether	0/20	29/30, then 591/600 at 20 pps (2–24 ms)
state.json over tether	timeout	200 in 17 ms ×5
usb_tx counters	17 sent / 411 expired / 979 busy in 150 s	+1003 sent / 0 expired / 1 ring-full in 35 s under flood
gateway probe	red strobe	12/0, inet_down=0

Fleet backend reaches the unit through the USB uplink; supervisor drops WiFi as designed once USB is healthy. Second unit (field remote re-plugged on the same host) to follow in a comment.

Known residual: a handful of duplicate frames when the TinyUSB task takes >20 ms (TIMEOUT after an actual send); harmless for IP and pstop sequencing.

14+/4−.

[graphify-labs]

Graphify reviewed this change.

Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.

---

Graphify review — findings

Gives USB frame offers a non-zero TX_SYNC_WAIT_MS (20 ms) window in usb_drain so tinyusb_net_send_sync waits for the TinyUSB task to actually xmit instead of reclaiming the packet under a zero-tick call. This fixes the ~95% frame loss and busy-retry storm caused by the equal-priority, cross-core TinyUSB task losing the race; on timeout the API has already withdrawn the packet, so the in-order retry can't duplicate.

Worth a look

• tinyusb_net_send_sync timeout is converted to ticks despite millisecond API — components/ml_dev_tether/src/ml_usb_tx.c:87 · 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 — 16 functions depend on the 7 functions this change touches.

Health — this change adds coupling hotspots:

• new: page_state() — 0 callers, 38 callees
• new: supervisor_task() — 0 callers, 8 callees

Verification — 16 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: 14 function(s) in the blast radius were not formally verified this run

· 2 more finding(s) on lines outside this diff (see the check run).

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline duplicate-frame finding, I also checked whether the longer (20 ms vs. ~0) tinyusb_net_send_sync wait widens the disable/teardown race in usb_drain() (epoch check at line 70 runs once, before the now-longer blocking call). It does widen that window, but the TX pool persists across enable/disable cycles and the epoch invariant only ever promised that no queued frame outlives a session — not that an in-flight offer aborts mid-call — so this is a benign timing change rather than a new correctness/memory-safety bug.

Extended reasoning...

This is the narrow findings-present exception: a CONFIRMED inline finding (duplicate frames possible when tinyusb_net_send_sync times out after an actual send) is already being posted, so a human look is signaled regardless. I independently re-read the diff and the surrounding usb_drain/ml_usb_tx_set_enabled logic to check the ruled-out teardown-race candidate before deciding whether it added anything beyond the confirmed finding: the epoch/parity mechanism documented at the top of the file (lines 11-14) guards against a queued frame from a torn-down session being sent after re-enable, not against an in-flight offer completing after a disable lands mid-call; since the frame pool is allocated once in ml_usb_tx_init and never freed/reused across enable/disable toggles, a stray late transmit from a fast toggle is a behavioral nuance, not a memory-safety or double-free issue. I did not re-litigate the confirmed duplicate-frame finding itself since it will already appear as an inline comment.

[iliabaranov]

Second unit confirmed: a field remote that showed the red internet strobe on this host all afternoon (v1.3: 0/20 pings over its tether, usb_tx 17 sent / 411 expired) was OTA'd to this branch and re-tested on the same host, cable and NM shared interface: 30/30 pings at 2.7 ms avg, state.json 200 in 16 ms, usb_tx 177 sent / 1 expired / 0 drops, gateway probe 16/0, inet_down=0; the supervisor then dropped WiFi in favour of the now-healthy tether, as designed.

Also verified at the USB level before the fix (usbmon): every host→device bulk-OUT NTB completed with status 0 and the device's DHCP renewals/broadcasts all worked — RX was never the problem, only transmit.

[iliabaranov]

Re the automated note that the timeout is "converted to ticks despite a millisecond API": the esp_tinyusb signature is esp_err_t tinyusb_net_send_sync(void *buffer, uint16_t len, void *buff_free_arg, TickType_t timeout) (tinyusb_net.h:93), and it is passed straight to xEventGroupWaitBits. Ticks are correct; pdMS_TO_TICKS(TX_SYNC_WAIT_MS) stays.

[graphify-labs]

Graphify reviewed this change.

Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.

---

Graphify review — findings

Gives the USB TX drain a non-zero TX_SYNC_WAIT_MS (20 ms) wait when calling tinyusb_net_send_sync instead of a zero-tick offer, so the equal-priority TinyUSB task on the other core actually runs the xmit before the packet is reclaimed. The old zero-wait path silently dropped ~95% of frames and triggered a busy-retry storm; the tradeoff is that a ESP_ERR_TIMEOUT racing a just-completed xmit can send a frame twice, which is accepted as harmless to IP/TCP.

Worth a look

• tinyusb timeout is double-converted from ms to ticks — components/ml_dev_tether/src/ml_usb_tx.c:91 · Escalate · high
  • 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 — 16 functions depend on the 7 functions this change touches.

Health — this change adds coupling hotspots:

• new: page_state() — 0 callers, 38 callees
• new: supervisor_task() — 0 callers, 8 callees

Verification — 16 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: 14 function(s) in the blast radius were not formally verified this run

· 2 more finding(s) on lines outside this diff (see the check run).

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding, I also checked whether the new 20 ms tinyusb_net_send_sync wait could make later frames in a burst hit the 100 ms TTL and get counted expired under sustained TinyUSB-task scheduling delay — a failure mode the old zero-tick wait couldn't produce. The pre-existing retry backoff already budgeted comparable per-attempt latency (up to 32 ms while busy) against that same 100 ms TTL, so this isn't a new bug introduced by this change.

Extended reasoning...

This run's inline finding (ml_usb_tx.h still documenting the old "zero-tick timeout" offer behavior, now stale after ml_usb_tx.c's usb_drain() blocks up to TX_SYNC_WAIT_MS) is a genuine but minor doc/code mismatch, already flagged as an inline comment. My prior review's concern about ESP_ERR_TIMEOUT-after-actual-send causing a duplicate was addressed by the follow-up commit 7dfa861, which rewrote the comment to honestly state that a duplicate is possible and accepted (rather than claiming TIMEOUT can never duplicate) — I won't restate that since it's resolved. I additionally examined the candidate that sustained TinyUSB-task delay could now cause later frames in a burst to be silently counted "expired" rather than offered, comparing the new per-attempt latency (up to 20 ms sync wait) against the pre-existing 32 ms max backoff sleep and the unchanged 100 ms TTL; the two are the same order of magnitude, so this isn't a materially new failure mode versus the pre-existing TTL/backoff design. Given a real (if minor) finding is present, I'm using the narrow ruled-out-note exception rather than approving or staying fully silent.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

• 🟡 components/ml_dev_tether/include/ml_usb_tx.h — nit: the public header still documents the old, now-wrong behavior. It says the head frame is offered with a 'zero-tick timeout', but ml_usb_tx.c's usb_drain() now blocks up to TX_SYNC_WAIT_MS (pdMS_TO_TICKS(20)) per offer. A maintainer or integrator reading only ml_usb_tx.h (the public contract) will still believe sends are non-blocking/instant, which is now false. Fix: update this comment to describe the bounded 20 ms wait and why it's required (letting the equal-priority TinyUSB task actually run the deferred xmit), matching the reasoning already in usb_drain()'s comment.

  Extended reasoning...

  ml_usb_tx.h:14 (unchanged by this diff) says: 'the head frame is offered with a zero-tick timeout; on NCM-busy it is KEPT and retried'. The diff changes ml_usb_tx.c:89 from tinyusb_net_send_sync(..., 0) to tinyusb_net_send_sync(..., pdMS_TO_TICKS(TX_SYNC_WAIT_MS)) (20 ms), but does not touch ml_usb_tx.h. Anyone consuming this component through its header (the documented public contract, per the file's own comment style) now gets an inaccurate description: they would reasonably assume the drain task never blocks in the send call, e.g. when reasoning about drain-task latency, task-priority tuning, or when deciding whether it's safe to call from a context that must not block. This is a documentation-only defect (no functional bug), but it directly contradicts the new, correct behavior explained in the sibling comment at ml_usb_tx.c:77-88, so the two comments in the same component now disagree with each other post-merge.

  Verification: nit: The public header comment is now stale/wrong as a direct result of this diff. ml_usb_tx.h:14-15 (unchanged) states: "the head frame is offered with a zero-tick / timeout; on NCM-busy it is KEPT and retried". The diff in ml_usb_tx.c changes the offer from tinyusb_net_send_sync(s->bytes, s->len, NULL, 0) to tinyusb_net_send_sync(s->bytes, s->len, NULL, pdMS_TO_TICKS(TX_SYNC_WAIT_MS))…

[iliabaranov]

Superseded by #156 (consolidated remote-connectivity PR, same commits cherry-picked linearly; all review findings from this thread are addressed there). Closing.