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

[RFC] digital::TriStatePin #31

Open
japaric opened this issue Jan 20, 2018 · 14 comments
Open

[RFC] digital::TriStatePin #31

japaric opened this issue Jan 20, 2018 · 14 comments
Labels

Comments

@japaric
Copy link
Member

japaric commented Jan 20, 2018

What the title says. The use case for this is some sort of generic charlieplexing interface / driver / library.

Alternatives

Vanilla

/// A tri-state (low, high, floating) pin
pub trait TriStatePin {
    /// Drives the pin low
    fn set_low(&mut self);

    /// Drives the pin high
    fn set_high(&mut self);

    /// Puts the pin in floating mode
    fn float(&mut self);

    /// Checks if the pin is currently in floating mode
    fn is_floating(&self) -> bool;

    // plus maybe other `is_*` methods
}

Enum based

/// The states of a tri-state pin
enum State { Low, High, Floating }

/// A tri-state (low, high, floating) pin
pub trait TriStatePin {
    /// Changes the state of the pin
    fn set(&mut self, state: State);

    /// Gets the state of the pin
    fn state(&self) -> State;
}

Once we pick an alternative that we are happy with we can land it behind the "unproven" feature gate. Once someone has demonstrated that it works by actually implementing the trait and building some generic library on top of it we can un-feature gate it.

cc @therealprof

@japaric japaric added the RFC label Jan 20, 2018
@japaric japaric mentioned this issue Jan 20, 2018
@japaric japaric added proposal and removed RFC labels Feb 14, 2018
@jamesmunns
Copy link
Member

jamesmunns commented Feb 19, 2018

I have a library that could use this (dw1000 driver).

I think I like the second alternative better (enum based), though we would need a "get state" (output/input), and "get value" (low or high, could work for input or output).

Nevermind, I see that this isn't meant to cover all states, just "Output with floating". I like the enum based approach even more.

@bachp
Copy link

bachp commented Feb 23, 2018

+1 for the enum based variant, it seems easier to comprehend

@EugeneGonzalez
Copy link

Enum approach is more pleasing. One concern is the "state" method could be confused for an InputPin read instead of the TriStatePin's set output.

@therealprof
Copy link
Contributor

Whoops, commented on so topics about this very thing that I forgot to raise my voice here. 😉

+1 for enum based, too.

@austinglaser
Copy link
Contributor

I think a way to read the pin's state is critical -- with it, for instance, you could build a portable bit-banged I2C implementation on top of a TriStatePin.

What if the API looked something like:

/// The states of a tri-state pin
enum State { Low, High, Floating }

/// A tri-state (low, high, floating) pin
pub trait TriStatePin {
    /// Changes the state of the pin
    fn set(&mut self, state: State);

    /// Gets the output state of the pin
    ///
    /// Queries the state set in `set()`
    fn state(&self) -> State;

    /// Whether the pin is driven high
    ///
    /// Reads the pin's actual electrical state
    fn is_high(&self) -> bool;

    /// Whether the pin is driven low
    ///
    /// Reads the pin's actual electrical state
    fn is_low(&self) -> bool;
}

The other option would be for TriStatePin implementers to also implement InputPin -- does that make more sense from a modularity perspective? It doesn't match what's done with an OutputPin, where the one trait encompasses all pin operations.

@japaric
Copy link
Member Author

japaric commented Mar 10, 2018

@austinglaser TriStatePin does not let you use the pin as an input. State::Low means the output level is low, State::High means that the output level is high, and State::Floating leaves the output floating but you can't read the voltage level when it is in that state.

What you want for a bit banged I2C implementation is the IoPin trait proposed in #29; that trait lets you toggle between the InputPin and OutputPin modes.


There seems to be consensus here on the enum based API so feel free to send a PR.

@austinglaser
Copy link
Contributor

@japaric I see what you're going for in principle here. I do think that there needs to be some discussion about reading the state of an output pin, however. Depending on the pin configuration in hardware (consider, for instance, an open-drain output), it can be critical to know the electrical state of a pin that is being "driven" high or low.

@therealprof
Copy link
Contributor

@austinglaser Not every MCU will allow to read back the state of an output pin so you might have to switch modes anyway (or store the state and hope it'll be the current one).

@austinglaser
Copy link
Contributor

@therealprof Indeed... but some do. Maybe the right place isn't in TriStatePin, but there should be some way at the trait level to express that a particular pin can be meaningfully read while in output mode

@thejpster
Copy link
Contributor

thejpster commented Mar 12, 2018 via email

@austinglaser
Copy link
Contributor

austinglaser commented Mar 12, 2018

That's a compelling idea. My first thought, though, is: "how do you read back something like a TriState pin?" Do you have a separate impl TriStateReadbackPin: TriStatePin trait? This would cause an explosion of Readback traits, for each possible type of output

What about a trait just called ReadbackPin, so you can express a trait bound such as OutputPin + ReadbackPin or TrStatePin + ReadbackPin?

@austinglaser
Copy link
Contributor

Although that's starting to look a lot like (exactly like?) an InputPin -- maybe it's more of an implementation level thing, which encourages (where possible) the implementation of InputPin for OutputPins. Then, the trait bound looks like OutputPin + InputPin or TriStatePin + InputPin

@thejpster
Copy link
Contributor

thejpster commented Mar 13, 2018 via email

@austinglaser
Copy link
Contributor

austinglaser commented Mar 13, 2018

What I've been talking about is actually reading the electrical state of an output pin. For cases where that makes sense (as I've said before, one such example is a pin configured in open-drain output mode), the implementation of InputPin could be the identical trait -- not a different one with the same name. I agree wholeheartedly that that would be a bad idea.

peckpeck pushed a commit to peckpeck/embedded-hal that referenced this issue Nov 10, 2022
31: added stable to test matrix, build deps for armhf r=posborne a=ryankurte



Co-authored-by: ryan <ryan@kurte.nz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants