tx: TxMode::no_agg keeps a frame out of an A-MPDU (Jaguar3)

[gilankpam]

The problem

Under SetAmpduMode the MAC folds consecutive co-queued data frames into one PPDU aired at the first MPDU's rate and bandwidth. A frame's own MCS/BW/LDPC/STBC are silently replaced whenever it lands behind a frame of another rate. A caller mixing rates in one aggregated stream (for example a robust control frame among video, or a probe at another rate) cannot rely on the rate it asked for.

The change

• TxMode::no_agg (rate grammar token /NOAGG) is carried per frame in a devourer-private radiotap TX_FLAGS bit, kRadiotapTxFlagNoAgg = 0x0100 (src/RadiotapTxFlags.h).
  • Radiotap assigns only 0x0001–0x0020, so a future registration of 0x0100 would collide; the declaration says so.
  • The HT radiotap stays 13 bytes, and a default TxMode is byte-identical.
• Jaguar3 reads the bit in build_tx_block and writes the descriptor AGG_EN=0 + BK=1 (dword2[16]) after the A-MPDU overrides.
  • This is the vendor rtl8822eu_xmit.c recipe for data frames it does not aggregate (EAPOL/ARP/DHCP included).
  • The frame stays on the same queue, so ordering is unchanged.
• AdapterCaps::tx_no_agg_ok (adapter.caps field tx_no_agg) advertises support: true on Jaguar3 only. Jaguar1/2, Kestrel, the RTL8733B and the MT7612U ignore the bit, and their caps say so.
• txdemo DEVOURER_TX_ALT_RATE=<spec>: odd-counter QoS frames carry their own rate radiotap from this spec, and even frames keep the default. The witness separates them by rx.seq pctr parity.

Measured

tests/tx_no_agg_onair.sh: one RTL8812EU transmitting, an RTL8812EU witness, ch36, A-MPDU 0/6, 4 senders, 1000 B, MCS5 and MCS0 alternating by frame counter.

arm	MCS0 frames aired at MCS0	MCS5 frames at MCS5	witness heard
no flag (×2)	40.3 % / 40.4 %	100 %	2372 fps
/NOAGG (×2)	100.0 % / 100.0 %	100 %	1250 fps
/NOAGG, descriptor write disabled	39.8 %	100 %	2391 fps

The counterpart: a flagged frame breaks the aggregate around it. Flagging every other frame, this harness's worst case, cut the witness's heard rate by 47 %.

The script aborts rather than passes when the unflagged control arm shows no fold, because then no mixed aggregates formed and nothing was tested.

Headless: radiotap_noagg covers every builder layout, with NOACK both ways, plus the /NOAGG parse. Removing the builder OR or the parse token fails it. ctest: full 76/76, without Jaguar3 76/76, Jaguar3-only 67/67.

Not covered

• The cost of occasional flagged frames, the realistic case (a control frame now and then among aggregated video). Only the every-other-frame worst case was measured.
• The 8822C: it shares the descriptor recipe and was not measured on air.
• The 20/40 MHz mixed-width case: the same fold applies to bandwidth, but this harness scores rate only.
• Porting to Jaguar1/2 and the MT7612U. Their A-MPDU paths exist, but no equivalent descriptor write was implemented or measured, so tx_no_agg_ok stays false there.

🤖 Generated with Claude Code

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

PR Summary by Qodo

Honor per-frame NOAGG requests on Jaguar3 transmitters

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Add /NOAGG so Jaguar3 frames retain their requested rate within mixed-rate transmit streams.
• Advertise adapter support and provide alternating-rate demo traffic for on-air validation.
• Add headless and hardware tests; document aggregation behavior and its throughput cost.

Diagram

graph TD
  M["TxMode /NOAGG"] --> B["Radiotap builder"] --> F["TX_FLAGS bit"] --> J["Jaguar3 descriptor"] --> D{"NOAGG set?"}
  D -->|yes| O["Disable aggregation"] --> P["Standalone PPDU"]
  D -->|no| A["Normal A-MPDU"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use a radiotap vendor namespace

• ➕ Avoids a possible future collision with a registered TX_FLAGS bit.
• ➖ Changes header layouts and lengths, and requires additional iterator handling across transmit paths.

Recommendation: Keep the private TX_FLAGS bit for this PR: it preserves existing header layouts and gives Jaguar3 a per-frame signal without changing queues or ordering. The documented collision risk warrants revisiting a vendor namespace if this wire format needs long-term interoperability.

Files changed (15) +414 / -20

Enhancement (8) +92 / -15

caps_event.hEmit transmit NOAGG support in adapter capabilities +1/-0

Emit transmit NOAGG support in adapter capabilities

• Reports 'AdapterCaps::tx_no_agg_ok' as 'tx_no_agg' in the shared capability event.

examples/common/caps_event.h

main.cppGenerate alternating per-frame rates in txdemo +36/-13

Generate alternating per-frame rates in txdemo

• Adds 'DEVOURER_TX_ALT_RATE' to give odd-counter QoS frames their own rate radiotap. A shared stamping helper selects the frame variant in single, batched, and parallel sending paths.

examples/tx/main.cpp

AdapterCaps.hAdd a transmit NOAGG capability flag +9/-0

Add a transmit NOAGG capability flag

• Introduces 'tx_no_agg_ok', defaulting to false so unsupported adapters do not promise per-frame aggregation control.

src/AdapterCaps.h

RadiotapBuilder.cppParse and encode per-frame NOAGG requests +5/-2

Parse and encode per-frame NOAGG requests

• Parses '/NOAGG' into 'TxMode' and ORs its private TX_FLAGS bit into generated radiotap headers independently of NOACK.

src/RadiotapBuilder.cpp

RadiotapTxFlags.hDefine the private radiotap NOAGG bit +14/-0

Define the private radiotap NOAGG bit

• Adds the '0x0100' TX_FLAGS constant and a bit-test helper. Documents that a future radiotap assignment could collide with this private value.

src/RadiotapTxFlags.h

TxMode.hAdd a per-frame aggregation opt-out to TxMode +7/-0

Add a per-frame aggregation opt-out to TxMode

• Adds 'no_agg', defaulting to false to preserve existing radiotap output unless requested.

src/TxMode.h

FrameParserJaguar3.hExpose Jaguar3's descriptor break bit +6/-0

Expose Jaguar3's descriptor break bit

• Defines the BK descriptor-field setter used to prevent a frame from merging with queue neighbors.

src/jaguar3/FrameParserJaguar3.h

RtlJaguar3Device.cppHonor NOAGG in Jaguar3 transmit descriptors +14/-0

Honor NOAGG in Jaguar3 transmit descriptors

• Advertises support, reads the per-frame TX_FLAGS bit, and writes 'AGG_EN=0' plus 'BK=1' after A-MPDU overrides. The frame remains on its existing queue.

src/jaguar3/RtlJaguar3Device.cpp

Tests (3) +287 / -0

CMakeLists.txtRegister the headless NOAGG self-test +11/-0

Register the headless NOAGG self-test

• Builds and registers 'RadiotapNoaggSelftest' with CTest.

CMakeLists.txt

radiotap_noagg_selftest.cppGuard NOAGG radiotap encoding across rate layouts +98/-0

Guard NOAGG radiotap encoding across rate layouts

• Checks parsing and wire encoding for legacy, HT, VHT, and HE modes with NOACK both ways. Verifies that the new bit alone changes and header lengths remain unchanged.

tests/radiotap_noagg_selftest.cpp

tx_no_agg_onair.shMeasure NOAGG against mixed-rate on-air controls +178/-0

Measure NOAGG against mixed-rate on-air controls

• Runs flagged and unflagged alternating-rate QoS traffic against a passive witness and scores each frame's observed rate. Aborts if the control fails to demonstrate aggregation or if an arm lacks enough received frames.

tests/tx_no_agg_onair.sh

Documentation (4) +35 / -5

CLAUDE.mdDocument NOAGG and the alternating-rate demo option +8/-2

Document NOAGG and the alternating-rate demo option

• Adds '/NOAGG' to the transmit-rate grammar and explains its capability requirement. Documents 'DEVOURER_TX_ALT_RATE' and how witness receipts distinguish alternating frames.

CLAUDE.md

aggregation.mdExplain mixed-rate aggregation and NOAGG tradeoffs +20/-0

Explain mixed-rate aggregation and NOAGG tradeoffs

• Describes why aggregation can replace a frame's requested rate, Jaguar3's opt-out behavior, on-air results, and the measured worst-case throughput cost.

docs/aggregation.md

logging.mdDocument the tx_no_agg capability field +1/-1

Document the tx_no_agg capability field

• Adds 'tx_no_agg' to the documented 'adapter.caps' event fields.

docs/logging.md

RadiotapBuilder.hDescribe the expanded transmit-rate grammar +6/-2

Describe the expanded transmit-rate grammar

• Documents '/NOAGG' and its use of a separate private radiotap TX_FLAGS bit.

src/RadiotapBuilder.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. Witness details clutter the root guide ✓ Resolved

Description

The DEVOURER_TX_ALT_RATE entry in CLAUDE.md explains txdemo’s odd/even frame selection and the
witness’s parity-based test instead of keeping those details with the TX demo or test. A reader
looking for demo-local environment variables encounters test-specific mechanics in the root guide,
while tests/tx_no_agg_onair.sh already explains how the witness separates the frames.

Code

CLAUDE.md[R265-268]

+- `DEVOURER_TX_ALT_RATE=<rate spec>` — txdemo (with `DEVOURER_TX_QOS_DATA`):
+  odd-counter frames carry their own rate radiotap from this spec, even ones
+  keep the default; the witness splits them by `rx.seq` pctr parity
+  (`tests/tx_no_agg_onair.sh`).

Evidence

Rule 1 requires subtree-specific facts to live at the narrowest applicable scope. The added
root-guide entry describes txdemo frame parity and witness scoring, which the test script also
documents.

CLAUDE.md: Keep Cross-Cutting Documentation Narrow and Evidence Balanced: CLAUDE.md: Keep Cross-Cutting Documentation Narrow and Evidence Balanced: CLAUDE.md: Keep Cross-Cutting Documentation Narrow and Evidence Balanced: CLAUDE.md: Keep Cross-Cutting Documentation Narrow and Evidence Balanced: CLAUDE.md: Keep Cross-Cutting Documentation Narrow and Evidence Balanced: CLAUDE.md: Keep Cross-Cutting Documentation Narrow and Evidence Balanced: CLAUDE.md: Keep Cross-Cutting Documentation Narrow and Evidence Balanced: CLAUDE.md: Keep Cross-Cutting Documentation Narrow and Evidence Balanced
CLAUDE.md[265-268]
tests/tx_no_agg_onair.sh[10-20]

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 root guide adds txdemo- and witness-specific test mechanics to its environment-variable list.
## Fix Focus Areas
- CLAUDE.md[265-268]
- tests/tx_no_agg_onair.sh[10-20]
## Recommended Fix
Keep a brief variable reference in the root guide and leave the parity and witness explanation with the TX test documentation.

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

---

2. The default-rate no-aggregation flag does nothing ✓ Resolved

Description

build_tx_block initializes no_agg from radiotap TX_FLAGS but copies the other transmit settings
from _tx_mode_default without copying its no_agg value. When a rate-less QoS frame uses
DEVOURER_TX_RATE=MCS0/NOAGG or SetTxMode(no_agg=true) under SetAmpduMode, its descriptor can
retain AGG_EN=1 and join an aggregate at another frame’s rate.

Code

src/jaguar3/RtlJaguar3Device.cpp[R2491-2492]

+  /* Radiotap TX_FLAGS devourer-private no-aggregation bit (TxMode::no_agg). */
+  bool no_agg = false;

Evidence

CLAUDE.md documents /NOAGG for DEVOURER_TX_RATE and says the default mode is applied through
SetTxMode; txdemo passes that mode to SetTxMode and sends rate-less QoS frames whose radiotap
TX_FLAGS contains only NOACK. Jaguar3 reads no_agg from TX_FLAGS, but its rate-less fallback
copies the other transmit-mode fields without copying _tx_mode_default->no_agg, so the active
A-MPDU path can set AGG_EN=1.

src/jaguar3/RtlJaguar3Device.cpp[2586-2594]
src/jaguar3/RtlJaguar3Device.cpp[2515-2517]
CLAUDE.md[252-264]
examples/tx/main.cpp[1166-1168]
examples/tx/main.cpp[1071-1082]
examples/tx/main.cpp[1142-1181]
src/jaguar3/RtlJaguar3Device.cpp[2491-2517]
src/jaguar3/RtlJaguar3Device.cpp[2582-2597]
src/jaguar3/RtlJaguar3Device.cpp[2715-2742]

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 reads `no_agg` from per-frame radiotap TX_FLAGS but does not propagate it from `SetTxMode` when a rate-less frame uses the default transmit mode. Consequently, the documented `DEVOURER_TX_RATE=.../NOAGG` setting does not prevent those frames from aggregating.
## Fix Focus Areas
- src/jaguar3/RtlJaguar3Device.cpp[2491-2517]
- src/jaguar3/RtlJaguar3Device.cpp[2582-2597]
- src/jaguar3/RtlJaguar3Device.cpp[2735-2742]
## Recommended Fix
Inside the `if (!rate_from_radiotap && _tx_mode_default.has_value())` block, propagate the default setting with `no_agg = no_agg || _tx_mode_default->no_agg;`. Preserve the per-frame radiotap setting for frames with an explicit rate, and add a regression test for a rate-less QoS frame under an active A-MPDU session.

ⓘ 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 route each severity your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[josephnef]

Reviewed at 8619a75. Built clean, ctest 80/80 (the new radiotap_noagg included), vendor recipe cross-checked against reference/rtl88x2eu/hal/rtl8822e/usb/rtl8822eu_xmit.c (BK=1 for every data frame it does not aggregate, AGG_EN left 0) and the halmac SET_TX_DESC_BK bit position (dword2[16]). Mechanism and wire form are right.

Requesting one change: no_agg is dropped for rate-less frames

Qodo's bug 2 is real. In build_tx_block the rate-less fallback (src/jaguar3/RtlJaguar3Device.cpp ~L2586) copies fixed_rate/vht/sgi/ldpc/stbc/bwidth from _tx_mode_default but not no_agg, and no_agg is only ever set from a radiotap TX_FLAGS field. txdemo's QoS frame is rate-less by design (rate comes from SetTxMode), so DEVOURER_TX_RATE=MCS5/NOAGG under DEVOURER_TX_AMPDU_MODE still aggregates. CLAUDE.md and RadiotapBuilder.h now advertise /NOAGG in the DEVOURER_TX_RATE grammar, and that path is inert. The on-air harness never exercises it because only the ALT frames (which carry their own radiotap) get the flag.

Fix is one line in that block:

no_agg = no_agg || _tx_mode_default->no_agg;

(per-packet radiotap rate still wins, as for the other fields.) Worth a sentence in the TxMode::no_agg comment that the default applies to rate-less frames only.

Measured here: the 8822C die passes

You listed the 8822C as unmeasured. Ran tests/tx_no_agg_onair.sh unchanged on this rig with an RTL8812CU (chip-id 0x13, USB high-speed) transmitting and a Comfast CF-924AC (RTL8822BU, external antennas, the rig's qualified ground station) as witness. ch36, 0/6, 4 senders, 1000 B, MCS5/MCS0 alternating:

arm	odd (MCS0) at own rate	even (MCS5) at own rate	heard
noagg #1	100.0 %	100.0 %	1159 fps
control #2	23.8 %	100.0 %	2913 fps
noagg #3	100.0 %	100.0 %	1156 fps
control #4	24.6 %	100.0 %	2749 fps

Verdict tx_no_agg_ok: true. The fold is deeper here than on your 8812EU (24 % vs 40 % own-rate in the control) and the every-other-frame cost is correspondingly larger (−60 % heard vs your −47 %). Please fold that into the AdapterCaps::tx_no_agg_ok comment and docs/aggregation.md so the 8822C row reads as measured (one 8812CU unit) rather than "shares the recipe".

Could not repeat the 8812EU arm: this rig's bare BL-M8812EU2 module drops off the USB bus about one second after the TX bring-up reset (dmesg USB disconnect then re-enumeration; build/doctor grades it HEALTHY on EFUSE/fw/RX). That is a supply problem on this bench, unrelated to the diff. Your EU numbers stand as the only EU measurement.

Nits

• CLAUDE.md DEVOURER_TX_ALT_RATE entry: keep the var + one clause, move the parity/witness mechanics to the script header where they already are (Qodo's point 1, agree).
• stamp_counter is called unconditionally on the single-frame path and behind if (qos_stamp) on the batch path. Harmless since the helper checks qos_stamp itself, but pick one.

[gilankpam]

@josephnef all four items addressed in 7662ab6.

no_agg dropped for rate-less frames: confirmed and fixed with the one line you suggested, in the rate-less fallback of build_tx_block. The TxMode::no_agg comment now says the SetTxMode default applies to rate-less frames only. The harness gap you pointed out is closed too: tests/tx_no_agg_onair.sh has a basenoagg arm (DEVOURER_TX_RATE=$BASE/NOAGG, ALT unflagged), now in the default arm list. Measured on 8812EU → 8812EU, ch36, 0/6, 4 senders, 1000 B, MCS5/MCS0:

arm	odd (MCS0) at own rate	even (MCS5) at own rate	heard
noagg ×2	100.0 % / 100.0 %	100 %	749 / 1237 fps
control ×2	38.9 % / 38.7 %	100 %	2394 / 2402 fps
basenoagg ×2	100.0 % / 100.0 %	100 %	1231 / 1229 fps
basenoagg ×2, fix line removed	38.5 % / 40.1 %	100 %	1394 / 2297 fps
control, fix line removed	39.1 %	100 %	2087 fps

So the new arm fails on the bug and passes with the fix. The first noagg arm heard ~40 % fewer frames than the identical later one, but its percentages are clean. I didn't chase it further; it looks like witness warm-up. The EU numbers agree with the original run: ~40 % own-rate in the control, −48 % heard for every-other-frame flagging.

8822C: your 8812CU run is now in the AdapterCaps::tx_no_agg_ok comment and docs/aggregation.md as measured (one unit, your rig and witness), including the deeper fold (23.8 / 24.6 %) and the larger cost (~−60 % heard).

Nits: the CLAUDE.md DEVOURER_TX_ALT_RATE entry is down to one line. stamp_counter is now called unconditionally on the batch path as well, since the helper checks qos_stamp itself.

ctest 76/76.

[gilankpam]

@josephnef one more 8822C ask, if the rig is still set up. The basenoagg arm (rate-less base frames flagged through SetTxMode, the path fixed in 7662ab6) has only run on the 8812EU. No 8812CU is available here. The fix is in build_tx_block, which both dies share, and nothing in it branches by die, so I expect a pass. It is still unmeasured, though. Could you run it on your 8812CU?

ARMS="basenoagg control basenoagg" DUT_PID=<8812CU PID> WIT_PID=<witness PID> \
  sudo -E bash tests/tx_no_agg_onair.sh

Pass means both parities are at their own rate in basenoagg, with the control still folding. I'll fold the result into docs/aggregation.md.

[josephnef]

Re-reviewed at 7662ab6. The fallback line, the TxMode::no_agg comment, the basenoagg arm, the caps/aggregation.md update and both nits are as asked. Build clean, ctest 80/80.

basenoagg on the 8812CU, as requested. Same rig and witness as before (RTL8812CU transmitting on USB high-speed, CF-924AC RTL8822BU witness), ch36, 0/6, 4 senders, 1000 B, MCS5/MCS0, script unchanged:

arm	odd (MCS0) at own rate	even (MCS5) at own rate	heard
basenoagg #1	100.0 %	100.0 %	1189 fps
control #2	23.4 %	100.0 %	2995 fps
basenoagg #3	100.0 %	100.0 %	1183 fps

Verdict tx_no_agg_ok: true. The rate-less path through the SetTxMode default behaves identically to the per-frame radiotap path on the 8822C die: same 100 % own-rate, same ~−60 % heard for every-other-frame flagging, control fold unchanged from the 28 Sep run (23.4 % vs 23.8/24.6 %). Fold it into docs/aggregation.md as you offered; nothing else blocking.

Approving.