Hal update and async implementation #12
Conversation
daniel-dbg-ginsburg
commented
Apr 16, 2024
daniel-dbg-ginsburg
commented
- Embedded-hal updated to 1.0.0
- Async implementation based on embedded-async-hal
HAL 1.0.0 substantially changes API, which in turn changed the crate API (mut requirements). It is a breaking change.
There was a problem hiding this comment.
There’s a lot of really good stuff in this PR and I appreciate it greatly. Most of my comments are nit-picky, but I really don’t like the query functions (e.g. is_active) requiring &mut self instead of just self. I would like to understand why that change was made. Was there something that drove that change?
src/input/mod.rs
Outdated
|
|
||
| fn is_active(&self) -> Result<bool, Self::Error> { | ||
| fn is_active(&mut self) -> Result<bool, Self::Error> { | ||
| self.pin.is_high() |
There was a problem hiding this comment.
Why mutable? Asking the pin for its state should not mutate it.
There was a problem hiding this comment.
Thank you for the review. The reason for this change (and other similar changes) is the change in the embedded-hal. In 1.0.0 the trait InputPin defines both is_high/is_low methods as taking &mut self. So I have propagated it to the switch-hal level.
There's is a solution for that, of course: interior mutability, and I can rework my patch using that approach. I just didn't want to overcomplicate it. So tell me your preference: would you like me to do it?
There was a problem hiding this comment.
Ahhhh and now I wish I hadn’t left the working group prior to that crate going 1.0. Give me a moment to go figure out why they made that decision so I can see if it affects the one we make here. Hold tight please. I appreciate your patience with me.
There was a problem hiding this comment.
Found the PR where the hal was changed.
Long story short, it should never have happened and a strong personality discarded some very wise advice to not make it mutable. I don’t wish to propagate that mistake if it can be avoided. If you don’t mind taking a stab at preserving our existing &self API, I would appreciate it.
There was a problem hiding this comment.
Let me look into this.
There was a problem hiding this comment.
Please review the recent changes
| @@ -19,14 +42,30 @@ pub trait InputSwitch { | |||
| /// use switch_hal::{InputSwitch, OutputSwitch, Switch, IntoSwitch}; | |||
| /// # let pin = mock::Pin::with_state(mock::State::High); | |||
| /// # let mut status_led = mock::Pin::new().into_active_high_switch(); | |||
| /// let button = pin.into_active_low_switch(); | |||
| /// let mut button = pin.into_active_low_switch(); | |||
There was a problem hiding this comment.
This example highlights why I question the mutability. It makes no sense for a button input to be mutable. For the application code, after setup is complete, it’s a read only peripheral.
There was a problem hiding this comment.
Unless you have a Debouncer pin wrapper (maintains state) for example. Hidden mutability is then problematic for multi threading etc. Same for port expanders (again, hidden mutability is needed for the underlying bus).
I would have been OK with both approaches, but this one is more explicitly saying that sharing is not guaranteed.
However that ship sailed 10 months ago, the API is stabilized and I am more affected by switch-hal still depending 0.2 than by the mut :) I can work around the dependency, but it is annoying.
There was a problem hiding this comment.
All the rest of this PR has already been changed to use interior mutability; this mut can be removed now.
| async fn wait_for_change(&mut self) -> Result<(), Self::Error> { | ||
| if self.pin.get_mut().is_high()? { | ||
| self.pin.get_mut().wait_for_low().await | ||
| } else { | ||
| self.pin.get_mut().wait_for_high().await | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Hi, doesn't this possibly introduce a race?
There was a problem hiding this comment.
I suppose that's alright behavior considering the semantics of the function.
There was a problem hiding this comment.
It does. In context switching environments (FreeRTOS based ESP32s for example) you could miss some toggles this way. But I do not see a way to make it reliable without hardware support or interrupts.
There was a problem hiding this comment.
Can't this just be a single get_mut across all the function?
After all, the refcell is kept locked for the duration of the awaiting period already because it is kept across await points, so wait_for_change'ing on a Switch<T, _> already panics with failure to get_mut. That wouldn't get worse by having a single mutex, but the raciness is reduced.
