diff --git a/src/common/pico_sync/critical_section.c b/src/common/pico_sync/critical_section.c index f28732b..7cbb622 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 2f94494..9d61150 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/common/pico_time/include/pico/time.h b/src/common/pico_time/include/pico/time.h index d1cc02d..206c24c 100644 --- a/src/common/pico_time/include/pico/time.h +++ b/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 diff --git a/src/common/pico_time/time.c b/src/common/pico_time/time.c index 3a585f6..2618e8e 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 e44c01d..87c245a 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 52a1c0c..75e9062 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 cd2e2c9..576626c 100644 --- a/src/rp2_common/pico_stdio_usb/stdio_usb.c +++ b/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) {