dcd/ch32hs: bus speed detection and data toggle gate#3727
Conversation
|
@claude review |
|
Claude finished @HiFiPhile's task in 2m 20s —— View job Code Review
SummaryTwo well-motivated changes to the CH32 USBHS driver. The approach is sound, and the OUT-toggle gate aligns with the pattern already established for USBFS variants ( Change 1 — Defer bus reset for speed detection (
|
There was a problem hiding this comment.
Pull request overview
This PR updates the CH32 USBHS device controller driver (dcd_ch32_usbhs.c) to improve correctness around bus reset handling and OUT packet processing, aiming to better align behavior with actual negotiated bus speed and to avoid consuming retransmitted/out-of-sequence OUT data.
Changes:
- Defer
DCD_EVENT_BUS_RESETemission until a subsequent SETUP packet so the driver can report the actual detected speed rather than assuming high-speed. - Gate OUT packet processing on the controller’s toggle-valid indication to drop packets with unexpected DATA0/DATA1 state.
💡 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 sizeNo entries. No changes
|
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| ch32v307v_r1_1v0/dfu_runtime | 9,380 → 9,476 (+96) | — | — | — | 9,876 → 9,972 (+96) | +1.0% |
| ch32v307v_r1_1v0/hid_generic_inout | 10,520 → 10,616 (+96) | — | — | — | 11,016 → 11,112 (+96) | +0.9% |
| ch32f205r-r0/dfu_runtime | 7,940 → 8,016 (+76) | — | — | — | 8,812 → 8,888 (+76) | +0.9% |
| ch32f205r-r0/hid_generic_inout | 8,956 → 9,032 (+76) | — | — | — | 9,656 → 9,732 (+76) | +0.8% |
| ch32v307v_r1_1v0/hid_composite | 11,712 → 11,808 (+96) | — | — | — | 12,208 → 12,304 (+96) | +0.8% |
| ch32v307v_r1_1v0/midi_test | 11,848 → 11,944 (+96) | — | — | — | 12,344 → 12,440 (+96) | +0.8% |
| ch32v307v_r1_1v0/hid_multiple_interface | 11,340 → 11,432 (+92) | — | — | — | 11,836 → 11,928 (+92) | +0.8% |
| ch32v307v_r1_1v0/hid_boot_interface | 11,364 → 11,456 (+92) | — | — | — | 11,860 → 11,952 (+92) | +0.8% |
| ch32f205r-r0/hid_multiple_interface | 9,568 → 9,644 (+76) | — | — | — | 10,440 → 10,516 (+76) | +0.7% |
| ch32f205r-r0/hid_boot_interface | 9,624 → 9,700 (+76) | — | — | — | 10,452 → 10,528 (+76) | +0.7% |
Signed-off-by: HiFiPhile <admin@hifiphile.com>
Signed-off-by: HiFiPhile <admin@hifiphile.com>
HiFiPhile
force-pushed
the
dcd_ch32hs_spd
branch
from
1f7a8e8 to
ca89edf
Compare
Hardware-in-the-loop (HIL) Test Reporthfp-iar
Legend: ✅ pass · ❌ fail · ⚪ skipped · blank not run hfp.json
Legend: ✅ pass · ❌ fail · ⚪ skipped · blank not run tinyusb.json
Legend: ✅ pass · ❌ fail · ⚪ skipped · blank not run |
Signed-off-by: HiFiPhile <admin@hifiphile.com>
HiFiPhile
changed the title
|
@codex review |
![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: de01a33b84
ℹ️ 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".
| } else if (token == USBHS_TOKEN_PID_OUT && (int_status & USBHS_DEV_UIS_TOG_OK)) { | ||
| update_out(rhport, ep_num, len); |
There was a problem hiding this comment.
When an isochronous OUT endpoint is active, queue_out_packet() and update_out() deliberately skip programming/advancing the data toggle for xfer->is_iso, because ISO transfers do not use the normal retry toggle sequence. This new global TOG_OK gate therefore risks dropping ISO OUT completions whenever the hardware does not assert a meaningful toggle check for that endpoint; limit the check to non-ISO endpoints so audio/ISO OUT payloads still reach update_out().
Useful? React with 👍 / 👎.
| // This interrupt probably triggered at start of bus reset before the speed negotiation has completed. | ||
| // Defer the bus reset event until the next setup packet is received in order to determine the actual speed of the bus. | ||
| reset_evt_pending = true; |
There was a problem hiding this comment.
When a configured device receives a bus reset, this now only sets reset_evt_pending, so any SOF or transfer interrupt that arrives before the first SETUP is still processed with the old TinyUSB configured state; DCD_EVENT_SOF even invokes class SOF handlers immediately in ISR context before the queued reset can run. If the reset is followed by normal SOFs before enumeration, or by suspend/disconnect with no SETUP, the stack remains stale until a SETUP arrives; reset the core immediately or suppress other events while the deferred speed detection is pending.
Useful? React with 👍 / 👎.
This pull request improves the handling of USB bus reset and OUT packet processing in the
dcd_ch32_usbhs.cdriver. The main changes include deferring the bus reset event until the actual device speed can be determined and filtering OUT packets based on the data toggle status.Bus Reset Handling Improvements:
reset_evt_pendingflag and related logic) [1] [2] [3]OUT Packet Processing:
Speed selection in
dcd_init()These changes improve the reliability and standards-compliance of the USB device controller implementation.