tx: count only full bulk writes as submitted on the sync generations

[josephnef]

Follow-up from the #400 review: the RTL8733B port counted an aggregated frame as submitted only on a full bulk write, and that made its siblings the odd ones out — Jaguar2 and Jaguar3 counted rc >= 0, but bulk_send_sync_ep returns bytes submitted, so a short write (the chip got a prefix) was reported as delivered work. In an aggregated URB that is worse: trailing blocks partial or absent, no way to say which frames aired, all of them counted.

What changed

• Jaguar2 + Jaguar3, single-frame and aggregated paths: full-write-or-nothing accounting, matching the 8733B (and Kestrel, which was already strict).
• Jaguar2 + Jaguar3 aggregated paths now emit the tx.agg sent byte count and log a genuine short write (rc >= 0, truncated) as an error. rc < 0 stays quiet there deliberately — their single-frame paths' NAK-backoff contract means a failure log would flood exactly when the caller is already backing off.
• RTL8733B: the aggregated error log no longer gates on rc >= 0 — a hard transport failure reaches stderr like it does on that backend's single-frame path (the rtl8733b: port USB TX aggregation (send_packets) #400 review's finding 1).
• Jaguar1: comment only. Its TX is async by design; ok is URB acceptance and bytes-on-wire resolve at completion reaping, so submit-time full-write accounting cannot apply there.
• docs/logging.md: the tx.agg row now states the sync-generation sent/full-write semantics and the Jaguar1 async exception.

Verification

• ctest 54/54.
• On-air on both changed generations (tests/txagg_bench.sh, BATCH=3, MCS7, 10 s floods):
  • 8812BU (Jaguar2) TX → 8812CU witness: 7459 agg URBs, every one sent==bytes, ok=true, frames/URB 3.0, rx_hits 17100 vs 17200 single — delivery parity.
  • 8812CU (Jaguar3) TX → 8812BU witness: 10654 agg URBs, every one full-write, frames/URB 3.0, rx_hits 26500 vs 26600 single.
  • Zero aggregated-TX error lines in either run — the new logging is silent when nothing is wrong.
• The 8733B change is log-gate-only (its accounting was already strict and hardware-validated in rtl8733b: port USB TX aggregation (send_packets) #400); no 8733B unit was on the bench for this run.

What this deliberately does not do

No retry of a short-written aggregated URB: the chip holds a prefix, and resubmitting could re-air frames that did make it. Frames in a truncated URB are dropped and simply not counted — the caller's own accounting sees the loss.

🤖 Generated with Claude Code

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

PR Summary by Qodo

TX: count only full bulk writes as submitted on sync generations

🐞 Bug fix 📝 Documentation 🕐 20-40 Minutes

AI Description

• Treat short USB bulk writes as NOT submitted on Jaguar2/Jaguar3 sync TX paths.
• Emit tx.agg.sent for Jaguar2/Jaguar3 and log actionable aggregated short writes.
• Align tx.agg logging/docs across Jaguar1 async and RTL8733B sync behaviors.

Diagram

graph TD
  A["TX send (sync)"] --> B["Build frame/URB"] --> C["bulk_send_sync_ep"] --> D{"Full write?"}
  D -->|"Yes"| E["Emit tx.agg (ok)"] --> F["Count frames ok"]
  D -->|"No"| G["Emit tx.agg (fail) + log"]
  H["TX send (async)"] --> I["submit URB"] --> J["Emit tx.agg (accepted)"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Return a richer result type from bulk_send_sync_ep

• ➕ Centralizes “full write vs short write vs transport error” semantics in one place
• ➕ Avoids repeating rc==size checks and per-backend logging policy in each device implementation
• ➖ API churn across all callers; larger refactor than this targeted fix
• ➖ May require updating tests/bench tooling expecting current return shape

2. Shared helper for sync-TX accounting + tx.agg emission

• ➕ Eliminates duplicated sent_all/emit/log logic across Jaguar2/Jaguar3/RTL8733B
• ➕ Makes it harder for sibling backends to drift again
• ➖ Adds an abstraction layer in a hot path; might complicate per-backend logging policies (NAK-backoff vs always-log)

Recommendation: The PR’s approach is the right scoped fix: it corrects the accounting bug by using rc==bytes everywhere it matters, adds sent visibility for aggregated paths, and keeps the intentional per-backend logging policy differences (NAK-backoff silence vs always-log). Consider a follow-up refactor (helper or richer return type) only if more sync-generation TX paths are added or further drift appears.

Files changed (5) +51 / -10

Bug fix (3) +46 / -9

RtlJaguar2Device.cppRequire full bulk write for Jaguar2 TX success; add agg 'sent' + short-write log +20/-3

Require full bulk write for Jaguar2 TX success; add agg 'sent' + short-write log

• Changes single-frame 'send_packet' to treat success as 'rc == usb_frame.size()' (not merely 'rc >= 0'). In aggregated 'send_packets', emits 'tx.agg.sent', sets 'ok' only on full writes, and logs actionable aggregated short writes while keeping 'rc < 0' quiet to avoid NAK-backoff log floods.

src/jaguar2/RtlJaguar2Device.cpp

RtlJaguar3Device.cppRequire full bulk write for Jaguar3 TX success; add agg 'sent' + short-write log +20/-3

Require full bulk write for Jaguar3 TX success; add agg 'sent' + short-write log

• Aligns single-frame 'send_packet' with “full-write-or-nothing” semantics by requiring 'rc == usb_frame.size()'. In aggregated 'send_packets', emits 'sent', sets 'ok' only on full writes, and logs aggregated short writes (rc>=0 truncated) while suppressing rc<0 logs to preserve the existing backoff contract.

src/jaguar3/RtlJaguar3Device.cpp

Rtl8733bDevice.cppLog RTL8733B aggregated transport failures as well as short writes +6/-3

Log RTL8733B aggregated transport failures as well as short writes

• Adjusts aggregated TX error logging to trigger on any non-full write ('!sent_all'), not only the rc>=0 short-write case. This makes rc<0 transport failures reach stderr in the aggregated path, matching the backend’s single-frame logging behavior.

src/rtl8733b/Rtl8733bDevice.cpp

Documentation (2) +5 / -1

logging.mdDocument sync-generation tx.agg 'sent' and full-write 'ok' semantics +1/-1

Document sync-generation tx.agg 'sent' and full-write 'ok' semantics

• Updates the 'tx.agg' event row to state that sync-TX generations emit 'sent' and set 'ok' only on full writes. Clarifies Jaguar1’s async exception: 'ok' is URB acceptance and 'sent' is not emitted at submit time.

docs/logging.md

RtlJaguarDevice.cppClarify Jaguar1 async aggregated TX semantics in-code +4/-0

Clarify Jaguar1 async aggregated TX semantics in-code

• Adds a comment explaining that Jaguar1’s aggregated TX is asynchronous, so 'ok' indicates URB acceptance and bytes-on-wire are only known at completion reaping. No behavioral changes to accounting or logging.

src/jaguar1/RtlJaguarDevice.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. tx.agg sent can be negative ✓ Resolved 📘 Rule violation ◔ Observability

Description

tx.agg.sent is documented as the non-negative number of bytes actually transferred, but the new
Jaguar2/Jaguar3 aggregated TX paths emit sent=rc unconditionally, allowing negative libusb
transport error codes when rc < 0. This violates the machine-event schema/contract and can mislead
downstream consumers/parsers that treat sent as a byte count (e.g., for detecting short writes).

Code

src/jaguar2/RtlJaguar2Device.cpp[R1363-1367]

  devourer::Ev(_logger->events(), "tx.agg")
      .f("frames", (unsigned long long)plan.frames())
      .f("bytes", (unsigned long long)urb.size())
+        .f("sent", (long long)rc)
      .f("shim", plan.shim)
-        .f("ok", rc >= 0);
-    if (rc >= 0)

Evidence

PR Compliance ID 7 requires machine-event logs to keep a consistent, tool-consumable schema, and the
updated logging docs define tx.agg.sent as “bytes actually transferred,” using 0 <= sent < bytes
to identify short writes. However, the Jaguar2/Jaguar3 aggregation code serializes sent directly
from the bulk_send_sync_ep()/UsbTransport::tx_sync() return value rc, which is a byte count
only on success and becomes a negative libusb error code on transport failure (rc < 0), so the
emitted sent can be negative and no longer matches the documented semantics.

CLAUDE.md: Logging Output Must Maintain Non-Interleaving Lines and Event Schema Requirements: CLAUDE.md: Logging Output Must Maintain Non-Interleaving Lines and Event Schema Requirements: CLAUDE.md: Logging Output Must Maintain Non-Interleaving Lines and Event Schema Requirements: CLAUDE.md: Logging Output Must Maintain Non-Interleaving Lines and Event Schema Requirements
src/jaguar2/RtlJaguar2Device.cpp[1352-1368]
docs/logging.md[109-113]
src/jaguar2/RtlJaguar2Device.cpp[1349-1369]
src/jaguar3/RtlJaguar3Device.cpp[1831-1851]
src/UsbTransport.cpp[1002-1023]
docs/logging.md[107-113]
src/jaguar3/RtlJaguar3Device.cpp[1834-1850]

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 `tx.agg` machine event currently emits `sent` as `(long long)rc` unconditionally, but the underlying send path returns a negative libusb error code on transport failure (`rc < 0`) and a byte count on success. Because `tx.agg.sent` is documented as “bytes actually transferred” (and consumers use `0 <= sent < bytes` to detect short writes), emitting negative values breaks the documented schema/contract and makes `sent` ambiguous/misleading.
## Issue Context
- PR Compliance ID 7 requires machine-event logs to maintain a consistent, tool-consumable schema.
- `bulk_send_sync_ep()` (via `UsbTransport::tx_sync()`) returns bytes submitted/actually sent on success, and a negative error code on failure.
- `docs/logging.md` defines `tx.agg.sent` as bytes actually transferred and uses `0 <= sent < bytes` to identify short writes.
- Jaguar2 and Jaguar3 aggregated TX paths now serialize `sent` from `rc` even when `rc < 0`.
## Fix Focus Areas
- src/jaguar2/RtlJaguar2Device.cpp[1352-1369]
- src/jaguar3/RtlJaguar3Device.cpp[1834-1851]
- docs/logging.md[107-113]

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

---

2. tx.stats misses short writes ✓ Resolved 🐞 Bug ≡ Correctness

Description

After this PR, Jaguar2/Jaguar3 treat short writes as failures (rc == size required), but
UsbTransport::tx_sync still does not increment TxStats.failed when libusb returns success with
actual < requested. This violates the TxStats contract (“failed counts those that did not
complete OK”) and makes tx.stats underreport failures relative to tx.frame/tx.agg’s new
full-write success semantics.

Code

src/jaguar2/RtlJaguar2Device.cpp[1261]

+  return rc == static_cast<int>(usb_frame.size());

Evidence

The PR changes Jaguar2’s success predicate to require full-byte completion, but the transport stats
layer still only marks failure on libusb error rc != success and ignores the actual < length
short-write case, contradicting the documented meaning of TxStats.failed and how tests interpret
tx.stats.

src/jaguar2/RtlJaguar2Device.cpp[1242-1262]
src/UsbTransport.cpp[1008-1023]
src/TxStats.h[13-20]
tests/ack_txreport_matrix.sh[63-68]

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 PR correctly reclassifies short writes as not-sent at the device layer (`rc == size`), but transport-level `TxStats` still treats a libusb-success short write as non-failure. This creates inconsistent accounting: `tx.frame`/`tx.agg.ok` can be false while `tx.stats.failed` doesn’t reflect the failure.
## Issue Context
- `TxStats` explicitly defines `failed` as submissions that “did not complete OK”. A short write is not “OK” under the new full-write semantics.
- `UsbTransport::tx_sync` currently increments `_tx_failed` only when `libusb_bulk_transfer` returns a non-success rc; it does not check `actual == length` on success.
- Several test harnesses treat `tx.stats.submitted` as the “frames sent” denominator, so failing to count short writes as failures skews those analyses.
## Fix Focus Areas
- src/UsbTransport.cpp[1002-1023]
- src/TxStats.h[13-20]
- tests/ack_txreport_matrix.sh[63-68]
## Suggested fix
- In `UsbTransport::tx_sync`, after a `LIBUSB_SUCCESS` return:
- if `actual != length`, increment `_tx_failed`, set `_tx_last_timeout` false, and set `_tx_last_rc` to a documented sentinel indicating short write (since no libusb error code exists for this case).
- keep returning `actual` so callers can continue to detect truncation.
- Optionally add an explicit `tx_short` counter/field (more invasive) if you want to avoid overloading `last_error_rc` with a sentinel.
- Update any docs/tests if they assume `failed` only tracks libusb errors, not short writes.

ⓘ 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 keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗