Fix error in watchdog.h #2497

Closed

T0byV wants to merge 1 commit from patch-1 into develop

T0byV commented

2025-06-01 15:33:43 -05:00

(Migrated from github.com)

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.

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 commented

2025-06-02 07:29:29 -05:00

(Migrated from github.com)

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"

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 commented

2025-06-02 07:43:26 -05:00

(Migrated from github.com)

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.

> 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 commented

2025-06-02 16:29:22 -05:00

(Migrated from github.com)

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.

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](https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/hardware_watchdog/watchdog.c#L31) 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](https://datasheets.raspberrypi.com/rp2040/rp2040-datasheet.pdf) and section 12.9.7 of [RP2350 datasheet](https://datasheets.raspberrypi.com/rp2350/rp2350-datasheet.pdf)) this is just the raw "ticks remaining" value (with each "tick" being 1 microsecond (`us`) ). If you compare this to the [implementation](https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/hardware_watchdog/watchdog.c#L43) of `_watchdog_enable` which **does** correctly accept a value in milliseconds (`ms`) you can see that it [multiplies](https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/hardware_watchdog/watchdog.c#L63) 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 ```c 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 ```c load_value = delay_ms * 1000; #if PICO_RP2040 load_value *= 2; #endif ``` with ```c 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? :thinking: 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 commented

2025-06-03 06:53:28 -05:00

(Migrated from github.com)

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) {

Ooof, I've just noticed that `watchdog.h` [also does](https://github.com/raspberrypi/pico-sdk/blob/master/src/rp2_common/hardware_watchdog/include/hardware/watchdog.h#L135) ```c 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](https://github.com/raspberrypi/pico-sdk/blob/6a7db34ff63345a7badec79ebea3aaef1712f374/src/rp2_common/hardware_watchdog/watchdog.c#L30) 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: ```patch 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: ```patch 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) { ```

kilograham commented

2025-06-18 10:31:50 -05:00

(Migrated from github.com)