Improve usb.core.Device error handling

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));
 }
 
 

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.

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) {

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)