embedded-hal icon indicating copy to clipboard operation
embedded-hal copied to clipboard

[RFC] `digital::TriStatePin`

Open japaric opened this issue 7 years ago • 14 comments

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 avatar Jan 20 '18 14:01 japaric

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.

jamesmunns avatar Feb 19 '18 21:02 jamesmunns

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

bachp avatar Feb 23 '18 22:02 bachp

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.

EugeneGonzalez avatar Mar 01 '18 08:03 EugeneGonzalez

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

+1 for enum based, too.

therealprof avatar Mar 01 '18 09:03 therealprof

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.

austinglaser avatar Mar 01 '18 17:03 austinglaser

@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.

japaric avatar Mar 10 '18 07:03 japaric

@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.

austinglaser avatar Mar 11 '18 00:03 austinglaser

@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).

therealprof avatar Mar 11 '18 11:03 therealprof

@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

austinglaser avatar Mar 12 '18 17:03 austinglaser

trait OutputReadbackPin: OutputPin?

On 12 Mar 2018 17:03, "austinglaser" [email protected] wrote:

@therealprof https://github.com/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

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/japaric/embedded-hal/issues/31#issuecomment-372385378, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6lj4CJRyWaA3ZH_SX5fAtQjGaykV2Dks5tdqpDgaJpZM4RlcvK .

thejpster avatar Mar 12 '18 17:03 thejpster

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 avatar Mar 12 '18 17:03 austinglaser

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

austinglaser avatar Mar 12 '18 17:03 austinglaser

Not sure I agree. An InputPin must be in input mode, while we're taking about readback which is checking which mode an output pin was most recently placed in (and perhaps therefore offering a toggle API). If we give these two traits the same function names, we'll have to use UFCS which is not that obvious, so I'd suggest the traits have different functions.

On 12 Mar 2018 17:27, "austinglaser" [email protected] wrote:

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

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/japaric/embedded-hal/issues/31#issuecomment-372394132, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6ljzdRWuBVw1OQskuCbdgCg92Bwkbxks5tdrAMgaJpZM4RlcvK .

thejpster avatar Mar 13 '18 07:03 thejpster

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.

austinglaser avatar Mar 13 '18 16:03 austinglaser