rtc_get_datetime: read the rtc registers just once each

In order for the returned value to accurately reflect a single moment in time,
ensure the registers are read just once and in the datasheet order.
Before this change, the RTC registers would each be read multiple times,
leading (infrequently) to the returned fields not all reflecting the same
moment in time.

The rp2040 datasheet has what I believe is an incorrect example (embedding
the source of this function); will the datasheet be updated if this function is
fixed?

This problem is only a speculative one; I did not actually observe it in the
wild.

cmake/pico_pre_load_toolchain.cmake

@@ -1,6 +1,5 @@
 # PICO_CMAKE_CONFIG: PICO_TOOLCHAIN_PATH, Path to search for compiler, default=none (i.e. search system paths), group=build
-# Set your compiler path here if it's not in the PATH environment variable.
+set(PICO_TOOLCHAIN_PATH "${PICO_TOOLCHAIN_PATH}" CACHE INTERNAL "")
-set(PICO_TOOLCHAIN_PATH "" CACHE INTERNAL "")
 
 # Set a default build type if none was specified
 set(default_build_type "Release")

cmake/preload/toolchains/pico_arm_gcc.cmake

@@ -52,8 +52,9 @@ set(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM NEVER)
 option(PICO_DEOPTIMIZED_DEBUG "Build debug builds with -O0" 0)
 
 # todo move to platform/Generix-xxx
-set(ARM_GCC_COMMON_FLAGS " -march=armv6-m -mcpu=cortex-m0plus -mthumb")
+
-#set(ARM_GCC_COMMON_FLAGS " -mcpu=cortex-m0plus -mthumb")
+# on ARM -mcpu should not be mixed with -march
+set(ARM_GCC_COMMON_FLAGS " -mcpu=cortex-m0plus -mthumb")
 foreach(LANG IN ITEMS C CXX ASM)
     set(CMAKE_${LANG}_FLAGS_INIT "${ARM_GCC_COMMON_FLAGS}")
     if (PICO_DEOPTIMIZED_DEBUG)

pico_sdk_version.cmake

@@ -1,12 +1,20 @@
-# PICO_BUILD_DEFINE: PICO_SDK_VERSION_MAJOR, SDK major version number, type=int, pico_base
+# PICO_BUILD_DEFINE: PICO_SDK_VERSION_MAJOR, SDK major version number, type=int, group=pico_base
-# PICO_CONFIG: PICO_SDK_VERSION_MAJOR, SDK major version number, type=int, pico_base
+# PICO_CONFIG: PICO_SDK_VERSION_MAJOR, SDK major version number, type=int, group=pico_base
 set(PICO_SDK_VERSION_MAJOR 1)
-# PICO_BUILD_DEFINE: PICO_SDK_VERSION_MINOR, SDK minor version number, type=int, pico_base
+# PICO_BUILD_DEFINE: PICO_SDK_VERSION_MINOR, SDK minor version number, type=int, group=pico_base
-# PICO_CONFIG: PICO_SDK_VERSION_MINOR, SDK minor version number, type=int, pico_base
+# PICO_CONFIG: PICO_SDK_VERSION_MINOR, SDK minor version number, type=int, group=pico_base
 set(PICO_SDK_VERSION_MINOR 1)
-# PICO_BUILD_DEFINE: PICO_SDK_VERSION_REVISION, SDK version revision, type=int, pico_base
+# PICO_BUILD_DEFINE: PICO_SDK_VERSION_REVISION, SDK version revision, type=int, group=pico_base
-# PICO_CONFIG: PICO_SDK_VERSION_REVISION, SDK version revision, type=int, pico_base
+# PICO_CONFIG: PICO_SDK_VERSION_REVISION, SDK version revision, type=int, group=pico_base
-set(PICO_SDK_VERSION_REVISION 0)
+set(PICO_SDK_VERSION_REVISION 1)
+# PICO_BUILD_DEFINE: PICO_SDK_VERSION_PRE_RELEASE_ID, optional SDK pre-release version identifier, type=string, group=pico_base
+# PICO_CONFIG: PICO_SDK_VERSION_PRE_RELEASE_ID, optional SDK pre-release version identifier, type=string, group=pico_base
+set(PICO_SDK_VERSION_PRE_RELEASE_ID develop)
+
 # PICO_BUILD_DEFINE: PICO_SDK_VERSION_STRING, SDK version, type=string, group=pico_base
 # PICO_CONFIG: PICO_SDK_VERSION_STRING, SDK version, type=string, group=pico_base
 set(PICO_SDK_VERSION_STRING "${PICO_SDK_VERSION_MAJOR}.${PICO_SDK_VERSION_MINOR}.${PICO_SDK_VERSION_REVISION}")
+
+if (PICO_SDK_VERSION_PRE_RELEASE_ID)
+    set(PICO_SDK_VERSION_STRING "${PICO_SDK_VERSION_STRING}-${PICO_SDK_VERSION_PRE_RELEASE_ID}")
+endif()

src/boards/include/boards/sparkfun_micromod.h

@@ -19,7 +19,7 @@
 
 #ifndef PICO_DEFAULT_UART
 #define PICO_DEFAULT_UART 0
-#define
+#endif
 
 #ifndef PICO_DEFAULT_UART_TX_PIN
 #define PICO_DEFAULT_UART_TX_PIN 0

src/common/pico_base/include/pico/types.h

@@ -25,7 +25,7 @@ typedef unsigned int uint;
     \see update_us_since_boot()
     \ingroup timestamp
 */
-#ifndef NDEBUG
+#ifdef NDEBUG
 typedef uint64_t absolute_time_t;
 #else
 typedef struct {
@@ -40,7 +40,7 @@ typedef struct {
  * \ingroup timestamp
  */
 static inline uint64_t to_us_since_boot(absolute_time_t t) {
-#ifndef NDEBUG
+#ifdef NDEBUG
     return t;
 #else
     return t._private_us_since_boot;
@@ -55,7 +55,7 @@ static inline uint64_t to_us_since_boot(absolute_time_t t) {
  * \ingroup timestamp
  */
 static inline void update_us_since_boot(absolute_time_t *t, uint64_t us_since_boot) {
-#ifndef NDEBUG
+#ifdef NDEBUG
     *t = us_since_boot;
 #else
     assert(us_since_boot <= INT64_MAX);
@@ -63,7 +63,7 @@ static inline void update_us_since_boot(absolute_time_t *t, uint64_t us_since_bo
 #endif
 }
 
-#ifndef NDEBUG
+#ifdef NDEBUG
 #define ABSOLUTE_TIME_INITIALIZED_VAR(name, value) name = value
 #else
 #define ABSOLUTE_TIME_INITIALIZED_VAR(name, value) name = {value}

src/common/pico_util/include/pico/util/queue.h

@@ -18,6 +18,10 @@
  * \ingroup pico_util
  */
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 typedef struct {
     spin_lock_t *lock;
     uint8_t *data;
@@ -69,7 +73,7 @@ void queue_free(queue_t *q);
 static inline uint queue_get_level_unsafe(queue_t *q) {
     int32_t rc = (int32_t)q->wptr - (int32_t)q->rptr;
     if (rc < 0) {
-        rc += + q->element_count + 1;
+        rc += q->element_count + 1;
     }
     return (uint)rc;
 }
@@ -181,4 +185,7 @@ void queue_remove_blocking(queue_t *q, void *data);
  */
 void queue_peek_blocking(queue_t *q, void *data);
 
+#ifdef __cplusplus
+}
+#endif
 #endif

src/rp2_common/hardware_rtc/rtc.c

@@ -92,13 +92,16 @@ bool rtc_get_datetime(datetime_t *t) {
     }
 
     // Note: RTC_0 should be read before RTC_1
-    t->dotw  = (rtc_hw->rtc_0 & RTC_RTC_0_DOTW_BITS ) >> RTC_RTC_0_DOTW_LSB;
+    uint32_t rtc_0 = rtc_hw->rtc_0;
-    t->hour  = (rtc_hw->rtc_0 & RTC_RTC_0_HOUR_BITS ) >> RTC_RTC_0_HOUR_LSB;
+    uint32_t rtc_1 = rtc_hw->rtc_1;
-    t->min   = (rtc_hw->rtc_0 & RTC_RTC_0_MIN_BITS  ) >> RTC_RTC_0_MIN_LSB;
+
-    t->sec   = (rtc_hw->rtc_0 & RTC_RTC_0_SEC_BITS  ) >> RTC_RTC_0_SEC_LSB;
+    t->dotw  = (rtc_0 & RTC_RTC_0_DOTW_BITS ) >> RTC_RTC_0_DOTW_LSB;
-    t->year  = (rtc_hw->rtc_1 & RTC_RTC_1_YEAR_BITS ) >> RTC_RTC_1_YEAR_LSB;
+    t->hour  = (rtc_0 & RTC_RTC_0_HOUR_BITS ) >> RTC_RTC_0_HOUR_LSB;
-    t->month = (rtc_hw->rtc_1 & RTC_RTC_1_MONTH_BITS) >> RTC_RTC_1_MONTH_LSB;
+    t->min   = (rtc_0 & RTC_RTC_0_MIN_BITS  ) >> RTC_RTC_0_MIN_LSB;
-    t->day   = (rtc_hw->rtc_1 & RTC_RTC_1_DAY_BITS  ) >> RTC_RTC_1_DAY_LSB;
+    t->sec   = (rtc_0 & RTC_RTC_0_SEC_BITS  ) >> RTC_RTC_0_SEC_LSB;
+    t->year  = (rtc_1 & RTC_RTC_1_YEAR_BITS ) >> RTC_RTC_1_YEAR_LSB;
+    t->month = (rtc_1 & RTC_RTC_1_MONTH_BITS) >> RTC_RTC_1_MONTH_LSB;
+    t->day   = (rtc_1 & RTC_RTC_1_DAY_BITS  ) >> RTC_RTC_1_DAY_LSB;
 
     return true;
 }

src/rp2_common/hardware_timer/timer.c

@@ -166,8 +166,9 @@ bool hardware_alarm_set_target(uint alarm_num, absolute_time_t target) {
         // 1) actually set the hardware timer
         spin_lock_t *lock = spin_lock_instance(PICO_SPINLOCK_ID_TIMER);
         uint32_t save = spin_lock_blocking(lock);
-        timer_hw->intr = 1u << alarm_num;
+        uint8_t old_timer_callbacks_pending = timer_callbacks_pending;
         timer_callbacks_pending |= (uint8_t)(1u << alarm_num);
+        timer_hw->intr = 1u << alarm_num; // clear any IRQ
         timer_hw->alarm[alarm_num] = (uint32_t) t;
         // Set the alarm. Writing time should arm it
         target_hi[alarm_num] = (uint32_t)(t >> 32u);
@@ -178,18 +179,26 @@ bool hardware_alarm_set_target(uint alarm_num, absolute_time_t target) {
             assert(timer_hw->ints & 1u << alarm_num);
         } else {
             if (time_us_64() >= t) {
-                // ok well it is time now; the irq isn't being handled yet because of the spin lock
+                // we are already at or past the right time; there is no point in us racing against the IRQ
-                // however the other core might be in the IRQ handler itself about to do a callback
+                // we are about to generate. note however that, if there was already a timer pending before,
-                // we do the firing ourselves (and indicate to the IRQ handler if any that it shouldn't
+                // then we still let the IRQ fire, as whatever it was, is not handled by our setting missed=true here
                 missed = true;
-                // disarm the timer
+                if (timer_callbacks_pending != old_timer_callbacks_pending) {
-                timer_hw->armed = 1u << alarm_num;
+                    // disarm the timer
-                timer_hw->intr = 1u << alarm_num; // clear the IRQ too
+                    timer_hw->armed = 1u << alarm_num;
-                // and set flag in case we're already in the IRQ handler waiting on the spinlock (on the other core)
+                    // clear the IRQ...
-                timer_callbacks_pending &= (uint8_t)~(1u << alarm_num);
+                    timer_hw->intr = 1u << alarm_num;
+                    // ... including anything pending on the processor - perhaps unnecessary, but
+                    // our timer flag says we aren't expecting anything.
+                    irq_clear(harware_alarm_irq_number(alarm_num));
+                    // and clear our flag so that if the IRQ handler is already active (because it is on
+                    // the other core) it will also skip doing anything
+                    timer_callbacks_pending = old_timer_callbacks_pending;
+                }
             }
         }
         spin_unlock(lock, save);
+        // note at this point any pending timer IRQ can likely run
     }
     return missed;
 }

src/rp2_common/pico_stdio_usb/reset_interface.c

@@ -60,7 +60,7 @@ static bool resetd_control_request_cb(uint8_t __unused rhport, tusb_control_requ
 
 #if PICO_STDIO_USB_RESET_INTERFACE_SUPPORT_RESET_TO_FLASH_BOOT
         if (request->bRequest == RESET_REQUEST_FLASH) {
-            watchdog_reboot(0, SRAM_END, PICO_STDIO_USB_RESET_RESET_TO_FLASH_DELAY_MS);
+            watchdog_reboot(0, 0, PICO_STDIO_USB_RESET_RESET_TO_FLASH_DELAY_MS);
             return true;
         }
 #endif

src/rp2_common/pico_stdio_usb/stdio_usb.c

@@ -10,12 +10,13 @@
 #include "pico/time.h"
 #include "pico/stdio/driver.h"
 #include "pico/binary_info.h"
+#include "pico/mutex.h"
 #include "hardware/irq.h"
 
 static_assert(PICO_STDIO_USB_LOW_PRIORITY_IRQ > RTC_IRQ, ""); // note RTC_IRQ is currently the last one
 static mutex_t stdio_usb_mutex;
 
-static void low_priority_worker_irq() {
+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

test/kitchen_sink/kitchen_sink.c

@@ -83,6 +83,7 @@ int main(void) {
     dma_channel_configure(0, &config, &dma_to, &dma_from, 1, true);
     dma_channel_set_config(0, &config, false);
 
+    // note this loop expects to cause a breakpoint!!
     for (int i = 0; i < 20; i++) {
         puts("sleepy");
         sleep_ms(1000);
@@ -94,4 +95,6 @@ int main(void) {
             irq_remove_handler(DMA_IRQ_1, dma_handler_b);
         }
     }
+    // this should compile as we are Cortex M0+
+    __asm volatile("SVC #3");
 }

test/pico_time_test/pico_time_test.c

@@ -64,12 +64,13 @@ static bool repeating_timer_callback(struct repeating_timer *t) {
 #define RESOLUTION_ALLOWANCE PICO_HARDWARE_TIMER_RESOLUTION_US
 #endif
 
+int issue_195_test(void);
+
 int main() {
     setup_default_uart();
     alarm_pool_init_default();
 
     PICOTEST_START();
-
     struct alarm_pool *pools[NUM_TIMERS];
     for(uint i=0; i<NUM_TIMERS; i++) {
         if (i == alarm_pool_hardware_alarm_num(alarm_pool_get_default())) {
@@ -215,6 +216,35 @@ int main() {
     PICOTEST_CHECK(absolute_time_diff_us(near_the_end_of_time, at_the_end_of_time) > 0, "near the end of time should be before the end of time")
     PICOTEST_END_SECTION();
 
+    if (issue_195_test()) {
+        return -1;
+    }
+
     PICOTEST_END_TEST();
 }
 
+#define ISSUE_195_TIMER_DELAY 50
+volatile int issue_195_counter;
+int64_t issue_195_callback(alarm_id_t id, void *user_data) {
+    issue_195_counter++;
+    return -ISSUE_195_TIMER_DELAY;
+}
+
+int issue_195_test(void) {
+    PICOTEST_START_SECTION("Issue #195 race condition - without fix may hang on gcc 10.2.1 release builds");
+    absolute_time_t t1 = get_absolute_time();
+    int id = add_alarm_in_us(ISSUE_195_TIMER_DELAY, issue_195_callback, NULL, true);
+    for(uint i=0;i<5000;i++) {
+        sleep_us(100);
+        sleep_us(100);
+        uint delay = 9; // 9 seems to be the magic number (at least for reproducing on 10.2.1)
+        sleep_us(delay);
+    }
+    absolute_time_t t2 = get_absolute_time();
+    cancel_alarm(id);
+    int expected_count = absolute_time_diff_us(t1, t2) / ISSUE_195_TIMER_DELAY;
+    printf("Timer fires approx_expected=%d actual=%d\n", expected_count, issue_195_counter);
+    PICOTEST_END_SECTION();
+    return 0;
+}
+