rx: gate PHY-status parsing on the RX-descriptor PHYST bit (Jaguar1/2/3)

[gilankpam]

Problem

REG_RX_DRVINFO_SZ is a global register on every generation that carries per-frame drvinfo — Jaguar1 _InitDriverInfoSize_8812A(4) (HalModule.cpp:268), Jaguar2 0x060F=4 (HalJaguar2.cpp:2578), Jaguar3 0x060F=4 (HalJaguar3.cpp:485). The drvinfo space is therefore reserved on every frame, while the PHY writes a report only into frames whose RX-descriptor PHYST bit (DW0 bit 26) is set.

All three RX loops gated the phy-status parse on the drvinfo size alone. On all-but-one subframe of an A-MPDU the area holds bytes left by an earlier frame, and the page nibble can alias a valid page number, so the parse succeeded on garbage and folded it into the RF running averages behind GetRxQuality(), GetActiveRxPaths() and the rx.path event — contaminated RSSI/SNR/EVM tails that track A-MPDU density rather than the link.

On Jaguar2 it went further than telemetry: the garbage cfo_tail fed _cfo.add(), which steers the closed-loop XtalCap crystal trim.

Change

The gate, on all three generations. Decode the descriptor PHYST bit and require it before parsing.

Generation	Site	Was
Jaguar1	FrameParser.cpp:237	bit decoded at line 88, memcpy gated on drvinfo_sz only — the comment there already named pattrib->physt as the vendor's gate it was approximating
Jaguar2	RtlJaguar2Device.cpp:622	GET_RX_DESC_PHYST_8822B present but unused; bit never decoded into Rx8822bFrame
Jaguar3	RtlJaguar3Device.cpp:330	bit never decoded

RxAtrib.physt gets one cross-generation meaning, documented at its declaration (src/RxPacket.h): the raw descriptor bit — "the PHY wrote a report into this frame's drvinfo" — explicitly not "the report parsed" and not "the signal fields are valid". Kestrel never sets it, and the comment says so and why (its PHY status arrives as its own PPDU-status frame). This resolves the open question the first revision raised: the parse outcome lives in a local instead.

parse_phy_sts_jgr2 / parse_phy_sts_jgr3 return PhyStsFill (None / Power / Full) — which fields they filled, not whether they liked the buffer. A bool could not express the two real tiers, and was wrong in both directions:

• it returned false for OFDM pages 2..6 after filling valid per-path RSSI, ldpc, stbc and bw from the common header those pages share with type1 — dropping a real measurement, and leaving GetRxQuality/GetActiveRxPaths to freeze with no diagnostic if the BB page selector ever left type1 (devourer's BB table pins 0x8C0[25:22]=1, but the vendor auto-switch and the debug page helper do not restore it)
• it returned true for the CCK page, whose layout carries path-A power and no SNR/EVM/CFO at all

The CFO trackers now take only a Full fill on Jaguar2 and Jaguar3.

RxQualityAccumulator folded SNR unconditionally (src/RxQuality.h) while its own doc comment three lines above claimed it folded only when present. EVM and the noise-floor term did filter; SNR did not. Not a Jaguar3 bug — Jaguar1, Jaguar2 and the RTL8733B all feed CCK frames through the same accumulator, so snr_min_db has been pinned to 0 on any 2.4 GHz channel with CCK traffic on all of them, and snr_mean_db dragged toward zero in proportion to CCK density. SNR now carries its own sample count like EVM, surfaced as RxQuality::snr_valid so a CCK-only window reports "no SNR" rather than a fabricated 0. RxPathActivityAccumulator already filtered per-metric and is unchanged.

Stale comment dropped (RtlJaguar3Device.cpp): it claimed CCK frames have short drvinfo and carry no report. Both false — the register write is global, and parse_phy_sts_jgr3 decodes the CCK page-0 report explicitly.

Testing

• tests/rx_physt_selftest.cpp (ctest rx_physt_bit) exercises both the bit decode and the return contract, on both generations: descriptor bit set/clear through parse_rx_8822c and parse_rx_8822b; null buffer and a 27-byte report → None with the attrib verified untouched; CCK page → Power with snr/evm/cfo_tail asserted still 0; OFDM page 1 → Full with rssi/snr/evm/cfo/ldpc/bw checked; OFDM page 5 → Power with common-header RSSI present and the type1-only fields still 0.
• rx_quality_selftest gained the mixed-window and CCK-only-window SNR cases.
• ctest: 55 passed, 1 skipped of 56 (la_csi_math skips, needs numpy), 0 failures — rebased onto master at 1413b14.
• Per-chip configs built clean: JAGUAR1-only, JAGUAR2_8822B-only, JAGUAR3_8822C-only. The selftest is registered under DEVOURER_JAGUAR3 OR DEVOURER_JAGUAR2_8822B OR DEVOURER_JAGUAR2_8821C — verified absent in the Jaguar1-only config and passing in the other two.
• No new compiler warnings.
• Hardware: the contamination was found on an 8822EU RX session; the bit-level decode and the parse tiers are what the selftests pin. Not re-validated on-air since the split from the working branch, hence draft.

Deliberately not included

• Demo-side consumers (examples/rx/main.cpp): the hopset per-slot accumulators, the g_rxagg energy/LinkHealth aggregate and the DEVOURER_RX_ALLPATHS rx.path rows read rssi[0]/snr[0]/evm[0] unconditionally. One correction to how this was framed in review: gating them on RxAtrib.physt would drop every sample on Kestrel, which never sets it. The generation-neutral gate is rssi[0] > 0 — the same test RxQualityAccumulator already applies. Follow-up.
• rx.txhit emitting physt so an encoding-truth-table run can tell a real BCC fallback from a subframe that carried no report. Follow-up.
• An unrelated usb.rx_zerocopy default flip that rode the same working branch.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JiAS32956Z3cmTbnmcXYkT

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Gate Jaguar3 PHY-status parsing on RX descriptor PHYST

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Decode Jaguar3 RX-descriptor PHYST before reading per-frame PHY-status data.
• Exclude missing or unsupported PHY reports from RSSI, path, and CFO averages.
• Add a conditional self-test pinning DW0 bit 26 decoding.

Diagram

graph TD
  A["RX descriptor"] --> B["Frame parser"] --> C{"PHYST set?"}
  C -->|Yes| D["PHY parser"] --> E{"Page known?"}
  E -->|Yes| F["Signal attributes"] --> G["RF EMAs"]
  C -->|No| H["Skip metrics"]
  E -->|No| H

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Preserve raw PHYST semantics

• ➕ Keeps rx_pkt_attrib::physt consistent across Jaguar1, Jaguar3, and RTL8733B.
• ➕ Retains the hardware-reported presence flag independently of parser support.
• ➕ Still protects EMAs by using the parser result as a local gate.
• ➖ Callers cannot use physt alone to determine whether signal fields were decoded.
• ➖ Requires maintaining a separate local parse-success variable.

2. Add explicit parse-status metadata

• ➕ Separates report presence from successful decoding without overloading physt.
• ➕ Can distinguish missing, malformed, and unsupported PHY pages.
• ➖ Expands the shared packet attribute API for a narrowly scoped fix.
• ➖ Requires broader consumer and compatibility review.

Recommendation: Preserve rx_pkt_attrib::physt as the raw descriptor bit and use a local parse-success boolean for EMA gating. This maintains cross-generation API semantics while retaining the PR's protection against stale or unsupported PHY reports; a separate status field is only warranted if consumers need detailed decode outcomes.

Files changed (4) +93 / -9

Bug fix (2) +25 / -9

FrameParserJaguar3.hDecode PHYST and report PHY parse success +15/-4

Decode PHYST and report PHY parse success

• Adds the RX descriptor PHYST flag to Rx8822cFrame and decodes it from DW0 bit 26. Changes parse_phy_sts_jgr3 to return true only for recognized page 0 or page 1 layouts that populate signal attributes.

src/jaguar3/FrameParserJaguar3.h

RtlJaguar3Device.cppGate PHY parsing and RF averages on valid reports +10/-5

Gate PHY parsing and RF averages on valid reports

• Requires PHYST before parsing Jaguar3 driver-info bytes and records whether the PHY page was understood. RSSI, active-path, and CFO averages now receive samples only from successfully parsed, CRC-valid reports.

src/jaguar3/RtlJaguar3Device.cpp

Tests (1) +58 / -0

rx_physt_selftest.cppVerify Jaguar3 DW0 PHYST bit decoding +58/-0

Verify Jaguar3 DW0 PHYST bit decoding

• Introduces a headless descriptor test covering both set and clear states of DW0 bit 26. The test verifies that parse_rx_8822c propagates the hardware flag into Rx8822cFrame.physt.

tests/rx_physt_selftest.cpp

Other (1) +10 / -0

CMakeLists.txtRegister the Jaguar3 PHYST descriptor self-test +10/-0

Register the Jaguar3 PHYST descriptor self-test

• Adds the RxPhystSelftest executable and rx_physt_bit CTest entry when DEVOURER_JAGUAR3 is enabled.

CMakeLists.txt

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. PHYST semantics diverge by chipset ✓ Resolved 🐞 Bug ≡ Correctness

Description

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.

Code

src/jaguar3/RtlJaguar3Device.cpp[R330-333]

+        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);

Evidence

rx_pkt_attrib belongs to the shared packet model, and its A-MPDU documentation associates PHY
status with the aggregate subframe carrying it. Jaguar1 and RTL8733B assign the raw descriptor bit
to this field and RTL8733B keeps parse success in a separate local, while the new Jaguar3 code
assigns the parser result; that parser explicitly returns false for short and unsupported-page
reports even when the descriptor bit was true.

src/RxPacket.h[8-11]
src/RxPacket.h[22-26]
src/RxPacket.h[72-79]
src/jaguar1/FrameParser.cpp[72-89]
src/rtl8733b/Rtl8733bDevice.cpp[353-380]
src/jaguar3/FrameParserJaguar3.h[278-317]
src/jaguar3/RtlJaguar3Device.cpp[330-340]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

---

Tip of the day

💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[josephnef]

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.

[gilankpam]

Pushed 40a2439 addressing the review. Every finding was checked against the working tree before acting; here is where each landed.

In-diff (all four inline threads answered on the threads themselves): the mutate-then-return-false contract, the stale CCK comment, the CCK page feeding snr=0/cfo=0, and the selftest that did not exercise the return value.

Items 1 and 2 — both confirmed, both fixed here rather than tracked.

Item 1 (jaguar2): verified end to end — GET_RX_DESC_PHYST_8822B sitting unused at FrameParserJaguar2.h:89, 0x060F=4 written globally at HalJaguar2.cpp:2578, and _cfo.add at RtlJaguar2Device.cpp:631 steering the XtalCap trim from it. It was ~4 lines once the enum from the jaguar3 fix existed, which was smaller than writing the issue would have been.

Item 2 (jaguar1): also confirmed — _InitDriverInfoSize_8812A(DRVINFO_SZ=4) at HalModule.cpp:268 is the global write, so the size check was never equivalent to the vendor gate the comment at FrameParser.cpp:231 already cited. One line, plus gating the EMA feed so the CFO tracker does not take a zero tail from a physt=0 frame.

The PR title and description now cover all three generations.

Item 5 — taken, plus a fourth semantics you did not list. RxAtrib.physt is the raw descriptor bit everywhere, with the parse outcome in a local, and the field now has a documented contract at its declaration. The fourth case was jaguar2 in the never-written state — so all four generations that carry per-frame drvinfo now populate it identically, and the contract notes Kestrel deliberately does not.

Item 3 — one correction before it becomes a follow-up. Gating the demo consumers on RxAtrib.physt would drop every sample on Kestrel, which never sets it (and never can, given how its physts arrives). The generation-neutral gate is rssi[0] > 0, which is what RxQualityAccumulator::add already applies internally. Left out of this PR to keep it library-side, along with item 4.

One thing the CCK finding turned up that was not in the review. Chasing where the zero SNR actually lands: RxQualityAccumulator::add folded SNR into snr_sum_/snr_min_ unconditionally, while its own doc comment three lines above claimed it folded only when present. EVM and the noise-floor term did filter; SNR did not. So the dilution was never jaguar3-specific — jaguar1, jaguar2 and the rtl8733b all feed CCK frames through the same accumulator, and snr_min_db has been pinned to 0 on any 2.4 GHz channel with CCK traffic on all of them. Fixed with a per-metric sample count, surfaced as RxQuality::snr_valid.

ctest 55/55 (la_csi_math skipped); JAGUAR1-only, JAGUAR2-only and JAGUAR3-only configs all build clean, with the selftest correctly absent from the first. Still draft pending on-air re-validation.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JiAS32956Z3cmTbnmcXYkT

[josephnef]

All review findings verified fixed at ca61f2d — approving.

Re-verified against the branch, not just the replies: the jaguar2 PHYST gate (including the PhyStsFill::Full requirement on the XtalCap _cfo.add feed), the jaguar1 memcpy gate, the three-tier PhyStsFill return (pages 2..6 as Power is a better shape than either option I offered — the common-header RSSI stays a measurement and the page-selector freeze scenario degrades to RSSI-only), the raw-bit physt contract documented at the RxPacket.h declaration, the corrected CCK/drvinfo comment, and the widened selftest (build + full ctest green locally, 56/56, including the new rx_physt_bit tiers and the rx_quality mixed/CCK-only windows).

The RxQualityAccumulator SNR find deserves its own credit: the unconditional snr_sum_ += snr_raw contradicting the accumulator's own doc comment was live on every generation, and snr_valid is the honest surface for a CCK-only window.

The two demo-side items from the review body (gating the examples/rx accumulators on the now-available RxAtrib.physt, and emitting physt on rx.txhit) plus the same ungated-SNR shape in chanscout and cell::UeRxAttribution are follow-up-sized and tracked separately rather than holding this fix.