Provide default implementation of usb ControlPipe trait

[Dominaezzz]

The ControlPipe trait has this comment.

https://github.com/embassy-rs/embassy/blob/fb22b46ebb40c16e35c651c0dacf810126856927/embassy-usb-driver/src/lib.rs#L244-L247

The comment makes sense but it would be really handy if there was a default implementation of it for drivers that don't have this special handling.

I'm thinking something like this.

pub struct DefaultControlPipe<E> {
    endpoint: E,
    state: Idk,
}

impl<E: EndpointIn + EndpointOut> ControlPipe for DefaultControlPipe<E> {
    // ......
}

Then I could just use in my driver like so.

impl<'d> Driver<'d> for MyDriver<'d> {
    type ControlPipe = DefaultControlPipe<MyCombinedEnpoint>;
}

Unfortunately for me, I'm only just starting to learn USB (device-side) so I don't actually know how to implement this (otherwise I would send a PR).

I'm in the middle of trying to add embassy-usb support to esp-hal by creating a wrapper around it's implementation of usb_device::bus::UsbBus. It's been going somewhat smoothly until I hit the ControlPipe trait and I'm unsure about how to implement the setup method (why does it only return 8 bytes?) or what to do with the first/last arguments of the data_in/data_out methods. If I had this DefaultControlPipe, I wouldn't have to worry about it :sweat_smile:.

[Dirbaio]

Seems the esp32 uses the same synopsys usb otg peripheral as found in some stm32's, you should be able to copy and adapt the embassy-usb driver found in embassy-stm32. Seems the usb-device one needed very little changes, so it should be easy https://github.com/esp-rs-compat/synopsys-usb-otg/commit/7a86c9316784934f84b3692df64af3279166aabe

FWIW the synopsys usb otg does need special handling for the control pipe, the usb-device does extra work to make it appear as endpoint 0 is a regular endpoint, doing ifs internally, because usb-device requires it. The semantics of the control pipe are really tricky, it's historically been very buggy especially with the usb-device model, so I predict if you were to write this "default control pipe" adapter you'll still run into a lot of trouble. I really wouldn't recommend it.

I'm unsure about how to implement the setup method (why does it only return 8 bytes?)

it needs to wait for a SETUP packet and return it. the SETUP packets are always 8 bytes.

or what to do with the first/last arguments of the data_in/data_out methods.

data in control transfers is chunked. embassy-usb chunks it for you and gives you the chunks. In some hardware, you can optimize stuff if you know a packet is the first or the last (for example, set a flag when sending the last packet which is "after this packet, automatically end the control transfer"), so embassy-usb tells you so. You can just ignore them.

Also note the "status phase" of the control transfer needs to be a 0-length packet in the opposite direction as the "data phase". embassy-usb requires the ControlPipe to handle it, while usb-device does an explicit 0-length read/write. So you'll have to do that yourself too when adapting from embassy-usb to usb-device. See docs of the call sequences in https://docs.embassy.dev/embassy-usb-driver/git/default/trait.ControlPipe.html

As I said, the semantics of control pipe handling in embassy-usb and usb-device are quite different, I predict you'll run into issues if you try to write an adapter...

[Dirbaio]

I just realized I wrote a lot of stuff, but didn't address the original question of this issue.

The comment makes sense but it would be really handy if there was a default implementation of it for drivers that don't have this special handling.

I haven't seen hardware that doesn't need special handling for the control pipe. You pretty much always do, because SETUP packets are different and always need special handling.

Assuming this, the only use case left is "write adapters for usb-device drivers". Due to the reasons above I don't think this'll yield good results so I'd say it's not a strong motivation.

So, I don't think we should have a default control pipe implementation like that, unfortunately :(

[Dominaezzz]

you should be able to copy and adapt the embassy-usb driver found in embassy-stm32

I might just do this, I felt bad copying over large chunks of code but it looks like I have very little options. At least this way I can throw in DMA support and other fun esp32 things.

FWIW the synopsys usb otg does need special handling for the control pipe.

I have no choice then, esp-hal wouldn't accept a PR with unnecessary shoehorning.

it's historically been very buggy

In what context do you mean this? You mean in all USB implementations in general or just usb-device?

so I predict if you were to write this "default control pipe" adapter you'll still run into a lot of trouble. I really wouldn't recommend it.

Sounds like I was right to give up haha, Looking deeper, usb-device does do a lot of work internally to hide the truth from users like you say. Do you think the "default control pipe" adapter could still be useful outside wrapping usb-device? At the moment it looks like the Endpoint traits don't support control transfers at all so that'll be needed first. Or perhaps no sane usb-device should have any control endpoints besides the default one. I don't know enough about USB to make this call. I generally believe that if USB supports it then the stack should allow/expose it.

In some hardware, you can optimize stuff if you know a packet is the first or the last.

This makes sense for last but can't the driver figure out if the packet is the first one or not?

Also note the "status phase" of the control transfer needs to be a 0-length packet in the opposite direction as the "data phase".

This is a fun thing I discovered while trying to write the adapter, how is the driver supposed to know the direction of the 0-length transfer when there's no data phase? i.e. What does accept() do?

Thanks for explaining the USB device quirks!

[Dominaezzz]

So, I don't think we should have a default control pipe implementation like that, unfortunately :(

Fair enough, the issue can be revisit once such hardware pops up.

adapt the embassy-usb driver found in embassy-stm32

Oh btw, was there some kind of reference manual for the synopsys usb otg peripheral?

[Dirbaio]

it's historically been very buggy

In what context do you mean this? You mean in all USB implementations in general or just usb-device?

usb-device, yes. this was one of the motivations to have a separate ControlPipe trait in embassy-usb.

Do you think the "default control pipe" adapter could still be useful outside wrapping usb-device? At the moment it looks like the Endpoint traits don't support control transfers at all so that'll be needed first. Or perhaps no sane usb-device should have any control endpoints besides the default one.

yeah control transfers are only ever used in endpoint 0.

This makes sense for last but can't the driver figure out if the packet is the first one or not?

yeah, it was just for convenience.

Oh btw, was there some kind of reference manual for the synopsys usb otg peripheral?

the Reference Manual documents for any stm32 chip that has OTG have docs, for example this one. I'd guess espressif has something similar?

how is the driver supposed to know the direction of the 0-length transfer when there's no data phase? i.e. What does accept() do?

for non-zero-length the status phase is in the opposite direction as data. for zero-length it's always IN (the opposite direction of the SETUP packet). (I think? there might be more edge cases, it's been a while).

[Dominaezzz]

yeah control transfers are only ever used in endpoint 0.

Ah in that case the ControlPipe trait makes sense to have even if hardware doesn't insist on it, and a default implementation won't make sense. This clears things up for me.

I'd guess espressif has something similar?

It's similar but the critical bit (the registers) requires NDA sadly. STM32 one actually shows the registers and the meanings of the fields which is great, thanks!

A bit of feedback about the documentation on the traits, when trying to implement them I found myself wondering when exactly the methods should return. What should've been completed before the future returns Ready. I'll open a separate issue with more concrete examples of what I mean.

Thanks again for explaining the USB stuff! I'll close this issue as we both agree a "default control pipe" isn't useful.

[MabezDev]

@Dominaezzz We have support for the esp32s2 & esp32s3 for use with usb-device here: https://github.com/esp-rs-compat/synopsys-usb-otg. I don't really know why the registers are behind NDA, but the IP block is basically identical to the stm32 ones, you can see the 5 extra commits in the repo for the differences. I imagine if the synopsis parts of the impl in embassy-stm32 were refactored out into its own crate it would be quite straightforward to add esp support.