[riot-notifications] [RIOT-OS/RIOT] usbus/hid_io: add missing header file, add RX callback function (#16689)

Leandro Lanzieri notifications at github.com
Wed Aug 4 09:12:24 CEST 2021


@leandrolanzieri commented on this pull request.



>  }
 
 void usb_hid_io_write(const void *buffer, size_t len)
 {
-    uint8_t* buffer_ep = hid.ep_in->ep->buf;
+    uint8_t *buffer_ep = hid.ep_in->ep->buf;

Could we `assert(buffer);`?

> + */
+typedef struct {
+    usb_hid_io_cb_t cb;         /**< callback function to call */
+    void *arg;                  /**< argument to pass to callback function */
+} usb_hid_io_t;
+
+/**
+ * @brief Set USBUS HID IO RX callback
+ *
+ * @param[in]   usb_hid_io  usb_hid_io structure to use
+ */
+void usb_hid_io_set_rx_cb(usb_hid_io_t* usb_hid_io);
+
+/**
+ * @brief Write data to USB HID IN endpoint buffer
+ *

`@pre buffer != NULL`

> + *
+ * @param[out]      buffer  buffer to read data into
+ * @param[in]       len     length of buffer
+ */

```suggestion
 *
 * @pre `buffer != NULL`
 *
 * @param[out]      buffer  buffer to read data into
 * @param[in]       len     length of buffer
 *
 * @return Number of read bytes
 */
```

> + *
+ * Wrapper for @ref isrpipe_read_timeout
+ *
+ * @param[out]      buffer  buffer to read data into
+ * @param[in]       len     length of buffer
+ * @param[in]       timeout timeout in microseconds

Same here

> + * Wrapper for @ref isrpipe_read_timeout
+ *
+ * @param[out]      buffer  buffer to read data into
+ * @param[in]       len     length of buffer
+ * @param[in]       timeout timeout in microseconds
+ */
+int usb_hid_io_read_timeout(void *buffer, size_t len, uint32_t timeout);
+
+/**
+ * @brief Initialize an USB HID IO interface
+ *
+ * @param[in]   usbus               USBUS context
+ * @param[in]   report_desc         USB_HID report descriptor
+ * @param[in]   report_desc_size    size of USB_HID report descriptor
+ */
+void usb_hid_io_init(usbus_t *usbus, uint8_t *report_desc, size_t report_desc_size);

Looks like we can make the descriptor `const`.
```suggestion
void usb_hid_io_init(usbus_t *usbus, const uint8_t *report_desc, size_t report_desc_size);
```

>  {
-    return isrpipe_read(&_hid_stdio_isrpipe, buffer, size);
+    return isrpipe_read(&_hid_stdio_isrpipe, buffer, len);

I think here and bellow we should also `assert(buffer);` at least.

>  }
 
 void usb_hid_io_init(usbus_t *usbus, uint8_t *report_desc,
                      size_t report_desc_size)
 {
     usbus_hid_init(usbus, &hid, _hid_rx_pipe,  report_desc, report_desc_size);
 }
+
+void usb_hid_io_set_rx_cb(usb_hid_io_t *usb_hid_io)

Is there an advantage on keeping a reference to this structure, that basically holds the arguments here? Why not have:
```suggestion
void usb_hid_io_set_rx_cb(usb_hid_io_cb_t cb, void *arg)
```

> + *
+ * @param[out]      buffer  buffer to read data into
+ * @param[in]       len     length of buffer
+ */

Please check if there are possible error return values, in that case we can add them with `@retval` bellow.

-- 
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/16689#pullrequestreview-721918499
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210804/bbe76d37/attachment.htm>


More information about the notifications mailing list