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

Improve watchdog API design using move semantics #222

Merged
merged 3 commits into from
Jul 15, 2020

Conversation

luojia65
Copy link
Contributor

@luojia65 luojia65 commented Jun 16, 2020

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 (L120-L153 for its Enable implementation, and there is Disable implementation)

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

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @therealprof (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@therealprof
Copy link
Contributor

Thanks for the PR. It looks good to me on first glance but I need to find time for a deeper review.

Two comments off the bat since you're touching this:

  • We don't want to kick dogs anymore. 😅 Can we maybe change the kick into pet or something more friendly?
  • Most watchdogs cannot be disabled once they've been armed/enabled so it would be great if we could mention that in the docs to not raise wrong expectations

@luojia65
Copy link
Contributor Author

luojia65 commented Jun 16, 2020

@therealprof Thank you! I've modified texts to avoid this word on dogs 😅 And additional documents are added to make it clear that, most watchdogs do not support disable process.

@eldruin
Copy link
Member

eldruin commented Jun 16, 2020

A shortcoming of this approach is that a naive implementation of start or disable may drop the watchdog instance if the operation failed. I am unsure if that is always fine.
I have encountered this "fallible transformations" issue in several drivers where some methods transform the driver struct type (via a mode marker type) (e.g. from config status to operation status, which expose different methods) and my solution so far is to return the original instance through the error type.
Here is an example.
This would still be possible to do with this approach but in my attempts non-trivial error recovery code starts getting messier.

@luojia65
Copy link
Contributor Author

luojia65 commented Jun 17, 2020

@eldruin Thanks for comment! :) You're right, it's somehow tricky in error handling for failable type state classes. Seems like the only way is to return value in error type, but if I'm correct, this is common on Rust's std crate. For example if a RwLock or Mutex is poisoned when trying to lock, a PoisonError is returned; this type contains an into_inner function to return the guard back. As far as I know, there's rarely an error when enabling or disabling watchdogs; but if there are (for external watchdogs etc.), embedded drivers could still take this design to solve the problem.
I don't know if I'm correct, but this process is only complex on enabling, just like conventional type conversation methods. On feeding operation the code could be easy to write as it takes only &mut self.

@eldruin eldruin added this to the v1.0.0 milestone Jul 15, 2020
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a couple comments but otherwise looks good to me.
@luojia65 could you rebase this to master and add an entry to the changelog?

src/watchdog.rs Outdated Show resolved Hide resolved
src/watchdog.rs Outdated Show resolved Hide resolved
@thejpster thejpster mentioned this pull request Jul 15, 2020
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so far :)
I rephrased the changelog entry a bit to make it a bit clearer that the changes are not additional but the API has changed. Would this be fine with you @luojia65?
Additionally, could you squash the changes into one commit?

CHANGELOG.md Outdated Show resolved Hide resolved
src/watchdog.rs Outdated Show resolved Hide resolved
Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>

Co-authored-by: Daniel Egger <@therealprof:matrix.org>
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, this looks fine to me now.
Thank you @luojia65!

@therealprof ?

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@therealprof
Copy link
Contributor

bors r+

@bors bors bot merged commit d983a3d into rust-embedded:master Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Review is incomplete T-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants