sensing: a vendor-neutral channel-busy contract, and the survey dwell as a library primitive - #432
Merged
Merged
Conversation
…adio ## Problem `GetRxEnergy` is declared on `IRtlRadio`, so `src/chanmig/` and `src/hopset/` — the adaptive-link subsystems — are vendor-neutral in their logic but can gather no evidence at all on the MediaTek backend. Commit 90e1cad (#415) named this follow-up: "hopset TX-side sensing and the chanmig energy probe are Realtek-only until a neutral frame-free energy type exists". `RxEnergy` cannot be that type. It is phydm-shaped — false-alarm classes, the DIG index, IGI-referenced NHM buckets — and IRadio.h and IRtlRadio.h both assign those to the Realtek level on purpose. But one field in it is already neutral, and says so: CLM is "AIRTIME, directly comparable across channels and adapters, and unlike NHM it needs no gain reference". Both silicon families count busy airtime in hardware — Realtek via CCX CLM, MediaTek via MT_CH_BUSY/MT_CH_IDLE with TX+RX+NAV+EIFS counted busy. ## Change - `ChannelBusy` + `BusySource` in `src/RxSense.h`, with two pure conversions (`busy_from_rx_energy`, `busy_from_ch_time`) so every arithmetic decision is testable without hardware. `BusySource` is part of the reading, not metadata: the MediaTek timers count a transmitting radio's own airtime and Realtek's CLM does not, so ranking across a mixed adapter pair compares two rulers. - `IRadio::GetChannelBusy()`, optional virtual with the house all-invalid default. - Implemented ONCE on `IRtlRadio` in terms of `GetRxEnergy(true)`. All five Realtek backends inherit it with no per-backend edits, and the two without CLM — the RTL8733B (no override at all) and Kestrel (NHM rides the halbb glue, not NhmReader) — report no reading rather than a fabricated zero. - `Mt7612uRadio::GetChannelBusy()` from a new narrow `mt7612u_ch_time()`. - `AdapterCaps::busy_airtime_ok` / `_measured` / `rx_energy_ok`, set honestly per family. - `RxQuality` carries the neutral reading. Deliberately NOT fed into `LinkHealthInput`: whether CLM belongs in a scoring law needs its own validation, and routing it through `classify_link_health` would change every backend's verdict as a side effect of an interface change. - The Realtek DIG rails hardcoded inside neutral `build_rx_quality` (`igi_min = 0x1c; igi_max = 0x7f`) move into `LinkHealthThresholds` with the same defaults — behaviour-identical, and a neutral header stops asserting Realtek register constants. `mt7612u_ch_time()` reads ONLY the two channel-timer registers. It is not `mt7612u_link_stats()` because that also reads MT_RX_STAT_1, whose false-CCA field is read-and-clear and owned by `mt7612u_phy_tick()`'s AGC loop; polling it at caller cadence would both misreport the figure and starve the gain tracking. For the same reason `energy_pct` is left invalid on MediaTek — the only candidate counter has an owner. Reads are checked, because `mt_rr`'s `~0u` failure value would otherwise surface as a 100%-busy channel. ## Validation New `channel_busy_math` ctest covers both conversions, including the case that matters most — `busy + idle == 0` reports NO reading, never 0% busy, because a never-armed counter is not an idle channel. Also a 64-bit ratio near UINT32_MAX that wraps to nonsense in 32 bits. `radio_iface_selftest` gains three things: the neutral default and caps defaults on a radio with no Realtek type in sight (that it compiles is the proof of placement); the RTL8733B shape, where the inherited implementation must fabricate nothing; and a third fixture overriding only `GetRxEnergy`, proving the once-on-IRtlRadio implementation reaches a derived reader. Its existing cast assertion is untouched. 65/65 ctest, plus the mt7612u-only subset — which is what caught a caps block landing in GetTxCaps instead of GetAdapterCaps, since MT7612U is off in a default build. No behaviour change on any Realtek path: nothing calls the new member yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Au9D2ntoFn4ABLJqp9vYDk
## Problem The loop that turns a channel plan into evidence lived only in `examples/chanscout/main.cpp`. Two consequences: An integrator wanting to survey the band had to re-implement it from a demo — which is why that kind of feature gets built against the kernel driver's procfs instead. And it was untested. `scan_plan_policy` covers the scheduler, `survey_aggregate` covers the consumer, and the retune/barrier/observe sequencing between them had nothing: a regression in it was findable only on air. ## Change New `src/sensing/`, the one helper subtree that calls device methods. It owns no thread, performs no sleep and takes no clock of record — which is precisely what lets `src/chanmig/` and `src/hopset/` keep the purity they both assert. `src/cell/` was not a precedent for this: it takes raw scalars and includes no `IRadio.h`. Two layers, because the split is what makes a later `examples/tx` conversion mechanical rather than a redesign: - `SenseWindow.h` — the shared settle -> DISCARD BARRIER -> observe -> read discipline, in microseconds, with no retune and no frame term. A null `IRtlRadio*` is a first-class case. `examples/tx`'s `hopset_sense_window` still carries its own copy (its comment says "the discipline is chanscout's"); this header is shaped for it, and it gains the counter-plausibility check it owns a flag for but never sets. - `DwellExecutor.h` — the survey-shaped layer: three phases the caller drives, because the two sleeps are policy (chanscout wants a chunked stop-aware nap so SIGINT stays responsive) and a library that slept would have to pick one. - `SurveyFrameAgg.h` — the frame fold as a library type, with the ours-vs-foreign attribution key as CONFIGURATION. A library hardcoding the canonical devourer SA would score an integrator's own video as interference. `examples/chanscout` converts onto it: 40 insertions, 186 deletions. What deliberately did NOT move is the demo policy — health reporting, the retune-p95 USB check, the Jaguar1 thermal probe, bin-age staleness, advise mode. If those migrated this would stop being an extraction. ## A latent bug the new test caught A `SetMonitorChannel` throw part-way through a full-width dwell left the width-restoration latch clear, so the next bin dwell took the lean same-width `FastRetune` at a possibly-80 MHz width — and every later bin would have observed at the candidate's width, a wrong reading that still looks entirely plausible. The latch is now set whenever a full-gate tune throws, because the chip may have been part-way reconfigured and its width is unknown. A `FastRetune` throw is left alone: that path never changes width. `observe_ms` is timed inside `finish()` rather than from a caller-supplied `now`. The argument form would be evaluated before the observation read and would silently exclude it, shortening the very window the plausibility ceiling is computed from — measured as a 107 -> 102 ms shift before this was fixed. ## Validation New `dwell_executor` ctest against a scripted fake radio and a scripted clock, so no duration is timing-dependent: barrier ordering (frames folded before or during the barrier are absent, frames inside the window are present), the width-restoration rule and both its throw paths, counter plausibility at the exact ceiling (`>` not `>=`), the read-failure latch (never-valid is unported, not failed), frame attribution flipping with the configured SA, and round/seq bookkeeping against the real `ScanScheduler`. Plus the case this whole line of work exists for: a plain `IRadio` with no Realtek type still produces usable dwells. 66/66 ctest; ASan+UBSan clean; TSan clean, newly meaningful because the frame aggregator is exercised by a test for the first time; jaguar1-only, jaguar3-only and mt7612u-only subsets build and pass. On air, 8812CU, baseline binary built from master and run back to back against the refactored one on the same plan: 314 dwells each side, seq gapless on both, identical flag histogram, no field lost, `observe_ms` median 112 -> 112 (0% drift), `retune_us` 1304 -> 1314 (1%). `tests/chanscout_stress.sh` ran past 1100 consecutive dwell retunes with no wedge and no errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Au9D2ntoFn4ABLJqp9vYDk
PR Summary by QodoAdd vendor-neutral channel sensing and reusable survey dwells
AI Description
Diagram
High-Level Assessment
Files changed (30)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
…estly Addresses the Qodo review on #432. Three real bugs and four documentation-duplication findings. ## The neutral contract reached no decisions (finding 5) `DwellExecutor` built its `SenseWindow` from an `IRtlRadio*` and read only `GetRxEnergy`, so on a non-Realtek radio the dwell carried frame evidence but no busy airtime — the reading commit 1 had just added was implemented and then never consumed. The PR claimed "a plain IRadio still produces usable dwells"; that was true only of the frame half. `SenseWindow` now takes the `IRadio*` as well. On a Realtek radio the neutral value is DERIVED from the `GetRxEnergy` read already taken — a second `GetChannelBusy` would drain the same delta counters twice and halve both readings. On any other backend it calls `GetChannelBusy` directly, at the barrier as well as the observation, because those counters are read-and-clear too and the barrier is what scopes the window. `SurveyDwell` gains `busy_source` (schema v3; the parser still accepts v1 and v2). Provenance is not decoration: a channel-timer family counts the radio's own TX as busy and Realtek's CLM does not, so ranking across a mixed pair of scouts compares two rulers. Having argued that in the PR, dropping it at the record boundary would have been inconsistent. The selftest now proves the claim rather than asserting it: a radio with only `GetChannelBusy` and no `IRtlRadio` anywhere produces a dwell carrying real busy airtime with its provenance, and one offering no reading records none — not 0% busy. ## MediaTek timers could be read unarmed or across a retune (findings 6, 7) Both are the kind of thing on-air testing would have caught, and this path has none. The timers were armed only in `StartRxLoop`, so a transmit-only session read registers nobody had configured — and `busy_from_ch_time` would have turned a previous session's residue into a perfectly plausible percentage for a window that was never measured. They are now armed on the transmit-only path too, an `ch_time_armed` flag is tracked per device, and `mt7612u_ch_time()` refuses while unarmed. `SetMonitorChannel` neither cleared the counters nor reset the interval mark, so the first sample after a retune mixed the old channel's airtime into the new channel's reading and reported it as valid. It now re-arms, which clears both. ## Documentation (findings 1-4) The root and subtree guidance restated contracts the headers already own; trimmed to pointers and the rationale that is genuinely subtree-specific. Finding 4 is the one that mattered: `src/mt7612u/CLAUDE.md` quoted the favourable half of a controlled benchmark — the tick's 5415-5470 fps — without its no-tick control of three frames in ten seconds. That is exactly the standing rule in the root file, "never quote a favourable measurement without its adversarial counterpart in the same breath". The number is gone; readers are pointed at the complete figure where the control sits beside it. ## Validation 66/66 ctest; jaguar1-only, jaguar3-only and mt7612u-only subsets pass. On air, 8812CU, baseline rebuilt from master and re-run back to back because the record schema and the executor both changed: 314 dwells each side, seq gapless, identical flag histogram, no field lost, `observe_ms` 112 -> 112 (0% drift), `retune_us` 1347 -> 1294 (4%). Every Realtek record now carries `busy_src: 1` alongside an unchanged `clm`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Au9D2ntoFn4ABLJqp9vYDk
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
1 participant
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.
Why
Analysing OpenIPC/PixelPilot_rk#148 — a GS spectrum screen built on the vendor driver's
chan_infoprocfs — sent us looking for the equivalent gaps here. Three turned out to be structural and connected:GetRxEnergyis onIRtlRadio, sosrc/chanmig/andsrc/hopset/— the whole adaptive-link story — are vendor-neutral in their logic but can gather no evidence at all on the MediaTek backend shipped in mt7612u: the IRadio backend — devourer opens, receives and transmits on a MediaTek adapter #422. Commit90e1cad(Rename the radio contract to vendor-neutral names (IRadio, ITransport, CreateRadio) and split the Realtek-only members onto IRtlRadio #415) named this exact follow-up: "hopset TX-side sensing and the chanmig energy probe are Realtek-only until a neutral frame-free energy type exists".RxQualitynever got rx: CLM busy-airtime and the NHM noise-floor reduction #431's new sensors.clm/nhm_envwent ontoRxEnergy(Realtek-only); the neutral struct that already carriesfa_ofdm/cca_ofdm/igidid not.examples/chanscout/main.cpp— and was untested: the scheduler has a selftest, the consumer has one, the executor between them had none.The neutral concept is busy airtime, not phydm counters
RxEnergycannot be the portable type — it is phydm-shaped, andIRadio.handIRtlRadio.hboth assign those counters to the Realtek level on purpose. But one field in it is already neutral and says so: CLM is "AIRTIME, directly comparable across channels and adapters, and unlike NHM it needs no gain reference."Both silicon families count it in hardware:
MT_CH_BUSY/MT_CH_IDLE, already implemented and armed in this portCommit 1 —
ChannelBusyonIRadioChannelBusy+BusySourceinsrc/RxSense.h, with two pure conversions so every arithmetic decision is testable with no device.BusySourceis load-bearing, not metadata: the MediaTek timers count TX+RX+NAV+EIFS, so a transmitting radio includes its own airtime, while Realtek's CLM is receive-side deferral only. Ranking across a mixed adapter pair compares two rulers.Implemented once on
IRtlRadioin terms ofGetRxEnergy(true)— all five Realtek backends inherit it with no per-backend edits, and the two without CLM (RTL8733B, Kestrel) report no reading rather than a fabricated zero, by construction.AdapterCaps::busy_airtime_ok/_measured/rx_energy_okretire a discriminator that was never correct: the RTL8733B passesdynamic_cast<IRtlRadio*>and implements no energy reader at all.The MediaTek side goes through a narrow new
mt7612u_ch_time()touching only the two channel-timer registers. Notmt7612u_link_stats(): that also readsMT_RX_STAT_1, whose false-CCA field is read-and-clear and owned bymt7612u_phy_tick()'s AGC loop, so polling it at caller cadence would both misreport the figure and starve the gain tracking. For the same reasonenergy_pctis left invalid there — the only candidate counter has an owner.Also: the Realtek DIG rails hardcoded inside neutral
build_rx_qualitymove intoLinkHealthThresholdswith the same defaults. Behaviour-identical; a neutral header stops asserting Realtek register constants.Commit 2 —
src/sensing/, the dwell as a library primitiveA new subtree, the one that calls device methods. It owns no thread, performs no sleep and takes no clock of record — which is what lets
chanmig/andhopset/keep the purity they both assert. (src/cell/was not a precedent: it takes raw scalars and includes noIRadio.h.)Two layers, so a later
examples/txconversion is mechanical:SenseWindow.his the shared settle → barrier → observe → read discipline with no retune and no frame term —hopset_sense_windowstill carries its own copy and its comment already says "the discipline is chanscout's" — andDwellExecutor.his the survey-shaped layer.chanscoutconverts onto it: 40 insertions, 186 deletions, with all the demo policy (health, thermal, p95, bin-age, advise) deliberately left behind.A latent bug the new test caught
A
SetMonitorChannelthrow part-way through a full-width dwell left the width-restoration latch clear, so the next bin dwell took the lean same-widthFastRetuneat a possibly-80 MHz width — and every later bin would have observed at the candidate's width. A wrong reading that still looks entirely plausible. Now any full-gate tune that throws sets the latch, because the chip may have been part-way reconfigured.Validation
Headless: 66/66 ctest including two new gates; ASan+UBSan clean; TSan clean — newly meaningful, because the frame aggregator is exercised by a test for the first time; jaguar1-only, jaguar3-only and mt7612u-only subsets build and pass. The mt7612u subset is what caught a caps block landing in
GetTxCapsinstead ofGetAdapterCaps, since MT7612U is off in a default build.On air (8812CU): a baseline
chanscoutbuilt from master, run back to back against the refactored one on the same plan — 314 dwells each side, seq gapless on both, identical flag histogram, no field lost,observe_msmedian 112 → 112 (0% drift),retune_us1304 → 1314 (1%). An earlier run showed 107 → 102, which is what exposedfinish()sampling its timestamp before the observation read and silently shortening the window the plausibility ceiling derives from.tests/chanscout_stress.shcompleted its full run: 2728 consecutive dwell retunes, seq gapless, zero flags of any kind set across the whole stream (no truncation, no retune failure, no read failure, no counter-suspect, no missing NHM), scout healthokthroughout, no wedge.Not validated, and shipped saying so: the MediaTek path — no MT7612U on the bench.
busy_airtime_measured = false, register basis documented insrc/mt7612u/CLAUDE.md. Two things to measure when an adapter is available: that busy/idle track real occupancy beyond repetition noise, and — the real risk — that polling at dwell cadence does not disturbphy_tick's gain tracking.Deliberately not here
ChannelScoreoff itsfa_rate/(fa_rate+200)magic constant onto real CLM airtime. That changes the migration law and needs its own re-validation of the 14-row failure matrix and an on-air soak.examples/tx. It is the TX hot path, called inside the slot-timed hop loop, and its correctness criterion is on-air FHSS lockstep that headless tests cannot see.GetChannelBusyyet, so commit 1 is provably no-behaviour-change on every Realtek path.🤖 Generated with Claude Code
https://claude.ai/code/session_01Au9D2ntoFn4ABLJqp9vYDk