Implement open drain output

[agausmann]

Based on #217 as it has been inactive. I've rebased it on the latest main and addressed the comments in the original pull request.

[Rahix]

Hey, thanks for picking this back up! I've got two comments, apart from that it looks good to go.

[Rahix]

Looks good, thanks! Just to make sure: Did you test that it behaves correctly on real hardware? If not, I can take care of it in the coming days.

[agausmann]

I haven't yet, but I can do a test sometime today

[agausmann]

I tested it with this modified uno-blink example:
(Switched to d12 pin so there's no other circuitry going on, and also because it's the reverse current direction of the built-in LED)

Measured the resistance to ground and the voltage (with a pullup) and it seems to work as expected.

#![no_std]
#![no_main]

use panic_halt as _;

#[arduino_hal::entry]
fn main() -> ! {
    let dp = arduino_hal::Peripherals::take().unwrap();
    let pins = arduino_hal::pins!(dp);

    let mut led = pins.d12.into_opendrain_high();

    loop {
        led.set_high();
        arduino_hal::delay_ms(500);
        led.set_low();
        arduino_hal::delay_ms(500);
        led.set_high();
        arduino_hal::delay_ms(500);
        led.set_low();
        arduino_hal::delay_ms(1000);
    }
}

[agausmann]

One thing I noticed is that there isn't a toggle method or an implementation of ToggleableOutputPin for Pin<OpenDrain> (in fact, there is no StatefulOutputPin or ToggleableOutputPin for Pin<Output> either 🤔 ). Is that something that we'd want?

[stappersg]

One thing I noticed is that there isn't a toggle method or an implementation of ToggleableOutputPin for Pin<OpenDrain> (in fact, there is no StatefulOutputPin or ToggleableOutputPin for Pin<Output> either thinking ). Is that something that we'd want?

We should want that the hardware provides. If the open drain output can not be read back, don't do it, do not fake toggle for something that can not be toggled.

(My definition of toggle is an atomic read level and write invert level back, something that will work for a push-pull-output-pin. (But we are dealing with open drain outpt.))

[agausmann]

@stappersg Glancing at the ATmega328P datasheet, the bits of DDRx registers are labeled R/W, so I'm pretty sure we can determine the state of open-drain pins, and I'd assume that's also a feature on other models. So that's some justification for a StatefulOutputPin impl. (EDIT: didn't even need to check that, we already have is_high and is_low methods for open-drain pins)

And there is precedent for a read-modify-write implementation for ToggleableOutputPin. (For example, the unproven toggleable::Default trait in embedded-hal, which was even used for push-pull outputs in avr-hal prior to #56 landing.

[Rahix]

One thing I noticed is that there isn't a toggle method or an implementation of ToggleableOutputPin for Pin (in fact, there is no StatefulOutputPin or ToggleableOutputPin for Pin either thinking ). Is that something that we'd want?

Sure, this would also be nice. But I suggest adding it in a separate MR...

didn't even need to check that, we already have is_high() and is_low() methods for open-drain pins

Careful! is_high()/is_low() have a different meaning than is_set_high()/is_set_low(). The former is meant to read the electrical state of the pin. Thus their return value can be influenced by outside circuitry. The latter must not read the electrical state but return what state the microcontroller last set the pin to. This means their return value must not be influenced by outside circuitry.

This is especially important for open-drain outputs. After a set_high(), the open-drain pin might still return true for is_low() when pulled to GND externally. But is_set_low() must not be true.

[agausmann]

Careful! is_high()/is_low() have a different meaning than is_set_high()/is_set_low(). The former is meant to read the electrical state of the pin.

Oh, thanks, I should have looked more closely, it does use in_get().