UART sensors cleanup

CHANGELOG.md

@@ -20,11 +20,13 @@
 - Fixed Powered Up Light not working ([support#1131]) on all hubs.
 - Fixed UART sensors not working on Technic Hub ([support#1137]).
 - Fixed incorrect number of ports on City Hub ([support#1131]).
+- Improved external device detection speed ([support#1140]).
 
 [support#1054]: https://github.com/pybricks/support/issues/1054
 [support#1105]: https://github.com/pybricks/support/issues/1105
 [support#1131]: https://github.com/pybricks/support/issues/1131
 [support#1137]: https://github.com/pybricks/support/issues/1137
+[support#1140]: https://github.com/pybricks/support/issues/1140
 
 ## [3.3.0b7] - 2023-06-30
 

lib/pbio/drv/core.c

@@ -37,6 +37,7 @@
 #include "random/random.h"
 #include "reset/reset.h"
 #include "sound/sound.h"
+#include "uart/uart.h"
 #include "usb/usb.h"
 #include "watchdog/watchdog.h"
 
@@ -87,6 +88,7 @@ void pbdrv_init(void) {
     pbdrv_random_init();
     pbdrv_reset_init();
     pbdrv_sound_init();
+    pbdrv_uart_init();
     pbdrv_usb_init();
     pbdrv_watchdog_init();
 

lib/pbio/drv/ioport/ioport_pup.c

@@ -5,9 +5,10 @@
 
 #if PBDRV_CONFIG_IOPORT_PUP
 
-#include <pbdrv/ioport.h>
+#include <contiki.h>
 
 #include "ioport_pup.h"
+#include "../core.h"
 
 static void init_one_port(const pbdrv_ioport_pup_pins_t *pins) {
     // Normally should be set already, but could have been changed by bootloader.
@@ -26,14 +27,21 @@ void pbdrv_ioport_enable_vcc(bool enable) {
     }
 }
 
+#if PBDRV_CONFIG_IOPORT_PUP_QUIRK_POWER_CYCLE
+// This is generic ioport process. It is currently only used for the power
+// cycle quirk, so it is protected by guards to save space elsewhere.
+PROCESS(pbdrv_ioport_pup_process, "ioport_pup");
+#endif
+
 void pbdrv_ioport_init(void) {
     for (uint8_t i = 0; i < PBDRV_CONFIG_IOPORT_NUM_DEV; i++) {
         init_one_port(&pbdrv_ioport_pup_platform_data.ports[i].pins);
     }
 
-    // Begin with vcc disabled. The relevant ioport mode will turn this on
-    // when needed.
-    pbdrv_ioport_enable_vcc(false);
+    #if PBDRV_CONFIG_IOPORT_PUP_QUIRK_POWER_CYCLE
+    pbdrv_init_busy_up();
+    process_start(&pbdrv_ioport_pup_process);
+    #endif
 }
 
 void pbdrv_ioport_deinit(void) {
@@ -45,4 +53,26 @@ void pbdrv_ioport_deinit(void) {
     pbdrv_gpio_input(&pbdrv_ioport_pup_platform_data.port_vcc);
 }
 
+#if PBDRV_CONFIG_IOPORT_PUP_QUIRK_POWER_CYCLE
+PROCESS_THREAD(pbdrv_ioport_pup_process, ev, data) {
+
+    static struct etimer timer;
+
+    PROCESS_BEGIN();
+
+    // Some hubs turn on power to the I/O ports in the bootloader. This causes
+    // UART sync delays after boot. This process turns them off to make sure
+    // power can be enabled at the right time by the legodev driver instead.
+    pbdrv_ioport_enable_vcc(false);
+
+    etimer_set(&timer, 500);
+    PROCESS_WAIT_EVENT_UNTIL(ev == PROCESS_EVENT_TIMER && etimer_expired(&timer));
+
+    pbdrv_init_busy_down();
+
+    PROCESS_END();
+}
+
+#endif // PBDRV_CONFIG_IOPORT_PUP_QUIRK_POWER_CYCLE
+
 #endif // PBDRV_CONFIG_IOPORT_PUP

lib/pbio/drv/legodev/legodev_pup.c

@@ -24,8 +24,8 @@
 #include <pbio/port.h>
 
 #include <pbdrv/counter.h>
-#include <pbdrv/legodev.h>
 #include <pbdrv/ioport.h>
+#include <pbdrv/legodev.h>
 #include "../ioport/ioport_pup.h"
 
 #include "legodev_pup.h"
@@ -35,10 +35,6 @@
 /** The number of consecutive repeated detections needed for an affirmative ID. */
 #define AFFIRMATIVE_MATCH_COUNT 20
 
-/** If the UART device did not call back to this driver after this time, assume
-    it is disconnected and restart device detection. */
-#define UART_WATCHDOG_TIMEOUT 1500
-
 typedef enum {
     DEV_ID_GROUP_GND,
     DEV_ID_GROUP_VCC,
@@ -48,23 +44,37 @@ typedef enum {
 
 // Device connection manager state for each port
 typedef struct {
-    dev_id_group_t dev_id1_group;
+    /** Most recent one-off device ID candidate. */
     pbdrv_legodev_type_id_t type_id;
+    /** Previous one-off device ID candidate. */
     pbdrv_legodev_type_id_t prev_type_id;
+    /** Most recent device ID with enough consecutive detections. */
+    pbdrv_legodev_type_id_t connected_type_id;
+    /** Previous device ID with enough consecutive detections. */
+    pbdrv_legodev_type_id_t prev_connected_type_id;
+    /** Number of consecutive detections of the same device ID. */
+    uint8_t dev_id_match_count;
+    // Intermediate state values to preserve in between async calls.
+    dev_id_group_t dev_id1_group;
     uint8_t gpio_value;
     uint8_t prev_gpio_value;
-    uint8_t dev_id_match_count;
-    struct timer watchdog;
 } dcm_data_t;
 
 typedef struct {
+    /**
+     * Main protothread for the legodev. Each external legodev port has one,
+     * all driven by a single process that runs until the hub shuts down.
+     */
+    struct pt pt;
+    /**
+     * Child protothread used for the device connection manager and the uart
+     * device process. These don't run at the same time, so we can reuse it. */
+    struct pt child;
     const pbdrv_ioport_pup_pins_t *pins;
     const pbdrv_legodev_pup_ext_platform_data_t *pdata;
     pbdrv_legodev_pup_uart_dev_t *uart_dev;
     dcm_data_t dcm;
-    struct pt pt;
-    pbdrv_legodev_type_id_t connected_type_id;
-    pbdrv_legodev_type_id_t prev_type_id;
+    struct etimer timer;
     pbio_angle_t angle;
 } ext_dev_t;
 
@@ -88,7 +98,7 @@ static const pbdrv_legodev_type_id_t legodev_pup_type_id_lookup[3][3] = {
     },
 };
 
-PROCESS(pbio_legodev_pup_process, "I/O port");
+PROCESS(pbio_legodev_pup_process, "legodev_pup");
 
 static void legodev_pup_enable_uart(const pbdrv_ioport_pup_pins_t *pins) {
     // REVISIT: Move to ioport.
@@ -100,8 +110,8 @@ static void legodev_pup_enable_uart(const pbdrv_ioport_pup_pins_t *pins) {
 // This is the device connection manager (dcm). It monitors the ID1 and ID2 pins
 // on the port to see when devices are connected or disconnected.
 // It is expected for there to be a 2ms delay between calls to this function.
-static PT_THREAD(poll_dcm(ext_dev_t * dev)) {
-    struct pt *pt = &dev->pt;
+PT_THREAD(poll_dcm(ext_dev_t * dev)) {
+    struct pt *pt = &dev->child;
     dcm_data_t *data = &dev->dcm;
     const pbdrv_ioport_pup_pins_t *pins = dev->pins;
 
@@ -282,7 +292,7 @@ static PT_THREAD(poll_dcm(ext_dev_t * dev)) {
         }
 
         if (data->dev_id_match_count >= AFFIRMATIVE_MATCH_COUNT) {
-            dev->connected_type_id = data->type_id;
+            dev->dcm.connected_type_id = data->type_id;
         }
     } else {
         data->dev_id_match_count = 0;
@@ -301,6 +311,12 @@ static void debug_state_change(ext_dev_t *dev) {
             break;
         }
     }
+
+    // Nothing to do if it's the same device as before.
+    if (dev->connected_type_id == dev->prev_connected_type_id) {
+        return;
+    }
+
     printf("Port %c: ", i + 'A' + PBDRV_CONFIG_LEGODEV_PUP_NUM_INT_DEV);
     if (dev->connected_type_id == PBDRV_LEGODEV_TYPE_ID_LPF2_UNKNOWN_UART) {
         printf("UART device detected.\n");
@@ -314,62 +330,51 @@ static void debug_state_change(ext_dev_t *dev) {
 #define debug_state_change(...)
 #endif
 
-PROCESS_THREAD(pbio_legodev_pup_process, ev, data) {
-    static struct etimer timer;
-
-    static ext_dev_t *dev;
+static PT_THREAD(pbdrv_legodev_pup_thread(ext_dev_t * dev)) {
 
-    PROCESS_BEGIN();
+    PT_BEGIN(&dev->pt);
 
-    // Some hubs turn on power to the I/O ports in the bootloader. This causes
-    // UART sync delays after boot. It is faster to power cycle the I/O devices
-    // than it is to wait for long sync in most cases.
-    pbdrv_ioport_enable_vcc(false);
-    etimer_set(&timer, 500);
-    PROCESS_WAIT_EVENT_UNTIL(ev == PROCESS_EVENT_TIMER && etimer_expired(&timer));
+    // Turn on power to the port. This is either known to be off since boot,
+    // or explicitly turned off by the ioport initialization process. Turning
+    // them on now means that UART sensors will begin sending data only now
+    // that we are ready to receive it, so we don't miss the first data.
     pbdrv_ioport_enable_vcc(true);
 
-    etimer_set(&timer, 2);
-
-    while (!pbsys_status_test(PBIO_PYBRICKS_STATUS_SHUTDOWN)) {
-        PROCESS_WAIT_EVENT();
-        if (ev == PROCESS_EVENT_TIMER && etimer_expired(&timer)) {
-            etimer_reset(&timer);
-
-            for (int i = 0; i < PBDRV_CONFIG_LEGODEV_PUP_NUM_EXT_DEV; i++) {
-                dev = &ext_devs[i];
-
-                // If UART device was connected but not reporting data, restart detection.
-                if (dev->connected_type_id == PBDRV_LEGODEV_TYPE_ID_LPF2_UNKNOWN_UART &&
-                    dev->connected_type_id == dev->prev_type_id && timer_expired(&dev->dcm.watchdog)) {
-                    dev->connected_type_id = PBDRV_LEGODEV_TYPE_ID_NONE;
-                    dev->dcm.dev_id_match_count = 0;
-                }
-
-                // Keep running device detection unless UART sensor is attached.
-                if (dev->connected_type_id != PBDRV_LEGODEV_TYPE_ID_LPF2_UNKNOWN_UART) {
-                    poll_dcm(dev);
-                }
-
-                // Nothing more to do if it's the same device as before.
-                if (dev->connected_type_id == dev->prev_type_id) {
-                    continue;
-                }
-
+    while (true) {
+
+        // Initially assume nothing is connected.
+        dev->dcm.connected_type_id = PBDRV_LEGODEV_TYPE_ID_NONE;
+        dev->dcm.dev_id_match_count = 0;
+
+        // Run the device detection manager until a UART device is detected.
+        // This process continously checks which type of passive device is
+        // attached by setting and reading the ID1 and ID2 pins in a predefined
+        // sequence at 2ms intervals. Once a UART device is detected, this
+        // detection process stops because it would interfere with the UART
+        // data. Once the UART process completes (i.e. the device disconnects),
+        // device detection will resume here.
+        etimer_set(&dev->timer, 2);
+        PT_INIT(&dev->child);
+        PT_WAIT_UNTIL(&dev->pt, ({
+            if (etimer_expired(&dev->timer)) {
+                etimer_reset(&dev->timer);
+                (void)PT_SCHEDULE(poll_dcm(dev));
                 debug_state_change(dev);
-
-                // New device. Enable UART if needed for this device.
-                if (dev->connected_type_id == PBDRV_LEGODEV_TYPE_ID_LPF2_UNKNOWN_UART) {
-                    legodev_pup_enable_uart(dev->pins);
-
-                    pbdrv_legodev_pup_uart_start_sync(dev->uart_dev);
-                }
-                dev->prev_type_id = dev->connected_type_id;
+                dev->dcm.prev_connected_type_id = dev->dcm.connected_type_id;
             }
-        }
+            dev->dcm.connected_type_id == PBDRV_LEGODEV_TYPE_ID_LPF2_UNKNOWN_UART;
+        }));
+
+        // UART device detected, so hand off control to that protocol until it
+        // disconnects, as observed by the UART process not getting valid data.
+        legodev_pup_enable_uart(dev->pins);
+        PT_SPAWN(&dev->pt, &dev->child, pbdrv_legodev_pup_uart_thread(&dev->child, dev->uart_dev));
     }
+    PT_END(&dev->pt);
+}
 
-    PROCESS_END();
+void pbdrv_legodev_pup_uart_process_poll(void) {
+    process_poll(&pbio_legodev_pup_process);
 }
 
 struct _pbdrv_legodev_dev_t {
@@ -384,6 +389,22 @@ struct _pbdrv_legodev_dev_t {
 
 static pbdrv_legodev_dev_t devs[PBDRV_CONFIG_LEGODEV_PUP_NUM_INT_DEV + PBDRV_CONFIG_LEGODEV_PUP_NUM_EXT_DEV];
 
+PROCESS_THREAD(pbio_legodev_pup_process, ev, data) {
+
+    static int i;
+
+    PROCESS_BEGIN();
+
+    while (!pbsys_status_test(PBIO_PYBRICKS_STATUS_SHUTDOWN)) {
+        for (i = 0; i < PBDRV_CONFIG_LEGODEV_PUP_NUM_EXT_DEV; i++) {
+            (void)PT_SCHEDULE(pbdrv_legodev_pup_thread(&ext_devs[i]));
+        }
+        PROCESS_WAIT_EVENT();
+    }
+
+    PROCESS_END();
+}
+
 void pbdrv_legodev_init(void) {
     #if PBDRV_CONFIG_LEGODEV_PUP_NUM_INT_DEV > 0
     for (uint8_t i = 0; i < PBDRV_CONFIG_LEGODEV_PUP_NUM_INT_DEV; i++) {
@@ -404,7 +425,6 @@ void pbdrv_legodev_init(void) {
         const pbdrv_ioport_pup_port_platform_data_t *port_data = &pbdrv_ioport_pup_platform_data.ports[legodev_data->ioport_index];
         legodev->ext_dev->pdata = legodev_data;
         legodev->ext_dev->pins = &port_data->pins;
-        timer_set(&legodev->ext_dev->dcm.watchdog, UART_WATCHDOG_TIMEOUT);
 
         // Initialize uart device manager.
         pbio_dcmotor_t *dcmotor;
@@ -413,36 +433,11 @@ void pbdrv_legodev_init(void) {
 
     }
     process_start(&pbio_legodev_pup_process);
-    pbdrv_legodev_pup_uart_process_start();
-}
-
-static pbdrv_legodev_dev_t *pbdrv_legodev_get_parent_of(pbdrv_legodev_pup_uart_dev_t *uartdev) {
-    for (uint8_t i = 0; i < PBIO_ARRAY_SIZE(devs); i++) {
-        pbdrv_legodev_dev_t *dev = &devs[i];
-        if (!dev->is_internal && dev->ext_dev->uart_dev == uartdev) {
-            return dev;
-        }
-    }
-    return NULL;
-}
-
-/**
- * Resets device connection watchdog timer. This allows the detection manager
- * to resume when a UART device stopped working, usually by being unplugged.
- *
- * @param [in]  dev           legodev device.
- */
-void pbdrv_legodev_pup_reset_watchdog(pbdrv_legodev_pup_uart_dev_t *uartdev) {
-    pbdrv_legodev_dev_t *dev = pbdrv_legodev_get_parent_of(uartdev);
-    if (dev == NULL) {
-        return;
-    }
-    timer_restart(&dev->ext_dev->dcm.watchdog);
 }
 
 pbdrv_legodev_pup_uart_dev_t *pbdrv_legodev_get_uart_dev(pbdrv_legodev_dev_t *legodev) {
     // Device is not a uart device.
-    if (legodev->is_internal || legodev->ext_dev->connected_type_id != PBDRV_LEGODEV_TYPE_ID_LPF2_UNKNOWN_UART) {
+    if (legodev->is_internal || legodev->ext_dev->dcm.connected_type_id != PBDRV_LEGODEV_TYPE_ID_LPF2_UNKNOWN_UART) {
         return NULL;
     }
     return legodev->ext_dev->uart_dev;
@@ -585,16 +580,16 @@ pbio_error_t pbdrv_legodev_get_device(pbio_port_id_t port_id, pbdrv_legodev_type
         }
 
         // Device check is ready, now test if something is attached.
-        if (candidate->ext_dev->connected_type_id == PBDRV_LEGODEV_TYPE_ID_NONE) {
+        if (candidate->ext_dev->dcm.connected_type_id == PBDRV_LEGODEV_TYPE_ID_NONE) {
             return PBIO_ERROR_NO_DEV;
         }
 
         // Now we can still have a passive or active device.
         *legodev = candidate;
 
         // Passive devices are always ready.
-        if (candidate->ext_dev->connected_type_id != PBDRV_LEGODEV_TYPE_ID_LPF2_UNKNOWN_UART) {
-            return type_id_matches(type_id, candidate->ext_dev->connected_type_id) ? PBIO_SUCCESS : PBIO_ERROR_NO_DEV;
+        if (candidate->ext_dev->dcm.connected_type_id != PBDRV_LEGODEV_TYPE_ID_LPF2_UNKNOWN_UART) {
+            return type_id_matches(type_id, candidate->ext_dev->dcm.connected_type_id) ? PBIO_SUCCESS : PBIO_ERROR_NO_DEV;
         }
 
         // Get device information, and check if device is ready.
@@ -616,7 +611,7 @@ bool pbdrv_legodev_needs_permanent_power(pbdrv_legodev_dev_t *legodev) {
     }
 
     // Known passive devices don't need permanent power.
-    if (legodev->ext_dev->connected_type_id != PBDRV_LEGODEV_TYPE_ID_LPF2_UNKNOWN_UART) {
+    if (legodev->ext_dev->dcm.connected_type_id != PBDRV_LEGODEV_TYPE_ID_LPF2_UNKNOWN_UART) {
         return false;
     }