Fix error in watchdog.h

[T0byV]

Fixes #2496

As said in the issue, the documentation for watchdog_get_time_remaining_ms says it returns the time in microseconds, while this should be milliseconds. Function its self is fine, just a small docs error.

[peterharperuk]

I wonder whether it's actually returning microseconds. I don't see any division in watchdog_get_time_remaining_ms

// Field       : WATCHDOG_CTRL_TIME
// Description : Indicates the time in usec before a watchdog reset will be
//               triggered
#define WATCHDOG_CTRL_TIME_RESET  _u(0x000000)
#define WATCHDOG_CTRL_TIME_BITS   _u(0x00ffffff)
#define WATCHDOG_CTRL_TIME_MSB    _u(23)
#define WATCHDOG_CTRL_TIME_LSB    _u(0)
#define WATCHDOG_CTRL_TIME_ACCESS "RO"

[T0byV]

I wonder whether it's actually returning microseconds. I don't see any division in watchdog_get_time_remaining_ms

// Field       : WATCHDOG_CTRL_TIME
// Description : Indicates the time in usec before a watchdog reset will be
//               triggered
#define WATCHDOG_CTRL_TIME_RESET  _u(0x000000)
#define WATCHDOG_CTRL_TIME_BITS   _u(0x00ffffff)
#define WATCHDOG_CTRL_TIME_MSB    _u(23)
#define WATCHDOG_CTRL_TIME_LSB    _u(0)
#define WATCHDOG_CTRL_TIME_ACCESS "RO"

Hmm, but then the function name should be watchdog_get_time_remaining_us instead of watchdog_get_time_remaining_ms.

I see the description on the RP2040 is also different:

// Description : Indicates the number of ticks / 2 (see errata RP2040-E1) before
//               a watchdog reset will be triggered

I have to say I'm not very familiar with the internals of the Pico SDK.

[lurch]

I had a look at this last night - I thought it might be a duplicate of an old issue, but I found #715 and #1567 which are similar but not actually the same as the issue here. I guess it shows just how easy it is to mix up milliseconds and microseconds!

It does indeed look like it's the comment-description that is correct (i.e. the function is returning microseconds). Looking at the current implementation of watchdog_get_time_remaining_ms shows that it's just returning the WATCHDOG's CTRL.TIME field, and as Peter shows above (and you can double-check in section 4.7.6 of RP2040 datasheet and section 12.9.7 of RP2350 datasheet) this is just the raw "ticks remaining" value (with each "tick" being 1 microsecond (us) ).

If you compare this to the implementation of _watchdog_enable which does correctly accept a value in milliseconds (ms) you can see that it multiplies the value in milliseconds by 1000, to get the value in microseconds, before updating the watchdog's LOAD register (which is also in the unit of "ticks" i.e. microseconds).

On top of all that, there's also errata RP2040-E1 which means that RP2040 decrements the wachdog twice per tick. _watchdog_enable already takes this into account, but watchdog_get_time_remaining_ms doesn't take RP2040-E1 into account.

So, putting all of that together, I think that as well as changing the "microseconds" in the comment to "milliseconds" in the .h file, you also ought to change the implementation of watchdog_get_time_remaining_ms in the .c file to do

return watchdog_hw->ctrl & WATCHDOG_CTRL_TIME_BITS / (1000 * WATCHDOG_XFACTOR);

(which means you'll need to move the function below the definition of WATCHDOG_XFACTOR).
@kilograham Perhaps it also makes sense to update _watchdog_enable for consistency, and replace

        load_value = delay_ms * 1000;
#if PICO_RP2040
        load_value *= 2;
#endif

with

        load_value = delay_ms * (1000 * WATCHDOG_XFACTOR);

?

For backwards-compatibility reasons, I guess we have to hope that nobody is relying on the current implementation of watchdog_get_time_remaining_ms incorrectly returning microseconds instead of milliseconds? 🤔 But IMHO given that the watchdog_enable value is set in milliseconds, it makes more sense for the "get_time_remaining" value to also be in milliseconds.

[lurch]

Ooof, I've just noticed that watchdog.h also does

static inline uint32_t watchdog_get_count(void) {
    return watchdog_get_time_remaining_ms();
}

so that probably explains why this function is returning milliseconds instead of microseconds. If we change watchdog_get_time_remaining_ms to actually return milliseconds instead of microseconds, we'll need to add a separate implementation of watchdog_get_count, just like we had pre-SDK2.0.0 (here's the implementation that was in SDK1.5.1). There's also another small bug here as in SDK1.5.1 watchdog_get_count was returning CTRL.TIME / 2 but in SDK2.0.0 (which is when RP2350 support was added and the watchdog_get_time_remaining_ms function was added) it's returning CTRL.TIME without division, even on RP2040.

TL;DR - I'm suggesting that perhaps this PR ought to do:

diff --git a/src/rp2_common/hardware_watchdog/include/hardware/watchdog.h b/src/rp2_common/hardware_watchdog/include/hardware/watchdog.h
index e6adf65..3fde778 100644
--- a/src/rp2_common/hardware_watchdog/include/hardware/watchdog.h
+++ b/src/rp2_common/hardware_watchdog/include/hardware/watchdog.h
@@ -121,21 +121,20 @@ bool watchdog_caused_reboot(void);
 bool watchdog_enable_caused_reboot(void);
 
 /**
- * \brief Returns the number of microseconds before the watchdog will reboot the chip.
+ * \brief Returns the number of milliseconds before the watchdog will reboot the chip.
  * \ingroup hardware_watchdog
  *
  * \if rp2040_specicifc
  * On RP2040 this method returns the last value set instead of the remaining time due to a h/w bug.
  * \endif
  * 
- * @return The number of microseconds before the watchdog will reboot the chip.
+ * @return The number of milliseconds before the watchdog will reboot the chip.
  */
 uint32_t watchdog_get_time_remaining_ms(void);
 
 // backwards compatibility with SDK < 2.0.0
-static inline uint32_t watchdog_get_count(void) {
-    return watchdog_get_time_remaining_ms();
-}
+uint32_t watchdog_get_count(void);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/src/rp2_common/hardware_watchdog/watchdog.c b/src/rp2_common/hardware_watchdog/watchdog.c
index 45528ce..25ed53b 100644
--- a/src/rp2_common/hardware_watchdog/watchdog.c
+++ b/src/rp2_common/hardware_watchdog/watchdog.c
@@ -28,16 +28,21 @@ void watchdog_update(void) {
 }
 // end::watchdog_update[]
 
-uint32_t watchdog_get_time_remaining_ms(void) {
-    return watchdog_hw->ctrl & WATCHDOG_CTRL_TIME_BITS;
-}
-
 #if PICO_RP2040
-// Note, we have x2 here as the watchdog HW currently decrements twice per tick
+// Note, we have x2 here as the watchdog HW currently decrements twice per tick (errata RP2040-E1)
 #define WATCHDOG_XFACTOR 2
 #else
 #define WATCHDOG_XFACTOR 1
 #endif
+
+uint32_t watchdog_get_time_remaining_ms(void) {
+    return (watchdog_hw->ctrl & WATCHDOG_CTRL_TIME_BITS) / (1000 * WATCHDOG_XFACTOR);
+}
+
+uint32_t watchdog_get_count(void) {
+    return (watchdog_hw->ctrl & WATCHDOG_CTRL_TIME_BITS) / WATCHDOG_XFACTOR;
+}
+
 // tag::watchdog_enable[]
 // Helper function used by both watchdog_enable and watchdog_reboot
 void _watchdog_enable(uint32_t delay_ms, bool pause_on_debug) {
@@ -60,10 +65,7 @@ void _watchdog_enable(uint32_t delay_ms, bool pause_on_debug) {
     if (!delay_ms) {
         hw_set_bits(&watchdog_hw->ctrl, WATCHDOG_CTRL_TRIGGER_BITS);
     } else {
-        load_value = delay_ms * 1000;
-#if PICO_RP2040
-        load_value *= 2;
-#endif
+        load_value = delay_ms * (1000 * WATCHDOG_XFACTOR);
         if (load_value > WATCHDOG_LOAD_BITS)
             load_value = WATCHDOG_LOAD_BITS;

But it's obviously up to @kilograham to decide if this is too much of a backwards-incompatible change.

Alternatively we could just decide that the function is misnamed, and do something like this instead:

diff --git a/src/rp2_common/hardware_watchdog/include/hardware/watchdog.h b/src/rp2_common/hardware_watchdog/include/hardware/watchdog.h
index e6adf65..709fc79 100644
--- a/src/rp2_common/hardware_watchdog/include/hardware/watchdog.h
+++ b/src/rp2_common/hardware_watchdog/include/hardware/watchdog.h
@@ -130,12 +130,16 @@ bool watchdog_enable_caused_reboot(void);
  * 
  * @return The number of microseconds before the watchdog will reboot the chip.
  */
-uint32_t watchdog_get_time_remaining_ms(void);
+uint32_t watchdog_get_time_remaining_us(void);
+
+// backwards compatibility with SDK < 2.1.2
+#define watchdog_get_time_remaining_ms watchdog_get_time_remaining_us
 
 // backwards compatibility with SDK < 2.0.0
 static inline uint32_t watchdog_get_count(void) {
-    return watchdog_get_time_remaining_ms();
+    return watchdog_get_time_remaining_us();
 }
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/src/rp2_common/hardware_watchdog/watchdog.c b/src/rp2_common/hardware_watchdog/watchdog.c
index 45528ce..88a3651 100644
--- a/src/rp2_common/hardware_watchdog/watchdog.c
+++ b/src/rp2_common/hardware_watchdog/watchdog.c
@@ -28,16 +28,17 @@ void watchdog_update(void) {
 }
 // end::watchdog_update[]
 
-uint32_t watchdog_get_time_remaining_ms(void) {
-    return watchdog_hw->ctrl & WATCHDOG_CTRL_TIME_BITS;
-}
-
 #if PICO_RP2040
-// Note, we have x2 here as the watchdog HW currently decrements twice per tick
+// Note, we have x2 here as the watchdog HW currently decrements twice per tick (errata RP2040-E1)
 #define WATCHDOG_XFACTOR 2
 #else
 #define WATCHDOG_XFACTOR 1
 #endif
+
+uint32_t watchdog_get_time_remaining_us(void) {
+    return (watchdog_hw->ctrl & WATCHDOG_CTRL_TIME_BITS) / WATCHDOG_XFACTOR;
+}
+
 // tag::watchdog_enable[]
 // Helper function used by both watchdog_enable and watchdog_reboot
 void _watchdog_enable(uint32_t delay_ms, bool pause_on_debug) {