shared/tinyusb: Fix dynamic USB control callbacks for wLength==0.

projectgus · projectgus · commit f2d7ce04dff7 · 2024-04-05T15:25:45.000+11:00
In the case where an OUT control transfer triggers with wLength==0 (i.e.
all data sent in the SETUP phase, and no additional data phase) the
callbacks were previously implemented to return b"" (i.e. an empty buffer
for the data phase).

However this didn't actually work as intended because b"" can't provide a
RW buffer (needed for OUT transfers with a data phase to write data into),
so actually the endpoint would stall.

The symptom was often that the device process the request (if processing
it in the SETUP phase when all information was already available), but the
host sees the endpoint stall and eventually returns an error.

This commit changes the behaviour so returning True from the SETUP phase of
a control transfer queues a zero length status response.

Signed-off-by: Angus Gratton &lt;angus@redyak.com.au&gt;
diff --git a/docs/library/machine.USBDevice.rst b/docs/library/machine.USBDevice.rst
@@ -130,15 +130,25 @@ Methods
 
       Second argument is a memoryview to read the USB control request
       data for this stage. The memoryview is only valid until the
-      callback function returns.
+      callback function returns. Data in this memoryview will be the same
+      across each of the three stages of a single transfer.
+
+      A successful transfer consists of this callback being called in sequence
+      for the three stages. Generally speaking, if a device wants to do
+      something in response to a control request then it's best to wait until
+      the ACK stage to confirm the host controller completed the transfer as
+      expected.
 
       The callback should return one of the following values:
 
-      - ``False`` to stall the endpoint and reject the transfer.
+      - ``False`` to stall the endpoint and reject the transfer. It won't
+        proceed to any remaining stages.
       - ``True`` to continue the transfer to the next stage.
-      - A buffer object to provide data for this stage of the transfer.
-        This should be a writable buffer for an ``OUT`` direction transfer, or a
-        readable buffer with data for an ``IN`` direction transfer.
+      - A buffer object can be returned at the SETUP stage when the transfer
+        will send or receive additional data. Typically this is the case when
+        the ``wLength`` field in the request has a non-zero value. This should
+        be a writable buffer for an ``OUT`` direction transfer, or a readable
+        buffer with data for an ``IN`` direction transfer.
 
     - ``xfer_cb`` - This callback is called whenever a non-control
       transfer submitted by calling :func:`USBDevice.submit_xfer` completes.
diff --git a/shared/tinyusb/mp_usbd_runtime.c b/shared/tinyusb/mp_usbd_runtime.c
@@ -295,6 +295,7 @@ static bool runtime_dev_control_xfer_cb(uint8_t rhport, uint8_t stage, tusb_cont
     mp_obj_usb_device_t *usbd = MP_OBJ_TO_PTR(MP_STATE_VM(usbd));
     tusb_dir_t dir = request->bmRequestType_bit.direction;
     mp_buffer_info_t buf_info;
+    bool result;
 
     if (!usbd) {
         return false;
@@ -319,7 +320,7 @@ static bool runtime_dev_control_xfer_cb(uint8_t rhport, uint8_t stage, tusb_cont
 
     // Check if callback returned any data to submit
     if (mp_get_buffer(cb_res, &buf_info, dir == TUSB_DIR_IN ? MP_BUFFER_READ : MP_BUFFER_RW)) {
-        bool result = tud_control_xfer(USBD_RHPORT,
+        result = tud_control_xfer(USBD_RHPORT,
             request,
             buf_info.buf,
             buf_info.len);
@@ -328,17 +329,21 @@ static bool runtime_dev_control_xfer_cb(uint8_t rhport, uint8_t stage, tusb_cont
             // Keep buffer object alive until the transfer completes
             usbd->xfer_data[0][dir] = cb_res;
         }
-
-        return result;
     } else {
         // Expect True or False to stall or continue
+        result = mp_obj_is_true(cb_res);
 
-        if (stage == CONTROL_STAGE_ACK) {
+        if (stage == CONTROL_STAGE_SETUP && result) {
+            // If no additional data but callback says to continue transfer then
+            // queue a status response.
+            tud_control_status(rhport, request);
+        } else if (stage == CONTROL_STAGE_ACK) {
             // Allow data to be GCed once it's no longer in use
             usbd->xfer_data[0][dir] = mp_const_none;
         }
-        return mp_obj_is_true(cb_res);
     }
+
+    return result;
 }
 
 static bool runtime_dev_xfer_cb(uint8_t rhport, uint8_t ep_addr, xfer_result_t result, uint32_t xferred_bytes) {
