Skip to content

Fix ESP32-S2/S3 raw-REPL paste corruption - #11354

Open
mikeysklar wants to merge 7 commits into
adafruit:mainfrom
mikeysklar:espressif-cdc-rx-drain
Open

Fix ESP32-S2/S3 raw-REPL paste corruption#11354
mikeysklar wants to merge 7 commits into
adafruit:mainfrom
mikeysklar:espressif-cdc-rx-drain

Conversation

@mikeysklar

@mikeysklar mikeysklar commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

Fixes raw-REPL paste corruption on ESP32-S2/S3: console input is now read on the TinyUSB task, not the VM task.

Cause

On espressif tud_task() runs in its own FreeRTOS task, so a VM read can re-arm the endpoint mid-copy.

Step Where
XFER_COMPLETE clears BUSY/CLAIMED on the OUT endpoint lib/tinyusb/src/device/usbd.c
cdcd_xfer_cb then copies ep_buf into the RX fifo lib/tinyusb/src/class/cdc/cdc_device.c
Meanwhile the VM's tud_cdc_read_char() re-arms DMA into the same ep_buf lib/tinyusb/src/tusb.c tu_edpt_stream_read_xfer
Result: one 64-byte packet lost, the next repeated, length preserved

Change

Espressif only (CFG_TUSB_OS == OPT_OS_FREERTOS). Other ports read the fifo directly, unchanged from main.

File Change
supervisor/shared/usb/usb_device.c 256-byte py/ringbuf, filled on the TinyUSB task, every access inside common_hal_mcu_disable_interrupts() like espressif _bleio/CharacteristicBuffer.c
ports/espressif/supervisor/usb.c drain on every usbd pass; tud_task_ext(10, false) so fifo input held back by a full ringbuf moves in without a new USB event (the loop's existing tud_cdc_write_flush() now runs too)
supervisor/shared/serial.c REPL reads the ringbuf
shared-module/usb_cdc/Serial.c usb_cdc.console read, in_waiting, reset_input_buffer() use the ringbuf

Hardware tested

Raw-REPL paste harness, 40 tests x 50 iterations per row. Host: Ubuntu 24.04.5 LTS.

Board Firmware Failed pastes / 2000
Metro ESP32-S3 stock 10.3.0 14
Metro ESP32-S3 main 201776a 25
Metro ESP32-S3 this PR 0
Metro ESP32-S2 main + v1.29 merge (#11349), three runs 4, 6, 2
Metro ESP32-S2 this PR 0
Check, this PR Metro ESP32-S3 Metro ESP32-S2
Fresh connection: prompt, echo, Ctrl-C into busy loop OK OK
usb_cdc.console.in_waiting / read() from a REPL loop OK OK
usb_cdc.enable(console=False, data=True), echo on usb_cdc.data OK OK
Non-espressif, this PR 40-test burn Ctrl-C 2 KB and 4 KB raw REPL burst
Metro RP2350 40/40 OK OK
Metro M0 Express 40/40 OK OK
Build check, this PR Result
ESP32-P4 adafruit_p4gpio, ESP32-C3 (no native USB) build, not run
Flash use, same-tree builds of bless_dev_board_multi_sensor, circuitbrains_deluxe_m4, kicksat-sprite identical to main

Not covered

usb_cdc.data reads still come from the VM task on espressif. Same race class, separate change.

CI job Note
zephyr-tests / nrf54lm20bsim BLE NUS test fails on main too (42af95d), unrelated

AI assistance

Written with Claude Code. I verified the mechanism in the source and ran every measurement above on my test boards.

🤖 Generated with Claude Code

mikeysklar and others added 2 commits September 11, 2026 20:00
On espressif, tud_task() runs in its own FreeRTOS task at the same priority
as the VM. TinyUSB releases the OUT endpoint before cdcd_xfer_cb copies the
received packet into its fifo, so a tud_cdc_read from the VM in that window
re-arms DMA into the same buffer: one 64-byte packet is lost and the next one
repeated (hathach/tinyusb#1292). Measured as 3.5% of whole-file raw-REPL
pastes failing with a SyntaxError on Metro ESP32-S3.

Move all console CDC reads onto the task that runs tud_task(): tud_cdc_rx_cb
drains the fifo into a small single-producer/single-consumer ring buffer and
serial_read()/serial_bytes_available() read that. If the ring is full the
leftover is drained after the next tud_task(), which on espressif now times
out instead of blocking so a full ring cannot stall. Ctrl-C clears the ring
too. This is the structure MicroPython uses (micropython#14462).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
On espressif tud_cdc_rx_wanted_cb runs on the TinyUSB task, so moving
the ring tail there gave the tail two writers. Ctrl-C now sets a flag
and the VM moves the tail on its next read.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@dhalbert dhalbert left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a already a (non-atomic) ringbuf implementation in py/ringbuf.{c,h}, which we use several other places. Could you use that?

mikeysklar and others added 2 commits September 12, 2026 06:39
Replace the custom ring with the existing ringbuf, guarded with
common_hal_mcu_disable_interrupts() the same way the espressif BLE
CharacteristicBuffer guards its ringbuf. The USB task reads the TinyUSB
fifo outside the critical section, since that takes a mutex, and puts
the chunk into the ringbuf inside it. Ctrl-C clears the ringbuf directly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Only espressif runs tud_task() in its own task, so the ringbuf is now
built only when CFG_TUSB_OS is OPT_OS_FREERTOS; other ports read the
TinyUSB fifo directly again, as on main. The espressif usbd loop calls
the drain on every pass, which replaces the pending flag.

The drain does nothing when the console is disabled, so a data serial
at index 0 keeps its input, and usb_cdc.console reads, in_waiting and
reset_input_buffer() use the ringbuf. The copy chunk is a fixed 64
bytes so a high-speed endpoint size does not put 512 bytes on the usbd
task stack.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mikeysklar

mikeysklar commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator Author

Good call. Switched to py/ringbuf.

Guarded with common_hal_mcu_disable_interrupts() the way espressif CharacteristicBuffer does.

This is limited it to espressif since only there does tud_task() run in its own task.

Metro ESP32-S3 and S2: 0 of 2000 pastes corrupted each.

Done in 759e763 and 3324f0f.

@dhalbert dhalbert left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional FREERTOS checks in the changes make the shared in supervisor/shared/... be less authentic. Previously there was only one tiny conditional, having to do with FreeRTOS tasking.

Can the code be refactored so there is less port-specifc code again, maybe with default impls of the usb_cdc_rx_... routines, and then espressif-specific impls? Could other ports switch to using usb_cdc_rx... calls (without the ringbuf) instead of usb_cdc...? Or could some abstraction be made to hide the details here?

mikeysklar and others added 2 commits September 13, 2026 10:15
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KwTBwHF9YvQvN18ctYzJ7U
Shared code now calls usb_cdc_rx_read/available/clear/drain unconditionally.
The default implementations in supervisor/shared/usb/usb_device.c are weak
pass-throughs to the TinyUSB fifo, the same behavior as before on ports that
run tud_task() on the VM task. ports/espressif/supervisor/usb.c overrides
them with the console ringbuf. No CFG_TUSB_OS conditionals remain in
supervisor/shared or shared-module.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KwTBwHF9YvQvN18ctYzJ7U
@mikeysklar

Copy link
Copy Markdown
Collaborator Author

Done in 6a8a2d8: shared defaults are pass-throughs, espressif overrides with the ringbuf.

The console object reads the ring, the drain skips when the console is off, and ARM keeps the direct fifo read since a ring there stalled a 2 KB paste on the M0. Merged main. S2 and S3 0/2000, M0 and RP2350 clean, 20 to 40 bytes flash on tight boards.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@mikeysklar

Copy link
Copy Markdown
Collaborator Author

Comments trimmed in 3e0a43e.

@dhalbert dhalbert left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me - thanks. @tannewt: how is this for you?

@dhalbert
dhalbert requested a review from tannewt September 13, 2026 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants