nrf-rs / nrf-hal

A Rust HAL for the nRF family of devices
Apache License 2.0
499 stars 139 forks source link

Add OpenDrainIO pin state (with InputPin capability) #401

Closed Finomnis closed 1 year ago

Finomnis commented 1 year ago

As already reported in https://github.com/nrf-rs/nrf-hal/issues/339, there is currently no way to create an I/O pin in nrf-hal.

There are several reasons why having this would be useful. My personal usecase is the DHT22/AM2302 sensor. Controlling it requires a single-wire pulled-up open drain IO.

Sadly, this means that all libraries that can interface with it require InputPin + OutputPin.

Solution

There is no inherent reason why nrf chips shouldn't be able to implement this. The input buffer is always available and allows reading back the pin values during every pin configuration. Although for energy saving reasons, the input buffer is currently disabled during the OpenDrain state.

My proposal is to add an OpenDrainIO state that does not disable the input buffer and implements InputPin.

Finomnis commented 1 year ago

Open Questions: How should IoPin behave? Should we switch back and forth between OpenDrain and OpenDrainIO? Or should we just stay in OpenDrainIO? What's the usecase where a library would use IoPin? Would that usecase benefit from switching the input buffer on and off, or would that rather be a performance hit?

In my case, the DHT22 sensor libraries require InputPin + OutputPin. When would they use IoPin instead? What's the benefit of IoPin over InputPin + OutputPin? Is IoPin meant to replace InputPin + OutputPin, giving libraries the extra capability to implement it if switching state requires code to run? Is it "expected" that a type implements IoPin<Self, Self> if the type is already InputPin + OutputPin?

Finomnis commented 1 year ago

Turns out other people have doubts as well. Therefore IoPin was removed from embedded-hal for now. I'll remove it from this PR as well.

Finomnis commented 1 year ago

Possible solution if we want to re-introduce IoPin at some point:

Finomnis commented 1 year ago

Will write tests for this as well, as soon as I have an nrf52840-DK.

jonas-schievink commented 1 year ago

I wonder if this should just be the behavior of OpenDrain pins in general? I agree with https://github.com/nrf-rs/nrf-hal/pull/340#issuecomment-917206892 that it's a footgun for PushPull pins, but for OpenDrain, not so much, since those are legitimate input pins.

Finomnis commented 1 year ago

I wonder if this should just be the behavior of OpenDrain pins in general?

To my knowledge this was an intentional decision, because having an input buffer connected to a floating pin can cause substantially increased power drain. So people wanted to have an explicit opt-in. But I'm open to dispute that decision. Note that this would be potentially a (behavioral) breaking change.

jonas-schievink commented 1 year ago

Okay, that sounds good!

bors r+

bors[bot] commented 1 year ago

Build succeeded:

Finomnis commented 1 year ago

@jonas-schievink Added tests in #402