Comments on Is Feeding a Watchdog Timer from an ISR a Bad Practice?
Parent
Is Feeding a Watchdog Timer from an ISR a Bad Practice?
In an embedded system, I require a watchdog to be able to pass ESD qualifications (the main reason for the watchdog requirement is that the device wasn't able to pass the harshest tests of the IEC 61000-4-2 standard). Having no experience with watchdogs, I went through this Memfault article. I liked the events "registration" and "flags" since it easily allows us to do conditional ORing of events depending on the system's state. Plus it feels scalable.
For the feeding part, here's what I put in place:
/** @brief Raise the event flag and feed the watchdog if all required event flags are set.
*
* @param[in] event_flag Bitmask corresponding to the triggering event.
*/
static void watchdog_feed_if_all_events_set(watchdog_event_t event_flag)
{
/* Declare as volatile since this function could be called from concurrent ISRs. */
static volatile watchdog_event_t watchdog_fed_events;
enter_critical();
watchdog_fed_events |= event_flag;
if ((watchdog_fed_events & watchdog_required_events) == watchdog_required_events) {
bsp_watchdog_feed();
watchdog_fed_events = 0;
}
exit_critical();
}
This function can then be called from any part of the application, including from ISRs.
Now for the question:
I was told it was senseless to feed a watchdog from an ISR and that a redesign is required, but no rationale was provided. I see nothing wrong with my implementation and it works just fine. I feel like a watchdog design is closely related/dependent to the system on which it is implemented and that there are no one-size-fits-all designs.
Am I missing something? Why would someone want to avoid feeding a watchdog from an ISR?
Post
The problem with refreshing the watchdog from an ISR is that in case the main program hangs for whatever the reason, the ISR will keep living and could keep the watchdog alive, effectively blocking the product from recovering from an error state, making the watchdog useless.
It could leave GPIO or other hardware peripherals in an active state, which in turn could cause larger system-level problems.
Similarly, memory corruption bugs could tear down the variables that the ISR relies on and maybe that's why main() got stuck in some exception handler in the first place. What if the problem is that runaway code or memory corruption bugs is overwriting .bss memory where the watchdog_fed_events variable is stored?
Best practices is not to mix up the watchdog with any application-tier functionality or error handling. Let it be an independent peripheral with the sole purpose to check if the program is still running.
The utopia of best practices, which isn't always possible, is to only kick the watchdog once, at the top of the main loop:
void main ()
{
for(;;)
{
kick_dog(); // the only point in the program where we kick the dog
/* rest of program */
}
}
But of course before you get to the for(;;) you need to init a bunch of stuff which might be time consuming. So you might need to refresh the wdog a couple of times during that.
In some cases you might need to do very time-consuming things in run-time like erasing flash and then you might need to refresh the dog before and after such an operation.
It's also common practice to refresh the dog immediately after initializing it. Watchdog initialization should happen very early on, normally in the reset interrupt vector before you even initialize system clock and run the CRT code.
1 comment thread