Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "switch-hal"
version = "0.4.0"
version = "0.5.0"
authors = ["Christopher J. McClellan <[email protected]>"]
edition = "2018"
description = "HAL and basic implementations for input and output switches (buttons, switches, leds, transistors)"
Expand All @@ -16,6 +16,19 @@ exclude = [

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[features]
default = []
async = ["dep:embedded-hal-async"]

[dependencies.embedded-hal]
version = "0.2.5"
features = [ "unproven" ]
version = "1.0"

[dependencies.embedded-hal-async]
version = "1.0"
optional = true

[dev-dependencies.noop-waker]
version = "0.1"

[dev-dependencies.assert_matches]
version = "1.5"
62 changes: 57 additions & 5 deletions src/input/mod.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,70 @@
#[cfg(feature = "async")]
use crate::WaitableInputSwitch;
use crate::{ActiveHigh, ActiveLow, InputSwitch, Switch};
use embedded_hal::digital::v2::InputPin;
use embedded_hal::digital::{ErrorType, InputPin};
#[cfg(feature = "async")]
use embedded_hal_async::digital::Wait;

impl<T: InputPin> InputSwitch for Switch<T, ActiveHigh> {
type Error = <T as InputPin>::Error;
type Error = <T as ErrorType>::Error;

fn is_active(&self) -> Result<bool, Self::Error> {
self.pin.is_high()
self.pin.borrow_mut().is_high()
Copy link

Choose a reason for hiding this comment

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

This can panic without the user expecting it. I told you earlier that internal mutability is complicated...

RefCell::borrow_mut - Panics if the value is currently borrowed. For a non-panicking variant, use try_borrow_mut.

}
}

impl<T: InputPin> InputSwitch for Switch<T, ActiveLow> {
type Error = <T as InputPin>::Error;
type Error = <T as ErrorType>::Error;

fn is_active(&self) -> Result<bool, Self::Error> {
self.pin.is_low()
self.pin.borrow_mut().is_low()
}
}

#[cfg(feature = "async")]
impl<T: Wait + InputPin> WaitableInputSwitch for Switch<T, ActiveHigh>
where
Switch<T, ActiveHigh>: InputSwitch,
{
type Error = <T as ErrorType>::Error;

async fn wait_for_active(&mut self) -> Result<(), Self::Error> {
self.pin.get_mut().wait_for_high().await
}

async fn wait_for_inactive(&mut self) -> Result<(), Self::Error> {
self.pin.get_mut().wait_for_low().await
}

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
}
}
}
Comment on lines +39 to +46
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, doesn't this possibly introduce a race?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose that's alright behavior considering the semantics of the function.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

The hidden panic is bad enough TBH.

Copy link
Owner

Choose a reason for hiding this comment

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

@MarSik I’m going to ask you to please watch your tone with folks.

Copy link

Choose a reason for hiding this comment

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

It was not my intent to be too harsh even though I do admit I tend to be direct.

The hidden panic coming from the RefCell behavior will cause hard to debug trouble in any multithreaded context. Be it interrupts, async (the reference is kept active within the await) or for real multi threading on cpus like RP2040 (dual core).

The RefCell is also not async aware so it can't await release of resources by a different thread like it could when using platform aware synchronization primitives (all rtic_sync, embassy_sync an even critical_section implementations provide those).


#[cfg(feature = "async")]
impl<T: Wait + InputPin> WaitableInputSwitch for Switch<T, ActiveLow>
where
Switch<T, ActiveHigh>: InputSwitch,
{
type Error = <T as ErrorType>::Error;

async fn wait_for_active(&mut self) -> Result<(), Self::Error> {
self.pin.get_mut().wait_for_low().await
Copy link

Choose a reason for hiding this comment

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

Here you correctly pair &mut self with RefCell:get_mut, however that totally negates the need for RefCell (= alloc and heap) in the first place.

}

async fn wait_for_inactive(&mut self) -> Result<(), Self::Error> {
self.pin.get_mut().wait_for_high().await
}

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
}
}
}
58 changes: 38 additions & 20 deletions src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
//! "Async" feature enables async support for input switches.

#![no_std]
#![allow(async_fn_in_trait)]

mod input;
mod output;
Expand All @@ -19,7 +22,7 @@ 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();
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

All the rest of this PR has already been changed to use interior mutability; this mut can be removed now.

Copy link

Choose a reason for hiding this comment

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

And sadly the way you did it will prevent me from using the crate now. RefCell requires alloc and heap that not everybody uses. It also prescribes a specific synchronization approach (and is unsound - see the other comments about the hidden panic).

Copy link
Owner

Choose a reason for hiding this comment

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

This crate should be #no-std.
I haven’t looked at it in ages, so I’m not certain the build enforces it, but it should if it’s not.

/// match button.is_active() {
/// Ok(true) => { status_led.on().ok(); }
/// Ok(false) => { status_led.off().ok(); }
Expand All @@ -29,6 +32,22 @@ pub trait InputSwitch {
fn is_active(&self) -> Result<bool, Self::Error>;
}

#[cfg(feature = "async")]
/// Represents an input switch that can be asynchronously waited for
pub trait WaitableInputSwitch {
type Error;

/// Waits until the switch becomes active. If the switch in already active, returns immediately
///
async fn wait_for_active(&mut self) -> Result<(), Self::Error>;
/// Waits until the switch becomes inactive. If the switch is already inactive, returns immediately
///
async fn wait_for_inactive(&mut self) -> Result<(), Self::Error>;
/// Waits until the switch changess from active to inactive, or from inactive to active
///
async fn wait_for_change(&mut self) -> Result<(), Self::Error>;
}

/// Represents an output switch, such as a LED "switch" or transistor
pub trait OutputSwitch {
type Error;
Expand Down Expand Up @@ -63,11 +82,11 @@ pub trait OutputSwitch {
/// Toggles the switch from it's current state to it's opposite state.
///
/// # Notes
/// This is only available if the underlying hal has implemented [ToggleableOutputPin](embedded_hal::digital::v2::ToggleableOutputPin)
/// This is only available if the underlying hal has implemented [StatefulOutputPin](embedded_hal::digital::StatefulOutputPin)
pub trait ToggleableOutputSwitch {
type Error;

/// Toggles the current state of the [OutputSwitch](OutputSwitch)
/// Toggles the current state of the [OutputSwitch]
///
/// # Examples
///
Expand All @@ -84,7 +103,7 @@ pub trait ToggleableOutputSwitch {
/// Checks current switch state
///
/// # Notes
/// This is only available if the underlying hal has implemented [StatefulOutputPin](embedded_hal::digital::v2::StatefulOutputPin)
/// This is only available if the underlying hal has implemented [StatefulOutputPin](embedded_hal::digital::StatefulOutputPin)
pub trait StatefulOutputSwitch {
type Error;

Expand All @@ -98,7 +117,7 @@ pub trait StatefulOutputSwitch {
/// # let pin = mock::Pin::new();
/// let mut led = pin.into_active_high_switch();
/// led.off().ok();
/// assert!(!led.is_on().unwrap());
/// assert_eq!(false, led.is_on().unwrap());
/// ```
fn is_on(&mut self) -> Result<bool, Self::Error>;

Expand All @@ -112,7 +131,7 @@ pub trait StatefulOutputSwitch {
/// # let pin = mock::Pin::new();
/// let mut led = pin.into_active_high_switch();
/// led.off().ok();
/// assert!(led.is_off().unwrap());
/// assert_eq!(true, led.is_off().unwrap());
/// ```
fn is_off(&mut self) -> Result<bool, Self::Error>;
}
Expand All @@ -122,23 +141,23 @@ pub struct ActiveHigh;
/// Zero sized struct for signaling to [Switch](struct.Switch.html) that it is active low
pub struct ActiveLow;

use core::marker::PhantomData;
use core::{cell::RefCell, marker::PhantomData};

/// Concrete implementation for [InputSwitch](trait.InputSwitch.html) and [OutputSwitch](trait.OutputSwitch.html)
///
/// # Type Params
/// - `IoPin` must be a type that implements either of the [InputPin](embedded_hal::digital::v2::InputPin) or [OutputPin](embedded_hal::digital::v2::OutputPin) traits.
/// - `ActiveLevel` indicates whether the `Switch` is [ActiveHigh](ActiveHigh) or [ActiveLow](ActiveLow).
/// - `IoPin` must be a type that implements either of the [InputPin](embedded_hal::digital::InputPin) or [OutputPin](embedded_hal::digital::OutputPin) traits.
/// - `ActiveLevel` indicates whether the `Switch` is [ActiveHigh] or [ActiveLow].
/// `ActiveLevel` is not actually stored in the struct.
/// It's [PhantomData](core::marker::PhantomData) used to indicate which implementation to use.
/// It's [PhantomData] used to indicate which implementation to use.
pub struct Switch<IoPin, ActiveLevel> {
pin: IoPin,
pin: RefCell<IoPin>,
active: PhantomData<ActiveLevel>,
}

impl<IoPin, ActiveLevel> Switch<IoPin, ActiveLevel> {
/// Constructs a new [Switch](struct.Switch.html) from a concrete implementation of an
/// [InputPin](embedded_hal::digital::v2::InputPin) or [OutputPin](embedded_hal::digital::v2::OutputPin)
/// [InputPin](embedded_hal::digital::InputPin) or [OutputPin](embedded_hal::digital::OutputPin)
///
/// **Prefer the [IntoSwitch](trait.IntoSwitch.html) trait over calling [new](#method.new) directly.**
///
Expand Down Expand Up @@ -183,12 +202,12 @@ impl<IoPin, ActiveLevel> Switch<IoPin, ActiveLevel> {
/// ```
pub fn new(pin: IoPin) -> Self {
Switch {
pin: pin,
pin: RefCell::new(pin),
active: PhantomData::<ActiveLevel>,
}
}

/// Consumes the [Switch](struct.Switch.html) and returns the underlying [InputPin](embedded_hal::digital::v2::InputPin) or [OutputPin](embedded_hal::digital::v2::OutputPin).
/// Consumes the [Switch](struct.Switch.html) and returns the underlying [InputPin](embedded_hal::digital::InputPin) or [OutputPin](embedded_hal::digital::OutputPin).
///
/// This is useful fore retrieving the underlying pin to use it for a different purpose.
///
Expand All @@ -204,19 +223,18 @@ impl<IoPin, ActiveLevel> Switch<IoPin, ActiveLevel> {
/// // do something else with the pin
/// ```
pub fn into_pin(self) -> IoPin {
self.pin
self.pin.into_inner()
}
}

/// Convenience functions for converting [InputPin](embedded_hal::digital::v2::InputPin)
/// and [OutputPin](embedded_hal::digital::v2::OutputPin) to a [Switch](struct.Switch.html).
/// Convenience functions for converting [InputPin](embedded_hal::digital::InputPin)
/// and [OutputPin](embedded_hal::digital::OutputPin) to a [Switch](struct.Switch.html).
///
/// The type of [Switch](struct.Switch.html) returned,
/// [InputSwitch](trait.InputSwitch.html) or [OutputSwitch](trait.OutputSwitch.html) is
/// determined by whether the `IoPin` being consumed is an [InputPin](embedded_hal::digital::v2::InputPin)
/// or [OutputPin](embedded_hal::digital::v2::OutputPin).
/// determined by whether the `IoPin` being consumed is an [InputPin](embedded_hal::digital::InputPin)
/// or [OutputPin](embedded_hal::digital::OutputPin).
pub trait IntoSwitch {

/// Consumes the `IoPin` returning a [Switch](struct.Switch.html) of the appropriate `ActiveLevel`.
///
/// This method exists so other, more convenient functions, can have blanket implementations.
Expand Down
Loading