Added initial watchdog proposal.

[xd009642]

Added initial trait proposal for watchdog timer as mentioned in #75

[austinbes]

You need to add pub mod watchdog; to src/lib.rs for this file to actually be found and compiled

[austinbes]

You're going to want to add #[cfg(feature = "unproven")] to this, since it's a new trait

[austinbes]

Is it expected that Self::Time handles the prevention of invalid periods? Otherwise, set_period would have to be fallible, I think.

Additionally, it seems like this might all be more ergonomic if there was no set_period(), and instead start() took the period as a parameter. With a set-and-forget design like this, I don't see a lot of benefit to separating the act of setting a period into its own operation, but I might be missing something

[xd009642]

Sure thing, I'll make these changes when I get home from work! With set_period I was considering whether anyone may want to change period after its started. But that may not be supported on everything so I'll change it 👍

[xd009642]

@austinbes So I've responded to the feedback as well as outcomes of the discussion on the issue. My feeling regarding period correctness is that it should be handled by Self::Time as it could map to types like PLLs which could have a design shared with other timers on the processor.

That's just a gut feeling though I haven't done any analysis on processors other than the ones I've experienced personally. I did mention it on the issue to see if anyone had any firmer thoughts.

[burrbull]

maybe WatchdogDisable?

[Serisium]

Shouldn’t this be WatchdogDisable?

[xd009642]

I haven't heard anymore on this, so is there anything that has to be done to progress it?

[therealprof]

@xd009642 Do you have an example implementation somewhere?

[xd009642]

No but I'll add one to my own embedded-hal implementation crate and link to it when I have

[xd009642]

This is a pretty quick implementation and an example showing it used. Ideally I'd have timers etc implemented so I could more scientifically trigger resets etc but I feel this is alright as a proof of concept.

Hal implementation:
https://github.com/xd009642/stm32f469xx-hal/blob/watchdog/src/watchdog.rs

Example usage:
https://github.com/xd009642/stm32f469i-disco/blob/watchdog-test/examples/watchdog.rs

[hannobraun]

This is just a nitpick, and maybe I'm squeamish, but I don't like the thought of kicking a dog :)

In the LPC82x documentation, this operation is called "feed"; nRF52 calls this "reload". I think "reload" is a better and more descriptive name.

[hannobraun]

I think this looks reasonable, although I would prefer if kick was changed to reload.

@xd009642 Could you squash the commits? I'd prefer not to merge commits with build errors into master.

@rust-embedded/hal I'd like to get this merged soon. Does anyone have objections?

[therealprof]

@hannobraun FWIW I only know the phrase "kicking the watchdog" (for over 20 years even). I know the term loading only with setting the watchdog timer so naturally I would associate "reloading" with changing the timer value (independent of whether watchdogs actually allow you to do that or not).

I'm not strongly attached to whether we kick the dog or not but deviating from commonly used terminology might create confusion.

[hannobraun]

@therealprof Good to know. I've heared about kicking the watchdog for the first time in this PR, but I don't have nearly as much experience as you :)

I've also checked the STM32F0x1/STM32F0x2/STM32F0x8 reference manual, and there's no mention of kicking either (the concept doesn't seem to be called anything except "writing to the key register"). And for what it's worth, feeding has more results than kicking on Google (273.000 vs 152.000). Do you think feed would be an acceptable name?

I do find the kicking kind of distasteful, but I don't feel that strongly about it. If you think that kick is the right name, I won't object.

[xd009642]

At my workplace we commonly use kick, even in documents sent out to customers and subcontractors. I'd never considered an alternate term it's only ever been kick for me.

That said I checked an STM register manual for the STMF469 and it used the term refresh (just to throw another word into the mix).

I personally prefer kick as a term but I'm fine with whatever is understood by people. So I'll change it to feed and squash the commits and if there are any strong objections I'll just switch it to whatever the consensus is!

[xd009642]

So I did the squash and push and it says I dismissed someones review and requested reviews. I assume this is github doing things automatically and that means it's ready to progress some more 😄

[hannobraun]

This looks good to me. I'm happy to merge this as-is, but will hold off for a bit to wait for other feedback.

[hannobraun]

Thank you, @xd009642! Regarding the name, as I said, I don't feel that strongly, and among the three of us, I'm guessing I have the least experience in the embedded space.

I just noticed that there's something wrong with Travis, but on first glance, it looks like it's not related to this PR. I'll look into it, but not today.

[therealprof]

@hannobraun https://en.wikipedia.org/wiki/Watchdog_timer also mentions "kick" but anyway, LGTM.

[therealprof]

bors r+

[bors]

Build failed

• continuous-integration/travis-ci/push

[xd009642]

@hannobraun I did see on the travis log when it failed that "line 14: GH_TOKEN: unbound variable" seems to be the only issue. Although it's in after_success so I wouldn't have personally expected that to result in a failure.

[hannobraun]

I'll look into it when I get a chance. I'm a bit busy right now though, so it might take a while. If anyone wants to take a look in the meantime, feel free to do so.

[hannobraun]

So, the problem here is that after_success.sh can push the documentation to the gh-pages branch, but needs an access token to do so. It tries to access a variable called GH_TOKEN, but that doesn't seem to exist. There's configuration (link only works if you have admin access to this repository) that sets this up, but I suspect something might have been lost in the move to the rust-embedded organization.

I'm not sure what to do:

• I don't know what's wrong with the configuration I linked, and the page says that GitHub services are being deprecated. I suspect we should install Travis as a GitHub app, but I don't think I can do that for the rust-embedded organization.
• I could add the GH_TOKEN variable in the Travis settings for embedded-hal (again, link only works if you have access), but I don't think I can generate an access token for just this repository. I don't want to add a personal access token there, that anyone with access to that configuration could use.

@japaric If you know what's going, I'd appreciate a hint. If you're unsure yourself, don't worry, I'll take another look when I get the chance.

[astro]

You could use moving self to assert you feed a watchdog that must have been started:

/// Feeds an existing watchdog to ensure the processor isn't reset. Sometimes
/// commonly referred to as "kicking" or "refreshing".
#[cfg(feature = "unproven")]
pub trait Watchdog {
    /// Triggers the watchdog. This must be done once the watchdog is started
    /// to prevent the processor being reset.
    fn feed(&mut self);
}

/// Enables A watchdog timer to reset the processor if software is frozen or 
/// stalled.
pub trait WatchdogEnable {
    /// Unit of time used by the watchdog
    type Time;
    /// The started watchdog that should be `feed()`
    type Target: Watchdog;
    /// Starts the watchdog with a given period, typically once this is done 
    /// the watchdog needs to be kicked periodically or the processor is reset. 
    ///
    /// This consumes the value and returns the `Watchdog` trait that you must
    /// `feed()`.
    fn start<T>(self, period: T) -> Self::Target where T: Into<Self::Time>;
}

/// Disables a running watchdog timer so the processor won't be reset.
#[cfg(feature = "unproven")]
pub trait WatchdogDisable {
    type Target: WatchdogEnable;
    /// Disables the watchdog
    ///
    /// This stops the watchdog and returns the `WatchdogEnable` trait so that
    /// it can be started again.
    fn disable(self) -> Self::Target;
}

[hannobraun]

bors retry

[hannobraun]

@astro I kinda like your suggestions, but I think we shouldn't hold up this pull request right now. It has been open for a long time and has already been approved.

Would you mind opening a separate issue, so we can discuss your proposal separately?

[bors]

Build succeeded

• continuous-integration/travis-ci/push

[astro]

@hannobraun not sure if worth the effort if you don't think it's important.

[hannobraun]

@astro I do think it's important to further improve our traits, and I certainly think it's worth the effort to open an issue. I've gone ahead and done so: #98

But I also think we need to make forward progress. This proposal was in the pipeline for months, has gone through various reviews and iterations, and reached consensus. I think it was time to declare victory.

I'd love to see further improvements, but getting those merged will require another round of reviews, discussions, and more deep thinking. The right place to do this is a new pull request, in my opinion.