jaguar3: honour rx.keep_corrupted in the monitor RCR (ACRC32|AICV)

[gilankpam]

DEVOURER_RX_KEEP_CORRUPTED / DeviceConfig::rx.keep_corrupted was a silent
no-op on Jaguar3 (RTL8812CU/8822CU, RTL8812EU/8822EU).

monitor_rx_cfg wrote a hardcoded RCR of 0xF410400F | BIT28, and its comment
claimed that value already accepted CRC/ICV-error frames — it listed
AAP/APM/AM/AB/ACF/AICV/ACRC32 + APP_PHYSTS. Decoding the literal against
halmac_bit_8822c.h / halmac_bit_8822e.h shows ACRC32 (BIT8) and AICV (BIT9)
are not in it. The WMAC filter therefore dropped every FCS-failed frame no
matter how the config field was set, so the corrupted-frame retention the
fused-FEC salvage layer is built on never worked on this generation.

The fix gates the two bits on _cfg.rx.keep_corrupted, which is what Jaguar1
(RadioManagementModule.cpp), Jaguar2 (HalJaguar2.cpp) and the RTL8733B
(Halmac8733bMac::configure_monitor_rx) already do. Both misleading comments
are corrected, including a full decode of the base literal so the next reader
does not have to redo it.

Default behaviour is unchanged: keep_corrupted defaults to false, so an
unconfigured Jaguar3 monitor session programs exactly the RCR it programmed
before this patch.

The separate | (1u << 28) for APP_PHYSTS is dropped: bit 28 is already
inside the leading 0xF nibble of the base literal, and ORing it again read
as if it were being added on top.

One shipped tool does change: doctor sets keep_corrupted unconditionally,
so on Jaguar3 its printed rx_frames_crc goes from structurally-always-0 to a
real count. The HEALTHY/SUSPECT verdict reads rx_frames_ok separately and
does not move.

Also widens the keep_corrupted doc comment in DeviceConfig.h, which said
"Jaguar1 + Jaguar2": it now names Jaguar3 and the RTL8733B, and states that
Kestrel has no port and the knob is inert there.

Validation

tests/keep_corrupted_j3_ab.sh (added here): one rxdemo cell per arm on the
same channel, host counting rx.corrupt events with the descriptor's
crc_err bit set (DEVOURER_RX_DUMP_ALL=1).

DUT	channel	knob off	knob on
RTL8812CU (0bda:c812)	6	2894 frames / 0 crc_err	4152 frames / 1258 crc_err (2894 good)
RTL8812EU (0bda:a81a)	36	47351 frames / 0 crc_err	41726 frames / 1918 crc_err

One unit of each die, one bench, single cells. The 8812EU unit on this bench
hears nothing on 2.4 GHz with or without the knob, so its 2.4 GHz cell is
uninformative rather than a counter-example. The original author's 8812EU
A/B (crc_err non-zero only with the bits set) agrees.

No headless selftest: monitor_rx_cfg writes straight to the device with no
seam to assert against, which is also why the Jaguar1 and Jaguar2 equivalents
have none. cmake --build + ctest green (66/66).

Note the Jaguar1 measurement in docs/pseudo-preamble-puncturing.md ("zero
FCS-failed frames" under a jammed slice) is unaffected — that experiment used
an 8814AU receiver, whose path already gated correctly.

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

PR Summary by Qodo

Honor keep_corrupted in Jaguar3 monitor receive configuration

🐞 Bug fix 📝 Documentation 🕐 10-20 Minutes

AI Description

• Enables Jaguar3 WMAC acceptance of CRC32 and ICV-failed frames when configured.
• Preserves existing monitor behavior when corrupted-frame retention remains disabled.
• Documents supported chip families and Kestrel's inert configuration behavior.

Diagram

graph TD
  CFG["RX config"] --> MON["Jaguar3 monitor"] --> KEEP{"Keep corrupted?"}
  KEEP -->|No| BASE["Base RCR"] --> WMAC["WMAC filter"] --> HOST["Host RX"]
  KEEP -->|Yes| BITS["Add error bits"] --> WMAC

Loading

High-Level Assessment

The conditional RCR-bit approach is optimal because it matches Jaguar1, Jaguar2, and RTL8733B behavior while preserving Jaguar3's default register value. Always enabling corrupted-frame delivery was considered but would change default filtering semantics and increase unwanted host traffic.

Files changed (2) +18 / -5

Bug fix (1) +16 / -4

HalJaguar3.cppConditionally accept corrupted Jaguar3 monitor frames +16/-4

Conditionally accept corrupted Jaguar3 monitor frames

• Builds the Jaguar3 monitor RCR dynamically and adds ACRC32 and AICV only when rx.keep_corrupted is enabled. Clarifies the base RCR decode, hardware filtering behavior, and descriptor error reporting while retaining the previous default value.

src/jaguar3/HalJaguar3.cpp

Documentation (1) +2 / -1

DeviceConfig.hDocument expanded keep_corrupted chipset support +2/-1

Document expanded keep_corrupted chipset support

• Extends the configuration comment to include Jaguar3 and RTL8733B support. It also explicitly documents that the option remains inert on Kestrel.

src/DeviceConfig.h

[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. MediaTek users miss a working option ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

DeviceConfig::Rx::keep_corrupted now lists four supported hardware families but omits the
supported MediaTek MT7612U backend. Mt7612uRadio forwards this setting to
mt7612u_set_monitor_rx, which changes the receive filter, so readers may incorrectly conclude that
the option is unavailable on that hardware.

Code

src/DeviceConfig.h[R120-121]

+     * at the WMAC filter (sets RCR ACRC32|AICV). Jaguar1, Jaguar2, Jaguar3 and
+     * the RTL8733B; not ported on Kestrel, where it is silently inert. */

Evidence

Rule 3 requires documented hardware and feature boundaries to match backend implementations. The
changed support list omits MediaTek MT7612U, while its radio passes cfg.rx.keep_corrupted into a
filter function that conditionally accepts CRC-error frames.

CLAUDE.md: Preserve Supported Hardware and Feature Boundaries: CLAUDE.md: Preserve Supported Hardware and Feature Boundaries: CLAUDE.md: Preserve Supported Hardware and Feature Boundaries: CLAUDE.md: Preserve Supported Hardware and Feature Boundaries: CLAUDE.md: Preserve Supported Hardware and Feature Boundaries: CLAUDE.md: Preserve Supported Hardware and Feature Boundaries: CLAUDE.md: Preserve Supported Hardware and Feature Boundaries: CLAUDE.md: Preserve Supported Hardware and Feature Boundaries
src/DeviceConfig.h[118-122]
src/mt7612u/Mt7612uRadio.cpp[407-414]
src/mt7612u/init.cpp[560-569]

Agent prompt

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

## Issue description
The updated `keep_corrupted` documentation omits MediaTek MT7612U even though that backend applies the setting to its monitor receive filter.
## Fix Focus Areas
- src/DeviceConfig.h[118-121]
## Recommended Fix
Add MediaTek MT7612U to the documented list of supported backends while retaining the explicit statement that Kestrel leaves the setting inert.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. A/B trials can report empty captures ✓ Resolved 🐞 Bug ≡ Correctness

Description

run_cell constructs its environment without forcing DEVOURER_EVENTS=stdout, even though it
counts events only from the redirected standard-output log. When the caller exports
DEVOURER_EVENTS=off or DEVOURER_EVENTS=stderr, capture events are disabled or written to the
error log and both trial counts are reported as zero.

Code

tests/keep_corrupted_j3_ab.sh[R25-26]

+  local env=(-u DEVOURER_RX_KEEP_CORRUPTED
+             DEVOURER_PID="$PID" DEVOURER_CHANNEL="$CH" DEVOURER_RX_DUMP_ALL=1 DEVOURER_LOG_LEVEL=warn)

Evidence

The script redirects standard output to $log and counts only that file, while apply_logging_env
disables events for off and sends them to standard error for stderr. Other hardware test scripts
explicitly set DEVOURER_EVENTS=stdout when parsing redirected event output.

tests/keep_corrupted_j3_ab.sh[25-28]
tests/keep_corrupted_j3_ab.sh[40-43]
examples/common/env_config.cpp[303-310]
tests/kestrel_beacon_onair.sh[56-56]

Agent prompt

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

## Issue description
The A/B script inherits `DEVOURER_EVENTS`, allowing callers to disable events or redirect them away from the file that the script counts.
## Fix Focus Areas
- tests/keep_corrupted_j3_ab.sh[25-28]
## Recommended Fix
Add `DEVOURER_EVENTS=stdout` to the environment passed to `rxdemo`, ensuring `rx.corrupt` events always reach the standard-output log used for counting.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. The off trial can retain bad frames ✓ Resolved 🐞 Bug ≡ Correctness

Description

run_cell omits DEVOURER_RX_KEEP_CORRUPTED for the off arm instead of explicitly setting it to
zero or removing it from the environment. When the caller exports a nonzero value, the first arm
inherits it and both trials run with corrupted-frame retention enabled.

Code

tests/keep_corrupted_j3_ab.sh[R23-24]

+  local env=(DEVOURER_PID="$PID" DEVOURER_CHANNEL="$CH" DEVOURER_RX_DUMP_ALL=1 DEVOURER_LOG_LEVEL=warn)
+  [ "$knob" = 1 ] && env+=(DEVOURER_RX_KEEP_CORRUPTED=1)

Evidence

The script adds the variable only when knob equals one, while the shared environment parser treats
every present value other than 0 as enabled. The resulting configuration directly controls the
newly added Jaguar3 receive-control bits.

tests/keep_corrupted_j3_ab.sh[20-25]
examples/common/env_config.cpp[11-17]
examples/common/env_config.cpp[51-55]
src/jaguar3/HalJaguar3.cpp[496-498]

Agent prompt

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

## Issue description
The off arm inherits `DEVOURER_RX_KEEP_CORRUPTED` from the caller because it only supplies the variable for the on arm, allowing both trials to run with retention enabled.
## Fix Focus Areas
- tests/keep_corrupted_j3_ab.sh[20-25]
## Recommended Fix
Include `DEVOURER_RX_KEEP_CORRUPTED="$knob"` in the environment array for every trial, relying on the existing `0` and `1` flag semantics to make each arm deterministic.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Failed trials look like valid results ✓ Resolved 🐞 Bug ☼ Reliability

Description

run_cell suppresses the background process status with wait "$p" ... || true and proceeds to
count its output unconditionally. If device discovery, interface claiming, or driver creation fails,
the script sleeps and reports zero frames with a successful exit instead of distinguishing failed
startup from a valid capture.

Code

tests/keep_corrupted_j3_ab.sh[28]

+  kill -INT "$p" 2>/dev/null || true; wait "$p" 2>/dev/null || true

Evidence

The new script ignores both kill and wait failures and always prints counts from the log.
rxdemo has several normal startup error paths that return one before receiving any frames,
including missing devices, failed interface claims, and unavailable drivers.

tests/keep_corrupted_j3_ab.sh[25-32]
examples/rx/main.cpp[1418-1424]
examples/rx/main.cpp[1450-1465]

Agent prompt

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

## Issue description
The validation script discards `rxdemo` failures and converts unsuccessful captures into apparently valid zero-frame measurements.
## Fix Focus Areas
- tests/keep_corrupted_j3_ab.sh[25-32]
## Recommended Fix
Check that `rxdemo` is still running before sending SIGINT, preserve and validate its `wait` status after shutdown, and return a nonzero status with the stderr-log location when startup or capture fails. Only calculate and print counts after a successful capture.

ⓘ 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 enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[josephnef]

Correct fix, and the decode in the new comment holds up.

I checked 0xF410400F | BIT28 against reference/rtl88x2cu/hal/halmac/halmac_bit_8822c.h:15954-15986: APP_FCS(31) APP_MIC(30) APP_ICV(29) APP_PHYSTS(28) VHT_DACK(26) PKTCTL_DLEN(20) HTC_LOC_CTRL(14) AB(3) AM(2) APM(1) AAP(0). ACRC32(8) and AICV(9) are genuinely absent, and the old comment's ACF was never in the literal either — the correction is right on both counts.

Downstream is wired, so the RCR bits really are the whole gate: FrameParserJaguar3.h:242-243 decodes crc_err/icv_err, RtlJaguar3Device.cpp:299-300 propagates them into RxAtrib, and the RX loop delivers the frame to the packetProcessor without a software drop (only the PHY-status fold at :343 gates on crc_err). The fix does reach the salvage layer.

Two things beyond the inline note:

A behaviour change the description doesn't cover. examples/doctor/main.cpp:229 sets cfg.rx.keep_corrupted = true unconditionally. On Jaguar3 the doctor's rx_frames_crc goes from structurally-always-0 to a real count. No verdict flip — ClassifyAdapterHealth reads rx_frames_ok separately — but the printed report changes on this generation, and "default behaviour is unchanged" reads as covering the shipped tools. Worth a line in the body.

Needs a rebase. The branch is BEHIND master; #415 renamed IRtlDevice→IRadio. Trivial here since this diff touches neither.

No objection to merging once rebased.

[josephnef]

/review

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

Code review by qodo was updated up to the latest commit 4c1ecc4

[josephnef]

/review

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

Code review by qodo was updated up to the latest commit c0744c7

[josephnef]

/review

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

Code review by qodo was updated up to the latest commit 7352dc0