sensing: pin the busy window's two asymmetries, and three stale doc facts - #439
Merged
Merged
Conversation
…acts OpenIPC#437 landed the armed busy window. Two of its rules are asymmetric, and the suite passes with either one inverted — so they are currently held by comments rather than by tests. Verified by mutation against this tree, before the tests below. Each mutation is caught by exactly one new assertion: - ClmWindow::read consumes a SPOILED window but keeps a NOT-ELAPSED one. Deleting the `armed_ = false` from the spoil branch — the tidy-looking "consume once, at the top" simplification — makes every existing check in busy_window_selftest.cpp pass. Consuming is what hands the caller back to the sampled path: IRtlRadio::GetChannelBusy takes the armed branch on armed(), and `spoil_` is sticky (only arm() clears it), so a spoiled window left armed strands every later call there returning invalid-Retuned, with the sampled fallback unreachable for every unrelated consumer until something re-arms or resets it. Not a wrong number — a sensor that stops answering. - busy_from_ch_time_window reports a lost window only when one was ARMED. Dropping the `w.armed &&` makes every existing check in channel_busy_selftest.cpp pass, while telling every sampled caller that its window was lost when it never armed one. "No sensor here" and "your window died" are the two facts the spoil field exists to separate. A third, smaller gap closes with them: the existing retune block reads `w.last_spoil()`, so a branch reporting every spoil as Interrupted passes the whole suite. What a consumer acts on is `ChannelBusy::spoil`, so that is what the new block asserts. Three documentation facts that OpenIPC#437 overtook: - AdapterCaps.h names Jaguar1 and the MT7612U as the false-as-unmeasured cases for busy_airtime_measured. OpenIPC#437 measured both and flipped them; all four backends that set busy_airtime_ok now set measured too. It also cited tests/ccx_clm_probe.sh, which separates a CARRIER from a quiet channel on the sampled read — the armed window's harness is tests/busy_window_probe.sh. Worst place in the tree to be stale, since it is where the flags are defined. - The portable-surface table still labels the MT7612U busy-airtime leg "unvalidated", which the same page contradicts two sections later. No production code changes. Verified: 69/69 ctest, ASan/UBSan clean on both selftests. As a no-change control — this commit touches no production code, so the run re-verifies OpenIPC#437's — tests/busy_window_probe.sh PASS on an 8812AU sensor against an MT7612U flooder on ch165, steady (armed 71%, spread 0 against the sampled path's 8) and bursty 50/450 (armed mean 9% with the true duty ~9%; the sampled read missed the load 5/6 and spread 70 against the armed window's 19). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SbJ75HT4fqof7e5e8NKL2j
PR Summary by QodoPin busy-window asymmetries and refresh sensing docs
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history |
josephnef
approved these changes
Sep 20, 2026
josephnef
left a comment
josephnef
left a comment
Collaborator
There was a problem hiding this comment.
Reproduced all three mutation claims locally in an ASan+UBSan build. Each mutation fails exactly the one new assertion the description names and nothing else:
| mutation | only failing check |
|---|---|
keep a spoiled window armed (drop armed_ = false in the spoil branch) |
spoiled: window consumed |
hardcode the reason to Interrupted |
spoiled: with its reason (got 1 want 2) |
drop w.armed && at RxSense.h:319 |
mt unarmed over dead counters: and no window to have lost |
The three doc facts check out against the tree. Approving; Qodo answered as an issue comment rather than a review, so the gate needs the skip-qodo-gate label.
One heads-up for #440: this PR and #440 both edit the same "today" row of docs/rx-spectrum-sensing.md, not only the AdapterCaps.h paragraph — whichever lands second needs a real rebase there.
josephnef
added
the
skip-qodo-gate
josephnef
added a commit
that referenced
this pull request
Sep 20, 2026
The RTL8733B has a working CCX CLM engine. #437's caps note recorded that and left the port as a follow-up; this is it. ## Where the register block comes from In this tree: `hal/hal8733b_tables.c:792` onward — the shipped 8733B phy_reg init table — programs `0x1e40`, `0x1e44`, `0x1e48`, `0x1e5c` and `0x1e60` at bring-up. That is the CCX block at the JGR3 addresses, on this die, checkable without leaving the repo. The vendor agrees: at the pinned `reference/rtl8733bu-20230626`, `hal/phydm/phydm_pre_define.h:513` lists `ODM_RTL8733B` in `PHYDM_IC_SUPPORT_IFS_CLM`, and `:523-525` define `PHYDM_IC_JGR3_SERIES_SUPPORT` when `RTL8733B_SUPPORT` is set. That init table is also the **only other writer** of those registers — nothing in `src/rtl8733b/` touches them at runtime — so the masked read-modify-writes cannot corrupt another subsystem's state, and `arm_clm_only` never touches `0x1e5c`, where the table leaves a non-zero value. `with_ccx()` lends the engine `nhm_regs_jgr3()` under `_reg_mu` then the CCX mutex, the way Jaguar2/3 do. No new machinery. ## Busy airtime without a phydm counter block `arm_clm_only`/`read_clm_only` take no IGI argument — busy airtime is a hardware tick count, not a histogram referenced to the receiver's own noise floor the way NHM's thresholds are. So CLM ports cleanly to a backend that implements no `GetRxEnergy`. **This is the first backend where the two caps split**: `busy_airtime_ok` true, `rx_energy_ok` false. The sampled path still reports *no reading* here; an armed window is the only way a number comes out. `AdapterCaps.h`'s `busy_airtime_ok` definition is amended accordingly — it promised that `GetChannelBusy` answers, which on this die is true only after `ArmChannelBusy`. ## Measured `tests/busy_window_probe.sh`, RTL8733BU sensor against an MT7612U flooder on ch165: | arm | result | |---|---| | quiet | 0.0% | | steady load | **69%**, spread 0 over 6 windows | | bursty 50/450 ms | mean 9-10%, true duty ~9% | | retune mid-window | refused, `spoil=retuned` | | read before elapsed | refused, `spoil=not-elapsed` | | re-arm then read | never returns the previous latched value | | sensor transmitting | valid, `own_tx` flagged | A Jaguar3 8812CU on the same flooder read **69%** too, so the two device paths onto the JGR3 map agree on one load. `busy_airtime_measured` is set on that evidence, per the house rule. The `Interrupted` spoiler is unreachable on this die by construction: with no NHM read, nothing can re-arm the shared engine mid-window. The CCA gate cannot bias the reading either — `SetCcaMode` throws on this backend, so a CCA-disabled session is unreachable rather than merely unlikely. ## Known gap, pre-existing, with one affected consumer `src/sensing/SenseWindow.h` picks its source by whether the `IRtlRadio*` is non-null rather than by `rx_energy_ok`, so on this backend it takes the phydm branch, gets the all-invalid base `GetRxEnergy`, and never calls `GetChannelBusy()`. That is the same wrong discriminator `AdapterCaps.h` opens by warning about. **`examples/chanscout` hits it.** `main.cpp:323` builds an `IRtlRadio*` by `dynamic_cast`, which succeeds on this die, and hands it to a `DwellExecutor` at `:342` — so a scout on an RTL8733B reports neither CLM nor NHM. *(An earlier revision of this description claimed nothing constructs `SenseWindow` outside tests. That was false, and it was the premise this section rested on. Corrected.)* Gating that branch on the capability is **necessary but not sufficient**: an unarmed `GetChannelBusy()` falls back to `busy_from_rx_energy(GetRxEnergy(true))` and yields nothing here, and nothing in `src/sensing/` ever calls `ArmChannelBusy` — the only callers in the tree are `tests/busy_window_probe.cpp`. A complete fix is the gate **plus an arm**, and arming needs an observation window that no layer between `ScanPlanConfig::dwell_ms` and `SenseWindow` carries. It would also change behaviour on four other backends, where an arm interacts with the NHM read that spoils the same engine — which is what `tests/dwell_exec_selftest.cpp` exists to pin. So this is a real gap with a real consumer, and I do not think it belongs in a backend port: the fix is a `src/sensing/` design change needing its own tests and its own review. The defect is pre-existing — on master the same die already produces the same nothing — and leaving `busy_airtime_ok` false to mask it would be a lie about the silicon in a header whose whole point is that the flag describes hardware. Documented in `docs/rx-spectrum-sensing.md` and `src/rtl8733b/CLAUDE.md`. Say the word if you would rather have it here. **One thing for out-of-tree consumers:** `busy_airtime_ok`'s definition changes meaning slightly — it now says the backend *has* an engine and explicitly does not promise an unarmed `GetChannelBusy()` answers. Anything that sampled on the strength of that flag must handle "no reading" from a backend whose flag is true. ## Locking Both retune notes are scoped. `FastRetune` calls `SetMonitorChannel` on its declined path while already holding `_reg_mu` — recursive, so that part is fine — and the CCX mutex is **not** recursive, so holding it across the tune would self-deadlock there. One note per site suffices because `_reg_mu` spans the whole tune and `with_ccx` takes `_reg_mu` first, so a concurrent arm cannot interleave: the Jaguar2/3 situation, not Jaguar1's. The `Init`/`InitWrite` window resets sit **inside** `_reg_mu` for the same reason — ahead of it, an arm landing between the released CCX lock and `_reg_mu` would survive the bring-up the reset exists to forget. `with_ccx` reads `_phy_ready` under `_reg_mu` rather than before it, which the Jaguar2/3 overrides do not: it is a plain bool written under that lock, so the earlier ordering was a data race and a TOCTOU against a concurrent `Stop()`. ## Harness `busy_window_probe.sh` gated its NHM and sampled arms on the sensor's USB VID, which calls this chip Realtek and then runs three arms it cannot pass. It now gates on `rx_energy_ok` from the probe's own caps record — the capability, not the identity, which is the discriminator `AdapterCaps.h` argues for. A caps value it cannot parse is a **loud failure, not a silent skip**: defaulting to "no GetRxEnergy" would switch off the sampled arm, all three NHM arms and the bursty comparison while still printing PASS. The verdict line now carries the skip count for the same reason. Also fixed: `expect_all` printed "ok" unconditionally, so a failing arm reported its FAILs and then said ok underneath them. ## Verification ctest 69/69 (67 run, 2 MediaTek generator tests skipped), ASan/UBSan clean on both selftests, and five on-air runs: | run | result | |---|---| | RTL8733BU steady | PASS — 69%, spread 0, 4 arms skipped | | RTL8733BU bursty 50/450 | PASS — mean 10% vs ~9% true duty, 4 arms + 1 check skipped | | Jaguar3 8812CU | PASS — every arm ran, **zero skips**, 69% | | MT7612U sensor vs 8812CU flooder | PASS — quiet arms, 7 arms skipped | | caps-field rename mutation | **fails loudly** where it previously printed PASS | The `Stop()` window reset is compile- and reasoning-verified rather than re-measured: its triggering sequence — a `Stop` followed by a retune with no re-`Init` — is out of contract, and there is no mock seam for the device class. Lock order and re-entrancy for it were checked against every `busy_window_mutex()` site in the tree. Note: #439 edits the adjacent `busy_airtime_measured` paragraph in the same `AdapterCaps.h` comment block. Whichever lands second needs a trivial rebase. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01SbJ75HT4fqof7e5e8NKL2j --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Joseph <162703152+josephnef@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #437. No production code — two regression tests and three
documentation facts that PR overtook.
The two rules it pins
#437's busy window has two asymmetries that decide whether a spoiled reading
refuses or lies. Both are currently held by comments only: I mutated this tree
and the suite passes with either one inverted.
1.
ClmWindow::readconsumes a SPOILED window but keeps a NOT-ELAPSED one.Deleting the
armed_ = falsefrom the spoil branch — the tidy "consume once,at the top" simplification — passes every existing check in
busy_window_selftest.cpp.Consuming is what hands the caller back to the sampled path.
IRtlRadio::GetChannelBusytakes the armed branch onarmed(), andspoil_is sticky (only
arm()clears it), so a spoiled window left armed strandsevery later call there returning invalid-Retuned — the sampled fallback
unreachable for every unrelated consumer until something re-arms or resets.
Not a wrong number: a sensor that stops answering.
2.
busy_from_ch_time_windowreports a lost window only when one was ARMED.Dropping the
w.armed &&passes every existing check inchannel_busy_selftest.cpp, while telling every sampled caller its window waslost when it never armed one. "No sensor here" and "your window died" are the
two facts the
spoilfield exists to separate.A third, smaller gap closes with them: the existing retune block asserts on
w.last_spoil(), so a branch reporting every spoil asInterruptedpassesthe whole suite. What a consumer acts on is
ChannelBusy::spoil.Each mutation is caught by exactly one new assertion:
spoiled: window consumedInterruptedspoiled: with its reason(got 1 want 2)w.armed &&and no window to have lostI deleted a fourth assertion I had written — it could not fail while the line
above it passed, because
read()returns at the!armed_guard beforetouching the mock.
The three stale facts
AdapterCaps.hnames Jaguar1 and the MT7612U as the false-as-unmeasuredcases for
busy_airtime_measured. sensing: a busy-airtime window the caller defines #437 measured both and flipped them — allfour backends that set
busy_airtime_oknow setmeasuredtoo. It alsocited
tests/ccx_clm_probe.sh, which separates a carrier from a quietchannel on the sampled read; the armed window's harness is
tests/busy_window_probe.sh. This is the worst place in the tree to bestale, since it is where the flags are defined.
docs/rx-spectrum-sensing.md:130still labels the MT7612U busy-airtimeleg "unvalidated", which the same page contradicts two sections later and
the backend contradicts with
busy_airtime_measured = true.Verification
69/69 ctest, ASan/UBSan clean on both selftests.
As a no-change control — this commit touches no production code, so the
run re-verifies #437's —
tests/busy_window_probe.shPASS on an 8812AU sensoragainst an MT7612U flooder on ch165:
🤖 Generated with Claude Code
https://claude.ai/code/session_01SbJ75HT4fqof7e5e8NKL2j