From d11ca092f75d2652df403dda7448fe1bc8f98cf7 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Fri, 5 Apr 2024 14:46:45 +1100 Subject: [PATCH] shared/tinyusb: Fix dynamic USB control callbacks for wLength==0. 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 --- docs/library/machine.USBDevice.rst | 20 +++++++++++++++----- shared/tinyusb/mp_usbd_runtime.c | 15 ++++++++++----- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/docs/library/machine.USBDevice.rst b/docs/library/machine.USBDevice.rst index 82897b280d..e02d3c02df 100644 --- 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 index 0b4c79831b..7ff7dd4ab2 100644 --- 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) {