add timeout; finish up for PR

Author: dhalbert

py/stream.c

@@ -99,14 +99,22 @@ const mp_stream_p_t *mp_get_stream_raise(mp_obj_t self_in, int flags) {
 
 STATIC mp_obj_t stream_read_generic(size_t n_args, const mp_obj_t *args, byte flags) {
     // What to do if sz < -1?  Python docs don't specify this case.
-    // CPython does a readall, but here we silently let negatives through,
-    // and they will cause a MemoryError.
+    // CPython does a readall, let's do the same.
     mp_int_t sz;
-    if (n_args == 1 || args[1] == mp_const_none || ((sz = mp_obj_get_int(args[1])) == -1)) {
-        return stream_readall(args[0]);
-    }
-
     const mp_stream_p_t *stream_p = mp_get_stream(args[0]);
+    if (stream_p->pyserial_read_compatibility) {
+        // Pyserial defaults to sz=1 if not specified.
+        if (n_args == 1) {
+            sz = 1;
+        } else {
+            // Pyserial treats negative size as 0.
+            sz = MAX(0, mp_obj_get_int(args[1]));
+        }
+    } else {
+        if (n_args == 1 || args[1] == mp_const_none || (sz = mp_obj_get_int(args[1])) <= -1) {
+            return stream_readall(args[0]);
+        }
+    }
 
     #if MICROPY_PY_BUILTINS_STR_UNICODE
     if (stream_p->is_text) {
@@ -284,7 +292,7 @@ STATIC mp_obj_t stream_readinto(size_t n_args, const mp_obj_t *args) {
     // https://docs.python.org/3/library/socket.html#socket.socket.recv_into
     mp_uint_t len = bufinfo.len;
     if (n_args > 2) {
-        if (mp_get_stream(args[0])->pyserial_compatibility) {
+        if (mp_get_stream(args[0])->pyserial_readinto_compatibility) {
             mp_raise_ValueError(translate("length argument not allowed for this type"));
         }
         len = mp_obj_get_int(args[2]);
@@ -297,7 +305,10 @@ STATIC mp_obj_t stream_readinto(size_t n_args, const mp_obj_t *args) {
     mp_uint_t out_sz = mp_stream_read_exactly(args[0], bufinfo.buf, len, &error);
     if (error != 0) {
         if (mp_is_nonblocking_error(error)) {
-            return mp_const_none;
+            // pyserial readinto never returns None, just 0.
+            return mp_get_stream(args[0])->pyserial_dont_return_none_compatibility
+                ? MP_OBJ_NEW_SMALL_INT(0)
+                : mp_const_none;
         }
         mp_raise_OSError(error);
     } else {
@@ -323,7 +334,10 @@ STATIC mp_obj_t stream_readall(mp_obj_t self_in) {
                 // If we read nothing, return None, just like read().
                 // Otherwise, return data read so far.
                 if (total_size == 0) {
-                    return mp_const_none;
+                    // pyserial read() never returns None, just b''.
+                    return stream_p->pyserial_dont_return_none_compatibility
+                        ? mp_const_empty_bytes
+                        : mp_const_none;
                 }
                 break;
             }

py/stream.h

@@ -72,7 +72,9 @@ typedef struct _mp_stream_p_t {
     mp_uint_t (*write)(mp_obj_t obj, const void *buf, mp_uint_t size, int *errcode);
     mp_uint_t (*ioctl)(mp_obj_t obj, mp_uint_t request, uintptr_t arg, int *errcode);
     mp_uint_t is_text : 1; // default is bytes, set this for text stream
-    bool pyserial_compatibility: 1;  // adjust API to match pyserial more closely
+    bool pyserial_readinto_compatibility: 1;         // Disallow size parameter in readinto()
+    bool pyserial_read_compatibility: 1;             // Disallow omitting read(size) size parameter
+    bool pyserial_dont_return_none_compatibility: 1; // Don't return None for read() or readinto()
 } mp_stream_p_t;
 
 MP_DECLARE_CONST_FUN_OBJ_VAR_BETWEEN(mp_stream_read_obj);

shared-bindings/_bleio/CharacteristicBuffer.c

@@ -231,8 +231,8 @@ STATIC const mp_stream_p_t characteristic_buffer_stream_p = {
     .write = bleio_characteristic_buffer_write,
     .ioctl = bleio_characteristic_buffer_ioctl,
     .is_text = false,
-    // Match PySerial when possible, such as disallowing optional length argument for .readinto()
-    .pyserial_compatibility = true,
+    // Disallow readinto() size parameter.
+    .pyserial_readinto_compatibility = true,
 };
 
 

shared-bindings/busio/UART.c

@@ -415,8 +415,8 @@ STATIC const mp_stream_p_t uart_stream_p = {
     .write = busio_uart_write,
     .ioctl = busio_uart_ioctl,
     .is_text = false,
-    // Match PySerial when possible, such as disallowing optional length argument for .readinto()
-    .pyserial_compatibility = true,
+    // Disallow optional length argument for .readinto()
+    .pyserial_readinto_compatibility = true,
 };
 
 const mp_obj_type_t busio_uart_type = {

shared-bindings/usb_cdc/Serial.c

@@ -41,36 +41,34 @@
 //|     def __init__(self) -> None:
 //|         """You cannot create an instance of `usb_cdc.Serial`.
 //|
-//|         Serial objects are constructed for every corresponding entry in the USB
+//|         Serial objects are pre-constructed for each CDC device in the USB
 //|         descriptor and added to the ``usb_cdc.ports`` tuple."""
 //|         ...
 //|
 
-// These are standard stream methods. Code is in py/stream.c.
-//
-//|     def read(self, nbytes: Optional[int] = None) -> Optional[bytes]:
-//|         """Read characters.  If ``nbytes`` is specified then read at most that many
-//|         bytes. Otherwise, read everything that arrives until the connection
-//|         times out. Providing the number of bytes expected is highly recommended
-//|         because it will be faster.
+//|     def read(self, size: int = 1) -> bytes:
+//|         """Read at most ``size`` bytes. If ``size`` exceeds the internal buffer size
+//|         only the bytes in the buffer will be read. If `timeout` is > 0 or ``None``,
+//|         and fewer than ``size`` bytes are available, keep waiting until the timeout
+//|         expires or ``size`` bytes are available.
 //|
 //|         :return: Data read
-//|         :rtype: bytes or None"""
+//|         :rtype: bytes"""
 //|         ...
 //|
-//|     def readinto(self, buf: WriteableBuffer, nbytes: Optional[int] = None) -> Optional[bytes]:
+//|     def readinto(self, buf: WriteableBuffer) -> bytes:
 //|         """Read bytes into the ``buf``.  If ``nbytes`` is specified then read at most
-//|         that many bytes.  Otherwise, read at most ``len(buf)`` bytes.
+//|         that many bytes, subject to `timeout`.  Otherwise, read at most ``len(buf)`` bytes.
 //|
 //|         :return: number of bytes read and stored into ``buf``
-//|         :rtype: bytes or None"""
+//|         :rtype: bytes"""
 //|         ...
 //|
-//|     def write(self, buf: ReadableBuffer) -> Optional[int]:
+//|     def write(self, buf: ReadableBuffer) -> int:
 //|         """Write as many bytes as possible from the buffer of bytes.
 //|
 //|         :return: the number of bytes written
-//|         :rtype: int or None"""
+//|         :rtype: int"""
 //|         ...
 //|
 //|     def flush(self) -> None:
@@ -79,7 +77,7 @@
 //|
 
 // These three methods are used by the shared stream methods.
-STATIC mp_uint_t usb_cdc_serial_read(mp_obj_t self_in, void *buf_in, mp_uint_t size, int *errcode) {
+STATIC mp_uint_t usb_cdc_serial_read_stream(mp_obj_t self_in, void *buf_in, mp_uint_t size, int *errcode) {
     usb_cdc_serial_obj_t *self = MP_OBJ_TO_PTR(self_in);
     byte *buf = buf_in;
 
@@ -91,16 +89,16 @@ STATIC mp_uint_t usb_cdc_serial_read(mp_obj_t self_in, void *buf_in, mp_uint_t s
     return common_hal_usb_cdc_serial_read(self, buf, size, errcode);
 }
 
-STATIC mp_uint_t usb_cdc_serial_write(mp_obj_t self_in, const void *buf_in, mp_uint_t size, int *errcode) {
+STATIC mp_uint_t usb_cdc_serial_write_stream(mp_obj_t self_in, const void *buf_in, mp_uint_t size, int *errcode) {
     usb_cdc_serial_obj_t *self = MP_OBJ_TO_PTR(self_in);
     const byte *buf = buf_in;
 
     return common_hal_usb_cdc_serial_write(self, buf, size, errcode);
 }
 
-STATIC mp_uint_t usb_cdc_serial_ioctl(mp_obj_t self_in, mp_uint_t request, mp_uint_t arg, int *errcode) {
+STATIC mp_uint_t usb_cdc_serial_ioctl_stream(mp_obj_t self_in, mp_uint_t request, mp_uint_t arg, int *errcode) {
     usb_cdc_serial_obj_t *self = MP_OBJ_TO_PTR(self_in);
-    mp_uint_t ret;
+    mp_uint_t ret = 0;
     switch (request) {
         case MP_IOCTL_POLL: {
             mp_uint_t flags = arg;
@@ -134,7 +132,7 @@ STATIC mp_obj_t usb_cdc_serial_get_connected(mp_obj_t self_in) {
 }
 MP_DEFINE_CONST_FUN_OBJ_1(usb_cdc_serial_get_connected_obj, usb_cdc_serial_get_connected);
 
-const mp_obj_property_t usb_cdc_serial__connected_obj = {
+const mp_obj_property_t usb_cdc_serial_connected_obj = {
     .base.type = &mp_type_property,
     .proxy = {(mp_obj_t)&usb_cdc_serial_get_connected_obj,
               (mp_obj_t)&mp_const_none_obj,
@@ -195,6 +193,32 @@ STATIC mp_obj_t usb_cdc_serial_reset_output_buffer(mp_obj_t self_in) {
 }
 MP_DEFINE_CONST_FUN_OBJ_1(usb_cdc_serial_reset_output_buffer_obj, usb_cdc_serial_reset_output_buffer);
 
+//|     timeout: Optional[float]
+//|     """The initial value of `timeout` is ``None``. If ``None``, wait indefinitely to satisfy
+//|     the conditions of a read operation. If 0, do not wait. If > 0, wait only ``timeout`` seconds."""
+//|
+STATIC mp_obj_t usb_cdc_serial_get_timeout(mp_obj_t self_in) {
+    usb_cdc_serial_obj_t *self = MP_OBJ_TO_PTR(self_in);
+    mp_float_t timeout = common_hal_usb_cdc_serial_get_timeout(self);
+    return (timeout < 0.0f) ? mp_const_none : mp_obj_new_float(self->timeout);
+}
+MP_DEFINE_CONST_FUN_OBJ_1(usb_cdc_serial_get_timeout_obj, usb_cdc_serial_get_timeout);
+
+STATIC mp_obj_t usb_cdc_serial_set_timeout(mp_obj_t self_in, mp_obj_t timeout_in) {
+    usb_cdc_serial_obj_t *self = MP_OBJ_TO_PTR(self_in);
+    common_hal_usb_cdc_serial_set_timeout(self,
+        timeout_in == mp_const_none ? -1.0f : mp_obj_get_float(timeout_in));
+    return mp_const_none;
+}
+MP_DEFINE_CONST_FUN_OBJ_2(usb_cdc_serial_set_timeout_obj, usb_cdc_serial_set_timeout);
+
+const mp_obj_property_t usb_cdc_serial_timeout_obj = {
+    .base.type = &mp_type_property,
+    .proxy = {(mp_obj_t)&usb_cdc_serial_get_timeout_obj,
+              (mp_obj_t)&usb_cdc_serial_set_timeout_obj,
+              (mp_obj_t)&mp_const_none_obj},
+};
+
 
 STATIC const mp_rom_map_elem_t usb_cdc_serial_locals_dict_table[] = {
     // Standard stream methods.
@@ -210,9 +234,10 @@ STATIC const mp_rom_map_elem_t usb_cdc_serial_locals_dict_table[] = {
     { MP_OBJ_NEW_QSTR(MP_QSTR_out_waiting),         MP_ROM_PTR(&usb_cdc_serial_out_waiting_obj) },
     { MP_OBJ_NEW_QSTR(MP_QSTR_reset_input_buffer),  MP_ROM_PTR(&usb_cdc_serial_reset_input_buffer_obj) },
     { MP_OBJ_NEW_QSTR(MP_QSTR_reset_output_buffer), MP_ROM_PTR(&usb_cdc_serial_reset_output_buffer_obj) },
+    { MP_OBJ_NEW_QSTR(MP_QSTR_timeout),             MP_ROM_PTR(&usb_cdc_serial_timeout_obj) },
 
     // Not in pyserial protocol.
-    { MP_OBJ_NEW_QSTR(MP_QSTR_connected),     MP_ROM_PTR(&usb_cdc_serial_get_connected_obj) },
+    { MP_OBJ_NEW_QSTR(MP_QSTR_connected),     MP_ROM_PTR(&usb_cdc_serial_connected_obj) },
 
 
 
@@ -221,10 +246,13 @@ STATIC MP_DEFINE_CONST_DICT(usb_cdc_serial_locals_dict, usb_cdc_serial_locals_di
 
 STATIC const mp_stream_p_t usb_cdc_serial_stream_p = {
     MP_PROTO_IMPLEMENT(MP_QSTR_protocol_stream)
-    .read = usb_cdc_serial_read,
-    .write = usb_cdc_serial_write,
-    .ioctl = usb_cdc_serial_ioctl,
+    .read = usb_cdc_serial_read_stream,
+    .write = usb_cdc_serial_write_stream,
+    .ioctl = usb_cdc_serial_ioctl_stream,
     .is_text = false,
+    .pyserial_read_compatibility = true,
+    .pyserial_readinto_compatibility = true,
+    .pyserial_dont_return_none_compatibility = true,
 };
 
 const mp_obj_type_t usb_cdc_serial_type = {

shared-bindings/usb_cdc/Serial.h

@@ -44,4 +44,7 @@ extern uint32_t common_hal_usb_cdc_serial_flush(usb_cdc_serial_obj_t *self);
 
 extern bool common_hal_usb_cdc_serial_get_connected(usb_cdc_serial_obj_t *self);
 
+extern mp_float_t common_hal_usb_cdc_serial_get_timeout(usb_cdc_serial_obj_t *self);
+extern void common_hal_usb_cdc_serial_set_timeout(usb_cdc_serial_obj_t *self, mp_float_t timeout);
+
 #endif  // MICROPY_INCLUDED_SHARED_BINDINGS_USB_CDC_SERIAL_H

shared-module/usb_cdc/Serial.c

@@ -24,10 +24,32 @@
  * THE SOFTWARE.
  */
 
+#include "lib/utils/interrupt_char.h"
 #include "shared-module/usb_cdc/Serial.h"
+#include "supervisor/shared/tick.h"
+
 #include "tusb.h"
 
 size_t common_hal_usb_cdc_serial_read(usb_cdc_serial_obj_t *self, uint8_t *data, size_t len, int *errcode) {
+    if (self->timeout < 0.0f) {
+        while (tud_cdc_n_available(self->idx) < len) {
+            RUN_BACKGROUND_TASKS;
+            if (mp_hal_is_interrupted()) {
+                return 0;
+            }
+        }
+    } else if (self->timeout > 0.0f) {
+        uint64_t timeout_ms = self->timeout * 1000;
+        uint64_t start_ticks = supervisor_ticks_ms64();
+        while (tud_cdc_n_available(self->idx) < len &&
+               supervisor_ticks_ms64() - start_ticks <= timeout_ms) {
+            RUN_BACKGROUND_TASKS;
+            if (mp_hal_is_interrupted()) {
+                return 0;
+            }
+        }
+    }
+    // Timeout of 0.0f falls through to here with no waiting or unnecessary calculation.
     return tud_cdc_n_read(self->idx, data, len);
 }
 
@@ -61,3 +83,11 @@ uint32_t common_hal_usb_cdc_serial_flush(usb_cdc_serial_obj_t *self) {
 bool common_hal_usb_cdc_serial_get_connected(usb_cdc_serial_obj_t *self) {
     return tud_cdc_n_connected(self->idx);
 }
+
+mp_float_t common_hal_usb_cdc_serial_get_timeout(usb_cdc_serial_obj_t *self) {
+    return self->timeout;
+}
+
+void common_hal_usb_cdc_serial_set_timeout(usb_cdc_serial_obj_t *self, mp_float_t timeout) {
+    self->timeout = timeout;
+}

shared-module/usb_cdc/Serial.h

@@ -31,8 +31,8 @@
 
 typedef struct {
     mp_obj_base_t base;
-    // Which CDC device?
-    uint8_t idx;
+    mp_float_t timeout;    // if negative, wait forever.
+    uint8_t idx;           // which CDC device?
 } usb_cdc_serial_obj_t;
 
 #endif // SHARED_MODULE_USB_CDC_SERIAL_H

shared-module/usb_cdc/__init__.c

@@ -38,11 +38,13 @@
 #error CFG_TUD_CDC must be exactly 2
 #endif
 
-static const usb_cdc_serial_obj_t serial_objs[CFG_TUD_CDC] = {
+static usb_cdc_serial_obj_t serial_objs[CFG_TUD_CDC] = {
     {   .base.type = &usb_cdc_serial_type,
+        .timeout = -1.0f,
         .idx = 0,
     }, {
         .base.type = &usb_cdc_serial_type,
+        .timeout = -1.0f,
         .idx = 1,
     }
 };