jaguar3: gate PHY-status parsing on the RX-desc PHYST bit (DW0 bit 26) - #409
jaguar3: gate PHY-status parsing on the RX-desc PHYST bit (DW0 bit 26)#409gilankpam wants to merge 1 commit into
Conversation
1ee3244 to
f83c2b0
Compare
The jgr3 RX loop parsed the drvinfo area as a PHY-status report whenever it was long enough (>= 28 bytes). But drvinfo space is reserved on *every* frame — RX_DRVINFO_SZ is a global register — while the PHY writes a report only into the frames whose descriptor PHYST bit is set. On all-but-one subframe of an A-MPDU the area therefore holds stale bytes, and the page nibble in byte0 can alias a valid page number (0 = CCK, 1 = OFDM type1), so the parse silently folded garbage RSSI/SNR/EVM into the RF EMAs behind GetRxQuality() / GetActiveRxPaths() / the rx.path event. Decode DW0 bit 26 into Rx8822cFrame.physt and require it before parsing. This brings jaguar3 in line with jaguar1 (FrameParser.cpp:88) and rtl8733b (FrameParser8733b.h:77), which already decode the same bit into rx_pkt_attrib::physt — jaguar3 was the generation leaving it unset. parse_phy_sts_jgr3 now returns whether it recognised the page layout (page 0 CCK or page 1 OFDM type1) and actually filled the attrib's signal fields, rather than returning void after silently ignoring an unknown page number. The internal EMA folds (_rxq, _rxpaths, _cfo) gate on that result, so an unparsed page no longer contributes a zero-valued sample. tests/rx_physt_selftest.cpp is a headless guard on the bit position and the plumbing (ctest: rx_physt_bit, built with DEVOURER_JAGUAR3) so a descriptor-layout regression fails the suite instead of quietly poisoning the RF averages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FN9qETmHFJr6ekfkViZv8
f83c2b0 to
51d743b
Compare
PR Summary by QodoGate Jaguar3 PHY-status parsing on RX descriptor PHYST
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1. PHYST semantics diverge by chipset
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| if (!is_c2h && f.physt && f.drvinfo_size >= 28) | ||
| p.RxAtrib.physt = jaguar3::parse_phy_sts_jgr3( | ||
| data + off + jaguar3::RXDESC_SIZE_8822C, f.drvinfo_size, | ||
| p.RxAtrib); |
There was a problem hiding this comment.
1. Physt semantics diverge by chipset 🐞 Bug ≡ Correctness
Jaguar3 now reports RxAtrib.physt=false when the descriptor bit is set but the report is short or uses an unsupported page, while Jaguar1 and RTL8733B expose the raw descriptor bit. Consumers of the shared packet callback can therefore no longer use physt consistently to identify which A-MPDU subframe carried PHY status.
Agent Prompt
## Issue description
Jaguar3 assigns PHY parse success to the shared `RxAtrib.physt` field, whereas other generations assign the raw RX-descriptor PHYST bit. Preserve the descriptor-bit meaning and use a separate local parse-success flag to gate RF EMAs.
## Issue Context
`RxAtrib.physt` is exposed through the generation-neutral `Packet` callback. `parse_phy_sts_jgr3` may return false even when the descriptor PHYST bit is set, including for unsupported pages, so these two facts must remain separate.
## Fix Focus Areas
- src/jaguar3/RtlJaguar3Device.cpp[297-340]
- src/jaguar3/FrameParserJaguar3.h[278-317]
- src/jaguar1/FrameParser.cpp[72-89]
- src/rtl8733b/Rtl8733bDevice.cpp[353-380]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
josephnef
left a comment
josephnef
left a comment
There was a problem hiding this comment.
Reviewed with verification passes (every finding below was independently re-verified against the working tree; one candidate — the alleged doc duplication across the four new comment blocks — was checked and refuted, the copies are house-convention self-contained headers with location-specific scope).
The core gate itself checks out: DW0 bit 26 matches the vendor halmac layout, the selftest's byte encoding is right, Packet p{} is per-frame zero-initialized, and the new selftest builds and passes locally. The inline comments cover the in-diff issues; five more findings land on files this PR doesn't touch but its own premise implicates:
1. The identical bug is left live on jaguar2 — and it steers a control loop there (src/jaguar2/RtlJaguar2Device.cpp:622). The loop parses phy-status on only !is_c2h && f.drvinfo_size >= 28; Rx8822bFrame never decodes PHYST even though GET_RX_DESC_PHYST_8822B (same DW0 bit 26) sits unused at FrameParserJaguar2.h:89 and REG_RX_DRVINFO_SZ=4 is global (HalJaguar2.cpp:2578). On 8822BU/8821CU A-MPDU RX, subframes' reserved-but-unwritten drvinfo bytes are decoded as rssi/snr/evm/cfo_tail, and with cfo_track on, _cfo.add(p.RxAtrib.cfo_tail) (lines 631-632) steers the closed-loop XtalCap crystal trim from garbage. The PR's own comments describe the mechanism as generic; landing the fix at per-chip altitude leaves the worst instance of it in the tree.
2. Jaguar1 has a plausible cousin of the same gap (src/jaguar1/FrameParser.cpp:237). pattrib.physt is decoded at line 88 but the phy-status memcpy is gated on drvinfo_sz alone. The vendor driver passes pattrib->physt ? pbuf+RXDESC_OFFSET : NULL (rtl8812au usb_ops_linux.c:179); the same one-line gate is already parsed and sitting unused here. Affects per-path RSSI/SNR/EVM on rx.path/GetActiveRxPaths — including 8814AU 4-chain spatial-diversity measurements.
3. Demo-side consumers are not gated on the new flag (examples/rx/main.cpp:911-913, 920-925, 1136-1146). The hopset per-slot accumulators, the g_rxagg energy/LinkHealth aggregate, and DEVOURER_RX_ALLPATHS rx.path rows all read rssi[0]/snr[0]/evm[0] unconditionally, so jaguar3 A-MPDU subframes now inject deterministic rssi=0/snr=0/evm=0 samples. Pre-PR these frames carried random stale garbage; the PR converts that into a systematic directional bias (per-dwell means scale down with aggregation depth, snr_min pins to 0, FHSS exclusion scores a more-aggregating channel as worse, LinkHealth reads a strong link as weak). Cheap to close now that RxAtrib.physt exists to gate on.
4. rx.txhit's proof-of-what-flew contract weakens (examples/rx/main.cpp:1076). ldpc/stbc/bw come only from parse_phy_sts_jgr3, so physt=0 subframes now deterministically report ldpc=0/stbc=0 — indistinguishable from a real BCC fallback — and the event doesn't emit physt for consumers to tell the difference. In an encoding-truth-table run against an aggregating TX, a sampled txhit on a non-first subframe records an LDPC-qualification pass as BCC fallback. Emitting physt in the event (or skipping ldpc/stbc when unset) closes it.
5. physt now carries three per-generation semantics in a shared struct (src/RxPacket.h:25, no doc comment pinning one): raw descriptor bit (jaguar1 FrameParser.cpp:88, rtl8733b Rtl8733bDevice.cpp:355), bit AND drvinfo>=28 AND page-0/1-parsed (jaguar3, this PR), and never-written/always-false (jaguar2, kestrel). A cross-generation consumer keying on it as "hardware wrote a report" sees false on a jaguar3 PHYST=1 frame with an unrecognized page; one keying on it as "signal fields valid" discards every jaguar2/kestrel frame whose fields ARE populated. The rtl8733b shape is the clean one: raw bit into the field, parse-success in a local for the gate — plus one documented contract at the declaration.
Items 1 and 2 don't have to land in this PR, but item 1 at minimum deserves a tracked issue before merge — the CFO-trim exposure is a live correctness problem this PR's analysis proves exists.
| } | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Mutate-then-return-false breaks the new contract for OFDM pages 2-6. By this point the function has already written a.rssi[0..3], ldpc, stbc, bw from the OFDM common header — which is valid on every jgr3 OFDM page (per the comment above and the vendor phy_sts_rpt_jgr3_ofdm_cmn struct) — and then returns false, telling the caller "do not trust a's signal fields". Two consequences: the half-mutated attrib flows to the packetProcessor tagged physt=false, and genuinely valid per-path RSSI is dropped from the EMAs. If the BB page selector ever leaves type1 (vendor auto-switch via 0x8c4, or the vendor debug page-switch helper, which never restores the page), every OFDM frame reads physt=false and GetRxQuality/GetActiveRxPaths freeze with no diagnostic. Latent under devourer's own config (the BB table pins 0x8C0[25:22]=1), but the contract is wrong today: either fill nothing on the false path, or return true for pages whose common header was parsed.
| @@ -322,12 +322,17 @@ void RtlJaguar3Device::StartRxLoop(Action_ParsedRadioPacket packetProcessor) { | |||
| * present (monitor_rx_cfg enables APP_PHYSTS + RX_DRVINFO_SZ=4, so the | |||
| * 32-byte report is counted in drvinfo). Skips C2H reports and any | |||
| * frame whose drvinfo is too short (e.g. CCK, which carries no OFDM | |||
There was a problem hiding this comment.
This retained sentence is wrong on both counts and now contradicts the new text three lines below. RX_DRVINFO_SZ is a global write (HalJaguar3.cpp:485: 0x060F=0x04 → 32 bytes on every frame, CCK included), and parse_phy_sts_jgr3 explicitly decodes the CCK page-0 report — so CCK frames are neither short-drvinfo nor report-less. A maintainer trusting this sentence could skip the parse for CCK rates or "optimize" the >= 28 guard, reintroducing exactly the misparse class this PR guards against. Please drop or correct it.
| p.Data = std::span<uint8_t>(const_cast<uint8_t *>(f.frame), f.frame_len); | ||
| if (!p.RxAtrib.crc_err) { | ||
| if (!p.RxAtrib.crc_err && p.RxAtrib.physt) { | ||
| _rxq.add(p.RxAtrib.rssi[0], p.RxAtrib.snr[0], p.RxAtrib.evm[0]); |
There was a problem hiding this comment.
CCK page-0 passes the gate having filled only rssi[0], so this line feeds snr=0 and the CFO tracker gets 0. parse_phy_sts_jgr3 returns true for page 0 with snr/evm/cfo_tail never populated; RxQuality.h sums snr_raw unconditionally (EVM has a !=0 filter, SNR does not), and _cfo.add(0) dilutes avg_khz below the 11-unit enable threshold, so on a 2.4 GHz channel with CCK beacon/probe traffic a real CFO can go uncorrected. The feed itself is pre-existing (the old gate was bare !crc_err), but the new per-page return was the natural place to require page 1 for the snr/cfo consumers — worth doing while the contract is being defined here.
| buf[3] |= 0x04; | ||
| } | ||
|
|
||
| static void test_physt_bit_decoded() { |
There was a problem hiding this comment.
The selftest covers only the descriptor-bit decode — parse_phy_sts_jgr3's new bool return, the value that actually gates the RF EMAs, is never exercised, despite the header promising a plumbing regression "fails ctest instead of poisoning the RF EMAs". A regression in the return plumbing (the page-1 branch losing its return true, or the page gate flipping) passes ctest green while all OFDM RSSI/SNR/EVM accounting silently stops. The function is inline and already #included by this TU; four 28-byte synthetic pages cover it: page 0 → true + rssi[0]; page 1 → true + evm/snr/cfo; page 5 → false; 27 bytes → false. tests/rtl8733b_rx_parse_selftest.cpp:20-37 has the set_bits/fixture-builder shape worth copying over the hand-poked masks.
2 participants
Problem
The Jaguar3 RX loop parsed the drvinfo area as a PHY-status report whenever it was long enough:
But
RX_DRVINFO_SZis a global register — the drvinfo space is reserved on every frame, while the PHY writes a report only into frames whose RX-descriptor PHYST bit (DW0 bit 26) is set. On all-but-one subframe of an A-MPDU the area therefore holds stale bytes from a previous frame, and the page nibble inphysts[0] & 0x0fcan alias a valid page number (0= CCK,1= OFDM type1).The parse then succeeded on garbage and folded it into the RF EMAs behind
GetRxQuality(),GetActiveRxPaths()and therx.pathevent — contaminated RSSI/SNR/EVM tails that track A-MPDU density rather than the link.Change
Rx8822cFrame.phystand require it before parsing the report.parse_phy_sts_jgr3now returnsbool— whether it recognised the page layout (page 0 CCK, or page 1 OFDM type1) and actually filled the attrib's signal fields. Previously it returnedvoidafter silently ignoring an unknown page number, leaving the caller unable to tell a filled attrib from an untouched one._rxq,_rxpaths,_cfo), so an unparsed page no longer contributes a zero-valued sample.tests/rx_physt_selftest.cpp— headless guard on the bit position and the plumbing (ctest: rx_physt_bit, built underDEVOURER_JAGUAR3), so a descriptor-layout regression fails the suite instead of quietly poisoning the averages.Consistency with the other generations
rx_pkt_attrib::phystalready exists and is already populated on two generations that decode the same bit — Jaguar3 was the one leaving it unset:src/jaguar1/FrameParser.cpp:88GET_RX_STATUS_DESC_PHY_STATUS_8812src/rtl8733b/FrameParser8733b.h:77rx_bits(buf + 0x00, 26, 1)GET_RX_DESC_PHYST_8822COpen question for reviewers
This PR sets
RxAtrib.phystto the parse result (report written and page understood and fields filled), whereas Jaguar1 and RTL8733B set it to the raw descriptor bit. The parse-result semantics is what the internal EMA gate wants, but it means a caller readingRxAtrib.phystgets a slightly different meaning per generation.The alternative is to assign
p.RxAtrib.physt = f.phystunconditionally (matching the other two) and keep the parse-succeeded flag in a local for the gate. Happy to switch to that if you'd rather the field keep one cross-generation meaning — the on-air behaviour is identical either way.Testing
ctest: 54 passed, 1 skipped (la_csi_math, needs numpy), 0 failures — including the newrx_physt_bit.Not included
Split out of the working branch deliberately — the same branch also carried an unrelated
usb.rx_zerocopydefault flip (intermittent zero-frame delivery on an xhci host). That is a separate concern and will come as its own PR if wanted.🤖 Generated with Claude Code
https://claude.ai/code/session_019FN9qETmHFJr6ekfkViZv8