fix(usb_device): give write_cdc the same bounded-blocking backpressure as write_vendor by finger563 · Pull Request #746 · esp-cpp/espp

finger563 · 2026-09-01T02:34:05Z

What

Brings write_cdc() to parity with write_vendor()'s TX backpressure handling.

Why

PR #742 taught write_vendor() to wait (bounded 250 ms) for the TX FIFO to drain instead of truncating a frame, with an all-or-nothing fast-fail when called from TinyUSB-callback context. write_cdc() was left on the old flush-retry-once-then-drop path, so a CDC frame larger than CONFIG_TINYUSB_CDC_TX_BUFSIZE was still silently truncated — which also loses the whole frame for a CRC-framed protocol. (This is also the standing review comment on this PR about the retry-once not reliably helping.)

Change

Mirror write_vendor() exactly for CDC:

poll-drain up to 250 ms off the TinyUSB task;
all-or-nothing (checked via tud_cdc_n_write_available) when on the TinyUSB task, since the drain can't run under us there;
distinguish not_connected (unmounted, via tud_cdc_n_connected) from no_buffer_space (backpressure).

This matters for the dual-transport protocols (e.g. the CAN bridge in #748) that stream framed replies over CDC as well as vendor.

Note

This supersedes the branch's original timeout-overload approach: #742 already added the equivalent (and better, callback-context-aware) blocking behavior to write_vendor on main, so this branch was rebased to only bring CDC to parity.

Test

usb_cdc_example builds clean (esp32s3).

🤖 Generated with Claude Code

github-actions · 2026-09-01T02:34:53Z

✅Static analysis result - no issues found! ✅

Copilot

Pull request overview

This PR extends the UsbDevice component’s transmit API by adding explicit blocking write_cdc / write_vendor overloads that accept a std::chrono::milliseconds timeout, while preserving the existing non-blocking “drop-on-full” behavior via timeout == 0. It also consolidates CDC + vendor TX logic into a shared retry loop and updates documentation to make FIFO truncation behavior and sizing knobs more discoverable.

Changes:

Add a shared tx_with_timeout() helper and new timeout-based overloads for CDC and vendor writes.
Fail fast with std::errc::not_connected when the device isn’t mounted (no host configured).
Document truncation behavior and the TX FIFO sizing Kconfig options in the component README.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
`components/usb_device/src/usb_device.cpp`	Adds shared TX-with-timeout loop and introduces new blocking write overloads for CDC/vendor.
`components/usb_device/include/usb_device.hpp`	Exposes new timeout overloads and documents non-blocking truncation vs blocking semantics.
`components/usb_device/README.md`	Clarifies truncation behavior and adds guidance on when to use blocking overloads / FIFO sizing.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

finger563 · 2026-09-01T14:02:54Z

+    const size_t queued = queue_flush(data.data() + offset, data.size() - offset);
    if (queued == 0) {
-      tinyusb_cdcacm_write_flush(kCdcPort, 0);
-      queued = tinyusb_cdcacm_write_queue(kCdcPort, data.data() + offset, data.size() - offset);
-      if (queued == 0) {
-        logger_.warn_rate_limited("CDC TX buffer full, dropping {} bytes", data.size() - offset);
-        ec = std::make_error_code(std::errc::no_buffer_space);
-        break;
+      // FIFO full: nothing accepted this pass.
+      if (timeout > std::chrono::milliseconds(0) && std::chrono::steady_clock::now() < deadline) {
+        vTaskDelay(1); // let the TinyUSB task drain the endpoint, then retry


This comment is now moot: the branch was reworked. main (#742) already gave write_vendor the callback-context-aware bounded-blocking behavior, so the timeout-overload approach (and tx_with_timeout) was dropped — this PR now only brings write_cdc to parity with that same bounded-blocking loop (poll-drain up to 250 ms off the TinyUSB task; all-or-nothing on it). There is no longer a timeout==0 non-blocking path to regress the CDC flush-retry.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…as write_vendor write_cdc() advanced its offset and flushed each chunk, then could break out of the loop when a stalled FIFO made no progress - leaving a truncated frame on the wire and corrupting framing for every subsequent message under sustained CDC backpressure. (The can_bridge example even documented it as "all-or-nothing", which was false.) Port write_vendor()'s contract to write_cdc() using the raw tud_cdc_n_* API (so the whole frame can be sized up front, mirroring tud_vendor_*): bounded (250 ms) sleep-wait for the TinyUSB task to drain the FIFO, and ALL-OR-NOTHING when called from TinyUSB-callback context (fail fast with no_buffer_space without enqueueing a partial frame). Distinguishes host-disconnect (not_connected) from a full FIFO. This is the fix that closed PR #746 carried; folded in here since the CAN bridge exposes write_cdc over Web Serial. Benefits every write_cdc caller (coredump, usb_cdc example, can bridge). Builds clean for esp32s3 on IDF v6.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…le (#748) * feat(canopen): USB<->CAN bridge example + WebUSB/Web Serial CAN console Adds a new can_bridge_example that turns an ESP32-S3 into a WebUSB / Web Serial CAN interface, and the hosted CAN console web app that drives it. - Firmware bridges the Twai (CAN 2.0) controller to the host over USB using the stream_frame framing and an espp::Dispatcher (this example owns module id 5). The same framed protocol is exposed on BOTH the vendor interface (WebUSB) and a CDC interface (Web Serial); device->host frames go to whichever transport the host last used. The system console stays on USB-Serial-JTAG. - Protocol (can_bridge_protocol.hpp): CAN_TX / SET_CONFIG / START / STOP / GET_STATUS requests (0x5X) and CAN_RX / OK / ERROR / STATUS replies (0xDX); a CAN frame encodes as [id u32][flags u8][dlc u8][data]. Supports normal (master, ACK) and listen-only (passive sniff) modes; the bus starts stopped and the host configures baudrate/mode then starts it. - Web app (components/canopen/web/can_console.html): dual WebUSB/Web Serial connect, bus config + live status/counters, a send-frame panel with full id/ext/rtr/dlc/hex validation, and a capped live RX+TX monitor table with pause/clear/autoscroll. Self-contained single file; node --check clean. The DS402 canopen_example is unchanged. Example builds clean (esp32s3) and is added to the CI build matrix; the web app is auto-hosted from components/*/web/. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(canopen): migrate CAN bridge to v2 stream_frame framing + address review Rebased onto the v2 stream_frame/dispatcher branch and migrated the CAN bridge: - send_frame() now builds v2 frames on module 5, deriving the reply flag from the type's high bit (0xD_ replies); the dispatcher handler takes the whole stream_frame::Frame (module already routed). - Protocol header comment corrected: every frame is module 5 with the type in the type byte and the reply flag set for 0xD_ replies (the old 'high nibble' wording described the retired v1 nibble scheme — the PR comment). - can_console.html migrated to the v2 9-byte header (module 5). Also addresses the other review comments: - top comment fixed: both vendor (WebUSB) and CDC (Web Serial) carry the framed protocol; the console/logs go to USB-Serial-JTAG (not CDC). - build.yml: can_bridge_example ordered before canopen/example (alphabetical). can_bridge_example builds clean (esp32s3); webapp node --check clean and the GET_STATUS/CAN_RX frames verified. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(canopen): address CAN bridge review comments + register example in manifest Firmware: - Vendor and CDC are independent byte streams, so each now gets its OWN Dispatcher (chunks are tagged by transport) — a frame split across reads on one transport can never be stitched onto the other's bytes. - SET_CONFIG validates the mode byte (0=normal / 1=listen-only) instead of silently accepting any value. - send() surfaces a rate-limited warning when a frame is dropped (USB TX backpressure / disconnect) rather than discarding the write result; the writes are all-or-nothing so no truncated frame reaches the host. - init failure no longer announces the bridge as 'ready'. - Strengthened the protocol-header comment: the v2 frame has a dedicated module byte set to 5 for every frame (requests AND replies) — 0x5X/0xD_ are the type values, not modules (the retired v1 nibble scheme is gone). Web app (can_console.html): - Web Serial teardown now releases the reader/writer locks before port.close() (a held writer lock made close() reject and leave the port open). - The per-ID summary is bounded (MAX_SUMMARY_IDS = 1024, with a '+N more' note). - DLC input is parsed strictly so '2abc' / '1.5' are rejected. Docs/manifest: - canopen.rst: fixed the section title/underline (was HTML-escaped + too short). - canopen idf_component.yml: registered can_bridge_example so it is discoverable and usable from the component registry. can_bridge_example builds clean (esp32s3); webapp node --check clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(canopen): CAN bridge handler ignores reply-flagged frames Same v2 direction hardening as the coredump/haptics examples: the CAN bridge is a device request responder (it SENDS the 0xD_ replies), so handle_can_frame now returns early on frame.is_reply() — an echoed/loopback reply can no longer re-enter the request switch and emit a spurious 'unknown CAN bridge message' error. (can_console.html already gates on module 5 + the reply flag.) Builds clean (esp32s3). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(canopen): make can_console.html parser correlation-aware (v2 optional field) The merged stream_frame v2 codec (#747) gained an optional u16 correlation id in the header (flags bit1). Update the CAN console StreamParser to match the other espp web apps: read flags first, derive the header size (9 or 11 bytes), read len at offset 5+ext, size the frame/CRC window off the dynamic header, and expose frame.correlation (u16 when present, else null). Added FLAG_CORRELATION / CORRELATION_SIZE constants. The CAN firmware never sets the bit, so received frames are unchanged — this is forward-compatible robustness. node --check clean; correlation + plain frames verified round-trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(usb_device): give write_cdc the same all-or-nothing backpressure as write_vendor write_cdc() advanced its offset and flushed each chunk, then could break out of the loop when a stalled FIFO made no progress - leaving a truncated frame on the wire and corrupting framing for every subsequent message under sustained CDC backpressure. (The can_bridge example even documented it as "all-or-nothing", which was false.) Port write_vendor()'s contract to write_cdc() using the raw tud_cdc_n_* API (so the whole frame can be sized up front, mirroring tud_vendor_*): bounded (250 ms) sleep-wait for the TinyUSB task to drain the FIFO, and ALL-OR-NOTHING when called from TinyUSB-callback context (fail fast with no_buffer_space without enqueueing a partial frame). Distinguishes host-disconnect (not_connected) from a full FIFO. This is the fix that closed PR #746 carried; folded in here since the CAN bridge exposes write_cdc over Web Serial. Benefits every write_cdc caller (coredump, usb_cdc example, can bridge). Builds clean for esp32s3 on IDF v6.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(canopen): address CAN console review - a11y labels, v2 header docs, distinct-ID count - can_console.html: add aria-labels to the baud/mode selects and the ID/DLC/data inputs (were announced without an accessible name); document the optional v2 correlation field in the wire-format comment (len is at offset 5 for a base frame, 7 when flags bit1 is set; header 9 or 11 bytes). - Summary "+N more IDs not shown" counted dropped FRAMES, so repeated traffic from one omitted ID could read "+1000 more IDs". Track DISTINCT omitted IDs in a Set so the count is accurate. - README: document the complete stream_frame base-header order (magic/flags/ module/type/len/payload/crc32, module 5) instead of an abbreviated list; and clarify that RTR CAN frames carry NO data bytes (6-byte payload even when dlc is nonzero), so third-party clients must not append data for RTR. - can_bridge_example.cpp: correct the send() comment now that write_cdc is all-or-nothing with a bounded drain-wait. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(usb_device): make write_cdc/write_vendor truly all-or-nothing for framed writes The previous streaming loop could still leave a truncated prefix on the wire: tud_*_write() enqueues and flushes a chunk (advancing offset), and a later iteration can hit the 250 ms timeout or a disconnect and return false after those bytes are already on the stream - poisoning the host-side framing parser. That contradicted the documented all-or-nothing contract. Now a frame that fits in the TX FIFO (CFG_TUD_CDC_TX_BUFSIZE / 512, CFG_TUD_VENDOR_TX_BUFSIZE / 2048) is written atomically: bounded-wait for room for the WHOLE frame, then enqueue it in a single write, so a timeout/disconnect returns false without enqueueing anything. TinyUSB-callback context still fails fast when the frame does not already fit. Only frames LARGER than the FIFO are streamed (inherently non-atomic - documented). Applied symmetrically to both write paths and updated the header docs + component README to match. Builds clean for esp32s3 on IDF v6.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(canopen): CAN console review round 2 - WebUSB partial write, bounded dropped-ID set, GPIO doc - can_console.html WebUSB send(): transferOut may complete with status "ok" but bytesWritten < length; loop over the unsent suffix until every byte is acknowledged so a truncated frame can never desync the device parser. - The summary "+N more IDs" set (summaryDroppedIds) was unbounded - a flood of distinct 29-bit IDs would grow it without limit, defeating the memory cap. Cap it at MAX_SUMMARY_IDS and show "N+" once it overflows. - README: the firmware defaults are TX=GPIO17/RX=GPIO16; the wiring table said 5/4. Align the doc to the code so the example works as documented. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(canopen): final review nits - shared USB write timeout, valid meta, rst literal - usb_device: the 250 ms write timeout (and the drain poll interval) were hard-coded in both write_cdc() and write_vendor(); factor them into shared file-scope constants kUsbWriteTimeoutTicks / kUsbWriteDrainPollTicks so the two TX paths stay consistent and future tuning is one edit. - can_console.html: the <meta name="description"> content had a literal "USB<->CAN" (an unescaped '<' makes the tag invalid); reworded to "USB-to-CAN". - canopen.rst: render Twai as an inline literal (``Twai``) to match the surrounding ``can_bridge_example`` / ``stream_frame`` literals and avoid a single-backtick interpreted-text role. (PR description GPIO defaults corrected to TX=17/RX=16 to match the code + README.) Builds clean for esp32s3 on IDF v6.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Copilot AI lite review requested due to automatic review settings September 1, 2026 02:34

Copilot started reviewing on behalf of finger563 September 1, 2026 02:34 View session

Copilot AI reviewed Sep 1, 2026

View reviewed changes

finger563 mentioned this pull request Sep 1, 2026

feat(canopen): USB<->CAN bridge example + WebUSB/Web Serial CAN console #748

Merged

finger563 requested a balanced review from Copilot September 1, 2026 03:16

Copilot started reviewing on behalf of finger563 September 1, 2026 03:17 View session

Copilot AI reviewed Sep 1, 2026

finger563 requested a balanced review from Copilot September 1, 2026 03:21

Copilot started reviewing on behalf of finger563 September 1, 2026 03:22 View session

Copilot AI reviewed Sep 1, 2026

finger563 requested a balanced review from Copilot September 1, 2026 04:03

Copilot started reviewing on behalf of finger563 September 1, 2026 04:05 View session

Copilot AI reviewed Sep 1, 2026

finger563 closed this Sep 1, 2026

finger563 force-pushed the feat/usb-tx-timeout branch from 8cb6cfc to 18714cb Compare September 1, 2026 04:11

finger563 changed the title ~~feat(usb_device): blocking write_vendor/write_cdc timeout overloads~~ fix(usb_device): give write_cdc the same bounded-blocking backpressure as write_vendor Sep 1, 2026

finger563 temporarily deployed to pypi September 1, 2026 19:32 — with GitHub Actions Inactive

Navigation Menu

Uh oh!

fix(usb_device): give write_cdc the same bounded-blocking backpressure as write_vendor - #746

fix(usb_device): give write_cdc the same bounded-blocking backpressure as write_vendor#746
finger563 wants to merge 0 commit into
mainfrom
feat/usb-tx-timeout

finger563 commented Sep 1, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

finger563 Sep 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

finger563 commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Change

Note

Test

Uh oh!

github-actions Bot commented Sep 1, 2026

✅Static analysis result - no issues found! ✅

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

finger563 Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

finger563 commented Sep 1, 2026 •

edited

Loading