Jaguar1/2/3: unified TX retry-limit knob (DEVOURER_TX_RETRY_LIMIT, default 0)

[vertexodessa]

Problem

All three Jaguar generations hardcode a per-frame TX retry limit of 12:

• Jaguar1 (src/jaguar1/RtlJaguarDevice.cpp): SET_TX_DESC_DATA_RETRY_LIMIT_8812(..., 12) (plain + A-MPDU paths)
• Jaguar2/3 (src/jaguar2/RtlJaguar2Device.cpp, src/jaguar3/RtlJaguar3Device.cpp): the fill_data_tx_desc_* builders hardcode RTS_DATA_RTY_LMT = 12, then nothing overrides it

With RETRY_LIMIT_ENABLE=1, on a busy half-duplex link (bidirectional monitor-mode FPV) the chip retries every frame up to 12× on collision with the continuous downlink. TX airtime explodes, the receiver's half-duplex blind windows drop consecutive video fragments — the one loss pattern the downlink FEC cannot repair (k=11/n=15, max 4). Observed as ~20-30 non-recoverable losses/s when enabling the uplink (adaptive link) on the ground station; with retry=0 losses drop to zero.

Change

• Add cfg.tx.retry_limit (DEVOURER_TX_RETRY_LIMIT, default 0) to DeviceConfig::Tx
• Jaguar1: write cfg.tx.retry_limit into the TX descriptor instead of 12 (plain + A-MPDU paths)
• Jaguar2/3: override RTS_DATA_RTY_LMT after the builder with cfg.tx.retry_limit and re-run the txdesc checksum (the field lives inside the checksummed span)

Default 0 = no MAC retries, matching WFB semantics (reliability comes from FEC, not MAC retries). Verified on-air: 8812AU ground station, alink enabled, 0 lost packets; drone side rebuilt with the same knob.

Notes

• Kestrel is untouched: its retry is firmware-level, different architecture.
• DEVOURER_TX_RETRY_LIMIT now works uniformly across jaguar1/2/3 on both link ends.

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

PR Summary by Qodo

Unify Jaguar1/2/3 TX retry limit via DEVOURER_TX_RETRY_LIMIT (default 0)

🐞 Bug fix ✨ Enhancement ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Add cfg.tx.retry_limit knob (DEVOURER_TX_RETRY_LIMIT) to control per-frame MAC retries.
• Apply retry_limit across Jaguar1 and Jaguar2/3 TX descriptors, including A-MPDU paths.
• Recompute Jaguar2/3 TX descriptor checksum after overriding checksummed retry fields.

Diagram

graph TD
  env{{"DEVOURER_TX_RETRY_LIMIT"}} --> cfg["DeviceConfig::Tx.retry_limit"]
  cfg --> j1["Jaguar1 build_tx_block"] --> set1["Set DATA_RETRY_LIMIT"] --> txd[("TX descriptor")]
  cfg --> j23["Jaguar2/3 build_tx_block"] --> set23["Set RTS_DATA_RTY_LMT + enable"] --> csum("Recompute txdesc checksum") --> txd
  subgraph Legend
    direction LR
    _env{{"Env var"}} ~~~ _cfg["Config" ] ~~~ _step("Processing step") ~~~ _txd[("TX desc")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Gate retry-limit enable bits when retry_limit==0

• ➕ Avoids enabling retry-limit control when intentionally using zero retries
• ➕ Reduces risk of subtle chip/firmware interactions tied to enable flags
• ➖ Behavior may diverge across chip generations (Jaguar1 already sets enable=1)
• ➖ Requires validating default/implicit hardware behavior per chipset

2. Plumb retry_limit into Jaguar2/3 fill_data_tx_desc_* builders

• ➕ Keeps descriptor construction in one place (no post-build override)
• ➕ Checksum can be computed once at the end with fewer mutations
• ➖ Broader refactor surface if builders are shared across modes/paths
• ➖ Higher regression risk than a localized post-override + re-checksum

Recommendation: The PR’s approach (post-override Jaguar2/3 retry fields followed by checksum recomputation, and direct Jaguar1 field write) is a good balance of minimal change and correctness. Consider a small follow-up to optionally clear/skip RTY_LMT_EN/RETRY_LIMIT_ENABLE when retry_limit==0 if chipset behavior warrants it, but the current default of 0 already achieves the intended “no retries” semantics.

Files changed (4) +25 / -5

Bug fix (3) +19 / -5

RtlJaguarDevice.cppUse cfg.tx.retry_limit for Jaguar1 DATA_RETRY_LIMIT (plain + A-MPDU) +5/-3

Use cfg.tx.retry_limit for Jaguar1 DATA_RETRY_LIMIT (plain + A-MPDU)

• Replaces the hardcoded retry limit of 12 with the configured retry_limit for non-8814A paths. Updates both the normal and A-MPDU descriptor setup to honor the unified knob while preserving no-ack behavior (retry=0).

src/jaguar1/RtlJaguarDevice.cpp

RtlJaguar2Device.cppOverride Jaguar2 RTS_DATA_RTY_LMT from cfg and re-checksum TX descriptor +7/-1

Override Jaguar2 RTS_DATA_RTY_LMT from cfg and re-checksum TX descriptor

• After descriptor building, forces RTY_LMT_EN and sets RTS_DATA_RTY_LMT to cfg.tx.retry_limit, then recomputes the TX descriptor checksum (fields are within the checksummed span). Aligns A-MPDU retry handling with the same knob.

src/jaguar2/RtlJaguar2Device.cpp

RtlJaguar3Device.cppOverride Jaguar3 RTS_DATA_RTY_LMT from cfg and re-checksum TX descriptor +7/-1

Override Jaguar3 RTS_DATA_RTY_LMT from cfg and re-checksum TX descriptor

• Mirrors the Jaguar2 fix for 8822C: enable retry-limit control, set RTS_DATA_RTY_LMT from cfg.tx.retry_limit, and recompute checksum to keep the descriptor valid. Updates the A-MPDU path to use the same configured value.

src/jaguar3/RtlJaguar3Device.cpp

Other (1) +6 / -0

DeviceConfig.hAdd cfg.tx.retry_limit (DEVOURER_TX_RETRY_LIMIT) with default 0 +6/-0

Add cfg.tx.retry_limit (DEVOURER_TX_RETRY_LIMIT) with default 0

• Introduces a new TX config field to control the per-frame hardware retry limit (0..63). Documents mapping to TX descriptor bits and rationale for defaulting to 0 to avoid half-duplex airtime blowups.

src/DeviceConfig.h

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

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

1. Retry env ignored 🐞 Bug ≡ Correctness

Description

The code documents DEVOURER_TX_RETRY_LIMIT but devourer_config_from_env() never reads it, so setting
the env var has no effect and cfg.tx.retry_limit remains the default (0) unless set
programmatically.

Code

src/DeviceConfig.h[R178-183]

+    /* env: DEVOURER_TX_RETRY_LIMIT — per-frame hardware retry limit (0..63).
+     * Maps to the TX descriptor DATA_RETRY_LIMIT / RTS_DATA_RTY_LMT field
+     * (Dword4 bits 18-23). 0 = no retries (WFB default: FEC provides
+     * reliability, not MAC retries). On a busy half-duplex link retries flood
+     * the air and blind the receiver. */
+    int retry_limit = 0;

Evidence

DeviceConfig now advertises an env var for the knob, but the only env->DeviceConfig mapper used by
examples does not set it, so the knob cannot be controlled via the environment as described.

src/DeviceConfig.h[173-184]
examples/common/env_config.cpp[88-109]

Agent prompt

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

### Issue description
`DeviceConfig::Tx::retry_limit` is documented as `env: DEVOURER_TX_RETRY_LIMIT`, but the environment-to-config loader (`devourer_config_from_env`) does not populate it. This makes the advertised knob non-functional for all example binaries relying on `devourer_config_from_env()`.

### Issue Context
The repository uses `examples/common/env_config.cpp` to translate `DEVOURER_*` environment variables into a `devourer::DeviceConfig`. Many TX knobs are wired there, but `DEVOURER_TX_RETRY_LIMIT` is missing.

### Fix Focus Areas
- examples/common/env_config.cpp[88-109]
- src/DeviceConfig.h[173-184]

### Suggested fix
1. In the TX section of `devourer_config_from_env()`, read `DEVOURER_TX_RETRY_LIMIT` (using `env_long`).
2. Clamp/validate to `[0, 63]` and assign into `cfg.tx.retry_limit`.
3. (Optional) If the provided value is out of range, log/warn in the caller binaries or document the clamping behavior.

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

---

2. Retry limit wraps silently 🐞 Bug ☼ Reliability

Description

TX descriptor builders write _cfg.tx.retry_limit directly into a 6-bit field; values outside 0..63
will be masked (wraparound) by SET_BITS_TO_LE_4BYTE, potentially producing an unintended retry
limit.

Code

src/jaguar2/RtlJaguar2Device.cpp[R1535-1537]

+  SET_TX_DESC_RTY_LMT_EN_8822B(out, 1);
+  SET_TX_DESC_RTS_DATA_RTY_LMT_8822B(out, _cfg.tx.retry_limit);
+  jaguar2::cal_txdesc_chksum_8822b(out);

Evidence

The retry limit setters target a 6-bit field; the shared bitfield writer masks the value to the
bit-length, which causes wraparound for out-of-range ints passed from cfg.

src/jaguar2/RtlJaguar2Device.cpp[1532-1537]
src/TxDescBits.h[38-54]
src/jaguar2/FrameParserJaguar2.h[63-67]

Agent prompt

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

### Issue description
`_cfg.tx.retry_limit` is an unconstrained `int`, but it is written into 6-bit TX-descriptor fields. The underlying bitfield setter masks to the field width, so out-of-range values silently wrap modulo 64 (e.g., 64 -> 0, -1 -> 63), which can unexpectedly enable maximum retries.

### Issue Context
This affects Jaguar1/2/3 because they all use the same 6-bit field width for retry limit. Even once env parsing is added, invalid inputs should not silently change semantics.

### Fix Focus Areas
- src/DeviceConfig.h[173-184]
- src/jaguar2/RtlJaguar2Device.cpp[1532-1537]
- src/TxDescBits.h[38-54]
- src/jaguar2/FrameParserJaguar2.h[63-67]

### Suggested fix
1. Validate and clamp `retry_limit` to `[0, 63]` at the boundary:
  - Preferably when ingesting configuration (e.g., env/CLI parsing), and/or
  - Defensively at descriptor construction (compute `uint8_t r = clamp(...)` and use `r`).
2. Consider changing the type to `uint8_t` (or `std::uint8_t`) to make the allowed range explicit.
3. If clamping occurs, optionally emit a warning via the logger to avoid silent misconfiguration.

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

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[josephnef]

Mechanism verified on hardware — works exactly as described on all three generations. But the default-0 flip silently breaks the shipped hardware-ARQ feature and its test matrix, so this needs the test/docs consequences carried in the same PR (or default 12).

On-air A/B (off-phase: unanswered unicast QoS + DEVOURER_TX_REPORT, ch36, no responder)

Cell	master (a71060f)	PR default	PR + DEVOURER_TX_RETRY_LIMIT=12
j2-8812BU (2357:012d)	retries_mean 12.00 / max 12 (500 rpts)	0.00 / 0	12.00 / 12
j3-8822CU (0bda:c812)	12.00 / 12 (1282 rpts)	0.00 / 0	12.00 / 12
j1-8821AU (2357:0120)	12	0	12

So: the knob works, the override lands in the descriptor on every generation, and =12 restores master behavior bit-for-bit. Headless ctest: 48/48 pass on the PR build.

Code mechanics are sound: the jaguar2/3 post-builder override + re-checksum is idempotent (cal_txdesc_chksum_* zeroes the field before XOR) and matches the existing tx.report / A-MPDU mutation-block pattern; the env layer clamps 0..63 and the 6-bit descriptor macros mask direct-API overflow.

Blocking: default 0 regresses hardware ARQ + fails the ACK test matrix

With retry_limit = 0 by default:

• The hardware-ACK closed loop (SetAckResponder + unicast TA — autonomous MAC retransmission until ACK, docs/scheduled-mac.md) silently loses retransmission. That's a shipped, bench-validated capability, not an accident of the hardcoded 12.
• tests/ack_txreport_matrix.sh now fails: ack_txreport_analyze.py hard-asserts off-phase retries_max >= 8 and pinned == 12 — the table above shows those go to 0.

Default 0 is defensible for the FPV/WFB use case (same spirit as DEVOURER_DIS_CCA defaulting on for streamtx — the link owns its reliability via FEC). But then the PR should also:

1. Update tests/ack_txreport_matrix.sh to pass DEVOURER_TX_RETRY_LIMIT=12 into the TX env (and let ack_txreport_analyze.py take the expected pin as a parameter rather than a literal 12).
2. Fix the docs/scheduled-mac.md wording ("retries pinned at the descriptor limit") to name the knob.
3. Extend the DeviceConfig.h doc comment: hardware-ARQ (SetAckResponder) needs a nonzero value; and the knob is inert on Kestrel (firmware-level retry) and on the 8814A die (the vendor's DATA_RETRY_LIMIT=0 carve-out is kept) — so "uniform across jaguar1/2/3" needs that asterisk.

Alternative that avoids all three: keep the default at 12 and let WFB deployments set DEVOURER_TX_RETRY_LIMIT=0 explicitly. Either resolution is fine; shipping the default flip without the test/docs updates is not.

Note: overlap with #334

#334 reaches the same end state (retry 0) via the radiotap NOACK flag, jaguar3-only, with no opt-out (every devourer demo sets NOACK unconditionally) and an extra EN_HWSEQ flip. This PR's explicit knob is the cleaner mechanism and effectively supersedes #334's retry half — the two conflict on the same descriptor lines and should be reconciled, not both merged.

🤖 Generated with Claude Code

[vertexodessa]

Thanks for the on-air A/B and the clear resolution. Default stays 0 (WFB semantics — FEC owns reliability); the three follow-ups are in the branch:

1. tests/ack_txreport_matrix.sh runs TX with DEVOURER_TX_RETRY_LIMIT=12 (overrideable via RETRY_LIMIT), and ack_txreport_analyze.py takes the expected pin as --expect-retries (default 12) instead of a literal — the off-phase still pins and still fails loudly if retries vanish.
2. docs/scheduled-mac.md names the knob in the "pinned at the descriptor limit" wording and notes it's now the single source of truth; contract item 2 states ARQ needs a nonzero limit.
3. DeviceConfig.h now documents that SetAckResponder ARQ needs a nonzero value and the knob is inert on Kestrel and the 8814A die.

[josephnef]

All three follow-ups verified on the new head (7abd756) — approving.

• Descriptor/env code is byte-identical to the head I hardware-validated (the delta is docs + tests + the DeviceConfig.h comment only), so the on-air A/B table stands: master 12.00 → PR-default 0.00 → =12 12.00 on j1/j2/j3.
• Updated matrix passes end-to-end on hardware (j3-8822CU cell, 8814AU responder, ch36, PR build): on ack_rate=1.0 retries_mean=0.21, retarget 1.0 / 0.2, off ack_rate=0.0 retries_mean=12.00 retries_max=12 — all capability_ok=true. The hardware-ARQ contract is preserved with the knob as the single source of truth.
• ack_txreport_analyze.py --selftest passes incl. the new custom-pin cases; headless ctest 48/48.
• Qodo's "env ignored" finding is stale — it reviewed an intermediate commit; the current head wires DEVOURER_TX_RETRY_LIMIT in env_config.cpp with the 0..63 clamp.

Two non-blocking nits, take or leave:

1. RETRY_LIMIT=0 makes the off-phase assertion retries_max >= 0 — trivially true, so a zero-pin run can't detect a dead report path via retries. Operator-chosen, so fine, but a one-line guard (refuse RETRY_LIMIT=0 or warn) would keep the phase meaningful.
2. Docs wording: "the value the descriptors used to hardcode" / "is now the single source of truth" is historical phrasing — this repo's docs convention is current-state-only (git is the changelog). "the matrix runs 12" alone carries it.

🤖 Generated with Claude Code