Skip to content

Commit

Permalink
refactor(gpio): remove support for open-drain outputs for now
Browse files Browse the repository at this point in the history
The API of embassy-nrf does not seem consistent with the hardware and
the other Embassy crates.
  • Loading branch information
ROMemories committed Jul 8, 2024
1 parent 0178b81 commit ec26010
Show file tree
Hide file tree
Showing 5 changed files with 6 additions and 141 deletions.
38 changes: 0 additions & 38 deletions src/riot-rs-embassy/src/arch/dummy/gpio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ pub mod output {
gpio::{FromDriveStrength, FromSpeed, PinState},
};

pub(crate) const OPEN_DRAIN_AVAILABLE: bool = false;
pub(crate) const DRIVE_STRENGTH_AVAILABLE: bool = false;
pub(crate) const SPEED_AVAILABLE: bool = false;

Expand All @@ -102,15 +101,6 @@ pub mod output {
unimplemented!();
}

pub(crate) fn new_open_drain(
pin: impl Peripheral<P: Pin> + 'static,
initial_state: PinState,
drive_strength: DriveStrength,
_speed: Speed, // Not supported by this architecture
) -> OpenDrainOutput<'static> {
unimplemented!();
}

#[derive(Copy, Clone, PartialEq, Eq)]
pub enum DriveStrength {}

Expand Down Expand Up @@ -156,32 +146,4 @@ pub mod output {
unimplemented!();
}
}

pub struct OpenDrainOutput<'d> {
_marker: core::marker::PhantomData<&'d ()>,
}

impl embedded_hal::digital::ErrorType for OpenDrainOutput<'_> {
type Error = core::convert::Infallible;
}

impl OutputPin for OpenDrainOutput<'_> {
fn set_low(&mut self) -> Result<(), Self::Error> {
unimplemented!();
}

fn set_high(&mut self) -> Result<(), Self::Error> {
unimplemented!();
}
}

impl StatefulOutputPin for OpenDrainOutput<'_> {
fn is_set_high(&mut self) -> Result<bool, Self::Error> {
unimplemented!();
}

fn is_set_low(&mut self) -> Result<bool, Self::Error> {
unimplemented!();
}
}
}
19 changes: 1 addition & 18 deletions src/riot-rs-embassy/src/arch/esp/gpio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,8 @@ pub mod output {
gpio::{FromDriveStrength, FromSpeed, PinState},
};

pub(crate) use esp_hal::gpio::{AnyOutput as Output, AnyOutputOpenDrain as OpenDrainOutput};
pub(crate) use esp_hal::gpio::AnyOutput as Output;

pub(crate) const OPEN_DRAIN_AVAILABLE: bool = true;
// FIXME: ESP32 *does* support setting the drive strength, but esp-hal seems to currently make
// this impossible on `AnyOuput` (unlike on `Output`), because it internally uses an
// `ErasedPin`.
Expand All @@ -118,22 +117,6 @@ pub mod output {
output
}

// FIXME
pub(crate) fn new_open_drain(
pin: impl Peripheral<P: Pin> + 'static,
initial_state: PinState,
drive_strength: DriveStrength,
_speed: Speed, // Not supported by this architecture
) -> OpenDrainOutput<'static> {
todo!();
// let initial_state: bool = initial_state.into();
// let initial_state = Level::from(initial_state);
// let mut output = OpenDrainOutput::new(pin, initial_state);
// TODO
// output.set_drive_strength(drive_strength.into());
// output
}

// We do not provide a `Default` impl as not all pins have the same reset value.
#[derive(Copy, Clone, PartialEq, Eq)]
pub enum DriveStrength {
Expand Down
23 changes: 1 addition & 22 deletions src/riot-rs-embassy/src/arch/nrf/gpio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,8 @@ pub mod output {
gpio::{FromDriveStrength, FromSpeed, PinState},
};

// We re-export `Output` twice, for consistency with other architectures that have a dedicated
// type for open-drain outputs.
pub(crate) use embassy_nrf::gpio::{Output, Output as OpenDrainOutput, Pin};
pub(crate) use embassy_nrf::gpio::{Output, Pin};

pub(crate) const OPEN_DRAIN_AVAILABLE: bool = true;
pub(crate) const DRIVE_STRENGTH_AVAILABLE: bool = true;
pub(crate) const SPEED_AVAILABLE: bool = false;

Expand All @@ -68,31 +65,13 @@ pub mod output {
) -> Output<'static> {
let initial_state: bool = initial_state.into();
let initial_state = Level::from(initial_state);
// TODO: this also depends on the open-drain configuration
let output_drive = match drive_strength {
DriveStrength::Standard => OutputDrive::Standard,
DriveStrength::High => OutputDrive::HighDrive,
};
Output::new(pin, initial_state, output_drive)
}

pub(crate) fn new_open_drain(
pin: impl Peripheral<P: Pin> + 'static,
initial_state: PinState,
drive_strength: DriveStrength,
_speed: Speed, // Not supported by this architecture
) -> OpenDrainOutput<'static> {
// TODO: maybe factor this out with `new()`
let initial_state: bool = initial_state.into();
let initial_state = Level::from(initial_state);
// TODO: this also depends on the open-drain configuration
let output_drive = match drive_strength {
DriveStrength::Standard => OutputDrive::Standard0Disconnect1,
DriveStrength::High => OutputDrive::HighDrive0Disconnect1,
};
Output::new(pin, initial_state, output_drive)
}

#[derive(Copy, Clone, PartialEq, Eq)]
pub enum DriveStrength {
Standard,
Expand Down
17 changes: 1 addition & 16 deletions src/riot-rs-embassy/src/arch/rp2040/gpio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,8 @@ pub mod output {
gpio::{FromDriveStrength, FromSpeed, PinState},
};

pub(crate) use embassy_rp::gpio::{Output, OutputOpenDrain as OpenDrainOutput, Pin};
pub(crate) use embassy_rp::gpio::{Output, Pin};

pub(crate) const OPEN_DRAIN_AVAILABLE: bool = true;
pub(crate) const DRIVE_STRENGTH_AVAILABLE: bool = true;
pub(crate) const SPEED_AVAILABLE: bool = true;

Expand All @@ -69,20 +68,6 @@ pub mod output {
output
}

pub(crate) fn new_open_drain(
pin: impl Peripheral<P: Pin> + 'static,
initial_state: PinState,
drive_strength: DriveStrength,
speed: Speed,
) -> OpenDrainOutput<'static> {
let initial_state: bool = initial_state.into();
let initial_state = Level::from(initial_state);
let mut output = OpenDrainOutput::new(pin, initial_state);
output.set_drive_strength(drive_strength.into());
output.set_slew_rate(speed.into());
output
}

// We provide our own type because the upstream type is not `Copy` and has no `Default` impl.
#[derive(Copy, Clone, PartialEq, Eq)]
pub enum DriveStrength {
Expand Down
50 changes: 3 additions & 47 deletions src/riot-rs-embassy/src/gpio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ use crate::arch::{
gpio::{
input::{Input as ArchInput, Pin as ArchInputPin},
output::{
DriveStrength as ArchDriveStrength, OpenDrainOutput as ArchOpenDrainOutput,
Output as ArchOutput, Pin as ArchOutputPin, Speed as ArchSpeed,
DriveStrength as ArchDriveStrength, Output as ArchOutput, Pin as ArchOutputPin,
Speed as ArchSpeed,
},
},
peripheral::Peripheral,
Expand Down Expand Up @@ -211,7 +211,6 @@ pub struct Output {
}

impl Output {
// TODO: is PinState appropriate if we turn this into a open-drain-capable output?
pub fn new(pin: impl Peripheral<P: ArchOutputPin> + 'static, initial_state: PinState) -> Self {
Self::builder(pin, initial_state).build()
}
Expand Down Expand Up @@ -244,28 +243,6 @@ impl Output {
}
}

// FIXME: rename it to OutputOpenDrain for consistency with Embassy?
pub struct OpenDrainOutput {
output: ArchOpenDrainOutput<'static>, // FIXME: is this ok to require a 'static pin?
}

impl OpenDrainOutput {
pub fn set_low(&mut self) {
// All architectures are infallible.
let _ = <Self as OutputPin>::set_low(self);
}

pub fn set_high(&mut self) {
// All architectures are infallible.
let _ = <Self as OutputPin>::set_high(self);
}

pub fn toggle(&mut self) {
// All architectures are infallible.
let _ = <Self as StatefulOutputPin>::toggle(self);
}
}

pub struct OutputBuilder<P: Peripheral<P: ArchOutputPin>> {
pin: P,
initial_state: PinState,
Expand Down Expand Up @@ -389,29 +366,9 @@ impl<P: Peripheral<P: ArchOutputPin> + 'static> OutputBuilder<P> {

Output { output }
}

pub fn build_open_drain(self) -> OpenDrainOutput {
// It is not clear whether any architectures does *not* support open-drain, but we still
// check it for forward compatibility.
const {
assert!(
arch::gpio::output::OPEN_DRAIN_AVAILABLE,
"This architecture does not support open-drain GPIO outputs."
);
}

// TODO: should we move this into `output::new()`s?
let drive_strength = <ArchDriveStrength as FromDriveStrength>::from(self.drive_strength);
// TODO: should we move this into `output::new()`s?
let speed = <ArchSpeed as FromSpeed>::from(self.speed);

let output =
arch::gpio::output::new_open_drain(self.pin, self.initial_state, drive_strength, speed);

OpenDrainOutput { output }
}
}

// We wrap this into a macro because it will be re-used for open-drain outputs
macro_rules! impl_embedded_hal_output_traits {
($type:ident, $arch_type:ident) => {
impl embedded_hal::digital::ErrorType for $type {
Expand Down Expand Up @@ -446,4 +403,3 @@ macro_rules! impl_embedded_hal_output_traits {
}

impl_embedded_hal_output_traits!(Output, ArchOutput);
impl_embedded_hal_output_traits!(OpenDrainOutput, ArchOpenDrainOutput);

0 comments on commit ec26010

Please sign in to comment.