digital::OutputPin providing toggle method

[DoumanAsh]

I'm not sure of future for the trait, but I found myself writing own convenience like toggle which is basically

if self.is_low() {
    self.set_high()
} else {
    self.set_low()
}

I feel like this could be provided method for the trait, unless there is plans for OutputPin to have more than two variants

[tib888]

Please see my last comment on https://github.com/japaric/embedded-hal/issues/29

[DoumanAsh]

Do you mean https://github.com/japaric/embedded-hal/issues/29#issuecomment-378212421 ? If so the only question is whether OutputPin methods will be changed(toggle method would be together with them)

[tib888]

@DoumanAsh, yes, if there will be methods to read back the previously set state of the OutputPin, then a toggle() method would fit there nicely (even with a default implementation).

[DoumanAsh]

I'm not sure I understand you... Should I hold on this idea for now? 😅

[tib888]

If a microcontroller hardware does not support to read back the values you have set earlier on an output pin, you may not have is_high/is_low and toggle functions on the OutputPin trait. So I recommended to split he current OutputPin trait like this:

trait OutputPin {
    fn set_high(&self);
    fn set_low(&self);
}

trait StatefulOutputPin: OutputPin {
    fn is_set_high(&self) -> bool;
    fn is_set_low(&self) -> bool;
    fn toggle();
}

OutputPin then usable on any hardware, StatefulOutputPin usable only where it has HW support. (+ I wanted to get rid of the name conflict between InputPin and OutputPin traits because some pins may need to implement both at the same time (like OpenDrain)).

[DoumanAsh]

I see, that's nice idea and if it is already on someone plan then there is no need for me to put toggle method, if we're going to split OutputPin like that.

[therealprof]

@tib888 Storing the state is a not so great idea because it might end up being wrong. Also there're MCUs which can toggle a pin in hardware without knowledge what state it is in, e.g. my arch enemy ATSAMD20.

[austinglaser]

@therealprof Correct me if I'm wrong, but I don't think tib's suggesting storing software state, but rather having StatefulOutputPin::is_set_high() and StatefulOutputPin::is_set_low() map to the same behavior as is currently implemented by OutputPin::is_high() and OutputPin::is_low() -- that is, reading back the output drive state from hardware registers. I think I've said it elsewhere, but I very much like this idea. I'm in favor primarily because of the disambiguated names, however.

Also there're MCUs which can toggle a pin in hardware without knowledge what state it is in, e.g. my arch enemy ATSAMD20.

That's a good point, and I don't think the current proposal would allow this to be easily leveraged on a platform which both allows blind toggling and disallows output readback. Do such platforms exist?

My gut feeling is that this is OK. Your particular example (ATSAMD20) would fit in well, since it seems to have output readback functionality -- so it's GPIO pins would have StatefulOutputPin implemented, with perhaps an overridden definition of StatefulOutputPin::toggle() as an optimization

[therealprof]

@austinglaser I can also see the problem the other way around. There're chips which don't allow you to read the value back in output mode and others which may allow to read back the registers but not the actual state. I think the only safe way to implement all of this is to have separate traits for all different possibilities so if a MCU doesn't support a certain variant the compiler will throw an error. A driver can then simply use a composition of traits to make sure the HAL is actually able to provide all required capabilities.

I was also thinking of writing a separate hal-support crate which allows for emulation/simulation of certain aspects required by a hardware setup, e.g. one think I can certainly see as a useful feature is negating a pin (driving common cathode vs. common anode LEDs so that high corresponds to LED on) or inverting a chip select. Similarly that could be used to mux/demux GPIOs with the support of discretes and other stuff...

[austinglaser]

I think the only safe way to implement all of this is to have separate traits for all different possibilities so if a MCU doesn't support a certain variant the compiler will throw an error.

I agree. I like the idea of structuring it so there's some hierarchy of supertraits -- such as the way the proposed OutputReadbackPin depends on OutputPin.

Would your proposed solution separate the toggle() function into its own trait (i.e. OutputTogglePin)?

[therealprof]

I agree. I like the idea of structuring it so there's some hierarchy of supertraits -- such as the way the proposed OutputReadbackPin depends on OutputPin.

I would not necessarily create "supertraits". I'd just have seperate traits for independent features so you can use trait bounds to say something like:

... where PIN: OutputPin + TogglePin

or:

... where PIN: OutputPin + ReadbackPin

(and come up with a clever name to distinguish between hardware and register state).

Would your proposed solution separate the toggle() function into its own trait (i.e. OutputTogglePin)?

Yes.

[austinglaser]

I suppose then you could have an opt-in default TogglePin impl for ReadbackPin types (ala the blocking serial default implementations), which neatly meets all use-cases.

Additionally, I like that this keeps the IO traits very focused -- I think that'll pay dividends in the future as more esoteric pin modes (open drain being my personal favorite example) get implemented.

I'm sold!

[therealprof]

I suppose then you could have an opt-in default TogglePin impl for ReadbackPin types (ala the blocking serial default implementations), which neatly meets all use-cases.

As mentioned I was thinking of doing a hal-support crate which allows emulating missing functionality so if a HAL can't natively support a trait (or just doesn't have the implementation for it) the user of the driver can decide what to use in its place, which for a toggle might be to read back register state, read back actual hardware state or keep track of the state in memory.

[RandomInsano]

Given the discussion and #29, I'm going to close this guy over in the linux-embedded-hal crate.

[DoumanAsh]

Closing as it is now part of 0.2