Watchdog timer trait. about embedded-hal HOT 13 OPEN

@ryankurte -- We'd have to think carefully about adding disable functionality -- not because I disagree with your point, but because some hardware watchdog timers cannot be disabled.

I think it might sense to have a separate trait (DisableableWatchdog is a bit of a mouthful, but it's what I can come up with off the top o fmy head) which owns the disable() function.

from embedded-hal.

Love the idea of this, though ime it is possible and there are a number of reasons you might want to disable the watchdog (for example, rebooting from an app with the watchdog to a boot-loader without, during long sleep states depending on architecture).
I personally would like to see disable added so we're not making application level design decisions.

from embedded-hal.

Good call, I think that's inline with the discussion on composable gpio traits also.

from embedded-hal.

I'm also familiar with platforms where the recommended method for performing a software-reset was to enable the watchdog timer, then to stall until the timer overflowed.

Should this action be captured in the trait, or should it be performed by the application?

from embedded-hal.

@ggreenwood What platform(s) are you talking about, specifically?

I don't think that's necessary or desirable to capture that trait in the application. What I can see being valuable is eventually adding a Reset (or similar) trait, which could be implemented in terms of the watchdog timer on platforms where that makes sense.

from embedded-hal.

@austinbes Some of the lower and middle-end AVR chips require using the Watchdog to reset: https://www.microchip.com/webdoc/AVRLibcReferenceManual/FAQ_1faq_softreset.html

from embedded-hal.

One other application comes to mind: A computationally-heavy library, or one that blocks to handle communication, may hog the processor for long enough to cause a watchdog reset. In this case, it would make sense for the library developer to access the Watchdog trait in order to reset it.

In this case, it could be dangerous to allow a library to have full access to the trait, since it includes the start() and set_period() functions. Only the kick() would actually be necessary.

from embedded-hal.

@xd009642 I very much like this idea. I also agree with @ryankurte that a watchdog should be disable-able since there are some architectures around that start with watchdog enabled by default (or per configuration bits) so it needs be disabled at boot (or properly taken care of by the software) otherwise it will periodically restart the MCU.

Just out of curiosity: Which platforms automatically forbid disabling the WDT? Typically enabling and locking are two separate operations (or configuration settings) and only if in locked state they cannot be disabled again.

from embedded-hal.

At least one example: The IWDG peripheral on the STM32 series (I'm referencing the STM32F303 specifically, but I believe most STM32s are the same).

from embedded-hal.

The WWDG peripheral on the STM32F469 I'm using also has the same behaviour, the watchdog is disabled at start and once enabled can't be disabled. Also, the period can be changed after it's started as setting the period to 0 causes an instant reset.

Given the differences in hardware implementations, how about splitting watchdog into two traits, WatchdogEnable which starts it and kicks it, and WatchdogDisable which only stops it. Changing the period isn't a common use case so my thoughts are it's probably out of the scope of the embedded-hal.

from embedded-hal.

It seems like both use-cases (watchdogs that are automatically enabled at reset, but can be disabled; and those which are by default disabled and unstoppable when started) would be served by having a composable set of watchdog traits -- does that match your interpretation?

@xd009642 I almost think (given @ggreenwood's comment) it should be three traits:

trait Watchdog {
    fn kick(&mut self);
}

trait WatchdogConfigure: Watchdog {
    type Time;

    fn set_period<T>(&mut self, period: T) where T: Into<Self::Time>;

    fn start(&mut self);
}

trait WatchdogDisable: Watchdog {
    fn disable(&mut self);
}

Given the WWDG example, I see what you're going for with having a separate set_period() function.

from embedded-hal.

Ah I see, slightly misread that part. Not too far off what I've just changed it to so no issue 👍

One thing mentioned on the PR comments, which I was just looking into is whether start should return a Result as on some architectures there may be invalid period values, or whether that is handled by the Time type. I personally err towards the time type as for more complicated things like PLLs I can see benefit in the type ensuring it's correctness regarding scalars etc.

from embedded-hal.

Fair enough.

from embedded-hal.