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

Dylan Laduranty notifications at github.com
Wed Mar 27 21:34:17 CET 2019


dylad commented on this pull request.

Here another round.
Just small nitpicks/questions. This PR seems almost ready.

> @@ -139,6 +139,9 @@ endif
 ifneq (,$(filter bluetil_%,$(USEMODULE)))
   DIRS += net/ble/bluetil
 endif
+ifneq (,$(filter usbus usbus_%,$(USEMODULE)))

`ifneq (,$(filter usbus,$(USEMODULE)))` isn't enough ?

> +#endif
+
+/**
+ * @name USB standard device request codes (USB 2.0 spec table 9-4)
+ * @{
+ */
+#define USB_SETUP_REQ_GET_STATUS            0x00    /**< Status request        */
+#define USB_SETUP_REQ_CLEAR_FEATURE         0x01    /**< Clear feature         */
+#define USB_SETUP_REQ_SET_FEATURE           0x03    /**< Set feature           */
+#define USB_SETUP_REQ_SET_ADDRESS           0x05    /**< Set address           */
+#define USB_SETUP_REQ_GET_DESCRIPTOR        0x06    /**< Get descriptor        */
+#define USB_SETUP_REQ_SET_DESCRIPTOR        0x07    /**< Set descriptor        */
+#define USB_SETUP_REQ_GET_CONFIGURATION     0x08    /**< Get configuration     */
+#define USB_SETUP_REQ_SET_CONFIGURATION     0x09    /**< Set configuration     */
+#define USB_SETUP_REQ_GET_INTERFACE         0x0a    /**< Get interface         */
+#define USB_SETUP_REQ_SET_INTERFACE         0x0b    /**< Set interface         */

missing SYNCH_FRAME.

> +#endif
+
+/**
+ * @name USB standard device request codes (USB 2.0 spec table 9-4)
+ * @{
+ */
+#define USB_SETUP_REQ_GET_STATUS            0x00    /**< Status request        */
+#define USB_SETUP_REQ_CLEAR_FEATURE         0x01    /**< Clear feature         */
+#define USB_SETUP_REQ_SET_FEATURE           0x03    /**< Set feature           */
+#define USB_SETUP_REQ_SET_ADDRESS           0x05    /**< Set address           */
+#define USB_SETUP_REQ_GET_DESCRIPTOR        0x06    /**< Get descriptor        */
+#define USB_SETUP_REQ_SET_DESCRIPTOR        0x07    /**< Set descriptor        */
+#define USB_SETUP_REQ_GET_CONFIGURATION     0x08    /**< Get configuration     */
+#define USB_SETUP_REQ_SET_CONFIGURATION     0x09    /**< Set configuration     */
+#define USB_SETUP_REQ_GET_INTERFACE         0x0a    /**< Get interface         */
+#define USB_SETUP_REQ_SET_INTERFACE         0x0b    /**< Set interface         */

Also 0xb or 0xB, you must choose and unify this :)

> +#define USB_SETUP_REQ_SET_INTERFACE         0x0b    /**< Set interface         */
+/** @} */
+
+/**
+ * @name USB descriptor types (USB 2.0 spec table 9-5)
+ * @{
+ */
+#define USB_TYPE_DESCRIPTOR_DEVICE          0x01    /**< Device descriptor         */
+#define USB_TYPE_DESCRIPTOR_CONFIGURATION   0x02    /**< Configuration Descriptor  */
+#define USB_TYPE_DESCRIPTOR_STRING          0x03    /**< String descriptor         */
+#define USB_TYPE_DESCRIPTOR_INTERFACE       0x04    /**< Interface descriptor      */
+#define USB_TYPE_DESCRIPTOR_ENDPOINT        0x05    /**< Endpoint descriptor       */
+#define USB_TYPE_DESCRIPTOR_DEV_QUALIFIER   0x06    /**< Device qualifier          */
+#define USB_TYPE_DESCRIPTOR_SPEED_CONFIG    0x07    /**< Other speed configuration */
+#define USB_TYPE_DESCRIPTOR_IFACE_POWER     0x08    /**< Interface power           */
+#define USB_TYPE_DESCRIPTOR_INTERFACE_ASSOC 0x0B    /**< Interface association     */

Didn't find this line within USB2.0 spec.

> +} usbus_control_slicer_t;
+
+/**
+ * @brief Endpoint zero event handler
+ */
+typedef struct {
+    usbus_handler_t handler;            /**< Inherited generic handler        */
+    usb_setup_t setup;                  /**< Last received setup packet       */
+    usbus_setuprq_state_t setup_state;  /**< Setup request state machine      */
+    usbus_control_slicer_t slicer;      /**< Slicer for multipart control
+                                            messages */
+    usbdev_ep_t *out;                   /**< EP0 out endpoint                 */
+    usbdev_ep_t *in;                    /**< EP0 in endpoint                  */
+} usbus_control_handler_t;
+
+int usbus_control_init(usbus_t *usbus, usbus_control_handler_t *handler);

missing doc ?

> + *
+ * @param[in] usbus     USBUS context
+ * @param[in] c         byte to add
+ *
+ * @return              Actual number of bytes written
+ */
+size_t usbus_control_slicer_put_char(usbus_t *usbus, char c);
+
+/**
+ * @brief Helper function to signal the end of the control message
+ *
+ * @param[in] usbus     USBUS context
+ */
+void usbus_control_slicer_ready(usbus_t *usbus);
+
+int usbus_control_slicer_nextslice(usbus_t *usbus);

missing doc

> + * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @ingroup usb_usbus
+ * @{
+ * @file
+ * @brief   USBUS USB manager thread, handles USB interaction
+ *
+ * @author  Koen Zandberg <koen at bergzand.net>
+ * @}
+ */
+
+#include "thread.h"
+#include "byteorder.h"

needed ?

> +
+    usbus->state = USBUS_STATE_DISCONNECT;
+
+    /* Initialize handlers */
+    _usbus_init_handlers(usbus);
+
+#if (USBUS_AUTO_ATTACH)
+    static const usbopt_enable_t _enable = USBOPT_ENABLE;
+    usbdev_set(dev, USBOPT_ATTACH, &_enable,
+               sizeof(usbopt_enable_t));
+#endif
+
+    while (1) {
+        msg_receive(&msg);
+        /* dispatch USBUS messages */
+        switch (msg.type & 0xFF00) {

Is the mask really needed ? If yes, please add a note for explaining black magic.

> +        for (usbus_interface_alt_t *alt = iface->alts; alt; alt = alt->next) {
+            for (usbus_endpoint_t *ep = alt->ep; ep; ep = ep->next) {
+                if (ep->active) {
+                    static const usbopt_enable_t enable = USBOPT_ENABLE;
+                    usbdev_ep_set(ep->ep, USBOPT_EP_ENABLE, &enable,
+                                  sizeof(usbopt_enable_t));
+                    DEBUG("usbus_control: activated endpoint %d, dir %s\n",
+                          ep->ep->num,
+                          ep->ep->dir == USB_EP_DIR_OUT ? "out" : "in");
+                }
+            }
+        }
+    }
+}
+
+static size_t usbus_cpy_str_to_utf16(usbus_t *usbus, const char *str)

```suggestion
static size_t _usbus_cpy_str_to_utf16(usbus_t *usbus, const char *str)
```

> +{
+    usbus_fmt_hdr_conf(usbus);
+    return 1;
+}
+
+static int _req_dev_qualifier(usbus_t *usbus)
+{
+    usb_speed_t speed = USB_SPEED_LOW;
+
+    usbus->dev->driver->get(usbus->dev, USBOPT_MAX_SPEED, &speed,
+                            sizeof(usb_speed_t));
+    if (speed == USB_SPEED_HIGH) {
+        /* TODO: implement device qualifier support (only required
+         * for High speed) */
+    }
+    return -1;

Why -1 ??

> +static int _req_descriptor(usbus_t *usbus, usb_setup_t *pkt)
+{
+    uint8_t type = pkt->value >> 8;
+    uint8_t idx = (uint8_t)pkt->value;
+
+    /* Decode descriptor type */
+    switch (type) {
+        case USB_TYPE_DESCRIPTOR_DEVICE:
+            return _req_dev(usbus);
+        case USB_TYPE_DESCRIPTOR_CONFIGURATION:
+            return _req_config(usbus);
+        case USB_TYPE_DESCRIPTOR_STRING:
+            return _req_str(usbus, idx);
+        case USB_TYPE_DESCRIPTOR_DEV_QUALIFIER:
+            return _req_dev_qualifier(usbus);
+        default:

please add a DEBUG statement here and in other default statements below.

> +    DEBUG("usbus_control: Initializing EP0\n");
+    usbus_control_handler_t *ep0_handler = (usbus_control_handler_t *)handler;
+    usbus_handler_set_flag(handler, USBUS_HANDLER_FLAG_RESET);
+    ep0_handler->setup_state = USBUS_SETUPRQ_READY;
+
+    ep0_handler->in = usbdev_new_ep(usbus->dev, USB_EP_TYPE_CONTROL,
+                                    USB_EP_DIR_IN, USBUS_EP0_SIZE);
+    ep0_handler->out = usbdev_new_ep(usbus->dev, USB_EP_TYPE_CONTROL,
+                                     USB_EP_DIR_OUT, USBUS_EP0_SIZE);
+}
+
+static int _handle_tr_complete(usbus_t *usbus,
+                               usbus_control_handler_t *ep0_handler,
+                               usbdev_ep_t *ep)
+{
+    switch (ep0_handler->setup_state) {

missing default statement.

> +}
+
+static size_t _num_endpoints(usbus_interface_t *iface)
+{
+    size_t num = 0;
+
+    for (usbus_endpoint_t *ep = iface->ep;
+         ep; ep = ep->next) {
+        num++;
+    }
+    return num;
+}
+
+static uint8_t _type_to_attribute(usbus_endpoint_t *ep)
+{
+    switch (ep->ep->type) {

missing default statement

> +    }
+    return len;
+}
+
+static size_t _ep_size(usbus_t *usbus, usbus_endpoint_t *ep)
+{
+    size_t len = 0;
+
+    for (; ep; ep = ep->next) {
+        len += sizeof(usb_descriptor_endpoint_t);
+        len += _hdr_gen_size(usbus, ep->hdr_gen);
+    }
+    return len;
+}
+
+size_t _alt_size(usbus_t *usbus, usbus_interface_alt_t *alt)

should be ?
```suggestion
static size_t _alt_size(usbus_t *usbus, usbus_interface_alt_t *alt)
```


> +static size_t _hdrs_fmt_additional(usbus_t *usbus, usbus_hdr_gen_t *hdr)
+{
+    size_t len = 0;
+
+    for (; hdr; hdr = hdr->next) {
+        len += call_get_header(usbus, hdr);
+    }
+    return len;
+}
+
+static size_t _hdrs_fmt_hdrs(usbus_t *usbus)
+{
+    return _hdrs_fmt_additional(usbus, usbus->hdr_gen);
+}
+
+size_t _hdrs_fmt_endpoints(usbus_t *usbus, usbus_endpoint_t *ep)

```suggestion
static size_t _hdrs_fmt_endpoints(usbus_t *usbus, usbus_endpoint_t *ep)
```

> +size_t usbus_fmt_hdr_conf(usbus_t *usbus)
+{
+    size_t len = 0;
+    usb_descriptor_configuration_t conf;
+
+    memset(&conf, 0, sizeof(usb_descriptor_configuration_t));
+    conf.length = sizeof(usb_descriptor_configuration_t);
+    conf.type = USB_TYPE_DESCRIPTOR_CONFIGURATION;
+    conf.total_length = sizeof(usb_descriptor_configuration_t);
+    conf.val = 1;
+    conf.attributes = USB_CONF_ATTR_RESERVED;
+    if (USB_CONFIG_SELF_POWERED) {
+        conf.attributes |= USB_CONF_ATTR_SELF_POWERED;
+    }
+    /* Todo: upper bound */
+    conf.max_power = USB_CONFIG_MAX_POWER / 2;

why / 2 ?

-- 
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-219680817
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190327/c2630823/attachment-0001.html>


More information about the notifications mailing list