USBUS: Initial work towards an USB stack#10916
Conversation
|
@bergzand I'll have a look this week-end or next week. |
Must have forgotten to commit/push this, thanks for notifying me |
sys/usb/usbus/usbus.c
Outdated
| usbus->addr = 0; | ||
| usbus->setup_state = USBUS_SETUPRQ_READY; | ||
| usbus->dev->driver->set(usbus->dev, USBOPT_ADDRESS, &usbus->addr, sizeof(uint8_t)); | ||
| puts("Reset"); |
There was a problem hiding this comment.
Huh, maybe something like usbus: event reset
Also why do you use puts() and not the DEBUG() function ?
There was a problem hiding this comment.
Because of lazy probably, sorry :(
cpu/sam0_common/usb/usb.c
Outdated
| usbdev_ep_ready(ep, 0); | ||
| } | ||
| res = sizeof(usbopt_enable_t); | ||
| res = sizeof(usbopt_enable_t); |
cpu/sam0_common/include/sam_usb.h
Outdated
| */ | ||
|
|
||
| /** | ||
| * @defgroup sys_uuid RFC 4122 compliant UUID's |
cpu/sam0_common/usb/usb.c
Outdated
| unsigned ep_num = bitarithm_lsb(USB->DEVICE.EPINTSMRY.reg); | ||
| UsbDeviceEndpoint *ep_reg = &USB->DEVICE.DeviceEndpoint[ep_num]; | ||
| if (_ep_in_flags_set(ep_reg)) { | ||
| usbdev_ep_t *ep = &endpoints[_get_ep_num(ep_num, USB_EP_DIR_IN)]; |
sys/usb/usbus/usbus.c
Outdated
| .content = { .ptr = usbdev } }; | ||
|
|
||
| if (msg_send(&msg, usbus->pid) <= 0) { | ||
| puts("usbus: possibly lost interrupt."); |
There was a problem hiding this comment.
I'd seriously consider using thread flags for this. You'd disable this failure case entirely, thread flags cannot get lost.
An arriving message also sets a thread flag, so a thread can wait for both:
thread_flag_t flags = thread_flag_wait_any(0xffff);
if (flags & THREAD_FLAG_MSG_WAITING) {
msg_t msg
while (msg_try_receive(&msg) == 1) {
// handle netapi messages
}
}
if (flags & YOUR_FLAG_SET_BY_ISR) {
/// handle
}
}
There was a problem hiding this comment.
I understand if you'd rather not go into this for now, can be changed later.
There was a problem hiding this comment.
This is actually one of the issues where I'd really would like to have some feedback on. I've used messages here because it's what I'm most familiar with from gnrc, but using thread flags or events are both open to suggestions for me.
There was a problem hiding this comment.
After some out-of-band discussion it became clear that it should be easy to move to thread flags with a flag for the generic usbdev ISR and a flag for endpoint specific ISR's. A uint32_t can keep track of the endpoints that require servicing (USB has 32 endpoints max, 16 IN and 16 OUT). This has as a consequence that usbus needs fast access to the usbdev endpoints, preferably O(1) complexity to resolve the thread flag back to an endpoint.
One solution here is to allocate the usbus_endpoint_t (not to be confused with usbdev_ep_t) as a static array. This could then reuse the USBDEV_NUM_ENDPOINTS, matching the number of endpoints allocated by the peripheral.
There was a problem hiding this comment.
@kaspar030 I rewrote the loop to use thread flags. 2 flags are used as described above. Additionally I've added handling for events to allow signalling events to usb functionality from outside or inside the usbus thread.
keestux
left a comment
There was a problem hiding this comment.
Here are a few more comments. I hope to spend more time in the next days.
cpu/sam0_common/usb/usb.c
Outdated
|
|
||
| static bool usb_enable_syncing(void) | ||
| { | ||
| if (USB->DEVICE.SYNCBUSY.reg & USB_SYNCBUSY_ENABLE) { |
There was a problem hiding this comment.
Personally I like the .bit notation easier to read.
if (USB->DEVICE.SYNCBUSY.bit.ENABLE) {
cpu/sam0_common/usb/usb.c
Outdated
|
|
||
| static bool usb_swrst_syncing(void) | ||
| { | ||
| if (USB->DEVICE.SYNCBUSY.reg & USB_SYNCBUSY_SWRST) { |
There was a problem hiding this comment.
if (USB->DEVICE.SYNCBUSY.bit.SWRST) {
cpu/sam0_common/usb/usb.c
Outdated
| GCLK_CLKCTRL_GEN_GCLK0 | | ||
| (GCLK_CLKCTRL_ID(USB_GCLK_ID))); | ||
| #elif defined(CPU_FAM_SAML21) | ||
| MCLK->AHBMASK.reg |= (MCLK_AHBMASK_USB); |
There was a problem hiding this comment.
Legacy probably, removed
cpu/sam0_common/usb/usb.c
Outdated
| static inline void poweroff(void) | ||
| { | ||
| #if defined(CPU_FAM_SAMD21) | ||
| PM->AHBMASK.reg |= PM_AHBMASK_USB; |
There was a problem hiding this comment.
This should be clearing the bit. And also the APBBMASK is missing here. (Besides, the poweroff function is not used)
There was a problem hiding this comment.
Ugh, let me check this one later, completely messed the function up
cpu/sam0_common/usb/usb.c
Outdated
|
|
||
| void usb_attach(void) | ||
| { | ||
| /* Datasheet is not clear whether device starts detached */ |
There was a problem hiding this comment.
The comment does not belong here. This function is simply doing what it's been told to do.
BTW. I don't have a direct answer, but ASF usb init and Arduino bootloader do an attach. So I guess it is detached when the device starts.
There was a problem hiding this comment.
This served as a reminder for myself to check this. As you said, this doesn't belong here, I'm removing it.
f740cdb to
e4ae27b
Compare
|
rebased to the current #10915 |
|
@bergzand examples/usbus_minimal doesn't build. |
|
@bergzand nvm, I miss the I was able to successfully test it on arduino-zero. |
|
Not much luck with my SODAQ board, which are almost the same as Arduino Zero. |
|
On the other hand, maybe I'm expecting too much at this point. What I do get is: |
|
@keestux Did you try with debugging disabled? My experience with running with debug logging is that the delays caused by the serial transfer are enough to cause timing issues with USB. It is still on my todo list to figure out how much debugging can be enabled without causing issues |
|
@bergzand I think you're right. Without debug On the RIOT device console it shows just two "Reset" messages: |
sys/include/usb/usbus.h
Outdated
| #include <stdlib.h> | ||
| #include "usb/usbdev.h" | ||
| #include "usb.h" | ||
| #include "usb/message.h" |
There was a problem hiding this comment.
This "usb/message.h" could just be "message.h" because it is included from a file in the same directory
There was a problem hiding this comment.
I prefer the long form to prevent confusion for a future reader
sys/include/usb.h
Outdated
| * @brief USB peripheral device vendor ID | ||
| */ | ||
| #ifndef USB_CONFIG_VID | ||
| #define USB_CONFIG_VID (0x046d) |
There was a problem hiding this comment.
I know it is just temporary using a Logitech identifier. But why not use Arduino's vendor and product id until we have our own (if that's even feasible)? I think Arduino Zero has 0x2341:0x804d
cpu/sam0_common/usb/usb.c
Outdated
| UsbDeviceEndpoint *ep_reg = &USB->DEVICE.DeviceEndpoint[ep->num]; | ||
|
|
||
| if (ep->dir == USB_EP_DIR_OUT) { | ||
| ep_reg->EPINTENCLR.reg = USB_DEVICE_EPINTENCLR_TRCPT0 | |
There was a problem hiding this comment.
Sorry to nitpick, but I like symmetry. The code for enable/disable IRQ is very, very similar. Don't just reorder lines, Use the same formatting (the bitwise or at the end of line or at the next line).
I like to be able to quickly visually compare the two functions.
cpu/sam0_common/usb/usb.c
Outdated
| res->num = 0; | ||
| } | ||
| else { | ||
| for (unsigned idx = 1; idx < 8; idx++) { |
There was a problem hiding this comment.
Don't use magic constants. This 8, is that USBDEV_NUM_ENDPOINTS?
cpu/sam0_common/usb/usb.c
Outdated
| void usbdev_init(usbdev_t *dev) | ||
| { | ||
| /* Only one usb device on this board */ | ||
| _usbdev = (sam0_common_usb_t *)dev; |
There was a problem hiding this comment.
Maybe add an assert(!_usbdev) so that nobody can get away with calling the function twice.
There was a problem hiding this comment.
Reworked this to not have a hard dependency on the sam0_common_usb_t struct
sys/usb/usbus/usbus.c
Outdated
| else { | ||
| usb_descriptor_string_t desc; | ||
| desc.type = USB_TYPE_DESCRIPTOR_STRING; | ||
| mutex_lock(&usbus->lock); |
There was a problem hiding this comment.
It would help the reader to know what the lock is protection...
Otherwise one might ask why there is no lock in the section above.
There was a problem hiding this comment.
These locks originate from the idea to have multiple threads interacting with the stack. I don't think that this was a very good idea, so for now it is one single thread and it should be possible to remove most if not all of these locks.
sys/usb/usbus/usbus.c
Outdated
| usbus_ep0_ready(usbus); | ||
| } | ||
| else { | ||
| usbus_ep0_ready(usbus); |
There was a problem hiding this comment.
Nothing has been "put". Is this OK then? To do usbus_ep0_ready?
There was a problem hiding this comment.
It is. Some times you just want to notify things without send any data.
sys/usb/usbus/usbus.c
Outdated
| return pkt->length > usbus->in->len ? usbus->in->len : pkt->length; | ||
| } | ||
|
|
||
| void recv_setup(usbus_t *usbus, usbdev_ep_t *ep) |
There was a problem hiding this comment.
You probably know better than I, do we have a function naming convention? I mean, since C has a single name space, there could easily arise a name conflict with functions named like this.
If this becomes a static function then there is no problem, of course.
There was a problem hiding this comment.
Should probably also be static
There was a problem hiding this comment.
do we have a function naming convention?
Yes, public (non-static) functions should be called module_function() (e.g., usbus_recv_setup()). static functions should start with underline (static void _recv_setup(void)).
There was a problem hiding this comment.
marked almost all these functions as static, please let me know if I missed one
sys/usb/usbus/usbus.c
Outdated
| usbus->state = USBUS_STATE_RESET; | ||
| usbus->addr = 0; | ||
| usbus->setup_state = USBUS_SETUPRQ_READY; | ||
| usbus->dev->driver->set(usbus->dev, USBOPT_ADDRESS, &usbus->addr, sizeof(uint8_t)); |
There was a problem hiding this comment.
Is the sizeof correct? I'm asking because usbus->addr is a uint16_t.
And if your answer is no, then I would strongly suggest to change it to sizeof(usbus->addr)
And if your answer is yes, you should realize you have an endianess problem.
There was a problem hiding this comment.
Not that it is breaking anything at the moment, but usbus->addr should be uint8_t 😕
There was a problem hiding this comment.
Even if the addr should be uint8_t, it is safer to program this as sizeof(usbus->addr).
sys/usb/usbus/usbus.c
Outdated
| } | ||
| break; | ||
| case USBDEV_EVENT_TR_FAIL: | ||
| if (ep->dir == USB_EP_DIR_OUT) { |
There was a problem hiding this comment.
Maybe add TODO marker? That is, if something needs to be done here. Otherwise drop the if
9e1a286 to
0ba41cc
Compare
|
No longer WIP btw |
sys/usb/usbus/usbus.c
Outdated
| void usbus_init(usbus_t *usbus, usbdev_t *usbdev) | ||
| { | ||
| /* TODO: Check if memset works/is necessary */ | ||
| memset(usbus, 0, sizeof(usbus)); |
There was a problem hiding this comment.
Shouldn't this be sizeof(*usbus) ?
There was a problem hiding this comment.
Yes
Noticed it after I rebased and pushed everything 😢
Fixed
|
Is this the right place for this? |
Sure, no worries. If possible next time please add I must have forgotten to actually commit and push the change that modified the |
|
Yes, much better now |
4f195a5 to
9b0334d
Compare
|
And rebased |
|
@kaspar030 Do you have any inspiration on how to fix the build failure? To be honest I really don't want to hardcode the pid.codes test PID in the example. |
|
Now depends on #11633 :'( |
|
Please squash and rebase |
061fa18 to
3d30886
Compare
Done! |
|
For the record: |
Aaah, thanks for testing, I switched around the VID/PID in the Makefile. Should be fixed now |
dylad
left a comment
There was a problem hiding this comment.
RE-ACK
Retested lastest changes. We're all good.
|
Here we go ! |
|
🎉 @dylad, @keestux and @kaspar030 Thanks for reviewing! |
|
@bergzand yay! |
|
@bergzand niiiice! |
|
@bergzand thank you so much. I have been checking this thread every morning the way some people check the lottery numbers. |
Contribution description
USBUS (Universal Serial Bus Unified Stack) is a generic USB stack specific for RIOT-os. It is a newly written USB stack making use of the usbdev module proposed in #9830. The idea is to have a single message/event based thread handling all the usb work.
Modularity is achieved by allowing different endpoint modules to register themselves with their specific settings. On startup, each module requiring USB endpoints have to request endpoints from USBUS and add a callback for handling the messages. Examples of modules that could be created are: USB CDC ACM (serial over USB), USB HID (sensors over USB) and USB CDC EEM (ethernet over USB).
Testing procedure
Run the
examples/usbus_minimalon asamr21-xproboard. Plugging in the second (TARGET USB) in an USB host device (your desktop) should cause it to show up in the usb device listing with something like:(I'm fully aware that I'm stealing one of the Logitech vendor codes, It's on my todo for #9830 to change it to something else).
Issues/PRs references
depends on #10915 and #9830
Todo: