From ea359fcf45426b362fbf3984fcd2ffbb27911bb7 Mon Sep 17 00:00:00 2001 From: sam blenny <68084116+samblenny@users.noreply.github.com> Date: Tue, 2 Sep 2025 22:16:35 +0000 Subject: [PATCH 1/8] fix bad xfer_result_t enum values shared-module/usb/core/Device.c was using its own 0xff value for the xfer_result_t enum defined by tinyusb/src/common/tusb_types.h. The 0xff value served the same purpose as the already exisiting XFER_RESULT_INVALID enum value (a placeholder to mark in-progress transactions). This commit standardizes on XFER_RESULT_INVALID in usb.core.Device consistent with the usage in tinyusb. Making this change allows implementing `switch(result){...}` style result code checks without compiler errors about 0xff not being a valid value for the enum. --- shared-module/usb/core/Device.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/shared-module/usb/core/Device.c b/shared-module/usb/core/Device.c index c9df0cf73805f..f6cd03ebcd40a 100644 --- a/shared-module/usb/core/Device.c +++ b/shared-module/usb/core/Device.c @@ -45,7 +45,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; } @@ -91,13 +91,13 @@ 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; } @@ -225,7 +225,7 @@ 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; + _xfer_result = XFER_RESULT_INVALID; xfer->complete_cb = _transfer_done_cb; if (!tuh_edpt_xfer(xfer)) { mp_raise_usb_core_USBError(NULL); @@ -234,7 +234,7 @@ static size_t _xfer(tuh_xfer_t *xfer, mp_int_t timeout) { 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) { + _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; @@ -244,11 +244,11 @@ static size_t _xfer(tuh_xfer_t *xfer, mp_int_t timeout) { return 0; } xfer_result_t result = _xfer_result; - _xfer_result = 0xff; + _xfer_result = XFER_RESULT_INVALID; if (result == XFER_RESULT_STALLED) { mp_raise_usb_core_USBError(MP_ERROR_TEXT("Pipe error")); } - if (result == 0xff) { + if (result == XFER_RESULT_INVALID) { tuh_edpt_abort_xfer(xfer->daddr, xfer->ep_addr); mp_raise_usb_core_USBTimeoutError(); } @@ -355,7 +355,7 @@ mp_int_t common_hal_usb_core_device_ctrl_transfer(usb_core_device_obj_t *self, .complete_cb = _transfer_done_cb, }; - _xfer_result = 0xff; + _xfer_result = XFER_RESULT_INVALID; if (!tuh_control_xfer(&xfer)) { mp_raise_usb_core_USBError(NULL); @@ -364,7 +364,7 @@ mp_int_t common_hal_usb_core_device_ctrl_transfer(usb_core_device_obj_t *self, 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) { + _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; @@ -374,11 +374,11 @@ mp_int_t common_hal_usb_core_device_ctrl_transfer(usb_core_device_obj_t *self, return 0; } xfer_result_t result = _xfer_result; - _xfer_result = 0xff; + _xfer_result = XFER_RESULT_INVALID; if (result == XFER_RESULT_STALLED) { mp_raise_usb_core_USBError(MP_ERROR_TEXT("Pipe error")); } - if (result == 0xff) { + if (result == XFER_RESULT_INVALID) { tuh_edpt_abort_xfer(xfer.daddr, xfer.ep_addr); mp_raise_usb_core_USBTimeoutError(); } From b7fb46b3d4f4795fe7d05c0111e3e2e7ac1fe47e Mon Sep 17 00:00:00 2001 From: sam blenny <68084116+samblenny@users.noreply.github.com> Date: Tue, 2 Sep 2025 22:40:46 +0000 Subject: [PATCH 2/8] use switch for ctrl_transfer result check This directly translates the Device.ctrl_transfer() result check logic from its old if-statements to an equivalent switch-statement. The point is to make it clear how each possible result code is handled. Note that XFER_RESULT_FAILED and XFER_RESULT_TIMEOUT both return 0 without generating any exception. (but also, tinyusb may not actually use XFER_RESULT_TIMOUT if its comments are still accurate) --- shared-module/usb/core/Device.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/shared-module/usb/core/Device.c b/shared-module/usb/core/Device.c index f6cd03ebcd40a..87d1141bfde39 100644 --- a/shared-module/usb/core/Device.c +++ b/shared-module/usb/core/Device.c @@ -375,17 +375,21 @@ mp_int_t common_hal_usb_core_device_ctrl_transfer(usb_core_device_obj_t *self, } xfer_result_t result = _xfer_result; _xfer_result = XFER_RESULT_INVALID; - if (result == XFER_RESULT_STALLED) { - mp_raise_usb_core_USBError(MP_ERROR_TEXT("Pipe error")); - } - if (result == XFER_RESULT_INVALID) { - tuh_edpt_abort_xfer(xfer.daddr, xfer.ep_addr); - mp_raise_usb_core_USBTimeoutError(); - } - if (result == XFER_RESULT_SUCCESS) { - return len; + switch (result) { + case XFER_RESULT_SUCCESS: + return len; + case XFER_RESULT_FAILED: + break; + case XFER_RESULT_STALLED: + mp_raise_usb_core_USBError(MP_ERROR_TEXT("Pipe error")); + break; + case XFER_RESULT_TIMEOUT: + break; + case XFER_RESULT_INVALID: + tuh_edpt_abort_xfer(xfer.daddr, xfer.ep_addr); + mp_raise_usb_core_USBTimeoutError(); + break; } - return 0; } From 45e8fe058ef511cae4accf227c6f451fae550c2f Mon Sep 17 00:00:00 2001 From: sam blenny <68084116+samblenny@users.noreply.github.com> Date: Tue, 2 Sep 2025 23:02:39 +0000 Subject: [PATCH 3/8] return actual length from ctrl_transfer Previously this returned the requested transfer length argument, ignoring the actual length of transferred bytes. This changes to returning the actual length. --- shared-module/usb/core/Device.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/shared-module/usb/core/Device.c b/shared-module/usb/core/Device.c index 87d1141bfde39..517eb5f357a07 100644 --- a/shared-module/usb/core/Device.c +++ b/shared-module/usb/core/Device.c @@ -355,7 +355,11 @@ mp_int_t common_hal_usb_core_device_ctrl_transfer(usb_core_device_obj_t *self, .complete_cb = _transfer_done_cb, }; + // 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; if (!tuh_control_xfer(&xfer)) { mp_raise_usb_core_USBError(NULL); @@ -377,7 +381,7 @@ mp_int_t common_hal_usb_core_device_ctrl_transfer(usb_core_device_obj_t *self, _xfer_result = XFER_RESULT_INVALID; switch (result) { case XFER_RESULT_SUCCESS: - return len; + return _actual_len; case XFER_RESULT_FAILED: break; case XFER_RESULT_STALLED: From 1acbade9e22bde019ec648855dad1e18a6847834 Mon Sep 17 00:00:00 2001 From: sam blenny <68084116+samblenny@users.noreply.github.com> Date: Wed, 3 Sep 2025 00:41:44 +0000 Subject: [PATCH 4/8] handle XFER_RESULT_FAILED with exception Previously, usb.core.Device.ctrl_transfer() did not raise an exception when TinyUSB returned an XFER_RESULT_FAILED result code. This change raises an exception which prevents a failed transfer from returning an all zero result buffer. --- shared-module/usb/core/Device.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/shared-module/usb/core/Device.c b/shared-module/usb/core/Device.c index 517eb5f357a07..79b6775dd0c1a 100644 --- a/shared-module/usb/core/Device.c +++ b/shared-module/usb/core/Device.c @@ -374,22 +374,30 @@ mp_int_t common_hal_usb_core_device_ctrl_transfer(usb_core_device_obj_t *self, 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: + mp_raise_usb_core_USBError(NULL); break; case XFER_RESULT_STALLED: mp_raise_usb_core_USBError(MP_ERROR_TEXT("Pipe error")); 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(); 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(); break; From fbf4e5145def02950484b0caa6ca2bf90b0d915e Mon Sep 17 00:00:00 2001 From: sam blenny <68084116+samblenny@users.noreply.github.com> Date: Wed, 3 Sep 2025 01:50:48 +0000 Subject: [PATCH 5/8] factor out timed transfer setup & err handling The code for endpoint transfers and control transfers previously had a bunch of duplicated logic for setup, timeout checking, and result code error handling. This change factors that stuff out into functions, using my new transfer result error handling code from the last commit. Now the endpoint and control transfers can share the setup, timeout, and error check logic. For me, this refactor reduced the firmware image size by 176 bytes. --- shared-module/usb/core/Device.c | 128 ++++++++++++++------------------ 1 file changed, 57 insertions(+), 71 deletions(-) diff --git a/shared-module/usb/core/Device.c b/shared-module/usb/core/Device.c index 79b6775dd0c1a..f71ac8844bb73 100644 --- a/shared-module/usb/core/Device.c +++ b/shared-module/usb/core/Device.c @@ -101,6 +101,59 @@ static bool _wait_for_callback(void) { 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(NULL); + 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: + mp_raise_usb_core_USBError(NULL); + break; + case XFER_RESULT_STALLED: + mp_raise_usb_core_USBError(MP_ERROR_TEXT("Pipe error")); + 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(); + 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(); + 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 +278,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 = XFER_RESULT_INVALID; + _prepare_for_transfer(); xfer->complete_cb = _transfer_done_cb; if (!tuh_edpt_xfer(xfer)) { mp_raise_usb_core_USBError(NULL); 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()) { - tuh_edpt_abort_xfer(xfer->daddr, xfer->ep_addr); - return 0; - } - xfer_result_t result = _xfer_result; - _xfer_result = XFER_RESULT_INVALID; - if (result == XFER_RESULT_STALLED) { - mp_raise_usb_core_USBError(MP_ERROR_TEXT("Pipe error")); - } - if (result == XFER_RESULT_INVALID) { - 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) { @@ -355,54 +383,12 @@ mp_int_t common_hal_usb_core_device_ctrl_transfer(usb_core_device_obj_t *self, .complete_cb = _transfer_done_cb, }; - // 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; - + _prepare_for_transfer(); if (!tuh_control_xfer(&xfer)) { mp_raise_usb_core_USBError(NULL); 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: - mp_raise_usb_core_USBError(NULL); - break; - case XFER_RESULT_STALLED: - mp_raise_usb_core_USBError(MP_ERROR_TEXT("Pipe error")); - 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(); - 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(); - break; - } - 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) { From 42b53b798efe72c0e8da9eee78af87ab3b9b459c Mon Sep 17 00:00:00 2001 From: sam blenny <68084116+samblenny@users.noreply.github.com> Date: Thu, 4 Sep 2025 18:31:13 +0000 Subject: [PATCH 6/8] set errno for USBError & USBTimeoutError This sets unique errno values for each place in the usb.core.Device implementation that raises USBError or USBTimeoutError. The idea is to provide more visibility so CircuitPython code can understand what's gone wrong when TinyUSB complains about something. CAUTION: This removes two error strings (stalled pipe and config not set). The new way relies on unique error codes as defined in shared-module/usb/core/Device.h. The "errno" values here are arbitrary with no relation to POSIX errno codes. I don't see this as a problem because the so-called errno codes returned by desktop PyUSB are undocumented and appear to be rather aribitrary. --- shared-bindings/usb/core/__init__.c | 19 ++++++------------- shared-bindings/usb/core/__init__.h | 4 ++-- shared-module/usb/core/Device.c | 21 +++++++++++---------- shared-module/usb/core/Device.h | 27 +++++++++++++++++++++++++++ 4 files changed, 46 insertions(+), 25 deletions(-) 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 f71ac8844bb73..d10a5acfafac1 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 @@ -111,7 +112,7 @@ static void _prepare_for_transfer(void) { static size_t _handle_timed_transfer_callback(tuh_xfer_t *xfer, mp_int_t timeout) { if (xfer == NULL) { - mp_raise_usb_core_USBError(NULL); + mp_raise_usb_core_USBError(USB_CORE_NULL_PTR); return 0; } uint32_t start_time = supervisor_ticks_ms32(); @@ -134,21 +135,21 @@ static size_t _handle_timed_transfer_callback(tuh_xfer_t *xfer, mp_int_t timeout case XFER_RESULT_SUCCESS: return _actual_len; case XFER_RESULT_FAILED: - mp_raise_usb_core_USBError(NULL); + mp_raise_usb_core_USBError(USB_CORE_XFER_FAIL); break; case XFER_RESULT_STALLED: - mp_raise_usb_core_USBError(MP_ERROR_TEXT("Pipe error")); + 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(); + 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(); + mp_raise_usb_core_USBTimeoutError(USB_CORE_INVALID); break; } return 0; @@ -281,7 +282,7 @@ static size_t _xfer(tuh_xfer_t *xfer, mp_int_t timeout) { _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; } return _handle_timed_transfer_callback(xfer, timeout); @@ -303,7 +304,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; } @@ -338,7 +339,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; @@ -351,7 +352,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; @@ -385,7 +386,7 @@ mp_int_t common_hal_usb_core_device_ctrl_transfer(usb_core_device_obj_t *self, _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; } return (mp_int_t)_handle_timed_transfer_callback(&xfer, timeout); diff --git a/shared-module/usb/core/Device.h b/shared-module/usb/core/Device.h index c7392a04ba7bc..4ad38170d511e 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,29 @@ typedef struct { uint8_t open_endpoints[8]; uint16_t first_langid; } usb_core_device_obj_t; + + +// 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) From f1ad79bb84a7100b2a2194c1b05202d14cedbd74 Mon Sep 17 00:00:00 2001 From: sam blenny <68084116+samblenny@users.noreply.github.com> Date: Thu, 4 Sep 2025 21:37:47 +0000 Subject: [PATCH 7/8] more usb.core.Device vid/pid error handling This fixes a bug where common_hal_usb_core_device_get_idVendor() and common_hal_usb_core_device_get_idProduct() were ignoring the error check boolean returned by tuh_vid_pid_get(). Also, this includes a workaround for what appears to be a TinyUSB glitch with enumeration and assigning addresses. By adding a 50 ms background task loop before raising the exception after a TinyUSB failure result, somehow the vid/pid error condition magically goes away on it's own. Something subtle and weird is happening here which could use further investigation. --- shared-module/usb/core/Device.c | 29 ++++++++++++++++++++++++++--- shared-module/usb/core/Device.h | 1 + 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/shared-module/usb/core/Device.c b/shared-module/usb/core/Device.c index d10a5acfafac1..3811d678b8b8e 100644 --- a/shared-module/usb/core/Device.c +++ b/shared-module/usb/core/Device.c @@ -68,17 +68,32 @@ 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 for 50 ms in a background task loop because some USB glitches + // will magically clear up if you just wait for a bit + // 2. Raise the exception so the calling code knows it needs to try again + const uint32_t end = supervisor_ticks_ms32() + 50; + 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; } @@ -135,7 +150,15 @@ static size_t _handle_timed_transfer_callback(tuh_xfer_t *xfer, mp_int_t timeout case XFER_RESULT_SUCCESS: return _actual_len; case XFER_RESULT_FAILED: - mp_raise_usb_core_USBError(USB_CORE_XFER_FAIL); + // 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); diff --git a/shared-module/usb/core/Device.h b/shared-module/usb/core/Device.h index 4ad38170d511e..22729cb3359ed 100644 --- a/shared-module/usb/core/Device.h +++ b/shared-module/usb/core/Device.h @@ -43,3 +43,4 @@ typedef struct { #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) From bd96c74a7b27a8171df6cf6ec66994559a4328ca Mon Sep 17 00:00:00 2001 From: sam blenny <68084116+samblenny@users.noreply.github.com> Date: Fri, 5 Sep 2025 03:27:02 +0000 Subject: [PATCH 8/8] tune TinyUSB fail wait delay to 15ms Previously, I used a 50ms delay after hitting a TinyUSB tuh_* API call failure result to have a big safety margin. But, that seems long enough that it might mess up animation loops. So, I tried many builds with delays between 0ms and 20ms, testing unplug detection with my big pile of USB devices. A 15ms delay worked fine with everything I tried. My Full Speed devices were marginal down to 13ms, but they stopped working at 12ms. Some of my Low Speed devices were okay down to 3ms, but the Adafruit generic SNES gamepad was marginal below 12ms. --- shared-module/usb/core/Device.c | 15 +++++++++++---- shared-module/usb/core/Device.h | 7 +++++++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/shared-module/usb/core/Device.c b/shared-module/usb/core/Device.c index 3811d678b8b8e..b02c100e187ec 100644 --- a/shared-module/usb/core/Device.c +++ b/shared-module/usb/core/Device.c @@ -69,10 +69,17 @@ void common_hal_usb_core_device_deinit(usb_core_device_obj_t *self) { } static void _wait_then_raise_USBError(int errno) { - // 1. Spin for 50 ms in a background task loop because some USB glitches - // will magically clear up if you just wait for a bit - // 2. Raise the exception so the calling code knows it needs to try again - const uint32_t end = supervisor_ticks_ms32() + 50; + // 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; } diff --git a/shared-module/usb/core/Device.h b/shared-module/usb/core/Device.h index 22729cb3359ed..9e2f4deaa2fa5 100644 --- a/shared-module/usb/core/Device.h +++ b/shared-module/usb/core/Device.h @@ -19,6 +19,13 @@ typedef struct { } 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,