Conversation
| fn feed(&mut self) -> Result<(), Self::Error>; | ||
| } | ||
|
|
||
| impl<T: Watchdog> Watchdog for &mut T { |
There was a problem hiding this comment.
This looked odd to my eye, but apparently this pattern was established in embedded-hal by rust-embedded/embedded-hal#310. I guess the idea is that this lets a function that takes an impl Watchdog as an argument be passed a &mut impl Watchdog and still work? It's not clear to me why you wouldn't just say that functions have to declare that their argument type is &mut impl Watchdog instead, but I think we want to align on what embedded-hal is doing, so I went with it. If anyone knows what the upside of doing this is I'd be interested in your take on it :)
There was a problem hiding this comment.
Yeah as we discussed offline, I think this pattern makes sense in some contexts/for some traits, but for a Watchdog specifically I can't think of a particular use case. Though I don't think it will necessarily hurt to have it here anyway as if someone really wants to pass an &mut impl Watchdog to a generic function there's nothing necessarily unsafe or problematic about that, since the compiler will just complain if they try to do anything funny.
There was a problem hiding this comment.
Though I do understand your concern that the blanket impl means a function that takes an impl Watchdog means we can't be certain if it will take an owned T or a &T and don't know if the watchdog will be dropped and thus disabled. I guess my thoughts are the user is responsible for understanding that (e.g. if they pass an owned watchdog they should understand it will be dropped).
embedded-mcu-hal/src/watchdog.rs
Outdated
| //! Traits for interactions with a processor's watchdog timer. | ||
|
|
||
| /// Feeds an existing watchdog to ensure the processor isn't reset. | ||
| pub trait Watchdog { |
There was a problem hiding this comment.
Apparently there was previously a HAL Watchdog type in embedded-hal, but it was removed by rust-embedded/embedded-hal#324 because it had an unconstrained time type that it was using in the Enable trait, and there was a push to remove all unconstrained time types before the 1.0 release because they're not very useful for writing generic code.
I looked into what we might be able to do to solve for that, but from looking at the spec sheets for a few watchdogs, I came to the conclusion that there may not be a great hardware-agnostic way to describe configuration of a watchdog. Some watchdogs have multiple channels, some have a minimum delay between feedings in addition to a maximum delay, etc.
My current thinking is that if we introduce this feeding trait, that will allow for writing things like a generic service to 'share' a watchdog among a few different clients (i.e. only feed the hardware watchdog when X different things have fed the service since the last feeding of the hardware watchdog), but we can leave configuration to individual HALs. If anyone has thoughts on better options I'd love to hear them, though :)
There was a problem hiding this comment.
Yeah, I lean towards traits shouldn't try to describe HW configuration, it's too much of a headache. I think configuration should all be done before the peripheral is used in any generic context.
There was a problem hiding this comment.
Pull request overview
This PR adds a new Watchdog trait to the embedded-mcu-hal crate, providing an abstraction for feeding a processor's watchdog timer to prevent resets. It includes a blanket implementation for &mut T references.
Changes:
- Added a new
Watchdogtrait with an associatedErrortype and afeedmethod - Added a blanket impl of
Watchdogfor&mut TwhereT: Watchdog - Registered the new
watchdogmodule as a public module inlib.rs
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
embedded-mcu-hal/src/watchdog.rs |
New file defining the Watchdog trait with a feed method and a blanket impl for &mut T |
embedded-mcu-hal/src/lib.rs |
Adds pub mod watchdog; to expose the new module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This adds a trait to feed a watchdog timer