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

Open Drain Output #217

Closed
wants to merge 4 commits into from
Closed
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
89 changes: 88 additions & 1 deletion avr-hal-generic/src/port.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
//! Please take a look at the documentation for [`Pin`] for a detailed explanation.

use core::marker::PhantomData;
use embedded_hal::digital::v2::{OutputPin, InputPin};
use embedded_hal::digital::v2::{InputPin, OutputPin};

pub trait PinMode: crate::Sealed {}
/// GPIO pin modes
Expand All @@ -18,6 +18,12 @@ pub mod mode {
impl Io for Output {}
impl crate::Sealed for Output {}

// Pin is configures as a digital output with open drain behaviour
pub struct OpenDrain;
impl super::PinMode for OpenDrain {}
impl Io for OpenDrain {}
impl crate::Sealed for OpenDrain {}

pub trait InputMode: crate::Sealed {}

/// Pin is configured as digital input (floating or pulled-up).
Expand Down Expand Up @@ -137,6 +143,28 @@ impl<PIN: PinOps, MODE: mode::Io> Pin<MODE, PIN> {
}
}

/// Convert this pin into an output pin with open drain, setting the state to low
/// See [Digital Output Open Drain](#digital-output-open-drain)
pub fn into_opendrain(mut self) -> Pin<mode::OpenDrain, PIN> {
unsafe { self.pin.out_clear() };
unsafe { self.pin.make_output() };
Pin {
pin: self.pin,
_mode: PhantomData,
}
}

/// Convert this pin into an output pin with open drain, setting the state to tristate
/// See [Digital Output Open Drain](#digital-output-open-drain)
pub fn into_opendrain_tristate(mut self) -> Pin<mode::OpenDrain, PIN> {
Copy link
Owner

Choose a reason for hiding this comment

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

Similarly, this should then be into_opendrain_high().

unsafe { self.pin.out_clear() };
unsafe { self.pin.make_input(false) };
Pin {
pin: self.pin,
_mode: PhantomData,
}
}

/// Convert this pin into a floating input pin. See [Digital Input](#digital-input).
///
/// *Note*: To read deterministic values from the pin, it must be externally pulled to a
Expand Down Expand Up @@ -300,6 +328,65 @@ impl<PIN: PinOps> OutputPin for Pin<mode::Output, PIN> {
}
}

/// # Digital Output Open Drain
impl<PIN: PinOps> Pin<mode::OpenDrain, PIN> {
/// Set the pin to tristate mode (Input without PullUp)
#[inline]
pub fn set_tristate(&mut self) {
unsafe { self.pin.make_input(false) }
}
Comment on lines +333 to +337
Copy link
Owner

Choose a reason for hiding this comment

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

To rephrase my previous comment: I think for the "open drain" mode, this method should be called set_high() for consistency. Reason is:

  • "open drain" and "push pull" are both used for binary outputs and thus just represent different "encodings" of the information. To make user-code not reliant on the used "encoding", I think the methods should have the same name.
  • in the embedded-hal trait impl, the method is already called set_high()
  • other HALs also keep the same method names irregardless of whether the pin is "push pull" or "open drain"


/// Set pin low (pull it to GND, Output to low).
#[inline]
pub fn set_low(&mut self) {
unsafe { self.pin.make_output() }
}

/// Check whether the pin is set high.
///
/// *Note*: The electrical state of the pin might differ due to external circuitry.
#[inline]
pub fn is_set_high(&self) -> bool {
unsafe { self.pin.in_get() }
}
Comment on lines +345 to +351
Copy link
Owner

Choose a reason for hiding this comment

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

The is_set_high() method is supposed to return a deterministic answer to whether the pin was previously "set high" (= tristated for open-drain). Reading the pin input register here makes the result dependent on whether an outside source is driving the pin high or low.

The correct implementation would be one checking the bit in the DDR register for this pin. If it is set (= pin is output), we are currently driving the pin low. If it is cleared (= pin is input), the pin is tristated and is_set_high() should return true.

Unfortunately, PinOps does not yet expose any method for reading the DDR bit, so you'll need to add something like a dir_get() method there and also implement it in the implementation macro.


/// Check whether the pin is set low.
///
/// *Note*: The electrical state of the pin might differ due to external circuitry.
#[inline]
pub fn is_set_low(&self) -> bool {
!unsafe { self.pin.in_get() }
}
Comment on lines +357 to +359
Copy link
Owner

Choose a reason for hiding this comment

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

Following the above, is_set_low() should probably just be implemented in terms of is_set_high():

Suggested change
pub fn is_set_low(&self) -> bool {
!unsafe { self.pin.in_get() }
}
pub fn is_set_low(&self) -> bool {
!self.is_set_high()
}

The compiler will optimize it to be equivalent to a handrolled implementation.

Comment on lines +345 to +359
Copy link
Owner

Choose a reason for hiding this comment

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

I think what you actually wanted to do here is implement is_high() and is_low(). Those return the actual electrical state on the pin and would be implemented the way you did it here (reading the PIN bit for each pin)

}

// Implements OutputPin from embedded-hal to make sure external libraries work
impl<PIN: PinOps> OutputPin for Pin<mode::OpenDrain, PIN> {
type Error = core::convert::Infallible;

fn set_high(&mut self) -> Result<(), Self::Error> {
self.set_tristate();
Ok(())
}

fn set_low(&mut self) -> Result<(), Self::Error> {
self.set_low();
Ok(())
}
}

// Implements InputPin from embedded-hal to make sure external libraries work
impl<PIN: PinOps> InputPin for Pin<mode::OpenDrain, PIN> {
type Error = core::convert::Infallible;

fn is_high(&self) -> Result<bool, Self::Error> {
Ok(self.is_set_high())
}

fn is_low(&self) -> Result<bool, Self::Error> {
Ok(self.is_set_low())
}
Comment on lines +381 to +387
Copy link
Owner

Choose a reason for hiding this comment

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

And these should then call self.is_high() / self.is_low() respectively.

}

// Implements InputPin from embedded-hal to make sure external libraries work
impl<PIN: PinOps, IMODE: mode::InputMode> InputPin for Pin<mode::Input<IMODE>, PIN> {
type Error = core::convert::Infallible;
Expand Down