Fix various stdio_usb issues, add stdio_init_all return code, and add alarm_pool_core_num() API (#918)

This issue addresses possible starvation issues when using `getchar()` with `stdio_usb` and also fixes possible missing of IRQs as a result of #871

src/common/pico_sync/critical_section.c

@@ -21,7 +21,5 @@ void critical_section_init_with_lock_num(critical_section_t *crit_sec, uint lock
 
 void critical_section_deinit(critical_section_t *crit_sec) {
     spin_lock_unclaim(spin_lock_get_num(crit_sec->spin_lock));
-#ifndef NDEBUG
+    crit_sec->spin_lock = NULL;
-    crit_sec->spin_lock = (spin_lock_t *)-1;
-#endif
 }

src/common/pico_sync/include/pico/critical_section.h

@@ -82,6 +82,16 @@ static inline void critical_section_exit(critical_section_t *crit_sec) {
  */
 void critical_section_deinit(critical_section_t *crit_sec);
 
+/*! \brief Test whether a critical_section has been initialized
+ *  \ingroup mutex
+ *
+ * \param crit_sec Pointer to critical_section structure
+ * \return true if the critical section is initialized, false otherwise
+ */
+static inline bool critical_section_is_initialized(critical_section_t *crit_sec) {
+    return crit_sec->spin_lock != 0;
+}
+
 #ifdef __cplusplus
 }
 #endif

src/common/pico_time/include/pico/time.h

@@ -200,7 +200,7 @@ static inline bool is_nil_time(absolute_time_t t) {
  * \note  These functions should not be called from an IRQ handler.
  *
  * \note  Lower powered sleep requires use of the \link alarm_pool_get_default default alarm pool\endlink which may
- * be disabled by the #PICO_TIME_DEFAULT_ALARM_POOL_DISABLED define or currently full in which case these functions
+ * be disabled by the PICO_TIME_DEFAULT_ALARM_POOL_DISABLED #define or currently full in which case these functions
  * become busy waits instead.
  *
  * \note  Whilst \a sleep_ functions are preferable to \a busy_wait functions from a power perspective, the \a busy_wait equivalent function
@@ -405,6 +405,14 @@ alarm_pool_t *alarm_pool_create(uint hardware_alarm_num, uint max_timers);
  */
 uint alarm_pool_hardware_alarm_num(alarm_pool_t *pool);
 
+/**
+ * \brief Return the core number the alarm pool was initialized on (and hence callbacks are called on)
+ * \ingroup alarm
+ * \param pool the pool
+ * \return the core used by the pool
+ */
+uint alarm_pool_core_num(alarm_pool_t *pool);
+
 /**
  * \brief Destroy the alarm pool, cancelling all alarms and freeing up the underlying hardware alarm
  * \ingroup alarm

src/common/pico_time/time.c

@@ -31,6 +31,7 @@ typedef struct alarm_pool {
     uint8_t *entry_ids_high;
     alarm_id_t alarm_in_progress; // this is set during a callback from the IRQ handler... it can be cleared by alarm_cancel to prevent repeats
     uint8_t hardware_alarm_num;
+    uint8_t core_num;
 } alarm_pool_t;
 
 #if !PICO_TIME_DEFAULT_ALARM_POOL_DISABLED
@@ -190,6 +191,7 @@ void alarm_pool_post_alloc_init(alarm_pool_t *pool, uint hardware_alarm_num) {
     hardware_alarm_set_callback(hardware_alarm_num, alarm_pool_alarm_callback);
     pool->lock = spin_lock_instance(next_striped_spin_lock_num());
     pool->hardware_alarm_num = (uint8_t) hardware_alarm_num;
+    pool->core_num = get_core_num();
     pools[hardware_alarm_num] = pool;
 }
 
@@ -286,6 +288,10 @@ uint alarm_pool_hardware_alarm_num(alarm_pool_t *pool) {
     return pool->hardware_alarm_num;
 }
 
+uint alarm_pool_core_num(alarm_pool_t *pool) {
+    return pool->core_num;
+}
+
 static void alarm_pool_dump_key(pheap_node_id_t id, void *user_data) {
     alarm_pool_t *pool = (alarm_pool_t *)user_data;
 #if PICO_ON_DEVICE

src/rp2_common/pico_stdio/include/pico/stdio.h

@@ -52,9 +52,10 @@ typedef struct stdio_driver stdio_driver_t;
  * When stdio_usb is configured, this method can be optionally made to block, waiting for a connection
  * via the variables specified in \ref stdio_usb_init (i.e. \ref PICO_STDIO_USB_CONNECT_WAIT_TIMEOUT_MS)
  *
+ * \return true if at least one output was successfully initialized, false otherwise.
  * \see stdio_uart, stdio_usb, stdio_semihosting
  */
-void stdio_init_all(void);
+bool stdio_init_all(void);
 
 /*! \brief Initialize all of the present standard stdio types that are linked into the binary.
  * \ingroup pico_stdio

src/rp2_common/pico_stdio/stdio.c

@@ -267,20 +267,25 @@ int __printflike(1, 0) WRAPPER_FUNC(printf)(const char* format, ...)
     return ret;
 }
 
-void stdio_init_all(void) {
+bool stdio_init_all(void) {
     // todo add explicit custom, or registered although you can call stdio_enable_driver explicitly anyway
     // These are well known ones
+
+    bool rc = false;
 #if LIB_PICO_STDIO_UART
     stdio_uart_init();
+    rc = true;
 #endif
 
 #if LIB_PICO_STDIO_SEMIHOSTING
     stdio_semihosting_init();
+    rc = true;
 #endif
 
 #if LIB_PICO_STDIO_USB
-    stdio_usb_init();
+    rc |= stdio_usb_init();
 #endif
+    return rc;
 }
 
 int WRAPPER_FUNC(getchar)(void) {

src/rp2_common/pico_stdio_usb/stdio_usb.c

@@ -25,6 +25,11 @@ static uint8_t stdio_usb_core_num;
 
 // when tinyusb_device is explicitly linked we do no background tud processing
 #if !LIB_TINYUSB_DEVICE
+// if this crit_sec is initialized, we are not in periodic timer mode, and must make sure
+// we don't either create multiple one shot timers, or miss creating one. this crit_sec
+// is used to protect the one_shot_timer_pending flag
+static critical_section_t one_shot_timer_crit_sec;
+static volatile bool one_shot_timer_pending;
 #ifdef PICO_STDIO_USB_LOW_PRIORITY_IRQ
 static_assert(PICO_STDIO_USB_LOW_PRIORITY_IRQ >= NUM_IRQS - NUM_USER_IRQS, "");
 #define low_priority_irq_num PICO_STDIO_USB_LOW_PRIORITY_IRQ
@@ -32,13 +37,45 @@ static_assert(PICO_STDIO_USB_LOW_PRIORITY_IRQ >= NUM_IRQS - NUM_USER_IRQS, "");
 static uint8_t low_priority_irq_num;
 #endif
 
+static int64_t timer_task(__unused alarm_id_t id, __unused void *user_data) {
+    int64_t repeat_time;
+    if (critical_section_is_initialized(&one_shot_timer_crit_sec)) {
+        critical_section_enter_blocking(&one_shot_timer_crit_sec);
+        one_shot_timer_pending = false;
+        critical_section_exit(&one_shot_timer_crit_sec);
+        repeat_time = 0; // don't repeat
+    } else {
+        repeat_time = PICO_STDIO_USB_TASK_INTERVAL_US;
+    }
+    irq_set_pending(low_priority_irq_num);
+    return repeat_time;
+}
+
 static void low_priority_worker_irq(void) {
-    // if the mutex is already owned, then we are in user code
-    // in this file which will do a tud_task itself, so we'll just do nothing
-    // until the next tick; we won't starve
     if (mutex_try_enter(&stdio_usb_mutex, NULL)) {
         tud_task();
         mutex_exit(&stdio_usb_mutex);
+    } else {
+        // if the mutex is already owned, then we are in non IRQ code in this file.
+        //
+        // it would seem simplest to just let that code call tud_task() at the end, however this
+        // code might run during the call to tud_task() and we might miss a necessary tud_task() call
+        //
+        // if we are using a periodic timer (crit_sec is not initialized in this case),
+        // then we are happy just to wait until the next tick, however when we are not using a periodic timer,
+        // we must kick off a one-shot timer to make sure the tud_task() DOES run (this method
+        // will be called again as a result, and will try the mutex_try_enter again, and if that fails
+        // create another one shot timer again, and so on).
+        if (critical_section_is_initialized(&one_shot_timer_crit_sec)) {
+            bool need_timer;
+            critical_section_enter_blocking(&one_shot_timer_crit_sec);
+            need_timer = !one_shot_timer_pending;
+            one_shot_timer_pending = true;
+            critical_section_exit(&one_shot_timer_crit_sec);
+            if (need_timer) {
+                add_alarm_in_us(PICO_STDIO_USB_TASK_INTERVAL_US, timer_task, NULL, true);
+            }
+        }
     }
 }
 
@@ -46,11 +83,6 @@ static void usb_irq(void) {
     irq_set_pending(low_priority_irq_num);
 }
 
-static int64_t timer_task(__unused alarm_id_t id, __unused void *user_data) {
-    assert(stdio_usb_core_num == get_core_num()); // if this fails, you have initialized stdio_usb on the wrong core
-    irq_set_pending(low_priority_irq_num);
-    return PICO_STDIO_USB_TASK_INTERVAL_US;
-}
 #endif
 
 static void stdio_usb_out_chars(const char *buf, int length) {
@@ -97,6 +129,9 @@ int stdio_usb_in_chars(char *buf, int length) {
     if (tud_cdc_connected() && tud_cdc_available()) {
         int count = (int) tud_cdc_read(buf, (uint32_t) length);
         rc =  count ? count : PICO_ERROR_NO_DATA;
+    } else {
+        // because our mutex use may starve out the background task, run tud_task here (we own the mutex)
+        tud_task();
     }
     mutex_exit(&stdio_usb_mutex);
     return rc;
@@ -111,6 +146,12 @@ stdio_driver_t stdio_usb = {
 };
 
 bool stdio_usb_init(void) {
+    if (get_core_num() != alarm_pool_core_num(alarm_pool_get_default())) {
+        // included an assertion here rather than just returning false, as this is likely
+        // a coding bug, rather than anything else.
+        assert(false);
+        return false;
+    }
 #ifndef NDEBUG
     stdio_usb_core_num = (uint8_t)get_core_num();
 #endif
@@ -139,8 +180,11 @@ bool stdio_usb_init(void) {
     if (irq_has_shared_handler(USBCTRL_IRQ)) {
         // we can use a shared handler to notice when there may be work to do
         irq_add_shared_handler(USBCTRL_IRQ, usb_irq, PICO_SHARED_IRQ_HANDLER_LOWEST_ORDER_PRIORITY);
+        critical_section_init_with_lock_num(&one_shot_timer_crit_sec, next_striped_spin_lock_num());
     } else {
-        rc = add_alarm_in_us(PICO_STDIO_USB_TASK_INTERVAL_US, timer_task, NULL, true);
+        rc = add_alarm_in_us(PICO_STDIO_USB_TASK_INTERVAL_US, timer_task, NULL, true) >= 0;
+        // we use initialization state of the one_shot_timer_critsec as a flag
+        memset(&one_shot_timer_crit_sec, 0, sizeof(one_shot_timer_crit_sec));
     }
 #endif
     if (rc) {