usbdev: Introducing a generic USB device driver API

[bergzand]

Contribution description

This PR adds usbdev, a common API for interfacing USB device peripherals, similar to netdev. For now this adds only the headers. An implementation for the Atmel sam devices is ready and is going to be PR't after a bit of cleanup as an example implementation.

I've tried to keep the API easy to use and to be able to fully use the hardware capabilities of the peripherals such as built in DMA.

See also the doxygen documentation in drivers/include/usb/usbdev.h

Testing procedure

Only headers here, not much to test yet really.

Issues/PRs references

[aabadie]

Awesome !

I don't know who is also interested in USB

I am so I added myself in the reviewer list. I can work later on the STM32 implementation part (@astralien3000 did some preliminary work for F4)

[bergzand]

I am so I added myself in the reviewer list. I can work later on the STM32 implementation part (@astralien3000 did some preliminary work for F4)

That would be awesome. I noticed that at least the stm32f446(re) has two USB interfaces, a full speed one and a high speed one with a few more features. I think that at the moment an implementation that is shared over the most devices among the F4 devices would be very nice to have.

Also having a second implementation with a different device class would be very useful to verify that the API is implementable with different devices.

[bergzand]

@aabadie I've looked a bit at the reference manuals from different supported CPU's. From what I've found, the NRF52840, the EFM32 and the atmel sam devices all support some form of DMA'ing the packets from and to memory.
The ST devices however seem to have a dedicated RAM for packet memory which might be troublesome to use with the current API model.

[kaspar030]

Awesome!

(BTW, are you aware of #3890?)

[kaspar030]

this means "arbitrary upper layer context pointer", right? or why thread?

[bergzand]

Reworded this a bit

[bergzand]

(BTW, are you aware of #3890?)

Yes, I've seen it and looked at it before writing this API. I have to admit that I didn't use much of that PR. Maybe parts of it are reusable for an USB CDC serial implementation.

@aabadie @kaspar030 I'd like to have some advice/discussion here. At the moment, the API works by letting an endpoint handler register some memory space with the peripheral driver. The idea is that the peripheral driver (the hardware actually) uses DMA to read/write from this memory space when needed for a USB transmission. The only thing the endpoint has to do is write something in the memory space and signal ready to the low level peripheral. It should then leave this memory space untouched until the USB peripheral signals an USBDEV_EVENT_TR_COMPLETE event.

This model works good with the samr21-xpro USB peripheral. From the looks of it this should also work with the nrf52 and the EFM32 devices. At least the samr21-xpro has a DMA master build in the USB peripheral to make this work smoothly.

I'm expecting issues with the ST devices as these have a dedicated RAM location for the USB transfer buffers. While it is possible to keep the API as is and let the low level driver copy the memory space to the dedicated buffer, this is in my opinion rather inefficient. I'm not sure if it is possible to use a DMA channel to move the data around. If that is fast enough then that might be a good enough solution for now.

At the moment I like the idea of being able to directly write into a transfer buffer and only having to signal ready to the low level driver. A different solution could be to move the responsibility of allocating a buffer to the low level driver. The ST driver could then distribute slices of the dedicated RAM to the endpoints. The other drivers would need some statically allocated memory space. The drawback of this is that the "other drivers" might have more memory allocated than strictly necessary.

[aabadie]

I like the idea of being able to directly write into a transfer buffer and only having to signal ready to the low level driver. A different solution could be to move the responsibility of allocating a buffer to the low level driver. The ST driver could then distribute slices of the dedicated RAM to the endpoints.

The problem with the first solution is that it highly relies on what the low-level hardware provides. And as you noticed with ST it cannot be generalized to all hardware on the market. So maybe the second solution could be a good trade-off.

What about adding read and write function pointers to the endpoint struct (instead of playing the address option) ? Maybe it would make the API more handy.

I have another question regarding the expected driver API. I guess you plan to provide it via a periph implementation or something ?

[bergzand]

The problem with the first solution is that it highly relies on what the low-level hardware provides. And as you noticed with ST it cannot be generalized to all hardware on the market. So maybe the second solution could be a good trade-off.

Yes, I'm looking at the USB peripheral for the stm32f103 at the moment to check if moving the buffers to the peripheral solves the issue.

What about adding read and write function pointers to the endpoint struct (instead of playing the address option) ? Maybe it would make the API more handy.

How would this work? What kind of arguments would you use here?

I have another question regarding the expected driver API. I guess you plan to provide it via a periph implementation or something ?

At the moment I want to keep it similar to the way netdev/GNRC is set up. This way it is possible to have multiple different USB peripherals on a board. This is the case with the stm32f446re (One USB HS OTG and one USB FS OTG peripheral).

[aabadie]

At the moment I want to keep it similar to the way netdev/GNRC is set up.

There is a big difference with netdev: the USB feature is directly provided by an internal peripheral of the CPU, like gpio, i2c, spi, etc. Netdev drivers are built on top the periph_<whatever> interface.
That's why the usbdev interface should also be thought as an HAL on top of the CPU.

This is the case with the stm32f446re (One USB HS OTG and one USB FS OTG peripheral).

Can both be used at the same time (on the same USB port) ? Looking at the nucleo-f446ze, only the USB OTG FS is supported. But that's true that the CPU can handle both.

[bergzand]

There is a big difference with netdev: the USB feature is directly provided by an internal peripheral of the CPU, like gpio, i2c, spi, etc. Netdev drivers are built on top the periph_ interface.
That's why the usbdev interface should also be thought as an HAL on top of the CPU.

Most of the time yes, but not necessarily always the case. Anyway, Is this discussion about whether we want the header file in drivers/include/periph?

Can both be used at the same time (on the same USB port) ? Looking at the nucleo-f446ze, only the USB OTG FS is supported. But that's true that the CPU can handle both.

From how I read it, the stm32f446ze offers two USB peripherals which both can be used at the same time. How the board implements this is of course a different matter.

[dylad]

This looks promising !

I guess you plan to provide it via a periph implementation or something ?

I think an USB device implementation should belong to periph_xxx rather than netdev.

Can't wait to try it on sam0 devices !

[bergzand]

Can't wait to try it on sam0 devices !

I have a very dirty WIP with an USB HID media button hidden somewhere if you want to try it out. :)

[bergzand]

I think an USB device implementation should belong to periph_xxx rather than netdev.

It's similar to the netdev architecture, but not build on top of netdev. Just to have that clear

There is a big difference with netdev: the USB feature is directly provided by an internal peripheral of the CPU, like gpio, i2c, spi, etc. Netdev drivers are built on top the periph_<whatever> interface.
That's why the usbdev interface should also be thought as an HAL on top of the CPU.

Another difference between the current periph group and the USB peripheral is that the peripherals are directly usable with their API while for USB an extra layer of message handling is required to actually have some form of communication with a host. A simple usb_init() is never going to work here.

I think my main concern here is that I'm afraid that a periph approach works for 80% of the cases (the on MCU periph case) but that there are a number of easy to catch exceptions which are not possible with the periph approach.

@aabadie maybe we can synchronize a bit on ideas today or tomorrow and see if we can reach a consensus on the approach.

[dylad]

Regarding VID/PID, maybe we can ask for a free PID there
Unless RIOT manages to get enough money to buy a VID (around 5k USD)...
The current VID here belongs to Logitech ;)

[dylad]

In the current state, how can we associate an USBOPT to an endpoint ?
For example, if I want to generate a stall request to a specific endpoint. How can I get this endpoint in int usbdev_set(usbdev_t *usbdev, usbopt_t opt, const void *value, size_t value_len) function ?

[bergzand]

Regarding VID/PID, maybe we can ask for a free PID there

I think this is a great idea. Most of the time we just need a generic VID/PID combination because default drivers are able to handle everything and they don't listen to specific VID/PID combinations.

I think we would have to include a small bit of documentation refering the requirements of pid.codes specifying something like that the hardware must be open when it is used and (but more for our own protection) that the VID/PID combination must not be used as a basis for driver selection.

[bergzand]

For example, if I want to generate a stall request to a specific endpoint. How can I get this endpoint inint usbdev_set(usbdev_t *usbdev, usbopt_t opt, const void *value, size_t value_len) function ?

I've separated the API into an API for the USB device itself and an API for the endpoints. The advantage here is that a USB interface implementation (such as an HID implementation) only needs an endpoint and doesn't have to care about the USB interface handling itself.

To answer your question, similar to how netdev works at the moment:

static const usbopt_enable_t enable = USBOPT_ENABLE;             
ep->driver->set(ep, USBOPT_EP_ENABLE, &enable, sizeof(usbopt_enable_t));

Where ep is of type usbdev_ep_t*

[bergzand]

After working a bit with the stack, I've come to a few conclusions:

It doesn't seem to make sense to have a usbdev_ep_event_cb_t callback member for every usbdev_ep_t instance. A single callback in the usbdev_t struct should be enough as it can then be multiplexed out to different handlers based on the upper layer stack. The same can be applied to the context ptr, it can be removed from the usbdev_ep_t struct and it is probably sufficient to use the usbdev_t ptr directly.
The usbdev_ep_driver_t can probably also be moved to the usbdev_t struct. The case where a peripheral actually requires wildly different handling depending on the endpoint is the only reason to have a per endpoint struct ptr.

I'm thinking of merging the endpoint direction and number into a single uint8_t to match the usb protocol numbering, but I'd like to check if there is sufficient merit in merging these first.

[bergzand]

Reworked a large part of the interface according to my own suggestion from above. I didn't rework the endpoint number/direction into a single uint8_t.

With this, there is only a single driver struct remaining covering both the usb device and the endpoint specific api. The usbdev_ep_t struct got stripped of the context, driver and cb members, those have been added/merged into the usbdev_t struct. I've added a usbdev_t ptr to the usbdev_ep_t struct. This simplifies the implementation quite a bit and prevents bugs where an usbdev_ep_t and usbdev_t from two different peripherals are used in a single call.

Helper functions for each call are also available and recommended to use. This:

• prevents things such as usbdev->dev->driver->init(usbdev->dev); (actual example) and is used as usbdev_init(usbdev->dev); instead.
• allows for a single point to add all preconditions for a function instead of each implementation having to add them.

[bergzand]

rebased

[bergzand]

I've replaced the current default PID and VID combination with #error messages. This to prevent stalling this PR on a VID/PID discussion. While I agree to request a PID at least for development and testing purposes at some point, I'd rather not configure a temporary pid without at least a fat compile time warning for the end developer.

So with this it is up to the developer to explicitly configure a VID and PID number. Having a default PID could be provided in a follow up

[bergzand]

Summarizing an out-of-band discussion with @kaspar030:

usbdev should be moved to periph_usbdev. The main advantage is to be able to reuse the periph feature handling. The preference is to keep the usbdev_ names as not to clutter the usb_ C function namespace unnecessarily. This also emphasizes that the peripheral is only a low level interface and not a fully functional USB stack.

@kaspar030 Did I forget something here?

[kaspar030]

Did I forget something here?

no, perfect!

[bergzand]

Moved usbdev.h to drivers/include/periph and adjusted the doxygen group. The drivers_usbdev group is also removed.

[bergzand]

And updated the header guards accordingly

[bergzand]

@kaspar030 @dylad any remaining blockers here? Can I squash a second time?

[dylad]

You can squash. I'll review again on wednesday.

[dylad]

What about define it within each cpu/ ? This will force user to ensure the MCU can handle them. We might have surprise in the future if we keep a default value here.

[bergzand]

The idea here was to have a global value to set the number of IN and OUT endpoints. This way, an application that only needs a limited number of endpoints can set the required number in a platform-independent way.

I agree that this might cause issues. We could move this to something like cpu.h, but I can't say I really like that solution. I don't mind keeping it this way at the moment and solve the issue in a follow up as soon as we have a few drivers.

[dylad]

ACK.
I am happy with this ! Time to move forward.
Please squash.

[bergzand]

That didn't exactly go as plan, but it should be okay now.

@dylad I've removed the usbdev pseudomodule, it is not necessary anymore now that it is a periph thing.

[kaspar030]

ACK from my side, too. &go!

[bergzand]

Thanks guys!