DEVOURER_TX_RETRY_FALLBACK=off: pin retries at the descriptor rate; floor form measured and rejected (#362)

[josephnef]

Closes #362 — with this, all four of the issue's acceptance items are met: the retry-limit curve and FEC dimensioning landed in #367 (steps 1+4), and steps 2–3 now have measured go/no-go with vendor citations plus the step-2 knob implemented.

What the measurements found (recorded sweep data + three fresh arms)

The fw fallback ladder is already on — every generation's inject path leaves DISDATAFB/DISABLE_FB at 0, and retried frames measurably step down: MCS3 → 54M → 24M → 18M → 9M → 6M, ~10% of retried frames finishing below the original rate. (My first issue comment quoted the vendor's injection path, which hard-disables fallback — devourer diverged; and a claimed J1 asymmetry was an NDPA-branch misread. Both corrected on the issue.)

arm (retry_limit 8, ~60 k reports)	retried final_rate	verdict
default	4×57, 5×13, 7×17, 8×37, 11×51, 15×1240	ladder confirmed
off	15×1200 — all pinned	knob works exactly
MCS0 floor	15×24,642, 45×6,744 (= VHT1SS_MCS1!), 20× retry inflation, −1 pt delivery	rejected

What ships

• DEVOURER_TX_RETRY_FALLBACK=off (DeviceConfig::Tx::retry_fallback) on all three generations — sets DISDATAFB/DISABLE_FB so retries re-air at the descriptor DATARATE, for constant-rate links where a 6M re-air of an MCS3 frame costs ~4× airtime. Unset = byte-identical current behaviour.
• Deliberately no floor form: DATA_RTY_LOWEST_RATE is not a plain DESC_RATE bound on this fw — with the inject path's RA-group 9 the fw reinterprets the bound inside the group's rate space and the ladder wanders into VHT rates. The enum note carries the finding; the FrameParser headers keep the field position documented as deliberately unwritten; the env parse warns on any non-"off" value.
• docs/scheduled-mac.md — measured distributions beside the retry-limit guidance; DRONE_FALLBACK passthrough in the arq harness.

Step 3's disposition (also on the issue, vendor-cited): per-packet deadline NO-GO (no descriptor lifetime field on Jaguar3); per-AC PARTIAL via REG_PKT_LIFE_TIME 0x4C0 (1024 µs units) — effectively per-link since injection rides one queue; the fw's CCX state-2/3 lifetime signature must be bench-verified before anything trusts it.

Validation

Three fresh on-air arms (table above) + an off-arm hygiene smoke after the floor removal (888/888 pinned); ctest 49/49; default path byte-identical by construction.

🤖 Generated with Claude Code

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

PR Summary by Qodo

Add DEVOURER_TX_RETRY_FALLBACK=off to pin TX retries to descriptor rate

✨ Enhancement 📝 Documentation ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Add an env-configured knob to disable firmware retry rate fallback on inject TX.
• Apply the knob across Jaguar1/2/3 TX descriptor builders without changing defaults.
• Document measured fallback behavior and explicitly reject a lowest-rate “floor” form.

Diagram

graph TD
  A["ENV: DEVOURER_TX_RETRY_FALLBACK"] --> B["env_config.cpp"] --> C["DeviceConfig::Tx"] --> D["RtlJaguar* build_tx_block"] --> E["TX desc DIS*FB bit"] --> F["FW retry ladder"]
  G["tests/arq_e2e_delivery.sh"] --> A
  H["docs/scheduled-mac.md"] --> A

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Implement an RA-group-aware retry floor

• ➕ Would allow bounded fallback (avoid lowest legacy rates) while retaining some resilience on weak links
• ➕ Could be more flexible than a binary on/off switch
• ➖ Requires firmware-accurate mapping of DATA_RTY_LOWEST_RATE semantics per RA-group and PHY mode
• ➖ Higher risk of surprising cross-rate behavior (as measured) without extensive bench validation

2. Auto-disable fallback when a fixed TX rate is configured

• ➕ No extra knob for common constant-rate workflows
• ➕ Reduces operator error/misconfiguration
• ➖ Heuristics can be wrong (fixed rate may still want fallback in marginal links)
• ➖ Harder to reason about and test than an explicit opt-in env switch

3. Expose a runtime setter instead of env-only

• ➕ Can be toggled during experiments without process restart
• ➕ Enables adaptive policies (e.g., disable fallback only in specific phases)
• ➖ Requires API surface changes and synchronization rules across devices/threads
• ➖ More code paths to validate across chip generations

Recommendation: The PR’s explicit opt-in knob ("off" only) is the safest and most reviewable approach given the measured firmware reinterpretation of DATA_RTY_LOWEST_RATE. Keeping default behavior byte-identical minimizes regression risk, while still enabling constant-rate links to avoid airtime blowups from deep-retry rate step-downs. If a bounded fallback is desired later, pursue an RA-group-aware design behind a separate experimental flag with dedicated bench coverage.

Files changed (9) +73 / -0

Enhancement (4) +38 / -0

DeviceConfig.hAdd RetryFallback enum and tx.retry_fallback config field +19/-0

Add RetryFallback enum and tx.retry_fallback config field

• Introduces RetryFallback { Default, Off } with detailed in-header notes capturing measured firmware behavior. Extends DeviceConfig::Tx with retry_fallback and documents env grammar and default/byte-identical behavior.

src/DeviceConfig.h

RtlJaguarDevice.cppDisable firmware retry fallback in Jaguar1 TX descriptors when configured +7/-0

Disable firmware retry fallback in Jaguar1 TX descriptors when configured

• Threads cfg.tx.retry_fallback into the Jaguar1 TX descriptor build path. When set to Off, sets the DISABLE_FB bit so retries re-air at the descriptor DATARATE; default leaves existing behavior unchanged.

src/jaguar1/RtlJaguarDevice.cpp

RtlJaguar2Device.cppDisable retry rate fallback in Jaguar2 TX descriptors when configured +6/-0

Disable retry rate fallback in Jaguar2 TX descriptors when configured

• Adds handling for cfg.tx.retry_fallback in the Jaguar2 build_tx_block path. When Off, sets DISDATAFB in the checksummed descriptor span and recalculates checksum as usual.

src/jaguar2/RtlJaguar2Device.cpp

RtlJaguar3Device.cppDisable retry rate fallback in Jaguar3 TX descriptors when configured +6/-0

Disable retry rate fallback in Jaguar3 TX descriptors when configured

• Adds handling for cfg.tx.retry_fallback in the Jaguar3 TX descriptor builder. When Off, sets DISDATAFB so deep retries do not step down toward legacy rates; defaults remain unchanged.

src/jaguar3/RtlJaguar3Device.cpp

Tests (1) +2 / -0

arq_e2e_delivery.shAdd DRONE_FALLBACK passthrough to DEVOURER_TX_RETRY_FALLBACK +2/-0

Add DRONE_FALLBACK passthrough to DEVOURER_TX_RETRY_FALLBACK

• Introduces a DRONE_FALLBACK parameter and conditionally forwards it as DEVOURER_TX_RETRY_FALLBACK for the drone-sim txdemo run. Enables A/B testing of pinned vs laddered retry behavior in the end-to-end ARQ harness.

tests/arq_e2e_delivery.sh

Documentation (3) +22 / -0

scheduled-mac.mdDocument retry rate-fallback ladder and the new off knob +11/-0

Document retry rate-fallback ladder and the new off knob

• Adds measured observations that injected-frame retries step down in rate by default. Documents DEVOURER_TX_RETRY_FALLBACK=off as a way to pin retries to the descriptor rate and explains why no lowest-rate floor is offered due to anomalous firmware behavior.

docs/scheduled-mac.md

FrameParserJaguar2.hDocument DATA_RTY_LOWEST_RATE field as deliberately unused +5/-0

Document DATA_RTY_LOWEST_RATE field as deliberately unused

• Adds a macro definition and comment for the DATA_RTY_LOWEST_RATE bitfield, explicitly documenting it as present but intentionally not written due to measured RA-group reinterpretation issues.

src/jaguar2/FrameParserJaguar2.h

FrameParserJaguar3.hDocument DATA_RTY_LOWEST_RATE field as deliberately unused +6/-0

Document DATA_RTY_LOWEST_RATE field as deliberately unused

• Adds a macro definition and comment for DATA_RTY_LOWEST_RATE on 8822C descriptors. Captures that it is not a plain descriptor-rate bound on the measured firmware and is intentionally unwritten.

src/jaguar3/FrameParserJaguar3.h

Other (1) +11 / -0

env_config.cppParse DEVOURER_TX_RETRY_FALLBACK with off-only semantics +11/-0

Parse DEVOURER_TX_RETRY_FALLBACK with off-only semantics

• Adds env parsing for DEVOURER_TX_RETRY_FALLBACK. Accepts only "off" (sets RetryFallback::Off) and warns for any other value while preserving the default firmware ladder behavior.

examples/common/env_config.cpp

[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. Missing stderr flush after warn ✓ Resolved 📘 Rule violation ◔ Observability

Description

The new DEVOURER_TX_RETRY_FALLBACK warning is printed to stderr without a per-line flush, so
when stderr is buffered (e.g., redirected) the diagnostic may be delayed/interleaved and can stall
piped consumers. This violates the requirement that each human diagnostic line be flushed per line.

Code

examples/common/env_config.cpp[R125-127]

+      std::fprintf(stderr,
+                   "devourer [W] DEVOURER_TX_RETRY_FALLBACK='%s' unsupported "
+                   "(only \"off\") — keeping the firmware ladder\n", e);

Evidence

PR Compliance ID 11 requires human diagnostic log lines to be flushed per line. The added warning
uses std::fprintf(stderr, ...) and does not follow it with std::fflush(stderr), so the line may
remain buffered when stderr is not unbuffered.

CLAUDE.md: Human Diagnostic Log Lines Must Be Written Atomically and Flushed Per Line: CLAUDE.md: Human Diagnostic Log Lines Must Be Written Atomically and Flushed Per Line: CLAUDE.md: Human Diagnostic Log Lines Must Be Written Atomically and Flushed Per Line: CLAUDE.md: Human Diagnostic Log Lines Must Be Written Atomically and Flushed Per Line
examples/common/env_config.cpp[125-127]

Agent prompt

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

## Issue description
A human diagnostic warning is written to `stderr` but not flushed per line, which can cause buffered/late logs when `stderr` is redirected.
## Issue Context
Compliance requires each diagnostic log line be written atomically and flushed per line.
## Fix Focus Areas
- examples/common/env_config.cpp[125-128]

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

---

2. Stale Floor mode comments ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

Comments in both the Jaguar TX descriptor builders and the ARQ e2e harness advertise a
retry-fallback “Floor”/`` mode (including floor-bounding via rate-limit fields), but the actual
configuration shipped by this PR only supports Default and Off and explicitly documents that the
floor form was rejected/unsupported. This mismatch is misleading for maintainers and can cause bench
runs or debugging sessions to assume behavior that will never be applied.

Code

src/jaguar1/RtlJaguarDevice.cpp[R1180-1182]

+   * disables it. Off pins retries at the descriptor rate; Floor bounds the
+   * ladder via DATA_RATE_FB_LIMIT. Dword3/4, inside the checksummed 32
+   * bytes. */

Evidence

The cited config/docs introduce RetryFallback::{Default, Off} and explicitly state that a
lowest-rate “floor” form (e.g., DATA_RTY_LOWEST_RATE / bounding via DATA_RATE_FB_LIMIT) was
rejected, yet the newly added TX descriptor builder comments still describe a Floor mode and
rate-floor bounding behavior, creating a direct contradiction. Similarly, the ARQ e2e harness script
documents DRONE_FALLBACK as accepting off |  | "", but the env-to-DeviceConfig translation only
recognizes the literal off and treats any other value as unsupported (emitting a warning and
keeping the firmware fallback ladder), so the advertised `` behavior cannot occur.

src/DeviceConfig.h[82-93]
src/DeviceConfig.h[221-226]
src/jaguar1/RtlJaguarDevice.cpp[1178-1184]
src/jaguar2/RtlJaguar2Device.cpp[1542-1547]
src/jaguar3/RtlJaguar3Device.cpp[2020-2025]
tests/arq_e2e_delivery.sh[47-47]
examples/common/env_config.cpp[118-128]

Agent prompt

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

## Issue description
Update documentation/comments to match the implemented retry-fallback configuration: remove or rewrite references to a non-existent `Floor`/rate-based fallback mode (including floor-bounding via rate-limit fields), because the PR only supports `RetryFallback` values `Default` and `Off` and explicitly rejects the floor form. Also align the ARQ e2e harness documentation for `DRONE_FALLBACK` with the env parser behavior, which only accepts `off` (all other values warn and leave firmware fallback enabled).
## Issue Context
This PR deliberately ships *no floor/rate form* (the floor concept was rejected/unsupported per `RetryFallback` documentation), but several newly added comments still describe “Floor bounds the ladder” and the test harness advertises a `<rate>` value for `DRONE_FALLBACK`. The env-to-config translation contradicts that harness documentation by only accepting `off` and otherwise preserving the firmware ladder.
## Fix Focus Areas
- src/jaguar1/RtlJaguarDevice.cpp[1178-1182]
- src/jaguar2/RtlJaguar2Device.cpp[1542-1545]
- src/jaguar3/RtlJaguar3Device.cpp[2020-2023]
- src/DeviceConfig.h[82-93]
- tests/arq_e2e_delivery.sh[47-47]
- examples/common/env_config.cpp[118-128]
- src/DeviceConfig.h[221-226]

ⓘ 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.}