Re-Introduce into_open_drain_output

[kellerkindt]

• OutputPin for $PXi<Output> is_low(&self) -> bool

• OutputPin for $PXx<Output> is_low(&self) -> bool

  need to read from idr instead from odr if MODE equals OpenDran to be able to use the OpenDrain mode properly (communicating with another chip via one line). I dont know how to do this in the macro, maybe someone can show me how that? see below

[kellerkindt]

Reading from IDR instead of ODR in OpenDrain mode allows one to implement protocols like OneWire using only the OutputPin trait (already implemented basic OneWire functionality that way in a local project).

[kellerkindt]

OneWire protocol implementation using an OpenDrain OutputPin. Crate also includes a ds18b20 (temperature sensor) implementation

@therealprof @japaric Shall I open a ticket for the merge request?

[tib888]

What if into_open_drain_output would return not only an OutputPin, but also an InputPin? This would be more clear than the specialization of the is_low() in OpenDrain mode. The only drawback I see is that both of the returned structs should be consumed by the next into_XXX call.

[kellerkindt]

Soooo? :grin: @tib888

[tib888]

@kellerkindt I'd rather keep every OutputPin implementation the same, so the toggle could work as expected. but to avoid the name conflicts between the two is_low implementation, I would change the name to is_set_low on OutputPin trait, like this: ` impl OutputPin for $PXi<Output> { fn is_set_high(&self) -> bool { !self.is_set_low() }

                fn is_set_low(&self) -> bool {
                    // NOTE(unsafe) atomic read with no side effects
                    unsafe { (*$GPIOX::ptr()).odr.read().bits() & (1 << $i) == 0 }
                }

                fn set_high(&mut self) {
                    // NOTE(unsafe) atomic write to a stateless register
                    unsafe { (*$GPIOX::ptr()).bsrr.write(|w| w.bits(1 << $i)) }
                }

                fn set_low(&mut self) {
                    // NOTE(unsafe) atomic write to a stateless register
                    unsafe { (*$GPIOX::ptr()).bsrr.write(|w| w.bits(1 << (16 + $i))) }
                }
            }

                impl<MODE> $PXi<Output<MODE>> {
                /// Toggles the output of the pin
                pub fn toggle(&mut self) {
                    if self.is_set_low() {
                        self.set_high()
                    } else {
                        self.set_low()
                    }
                }
            }

                impl InputPin for $PXi<Output<OpenDrain>> {
                fn is_high(&self) -> bool {
                    !self.is_low()
                }

                fn is_low(&self) -> bool {
                    // NOTE(unsafe) atomic read with no side effects
                    unsafe { (*$GPIOX::ptr()).idr.read().bits() & (1 << $i) == 0 }
                }
            }

` But I haven't tested this yet...

[kellerkindt]

This isn't possible without modifying the OutputPin trait

[kellerkindt]

The more I look at the additional InputPin implementation (which is redundant and causes ambiguity regarding the is_low and is_high functions) the less I like it. Maybe reverting da288c3? What do you think @japaric, what needs to be done that this can be merged?

[tib888]

I recommend modifying the names in OutputPin, because is_low has differnt meaning on OutputPin: it just reads back the previously set state, not the true level on the pin. @japaric please make a wise decision. (I have forked embeded-hal and tested this approach with my onewire implementation. See the example here. )

[kellerkindt]

I recommend modifying the names in OutputPin, because is_low has differnt meaning on OutputPin: it just reads back the previously set state, not the true level on the pin.

Since in every other method, the state should be as set by set_low / set_high. Otherwise a physical error is present (damaged output driver or short circuit - which would probably either destroy the output driver or cause the controller to be powered off). In OpenDrain mode, the state doesn't depend solely on whether one called set_low / set_high, but on the connected periphery. See also my concerns raised in 86842b3.

The only point I see in your argumentation is, that OutputPin::is*(&pin) would always return the previously set state, while InputPin::is*(&pin) reads back from the input. But as mentioned in 86842b3, I prefer to hear @japaric opinion first. (also in da288c3 was the removal of the specialization feature and default implementation missing, therefore incomplete)

[tib888]

@kellerkindt, please find my proposal here, if that will be accepted it will solve this nicely. Until then I'm fine with your solution.

[kellerkindt]

So, I updated this pull request to the latest changes. The specialization-feature is no longer required and I had to update panic-semihosting to 0.3.0 and panic-itm to 0.2.0 because of your (@japaric ) recent changes. The examples also no longer work because of those changes, they need to reorientate to the minimal example of cortex-m-quickstart - but that is not within the scope of this pull request. I meant those but there are also these that seem fine to me...

Is this pull request now ready to be merged? If not, what further changes need to be made?

[kellerkindt]

Since #86 has been merged, there is no need to update the dependencies by this PR anymore.

This is getting a bit frustrating with no replies in months... So, please, tell me, what is blocking this PR? What am I missing?

@tib888 @ilya-epifanov @therealprof @japaric

[tib8]

@kellerkindt, I reintroduced into_open_drain_output on my branch too. It is a nice fit since the embedded-hal improved the OutputPin trait.

[kellerkindt]

@tib8 Yeah, I updated this PR beginning of June to fit the the changes of the new OutputPin trait. Your implementation is pretty similar to this PR - but this PR implements the InputPin also for $PXx<Output> in addition to $PXi<Output>.

[tib8]

I would not do that! I think StatefulOutputPin implementation of $PXi

is enough.

[kellerkindt]

The doc of StatefulOutputPin says "Output pin that can read its output state". Correct me if I am wrong, but that says that it can read back wether it was once set to high or low, right? The InputPin is reading the actual state, which you seem to be aware of, since you also implemented InputPin for $PXi<Output<OpenDrain>> on your branch.

The existing OutputPin and InputPin implementations are implemented for $PXi as well as for $PXx, so I am sticking to that pattern.

[tib8]

I implemented it only for $PXi<Output> because it makes sense. But in other cases it is too bad if the is_high() != is_set_high() (it means the pin is sorted)... Anyway, rethinking the big picture, your solution is fine: lets implement InputPin whenever that pin input register is valid.