Generic Watchdog Timer (WDT) API

[geith]

In my opinion, a hardware watchdog timer is an essential feature for embedded systems. To make use of this, RIOT should support this feature with a generic API. A challenge on this is the differing behavior of individual hardware implementations, especially the WDT timing.

I wrote an experimental implementation for the cc2538 and tried to generalize the API (cc2538-wdt, branch cc2538-wdt, currently in /cpu/cc2538/include/cc2538_wdt.h and with cc2538_ prefix).

I would like to hear your opinion about this, and if you agree, contribute this to RIOT (maybe after some changes) as a generic API to be implemented by other platforms too.

Here my proposed API:

/**
 * @brief   Initialize the Watchdog Timer
 *
 * The watchdog timer (WDT) is initialized preferred interval in microseconds.
 * Depending on the hardware, this exact interval might be not feasible. So,
 * the initialization routine select the best suitable time (real WDT interval).
 * If possible, the real WDT interval should not be shorter than the given
 * argument, or otherwise, the maximum possible and interval is used.
 * The real WDT interval selected by the routine is returned (in microseconds).
 *
 * The function initializes but does not start the WDT.
 *
 *
 * @param[in] t_wdt         preferred WDT interval in microseconds
 *
 * @return                  -1 on error
 * @return                  applied WDT interval in microseconds
 */
int wdt_init(uint32_t t_wdt);


/**
 * @brief   Initialize the Watchdog Timer
 *
 * Initializes the WDT similarly to wdt_init(), but in contrast the
 * real WDT interval should not be longer than the given argument. If this is
 * not possible, the shortest available interval is used.
 *
 *
 * @param[in] t_wdt         preferred WDT interval in microseconds
 *
 * @return                  -1 on error
 * @return                  applied WDT interval in microseconds
 */
int wdt_init_max(uint32_t t_wdt);

/**
 * @brief   Enables the Watchdog Timer
 *
 * Enables the WDT. From this time on, the WDT has to be reseted using
 * wdt_reset() in intervals shorter than the real WDT interval
 * returned by wdt_init() or wdt_init_max() to avoid a system
 * reboot.
 *
 * @return                  0 on success
 * @return                  -1 on error
 */
int wdt_enable(void);

/**
 * @brief   Disables the Watchdog Timer
 *
 * Disables the WDT. If successful, resetting the WDT with wdt_reset()
 * is not longer required.
 * Depending on the hardware, this might not be possible; some implementations
 * do not allow disabling a started WDT.
 *
 * @return                  0 on success
 * @return                  -1 on error
 */
int wdt_disable(void);

/**
 * @brief   Checks, if the Watchdog Timer is enabled
 *
 * Checks, if the Watchdog Timer is enabled
 *
 * @return                  0 if the WDT is disabled
 * @return                  1 if the WDT is enabled
 */
int wdt_is_enabled(void);

/**
 * @brief   Resets the Watchdog Timer
 *
 * Resets the WDT.
 *
 */
void wdt_reset(void);

[jnohlgard]

Good initiative!
I have a bunch of bookmarks on my laptop related to using the watchdog in multi-tasking systems that may be relevant background information to read when designing and reviewing this API. I'll try to find them when I am back at my laptop.

[photonthunder]

Agreed, this would be very nice to have...

[jnohlgard]

Some initial thoughts:
Windowed watchdog timers should also be supported, and selectable through the api. wdt_init and wdt_init_max could be merged into a single function. As far as I can see, the only difference is the rounding behavior? (Round up to nearest greater than, or round down to nearest less than)
Also, many watchdogs have saveguards on the setting registers which trigger a hardware reset if the application tries to change the settings after the initial configuration. I don't know if the api needs to consider this, but the board initialization code needs to know this and not run the initialization more than once (e.g. First by the board_init with some default settings, then again by the application main itself for the application specific settings)

[geith]

Generally, I agree about the windowed wathdog timers. But I have no experience with them and my hardware (cc2538) has only a basic watchdog functionality. So, I would depend on additional contribution on this point.

Yes, the only difference between wdt_init() and wdt_init_max() is the round up/down behavior. So, it could be merged into one function with an additional argument (e.g. as enum with WDT_ROUND_UP and WDT_ROUND_DOWN). But it can also mapped to one internal function while keeping two different API functions. So, this simply depends on the common preference and I'm happy with both versions.

You are right about the saveguard functions; e.g. in case of the cc2538, the timing can't be changed then the WDT is enabled (causes an error on wdt_init()) and the WDT can't be disabled (causes error on wdt_disable()). In my opinion, this should be sufficient, or do you have any better suggestions?

I'm sceptical about any auto initalization. As mentioned before, the WDT settings can't be changed on the cc2538 once it is enabled and even on system startup, the reset process must startet fast enough. But this could be critical, when the reset is part of the application code. But wdt_init() should return an error when the WDT is already running.

[geith]

I was thinking again about the different initialization functions. Now, I would also prefer having only one function with selectable modes like this:

/**
 * @brief   Possible WDT timing modes
 */
typedef enum {
    WDT_EXACT,   /**< WDT interval shall be exact as requested */
    WDT_MIN,     /**< WDT interval shall be >= than requested */
    WDT_MAX,     /**< WDT interval shall be <= than requested */
} wdt_mode_t;

/**
 * @brief   Initialize the Watchdog Timer
 *
 * The watchdog timer (WDT) is initialized preferred interval in microseconds.
 * Depending on the hardware, this exact interval might be not feasible. So,
 * the initialization routine select the best suitable time (real WDT interval)
 * depending on the selected WDT mode:
 *
 * WDT_EXACT: the real WDT interval shall be exact as specified by the preferred
 *            interval (within the bounds of the timer resolution)
 *
 * WDT_MIN:   the real WDT interval shall be >= than the preferred interval,
 *            or the maximum possible one
 *
 * WDT_MAX:   the real WDT interval shall be <= than the preferred interval,
 *            or the minimum possible one
 *
 *
 * The function initializes but does not start the WDT.
 *
 *
 * @param[in] t_wdt         preferred WDT interval in microseconds
 *
 * @return                  -1 on error
 * @return                  applied WDT interval in microseconds
 */
int wdt_init(uint32_t t_wdt, wdt_mode_t mode);

[roberthartung]

You are missing an option for interrupt mode. E.g. on atmega platforms you can use the WDT in low power modes where you can wake up from the interrupt.

[geith]

I was already considering an optional interrupt callback (see below), but I'm not sure about the callback behavior. In my opinion, it should only be used for emergency actions (e.g. storing some data) before the system is reset by the WDT. The possible behavior may depend on the system implementation and the API should define a general functionality which fits to most systems (in case of the cc2538, WDT interrupts seems to be not even supported).

I'm not convinced on your example, waking up the ATmega by the WDT in low power mode. Even though you can find a lot of exampled for this, it is in my opinion just a abuse of the WDT functionality. On RIOT, this could be better implemented by threads and xtimers (please correct me, if I'm wrong...).

/**
 * @brief   Signature for WDT interrupt callback
 *
 * @param[in] arg           context to the callback (optional)
 */
typedef void(*wdt_cb_t)(void *arg);

/**
 * @brief   Initialize the Watchdog Timer
 *
 * Initialized the watchdog timer (WDT) similarly to wdt_init(), but with an
 * additional callback function which is executed when a WDT interrupt is
 * triggered. After returning from the callback function, a system reset is
 * performed.
 *
 *
 * @param[in] t_wdt         preferred WDT interval in microseconds
 *
 * @return                  -2 if WDT interrupts are not supported
 * @return                  -1 on other errors
 * @return                  applied WDT interval in microseconds
 */
int wdt_init_cb(uint32_t t_wdt, wdt_timing_t timing, wdt_cb_t wdt_cb);

[roberthartung]

@geith xtimer relies on regular timers and is therefore currently not usable for waking you up! Additionally, the timers on some atmega platforms require an external oscillator for wakeup from deep sleep modes, but the WDT can do this without, in case these are not present.
For the atmega1284p the WDTI is explicitely listed as a wakeup source. Therefore I would really vote for a generic solution here. As the functionality is given that way and I dont think it is an abuse of the WDT.

[geith]

@roberthartung:
I'm not aware of the details, but to my understanding the power management including low power operation is something that is being worked on (e.g. #6802). I also understand your "... currently not usable for waking you up..." in the same way. So, this is something that could be expected in future. Therefore, I would see the WDT for this only as a kind of workaround...
Even if the ATmega uses the WDT for the wakeup functionality, this is in my opinion not part of the general WDT concept. So, my "abuse" refers to the general WDT concept which should be reflected by the API, not to the ATmega specific usage. But the API could be designed flexible enough for supporting this too.

The question for the API design would be the expected behavior of the callback function. For the usage as wakeup timer, it must be possible to avoid a system reset. If this is intended, I see two possibilities:

1. the return value of the callback function specifies, if the system is reset or not
2. the system does no reset after returning; if needed this should be implemented in the callback function
   What is your opinion about this or do you have any other suggestions?

[roberthartung]

@geith Currently I am probably the one that is most responsible for the low power management and I am working on creating the pull requests at the moment :-)

Writing a new timer will take a longer time and for the PM I already "hacked" in a RTT implementation that is based on the WDT (because of these problems), this is ugly and a general WDT api with wakeup capability would satisfy the needs here.

Using the return value sounds generally a good idea to me and I dont see any problems here. From my point of understanding the wdt_init function would use the watchdog as it is "supposed to be". If I use wdt_init_cb, then the return value decides if I want to reset or not!

I'd say we give this interface a try and I'll just implement it for the atmega platform.

[geith]

@roberthartung Okay, I will integrate the API update into my pull request (#7374). But I can't test this functionality on my development platform (cc2538 based). So, I will keep the test program as it currently is and it could be extended later.

[geith]

My intention was to propose a generic API which should be used by other platforms too. After receiving some comments on my initial approach, I made another suggestion (my previous comment in this discussion) but did not receive any feedback.

As long as there is no aggreement on the API, this is somehow experimental.

But, in my opinion, it could be helpful to test this on other platforms too and receive some feedback.

@fjmolinas: If you want to implement this on another platform, would you prefer the original proposal or the latest suggestion in the discussion (smaller API and runtime code, more complex compile time macro stuff)? I could update my PR for this.

btw: I just noticed, that even my original PR requires some minor updates for the latest RIOT version (feature definitions/requirements).

[pekkanikander]

I implemented watchdog for nRF52, initially without knowing this issue and #7374. I'm now converting it towards the proposed standard API above.

My current code can be found in this branch: https://github.com/AaltoNEPPI/RIOT/tree/feature-nrf52-watchdog

My initial API is as follows:

/**
 * @brief   Watchdog callback type
 *
 * Called from the watchdog interrupt the interrupt context, with
 * interrupts disabled.  Can run for about 3500 instructions before
 * the watchdog resets the system.  There is no way to prevent the
 * system reset.
 */
typedef void (*watchdog_callback_t)(void);

/**
 * @brief   Silence the watchdog at the given index
 *
 * To silence the dog, this function must be called separately for
 * each index from zero to silencer_count before the watchdog timer
 * runs out.
 *
 * @param [in] idx  Index of the watchdog to feed
 */
void watchdog_silence(int idx);

/**
 * @brief   Initialise the watchdog
 *
 * @param [in] waiting_time_us  The maximum time WD will wait before
                                interrupting, in microseconds
 * @param [in] silencer_count   The number of silencers indices
 *                              configured
 * @param [in] cb               Function called at the WD interrupt.
 */
void watchdog_init(
    uint32_t waiting_time_us, int silencer_count, watchdog_callback_t cb);

The main difference of this to what has been proposed above is that nRF52 supports up to 8 distinct watchdog reset/silencing registers, making it e.g. easy to make sure that several threads are all active and running. (I have issues with thread lockup with the nRF52 SoftDevice.) Of course, it is also relatively easy to implement that as an additional software layer for those watchdogs that don't support that feature.

Wrt. the API otherwise, I think using the term reset may be misleading for feeding the dog and making it to wait again. I choose the term silence, but that is not that nice either.

Any suggestions how to proceed? Pick up #7374, rebase it on master, modify the API there, and add my code?

[jnohlgard]

Re: nomenclature, how about pet for keeping the watchdog happy? 😊

[fjmolinas]

Closing this one since #11527 got merged.