Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

port: Add methods to temporarily switch a pin to a different mode #205

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Rahix
Copy link
Owner

@Rahix Rahix commented Jul 28, 2021

This allows temporarily making an output pin an input and vice versa:

let mut pin = pins.d0.into_output();

pin.with_pin_as_floating_input(|p| {
    // p is input here
    p.is_high();
});

Similarly:

  • pin.with_pin_as_pull_up_input()
  • pin.with_pin_as_output()
  • pin.with_pin_as_output_high()

TODO

  • Deduplicate implementation.
  • Check that all invariants are upheld and pin-state is always returned to what it was before.
  • Investigate whether we want the pin to be in the same state, or just the same mode - an alternative API could be one where a correct pin must be returned from the closure again:
    let pin = pins.d0.into_output();
    assert!(pin.is_set_low())
    pin.with_temporary(|p| {
      let p = p.into_floating_input();
      // ...
      p.into_output_high()
    })
    assert!(pin.is_set_high())

@Rahix Rahix added hal-api API design for the different components of avr-hal hal-generic Related to MCU generic parts of avr-hal labels Aug 2, 2021
@Rahix
Copy link
Owner Author

Rahix commented Jan 28, 2022

FWIW, other HALs have come up with a very similar API. For example: https://docs.rs/stm32f4xx-hal/0.11.1/stm32f4xx_hal/gpio/struct.Pin.html#method.with_floating_input

@mutantbob
Copy link
Contributor

I wonder how this would behave if the pin were currently being used as the CS for an SPI transaction.

@Rahix
Copy link
Owner Author

Rahix commented May 10, 2022

Well, the CS pin must be managed manually anyway so you can do weird stuff in any case... I don't think this changeset here would make it worse than it already is, or am I missing something?

@haennes
Copy link

haennes commented Jul 20, 2022

Hello,
I really need this for my project.
Is there any way I can help?
Should I fork it and then open a new PR?

@Rahix
Copy link
Owner Author

Rahix commented Jul 20, 2022

@haennes, giving me some feedback and testing this PR here is all I need. I just held off merging it because I wanted to hear from users first. Maybe you can comment on some of the open questions from the OP? What would be best for your usecase?

@haennes
Copy link

haennes commented Jul 23, 2022

It works on an Arduino Nano (from Elegoo) (Atmega m328p).
I will test it on an Arduino Uno R3 (from Elegoo) (Atmega 328p) too.
Although the 2nd implementations seems better at a first glance, the way it currently is is quite perfect, as it removes a lot of boilerplate code in the users project.

Comment on lines +229 to +236
let pin: Self = unsafe { core::ptr::read(self) };
let mut pin = pin.into_pull_up_input();
let res = f(&mut pin);
*self = if state {
pin.into_output_high()
} else {
pin.into_output()
};
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is still quite ugly and not 100% sound either. I quite like the approach in stm32f4xx-hal: https://docs.rs/stm32f4xx-hal/0.11.1/src/stm32f4xx_hal/gpio/convert.rs.html#476-490

This is at least more sound than my hack. So I'll try to reimplement that pattern here.

Copy link
Owner Author

@Rahix Rahix Oct 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source: https://github.com/stm32-rs/stm32h7xx-hal/blob/21d9c06a57169e92cf5745e582dca171c73294c9/src/gpio/convert.rs

It looks like this will unfortunately require a larger change to make it work. But I think following that route is still a good idea and will improve the remainder of the port module as well. Will just need a bit more time...

@hiszd
Copy link

hiszd commented May 18, 2023

Am I correct in saying that implementing the IoPin trait from embedded-hal would take care of this as well?
https://docs.rs/embedded-hal/latest/embedded_hal/digital/v2/trait.IoPin.html

@Rahix
Copy link
Owner Author

Rahix commented Jun 1, 2023

@hiszd, not really. IoPin moves the pin to change its state more permanently while the methods in this PR only change the pin state for the duration of a closure.

@RenjiSann
Copy link

RenjiSann commented Jul 14, 2023

Hi,

I just finished implementing a driver for the DHT11 sensor on an Elegoo Arduino Uno R3.
I had to use a pull up pin with temporary output mode to send the data request signal, and I can attest that it worked without any problem.

This might need a little refactoring before it is anywhere close to presentable, but it could eventually be integrated into the examples someday.

@jaqx0r
Copy link

jaqx0r commented Sep 23, 2024

With regards to the final bullet in the first comment, I think the user should not be responsible for returning a valid pin from the closure. We should assume the with* function takes care of maintaining state and correctness. I think that also makes it easier to return a pin's value from the closure in the case the pin was made input.

Is there a difference between changing mode with into_output_high() and calling set_high() on a low output? I think there isn't so a user can change state if they need after calling this closure with no return control.

jaqx0r added a commit to jaqx0r/bulbdialclock that referenced this pull request Sep 24, 2024
The original code sets pins to floating inputs in the `all_off` function,
which is not what this code was doing until now.

This change converts the activation function to move the LED pins to hi/lo
output when activating a specific LED, then moving them back to the pin array
when done, as deactivated.

This isn't exactly nice, and we eagerly await
Rahix/avr-hal#205 to land so this code doesn't use
`mode::Dynamic` overhead, or the unsafe pointer access.
@Rahix
Copy link
Owner Author

Rahix commented Sep 25, 2024

With regards to the final bullet in the first comment, I think the user should not be responsible for returning a valid pin from the closure. We should assume the with* function takes care of maintaining state and correctness. I think that also makes it easier to return a pin's value from the closure in the case the pin was made input.

If we switch to the design from stm32-hal I linked above, this can be done. The pin is only passed to the closure by (mutable) reference so there is no way the closure can move the pin to the outside world and break invariants.

I agree that a user-controller closure return type makes for a much nicer API.

Is there a difference between changing mode with into_output_high() and calling set_high() on a low output? I think there isn't so a user can change state if they need after calling this closure with no return control.

Yes, there is a difference. Calling into_output() and then set_high() leads to a brief glitch on the pin where it's state is LOW for a short time. In some applications, this can trip up external components. into_output_high() switches from high-impedance input mode to HIGH output without any glitches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hal-api API design for the different components of avr-hal hal-generic Related to MCU generic parts of avr-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Making a tri-state pin (switching between input and output), perhaps like embedded_hal::digital::IoPin
6 participants