fix shared epx for rp2040#3503
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to address RP2040 host-controller behavior where control endpoint state (EPX/EP0 max packet size) can become stale across different devices, leading to fragmented control transfers that some devices fail to reassemble.
Changes:
- Re-opens EP0 inside
usbh_setup_send()using the device’sbMaxPacketSize0to ensure correct control max packet size before sending a SETUP packet.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Re-open EP0 with the correct max packet size for this device. | ||
| // The RP2040 shares a single EPX for all control transfers, so its | ||
| // wMaxPacketSize can be stale from a different device's enumeration. | ||
| // Without this, a CBI device with MPS=64 can have its CDB split into | ||
| // multiple packets when a low-speed device (MPS=8) enumerated last. | ||
| uint8_t ep0_mps = 8; // default for addr0 | ||
| if (daddr > 0) { | ||
| usbh_device_t const* dev = get_device(daddr); | ||
| if (dev && dev->bMaxPacketSize0) { | ||
| ep0_mps = dev->bMaxPacketSize0; | ||
| } | ||
| } | ||
| usbh_edpt_control_open(daddr, ep0_mps); |
There was a problem hiding this comment.
usbh_setup_send() now calls usbh_edpt_control_open() for every control transfer on all host controllers. For HCDs like DWC2, hcd_edpt_open() allocates a fresh endpoint each call (no “already opened” check), so this can exhaust endpoint slots / leak state and break control transfers. This re-open should be guarded to only run on controllers that share EP0/EPX across devices (e.g. RP2040), or moved behind a dedicated opt-in config/HCD hook so other ports keep the existing behavior.
| ep0_mps = dev->bMaxPacketSize0; | ||
| } | ||
| } | ||
| usbh_edpt_control_open(daddr, ep0_mps); |
There was a problem hiding this comment.
The return value of usbh_edpt_control_open(daddr, ep0_mps) is ignored. If the open fails, the code still proceeds to hcd_setup_send(), which can lead to confusing failures later. Consider handling the failure here (e.g., set control stage back to IDLE and return false) so callers’ TU_ASSERT(usbh_setup_send(...)) behaves correctly.
| usbh_edpt_control_open(daddr, ep0_mps); | |
| if (!usbh_edpt_control_open(daddr, ep0_mps)) { | |
| _control_set_xfer_stage(CONTROL_STAGE_IDLE); | |
| return false; | |
| } |
HiFiPhile
left a comment
HiFiPhile
left a comment
There was a problem hiding this comment.
Hi, you shouldn't put rp2040's fix in usbh.
|
I was able to move this to hcd_rp2040. |
|
These are all the changes I needed to make for an XInput driver and bespoke MSC driver that supports removable media on both CBI and BOT. Because I want to use floppy drives. I may even add LUN support. The shared control endpoint had so many problems that I nearly gave up. The final piece was hcd_pause_interrupt_eps. I'm seeing massive stability improvements with this upgraded hcd. Testing error conditions by removing media while reading is how found all these edge cases that don't normally show up with the regular MSC driver. I hope they are useful to others making custom drivers. |
|
It looks like this code may be a bit too defensive. Turns out I have a device that's hitting a hardware bug. |
Not all devices will reassemble fragmented control packets. This fix might benefit from being guarded.