Skip to content

usb.core: add raise_on_timeout=False option to Device.read #11137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lynt-smitka wants to merge 1 commit into
base: main
Choose a base branch
from
Open

usb.core: add raise_on_timeout=False option to Device.read #11137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions shared-bindings/usb/core/Device.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,23 +226,35 @@ MP_DEFINE_CONST_FUN_OBJ_KW(usb_core_device_write_obj, 2, usb_core_device_write);


//| def read(
//| self, endpoint: int, size_or_buffer: array.array, timeout: Optional[int] = None
//| self,
//| endpoint: int,
//| size_or_buffer: array.array,
//| timeout: Optional[int] = None,
//| *,
//| raise_on_timeout: bool = True,
//| ) -> int:
//| """Read data from the endpoint.
//|
//| :param int endpoint: the bEndpointAddress you want to communicate with.
//| :param array.array size_or_buffer: the array to read data into. PyUSB also allows size but CircuitPython only support array to force deliberate memory use.
//| :param int timeout: Time to wait specified in milliseconds. (Different from most CircuitPython!)
//| :param bool raise_on_timeout: when False, an elapsed timeout returns ``0``
//| instead of raising `usb.core.USBTimeoutError`. Polling an interrupt
//| endpoint every frame (e.g. a HID gamepad in a game loop) treats "no
//| new report yet" as a normal outcome; the exception object each such
//| poll would otherwise allocate is measurable garbage-collector
//| pressure on small heaps.
//| :returns: the number of bytes read
//| """
//| ...
//|
static mp_obj_t usb_core_device_read(size_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {
enum { ARG_endpoint, ARG_size_or_buffer, ARG_timeout };
enum { ARG_endpoint, ARG_size_or_buffer, ARG_timeout, ARG_raise_on_timeout };
static const mp_arg_t allowed_args[] = {
{ MP_QSTR_endpoint, MP_ARG_REQUIRED | MP_ARG_INT },
{ MP_QSTR_size_or_buffer, MP_ARG_REQUIRED | MP_ARG_OBJ },
{ MP_QSTR_timeout, MP_ARG_INT, {.u_int = 0} },
{ MP_QSTR_raise_on_timeout, MP_ARG_KW_ONLY | MP_ARG_BOOL, {.u_bool = true} },
};
usb_core_device_obj_t *self = MP_OBJ_TO_PTR(pos_args[0]);
check_for_deinit(self);
Expand All @@ -252,7 +264,7 @@ static mp_obj_t usb_core_device_read(size_t n_args, const mp_obj_t *pos_args, mp
mp_buffer_info_t bufinfo;
mp_get_buffer_raise(args[ARG_size_or_buffer].u_obj, &bufinfo, MP_BUFFER_WRITE);

return MP_OBJ_NEW_SMALL_INT(common_hal_usb_core_device_read(self, args[ARG_endpoint].u_int, bufinfo.buf, bufinfo.len, args[ARG_timeout].u_int));
return MP_OBJ_NEW_SMALL_INT(common_hal_usb_core_device_read(self, args[ARG_endpoint].u_int, bufinfo.buf, bufinfo.len, args[ARG_timeout].u_int, args[ARG_raise_on_timeout].u_bool));
}
MP_DEFINE_CONST_FUN_OBJ_KW(usb_core_device_read_obj, 2, usb_core_device_read);

Expand Down
2 changes: 1 addition & 1 deletion shared-bindings/usb/core/Device.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ mp_int_t common_hal_usb_core_device_get_speed(usb_core_device_obj_t *self);

void common_hal_usb_core_device_set_configuration(usb_core_device_obj_t *self, mp_int_t configuration);
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);
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);
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, bool raise_on_timeout);
mp_int_t common_hal_usb_core_device_ctrl_transfer(usb_core_device_obj_t *self,
mp_int_t bmRequestType, mp_int_t bRequest,
mp_int_t wValue, mp_int_t wIndex,
Expand Down
31 changes: 22 additions & 9 deletions shared-module/usb/core/Device.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ static void _abort_transfer(tuh_xfer_t *xfer) {
}

// Only frees the transfer buffer on error.
static size_t _handle_timed_transfer_callback(tuh_xfer_t *xfer, mp_int_t timeout, bool our_buffer) {
static size_t _handle_timed_transfer_callback(tuh_xfer_t *xfer, mp_int_t timeout, bool our_buffer, bool raise_on_timeout) {
if (xfer == NULL) {
mp_raise_usb_core_USBError(NULL);
return 0;
Expand All @@ -196,7 +196,14 @@ static size_t _handle_timed_transfer_callback(tuh_xfer_t *xfer, mp_int_t timeout
// Handle transfer result code from TinyUSB
xfer_result_t result = _xfer_result;
_xfer_result = XFER_RESULT_INVALID;
if (our_buffer && result != XFER_RESULT_SUCCESS && result != XFER_RESULT_INVALID) {
// Free the bounce buffer only on paths that raise: raising unwinds past
// the caller's cleanup, so it must be freed here. Paths that return
// normally leave ownership with the caller, which copies out of and
// frees the buffer itself (freeing on both sides would be a double free).
bool will_raise = result == XFER_RESULT_ABORTED || result == XFER_RESULT_FAILED ||
result == XFER_RESULT_STALLED ||
(result == XFER_RESULT_TIMEOUT && raise_on_timeout);
if (our_buffer && will_raise) {
port_free(xfer->buffer);
}
switch (result) {
Expand All @@ -212,12 +219,18 @@ static size_t _handle_timed_transfer_callback(tuh_xfer_t *xfer, mp_int_t timeout
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)
if (!raise_on_timeout) {
return 0;
}
mp_raise_usb_core_USBTimeoutError();
break;
case XFER_RESULT_INVALID:
// This timeout comes from CircuitPython, not TinyUSB, so tell TinyUSB
// to stop the transfer and then wait to free the buffer.
// to stop the transfer.
_abort_transfer(xfer);
if (!raise_on_timeout) {
return 0; // the caller owns (and frees) the buffer
}
if (our_buffer) {
port_free(xfer->buffer);
}
Expand Down Expand Up @@ -385,7 +398,7 @@ void common_hal_usb_core_device_set_configuration(usb_core_device_obj_t *self, m
}

// Raises an exception on failure. Returns the number of bytes transferred (maybe zero) on success.
static size_t _xfer(tuh_xfer_t *xfer, mp_int_t timeout, bool our_buffer) {
static size_t _xfer(tuh_xfer_t *xfer, mp_int_t timeout, bool our_buffer, bool raise_on_timeout) {
_prepare_for_transfer();
xfer->complete_cb = _transfer_done_cb;
if (!tuh_edpt_xfer(xfer)) {
Expand All @@ -395,7 +408,7 @@ static size_t _xfer(tuh_xfer_t *xfer, mp_int_t timeout, bool our_buffer) {
mp_raise_usb_core_USBError(NULL);
return 0;
}
return _handle_timed_transfer_callback(xfer, timeout, our_buffer);
return _handle_timed_transfer_callback(xfer, timeout, our_buffer, raise_on_timeout);
}

static bool _open_endpoint(usb_core_device_obj_t *self, mp_int_t endpoint) {
Expand Down Expand Up @@ -469,7 +482,7 @@ mp_int_t common_hal_usb_core_device_write(usb_core_device_obj_t *self, mp_int_t
xfer.ep_addr = endpoint;
xfer.buffer = dma_buffer;
xfer.buflen = len;
size_t result = _xfer(&xfer, timeout, dma_buffer != buffer);
size_t result = _xfer(&xfer, timeout, dma_buffer != buffer, true);
#if !CIRCUITPY_ALL_MEMORY_DMA_CAPABLE
if (dma_buffer != buffer) {
port_free(dma_buffer);
Expand All @@ -478,7 +491,7 @@ mp_int_t common_hal_usb_core_device_write(usb_core_device_obj_t *self, mp_int_t
return result;
}

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) {
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, bool raise_on_timeout) {
if (!_open_endpoint(self, endpoint)) {
mp_raise_usb_core_USBError(NULL);
return 0;
Expand All @@ -500,7 +513,7 @@ mp_int_t common_hal_usb_core_device_read(usb_core_device_obj_t *self, mp_int_t e
xfer.ep_addr = endpoint;
xfer.buffer = dma_buffer;
xfer.buflen = len;
mp_int_t result = _xfer(&xfer, timeout, dma_buffer != buffer);
mp_int_t result = _xfer(&xfer, timeout, dma_buffer != buffer, raise_on_timeout);

#if !CIRCUITPY_ALL_MEMORY_DMA_CAPABLE
// Copy data back to original buffer if needed
Expand Down Expand Up @@ -556,7 +569,7 @@ mp_int_t common_hal_usb_core_device_ctrl_transfer(usb_core_device_obj_t *self,
mp_raise_usb_core_USBError(NULL);
return 0;
}
mp_int_t result = (mp_int_t)_handle_timed_transfer_callback(&xfer, timeout, dma_buffer != buffer);
mp_int_t result = (mp_int_t)_handle_timed_transfer_callback(&xfer, timeout, dma_buffer != buffer, true);

#if !CIRCUITPY_ALL_MEMORY_DMA_CAPABLE
if (dma_buffer != buffer) {
Expand Down
Loading