[riot-notifications] [RIOT-OS/RIOT] [WIP] USBUS: Initial work towards an USB stack (#10916)

Kees Bakker notifications at github.com
Sun Feb 3 23:00:01 CET 2019


keestux commented on this pull request.



> + * @{
+ *
+ * @file
+ * @brief       USBUS basic interface
+ *
+ * @author      Koen Zandberg <koen at bergzand.net>
+ */
+
+#ifndef USB_USBUS_H
+#define USB_USBUS_H
+
+#include <stdint.h>
+#include <stdlib.h>
+#include "usb/usbdev.h"
+#include "usb.h"
+#include "usb/message.h"

This "usb/message.h" could just be "message.h" because it is included from a file in the same directory

> + *
+ * @author      Koen Zandberg <koen at bergzand.net>
+ */
+
+#ifndef USB_H
+#define USB_H
+
+#ifdef __cplusplus
+extern "c" {
+#endif
+
+/**
+ * @brief USB peripheral device vendor ID
+ */
+#ifndef USB_CONFIG_VID
+#define USB_CONFIG_VID          (0x046d)

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

> +            ep_reg->EPINTENSET.reg = USB_DEVICE_EPINTENSET_RXSTP;
+        }
+    }
+    else {
+        ep_reg->EPINTENSET.reg = USB_DEVICE_EPINTENSET_TRCPT1
+                                 | USB_DEVICE_EPINTENSET_TRFAIL1
+                                 | USB_DEVICE_EPINTENSET_STALL1;
+    }
+}
+
+static void _disable_ep_irq(usbdev_ep_t *ep)
+{
+    UsbDeviceEndpoint *ep_reg = &USB->DEVICE.DeviceEndpoint[ep->num];
+
+    if (ep->dir == USB_EP_DIR_OUT) {
+        ep_reg->EPINTENCLR.reg = USB_DEVICE_EPINTENCLR_TRCPT0 |

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.

> +    }
+    return false;
+}
+
+static usbdev_ep_t *usbdev_new_ep(usbdev_t *dev, usb_ep_type_t type,
+                                  usb_ep_dir_t dir, size_t buf_len)
+{
+    /* The IP supports all types for all endpoints */
+    (void)dev;
+    usbdev_ep_t *res = NULL;
+    if (type == USB_EP_TYPE_CONTROL) {
+        res = _get_ep(0, dir);
+        res->num = 0;
+    }
+    else {
+        for (unsigned idx = 1; idx < 8; idx++) {

Don't use magic constants. This 8, is that USBDEV_NUM_ENDPOINTS?

> +#if defined(CPU_FAM_SAMD21)
+    PM->AHBMASK.reg |= PM_AHBMASK_USB;
+    GCLK->CLKCTRL.reg = (uint32_t)(GCLK_CLKCTRL_CLKEN |
+                                   GCLK_CLKCTRL_GEN_GCLK0 |
+                                   (GCLK_CLKCTRL_ID(USB_GCLK_ID)));
+#elif defined(CPU_FAM_SAML21)
+    MCLK->AHBMASK.reg |= (MCLK_AHBMASK_USB);
+    GCLK->PCHCTRL[USB_GCLK_ID].reg = GCLK_PCHCTRL_CHEN |
+                                     GCLK_PCHCTRL_GEN_GCLK0;
+#endif
+}
+
+void usbdev_init(usbdev_t *dev)
+{
+    /* Only one usb device on this board */
+    _usbdev = (sam0_common_usb_t *)dev;

Maybe add an `assert(!_usbdev)` so that nobody can get away with calling  the function twice.

> + *
+ * Configures the number of endpoints allocated. An equal number of IN and OUT
+ * endpoints are allocated
+ */
+#ifndef USBDEV_NUM_ENDPOINTS
+#define USBDEV_NUM_ENDPOINTS       8
+#endif
+
+
+/**
+ * @brief   Possible event types that are send from the device driver to the
+ *          upper layer
+ */
+typedef enum {
+    /**
+     * @brief Driver needs it's ISR handled

ISR => ESR?
And also, not everybody may know what an ESR is. You could write: "... ESR (i.e. Event S.. R..) handled"

> +    }
+    else {
+        if (ep_reg->EPINTFLAG.bit.TRCPT1) {
+            ep_reg->EPINTFLAG.reg = USB_DEVICE_EPINTFLAG_TRCPT1;
+            event = USBDEV_EVENT_TR_COMPLETE;
+        }
+        else if (ep_reg->EPINTFLAG.bit.TRFAIL1) {
+            ep_reg->EPINTFLAG.reg = USB_DEVICE_EPINTFLAG_TRFAIL1;
+            event = USBDEV_EVENT_TR_FAIL;
+        }
+        else if (ep_reg->EPINTFLAG.bit.STALL1) {
+            ep_reg->EPINTFLAG.reg = USB_DEVICE_EPINTFLAG_STALL1;
+            event = USBDEV_EVENT_TR_STALL;
+        }
+        else {
+            DEBUG("Unhandled out %u: %x\n", ep->num, ep_reg->EPINTFLAG.reg);

%x => 0x%x

> +{
+    if (USB->DEVICE.EPINTSMRY.reg) {
+        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 = _get_ep(ep_num, USB_EP_DIR_IN);
+            _disable_ep_irq(ep);
+            ep->cb(ep, USBDEV_EVENT_ESR);
+        }
+        else if (_ep_out_flags_set(ep_reg)) {
+            usbdev_ep_t *ep = _get_ep(ep_num, USB_EP_DIR_OUT);
+            _disable_ep_irq(ep);
+            ep->cb(ep, USBDEV_EVENT_ESR);
+        }
+        else {
+            DEBUG("Unhandled ISR\n");

Suggestion: "Unhandled EP interrupt"

> +        if (USB->DEVICE.INTFLAG.bit.EORST) {
+            /* Clear flag */
+            USB->DEVICE.INTFLAG.reg = USB_DEVICE_INTFLAG_EORST;
+            usbdev_ep_init(&endpoints[0]);
+            _ep_enable(&endpoints[0]);
+            usbdev_ep_init(&endpoints[1]);
+            _ep_enable(&endpoints[1]);
+            _usbdev->usbdev.cb(&_usbdev->usbdev, USBDEV_EVENT_RESET);
+        }
+        /* Re-enable the USB IRQ */
+        _enable_irq();
+    }
+}
+
+
+static void _ep_address(usbdev_ep_t *ep)

This function is only used once. And it doesn't do a whole lot, just setting the address.
If you want to keep it then you should perhaps rename it to: `_ep_set_address`

> +
+    if (ep->dir == USB_EP_DIR_IN) {
+        res = ep_reg->EPSTATUSSET.bit.STALLRQ1
+            ? USBOPT_ENABLE
+            : USBOPT_DISABLE;
+    }
+    else {
+        res = ep_reg->EPSTATUSSET.bit.STALLRQ0
+            ? USBOPT_ENABLE
+            : USBOPT_DISABLE;
+    }
+    return res;
+
+}
+
+static void _ep_size(usbdev_ep_t *ep)

Perhaps rename this to: `_ep_set_size`

> +        if (_ep_in_flags_set(ep_reg)) {
+            usbdev_ep_t *ep = _get_ep(ep_num, USB_EP_DIR_IN);
+            _disable_ep_irq(ep);
+            ep->cb(ep, USBDEV_EVENT_ESR);
+        }
+        else if (_ep_out_flags_set(ep_reg)) {
+            usbdev_ep_t *ep = _get_ep(ep_num, USB_EP_DIR_OUT);
+            _disable_ep_irq(ep);
+            ep->cb(ep, USBDEV_EVENT_ESR);
+        }
+        else {
+            DEBUG("Unhandled ISR\n");
+        }
+    }
+    else {
+        _disable_irq();

Perhaps add a comment
```
    /* Device interrupt */
```

> +            break;
+    }
+    return res;
+}
+
+int usbdev_ep_set(usbdev_ep_t *ep, usbopt_ep_t opt,
+                  const void *value, size_t value_len)
+{
+    (void)ep;
+    (void)value;
+    (void)value_len;
+    int res = -ENOTSUP;
+    assert(ep != NULL);
+    switch (opt) {
+        case USBOPT_EP_ENABLE:
+            assert(value_len == sizeof(usbopt_enable_t));

Do you really want to return `-ENOTSUP` for this enable?

> +        case USBOPT_EP_ENABLE:
+            assert(value_len == sizeof(usbopt_enable_t));
+            if (*((usbopt_enable_t *)value)) {
+                usbdev_ep_init(ep);
+                _ep_enable(ep);
+            }
+            else {
+                _ep_disable(ep);
+            }
+            break;
+        case USBOPT_EP_BUF_ADDR:
+            assert(value_len == sizeof(uint8_t *));
+            res = sizeof(char *);
+            break;
+        case USBOPT_EP_BUF_SIZE:
+            assert(value_len == sizeof(size_t));

Is this a TODO? If not, you should add a comment.

> +    int res = -ENOTSUP;
+    assert(ep != NULL);
+    switch (opt) {
+        case USBOPT_EP_ENABLE:
+            assert(value_len == sizeof(usbopt_enable_t));
+            if (*((usbopt_enable_t *)value)) {
+                usbdev_ep_init(ep);
+                _ep_enable(ep);
+            }
+            else {
+                _ep_disable(ep);
+            }
+            break;
+        case USBOPT_EP_BUF_ADDR:
+            assert(value_len == sizeof(uint8_t *));
+            res = sizeof(char *);

Why `char *` and not `uint8_t *`?

> +    size_t len = 0;
+    for (; hdr; hdr = hdr->next) {
+        len += hdr->gen_hdr(usbus, hdr->arg);
+    }
+    return len;
+}
+
+size_t usbus_hdrs_fmt_iface_alts(usbus_t *usbus, usbus_interface_t *iface)
+{
+    size_t len = 0;
+    uint8_t alts = 1;
+    for (usbus_interface_alt_t *alt = iface->alts;
+            alt;
+            alt = alt->next) {
+        usb_descriptor_interface_t usb_iface;
+        memset(&usb_iface, 0 , sizeof(usb_descriptor_interface_t));

Space after `0`
It occurs in more places where there is a `memset`

> +        }
+    }
+}
+
+static void _usbus_config_ep0(usbus_t *usbus)
+{
+    static const usbopt_enable_t enable = USBOPT_ENABLE;
+    usbus->in->driver->set(usbus->in, USBOPT_EP_ENABLE, &enable, sizeof(usbopt_enable_t));
+    usbus->out->driver->set(usbus->out, USBOPT_EP_ENABLE, &enable, sizeof(usbopt_enable_t));
+    usbus->out->driver->ready(usbus->out, 0);
+}
+
+void _req_status(usbus_t *usbus)
+{
+    uint8_t status[2];
+    memset(status, 0, 2);

Instead of `2` it is safer to use `sizeof(status)`

> +}
+
+static void _usbus_config_ep0(usbus_t *usbus)
+{
+    static const usbopt_enable_t enable = USBOPT_ENABLE;
+    usbus->in->driver->set(usbus->in, USBOPT_EP_ENABLE, &enable, sizeof(usbopt_enable_t));
+    usbus->out->driver->set(usbus->out, USBOPT_EP_ENABLE, &enable, sizeof(usbopt_enable_t));
+    usbus->out->driver->ready(usbus->out, 0);
+}
+
+void _req_status(usbus_t *usbus)
+{
+    uint8_t status[2];
+    memset(status, 0, 2);
+    usbus_put_bytes(usbus, status, sizeof(status));
+    usbus->in->driver->ready(usbus->in, 2);

Consider `2` => `sizeof(status)` if that makes sense

> +                    printf("activated endpoint %d, dir %s\n", ep->ep->num, ep->ep->dir == USB_EP_DIR_OUT? "out" : "in");
+                }
+            }
+        }
+    }
+}
+
+static void _usbus_config_ep0(usbus_t *usbus)
+{
+    static const usbopt_enable_t enable = USBOPT_ENABLE;
+    usbus->in->driver->set(usbus->in, USBOPT_EP_ENABLE, &enable, sizeof(usbopt_enable_t));
+    usbus->out->driver->set(usbus->out, USBOPT_EP_ENABLE, &enable, sizeof(usbopt_enable_t));
+    usbus->out->driver->ready(usbus->out, 0);
+}
+
+void _req_status(usbus_t *usbus)

Are these `_req_xxx` functions meant to be `static`?

> +void _req_str(usbus_t *usbus, uint16_t idx)
+{
+    if (idx == 0) {
+        usb_descriptor_string_t desc;
+        desc.type = USB_TYPE_DESCRIPTOR_STRING;
+        desc.length = sizeof(uint16_t)+sizeof(usb_descriptor_string_t);
+        usbus_put_bytes(usbus, (uint8_t*)&desc, sizeof(desc));
+        /* Only one language ID supported */
+        uint16_t us = USB_CONFIG_DEFAULT_LANGID;
+        usbus_put_bytes(usbus, (uint8_t*)&us, sizeof(uint16_t));
+        usbus_ep0_ready(usbus);
+    }
+    else {
+        usb_descriptor_string_t desc;
+        desc.type = USB_TYPE_DESCRIPTOR_STRING;
+        mutex_lock(&usbus->lock);

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.

> +        usbus_ep0_ready(usbus);
+    }
+    else {
+        usb_descriptor_string_t desc;
+        desc.type = USB_TYPE_DESCRIPTOR_STRING;
+        mutex_lock(&usbus->lock);
+        usbus_string_t *str = _get_descriptor(usbus, idx);
+        if (str) {
+            desc.length = sizeof(usb_descriptor_string_t);
+            desc.length += 2*strlen(str->str);
+            usbus_put_bytes(usbus, (uint8_t*)&desc, sizeof(desc));
+            usbus_cpy_str(usbus, str->str);
+            usbus_ep0_ready(usbus);
+        }
+        else {
+            usbus_ep0_ready(usbus);

Nothing has been "put". Is this OK then? To do `usbus_ep0_ready`?

> +    mutex_lock(&usbus->lock);
+    for (usbus_interface_t *iface = usbus->iface; iface; iface = iface->next) {
+        if (destination == iface->idx) {
+            iface->handler->driver->event_handler(usbus, iface->handler, USBUS_MSG_TYPE_SETUP_RQ, pkt);
+        }
+    }
+    mutex_unlock(&usbus->lock);
+}
+
+
+static inline size_t usbus_pkt_maxlen(usbus_t *usbus, usb_setup_t *pkt)
+{
+    return pkt->length > usbus->in->len ? usbus->in->len : pkt->length;
+}
+
+void recv_setup(usbus_t *usbus, usbdev_ep_t *ep)

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.

> +    if (event == USBDEV_EVENT_ESR) {
+        msg_t msg = { .type = USBUS_MSG_TYPE_EVENT,
+                      .content = { .ptr = usbdev } };
+
+        if (msg_send(&msg, usbus->pid) <= 0) {
+            puts("usbus: possibly lost interrupt.");
+        }
+    }
+    else {
+        switch (event) {
+            case USBDEV_EVENT_RESET:
+                {
+                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));

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.

> +                }
+                else if (usbus->setup_state == USBUS_SETUPRQ_OUTDATA && ep->dir == USB_EP_DIR_OUT) {
+                    /* Ready in ZLP */
+                    usbus->setup_state = USBUS_SETUPRQ_INACK;
+                    usbus->in->driver->ready(usbus->in, 0);
+                }
+                else if (ep->dir == USB_EP_DIR_OUT) {
+                    memset(&usbus->builder, 0, sizeof(usbus_controlbuilder_t));
+                    memcpy(&usbus->setup, usbus->out->buf, sizeof(usb_setup_t));
+                    usbus->builder.reqlen = usbus->setup.length;
+                    usbus->out->driver->ready(usbus->out, 0);
+                    recv_setup(usbus, ep);
+                }
+                break;
+            case USBDEV_EVENT_TR_FAIL:
+                if (ep->dir == USB_EP_DIR_OUT) {

Maybe add TODO marker? That is, if something needs to be done here. Otherwise drop the `if`

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/10916#pullrequestreview-199396662
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190203/27a6f4cc/attachment-0001.html>


More information about the notifications mailing list