USB Driver trait documentation is unclear about some things

[Dominaezzz]

I'm trying to implement embassy_usb_driver::Driver for the MAX3421E using embedded_hal_async::spi::SpiDevice and some parts of the trait docs aren't clear.

1. What is Endpoint::wait_enabled? Is it just waiting for Driver::start to be called?
2. Must EndpointIn::write wait for the host to ACK the transfer before returning? Could the packet be buffered?
3. What are the requirements/expectations with regards to cancel safety? Should I expect embassy-usb to not cancel futures I return? Should I expect the entire driver to be recreated?
4. How to handle errors in implementation? i.e. SpiDevice returns an error. Just panic?
5. Should Bus::remote_wakeup wait for the operation to complete or simply just start it?
6. What's with the variation in async/sync in the Bus? Why is stalling/enabling not async? At the moment I'm having to performing these operations asynchronously as the SPI is async and it's not safe to block.

[Dirbaio]

What is Endpoint::wait_enabled? Is it just waiting for Driver::start to be called?

it waits for the endpoint to be enabled. "enabled/disabled" is a thing defined by USB. For example endpoints get disabled on reset, they get enabled/disabled when you set a configuration or an interface altsetting depending on whether they're used or not.

Must EndpointIn::write wait for the host to ACK the transfer before returning? Could the packet be buffered?

buffering is allowed. existing impls in this repo already buffer 1 packet, but buffering more is allowed.

What are the requirements/expectations with regards to cancel safety? Should I expect embassy-usb to not cancel futures I return? Should I expect the entire driver to be recreated?

embassy-usb itself won't cancel calls into Bus/ControlPipe. The user can cancel stuff, no guarantees are made if they do that (data might be written or not written).

How to handle errors in implementation? i.e. SpiDevice returns an error. Just panic?

True, there's no good way of doing this right now. Doing this properly will require an "Error" associated type, this is breaking so it'll have to wait for the next embassy-usb-driver version.

Should Bus::remote_wakeup wait for the operation to complete or simply just start it?

implementation-defined. (remote_wakeup is just setting a signal level on the DP/DM lines for 20ms. the host may or may not wakeup, and after wakeup it may have to reenumerate which might take seconds. So it doesn't really matter whether it waits for these 20ms or not, because you can't assume that USB is usable immediately after, anyway).

What's with the variation in async/sync in the Bus? Why is stalling/enabling not async? At the moment I'm having to performing these operations asynchronously as the SPI is async and it's not safe to block.

good point. we maybe should make them async in the next driver version. for now you can use blocking SPI, or use a background task, or queue it later and do it in poll().

[Dominaezzz]

Thanks for getting back to me!

What is Endpoint::wait_enabled? Is it just waiting for Driver::start to be called?

it waits for the endpoint to be enabled. "enabled/disabled" is a thing defined by USB. For example endpoints get disabled on reset, they get enabled/disabled when you set a configuration or an interface altsetting depending on whether they're used or not.

Apologies for making you explain the USB spec to me 😅. After glancing through the stm32 implementation, I did figure out that enable/disable can happen when configuration is set or an alternate interface is set. As a Driver implementation though, I don't (and shouldn't I guess) have access to this information (unless I start doing embassy-usb's job of handling control transfers) to apply this state. However, there is the Bus::endpoint_set_enabled which is how embassy-usb tells the driver to enable/disable an endpoint.

I think the documentation should mention this explicit relationship between Endpoint::wait_enabled and Bus::endpoint_set_enabled. As a Driver implementation, I don't care about configurations and interfaces, I just want to know what to do to the hardware when the stack asks for something.

After implementing this, I quickly realised that embassy-usb could actually handle this enable/disable logic in the stack, is there a particular reason this is delegated to the Driver. I'm not against it but my MAX3421E implementation does it all in software, so I have to ask.

Assuming there's a good reason for the above, the documentation isn't super clear about what states the endpoints should be in. For example,

• I think the docs should mention that the endpoints are initially disabled when the driver is created. (This one is a bit more obvious I guess but it doesn't hurt to mention 😄)
• When the bus is reset, the Driver implementation is expected to disable all the endpoints. The stack won't call Bus::endpoint_set_enabled like it does for config and interface changes.

Must EndpointIn::write wait for the host to ACK the transfer before returning? Could the packet be buffered?

buffering is allowed. existing impls in this repo already buffer 1 packet, but buffering more is allowed.

You think it's worth mentioning in the doc? Or is this just too obvious as someone who actually know USB?

What are the requirements/expectations with regards to cancel safety? Should I expect embassy-usb to not cancel futures I return? Should I expect the entire driver to be recreated?

embassy-usb itself won't cancel calls into Bus/ControlPipe. The user can cancel stuff, no guarantees are made if they do that (data might be written or not written).

It does actually. On every iteration of this loop, one of these futures get cancelled. Ideally the future should be kept alive for the next iteration I guess. https://github.com/embassy-rs/embassy/blob/1b582c6830e047154c9a9883d85b7246392986d9/embassy-usb/src/lib.rs#L284-L291 The setup() getting cancelled is problematic for me because I may end up dropping messages because of it. Should I consider this a bug, should I expect cancellation to mean embassy-usb doesn't mind losing setup packets or should I guarantee some atomicity (I'd be forced to panic when I lose the guarantee)? I ask because the USB protocol may be fine with this, I don't know (Apologies for my lack of spec knowledge again 😅)

In the case that the user or class implementation cancels, I can treat that as "I don't care if that packet was written or not" / "I don't care I may have dropped a packet I was trying to read" (and also the endpoint is still usable for future packets) ? Is this worth documenting? At the moment I panic!() if I (might've) dropped a packet.

How to handle errors in implementation? i.e. SpiDevice returns an error. Just panic?

True, there's no good way of doing this right now. Doing this properly will require an "Error" associated type, this is breaking so it'll have to wait for the next embassy-usb-driver version.

Is there a "list of breaking changes for the next version" I can add this too? I don't want it to be forgotten.

Should Bus::remote_wakeup wait for the operation to complete or simply just start it?

implementation-defined. (remote_wakeup is just setting a signal level on the DP/DM lines for 20ms. the host may or may not wakeup, and after wakeup it may have to reenumerate which might take seconds. So it doesn't really matter whether it waits for these 20ms or not, because you can't assume that USB is usable immediately after, anyway).

Ah I see. You think it's worth mentioning in the doc? Or is this just too obvious as someone who actually know USB? I suppose it's good to know what the stack expects of me.

What's with the variation in async/sync in the Bus? Why is stalling/enabling not async? At the moment I'm having to performing these operations asynchronously as the SPI is async and it's not safe to block.

good point. we maybe should make them async in the next driver version. for now you can use blocking SPI, or use a background task, or queue it later and do it in poll().

Yes I'm currently using a background task for this. (I couldn't use poll() since there's no guarantees as to when it gets called #2751) In the case of Bus::endpoint_set_enabled I don't really need it to be async due to the use case. All I really need to do is cancel any pending endpoint operations and have them return EndpointError::Disabled. This doesn't need async, in my case.

Stalling on the other hand I'm not sure about since I'm kinda modifying the endpoint without a mutable reference to it, it also means I have to decide what to do with ongoing writes/reads (which is unclear to me at least). This sort of ties into #2837 where a class implementation needs to stall the endpoint. Ideally, I'd like the user/class/stack to cancel their write/read to stall.

To be honest, these can probably stay non-async if the possibly delayed side effect is documented I suppose.

[Dirbaio]

I see the sort of problems you're facing... I think you're the first person to try a driver for an SPI-USB chip. MCU peripherals where all the IO is volatile reg access, no actual async SPI I/O.

I wonder if in this case the best way is to do all SPI I/O on a background task, and have the read/write methods just dequeue/enqueue the packets in buffers in RAM. This way the cancelation becomes a non-issue: enqueuing/dequeuing can be atomic, and the actual SPI I/O can never get canceled.

re mentioning stuff in docs: more clarification is always welcome! send PRs if you want.

It does actually. On every iteration of this loop, one of these futures get cancelled.

oh oops you're right, I had forgotten about that. Perhaps the fix there is to merge Bus and ControlPipe, and treat "received SETUP" as another event that .poll() can return?

(and while we're at it, rename .poll() to .wait_for_event() or something, since it's causing so much confusion. it's really not meant for generic background work. And perhaps add something actually designed for background work...?).

Is there a "list of breaking changes for the next version" I can add this too? I don't want it to be forgotten.

none planned, other than this issue!

Stalling on the other hand I'm not sure about since I'm kinda modifying the endpoint without a mutable reference to it, it also means I have to decide what to do with ongoing writes/reads (which is unclear to me at least). This sort of ties into https://github.com/embassy-rs/embassy/issues/2837 where a class implementation needs to stall the endpoint. Ideally, I'd like the user/class/stack to cancel their write/read to stall.

I haven't looked at how stalling works in endpoints other than the control pipe. It sounds a bit cursed, but if it's required by classes we should support it ...

[Dominaezzz]

I wonder if in this case the best way is to do all SPI I/O on a background task, and have the read/write methods just dequeue/enqueue the packets in buffers in RAM. This way the cancelation becomes a non-issue: enqueuing/dequeuing can be atomic, and the actual SPI I/O can never get canceled.

This is something I am indeed contemplating, though it's a tedious alternative. It has the nice side effect of being able to put the IO on a high priority task to increase throughput.

re mentioning stuff in docs: more clarification is always welcome! send PRs if you want.

Alright, I'll make PRs for each point and the correctness can be discussed there.

It does actually. On every iteration of this loop, one of these futures get cancelled.

oh oops you're right, I had forgotten about that. Perhaps the fix there is to merge Bus and ControlPipe, and treat "received SETUP" as another event that .poll() can return?

I quite like the separation tbh. Maybe there's something in embassy-futures that can help keep the cancelled one alive for the next iteration. Or maybe poll() and setup() should be returning AsyncIterator (or some stable version of it) instead of a plain Future.

Yes to the rename (as someone who used to play with usb-device, poll() name is misleading).