sensing: port the CCX CLM busy window to the RTL8733B - #440
Conversation
PR Summary by QodoPort RTL8733B CCX CLM busy-window sensing
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
b55a21b to
c444f17
Compare
|
Addressed all four findings; pushed 2 — sensing surveys miss busy airtime. Real, and the sharp part of the finding is right: changing only the discriminator would not recover it, because an unarmed It was also worse than I wrote. The description claimed "nothing constructs I have still left the fix out of this PR, with the reasoning now stated honestly rather than resting on a false premise: the complete fix is the capability gate plus an arm plus an observation window no layer carries, and it changes behaviour on four other backends where an arm interacts with the NHM read that spoils the same engine. Happy to do it here if you would rather. 3 — restart reports a stale spoiler. Real, and my earlier reasoning was wrong: I had concluded 4 — skip counts. Real, and my own comment said "skips four arms" while the code counted 2. Each branch now states how many arms it bypasses: NHM three, sampled one, MediaTek exit seven (my first attempt said six — it omitted 1 — chip guide duplication. Partly. The rule is real (root Re-verified on air after the changes: RTL8733BU steady and bursty PASS, 8812CU PASS with every arm running and zero skips, MT7612U quiet arms PASS, and the caps-gate mutation still fails loudly. The |
c444f17 to
686a0b4
Compare
josephnef
left a comment
josephnef
left a comment
There was a problem hiding this comment.
Re-validated on the bench (no MT7612U plugged, so the flooder is an RTL8812CU txdemo), all through tests/busy_window_probe.sh at 686a0b4:
| run | result |
|---|---|
| RTL8733BU sensor, steady, ch165 | quiet 0%, loaded 62-63% (spread 1), retune → retuned, early → not-elapsed, stale re-arm never leaks, own-TX flagged. PASS, 4 arms skipped |
| RTL8733BU sensor, bursty 50/450 ms | per-window 0 / 13 / 0 / 19 / 0, PASS |
| Jaguar3 8812CU sensor vs 8822BU flooder (harness control) | every arm ran, zero skips, armed spread 1 vs sampled spread 27. PASS |
ctest 69/69 in a clean worktree. Lock order (_reg_mu → CCX), the readiness flag read under the lock, and the Init/InitWrite/Stop resets all read correctly; I also confirmed the claim that none of the Jaguar Stop() paths resets the window.
Four small things, which I'll push onto this branch after rebasing over #439:
- Conflicts with #439 in
docs/rx-spectrum-sensing.md(the "today" row), not only inAdapterCaps.h. tests/channel_busy_selftest.cpp:33still says the RTL8733B has "no CCX engine".- The bursty figure ("10% vs true duty ~9%") reads as agreement it does not have: a 240 ms window inside a 500 ms burst period misses whole bursts, and my five reads spread 19 points around a 6% mean. The spread belongs beside the number.
- The
src/sensing/discriminator gap is a sensing-layer bug; the narrowest home issrc/sensing/CLAUDE.mdplus a follow-up issue, so the ~40-line copy insrc/rtl8733b/CLAUDE.mdshrinks to a pointer.
reports as absent. It does. Ported and measured. The register block is 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 — the CCX block at the JGR3 addresses. 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, 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 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; the armed window is the only way a number comes out. AdapterCaps.h's busy_airtime_ok definition is amended to say so — it promised that GetChannelBusy answers, which on this die is true only after ArmChannelBusy. An out-of-tree consumer that sampled on the strength of that flag must now handle "no reading" from a backend whose flag is true. 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 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 here reports neither CLM nor NHM. (An earlier draft of this message and of the PR body claimed nothing constructs SenseWindow outside tests. That was false; chanscout does.) 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. That belongs in src/sensing/ with its own tests, not in a backend port. Documented in docs/rx-spectrum-sensing.md and src/rtl8733b/CLAUDE.md. MEASURED, tests/busy_window_probe.sh, RTL8733BU sensor against an MT7612U flooder on ch165: quiet 0.0% steady load 69%, spread 0 over 6 windows bursty 50/450 ms mean 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 that 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 there is nothing that can re-arm the shared engine mid-window. The other two fire normally. The CCA gate cannot bias the reading either — SetCcaMode throws on this backend, so a CCA-disabled session is unreachable rather than merely unlikely. LOCKING AND LIFECYCLE. 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 resets sit INSIDE _reg_mu for the same reason. 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 read was a data race and a TOCTOU against a concurrent Stop(). Stop() forgets the window too, which is not obvious: a stop is not a retune, but clearing _phy_ready does not protect an armed window either, because SetMonitorChannel and FastRetune both call bring_up_to_phy(), which sets that flag true again. Without the reset, a window armed before a stop revives on the brought-up chip and the retune note hands back a Retuned spoil earned by a session that no longer exists. Jaguar1/2/3 share this hole and have it worse — none of their Stop() implementations resets the window, and none clears the flag their with_ccx gates on — noted in the chip guide so the asymmetry is not read as an 8733B quirk. 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 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. Each skip branch now states how many arms it bypasses (the NHM branch skips three, not one; the MediaTek exit seven, not one) and the bursty comparison is counted as a check rather than an arm, so the verdict's number means "arms that did not run". Also fixed: expect_all printed "ok" unconditionally, so a failing arm reported its FAILs and then said ok underneath them. Verified: ctest 69/69 (67 run, 2 MediaTek generator tests skipped), ASan/UBSan clean on both selftests, and five on-air runs — RTL8733BU steady (PASS, 4 arms skipped) and bursty (PASS, 4 arms + 1 check), Jaguar3 8812CU PASS with every arm running and zero skips (so the gate discriminates rather than disabling), MT7612U sensor against an 8812CU flooder PASS on the quiet arms (7 arms skipped), and a mutation renaming the caps field fails loudly where it would previously have printed PASS. The Stop() 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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SbJ75HT4fqof7e5e8NKL2j
The 50/450 ms burst figure now carries its 19-point spread: a 240 ms window inside a 500 ms burst period misses whole bursts, so only the mean is a measurement. The SenseWindow discriminator gap is a sensing-layer bug and now lives in src/sensing/CLAUDE.md, with the 8733B subtree and the sensing article pointing at it. The channel_busy selftest comment no longer calls the RTL8733B a die without a CCX engine. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
686a0b4 to
f3e05e8
Compare
2 participants
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:792onward — the shipped 8733B phy_reginit table — programs
0x1e40,0x1e44,0x1e48,0x1e5cand0x1e60atbring-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:513listsODM_RTL8733BinPHYDM_IC_SUPPORT_IFS_CLM, and:523-525definePHYDM_IC_JGR3_SERIES_SUPPORTwhenRTL8733B_SUPPORTis set.That init table is also the only other writer of those registers —
nothing in
src/rtl8733b/touches them at runtime — so the maskedread-modify-writes cannot corrupt another subsystem's state, and
arm_clm_onlynever touches0x1e5c, where the table leaves a non-zerovalue.
with_ccx()lends the enginenhm_regs_jgr3()under_reg_muthen the CCXmutex, the way Jaguar2/3 do. No new machinery.
Busy airtime without a phydm counter block
arm_clm_only/read_clm_onlytake no IGI argument — busy airtime is ahardware 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_oktrue,
rx_energy_okfalse. The sampled path still reports no reading here;an armed window is the only way a number comes out.
AdapterCaps.h'sbusy_airtime_okdefinition is amended accordingly — it promised thatGetChannelBusyanswers, which on this die is true only afterArmChannelBusy.Measured
tests/busy_window_probe.sh, RTL8733BU sensor against an MT7612U flooder onch165:
spoil=retunedspoil=not-elapsedown_txflaggedA Jaguar3 8812CU on the same flooder read 69% too, so the two device paths
onto the JGR3 map agree on one load.
busy_airtime_measuredis set on thatevidence, per the house rule.
The
Interruptedspoiler is unreachable on this die by construction: with noNHM read, nothing can re-arm the shared engine mid-window. The CCA gate cannot
bias the reading either —
SetCcaModethrows on this backend, so aCCA-disabled session is unreachable rather than merely unlikely.
Known gap, pre-existing, with one affected consumer
src/sensing/SenseWindow.hpicks its source by whether theIRtlRadio*isnon-null rather than by
rx_energy_ok, so on this backend it takes the phydmbranch, gets the all-invalid base
GetRxEnergy, and never callsGetChannelBusy(). That is the same wrong discriminatorAdapterCaps.hopensby warning about.
examples/chanscouthits it.main.cpp:323builds anIRtlRadio*bydynamic_cast, which succeeds on this die, and hands it to aDwellExecutorat
:342— so a scout on an RTL8733B reports neither CLM nor NHM.(An earlier revision of this description claimed nothing constructs
SenseWindowoutside tests. That was false, and it was the premise thissection rested on. Corrected.)
Gating that branch on the capability is necessary but not sufficient: an
unarmed
GetChannelBusy()falls back tobusy_from_rx_energy(GetRxEnergy(true))and yields nothing here, and nothingin
src/sensing/ever callsArmChannelBusy— the only callers in the treeare
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_msandSenseWindowcarries. It would also changebehaviour on four other backends, where an arm interacts with the NHM read
that spoils the same engine — which is what
tests/dwell_exec_selftest.cppexists 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 testsand its own review. The defect is pre-existing — on master the same die
already produces the same nothing — and leaving
busy_airtime_okfalse tomask 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.mdandsrc/rtl8733b/CLAUDE.md. Say the word if you would rather have it here.One thing for out-of-tree consumers:
busy_airtime_ok's definitionchanges meaning slightly — it now says the backend has an engine and
explicitly does not promise an unarmed
GetChannelBusy()answers. Anythingthat sampled on the strength of that flag must handle "no reading" from a
backend whose flag is true.
Locking
Both retune notes are scoped.
FastRetunecallsSetMonitorChannelon itsdeclined path while already holding
_reg_mu— recursive, so that part isfine — and the CCX mutex is not recursive, so holding it across the tune
would self-deadlock there. One note per site suffices because
_reg_muspansthe whole tune and
with_ccxtakes_reg_mufirst, so a concurrent armcannot interleave: the Jaguar2/3 situation, not Jaguar1's.
The
Init/InitWritewindow resets sit inside_reg_mufor the samereason — ahead of it, an arm landing between the released CCX lock and
_reg_muwould survive the bring-up the reset exists to forget.with_ccxreads
_phy_readyunder_reg_murather than before it, which the Jaguar2/3overrides 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.shgated 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_okfrom the probe's own caps record — the capability, notthe identity, which is the discriminator
AdapterCaps.hargues 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_allprinted"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:
The
Stop()window reset is compile- and reasoning-verified rather thanre-measured: its triggering sequence — a
Stopfollowed by a retune with nore-
Init— is out of contract, and there is no mock seam for the deviceclass. 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_measuredparagraph in the sameAdapterCaps.hcomment block. Whichever lands second needs a trivial rebase.🤖 Generated with Claude Code
https://claude.ai/code/session_01SbJ75HT4fqof7e5e8NKL2j