IRtlRadio: address the two carrier-sense gates separately - #427
Conversation
SetCcaMode is all-or-nothing, and on these families it is two gates doing different jobs: 0x520[14] primary CCA defers to a decodable preamble, 0x520[15] EDCCA defers to raw in-band energy. This adds SetCcaGates / GetCcaGates so a caller can address them one at a time, with not-ported defaults and SetCcaMode reduced to SetCcaGates(d, d). Why it is worth splitting: they do not behave the same way, and on Jaguar1 they behave OPPOSITELY to what tests/dis_cca_tx_onair.sh found. That test is Jaguar3 — it uses the 8812AU only as its flooder and has never run a Jaguar1 as the DUT. Running one (RTL8812AU injecting 300 broadcast frames at 6M on an otherwise idle ch6, fresh radio open per arm so nothing latches, two independent receivers, repeated): both gates on (the default) 0.0% 1.7% EDCCA off only 94.3% 94.7% primary CCA off only 13.7% 2.7% both off 94.3% 95.3% On Jaguar1 the energy bit is everything and the preamble bit is nearly null. Without the split there is no way to find that out, and no way to act on it except dis_cca, which turns off both. Acting on it matters, because leaving primary CCA on is strictly better than dis_cca. Same DUT against a saturating co-channel flooder: EDCCA off, primary CCA ON, no flooder 95.3% EDCCA off, primary CCA ON, flooding 78.0% both gates off, flooding 0.3% Carrier sense still defers, and both-off is worse for the injector's own delivery because it transmits into the flood and collides. Behaviour is unchanged for every existing caller. SetCcaMode's two states write exactly the bytes they wrote before: on Jaguar1 apply_cca(d, d) is the old body with the BB threshold half keyed on the EDCCA argument, and on Jaguar3 0x524[11] moves with the pair and only with the pair — what that bit does on its own is undocumented and unmeasured, so a mixed state leaves it at the enabled value rather than guessing. Jaguar3 tracks the two gates so SetMonitorChannel re-asserts what the caller asked for rather than only the all-or-nothing pair. Verified on hardware: RTL8812AU (Jaguar1) and RTL8822C (Jaguar3, an 8812CU). All four states read back from 0x520 on both, and the Jaguar3 state survives a retune. Defaults checked against a build without this change, alternating builds on the same bench: ambient RX rates, bridge drop counts and default-path TX delivery all inside the baseline-to-baseline spread, with the Jaguar3 injector at 100% on both. Not verified here: Jaguar2, Kestrel and RTL8733B have no hardware on this bench and are left on the not-ported default, as is MediaTek, which is not an IRtlRadio at all.
PR Summary by QodoControl primary CCA and EDCCA gates independently
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Reading 0x520 on a chip that was never brought up returns whatever the bus gives back, and reporting that as the gate state is a fabricated measurement; writing it pokes an uninitialised MAC. Jaguar3's SetCcaGates already guards on _brought_up — this is the Jaguar1 equivalent, on both the read and the write side. Found by review, then confirmed against an RTL8812AU: opened but not tuned, the op now refuses instead of answering.
josephnef
left a comment
josephnef
left a comment
There was a problem hiding this comment.
Verified locally on the head: builds clean, ctest 63/63. The API shape is right — not-ported defaults, SetCcaMode reduced to SetCcaGates(d, d), and the Jaguar1 byte-for-byte claim holds (apply_cca(d, d) is the old body with the BB 0x8a4 half keyed on the EDCCA argument). The Jaguar1 measurement is the valuable part of this PR and the inversion is worth acting on.
Three blocking items, all on Jaguar3, all invisible to a Jaguar1-only bench:
0x524[11]is not undocumented — it isBIT_EDCCA_MSK_CNTDOWN_ENand it is EDCCA-scoped, so it should followedcca_disabledrather than the pair.- phydm's
edcca_trackargument is still keyed on the all-or-nothing_cca_disabled, so the watchdog keeps tracking EDCCA in a mixed state.
Together those two mean the arm this PR recommends — EDCCA off, primary CCA on — is only half applied on Jaguar3. The on-air evidence here is all Jaguar1, where the path is correct, so the bench could not have caught it.
- The pre-bring-up contract diverges between the two backends, and
GetCcaGateson Jaguar3 has no_brought_upguard — the exact fabricated-gate-state hazard the Jaguar1 follow-up commit fixed.
Non-blocking: stickiness across SetMonitorChannel differs between the two backends and is undocumented (and Jaguar1 bring-up unconditionally re-runs SetCcaMode(_cfg.tuning.disable_cca), clobbering anything set before Init); there is no in-tree caller, env knob or regcheck script, so nothing in the repo reproduces the tables in the description — the tests/txpwr_*_regcheck.sh shape would cover the four gate states plus retune survival.
On the CLAUDE.md question: please scope it in this PR rather than a follow-up. The current text states the Jaguar3 result as general and it is now measured inverted on Jaguar1, and the new API is not mentioned in Configuration at all. Pair the Jaguar1 table with the flooder arm (95% → 78%, both-off 0.3%) in the same section — turning both gates off being worse for the injector is the half that keeps the first half from being read as "disable everything".
On Jaguar1's bring-up enabling EDCCA off its parked default with a hard-coded th_l2h_ini: agreed it is a policy question and out of scope here, but the 94% makes it a real one — worth an issue.
Two Jaguar3 defects from review, both invisible on a Jaguar1 bench because every on-air number in the original description came from a Jaguar1 DUT. 0x524[11] is BIT_EDCCA_MSK_CNTDOWN_EN (REG_RD_CTRL), the same name and bit on 8822B, 8822C and 8822E in the vendor HALMAC headers, so the meaning is family-stable. Being EDCCA-scoped it follows edcca_disabled alone. Moving it with the pair — on the reasoning that an unmeasured bit should not be guessed at — was the wrong kind of caution, and it left the EDCCA-off arm this work recommends with EDCCA still masking the backoff countdown. Measured on an 8812CU, exactly one of the six states changes, and SetCcaMode's two states write what they always wrote. phydm's edcca_track was keyed on the all-or-nothing flag, so with EDCCA off and primary CCA on the ~2 s tick went on running PhydmRuntimeJaguar3::edcca() and rewriting the BB thresholds at 0x84c. It now follows the EDCCA gate, the way Jaguar1 already did via SetEdccaTrack at the end of apply_cca. Sampling 0x84c cannot detect this — the tracker recomputes the same th_l2h from a static IGI, so an active tracker writes identical bytes — so it is measured by poking the register with a value it would never choose and seeing whether it is restored. Jaguar3's GetCcaGates and SetCcaGates now refuse before bring-up, caching nothing, as Jaguar1 already did. Reading 0x520 on an unconfigured MAC returned a plausible-looking state that depended on whatever the previous session left behind, and the setter reported success for a write that nothing replayed. _cca_disabled is removed: once both readers moved to the per-gate flags it was write-only, and a write-only field invites a future reader to assume it means something. The contract moves to the declaration in IRtlRadio.h, since FastRetune and SetMonitorChannel are the normal operating pattern for this lever: both calls are post-bring-up only, `false` means either "not ported" or "not brought up", a refusal leaves GetCcaGates' out-parameters alone, and the state survives a retune on both families — but by different mechanisms, so the Jaguar1 case is documented as incidental rather than guaranteed. That last point corrects the review: measured on an 8812AU, the gates and the BB thresholds are intact across a same-band retune, a band change, and FastRetune. Jaguar1 readability: the re-aliased `disabled` local is gone and the four uses name the gate, and the braceless same-line if/else pairs are braced. tests/cca_gates_probe.cpp is the in-tree caller the split lacked, and tests/cca_gates_regcheck.sh the register-level check, in the shape of the txpwr regchecks: the four gate states, the pre-bring-up refusal, both legacy SetCcaMode states, the 0x524[11] scoping, EDCCA-tracking shutdown, and retune survival by both channel paths — cross-checking the API's readback against a raw chipstate peek so an API that lies about the silicon fails rather than passes. 23 cells across an RTL8812AU, an RTL8822C and an RTL8733BU; reverting either fix above makes the matching cell fail. CLAUDE.md no longer states the Jaguar3 result as general. Both families are given as disagreeing measurements of different things, the flooder arm is paired in so the Jaguar1 figure cannot be read as "disable everything", and SetCcaGates is named in Configuration. The three roundings of one run are replaced by one citation of it. Verified on RTL8812AU (Jaguar1), RTL8822C (Jaguar3) and RTL8733BU (an IRtlRadio that ports neither, confirming the not-ported defaults refuse on real silicon). The 8822EU/8812EU arms of the original tables are unchanged and not re-run.
|
Thanks — both blocking Jaguar3 items were real, and you were right that the 1.
Exactly one row moves and it is the recommended arm, so the byte-identical My original comment called the bit undocumented. That was the wrong kind of 2.
3. Pre-bring-up contract aligned, not documented-as-divergent. Jaguar3's Worth noting what the unguarded read actually did, since it is less dramatic Non-blocking items: Stickiness — your assertion needs a correction, and it is in my favour, so In-tree caller and regcheck. I did not add an env knob — that is new public configuration
Jaguar1 nits applied: the Also removed: The EDCCA-at-bring-up policy question — Jaguar1 enabling EDCCA off its Devices actually verified for the above: RTL8812AU (Jaguar1), RTL8822C Pushed as |
josephnef
left a comment
josephnef
left a comment
There was a problem hiding this comment.
All three blocking items are fixed, and I checked the mechanism rather than the claim. 0x524[11] now follows edcca_disabled alone; both _phydm.tick sites pass !_cca_edcca_disabled; Jaguar3's SetCcaGates/GetCcaGates refuse before bring-up and the combined _cca_disabled is gone with no readers left behind. _brought_up = true precedes SetCcaMode(_cfg.tuning.disable_cca) on both families, so the config path still applies. Build clean, ctest 63/63.
I ran tests/cca_gates_regcheck.sh on my own bench, and it picked up an arm you said you could not cover:
0xa81a RtlJaguar3Device, chip-id 0x17 — 8822E 12 passed
0xc812 RtlJaguar3Device, chip-id 0x13 — 8822C 12 passed
0xb812 Jaguar2 8822BU not ported, refuses cleanly
0xb733 RTL8733BU not ported, refuses cleanly
35bc:0101 Kestrel C8852C not ported, refuses cleanly (rc=5)
== 25 passed, 0 failed, 0 skipped ==
cntdown reads 00:1 01:0 10:1 11:0 on both Jaguar3 dies — so 0x524[11] follows EDCCA alone on the 8822E as well as the 8822C, and the two SetCcaMode states still bracket it. The track cell passes on both. I have no Jaguar1 plugged, so that family is still only your bench.
The correction on stickiness is a good catch and I was wrong: I asserted a mechanism where you measured an outcome. The header's wording — survives on both, means it on only one, don't build on the Jaguar1 case — is the right shape.
No blocking items. Seven things worth a pass before merge, none of which need another review round.
1. Jaguar1 apply_cca clears the tracker last, so a mid-session EDCCA-disable can race the watchdog. src/jaguar1/RtlJaguarDevice.cpp parks 0x8a4 at 0x7f7f and then calls SetEdccaTrack(false). PhydmWatchdog is started in rtw_hal_init, i.e. before bring-up's SetCcaMode, and its tick rewrites both 0x8a4 bytes from the IGI-derived L2H while _edcca_track is still set. A tick landing in that window leaves live thresholds behind the disable the caller just asked for — the same defect as round-one finding #2, reached through a window instead of a wrong argument. It is inert at bring-up and gated on DEVOURER_PHYDM_WATCHDOG=1, but SetCcaGates is the first API that makes this a runtime call. Hoisting SetEdccaTrack(false) above the park (and leaving SetEdccaTrack(true) after the enable programming) closes it.
Related: Jaguar1's SetCcaGates/GetCcaGates take no lock while Jaguar3's take _reg_mu, and the 0x520/0x8a4 read-modify-writes share the bus with that same watchdog thread.
2. The track cell hard-codes 0x84c, which is Jaguar3's register — Jaguar1's is 0x8a4. On a Jaguar1 DUT the cell pokes an unrelated BB register, sees no restore, and prints no EDCCA tracker running in the default arm, which is false: PhydmWatchdog::SetEdccaTrack drives 0x8a4. The EDCCA-off arm then reports SKIP: no tracker to stop in this configuration. I suspect that is the 1 skipped in your 23/0/1, which would mean the functional half of fix #2 is unverified on the family whose measurement motivated the PR. Keying the address off the family turns that skip into a real cell — and a harness that quietly applies the Jaguar3 register to Jaguar1 is the exact generalisation this PR is about not making.
3. tests/cca_gates_regcheck.sh:221 — elif true; then makes the else at 231-233 unreachable, and that dead note duplicates line 217 verbatim. Plain else.
4. The cntdown cell disappears silently when fewer than four arms report. The [ ... -eq 4 ] at line 204 has no else, so a single failed arm drops the whole cell with no PASS/FAIL/SKIP and the counters simply do not move. A skip in the else keeps the ledger honest about what was not measured.
5. stop_hold cannot stop the probe. start_hold backgrounds sudo -n "$BUILD/CcaGatesProbe", so $! is a root-owned process; run as documented (sudo -v && tests/cca_gates_regcheck.sh) the plain kill "$probe_pid" gets EPERM and is swallowed by 2>/dev/null, and wait then blocks for the probe's entire hold walk. That is why a run is minutes per adapter — the two Jaguar3 cells took ~25 minutes here. sudo -n kill fixes it. The EXIT trap is fine, its pkill is already under sudo -n.
6. The retune cells can only see half the gate state. GetCcaGates reads 0x520 alone, so retune/fast-retune cannot detect loss of 0x524[11] or of the BB thresholds. IRtlRadio.h claims survival "at 0x520, 0x524 and Jaguar1's BB thresholds"; peek32 is already there, so one extra read after the retune would put the 0x524 half of that claim in the harness instead of in the prose.
7. Two smaller ones. src/jaguar2/RtlJaguar2Device.cpp:1932 still says "The primary-CCA bit is the one that stops TX deferring to a co-channel transmitter" on a family with no measurement of its own — the last unscoped copy of the claim you just scoped in CLAUDE.md, one file over. And in CLAUDE.md the new text now runs straight into On by default on the streamtx FPV downlink, which lands immediately after the SetCcaGates sentence and reads as though it describes the new API rather than DEVOURER_DIS_CCA; re-anchoring it on the env var restores the referent.
On the env knob: I would leave it out, as you did. DEVOURER_DIS_CCA covers the shipped default and the split has an in-tree caller now; adding two more vars before anything in the tree wants them is surface for its own sake.
Scoping the Jaguar3 dis_cca figure as the DUT's host-side submitted rate, next to the Jaguar1 numbers that are decoded delivery from two witnesses, is the part of this description I would keep verbatim — the two results only stop looking contradictory once the metrics are named.
Approving. The list above is all follow-up-sized; #1 and #2 are the two I would do before merge, since both are the same bug class the first round found.
…to fail
Review follow-ups on the gate split. The first is a real defect of the same
class the split already fixed once; the rest are the harness not being able
to catch it.
Jaguar1 apply_cca cleared the EDCCA tracker AFTER parking 0x8a4. The phydm
watchdog owns that register while tracking and runs on its own thread from
rtw_hal_init, so a tick landing between the park write and the flag store
re-derived L2H from IGI and overwrote the park — live thresholds behind a
disable the caller had asked for. Harmless while the gates could only be set
at bring-up; SetCcaGates is what makes it a mid-session call.
Reordering alone only narrows the window, so SetEdccaTrack is now
synchronous: the tick's EDCCA block and the flag store share a mutex, and
turning tracking off also drops the write-on-change cache, since the register
no longer holds the value that cache names. apply_cca stops the tracker
before it writes and hands the register back only after programming it.
The harness could not have found that, because its track cell poked 0x84c on
every family. That is Jaguar3's threshold register; Jaguar1's is 0x8a4. On a
Jaguar1 DUT the cell poked something unrelated, saw no restore, and reported
"no EDCCA tracker running" — a false negative on the family whose measurement
motivated the split, and the one skipped cell in the original run. The probe
now reports its generation from AdapterCaps and the cell picks the register
from that, skipping a generation it has no address for rather than guessing.
Also in the harness:
- The retune cells only ever saw 0x520, because that is all GetCcaGates
reads. They now peek 0x524[11] while the state is held. Negative control
on an 8812CU with the countdown write deliberately broken: the API still
reported ret=1 primary=1 edcca=0 while 0x524[11] read 0, so this is the
half of the claim the API cannot speak for.
- stop_hold could not stop anything: the probe is started through sudo, so
a plain kill from an unprivileged shell got EPERM silently and wait then
blocked for the probe's whole hold walk. Stops go through sudo now.
- `elif true` made the cntdown cell's else unreachable, and the cell
vanished with no verdict at all when fewer than four arms reported.
Two scoping fixes. RtlJaguar2Device.cpp still asserted that primary CCA is
the bit that stops an injector, on a family with no measurement of its own —
the last unscoped copy of the claim this work scoped everywhere else. And in
CLAUDE.md "on by default on the streamtx FPV downlink" had come to sit after
the SetCcaGates sentence, where it read as describing the new API instead of
DEVOURER_DIS_CCA.
Verified: build clean, ctest 63/63, jaguar1-only config builds.
tests/cca_gates_regcheck.sh 26 passed / 0 failed / 0 skipped over an 8822E
(0xa81a), an 8822C (0xc812), an 8822BU and an RTL8733BU. No Jaguar1 on this
bench, so that family's arm is still the one in the PR description.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0192ViRjaiFT9vTiiJpohZGT
|
Pushed The first one turned out to be a real defect, not a nit. Jaguar1 Reordering alone only narrows the window, so Your harness could not have caught that, and the reason is worth stating: the The retune cells got the same treatment. They only ever saw Which is the point: the API cannot speak for that half. Rest as described — Verified: build clean, Still no Jaguar1 on this bench, so that family's on-air table remains yours. Given the tracker fix lands on exactly that path, a re-run of the regcheck against your 8812AU would be worth doing — the Merging. |
The harness from #427 could not check Jaguar1 at all, for two independent reasons that both produced a clean-looking non-result rather than a failure. The probe built a default `DeviceConfig`, so `tuning.phydm_watchdog` was false — and on Jaguar1 that watchdog IS the EDCCA tracker, so `SetEdccaTrack` was never reached and the cell reported "no tracker running" about a path that had never been instantiated. A `--phydm-watchdog` flag builds it; Jaguar3 ignores the field. Separately, `peek32`/`poke32` addressed `chipstate` by `--pid` alone while the probe got `--vid`, so any adapter under a vendor VID rather than 0x0bda failed every register cell. Also fixes a teardown race the flag made reachable: `rtw_hal_deinit()` cleared `REG_CR`, zeroed `REG_RCR` and ran the card-disable sequence with the watchdog thread still doing BB reads and writes. `Stop()` now runs first, before any MAC-disable or power-off write. This retires the unvalidated caveat on the tracker-ordering fix from #427 — the race reproduces on an RTL8821AU once the window is widened and the disable lands while DIG is still walking IGI, and does not with the fix. Hardware: RTL8821AU (Jaguar1) 11 passed / 0 failed / 0 skipped; RTL8822E, RTL8822C, RTL8822BU and RTL8733BU 26 passed / 0 failed / 0 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0192ViRjaiFT9vTiiJpohZGT
2 participants
SetCcaModeis all-or-nothing, and on these families it is two gates doing different jobs:0x520[14]primary CCA defers to a decodable preamble,0x520[15]EDCCA defers to raw in-band energy. This addsSetCcaGates/GetCcaGatesso a caller can address them one at a time, with not-ported defaults, and reducesSetCcaModetoSetCcaGates(d, d).Why split them
They do not behave the same way — and on Jaguar1 they behave oppositely to what
tests/dis_cca_tx_onair.shfound.That test is Jaguar3. It uses the 8812AU only as its flooder and has never run a Jaguar1 as the DUT. Running one — RTL8812AU injecting 300 broadcast frames at 6M on an otherwise idle ch6, fresh radio open per arm so nothing latches across points, two independent receivers, repeated:
On Jaguar1 the energy bit is everything and the preamble bit is nearly null. Without the split there is no way to discover that, and no way to act on it except
dis_cca, which turns off both.Acting on it beats dis_cca
Same DUT, against a saturating co-channel flooder:
Carrier sense still defers (95% → 78% when a real transmitter takes the channel), and turning both gates off is worse for the injector's own delivery — it transmits into the flood and collides.
No behaviour change for existing callers
SetCcaMode's two states write exactly the bytes they wrote before:apply_cca(d, d)is the old body, with the BB threshold half at0x8a4keyed on the EDCCA argument.0x524[11]moves with the pair and only with the pair. What that bit does on its own is undocumented and unmeasured, so a mixed state leaves it at the enabled value rather than guessing. Jaguar3 also tracks the two gates separately soSetMonitorChannelre-asserts what the caller asked for, not just the all-or-nothing pair.Verification
Hardware: RTL8812AU (Jaguar1) and RTL8822C (Jaguar3, an 8812CU). All four gate states read back from
0x520on both, and the Jaguar3 state survives a retune.Defaults checked against a build without this change, alternating the two builds on the same bench so ambient drift shows as baseline-vs-baseline disagreement:
Every difference is inside the baseline-to-baseline spread.
cmake --buildclean andctest63/63 on this branch.Not verified here: Jaguar2, Kestrel and RTL8733B have no hardware on this bench and stay on the not-ported default, as does MediaTek, which is not an
IRtlRadioat all.A note on the CLAUDE.md claim
The current text reads as general:
It is accurate for Jaguar3 and inverted on Jaguar1. Happy to add the scoping and the Jaguar1 numbers in this PR or a follow-up, whichever you prefer.
Separately, this bench found that Jaguar1's bring-up enables EDCCA — programming
0x8a4off its parked never-trigger default, which the code comment itself identifies as the vendor's adaptivity-off default (CONFIG_RTW_ADAPTIVITY_EN 0) — and hard-codesth_l2h_iniand the H2L gap, both of which the vendor driver exposes as runtime module parameters. Measured cost of that default on an 8812AU injector is the 94% above. That is a policy question rather than a bug, so it is not in this PR; glad to raise it as an issue if useful.