Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new device-side “usbtest” example (to act as a peer for the Linux kernel usbtest/testusb battery), integrates a host-side runner into HIL, and includes a broad set of DCD/core fixes uncovered by running the battery across many device ports.
Changes:
- Add
examples/device/usbtestplus vendor-class extensions (altsettings + optional bulk/int/iso endpoints) to support the Linuxusbtestsource/sink protocol. - Add
test/hil/usbtest.pyrunner and wire it into the HIL harness/reporting and rig configuration. - Fix multiple device-controller drivers and USBD core behaviors exposed by the usbtest battery (stall/clear-stall, toggle reset, iso support, FIFO readiness, etc.).
Reviewed changes
Copilot reviewed 35 out of 37 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/hil/usbtest.py | New host-side runner for binding usbtest + executing/parsing the testusb battery. |
| test/hil/tinyusb.json | Register CH32V307 variants and adjust rig board config/skip list. |
| test/hil/tinyusb-sudoer | Add sudoers allowlist template for usbtest binding/recovery workflows. |
| test/hil/hil_test.py | Integrate device/usbtest into HIL, add metric-aware failures and reporting tally fixes. |
| src/portable/wch/dcd_ch32_usbhs.c | Fix clear-stall behavior to re-queue armed OUT transfers (toggle reset case). |
| src/portable/wch/dcd_ch32_usbfs.c | Add/enable iso support details, per-direction iso state, EP3 sizing, and toggle handling fixes. |
| src/portable/wch/ch32_usbfs_reg.h | Add EP3 1023B capability macro for relevant WCH MCUs. |
| src/portable/st/stm32_fsdev/dcd_stm32_fsdev.c | Refine clear-stall so it doesn’t disarm VALID endpoints unintentionally. |
| src/portable/renesas/rusb2/dcd_rusb2.c | Bound FIFO-ready spins, add ZLP deferral, improve control OUT flow control, and fix clear-stall re-arm logic. |
| src/portable/raspberrypi/rp2040/rp2040_usb.c | Prevent iso endpoints from being double-buffered into neighboring DPRAM. |
| src/portable/raspberrypi/rp2040/dcd_rp2040.c | Re-issue active transfers on clear-stall toggle reset so claims complete properly. |
| src/portable/nxp/lpc_ip3511/dcd_lpc_ip3511.c | Fix stall semantics and implement iso alloc/activate handling. |
| src/portable/nordic/nrf5x/dcd_nrf5x.c | Add errata 199 workaround latch + implement iso alloc/activate path. |
| src/portable/microchip/samd/dcd_samd.c | Implement iso alloc/activate to reserve bank sizes and enable ISO endpoints. |
| src/portable/mentor/musb/dcd_musb.c | Prevent IN ISR from loading disarmed transfers; flush FIFO before stalling. |
| src/device/usbd.c | Harden GET_STATUS handling and endpoint feature validation; adjust vendor control callback wiring; refine clear-stall claim release. |
| src/common/tusb_mcu.h | Update endpoint-close vs iso-alloc capability documentation/deprecation notes. |
| src/class/vendor/vendor_device.h | Add config gates and APIs for optional interrupt/iso endpoints + altsetting support. |
| src/class/vendor/vendor_device.c | Implement vendor altsettings logic and add non-buffered helpers for bulk/int/iso endpoints. |
| src/class/audio/audio_device.c | Remove prior “clear busy” workaround comment in favor of standardized endpoint state clearing. |
| lib/rt-thread/port/msc_device_port.c | Bound INQUIRY string copies to avoid overflow. |
| hw/bsp/ch32v30x/family.c | Implement board_get_unique_id() for CH32V30x using ESIG UID. |
| hw/bsp/ch32v20x/family.c | Replace interrupt handlers with nested-safe naked ISRs for QingKe HWSTK scenario. |
| examples/device/video_capture/src/main.c | Fix VIDEO_STACK_SIZE typo. |
| examples/device/video_capture_2ch/src/main.c | Fix VIDEO_STACK_SIZE typo. |
| examples/device/usbtest/src/usb_descriptors.h | New usbtest descriptor/tier constants and per-MCU endpoint sizing rules. |
| examples/device/usbtest/src/usb_descriptors.c | New FS/HS descriptors (alt0 empty, alt1 endpoints) and string descriptors for usbtest. |
| examples/device/usbtest/src/tusb_config.h | New example TinyUSB config enabling vendor bulk/int/iso + altsettings and manual RX arming. |
| examples/device/usbtest/src/main.c | New device firmware implementing source/sink pumps + ctrl_out protocol. |
| examples/device/usbtest/src/CMakeLists.txt | ESP-IDF component registration for usbtest example. |
| examples/device/usbtest/skip.txt | Skip list for unsupported/unsuitable MCUs. |
| examples/device/usbtest/README.md | Documentation for tiers, protocol, and running via test/hil/usbtest.py. |
| examples/device/usbtest/Makefile | Make build integration for the new example. |
| examples/device/usbtest/CMakePresets.json | Presets include for board presets (CMake). |
| examples/device/usbtest/CMakeLists.txt | CMake build integration for the new example. |
| examples/device/CMakeLists.txt | Add usbtest to the device example list. |
| .claude/skills/usb-recover/SKILL.md | New rig USB recovery documentation. |
| .claude/skills/usb-recover/scripts/usb_recover.sh | New restricted recovery helper script (sudoers-friendly). |
| .claude/skills/usb-debug/SKILL.md | New rig USB dynamic-debug documentation. |
| .claude/skills/usb-debug/scripts/usb_dyndbg.sh | New restricted dynamic-debug toggle helper script (sudoers-friendly). |
| .claude/skills/hil/SKILL.md | Add guidance to stop/start the Actions runner on ci during manual hardware use. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Size Difference ReportBecause TinyUSB code size varies by port and configuration, the metrics below represent the averaged totals across all example builds. Note: If there is no change, only one value is shown. Changes >1% in size
Changes <1% in size
No changes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 099b6538dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
1 similar comment
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| raspberrypi_zero/webusb_serial | 48,920 → 49,360 (+440) | 1,087 → 4,743 (+3,656) | — | — | 50,007 → 54,103 (+4,096) | +8.2% |
| raspberrypi_zero/audio_4_channel_mic | 57,216 → 57,568 (+352) | 1,591 → 5,335 (+3,744) | — | — | 58,807 → 62,903 (+4,096) | +7.0% |
| ch32v103c_bluepill/dfu_runtime | 8,556 → 8,668 (+112) | — | — | 2,880 → 1,800 (-1,080) | 13,900 → 12,932 (-968) | -7.0% |
| raspberrypi_zero/dynamic_configuration | 61,420 → 61,792 (+372) | 939 → 4,663 (+3,724) | — | — | 62,359 → 66,455 (+4,096) | +6.6% |
| raspberrypi_cm4/cdc_uac2 | 61,248 → 61,568 (+320) | 1,320 → 5,096 (+3,776) | — | — | 62,568 → 66,664 (+4,096) | +6.5% |
| ch32v103c_bluepill/hid_generic_inout | 9,528 → 9,648 (+120) | — | — | 3,084 → 2,004 (-1,080) | 15,080 → 14,120 (-960) | -6.4% |
| ch32v103c_bluepill/hid_multiple_interface | 10,368 → 10,484 (+116) | — | — | 2,960 → 1,880 (-1,080) | 15,792 → 14,828 (-964) | -6.1% |
| ch32v103c_bluepill/hid_boot_interface | 10,388 → 10,504 (+116) | — | — | 2,960 → 1,880 (-1,080) | 15,812 → 14,848 (-964) | -6.1% |
| ch32v103c_bluepill/hid_composite | 10,612 → 10,732 (+120) | — | — | 2,948 → 1,868 (-1,080) | 16,028 → 15,068 (-960) | -6.0% |
| portenta_c33/uac2_headset | 15,420 → 16,340 (+920) | — | — | — | 15,468 → 16,388 (+920) | +5.9% |
Hardware-in-the-loop (HIL) Test Reporthfp-iar✅ 55 passed · ❌ 0 failed · ⚪ 0 skipped · blank not run
hfp.json✅ 44 passed · ❌ 11 failed · ⚪ 0 skipped · blank not run
tinyusb.json✅ 245 passed · ❌ 159 failed · ⚪ 15 skipped · blank not run
|
There was a problem hiding this comment.
💡 Codex Review
tinyusb/src/class/vendor/vendor_device.c
Lines 152 to 155 in 6d707a1
Include new vendor endpoints in mounted checks
With the new interrupt/iso endpoint support, a valid vendor interface no longer necessarily has bulk endpoints. For an interrupt-only descriptor, or an altsetting that exposes only interrupt/iso endpoints, vendord_open() records the new endpoint fields but tud_vendor_n_mounted() still checks only the bulk streams/endpoints, so typical application code guarded by tud_vendor_mounted() will never arm or write those endpoints even though the interface is configured.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
tinyusb/src/class/vendor/vendor_device.c
Line 155 in b7acb73
Include optional vendor endpoints in mounted checks
When CFG_TUD_VENDOR_TXRX_BUFFERED=0 and a descriptor uses the new interrupt-only vendor support (the open path explicitly allows bulk to be absent), vendord_open() records ep_int_in/out but leaves ep_in/out at zero, so tud_vendor_mounted() reports false even though the interface is configured and its interrupt endpoints are open. Applications that gate tud_vendor_int_read_xfer() or tud_vendor_int_write() on the mounted predicate will never arm/service these endpoints; include the optional interrupt/iso endpoint fields in this predicate.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24a92e4831
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Double-buffered iso, EP3 1023-byte packets on V20x/V30x (10-bit R16_UEP3_T_LEN), CH583 and V103 enabled at 64 B. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeF2gZ1M7GWkz6Av4BpKPg
The three USBD lines nest under QingKe HWSTK; gcc's interrupt prologue corrupts the return, so rely on the hardware stack and bare mret. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeF2gZ1M7GWkz6Av4BpKPg
Gadget-Zero style source/sink on a vendor interface (alt0 empty, alt1 bulk+int+iso) plus EP0 ctrl_out; tier advertised in bcdDevice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeF2gZ1M7GWkz6Av4BpKPg
Binds the kernel usbtest driver (gadget-zero profile), runs the tier-based battery, auto-recovers kernel-side hangs, and skips cases the host controller cannot run (MosChip MCS9990 EHCI int-OUT). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeF2gZ1M7GWkz6Av4BpKPg
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeF2gZ1M7GWkz6Av4BpKPg
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeF2gZ1M7GWkz6Av4BpKPg
hathach
force-pushed
the
usbtest
branch
2 times, most recently
from
da86b80 to
34bcdaf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34bcdaf53f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| *ctr = RUSB2_PIPE_CTR_ACLRM_Msk | RUSB2_PIPE_CTR_SQCLR_Msk; // abort in-flight + reset data toggle | ||
| *ctr = 0; | ||
| *ctr = RUSB2_PIPE_CTR_PID_BUF; // enable |
There was a problem hiding this comment.
When an RUSB2 ISO altsetting is reselected while an ISO transfer from the previous selection is still armed, this aborts the hardware buffer and immediately re-enables the pipe but leaves _dcd.pipe[num] (queued, zlp_pending, buf, remaining) unchanged. The next BRDY can therefore run process_pipe_brdy() against the stale descriptor before the class re-arms, completing or loading the old buffer after usbd_edpt_iso_activate() has already cleared the core endpoint state. Reset the pipe bookkeeping here before setting PID_BUF.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a6bb32c — dcd_edpt_iso_activate now resets the pipe bookkeeping (buf/remaining/queued/zlp_pending) inside the critical section, mirroring the nrf5x fix. Hardware-verified: usbtest 30/30 on ra6m5_ek (HS), ra4m1_ek, ra8m1_ek.
There was a problem hiding this comment.
Thanks for the catch — this is already fixed on the current head: dcd_edpt_iso_activate() now resets pipe->buf, pipe->remaining, pipe->queued, and pipe->zlp_pending (dcd_rusb2.c:913-917) before re-enabling the pipe with PID_BUF, with a comment noting this exists specifically to drop a transfer armed before SET_INTERFACE so a stray BRDY can't replay it.
| rusb->PIPEBUF = 0x7C08; | ||
| } | ||
| rusb->PIPESEL = (uint16_t) num; |
There was a problem hiding this comment.
On RUSB2 high-speed ports, PIPEBUF is selected by PIPESEL, but this writes the HS buffer allocation before selecting the ISO pipe. When device/usbtest allocates ISO pipes on RA6M5/RA8 HS, the write configures whichever pipe was previously selected and the newly reserved ISO pipe keeps the default/overlapping FIFO allocation, so ISO traffic can corrupt neighboring pipe buffers or fail under load. Select num before writing PIPEBUF and allocate a distinct buffer for that pipe.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a6bb32c — PIPEBUF is now written after PIPESEL in both dcd_edpt_open and dcd_edpt_iso_alloc (the stale 'no HS board to validate' justification no longer holds; the reorder is hardware-verified at 480M on ra6m5_ek, usbtest 30/30 including iso). The fixed-BUFNMB allocator limitation remains tracked in the FIXME.
There was a problem hiding this comment.
Fixed — PIPESEL is now written before PIPEBUF in both dcd_edpt_open() (dcd_rusb2.c:791-795) and dcd_edpt_iso_alloc() (dcd_rusb2.c:880-884), per the added comment 'PIPEBUF is PIPESEL-windowed... write it after selecting the pipe.' (The BUFNMB-sharing concern raised separately on line 884 is real and stays open as a follow-up.)
| #if CFG_TUSB_MCU == OPT_MCU_CH583 | ||
| return false; // CH58x has no isochronous support (see dcd_edpt_iso_alloc) | ||
| #else | ||
| return true; |
There was a problem hiding this comment.
When a WCH USBFS ISO altsetting is stopped and later reselected, this activation path returns without clearing data.xfer[ep][dir].valid or resetting the endpoint response left by the previous stream. Because ISO_ALLOC ports do not close the endpoint at alt 0, a packet that was armed before SET_INTERFACE can still be completed by the ISR against the stale application buffer before the class submits the fresh transfer. Clear the per-direction xfer state and park/reinitialize the endpoint here before reporting activation success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 283121a — dcd_edpt_iso_activate now drops the stale xfer descriptor and NAKs the endpoint, mirroring nrf5x. Hardware-verified: usbtest 30/30 on ch32v103r, nanoch32v203, and ch582m_evt (all three register-map variants of this driver).
There was a problem hiding this comment.
Already addressed: dcd_edpt_iso_activate() now sets data.xfer[ep][dir].valid = false and NAKs the endpoint (dcd_ch32_usbfs.c:512-520) before returning success, so a transfer armed before SET_INTERFACE can no longer be completed by the ISR against the stale buffer.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2 similar comments
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
… for reopen EPSKIP raises a transfer completion, so using it on the stall path let the class re-arm the endpoint and Active+Stall never actually stalled (usbtest case 13); write bare Active=0 instead, and retire skipped transfers on endpoint reopen where the completion is wanted. Verified: usbtest 30/30 on lpcxpresso11u37. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeF2gZ1M7GWkz6Av4BpKPg
The no-op activate left a transfer armed before SET_INTERFACE valid in data.xfer, letting the ISR complete it against the old buffer. Drop the descriptor and NAK the endpoint (mirrors the nrf5x fix). Verified: usbtest 30/30 on ch32v103r, nanoch32v203, ch582m_evt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeF2gZ1M7GWkz6Av4BpKPg
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61990f7310
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # several batteries at once have hard-frozen the CI rig (fatal PCIe error on its VFIO-passed | ||
| # xHCI), and battery + flash/re-enumeration churn has killed the uPD720201's command ring. | ||
| script = Path(__file__).resolve().parent / 'usbtest.py' | ||
| cmd = f'python3 "{script}" --serial "{uid}" --json --keep-binding --timeout 60' |
There was a problem hiding this comment.
When the shuffled HIL order runs device/usbtest before another 0xcafe:0x4010 example such as device/audio_test_freertos (the collision is recorded in EXAMPLE_PID), this --keep-binding invocation leaves the usbtest dynamic id registered after the battery. The next same-PID firmware can then auto-bind interface 0 to the kernel usbtest driver instead of its real audio driver, so the later test never gets an ALSA device even though the firmware is correct; keep the current binding if unbind is risky, but remove the dynamic id before continuing to other examples.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This scenario no longer applies: every example now has a unique hardcoded PID (0x4001-0x4022), so usbtest (cafe:4010) never shares a PID with another example — audio_test_freertos is 0x4004, for instance. See the comment at the --keep-binding call site in hil_test.py explaining why leaving the binding is safe under unique PIDs.
| root = os.path.realpath(f'/sys/bus/usb/devices/usb{bus}') | ||
| m = re.findall(r'[0-9a-f]{4}:[0-9a-f]{2}:[0-9a-f]{2}\.[0-9a-f]', root) | ||
| if m: | ||
| ctrl_map[f'uid:{uid}'] = m[-1] |
There was a problem hiding this comment.
Caching the controller by serial alone is unsafe for the dual-port variants already in this HIL config (for example the CH32 fsdev/usbfs rows share one USB serial, and stale same-serial nodes are explicitly handled elsewhere). If the first matching sysfs node is the old port, or the next variant enumerates on a different root controller, every later flash_permit/battery_permit uses the stale slot and the intended one-battery-per-controller throttling no longer protects the controller actually running usbtest. Resolve the current live device each time, or key the cache by the selected sysfs node/variant rather than only uid.
Useful? React with 👍 / 👎.
| "device/cdc_msc_freertos", | ||
| "device/hid_composite_freertos", | ||
| "device/audio_test_freertos", | ||
| "device/usbtest", |
There was a problem hiding this comment.
When --build is used for this Espressif row, tools/build.py:get_examples() only includes Espressif examples whose directory name contains freertos plus a few explicit exceptions, so adding device/usbtest here never produces a firmware under cmake-build-.../device/usbtest. test_example() then reports Skip (no binary), meaning the S3 HIL config appears to enable usbtest but never actually runs that battery; add usbtest to the Espressif build list (or make it a freertos-named example) before enabling it here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed from the other direction: rather than making device/usbtest buildable under the Espressif fleet, it was left out of the only allowlist for both espressif rows in tinyusb.json, with an explicit comment ('only IDF/FreeRTOS examples are part of the espressif fleet build; device/usbtest builds under IDF but is not built/flashed by the fleet, so it is not listed').
- EP0 OUT: park a back-to-back data-stage packet the DCP accepted before PID could go NAK and deliver it into the next armed chunk; flow-control the single-buffer control pipe between chunks (usbtest ctrl_out corruption); discard a packet parked while an OUT pipe was halted so BOT reset recovery's fresh CBW read can't receive stale WRITE data - HS UTMI PHY power-up per the FSP sequence, shared by dcd/hcd: CLKSEL programmed from the board XTAL (EK-RA8M1 runs 20 MHz; the 24 MHz reset default never locks) while DIRPD holds the PHY down, then timed release - hw/bsp(ra8m1_ek): fix U60CK divider macro - BSP_CFG_U60CK_DIV used the generic USB_CLOCK_DIV_8 encoding (7), which USB60CKDIVCR rejects, leaving the USBHS link domain at 480 MHz; the USB60-specific BSP_CLOCKS_USB60_CLOCK_DIV_8 (4) sticks and yields the required 60 MHz from PLL1P - support FS-only builds on the high-speed port: gate SYSCFG.HSE on TUD_OPT_HIGH_SPEED (RHPORT_DEVICE_SPEED=OPT_MODE_FULL_SPEED was a silent no-op) and always compile both hwfifo access widths - the FIFO width belongs to the module, not the link speed (FS builds corrupted odd-length tails: 16-bit access against MBW-32) - iso activate: reset stale pipe bookkeeping so a BRDY firing before the class re-arms can't replay a pre-SET_INTERFACE transfer; write PIPEBUF after PIPESEL selects the pipe (PIPESEL-windowed register) - clear-halt: re-assert BUF on a still-armed OUT pipe (usbtest case 29) - bound the D0FIFO ready spin so an undrained double-buffered IN pipe can't freeze the stack with the IRQ masked - usbtest example: cap interrupt mps at 64 on RUSB2 high speed (pipes 6-9 have a fixed 64-byte buffer, RA6M5 UM 29.1) Verified: usbtest 30/30 on ra6m5_ek (HS), ra4m1_ek (FS) and ra8m1_ek (FS-forced build on the HS port). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeF2gZ1M7GWkz6Av4BpKPg
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38c315d6b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (rusb2_is_highspeed_rhport(rhport)) { | ||
| // PIPEBUF is PIPESEL-windowed (RA6M5 UM 29.2.35): write it after selecting the pipe. | ||
| // FIXME (as in dcd_edpt_open): BUFNMB is a fixed 0x08 for every pipe; a real allocator is needed. | ||
| rusb->PIPEBUF = 0x7C08; |
There was a problem hiding this comment.
Fresh evidence after the PIPESEL reorder is that the ISO allocation path still hard-codes PIPEBUF to 0x7C08, whose low BUFNMB bits select the FIFO buffer for the currently selected pipe. On high-speed RUSB2 configurations that allocate ISO alongside existing bulk/interrupt pipes, the new ISO pipe can share buffer number 0x08 with another pipe, so concurrent or stale packets can overwrite each other; assign a unique BUFNMB range per pipe instead of reusing this constant.
Useful? React with 👍 / 👎.
| #define USB_PID (0x4000 | PID_MAP(CDC, 0) | PID_MAP(MSC, 1) | PID_MAP(HID, 2) | \ | ||
| PID_MAP(MIDI, 3) | PID_MAP(VENDOR, 4) ) | ||
| // Unique PID per example: guarantees re-enumeration on re-flash and a fresh host driver match. | ||
| #define USB_PID 0x4006 |
There was a problem hiding this comment.
This example now enumerates as cafe:4006, but tools/usb_drivers/tinyusb_win_usbser.inf still matches only the old odd PID set and has no USB\VID_CAFE&PID_4006&MI_00 entry, with the same gap for other reassigned CDC examples such as 0x4008 and 0x400a. On Windows setups that use the shipped INF for composite CDC interfaces, these examples will no longer bind to usbser after the PID reassignment; update the INF or choose PIDs it already covers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — tinyusb_win_usbser.inf now lists PID_4006, PID_4008, PID_400a, PID_4020, and PID_4022 in both DeviceList sections, covering cdc_dual_ports and the other reassigned even-PID examples.
| #define USB_PID (0x4000 | PID_MAP(CDC, 0) | PID_MAP(MSC, 1) | PID_MAP(HID, 2) | \ | ||
| PID_MAP(MIDI, 3) | PID_MAP(VENDOR, 4) ) | ||
| // Unique PID per example: guarantees re-enumeration on re-flash and a fresh host driver match. | ||
| #define USB_PID 0x400d |
There was a problem hiding this comment.
With this new base PID, desc_device_1 in the same file evaluates USB_PID + 11 to 0x4018, which is also the new PID assigned to device/net_lwip_webserver. When dynamic_configuration enumerates in its MSC mode and the next shuffled HIL or manual example flash is net_lwip_webserver on the same board serial, the host can reuse the cached driver state this change was meant to avoid; choose a second-mode PID outside the reassigned example range.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — desc_device_1's PID is now USB_PID + 0x0100 instead of + 11, explicitly kept outside the 0x40xx per-example space (comment at usb_descriptors.c:75), so it no longer collides with net_lwip_webserver (0x4018). A new pre-commit hook (tools/check_example_pids.py) now asserts uniqueness across defines, literals, and derived PIDs so this class can't recur.
| mcu:MSP430x5xx | ||
| mcu:NUC121 | ||
| mcu:SAMD11 | ||
| # DCD has no isochronous support (dcd_edpt_iso_alloc refuses), tier-4 cannot enumerate: |
There was a problem hiding this comment.
This no-ISO skip list omits RA2A1, but the RUSB2 DCD explicitly maps RA2A1 to pipes 4–7 only with no isochronous pipe, so dcd_edpt_iso_alloc() reaches TU_ASSERT(num) when usbtest's tier-4 descriptor is opened. Building or flashing device/usbtest for ra2a1_ek therefore creates firmware that stalls configuration instead of running the battery; add board:ra2a1_ek or an RA2A1-specific skip here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Superseded by a more precise fix: RA2A1 now gets USBTEST_TIER = 3 at compile time (usb_descriptors.h, gated on BOARD_RA2A1_EK), so the firmware never advertises or opens the tier-4 ISO endpoints — nothing to fail TU_ASSERT(num) on, and the board still runs the rest of the battery instead of being skipped wholesale. The host battery follows the tier advertised in bcdDevice.
hathach
force-pushed
the
usbtest
branch
2 times, most recently
from
864fa89 to
117fc65
Compare
Pool/config: - record real uids (ra8m1_ek), enable usbtest for espressif s3/p4, then park ra6m5_ek and ra8m1_ek in boards-skip (ra6m5's usbtest/MSC traffic can kill the uPD720201 host on its ROM firmware; ra8m1 USBHS bring-up pending); max32666/nrf54lm20 stay enabled - their MosChip flakiness never wedges - re-enable device/usbtest on HS boards (mimxrt1064, ch32v307) now that uPD720201 firmware 2.0.2.6 fixes the command-ring death; mimxrt1015 stays skipped - its HS battery killed the controller on both ROM and 2.0.2.6 firmware (board-specific); match the moved host-test bundles (f723 <-> rt1064); skip never-passing tests on the new nrf5340dk/nrf54lm20dk boards and the detached pico host bundle, each documented with a comment Host-controller quirk gating in usbtest.py (auto-skip, self-heals on a healthy xHCI): - MosChip MCS9990 EHCI: case 25 (int-OUT never scheduled, FRINDEX bug) and case 11 (unlinked reads complete short/EREMOTEIO) - Renesas uPD720201 xHCI: firmware-gated. The card must run firmware >= 2.0.2.6 (RAM-uploaded - it reverts to ROM on every power cycle): on older firmware the command ring dies under unlink stress (a Configure Endpoint command stops completing; the hub worker deadlocks holding the device lock; only a host power cycle recovers; three boards reproduced it). usbtest.py reads the FW version register (PCI config 0x6c) and refuses to run at all on older firmware - hil_test surfaces that as a failed test with the reason. On current firmware the full 30-case battery runs (validated FS+HS: metro_m4, f723, f723-DMA all 30/30). Scheduling (hil_test.py): - Shuffle each (board, variant)'s test order with a seeded RNG (HIL_SHUFFLE_SEED to replay) so usbtest batteries and flash churn spread across the timeline instead of convoying on one controller. - Per-controller usbtest + flash semaphores: HIL_USBTEST_PARALLEL (default 4) concurrent usbtest batteries and HIL_FLASH_PARALLEL (default 8) concurrent flashes per host controller. Profiled on uPD720201 firmware 2.0.2.6 across 8/1..12/8: wall time falls 22.2/14.3/12.5/10.8 min at usbtest width 1/2/3/4 and plateaus there; zero controller errors everywhere; first battery case failures (leaf-hub bandwidth stretch) appear at 12/8, and flash width 12 only amplifies flasher-hub contention flakes - so 8/4 is the optimum. A separate battery-window flash throttle was profiled and dropped. - Give every example a unique hardcoded USB PID (0x4001-0x4022, usbtest keeps 0x4010) instead of the PID_MAP interface bitmap: different examples now always re-enumerate back-to-back, even on boards whose CPU reset does not drop D+ (WCH CH58x), so the EXAMPLE_PID table and same-PID adjacency reordering in hil_test.py are gone; only the variant-boundary same-example repeat needs a swap. - Report matrix: stable columns with the metric-bearing tests pinned first (usbtest, cdc_msc_throughput, msc_file_explorer[_freertos]), the rest alphabetical. Fail fast: - enum wait budget 8 s on the first attempt, 4 s on retries; dfu waits are deadline-based so dfu-util's own runtime counts against the budget. A device-absent failure now costs ~3-5x a passing test (20-30 s) instead of 10-30x (47-150 s). - CI runs hil_test with --retry 1 and no in-run second pass: a broken fixture fails the job fast instead of holding the self-hosted runner for hours and blocking other PRs' HIL jobs. hil_test still writes the .skip sidecar, so a manual re-run attempt only retests what failed. Review fixes (multi-agent adversarial review of this commit): - tinyusb_win_usbser.inf: the PID rework moved five CDC examples onto even PIDs the INF's odd-only DeviceList never matched (legacy-Windows usbser binding) - appended 0x4006/4008/400a/4020/4022 to both lists. - usbtest example: USBTEST_TIER is now overridable and the descriptors and pumps are tier-conditional, so a board whose DCD cannot serve a tier lowers it instead of skipping the whole example - RA2A1 (RUSB2 with no isochronous pipe) builds at tier 3 via its BOARD_ define; the host battery follows the tier advertised in bcdDevice. Tier-4 output verified byte-identical after the refactor. - dynamic_configuration's second config derived USB_PID + 11 = 0x4018, colliding with net_lwip_webserver - now USB_PID + 0x0100, outside the per-example space. tools/check_example_pids.py (pre-commit hook) enforces PID uniqueness incl. derived and literal idProduct values. - usbtest.py firmware gate: matched by device ID (uPD720201/720202, both use the 0x6c FW register), and an unreadable version (setpci missing/denied) now refuses with its own message instead of masquerading as "firmware 0x00000000"; noted the gate is necessary but not sufficient (board-specific kills stay per-board skips). - hil_test: deadline waits use time.monotonic(); multiprocessing context pinned to fork (raw semaphores in Pool initargs); flash and usbtest permits unified into one fail-closed, exception-safe ctrl_permit (unknown controller takes every slot and logs a warning instead of silently borrowing slot 0); an all-skipped battery reports as skip, not "0/0" failure; slow-body polls (mtp, printer, disk read) go through a shared deadline-based wait_until so their bodies count against the enum budget; throughput's FS detection compares serials case-insensitively like every other walk; a missing MSC read-speed line now fails the host msc_file_explorer test instead of passing with an empty metric. Hardening: - fail fast (15 s) when a driver-registry sysfs write blocks: a wedged device otherwise turns every subsequent battery into an unkillable D-state writer and silently hangs the whole run - usb-recover skill: a VM reboot is not a reliable cure (MosChip hubs latch up across the PCIe reset); full host power cycle is Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HeF2gZ1M7GWkz6Av4BpKPg
# Conflicts: # .claude/skills/hil/SKILL.md # test/hil/hil_test.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a686c70631
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| static inline void vendord_abort_ep(uint8_t rhport, uint8_t ep_addr) { | ||
| if (ep_addr) { | ||
| usbd_edpt_stall(rhport, ep_addr); | ||
| usbd_edpt_clear_stall(rhport, ep_addr); |
There was a problem hiding this comment.
Fresh evidence beyond the earlier altsetting-abort thread is that several updated DCD clear-stall paths now intentionally preserve an armed OUT transfer for toggle-reset recovery (for example dcd_rusb2.c:989 checks _dcd.pipe[num].queued, and WCH checks xfer->valid). This helper's stall+clear sequence therefore is not an abort on those ports: switching a vendor interface from an endpoint altsetting to alt 0 while a read is armed can leave the hardware endpoint ACKing after the class zeros ep_out, so a zero-bandwidth altsetting can still consume packets and any late completion no longer matches the selected endpoints. Use a real DCD abort/close path or clear the DCD transfer state before the clear-stall step.
Useful? React with 👍 / 👎.
- dcd_edpt_stall flushes the primed buffer (ENDPTFLUSH), but the aborted transfer's dQH overlay can be left ACTIVE with mid-transfer state; the next prime after clear-halt then resumes the stale overlay instead of loading the fresh qtd, so post-halt IN reads return mid-buffer data (usbtest case 13 'buf[32] = 56 (not 0)', with case 18 failing downstream of the same corruption in the full battery). qhd_start_xfer now clears overlay.active alongside overlay.halted before linking the new qtd. - test/hil(hfp): drop lpcxpresso43s67's device/usbtest skip - the historical first-case wedge no longer reproduces on this branch, and with the overlay fix the board runs 30/30 on its Fresco xHCI host (previously 28/30 with deterministic case 13/18 failures). mimxrt1064_evk (imxrt dcache path) 30/30 regression-clean. - docs(hil skill): document the external hifiphile rig - pool test/hil/hfp.json, SSH-reachable from htpc/ci with no outbound SSH, exercised by the CI hil-tinyusb (hfp.json) job; never run HIL against it during development unless the user explicitly asks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017TQZrFfU3K4Y198aLsUpBC
Summary
Adds
examples/device/usbtest, a device-side peer for the Linux kernel'susbtest.ko/testusbbattery (the gadget-zero source/sink protocol), a host runner (test/hil/usbtest.py), and HIL-suite integration. Driving the full 30-case tier-4 battery (bulk / control / interrupt / isochronous, including unlink, halt, and data-toggle cases) across the rig surfaced and fixed real bugs in 11 device ports plus the vendor class and usbd core.Every device-capable board on the HIL rig now passes 30/30, across 12 DCDs: dwc2 (incl. nRF54LM20 at high speed), rp2040/rp2350, stm32_fsdev, lpc_ip3511, nrf5x, samd, ci_hs, musb, rusb2, ch32_usbfs, ch32_usbhs.
Device example + vendor class
examples/device/usbtest: tiered capability (bcdDevice advertises tier), vendor interface with bulk/int/iso source-sink pairs, iso endpoints in altsetting 1, vendor control 0x5b/0x5c write/read-back. Non-buffered vendor mode so the host never sees an unexpected short packet/ZLP.class/vendor: optional interrupt endpoints (CFG_TUD_VENDOR_EP_INT_*), isochronous endpoints + alternate-setting support (CFG_TUD_VENDOR_EP_ISO_*,CFG_TUD_VENDOR_ALT_SETTINGS), manual RX arming for halt recovery.usbd: harden endpoint/interface standard control requests (GET_STATUS(interface), endpoint feature ACKs, direction check on GET_STATUS data stage), release endpoint claim on clear-stall.DCD fixes (each found by a failing battery case, root-caused on hardware)
queuedflag; deferred-ZLP retrydcd_edpt_iso_alloc/activate(were stubs returning false)CH32_USBFS_EP3_1023_BUFSIZE) while V103/X035/CH58x cap at 64 Binterruptattribute corrupts the return (noWCH-Interrupt-fastsupport)board_get_unique_id(ESIG UID; serial was a placeholder)Host runner + HIL integration
test/hil/usbtest.py: binds the interface via dynamic id, runs the tiered battery with packet-aligned parameters, per-case verdicts with dmesg capture, kernel-hang detection with auto-recovery,--jsonoutput. Rig sudoers allowlist versioned attest/hil/tinyusb-sudoer.test/hil/hil_test.py:device/usbtestadded to the device battery; the report cell shows the case count (✅ 30/30, or❌ 29/30on a partial via the new fail-with-metric path) and the tally counts icon-prefixed cells correctly.test/hil/tinyusb.json: CH32V307 registered with usbhs/usbfs variants; adds ~50 s per board to a full sweep.Validation
pre-commit(format/codespell/unit tests) clean; full example sets build across the touched families🤖 Generated with Claude Code
https://claude.ai/code/session_01HeF2gZ1M7GWkz6Av4BpKPg