4th digital::IoPin proposal from #29

[astro]

This is Jorge Aparicio's most recent IoPin proposal from #29. It leverages the type system for representing the output state. It does not automatically (unintentionally) switch modes. It avoids closures.

I've implemented it in a proof of concept. The only issue regarding ergonomics is the additional IoPin implementation that is required for both InputPin and OutputPin:

/// For an OutputPin
impl<'a> IoPin for MyOutput<'a> {
    type Input = MyInput<'a>;
    type Output = Self;

    fn into_input(self) -> Self::Input {
        MyPin {
            gpio: self.gpio
        }.into_input()
    }

    fn into_output(self) -> Self::Output {
        self
    }
}

[therealprof]

@astro Hm, we had a discussion somewhere (can't remember where exactly) where we discussed a slightly different variant with composable traits. I think @austinglaser made a nice proposal based on a discussion we had.

The "problem" I see with your approach is that switching modes between input and output might not be all the much useful and be quite a bit of overhead since that actually requires changing the GPIO mode (which also has some side effects like electrically switching from a set low/high level into high impedance mode) where usually one might only want to check the logic state of the pin (i.e. what do the registers say) or the real hardware state (what is the actual voltage level at the pin), both of which may or may not actually be implementable on some MCUs.

[astro]

I don't understand. Changing the GPIO mode is what this is about?

[therealprof]

The previous is_high() and is_low() functions kind of assumed that the actual state (or the logical state, who knows) can be figured out for any input pin while it is in input mode. Your proposal does something completely different by actually changing the mode of the pin which is not only a somewhat unexpected thing to do from a hardware perspective but also has a number of side effects.

But maybe my understanding of your proposal is just inaccurate?

[astro]

#72 is separate from this. This one is about change the mode. It does not touch is_high() or is_low().

[therealprof]

I understand what it does. I don't understand why and kind of assumed it was so one could use is_high() and is_low() which is defined for InputPins... Please explain.

[astro]

IoPin is meant for the configuration. You use into_input() to get an InputPin implementation. With that, is_high() and is_low() are available.

[therealprof]

This (in a nutshell) is a dangerous (and potentially expensive) version of a TriStatePin with readback capabilities. I really can't see any use case for that: If you explicitly change the direction of a pin, that implicitly assumes that whatever is connected to the pin has the same capabilities which IRL is almost never the case.

If bidirectional communication is needed there're special operating modes, e.g. open drain which need to be setup exactly once and require special consideration in their use. Same goes for a TriStatePin in case you need to be able to make a pin "High-Z" in addition to low/high. And for read back of logical or electrical state in Output mode, with logical being most useful for a software Toggle implementation and electrical most useful on pins with pull-up/pull-down resistors to check whether some external component overrides those, there should be additional traits allowing for composite declarations.

Please give some real world examples (and yes, there should be more than one to make it suitable for embedded-hal) where such a proposed IoPin would be actually useful and cannot be implemented in a simpler and more correct way.

[astro]

Thank you for your renewed attempts at trying to make me understand. I understand that you wish for more configuration options, eg. output type, output speed, pull-up/down, alternate functions… which are already modelled in some -hal crates, just not with generic traits.

#29 was originally about pins that can configure as either input or output. That's what IoPin tries to solve. All other options are usually set up before you use the pin. Do I understand correctly that you would rather force the user to go back to the IoPin base type instead of allowing to convert between InputPin and OutputPin?

My use-case is the linked DHT22 application, so for 1-wire this works.

[therealprof]

I understand that you wish for more configuration options, eg. output type, output speed, pull-up/down, alternate functions… which are already modelled in some -hal crates, just not with generic traits.

Indirectly, yes.

#29 was originally about pins that can configure as either input or output. That's what IoPin tries to solve. All other options are usually set up before you use the pin. Do I understand correctly that you would rather force the user to go back to the IoPin base type instead of allowing to convert between InputPin and OutputPin?

My concern is that a pin should be configured exactly once and then stay at this configuration for the runtime of the application. Typically the hardware cannot change during runtime so if there's a need to reconfigure pins to properly, it's most likely that something is wrong. I can see that many very hardware specific DHT22 drivers exist out there which simply flip a register to switch from Input to Output and vice versa but here with the IoPin there's absolutely no guarantee that this work with any HAL implementation out there (new or in the future) because they're free to do whatever they want to do the mode configuration (like reset OpenDrain or Pull-Up/Down settings or when changing into Output Mode briefly generate a signal level you don't want...) and may break your driver when relying on identical implementation and peripheral behaviour when using IoPin.

After a brief look into the Chenglish data sheet, I believe what you really need for your driver is a TriStatePin + ReadBack pin: instead of switching it into a different operating mode, you'd set the pin floating after sending the data (which should in many implementations result in the very same lightweight direction change to input mode) and then use wait use the ReadBack capabilities to read the sensor values.

[hannobraun]

Discussion here has stalled. @therealprof has made a convincing argument against the approach taken here and has suggested a different abstraction (TriStatePin + ReadBack) for the stated use case. It seems to me that this PR should be closed.

@astro Do you have any objections to closing this pull request?