From a3b7d32dfaa192b7dd5c4447d0de71afdaf92c07 Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Wed, 13 Jul 2022 13:59:33 -0500 Subject: [PATCH 1/9] Add call to tud_task in getchar() if no characters are available, in case low prio IRQ is starved --- src/rp2_common/pico_stdio_usb/stdio_usb.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/rp2_common/pico_stdio_usb/stdio_usb.c b/src/rp2_common/pico_stdio_usb/stdio_usb.c index cd2e2c93f..6f66f7ab1 100644 --- a/src/rp2_common/pico_stdio_usb/stdio_usb.c +++ b/src/rp2_common/pico_stdio_usb/stdio_usb.c @@ -97,6 +97,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; From 647d6fcebca25b7a697d875bbe7a945a7f3833b5 Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Wed, 13 Jul 2022 14:07:46 -0500 Subject: [PATCH 2/9] Fix possible tud_task skipping due to removal of periodic timer --- src/rp2_common/pico_stdio_usb/stdio_usb.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/rp2_common/pico_stdio_usb/stdio_usb.c b/src/rp2_common/pico_stdio_usb/stdio_usb.c index 6f66f7ab1..966649af8 100644 --- a/src/rp2_common/pico_stdio_usb/stdio_usb.c +++ b/src/rp2_common/pico_stdio_usb/stdio_usb.c @@ -31,6 +31,13 @@ static_assert(PICO_STDIO_USB_LOW_PRIORITY_IRQ >= NUM_IRQS - NUM_USER_IRQS, ""); #else static uint8_t low_priority_irq_num; #endif +static bool using_periodic_timer; + +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 using_periodic_timer ? PICO_STDIO_USB_TASK_INTERVAL_US : 0; +} static void low_priority_worker_irq(void) { // if the mutex is already owned, then we are in user code @@ -39,6 +46,11 @@ static void low_priority_worker_irq(void) { if (mutex_try_enter(&stdio_usb_mutex, NULL)) { tud_task(); mutex_exit(&stdio_usb_mutex); + } else { + // if we don't have a periodic timer, then we need to make sure the tud_task is tried again later + if (!using_periodic_timer) { + add_alarm_in_us(PICO_STDIO_USB_TASK_INTERVAL_US, timer_task, NULL, true); + } } } @@ -46,11 +58,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) { @@ -142,8 +149,10 @@ 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); + using_periodic_timer = false; } 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; + using_periodic_timer = rc; } #endif if (rc) { From 6124bc9104929fc0c4f4bb01ae6840c1862f35f1 Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Wed, 20 Jul 2022 16:01:36 -0500 Subject: [PATCH 3/9] stop creation of multiple timers when not useing repeating timers --- src/common/pico_sync/critical_section.c | 4 +-- .../pico_sync/include/pico/critical_section.h | 10 ++++++ src/rp2_common/pico_stdio_usb/stdio_usb.c | 34 +++++++++++++++---- 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/src/common/pico_sync/critical_section.c b/src/common/pico_sync/critical_section.c index f28732b7f..7cbb6227d 100644 --- a/src/common/pico_sync/critical_section.c +++ b/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 = (spin_lock_t *)-1; -#endif + crit_sec->spin_lock = NULL; } \ No newline at end of file diff --git a/src/common/pico_sync/include/pico/critical_section.h b/src/common/pico_sync/include/pico/critical_section.h index 2f9449475..9d61150a8 100644 --- a/src/common/pico_sync/include/pico/critical_section.h +++ b/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 diff --git a/src/rp2_common/pico_stdio_usb/stdio_usb.c b/src/rp2_common/pico_stdio_usb/stdio_usb.c index 966649af8..f2a503355 100644 --- a/src/rp2_common/pico_stdio_usb/stdio_usb.c +++ b/src/rp2_common/pico_stdio_usb/stdio_usb.c @@ -19,6 +19,11 @@ #include "hardware/irq.h" static mutex_t stdio_usb_mutex; +// 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; #ifndef NDEBUG static uint8_t stdio_usb_core_num; #endif @@ -31,25 +36,41 @@ static_assert(PICO_STDIO_USB_LOW_PRIORITY_IRQ >= NUM_IRQS - NUM_USER_IRQS, ""); #else static uint8_t low_priority_irq_num; #endif -static bool using_periodic_timer; 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 + 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 using_periodic_timer ? PICO_STDIO_USB_TASK_INTERVAL_US : 0; + 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 + static int foo; if (mutex_try_enter(&stdio_usb_mutex, NULL)) { tud_task(); mutex_exit(&stdio_usb_mutex); } else { // if we don't have a periodic timer, then we need to make sure the tud_task is tried again later - if (!using_periodic_timer) { - add_alarm_in_us(PICO_STDIO_USB_TASK_INTERVAL_US, timer_task, NULL, true); + 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); + } } } } @@ -149,10 +170,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); - using_periodic_timer = false; + 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) >= 0; - using_periodic_timer = rc; + // 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) { From 182c4c8c79d1035f9ddaff3f5f69ce0c9b94af68 Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Wed, 20 Jul 2022 16:03:59 -0500 Subject: [PATCH 4/9] oops --- lib/tinyusb | 2 +- src/rp2_common/pico_stdio_usb/stdio_usb.c | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/tinyusb b/lib/tinyusb index 4bfab30c0..b0b155c94 160000 --- a/lib/tinyusb +++ b/lib/tinyusb @@ -1 +1 @@ -Subproject commit 4bfab30c02279a0530e1a56f4a7c539f2d35a293 +Subproject commit b0b155c9491d9e8cfc3d40366f77accfd3302cec diff --git a/src/rp2_common/pico_stdio_usb/stdio_usb.c b/src/rp2_common/pico_stdio_usb/stdio_usb.c index f2a503355..2a359de87 100644 --- a/src/rp2_common/pico_stdio_usb/stdio_usb.c +++ b/src/rp2_common/pico_stdio_usb/stdio_usb.c @@ -56,7 +56,6 @@ 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 - static int foo; if (mutex_try_enter(&stdio_usb_mutex, NULL)) { tud_task(); mutex_exit(&stdio_usb_mutex); From c4338dcbe0a84643bd6f3b3a84927d4b9c228af1 Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Wed, 20 Jul 2022 16:10:32 -0500 Subject: [PATCH 5/9] oops - accidental bump of tinyusb --- lib/tinyusb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/tinyusb b/lib/tinyusb index b0b155c94..4bfab30c0 160000 --- a/lib/tinyusb +++ b/lib/tinyusb @@ -1 +1 @@ -Subproject commit b0b155c9491d9e8cfc3d40366f77accfd3302cec +Subproject commit 4bfab30c02279a0530e1a56f4a7c539f2d35a293 From dc2a6b3263580a00a05aed1b5e77251b07090862 Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Wed, 20 Jul 2022 16:18:49 -0500 Subject: [PATCH 6/9] ugh this is more complicated than I thought - added some comments to clarify --- src/rp2_common/pico_stdio_usb/stdio_usb.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/rp2_common/pico_stdio_usb/stdio_usb.c b/src/rp2_common/pico_stdio_usb/stdio_usb.c index 2a359de87..19ef3f8f8 100644 --- a/src/rp2_common/pico_stdio_usb/stdio_usb.c +++ b/src/rp2_common/pico_stdio_usb/stdio_usb.c @@ -53,14 +53,20 @@ static int64_t timer_task(__unused alarm_id_t id, __unused void *user_data) { } 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 we don't have a periodic timer, then we need to make sure the tud_task is tried again later + // 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 initialzied 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); From cde95d82848dbc6f4bfa88c2773905fc1ec0a5ba Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Wed, 20 Jul 2022 16:23:32 -0500 Subject: [PATCH 7/9] move some data inside #ifdef --- src/rp2_common/pico_stdio_usb/stdio_usb.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/rp2_common/pico_stdio_usb/stdio_usb.c b/src/rp2_common/pico_stdio_usb/stdio_usb.c index 19ef3f8f8..9390b00ff 100644 --- a/src/rp2_common/pico_stdio_usb/stdio_usb.c +++ b/src/rp2_common/pico_stdio_usb/stdio_usb.c @@ -19,17 +19,17 @@ #include "hardware/irq.h" static mutex_t stdio_usb_mutex; -// 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; #ifndef NDEBUG static uint8_t stdio_usb_core_num; #endif // 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 @@ -62,7 +62,7 @@ static void low_priority_worker_irq(void) { // 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 initialzied in this case), + // 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 From d095c415c04db420532e80631e2e6f5ef0884de6 Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Mon, 8 Aug 2022 09:36:09 -0500 Subject: [PATCH 8/9] added method to get alarm_pool core_num so stdio_usb_init can fail early --- src/common/pico_time/include/pico/time.h | 8 ++++++++ src/common/pico_time/time.c | 6 ++++++ src/rp2_common/pico_stdio/include/pico/stdio.h | 3 ++- src/rp2_common/pico_stdio/stdio.c | 9 +++++++-- src/rp2_common/pico_stdio_usb/stdio_usb.c | 7 ++++++- 5 files changed, 29 insertions(+), 4 deletions(-) diff --git a/src/common/pico_time/include/pico/time.h b/src/common/pico_time/include/pico/time.h index 0fde00feb..b13791c0e 100644 --- a/src/common/pico_time/include/pico/time.h +++ b/src/common/pico_time/include/pico/time.h @@ -403,6 +403,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 diff --git a/src/common/pico_time/time.c b/src/common/pico_time/time.c index 3a585f652..2618e8e47 100644 --- a/src/common/pico_time/time.c +++ b/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 diff --git a/src/rp2_common/pico_stdio/include/pico/stdio.h b/src/rp2_common/pico_stdio/include/pico/stdio.h index e44c01d05..87c245afe 100644 --- a/src/rp2_common/pico_stdio/include/pico/stdio.h +++ b/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 diff --git a/src/rp2_common/pico_stdio/stdio.c b/src/rp2_common/pico_stdio/stdio.c index 52a1c0cc0..75e906298 100644 --- a/src/rp2_common/pico_stdio/stdio.c +++ b/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) { diff --git a/src/rp2_common/pico_stdio_usb/stdio_usb.c b/src/rp2_common/pico_stdio_usb/stdio_usb.c index 9390b00ff..576626c31 100644 --- a/src/rp2_common/pico_stdio_usb/stdio_usb.c +++ b/src/rp2_common/pico_stdio_usb/stdio_usb.c @@ -38,7 +38,6 @@ static uint8_t low_priority_irq_num; #endif 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 int64_t repeat_time; if (critical_section_is_initialized(&one_shot_timer_crit_sec)) { critical_section_enter_blocking(&one_shot_timer_crit_sec); @@ -147,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 From e43f154f7ea5cf2ed51e80aab9644b2824125e3a Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Mon, 8 Aug 2022 09:40:52 -0500 Subject: [PATCH 9/9] fix comment --- src/common/pico_time/include/pico/time.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/pico_time/include/pico/time.h b/src/common/pico_time/include/pico/time.h index b13791c0e..a85421506 100644 --- a/src/common/pico_time/include/pico/time.h +++ b/src/common/pico_time/include/pico/time.h @@ -198,7 +198,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