diff --git a/shared-bindings/usb/core/__init__.c b/shared-bindings/usb/core/__init__.c index 545e182ea99a3..f713e9a281c00 100644 --- a/shared-bindings/usb/core/__init__.c +++ b/shared-bindings/usb/core/__init__.c @@ -30,17 +30,9 @@ //| //| MP_DEFINE_USB_CORE_EXCEPTION(USBError, OSError) -NORETURN void mp_raise_usb_core_USBError(mp_rom_error_text_t fmt, ...) { - mp_obj_t exception; - if (fmt == NULL) { - exception = mp_obj_new_exception(&mp_type_usb_core_USBError); - } else { - va_list argptr; - va_start(argptr, fmt); - exception = mp_obj_new_exception_msg_vlist(&mp_type_usb_core_USBError, fmt, argptr); - va_end(argptr); - } - nlr_raise(exception); +NORETURN MP_COLD void mp_raise_usb_core_USBError(int errno) { + mp_obj_t args[1] = {MP_OBJ_NEW_SMALL_INT(errno)}; + nlr_raise(mp_obj_new_exception_args(&mp_type_usb_core_USBError, 1, args)); } //| class USBTimeoutError(USBError): @@ -50,8 +42,9 @@ NORETURN void mp_raise_usb_core_USBError(mp_rom_error_text_t fmt, ...) { //| //| MP_DEFINE_USB_CORE_EXCEPTION(USBTimeoutError, usb_core_USBError) -NORETURN void mp_raise_usb_core_USBTimeoutError(void) { - mp_raise_type(&mp_type_usb_core_USBTimeoutError); +NORETURN void mp_raise_usb_core_USBTimeoutError(int errno) { + mp_obj_t args[1] = {MP_OBJ_NEW_SMALL_INT(errno)}; + nlr_raise(mp_obj_new_exception_args(&mp_type_usb_core_USBTimeoutError, 1, args)); } diff --git a/shared-bindings/usb/core/__init__.h b/shared-bindings/usb/core/__init__.h index 0dc08fae53257..ef2621899fb96 100644 --- a/shared-bindings/usb/core/__init__.h +++ b/shared-bindings/usb/core/__init__.h @@ -25,8 +25,8 @@ void usb_core_exception_print(const mp_print_t *print, mp_obj_t o_in, mp_print_k extern const mp_obj_type_t mp_type_usb_core_USBError; extern const mp_obj_type_t mp_type_usb_core_USBTimeoutError; -NORETURN void mp_raise_usb_core_USBError(mp_rom_error_text_t fmt, ...); -NORETURN void mp_raise_usb_core_USBTimeoutError(void); +NORETURN void mp_raise_usb_core_USBError(int errno); +NORETURN void mp_raise_usb_core_USBTimeoutError(int errno); // Find is all Python object oriented so we don't need a separate common-hal API // for it. It uses the device common-hal instead. diff --git a/shared-module/usb/core/Device.c b/shared-module/usb/core/Device.c index c9df0cf73805f..b02c100e187ec 100644 --- a/shared-module/usb/core/Device.c +++ b/shared-module/usb/core/Device.c @@ -1,6 +1,7 @@ // This file is part of the CircuitPython project: https://circuitpython.org // // SPDX-FileCopyrightText: Copyright (c) 2022 Scott Shawcroft for Adafruit Industries +// SPDX-FileCopyrightText: Copyright (c) 2025 Sam Blenny // // SPDX-License-Identifier: MIT @@ -45,7 +46,7 @@ bool common_hal_usb_core_device_construct(usb_core_device_obj_t *self, uint8_t d } self->device_address = device_address; self->first_langid = 0; - _xfer_result = 0xff; + _xfer_result = XFER_RESULT_INVALID; return true; } @@ -67,17 +68,39 @@ void common_hal_usb_core_device_deinit(usb_core_device_obj_t *self) { self->device_address = 0; } +static void _wait_then_raise_USBError(int errno) { + // 1. Spin briefly in background task loop because this seems to fix the + // enumeration or addressing glitch that can cause tuh_* calls to fail after + // a device is unplugged. Don't know why this works, but things magically + // self correct if you just wait for a bit before attempting another control + // transfer. + // 2. Raise the exception so the calling code knows it needs to try again. + // + // TODO: Consider removing this mechanism if somebody finds and fixes whatever is + // causing TinyUSB to get confused about unplugged devices. + // + const uint32_t end = supervisor_ticks_ms32() + USB_CORE_TUH_FAIL_WAIT_MS; + while (supervisor_ticks_ms32() < end && !mp_hal_is_interrupted()) { + RUN_BACKGROUND_TASKS; + } + mp_raise_usb_core_USBError(errno); +} + uint16_t common_hal_usb_core_device_get_idVendor(usb_core_device_obj_t *self) { uint16_t vid; uint16_t pid; - tuh_vid_pid_get(self->device_address, &vid, &pid); + if (!tuh_vid_pid_get(self->device_address, &vid, &pid)) { + _wait_then_raise_USBError(USB_CORE_VID_PID_GET); + } return vid; } uint16_t common_hal_usb_core_device_get_idProduct(usb_core_device_obj_t *self) { uint16_t vid; uint16_t pid; - tuh_vid_pid_get(self->device_address, &vid, &pid); + if (!tuh_vid_pid_get(self->device_address, &vid, &pid)) { + _wait_then_raise_USBError(USB_CORE_VID_PID_GET); + } return pid; } @@ -91,16 +114,77 @@ static void _transfer_done_cb(tuh_xfer_t *xfer) { static bool _wait_for_callback(void) { while (!mp_hal_is_interrupted() && - _xfer_result == 0xff) { + _xfer_result == XFER_RESULT_INVALID) { // The background tasks include TinyUSB which will call the function // we provided above. In other words, the callback isn't in an interrupt. RUN_BACKGROUND_TASKS; } xfer_result_t result = _xfer_result; - _xfer_result = 0xff; + _xfer_result = XFER_RESULT_INVALID; return result == XFER_RESULT_SUCCESS; } +static void _prepare_for_transfer(void) { + // Prepare for transfer. Unless there is a timeout, these static globals will + // get modified by the _transfer_done_cb() callback when tinyusb finishes the + // transfer or encounters an error condition. + _xfer_result = XFER_RESULT_INVALID; + _actual_len = 0; +} + +static size_t _handle_timed_transfer_callback(tuh_xfer_t *xfer, mp_int_t timeout) { + if (xfer == NULL) { + mp_raise_usb_core_USBError(USB_CORE_NULL_PTR); + return 0; + } + uint32_t start_time = supervisor_ticks_ms32(); + while ((timeout == 0 || supervisor_ticks_ms32() - start_time < (uint32_t)timeout) && + !mp_hal_is_interrupted() && + _xfer_result == XFER_RESULT_INVALID) { + // The background tasks include TinyUSB which will call the function + // we provided above. In other words, the callback isn't in an interrupt. + RUN_BACKGROUND_TASKS; + } + if (mp_hal_is_interrupted()) { + // Handle case of VM being interrupted by Ctrl-C or autoreload + tuh_edpt_abort_xfer(xfer->daddr, xfer->ep_addr); + return 0; + } + // Handle control transfer result code from TinyUSB + xfer_result_t result = _xfer_result; + _xfer_result = XFER_RESULT_INVALID; + switch (result) { + case XFER_RESULT_SUCCESS: + return _actual_len; + case XFER_RESULT_FAILED: + // TODO: swap this for mp_raise_usb_core_USBError if TinyUSB can be + // improved to prevent a series of failures when usb.core.find() is called + // in a tight loop after a device has been unplugged. + // + // This workaround adds a delay before raising the exception because, for + // mysterious as-yet unknown reasons, that seems to let TinyUSB clear up + // whatever fault condition is triggered by unplugging a device. + // + _wait_then_raise_USBError(USB_CORE_XFER_FAIL); + break; + case XFER_RESULT_STALLED: + mp_raise_usb_core_USBError(USB_CORE_STALLED); + break; + case XFER_RESULT_TIMEOUT: + // This timeout comes from TinyUSB, so assume that it has stopped the + // transfer (note: timeout logic may be unimplemented on TinyUSB side) + mp_raise_usb_core_USBTimeoutError(USB_CORE_TIMEOUT); + break; + case XFER_RESULT_INVALID: + // This timeout comes from CircuitPython, not TinyUSB, so tell TinyUSB + // to stop the transfer + tuh_edpt_abort_xfer(xfer->daddr, xfer->ep_addr); + mp_raise_usb_core_USBTimeoutError(USB_CORE_INVALID); + break; + } + return 0; +} + static mp_obj_t _get_string(const uint16_t *temp_buf) { size_t utf16_len = ((temp_buf[0] & 0xff) - 2) / sizeof(uint16_t); if (utf16_len == 0) { @@ -225,38 +309,13 @@ void common_hal_usb_core_device_set_configuration(usb_core_device_obj_t *self, m } static size_t _xfer(tuh_xfer_t *xfer, mp_int_t timeout) { - _xfer_result = 0xff; + _prepare_for_transfer(); xfer->complete_cb = _transfer_done_cb; if (!tuh_edpt_xfer(xfer)) { - mp_raise_usb_core_USBError(NULL); + mp_raise_usb_core_USBError(USB_CORE_EDPT_XFER); return 0; } - uint32_t start_time = supervisor_ticks_ms32(); - while ((timeout == 0 || supervisor_ticks_ms32() - start_time < (uint32_t)timeout) && - !mp_hal_is_interrupted() && - _xfer_result == 0xff) { - // The background tasks include TinyUSB which will call the function - // we provided above. In other words, the callback isn't in an interrupt. - RUN_BACKGROUND_TASKS; - } - if (mp_hal_is_interrupted()) { - tuh_edpt_abort_xfer(xfer->daddr, xfer->ep_addr); - return 0; - } - xfer_result_t result = _xfer_result; - _xfer_result = 0xff; - if (result == XFER_RESULT_STALLED) { - mp_raise_usb_core_USBError(MP_ERROR_TEXT("Pipe error")); - } - if (result == 0xff) { - tuh_edpt_abort_xfer(xfer->daddr, xfer->ep_addr); - mp_raise_usb_core_USBTimeoutError(); - } - if (result == XFER_RESULT_SUCCESS) { - return _actual_len; - } - - return 0; + return _handle_timed_transfer_callback(xfer, timeout); } static bool _open_endpoint(usb_core_device_obj_t *self, mp_int_t endpoint) { @@ -275,7 +334,7 @@ static bool _open_endpoint(usb_core_device_obj_t *self, mp_int_t endpoint) { } if (self->configuration_descriptor == NULL) { - mp_raise_usb_core_USBError(MP_ERROR_TEXT("No configuration set")); + mp_raise_usb_core_USBError(USB_CORE_NOCONFIG); return false; } @@ -310,7 +369,7 @@ static bool _open_endpoint(usb_core_device_obj_t *self, mp_int_t endpoint) { mp_int_t common_hal_usb_core_device_write(usb_core_device_obj_t *self, mp_int_t endpoint, const uint8_t *buffer, mp_int_t len, mp_int_t timeout) { if (!_open_endpoint(self, endpoint)) { - mp_raise_usb_core_USBError(NULL); + mp_raise_usb_core_USBError(USB_CORE_OPEN_ENDPOINT); return 0; } tuh_xfer_t xfer; @@ -323,7 +382,7 @@ mp_int_t common_hal_usb_core_device_write(usb_core_device_obj_t *self, mp_int_t mp_int_t common_hal_usb_core_device_read(usb_core_device_obj_t *self, mp_int_t endpoint, uint8_t *buffer, mp_int_t len, mp_int_t timeout) { if (!_open_endpoint(self, endpoint)) { - mp_raise_usb_core_USBError(NULL); + mp_raise_usb_core_USBError(USB_CORE_OPEN_ENDPOINT); return 0; } tuh_xfer_t xfer; @@ -355,38 +414,12 @@ mp_int_t common_hal_usb_core_device_ctrl_transfer(usb_core_device_obj_t *self, .complete_cb = _transfer_done_cb, }; - _xfer_result = 0xff; - + _prepare_for_transfer(); if (!tuh_control_xfer(&xfer)) { - mp_raise_usb_core_USBError(NULL); + mp_raise_usb_core_USBError(USB_CORE_CONTROL_XFER); return 0; } - uint32_t start_time = supervisor_ticks_ms32(); - while ((timeout == 0 || supervisor_ticks_ms32() - start_time < (uint32_t)timeout) && - !mp_hal_is_interrupted() && - _xfer_result == 0xff) { - // The background tasks include TinyUSB which will call the function - // we provided above. In other words, the callback isn't in an interrupt. - RUN_BACKGROUND_TASKS; - } - if (mp_hal_is_interrupted()) { - tuh_edpt_abort_xfer(xfer.daddr, xfer.ep_addr); - return 0; - } - xfer_result_t result = _xfer_result; - _xfer_result = 0xff; - if (result == XFER_RESULT_STALLED) { - mp_raise_usb_core_USBError(MP_ERROR_TEXT("Pipe error")); - } - if (result == 0xff) { - tuh_edpt_abort_xfer(xfer.daddr, xfer.ep_addr); - mp_raise_usb_core_USBTimeoutError(); - } - if (result == XFER_RESULT_SUCCESS) { - return len; - } - - return 0; + return (mp_int_t)_handle_timed_transfer_callback(&xfer, timeout); } bool common_hal_usb_core_device_is_kernel_driver_active(usb_core_device_obj_t *self, mp_int_t interface) { diff --git a/shared-module/usb/core/Device.h b/shared-module/usb/core/Device.h index c7392a04ba7bc..9e2f4deaa2fa5 100644 --- a/shared-module/usb/core/Device.h +++ b/shared-module/usb/core/Device.h @@ -1,6 +1,7 @@ // This file is part of the CircuitPython project: https://circuitpython.org // // SPDX-FileCopyrightText: Copyright (c) 2022 Scott Shawcroft for Adafruit Industries +// SPDX-FileCopyrightText: Copyright (c) 2025 Sam Blenny // // SPDX-License-Identifier: MIT @@ -16,3 +17,37 @@ typedef struct { uint8_t open_endpoints[8]; uint16_t first_langid; } usb_core_device_obj_t; + + +// Number of ms to wait in a background task loop after a TinyUSB tuh_* api call +// returns a failure result. This delay is part of a kludge to help TinyUSB +// recognize when a device has been unplugged. Without the delay, usb.core.find() +// can get super broken. This mechanism can be removed once somebody finds and +// fixes whatever is causing TinyUSB to get confused about unplugged devices. +#define USB_CORE_TUH_FAIL_WAIT_MS (15) + +// These values get used to set USBError.errno and USBTimeoutError.errno. +// It would be possible to define these to more closely mimic desktop PyUSB on +// a given OS (maybe Debian?). But, for USB error handling in CircuitPython, +// there's an argument to be made that it's more useful to set arbitrary codes +// here that map directly to errors coming from TinyUSB. That way, CircuitPython +// code can have more visibility into what went wrong. POSIX errno codes are +// pretty far removed from the details of how TinyUSB can fail, so using them +// here would be uninformative. + +// Error due to attempting to open endpoint before setting configuration +#define USB_CORE_NOCONFIG (1) + +// Errors from transfer callback result +#define USB_CORE_NULL_PTR (2) +#define USB_CORE_XFER_FAIL (3) +#define USB_CORE_STALLED (4) +#define USB_CORE_TIMEOUT (5) +#define USB_CORE_INVALID (6) + +// Errors from a non-callback TinyUSB function returning false. Seeing one of +// these probably means a TU_VERIFY(...) check failed in the TinyUSB code. +#define USB_CORE_EDPT_XFER (7) +#define USB_CORE_OPEN_ENDPOINT (8) +#define USB_CORE_CONTROL_XFER (9) +#define USB_CORE_VID_PID_GET (10)