cleanup: follow-ups from the station series reviews (#451-#464), part of #465 by snokvist · Pull Request #466 · OpenIPC/devourer

snokvist · 2026-10-03T09:44:41Z

Follow-ups and fixes collected from merged PRs: the maintainer's non-blocking review notes, a probable cause for #461's one-entry loss, part of the #465 harness defect sweep, and two txdemo teardown fixes we found ourselves. Fifteen commits, one per item or per harness, so any can be dropped or reordered on review, plus three follow-ups from the qodo review of this PR.

Refs #461, Refs #465 (partial).

What changed

Maintainer review notes

Commit	Change
`0fee106`	#452: `txs_parse_long` carried two comment blocks back to back. The first one documents `txs_retry_limit_env`, and it now sits there.
`4d862ff`	#453: TxBeaconGuard retries StopBeacon when an armed beacon's disable is refused. After a successful StartBeacon, the Jaguar2/3 StopBeacon returns false only for a refused disable, and `_bcn_hw_touched` makes a retry re-run it. A clean false after a refused StartBeacon still ends the loop.
`294a671`	#453: the dated, attributed bench paragraph in `docs/jaguar3-tx-ring.md` becomes plain measurement. `IRtlRadio::GetTxDmaStatus` now says bit 13 (`BIT_PAYLOAD_OVF_8822C`) can latch at max duty on USB2, not that it does.
`ddd8467`	The older bring-up gates in `src/mt7612u/tools/bringup.cpp` now call `mt_mac_stop()` when `mt_mac_start()` fails. `mt_mac_start` sets `ENABLE_TX` before its WPDMA poll, so a failed start could leave TX enabled. Each gate keeps its own teardown order (`rx_teardown()` first where a ring was draining EP 4).
`2cc5330`	#463 review: Jaguar1/2/3 `Stop()` clears `_station_ready` in the block that clears the arm, under the station lock. A `SetStationIdentity` after `Stop()` is refused until the next bring-up.
`907a5b5`	#463 review: `tests/realtek_station_onair.sh`'s submission floor is scaled to the span the arm actually aired, not to SECS, which also holds the peer's bring-up. The span runs from the first submit to the final `tx.stats` (`fcdec63`, below).
`da8cd6d`	#463 review: the same script runs its whole AP guard in the preflight, before the DOWN half spends its minute, and again at the start of UP.

#461: the txs gate's lost status entry

Commit	Change
`2e7a3de`	`gate_txs` claims an arm's first status entry back when a stale `MT_TX_STAT_FIFO_EXT` word mislabelled it with the previous arm's pktid. Details under Why.

#465 (partial): the harness defect sweep

Commit	Harness	Change
`53477e0`	`mt7612u_ap_onair.sh`	An interrupt stops the run instead of carrying on into the next cell. Before re-enumerating `AP_SYSFS`, cleanup waits for its children to exit (10 s bound, zombie-aware via the lib's `sta_pid_alive`), and skips the re-enumeration if it had to KILL one. The stop cell FAILs when PHASE 2 never appeared, rather than scoring "beacon gone". The ping-loss figure is quoted whole ("66.6667%", not "6667%").
`05bdaf7`	`mt7612u_sta_autoack.sh`	DUT hand-back only after its process is confirmed gone, with a CLEANED guard so the EXIT pass after an INT cannot undo that refusal.
`8a2ab14`	`mt7612u_sta_uplink.sh`	The same hand-back rule. The DUT's `mt7612uprobe txs` runs under `timeout -s INT -k 10`, bounded by the gate's own worst case.
`bf9c0dd`	`mt7612u_sta_identity.sh`	`AP_REENUM` is set before hostapd starts, and hostapd is killed unconditionally. The BSSID gate is bounded. The monitor vif is probed in the preflight. Exit codes are 0/1/2/3, with rig refusals as 2 and the INT/TERM trap as 3.
`d473027`	`mt7612u_sta_onair.sh`	The hostapd restart race. Before each launch, `ap_up` waits (10 s bound, inside the netns) for the AP netdev, forces it back to `type managed`, and brings it up.

Not in this PR, from #465: the generic Realtek DUT take, the reconnect cell and the CCMP soak. Those come in a separate PR.

Our own finding: txdemo thread joins

Commit	Change
`64ebce3`	`examples/tx/main.cpp` joins the optional IN drainers (`DEVOURER_DRAIN_BULK_IN`, `DEVOURER_POLL_INTR_IN`) on every exit.
`6b31d5f`	It also joins the RX and USB event threads on every exit.

In both cases an early return used to destroy a still-joinable std::thread, which calls std::terminate. Small scope guards now join the threads in the normal teardown's order, before session.close(). The legacy fork child is covered by 18f0a20, below.

qodo review of this PR

Commit	Change
`fcdec63`	`realtek_station_onair.sh`: the aired span started at the first `tx.report`, so a transmitter that started, stalled and then burst 50 reports in its last second passed both floors. txdemo's `tx.frame` now carries `t` (the `tx.report` timebase; `docs/logging.md`). The span runs from the first submit, and liveness also refuses a first report more than `MAX_GAP_MS` after it (`lead_ms`).
`92dfa1d`	`mt7612u_ap_onair.sh`: if a between-cell cleanup could not reset the AP (a process outlived TERM), the remaining cells are recorded as NOT RUN and the run exits 2, instead of being scored on an unreset adapter that may still be beaconing.
`18f0a20`	txdemo: the `DEVOURER_TX_WITH_RX` fork child leaves through `std::_Exit` after flushing stdio, with `Init` in a try. No destructor runs in it, so its copies of the IN-drainer threads no longer terminate it.

Why

Follow-ups left open by the station identity seam (#460) #461 (the txs entry loss). The FIFO read is two USB control transfers: EXT first, then the main word, which pops the entry.
- If an entry is filed between the two reads, the pop is paired with the previous entry's EXT word.
- Inside an arm that is harmless, because every entry carries the same pktid.
- On an arm's first entry it carries the previous arm's pktid. That entry is counted late, the arm stays one short, and every per-frame wait then times out. These are the ~6 fps N-1/N rows.
- docs/mt7612u-tx-retry.md's own table rules out the alternative, "status posted only on the next TX". At limit 0, receiver ON, arm f lags with its late entry in its own row, after arm e settled and owed nothing. Arm g after it shows no late entry.
Follow-ups left open by the station client (#464) #465. Each item was a way for a harness to score a dead or wedged device as a result, hang without bound, or re-enumerate an adapter under a process still in its de-init.
The txdemo threads. A joinable std::thread destructor terminates the process, so an otherwise clean early exit aborted.

Measured

Follow-ups left open by the station identity seam (#460) #461, on the author's unit: gate_txs with the stale-EXT claim settled 16/16 arms at retry limits 5 and 0 (recorded on Follow-ups left open by the station identity seam (#460) #461).
- The claim's current form is keyed to the last arm that actually sent a frame, so an arm whose every submit failed is skipped. That refinement has not been run on hardware.
The uplink harness (tests/mt7612u_sta_uplink.sh) is INCONCLUSIVE both on this branch (48/53 status entries) and on master (57/60). It predates the branch.
- That loss is several entries per arm. It is not the race above, which accounts for at most one entry per arm, and it is still being investigated.
Submission floor (907a5b5), checked on synthetic JSONL through the script's own summarize/usable:

Fixture	Old floor	New floor
Slow bring-up, 327 frames in 2.0 s	refused at 500	usable, floor 101
Start, stall, 60 reports in the last second	refused at 500	refused: lead_ms 8800 (live=0), and 60 < floor 490
Rate stall, 110 frames over 10 s	—	refused, floor 500
Hard stall	—	refused, live=0
No `tx.frame` with `t` (an older txdemo)	—	refused, live=0

What it can't do

The stale-EXT claim recovers at most one entry per arm, and only when the previous sending arm settled. A deficit larger than one entry is not this race.
The INT/TERM exit code is not uniform across the harnesses:
- realtek_station_onair, mt7612u_sta_onair and now mt7612u_sta_identity exit 3, as their headers document;
- autoack, uplink and ap_onair still exit 130.
- Unifying them is left for later.

Follow-ups

Find the uplink harness's multi-entry status loss.
Hardware-run the "last arm that sent" form of the txs claim.
Unify the interrupt exit code across the station harnesses.
From Follow-ups left open by the station client (#464) #465: the generic Realtek DUT take, the reconnect cell and the CCMP soak (separate PR).

Verification

ctest: 83/83, both plain and under -DDEVOURER_SANITIZE=address+undefined, built with -DDEVOURER_MT7612U=ON -DDEVOURER_REQUIRE_STA_CRYPTO_TESTS=ON. The two reference-submodule tests are skipped without reference/.
make -C src/mt7612u check passes.
bash -n (and sh -n for the /bin/sh scripts) is clean on every touched script. shellcheck -x is clean on all of them except tests/mt7612u_ap_onair.sh, whose remaining SC2046/SC2015/SC2012 notes predate this branch.
Hardware, on this head (6b31d5f): MT7612U at 1-1, RTL8812CU at 5-1, RTL8812BU at 8-1 (rtw88), ch6, near field. Every adapter was handed back after each harness, and no hostapd, netns or monitor interface was left.

Harness	Result
realtek_station_onair	5 passed, 0 failed, 0 inconclusive, both ways (8812CU and 8812BU as the station)
mt7612u_sta_onair	16/16
mt7612u_sta_identity	rc 0 (STAID 12/12; STAACK reports its own A/B as non-discriminating on this rig, and only arm C is scored, as before)
mt7612u_ap_onair	14/14
mt7612u_sta_autoack	3/3

The three qodo follow-ups (fcdec63, 92dfa1d, 18f0a20), re-run on 18f0a20: realtek_station_onair 5 passed, 0 failed, 0 inconclusive, both ways, under the floor that now starts at the first submit. mt7612u_ap_onair 14/14. Every adapter was handed back.
The same harnesses also ran on the pre-squash head, with the same results. mt7612u_ap_onair showed one flaky open-cell ping run there, which passed on three repeats. mt7612u_sta_uplink was INCONCLUSIVE there and on master alike. That is the multi-entry loss under Follow-ups left open by the station identity seam (#460) #461, which this PR doesn't address.
Review record: qodo-gate flagged three threads on 6b31d5f. All three held, and are fixed in fcdec63, 92dfa1d and 18f0a20.

Blast radius

Library: the three Jaguar Stop()s only. Each clears _station_ready, and only the SetStationIdentity gate reads it. Every bring-up already clears it on entry and sets it again at the end.
Everything else is src/mt7612u/tools/ (the bring-up tool), examples/tx/ (txdemo), tests/ and docs/.
One event-schema addition: txdemo's tx.frame gains a trailing t. Every in-tree consumer either matches on "n"/"rc" or parses the JSON, so none of them changes.
src/IRtlRadio.h and src/StationArm.h: comment-only.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

txs_parse_long carried two comment blocks back to back; the first one documents txs_retry_limit_env (DEVOURER_TX_RETRY_LIMIT, 1/0/-1 return), which had no comment of its own. Move it there. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

After a successful StartBeacon, the Jaguar2/3 StopBeacon returns false only when the disable itself was refused - the "nothing active" early return cannot fire, and _bcn_hw_touched makes a retry re-run the disable. TxBeaconGuard treated every false as done. Retry on false while armed; a clean false after a refused StartBeacon still ends the loop. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

docs/jaguar3-tx-ring.md: the dated, attributed bench paragraph becomes plain measurement inside the 8812CU paragraph it qualifies. - On USB2 at DEVOURER_TX_GAP_US=0, bit 13 (BIT_PAYLOAD_OVF_8822C, 0x2000) latched while 8051/8051 frames completed. - It read 0 at the default gap. - One later USB2 gap-0 run did not reproduce the latch, so the bit can latch at max duty on USB2. - A second bench reproduced the defect and the fix clearing it. Git carries the provenance. src/IRtlRadio.h, the GetTxDmaStatus contract, said bit 13 "latches" under host-side max-duty backpressure. It now says "can latch", noting that a later such run did not latch it. docs/logging.md defers to that declaration. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

mt_mac_start() writes MT_MAC_SYS_CTRL_ENABLE_TX before its WPDMA-idle poll and returns -1 without clearing it, so a gate that returned straight out of a failed start could leave TX enabled. Every such failure path now calls mt_mac_stop() before returning, after the gate's existing cleanup and in its normal teardown order: rx_teardown() first where a ring was draining EP 4 (gate_ap's bare rx_stop becomes rx_teardown), mt_mac_rx_disable() first in gate_rx as its normal exit does. The failed start never reaches mt_async_start() in gate_txs, and gate_caps' async drainer is torn down by rx_teardown() as on its normal exit. gate_tsfwrite already did this. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

…EXT read Refs OpenIPC#461: on a second MT7612U the ~6 fps txs arms read 199/200 (59/60) while the fast arms settle 200/200, which leaves the uplink harness INCONCLUSIVE. The slow rate is a symptom, not the cause: those are the arms that lost their first entry, after which every per-frame wait (entries >= n) times out. The status read is EXT, then the main word: two USB transfers. When the FIFO is empty at the EXT read and an entry is filed before the main read, the pop is paired with the stale EXT word of the previously popped entry. - Inside an arm that is the same pktid, and harmless. - On an arm's first entry it is the previous arm's pktid, so the entry was counted late (foreign on the session's first arm). That is the "one-step status lag" of docs/mt7612u-tx-retry.md. Its own table rules out the alternative, status posted only on the next TX: at limit 0, receiver ON, arm f lags with L1 after arm e settled 40/40 and owed nothing, and arm g after it shows no late entry. The late entry belongs to the lagging arm, not the one after it. A race on the poll timing also explains why it varies from pass to pass and from host to host. txs_drain now claims that one entry back when it can only be ours, which takes all of the following: - it is the arm's first popped entry, after the arm has submitted a frame; - it carries the pktid of the last arm that SENT a frame, and that arm settled with no entry owed; - or, on the session's first arm, it carries any pktid. An arm whose every submit failed popped nothing and leaves that reference untouched. The claimed entry counts toward entries, and toward success when its SUCCESS bit is set (the main word is fresh). It stays out of the retry columns (its retry count is the stale word's), and is reported as "stale-EXT entries claimed". After an UNSETTLED arm, a previous-pktid entry is still reported as late. The claim recovers at most one entry per arm. A larger deficit, like the uplink harness's multi-entry loss, is not this race. Hardware: in the form keyed to the previous arm, the author's unit settled 16/16 arms at limits 5 and 0 (issue OpenIPC#461). Keying it to the last arm that sent has not been run on hardware. No headless test: txs_drain reads the FIFO through mt_rr_chk on a live device inside the bring-up tool, which no selftest links. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

With DEVOURER_DRAIN_BULK_IN or DEVOURER_POLL_INTR_IN set, bulk_in_thread and intr_in_thread were joined only on the InitWrite-exception path. The normal end of main and the early returns after a PCIe-open or no-driver failure destroyed them still joinable, so the process ended in std::terminate. A small scope guard after the two threads now clears both run flags and joins them on every return. The normal path joins explicitly before session.close(), because both threads poll the handle it releases. The InitWrite catch block's open-coded join becomes the guard. A fork() child of the legacy DEVOURER_TX_WITH_RX path holds copies of the thread objects but not the threads, so it skips the join and keeps its pre-existing exit behaviour. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

…d span MIN_SUBMITTED defaulted to a quarter of SECS * 1e6 / GAP_US, but the SECS window also holds the transmitter's bring-up. On an 8812BU peer txdemo.init_write took 6.2 s and 8.7 s of the 10 s window, so the peer aired for only 1.3-3.7 s and submitted 283-327 frames against the 500 floor, with live=1 and max_gap under 35 ms. Every DOWN arm came back INCONCLUSIVE: a healthy transmitter scored as a stall. summarize now prints aired_ms, the span from the arm's first tx.report to its final tx.stats. Those are the first two timestamps in one timebase; txdemo.first_tx_submit counts ms from a different epoch. It also prints min_submitted, a quarter of the GAP_US rate over that span, and usable() holds each arm to its own floor. The span ends at the final tx.stats, not the last report, so a transmitter that slows or stops after MIN_REPORTS still owes the whole span. A set MIN_SUBMITTED stays a fixed floor. Checked on synthetic JSONL fixtures run through the script's own summarize and usable: - slow bring-up, 327 frames over 2.0 s: usable, floor 100 (refused under the old fixed 500); - rate stall, 110 frames over 10 s with sparse reports under MAX_GAP_MS: refused, floor 500; - hard stall: refused, live=0; - healthy full window: usable. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

The AP guard for the UP half ran only after the whole DOWN half. A run with the AP already off the bus spent 60 s on DOWN and then refused, and a hub, an adapter with no wireless netdev, one carrying a default route, or a phy without AP mode was refused just as late. The UP half's guard is now a function, ap_guard. It runs right after the lib is sourced, for the UP or both halves, and again at the start of UP, since the adapter can move during DOWN. The preflight only reads; nothing is written before the lock. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

Stop() cleared a held station arm but left _station_ready set, so a SetStationIdentity issued after Stop() was accepted: the same hole the gate closed for _brought_up. That arm went to a torn-down chip on Jaguar1/3, or to a still-powered chip on Jaguar2, which Stop() does not power down. All three Stop()s now clear _station_ready under the station lock, in the block that clears the arm (_port0_mu on Jaguar1, _reg_mu on Jaguar2/3). Nothing else reads the flag except the SetStationIdentity gate, and every bring-up already clears it on entry and commits it at the end, so a re-Init after Stop() arms again as before. StationArm.h's LIFETIME note says so. No selftest: _station_ready only becomes true at the end of a real Init/InitWrite, which no headless test can run. The existing station_arm selftest covers StationArm, not the device classes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

…live process - Interrupt. `trap cleanup EXIT INT TERM` ran cleanup on Ctrl-C and then carried on into the next cell against a re-enumerated adapter. The traps now follow the station harnesses: `cleanup; exit 130` on INT/TERM, cleanup on EXIT. A CLEANED flag, set only on the interrupt path, spares the EXIT pass a second re-enumeration. It is not a once-only guard, because CELLS=all calls cleanup between cells. - Hand-back. cleanup re-enumerated AP_SYSFS right after sending TERM, with the AP demo possibly still inside its de-init. reap now polls its children for up to 10 s with the station lib's zombie-aware sta_pid_alive (the lib is sourced for that alone). Anything still alive is KILLed, and cleanup then skips the re-enumeration, saying so. - PHASE 2. cell_stop waited up to 60 s for "PHASE 2" and then scored "beacon gone" whether or not it appeared. A bstop_onair that ended before StopBeacon - its teardown silencing the beacon - read as a StopBeacon PASS. Without PHASE 2 the cell now FAILs, naming that. - Ping loss. The open cell's failure message extracted the loss with `[0-9]+% packet loss`, which cut ping's "66.6667% packet loss" to "6667% packet loss". It now takes `[0-9.]+%`. The pass test is unchanged. - Dead loop counters (`local i`) dropped. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

cleanup ignored sta_pid_kill's result for the DUT and re-enumerated it regardless, including while the bring-up process was still inside its de-init. The peer already had this rule; the hand-back is now gated the same way, as in realtek_station_onair.sh. That needed a CLEANED guard. sta_pid_kill forgets the PID on its first call, so the EXIT pass that follows an INT would have read "gone" and handed back the adapter the first pass had just refused to. The old comment called that second pass harmless. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

… runs The DUT's `mt7612uprobe txs` ran under a bare `wait`, so a wedged gate held the run and both adapters forever. It now runs under `timeout -s INT -k 10` with a bound derived from the gate's own worst case: 16 gate arms; per frame, the status wait (b + 50 ms) plus the settle (b), where b is gate_txs's frame_budget_ms for RETRY_LIMIT; and about 7 s of fixed cost per arm. On top of that, half again plus 2 min for bring-up: about 9 min at FRAMES=60 and 19 at FRAMES=200, against the ~9 min measured for a 200-frame arm. An overrun ABORTs the arm. cleanup re-enumerated the DUT whether or not sta_pid_kill saw its process exit. The hand-back is now gated on that, as for the peer, with a CLEANED guard so the EXIT pass after an INT cannot undo the refusal. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

- hostapd. AP_REENUM was set only after the hostapd launch and its 10 s wait loop, and cleanup killed hostapd only when the flag was set. A Ctrl-C in that window left hostapd running and the AP in AP mode, with the lock released. The flag is now set once the AP guard has passed, before the interface is touched, and hostapd is killed unconditionally, as in realtek_station_onair.sh. - DUT hand-back. It ignored sta_pid_kill's result and re-enumerated the DUT with the BSSID gate still running. It is now gated on the gate having exited. - Bounded gate. `wait "$sta_gate"` was unbounded. The gate now runs under `timeout -s INT -k 10` for six arms of SECS plus 183 s. An overrun is no measurement (2), and stays 2 even when the injector never started. - Monitor vif. It was probed only after the contract and probe-response gates. It is now also probed (create, then delete) right after the AP guard. - Exit status. Everything collapsed into 0/1. It is now 0 pass, 1 a gate failed, 2 INCONCLUSIVE, 3 interrupted, combined as: 1 if any gate failed, else 3, else 2, else 0. - Rig refusals exit 2: a hostapd that never brought the AP up, and no monitor vif. realtek_station_onair scores the same hostapd failure as 2. - The INT/TERM trap exits 3, as documented. - Nothing in the tree reads this exit code. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

cell_end returned as soon as the previous hostapd exited, and ap_up launched the next one at once. On one rig, one restart in four started 30 ms after AP-DISABLED and died with "Could not read interface <if> flags: No such device" / "nl80211 driver initialization failed", which the cell scored INCONCLUSIVE. Before each launch, ap_up now spends up to 10 s, inside the netns, on these steps: - it waits for the netdev to be present; - once present, if it reports a type other than managed, it takes the interface down and sets type managed, retrying until the type reads managed; - it brings the interface up. Forcing the type matters because a driver may leave the vif in AP type after hostapd exits, and hostapd then fails with "Match already configured". realtek_station_onair.sh and mt7612u_sta_identity.sh force the type before every hostapd for the same reason. Nothing here is driver-specific, so it holds for an mt76x2u AP as well as rtw88. If the window runs out, the cell is refused as before (INCONCLUSIVE). The reason is written into the hostapd log that message points at, and the interface is left up. No hostapd retry: the wait removes the race. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

usb_thread and rx_thread were joined only at the end of main. The early returns after them - the three `return 1`s on the TX path - destroyed them still joinable, so the process ended in std::terminate. It is the same class as the IN drainers fixed earlier on this branch. A scope guard, IoThreadsJoin, declared right after rx_thread, runs the normal teardown's sequence on every exit: 1. StopRxLoop. 2. Join the RX thread. 3. Set g_devourer_should_stop. 4. Join the event pump. The normal path calls it explicitly where the joins were, before Stop(), so the order against Stop(), the drainers' join and session.close() is unchanged. On an early return it runs before the session releases the device. Both threads start after the DEVOURER_TX_WITH_RX fork, whose child returns first, so no fork-child skip is needed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

snokvist · 2026-10-03T09:44:46Z

@josephnef the cleanup PR I mentioned on #464: your non-blocking notes from #451–#464, plus part of #465's harness items, in 15 small commits on 927cfc7.

It's grouped by origin in the description.

Library changes: only the three Jaguar Stop()s, which now refuse a station arm until the next bring-up, as you suggested on jaguar1/2/3: the Realtek station arm for IRadio::SetStationIdentity #463.
Everything else: src/mt7612u/tools/, examples/tx/, tests/ and docs/.

Hardware, on this head (same rig as #463/#464):

realtek_station_onair: 5/5 both ways
mt7612u_sta_onair: 16/16, including the hostapd-restart wait from Follow-ups left open by the station client (#464) #465
identity: rc 0
ap_onair: 14/14
autoack: 3/3

Every adapter was handed back.

Not in this PR:

The uplink harness's multi-entry txs loss (Follow-ups left open by the station identity seam (#460) #461). The cause turned out to be the gate's pacing: frames were submitted faster than an unACKed ladder retires, so the USB ring's 1 s timeout cancelled them. There's also a likely library bug behind it: data frames go out on QSEL 1 (HCCA), not EDCA. Both are being measured now and will come separately.
The rest of Follow-ups left open by the station client (#464) #465: the generic Realtek DUT take, the reconnect cell and the CCMP soak. That is a separate PR in preparation.

qodo-free-for-open-source-projects · 2026-10-03T09:45:25Z

PR Summary by Qodo

Fix TX status attribution and harden on-air teardown

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Recover MT7612U status entries mislabelled by stale FIFO metadata.
• Prevent harness races, hangs, and unsafe USB hand-back from producing misleading verdicts.
• Join txdemo threads on early exits and reject station arms after Stop().

Diagram

graph TD
  H["On-air harnesses"] --> P["AP preflight"] --> G["Bounded gates"] --> V["Scored verdicts"] --> C["PID cleanup"] --> D{"Processes exited?"}
  D -->|Yes| U["USB handback"]
  D -->|No| S["Skip handback"]

High-Level Assessment

The following are alternative approaches to this PR:

1. Centralize harness teardown

➕ One shared policy could prevent cleanup and hand-back behavior from drifting across scripts.
➖ The harnesses have different child processes, AP ownership, and teardown orders; a shared abstraction would broaden this fix's regression surface.

Recommendation: Keep the targeted harness fixes for this PR: they preserve each rig's teardown requirements while closing the identified failure paths. Consider extracting a shared lifecycle helper after the remaining harness sweep establishes a common contract.

Files changed (15) +457 / -169

Bug fix (5) +190 / -68

main.cppJoin txdemo workers and retry refused beacon stops +63/-22

Join txdemo workers and retry refused beacon stops

• Adds scope guards so optional IN drainers, RX, and USB event threads are joined on early exits as well as normal teardown. Retries StopBeacon when an armed beacon's disable is refused, while accounting for the legacy fork child's thread copies.

examples/tx/main.cpp

RtlJaguarDevice.cppDisarm Jaguar1 station readiness on Stop +2/-1

Disarm Jaguar1 station readiness on Stop
• Clears '_station_ready' under the station lock before clearing any held identity, rejecting subsequent arms until bring-up.
src/jaguar1/RtlJaguarDevice.cpp

RtlJaguar2Device.cppDisarm Jaguar2 station readiness on Stop +3/-1

Disarm Jaguar2 station readiness on Stop
• Clears '_station_ready' under the register lock alongside station-identity teardown, preventing a post-Stop re-arm.
src/jaguar2/RtlJaguar2Device.cpp

RtlJaguar3Device.cppDisarm Jaguar3 station readiness on Stop +3/-1

Disarm Jaguar3 station readiness on Stop
• Clears '_station_ready' under the register lock before clearing a held station identity, preventing a post-Stop re-arm.
src/jaguar3/RtlJaguar3Device.cpp

bringup.cppRecover first TX status entries and stop MAC on failed starts +119/-43

Recover first TX status entries and stop MAC on failed starts

• Conditionally claims a first status entry carrying stale EXT metadata when the last sending arm settled, counting its success but excluding its unreliable retry value. Adds 'mt_mac_stop()' to failed-start paths across bring-up gates, preserving RX teardown order where needed, and relocates the retry-limit parser comment.

src/mt7612u/tools/bringup.cpp

Tests (6) +218 / -69

mt7612u_ap_onair.shMake AP harness interruption and teardown safe +44/-13

Make AP harness interruption and teardown safe

• Stops the run on INT/TERM and waits for children before considering AP re-enumeration, skipping hand-back after a forced kill. Requires the stop cell to reach PHASE 2 before scoring StopBeacon and retains decimal ping-loss percentages.

tests/mt7612u_ap_onair.sh

mt7612u_sta_autoack.shGuard auto-ACK DUT hand-back after cleanup +10/-4

Guard auto-ACK DUT hand-back after cleanup
• Re-enumerates the DUT only after its process is confirmed gone. Makes cleanup single-pass so an EXIT trap after interruption cannot reverse that decision.
tests/mt7612u_sta_autoack.sh

mt7612u_sta_identity.shBound and preflight the station-identity harness +54/-13

Bound and preflight the station-identity harness

• Probes monitor-interface support before measurement, marks the AP for restoration before hostapd starts, and always attempts to stop hostapd. Bounds the BSSID gate and distinguishes failed, inconclusive, and interrupted outcomes in exit codes.

tests/mt7612u_sta_identity.sh

mt7612u_sta_onair.shWait for AP netdev recovery before hostapd restart +27/-1

Wait for AP netdev recovery before hostapd restart

• Before each hostapd launch, waits up to ten seconds for the AP interface inside its namespace, restores managed mode, and brings it up. This addresses restarts racing the previous hostapd teardown.

tests/mt7612u_sta_onair.sh

mt7612u_sta_uplink.shBound uplink gates and protect DUT hand-back +24/-4

Bound uplink gates and protect DUT hand-back

• Runs the TX-status gate under a timeout derived from its worst-case waits. Re-enumerates the DUT only after its process exits and prevents a second trap cleanup from undoing a refusal.

tests/mt7612u_sta_uplink.sh

realtek_station_onair.shScore submissions over airtime and preflight the AP +59/-34

Score submissions over airtime and preflight the AP

• Scales the default submission floor to the interval from first TX report to final TX statistics, rather than time spent in bring-up. Moves AP validation into preflight and repeats it before the UP half.

tests/realtek_station_onair.sh

Documentation (4) +49 / -32

jaguar3-tx-ring.mdQualify the Jaguar3 TX-DMA overflow observation +10/-11

Qualify the Jaguar3 TX-DMA overflow observation

• Integrates the USB2 bench observation into the TX-ring analysis and clarifies that bit 13 can latch without a TX wedge. Distinguishes it from the bit 18 observation associated with the wedge.

docs/jaguar3-tx-ring.md

mt7612u-tx-retry.mdExplain the stale-EXT status lag and recovery limits +31/-16

Explain the stale-EXT status lag and recovery limits

• Reinterprets the one-entry lag using the two-transfer FIFO read and documents the gate's conditional first-entry claim. Records that the final last-sending-arm refinement has not been hardware-tested and cannot explain larger deficits.

docs/mt7612u-tx-retry.md

IRtlRadio.hQualify the bit 13 TX-DMA status contract +5/-4

Qualify the bit 13 TX-DMA status contract

• Clarifies in the API comment that bit 13 may, but does not invariably, latch under maximum-duty USB2 transmission without indicating a wedge.

src/IRtlRadio.h

StationArm.hDocument station readiness after Stop +3/-1

Document station readiness after Stop
• Notes that Stop clears station readiness, preventing a new station arm until bring-up.
src/StationArm.h

qodo-free-for-open-source-projects · 2026-10-03T09:52:37Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. ~~Late first reports can pass stalled arms~~ ✓ Resolved

Description

summarize() starts aired_ms at the first tx.report, so the new submission floor excludes any
time after transmission begins but before that report arrives. If reports stall until near the end
of a ten-second run, 50 reports in its last second can meet both the default 50-report minimum and
the roughly 50-submission floor, where the former 500-submission floor would have rejected the arm.

Code

tests/realtek_station_onair.sh[R330-334]

+aired = (final_t - ts[0]) if (final_t is not None and ts) else 0
+if fixed_floor:
+    floor = int(fixed_floor)
+else:
+    floor = aired * 1000 // gap_us // 4 if gap_us > 0 else 0

Evidence
The transmitter records a first-submit stage before sending, but the new floor uses only the first
report and final stats. The usability check accepts that computed floor alongside the 50-report
default, without checking the interval before the first report.
examples/tx/main.cpp[2093-2099]
tests/realtek_station_onair.sh[300-335]
tests/realtek_station_onair.sh[503-513]
tests/realtek_station_onair.sh[88-101]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The scaled submission floor starts at the first status report, so it cannot detect a transmitter that begins sending but stalls before that report.
## Fix Focus Areas
- tests/realtek_station_onair.sh[289-295]
- tests/realtek_station_onair.sh[330-335]
- examples/tx/main.cpp[2093-2099]
## Recommended Fix
Measure the submission floor from the first actual TX submission through final `tx.stats`, using timestamps in the report timebase. Exclude bring-up time, but include any delay between the first submission and first report.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~Later cells run on an adapter that was never reset~~ ✓ Resolved

Description

When reap() KILLs a child that ignored TERM, cleanup() skips the authorized toggle on
AP_SYSFS. With CELLS=all that cleanup also runs between cells, so the next cell starts on an unreset
adapter with only an echo.

Code

tests/mt7612u_ap_onair.sh[R112-113]

+  if [ "$reaped" != 0 ]; then
+    echo "a process outlived TERM - not re-enumerating AP_SYSFS=$AP_SYSFS"

Evidence

The file's own cleanup comment says the beacon is autonomous: it survives the host process being
killed and only the toggle stops it. The PR adds the skip whenever reap returns 1, and cells are
chained with cleanup between them, so the run carries on.

tests/mt7612u_ap_onair.sh[93-102]
tests/mt7612u_ap_onair.sh[339-339]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When reap() had to KILL a child, cleanup() skips re-enumerating AP_SYSFS. Under CELLS=all, the run then continues into the next cell even though the killed cell's autonomous beacon may still be airing. The next cell's checks are scored against that state.
## Fix Focus Areas
- tests/mt7612u_ap_onair.sh[68-86]
- tests/mt7612u_ap_onair.sh[112-114]
## Recommended Fix
Pick one:
- In cleanup, when reaped != 0, mark the run so that later cells are not scored, or exit with an inconclusive status instead of continuing.
- Or, after the KILL, poll until the PIDs are gone (sta_pid_alive) and then re-enumerate as usual. A KILLed process cannot still be in de-init.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. ~~Legacy RX fork child still aborts on exit~~ ✓ Resolved

Description

In the fork child, in_fork_child makes ~DrainerJoin skip the join, but the child then does
return 1 from main with joinable copies of bulk_in_thread and intr_in_thread. With
DEVOURER_DRAIN_BULK_IN or DEVOURER_POLL_INTR_IN set, their destructors call std::terminate, so the
child aborts instead of exiting with 1.

Code

examples/tx/main.cpp[R1032-1034]

+#if !defined(_MSC_VER) /* fork() is a real fork here, not the (0) stub */
+      drainers.in_fork_child = true;
+#endif

Evidence

The thread objects are declared before the guard, and the guard skips the join in the child. The
child exits through return 1, which runs normal destructors, and a destroyed joinable std::thread
terminates the process.

examples/tx/main.cpp[803-807]
examples/tx/main.cpp[1031-1042]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The DEVOURER_TX_WITH_RX fork child returns from main while it still holds joinable std::thread copies of the IN drainers. Their destructors call std::terminate.
## Fix Focus Areas
- examples/tx/main.cpp[1031-1042]
## Recommended Fix
After rtlDevice->Init(...) in the fork child, call _exit(1) instead of `return 1`, so no destructors run on the copied thread objects.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Tip of the day

💡 Did you know, you can copy the agent prompt from any finding and feed it to your IDE agent

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

The floor's aired_ms started at the arm's FIRST tx.report. A transmitter that started, stalled, and then burst 50 reports in its last second met MIN_REPORTS and a ~50-frame floor, where the old fixed 500 would have refused it (qodo, PR OpenIPC#466). txdemo's tx.frame now carries t, the same host-monotonic timebase as tx.report and the final tx.stats. The first tx.frame follows the first submit, so a harness can date it. docs/logging.md lists the field. Existing consumers match on "n" and "rc" or parse JSON, so the extra trailing field changes nothing for them. summarize uses that first-submit time in two places: - aired_ms now runs from the first submit to the final tx.stats. - Liveness gains lead_ms, the silence from the first submit to the first report. live=0 when it exceeds MAX_GAP_MS, or when no tx.frame carries t. The bring-up stays outside the window either way, because the first submit follows InitWrite. Synthetic JSONL through the script's own summarize/usable: | Fixture | Result | |---|---| | slow bring-up, 327 frames in 2.0 s | usable, floor 101 | | start, stall, 60 reports in the last second | refused: live=0 on lead_ms 8800, and 60 < floor 490 | | rate stall | refused, floor 500 | | hard stall | refused, live=0 | | healthy full window | usable | | no tx.frame t | refused, live=0 | Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

When reap() had to KILL a child, cleanup rightly skipped the authorized toggle. Under CELLS=all, though, that cleanup also runs between cells, and the run carried on into the next cell on an adapter that was never reset and whose autonomous beacon could still be airing (qodo, PR OpenIPC#466). cleanup now returns 1 when it could not reset the AP. In the CELLS=all sequence, the cells after such a cleanup are not run; they are listed as NOT RUN, and the run exits 2 (INCONCLUSIVE) unless a check already failed (1). It never scores those cells. A single-cell run is unchanged. The exit status is otherwise as before: 0, or 1 on a failed check. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

The legacy DEVOURER_TX_WITH_RX fork child returned from main holding copies of the parent's objects, including the IN drainers' std::threads. Those are joinable copies of threads that do not exist in the child, so ~std::thread terminated it, and the DrainerJoin guard's in_fork_child skip could not prevent that (qodo, PR OpenIPC#466). The child now follows the standard post-fork rule: - It runs Init inside a try, so an exception does not unwind it either. - It flushes stdio. - It leaves through std::_Exit(1), so no destructor runs in the child. That is the child's only exit path. The in_fork_child flag is gone, since the child never reaches the guard's destructor. On MSVC fork() is the (0) stub, which makes that branch the only process, so it keeps its normal return and teardown. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNC8xhn1rNCi5t6uLvE6M3

josephnef

Reviewed at 18f0a20. Read every hunk; built the branch with -DDEVOURER_MT7612U=ON (ctest 81/81 run, 2 skipped without reference/); ran two harnesses on hardware, plus a master A/B of one of them. Approving - the notes below are non-blocking; the first one is worth a small fix in the squash if you agree.

Code

The scope guards in txdemo hold: drainers is declared after session and before the device, io_threads after both and before tx_beacon/sta_clear_join, so every early return joins while the handle, device and context are alive, and the normal path's explicit join()s leave the destructors as no-ops. The fork child's _Exit guard matches the fork() stub guard exactly. The TxBeaconGuard retry is right for the Jaguar2/3 contract (false after a true arm = refused disable). The three Stop()s write _station_ready under the same lock the SetStationIdentity gate reads it. mt_mac_stop after a failed mt_mac_start is safe on both failure paths (the RING refusal writes nothing; the WPDMA path has set ENABLE_TX). All in-tree tx.frame consumers match on "ev"/"n"/"rc", so the trailing t is harmless.

Hardware (this bench, ch6, near field)

realtek_station_onair.sh, DUT 8812CU at 3-2.4, peer 8812BU (CF-924AC) at 4-2.3.3, AP = MT7612U on mt76x2u at 10-1.3: 5 passed, 0 failed, 0 inconclusive, rc 0. The new floor did exactly what it is for: the 8812BU peer spent ~8.5 s of the 10 s window in InitWrite and aired 327 frames over aired_ms=1475; min_submitted came out 73 (old fixed floor: 500, refused). Every arm live=1, lead_ms 1-63.

One oddity, harmless to the verdict: arm C read tail_ms=-6 - the final tx.stats t landed 6 ms before the last tx.report t. Presumably the final stats are stamped before the last C2H decodes. live is unaffected (negative ≤ MAX_GAP), but a reader of the summary line will wonder; clamping at 0 or noting it in the summarize comment would do.

mt7612u_sta_uplink.sh, DUT MT7612U at 10-1.3, responder 8812BU, default FRAMES=60 / RETRY_LIMIT=15, run on this branch and then on master (927cfc7) with the same adapters:

The multi-entry loss you flag as open is reproduced here and is much worse than your 48/53: the unicast-to-peer arms (b-e) land 0-11/60 own entries on BOTH passes, with ~10 entries per arm carrying the previous arm's pktid, and that pattern is identical on master. So: pre-existing, not this PR, and your "a deficit larger than one entry is not this race" holds on a second unit. Both runs INCONCLUSIVE/FAIL for that reason; both adapters handed back cleanly each time.
The stale-EXT claim fired where it should: the session's first arm reads 60/60, 1 stale-EXT entries claimed on this branch where master reads 59/60 UNSETTLED, 1 foreign. That also hardware-runs the "last arm that sent" form (no arm had every submit fail, so it coincides with the previous-arm form here).
One over-claim: h broadcast, wcid1 control (receiver OFF, arm A) read 61/60, 1 stale-EXT claimed, 11 foreign - the arm then received all 60 own entries, so the claimed one was not ours (or the MAC filed a duplicate). It is printed rather than clipped, as designed, but it inflates success by one. Cheap self-correction: track own-pktid entries separately and, at arm end, if own == n and stale_ext == 1, move the claimed entry back to late_prev. That makes the one case that can prove the claim wrong correct itself.
The stale_settled gate means one UNSETTLED arm disables the claim for every following arm until one settles on its own: in run B that was a chain of seven consecutive 59/60 arms, each with "1 late entry from the previous arm". Conservative and correct by the documented rule, but worth one sentence in docs/mt7612u-tx-retry.md: on a unit where arms do not settle, the claim buys almost nothing.

Harness nits

mt7612u_sta_identity.sh: an overrun gate sets r_bss=2, but the verdict case has only 0, 3, *, so the operator reads "did not pass" for what the exit code calls INCONCLUSIVE. Add a 2) branch.
mt7612u_sta_uplink.sh: the new dut_bound is per harness arm (552 s at FRAMES=60, 1128 s at 200) while the header's 25-minute figure is for both arms, so the margin for the slower un-ACKed arm at FRAMES=200 is thin (~15% if arm B is ~16 min). Fine at the default; maybe size it from the measured figure rather than the ladder.
mt7612u_ap_onair.sh: on the KILL path reap drops KIDS without waiting the killed children, so they stay zombies for the rest of the run. Harmless, a wait is free.
A #465-class item for the follow-up list: on a host whose MediaTek firmware is zstd-compressed (/lib/firmware/mediatek/*.bin.zst, this one), sta_fw_link succeeds, the probe logs cannot open firmware/mt7662_rom_patch.bin, and the harness scores it as ABORTED the DUT did not confirm retry limit 15: with an empty reason. A preflight that checks the two blobs are readable through the link would turn a dead rig into a refusal in seconds.

snokvist and others added 15 commits October 3, 2026 10:52

qodo-free-for-open-source-projects Bot reviewed Oct 3, 2026

View reviewed changes

Comment thread tests/realtek_station_onair.sh Outdated

Comment thread tests/mt7612u_ap_onair.sh

Comment thread examples/tx/main.cpp Outdated

snokvist and others added 3 commits October 3, 2026 11:55

josephnef approved these changes Oct 3, 2026

View reviewed changes

josephnef merged commit 1134df9 into OpenIPC:master Oct 3, 2026
30 of 31 checks passed

josephnef mentioned this pull request Oct 3, 2026

Follow-ups left open by the station-series cleanup (#466) #467

Open

Navigation Menu

Uh oh!

cleanup: follow-ups from the station series reviews (#451-#464), part of #465 - #466

cleanup: follow-ups from the station series reviews (#451-#464), part of #465#466
josephnef merged 18 commits into
OpenIPC:masterfrom
snokvist:cleanup/followups

snokvist commented Oct 3, 2026 •

edited

Loading

Uh oh!

snokvist commented Oct 3, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Oct 3, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Oct 3, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josephnef left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

snokvist commented Oct 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Maintainer review notes

#461: the txs gate's lost status entry

#465 (partial): the harness defect sweep

Our own finding: txdemo thread joins

qodo review of this PR

Why

Measured

What it can't do

Follow-ups

Verification

Blast radius

Uh oh!

snokvist commented Oct 3, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Oct 3, 2026

PR Summary by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot commented Oct 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josephnef left a comment

Choose a reason for hiding this comment

Code

Hardware (this bench, ch6, near field)

Harness nits

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

snokvist commented Oct 3, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Oct 3, 2026 •

edited

Loading