machine/rp2: expose usb endpoint stall handling #4850

mikesmitty · 2025-04-11T22:19:17Z

Since the USB mass storage PR (#4844) is getting a bit monolithic I figured I'd break out smaller parts of it as I can. I figure I'll work my way through the MCUs that support USB natively, but here's the RP2 changes to start. If there's any suggestions on which MCUs would make the most sense to add support next I'm happy to hear it

src/machine/machine_rp2040_usb.go

deadprogram · 2025-04-15T09:09:25Z

Now that #4856 has been merged, this PR is the next up @mikesmitty

Can you please resolve the merge conflicts with the dev branch?

mikesmitty · 2025-04-15T13:37:23Z

Sure thing, done

deadprogram · 2025-04-15T14:06:59Z

If there's any suggestions on which MCUs would make the most sense to add support next I'm happy to hear it

The SAMD21/SAMD51 are the next logical addition, in part because we also need more USB implementations! See #4751

mikesmitty · 2025-04-15T14:28:17Z

Sounds good, coincidentally I started looking at the USB support in the atsamd21 the other day. I'm still working through the FAT12 emulation layer right now, but I'll come back to it when I'm done

deadprogram · 2025-04-15T15:32:04Z

@eliasnaur any other feedback now on this PR? Looks good to me.

eliasnaur

Some comments. I don't know the USB protocol nor the rpxxxx USB hardware, so my comments may well be misguided.

src/machine/usb.go

eliasnaur · 2025-04-15T15:50:01Z

src/machine/machine_rp2_usb.go

@@ -138,7 +150,8 @@ func sendViaEPIn(ep uint32, data []byte, count int) {
 	_usbDPSRAM.EPxBufferControl[ep&0x7F].In.Set(val)
 }

-func sendStallViaEPIn(ep uint32) {
+// Enable ENDPOINT_HALT/stall on a USB IN endpoint
+func SetStallEPIn(ep uint32) {


Can you explain why the SetStall* functions must be exported? Can't you infer their calls from the nil-ness of ep.StallHandler below?

Sure, the short version is that a USB endpoint halt/stall is a form of flow control/out-of-band communication that specific protocols on the device can use to signal to the host when something goes wrong. Individual USB drivers on the device are assigned endpoints (akin to network ports) and the mass storage driver needs to be able to stall those endpoints to notify the host of fatal errors so just the mass storage connection can be reset when needed. In response the host sends a secret handshake to the mass storage driver via a series of USB Setup Packets, the mass storage driver sees this and recognizes the host is back in sync, un-stalls its endpoints and mass storage communication can resume.

If you're interested in more detail about the flow, this is the long version:

In the mass storage Bulk-Only Transfer (BOT) protocol for example the typical communication flow goes like this:

USB host sends a CBW (Command Block Wrapper) to the device containing a SCSI command (e.g. a READ command)

USB device validates the CBW and if everything checks out it immediately starts sending data from the requested sector/block, packet by packet as the host ACKs them. Each sector/block takes 8 full packets to send so the host will keep accepting raw packets as the requested data until it gets the number of bytes it requested in the original CBW

When the device finishes transferring the data it finishes the transaction by sending a CSW (Command Status Wrapper) with the final status of the command and a few other details, then starts waiting for the next CBW to arrive

If the CBW gets corrupted in transit however, the device won't starting sending the requested data to ensure data integrity. There's no frame encapsulating the data, it's just full buffers of data so sending a CSW immediately would be interpreted by the host as part of the response to its READ command. Instead, the device stalls its TX and RX endpoints and refuses to perform any commands. The stall is an actively negotiated status between the host and the device by the underlying host/device USB firmwares so the host will stop listening for the response data and perform a BOT Reset Recovery process (or sometimes take the easy way out, power-cycling the device haha). The Reset Recovery process is three requests sent to the control endpoint (endpoint 0) in this specific order:

A "Bulk-Only Mass Storage Reset" request (class-specific)

A standard CLEAR_FEATURE request with the ENDPOINT_HALT function, addressed to the stalled mass storage IN (device TX) endpoint. CLEAR_FEATURE is also used for other features, but in this case it's for

Another standard CLEAR_FEATURE request, addressed to the stalled mass storage OUT (device RX) endpoint

This is where the Setup Packets come in, all three of these commands get sent using USB Setup Packets. Typically just for initial negotiation/configuration, but also for recovery procedures. Since the first request is marked as a class-specific request and addressed to the mass storage interface id it gets routed through the usb.SetupConfig.Handler function, but since CLEAR_FEATURE is a standard request it gets routed to machine.handleStandardSetup() where most standard requests are handled. Endpoint 0 stalls/un-stalls a lot in the course of its normal processes so that makes sense, but these specific requests are addressed to the stalled mass storage endpoints with the wIndex field and the mass storage driver needs to receive them so it knows when to un-stall its endpoints. That's the reason behind adding the usb.EndpointConfig.StallHandler

Or to put it less verbosely (apologies for the novel), the mass storage driver calls SetStall*() to signal the host of an issue then waits for the host to acknowledge it via Setup Packets to the usb.SetupConfig.Handler and usb.EndpointConfig.StallHandler functions, then the mass storage driver can resume the communications by clearing the stall conditions

Nice explanation, thank you! (now that you went through the trouble, consider adding (some of) your explanation somewhere in the documentation).

I think I understand the need for the exports now. However, I don't understand why they're not methods on USBDevice. I see there's already machine.ReceiveUSBControlPacket, so I suppose it's not a new pattern. I do worry locking us into a singleton USBDevice will bite our behinds in future.

Sure, that makes sense. I'll update that here in a sec

consider adding (some of) your explanation somewhere in the documentation).

Yes please!!

Will do, for sure!

src/machine/usb.go

mikesmitty · 2025-04-15T23:21:48Z

It's interesting that the sizes_test is failing again. I just updated the size for that test from 7359 to 7395 in the previous PR and now it's flipped back

src/machine/usb.go

eliasnaur · 2025-04-16T04:15:41Z

src/machine/machine_rp2_usb.go

@@ -138,7 +150,8 @@ func sendViaEPIn(ep uint32, data []byte, count int) {
 	_usbDPSRAM.EPxBufferControl[ep&0x7F].In.Set(val)
 }

-func sendStallViaEPIn(ep uint32) {
+// Enable ENDPOINT_HALT/stall on a USB IN endpoint
+func SetStallEPIn(ep uint32) {


Nice explanation, thank you! (now that you went through the trouble, consider adding (some of) your explanation somewhere in the documentation).

I think I understand the need for the exports now. However, I don't understand why they're not methods on USBDevice. I see there's already machine.ReceiveUSBControlPacket, so I suppose it's not a new pattern. I do worry locking us into a singleton USBDevice will bite our behinds in future.

src/machine/machine_rp2_usb.go

deadprogram · 2025-04-16T10:19:16Z

@mikesmitty looks like this PR needs to be rebased against the latest dev, please.

eliasnaur

A question and another nit. LGTM, with the caveat that I don't speak USB :)

eliasnaur · 2025-04-17T05:19:23Z

src/machine/machine_rp2_usb.go

+	_usbDPSRAM.EPxBufferControl[ep].In.ClearBits(val)
+	if epXPIDReset[ep] {
+		// Reset the PID to DATA0
+		setEPDataPID(ep&0x7F, false)


Nit: the masking is redundant here.

eliasnaur · 2025-04-17T05:20:47Z

src/machine/machine_rp2_usb.go

@@ -149,6 +162,37 @@ func sendStallViaEPIn(ep uint32) {
 	_usbDPSRAM.EPxBufferControl[ep&0x7F].In.Set(val)
 }

+// Set ENDPOINT_HALT/stall status on a USB OUT endpoint.
+func (dev *USBDevice) SetStallEPOut(ep uint32) {
+	if ep == 0 {


Should ep be masked before comparing with 0?

The 0 endpoint is a special case. It's both an in and out endpoint in one so there's no direction bit applied for it, but couldn't hurt

deadprogram · 2025-04-17T08:15:39Z

Thanks for the work on this PR @mikesmitty and to @eliasnaur for the excellent feedback.

Now merging.

eliasnaur · 2025-04-17T13:06:40Z

@mikesmitty I'd still like to resolve the two comments above. Sorry for approving too soon (my intention was to say that I found no more issues with the PR).

mikesmitty · 2025-04-17T13:19:42Z

Sure, I'll make those changes in the main PR

mikesmitty force-pushed the ms/expose-usb-stall branch 2 times, most recently from 713b9df to baff40b Compare April 11, 2025 23:18

eliasnaur reviewed Apr 12, 2025

View reviewed changes

src/machine/machine_rp2040_usb.go Outdated Show resolved Hide resolved

mikesmitty mentioned this pull request Apr 12, 2025

machine/rp2: merge common usb code #4856

Merged

deadprogram mentioned this pull request Apr 15, 2025

usb: add USB mass storage class support #4844

Open

mikesmitty force-pushed the ms/expose-usb-stall branch from baff40b to 763f846 Compare April 15, 2025 13:29

eliasnaur reviewed Apr 15, 2025

View reviewed changes

eliasnaur reviewed Apr 16, 2025

View reviewed changes

mikesmitty force-pushed the ms/expose-usb-stall branch 2 times, most recently from cf78f4e to 624630d Compare April 16, 2025 14:06

machine/rp2: expose usb endpoint stall handling

9f7f4a1

mikesmitty force-pushed the ms/expose-usb-stall branch from 7ec7ea8 to 9f7f4a1 Compare April 17, 2025 01:16

eliasnaur approved these changes Apr 17, 2025

View reviewed changes

deadprogram merged commit febb390 into tinygo-org:dev Apr 17, 2025
18 checks passed

mikesmitty deleted the ms/expose-usb-stall branch April 17, 2025 13:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

machine/rp2: expose usb endpoint stall handling #4850

machine/rp2: expose usb endpoint stall handling #4850

mikesmitty commented Apr 11, 2025 •

edited

Loading

deadprogram commented Apr 15, 2025

mikesmitty commented Apr 15, 2025

deadprogram commented Apr 15, 2025

mikesmitty commented Apr 15, 2025

deadprogram commented Apr 15, 2025

eliasnaur left a comment

eliasnaur Apr 15, 2025

mikesmitty Apr 15, 2025

mikesmitty Apr 16, 2025

eliasnaur Apr 16, 2025

mikesmitty Apr 16, 2025

deadprogram Apr 17, 2025

mikesmitty Apr 17, 2025

mikesmitty commented Apr 15, 2025

eliasnaur Apr 16, 2025

deadprogram commented Apr 16, 2025

eliasnaur left a comment

eliasnaur Apr 17, 2025

eliasnaur Apr 17, 2025

mikesmitty Apr 17, 2025

deadprogram commented Apr 17, 2025

eliasnaur commented Apr 17, 2025

mikesmitty commented Apr 17, 2025

machine/rp2: expose usb endpoint stall handling #4850

machine/rp2: expose usb endpoint stall handling #4850

Conversation

mikesmitty commented Apr 11, 2025 • edited Loading

deadprogram commented Apr 15, 2025

mikesmitty commented Apr 15, 2025

deadprogram commented Apr 15, 2025

mikesmitty commented Apr 15, 2025

deadprogram commented Apr 15, 2025

eliasnaur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikesmitty commented Apr 15, 2025

Choose a reason for hiding this comment

deadprogram commented Apr 16, 2025

eliasnaur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadprogram commented Apr 17, 2025

eliasnaur commented Apr 17, 2025

mikesmitty commented Apr 17, 2025

mikesmitty commented Apr 11, 2025 •

edited

Loading