Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider improving watchdog API using move semantics #98

Closed
hannobraun opened this issue Sep 30, 2018 · 3 comments
Closed

Consider improving watchdog API using move semantics #98

hannobraun opened this issue Sep 30, 2018 · 3 comments
Labels
Milestone

Comments

@hannobraun
Copy link
Member

@astro suggested the following:

/// 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;
}

I think this is an interesting approach that's worth thinking about.

@luojia65
Copy link
Contributor

luojia65 commented Feb 27, 2020

This would work well on my HAL crate: https://github.com/gd32v-rust/gd32vf103-hal/blob/68a9d3b594038480504e3b52abea1a48aefaf148/src/wdog.rs

This new API could support the following two scenarios:

  1. The Watchdog after enabled is turned into another struct, like Watchdog<Enabled> or other probable designs. This could take ownership of old struct to prevent double start calls in compile time, and prevent disable calls if the watchdog is already disabled.

  2. If we don't need to implement different traits and functions for a disabled watchdog and an enabled one, we may use type Target = Self, which can also be adapted to the older WatchdogEnable trait.

By now the current API only support the second one. By this new design we support both second one and the first one, and fallback to second one if library designers still uses the second one by now.

luojia65 added a commit to luojia65/embedded-hal that referenced this issue Jun 16, 2020
@eldruin eldruin added this to the v1.0.0 milestone Jul 15, 2020
bors bot added a commit that referenced this issue Jul 15, 2020
222: Improve watchdog API design using move semantics r=therealprof a=luojia65

This pull request improved watchdog API design. When starting watchdog, we may convert it into another type. We may implement different functions for this type. Or downstream developers can implement `Watchdog` for only an enabled type, to prevent feed to disabled watchdogs or forget to enable before feeding. If we are able to stop this watchdog, it can be converted into the former type.

If current design still need a same type after the watchdog is enabled, they may use `Target = Self`. In this way we create a fallback for earlier designs. 

A simple proof of concept: [here](https://github.com/gd32v-rust/gd32vf103-hal/blob/new-watchdog-design/src/wdog.rs#L155-L169) (L120-L153 for its `Enable` implementation, and there is `Disable` implementation)

Related issue: #98
Earlier discussion: #76 (comment)

Co-authored-by: luojia65 <me@luojia.cc>
Co-authored-by: Luo Jia <account@luojia.cc>
@eldruin
Copy link
Member

eldruin commented Jul 15, 2020

PR #222 by @luojia65 was merged and will be released in v1.0.0.
Can this be closed now?

@hannobraun
Copy link
Member Author

Yes, I think so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants