PinDriver::output causes gpio to unexpectedly go high

[noahbliss]

Just continuing the conversation from element.

When using the PinDriver::output against a pin to create the pin driver, the pin state of that pin briefly becomes high. For boards where the esp32 is not the main IC and is sharing resources or interacting with external systems which are input sensitive, this is very dangerous behavior.

The following code can be used to demonstrate the behavior: https://gist.github.com/noahbliss/d86fcf8f64b6fa901330656636be31c3
To reproduce:
Modify the gpio pin to one which matches and led on your board.
Flash the board.
Disconnect power or reset the board.
Observe that the led briefly flashes despite never being told to go high.

Tested on the freenove esp32-s3 wroom developer module as well as on esp32-s3 custom hardware.

Unless I am mistaken, it seems that this drop is to blame because it subsequently calls a pin reset.
gpio.rs line 848

It also seems this might have been mentioned in a past issue, albeit for a different reason: #217

[Vollbrecht]

if you suspect that the drop call is the culprit, can you test it out what happens if its not called? you can simply use [patch-crates.io] in your Cargo.toml to point to a modified copy of esp-idf-hal

[noahbliss]

commenting out drop still allows it to compile, but it quite thoroughly breaks gpio it seems.

[noahbliss]

Alright, seems I've run this down, summary below:

I am using esp-idf v5.1.1
It looks like the PinDriver, as part of setting up an output, calls drop() which itself calls gpio_reset_pin() from esp-idf.
This function in esp-idf (located here) cause the pin to be pulled up as part of the reset, causing an undesirable output which can cause some severe damage depending on what the gpio is connected to.

As a short-term for my specific use-case, I simply replaced this with a pull-down instead of a pull-up, but think that a better long-term solution would be to replace this drop(self); in the PinDriver with a gpio configuration which either doesn't pull the pin or allows users to specify which direction to pull the pin.

[ivmarkov]

Alright, seems I've run this down, summary below:

I am using esp-idf v5.1.1 It looks like the PinDriver, as part of setting up an output, calls drop() which itself calls gpio_reset_pin() from esp-idf. This function in esp-idf (located here) cause the pin to be pulled up as part of the reset, causing an undesirable output which can cause some severe damage depending on what the gpio is connected to.

As a short-term for my specific use-case, I simply replaced this with a pull-down instead of a pull-up, but think that a better long-term solution would be to replace this drop(self); in the PinDriver with a gpio configuration which either doesn't pull the pin or allows users to specify which direction to pull the pin.

What you suggest would be a half solution only, as if you decide to drop the driver itself, you'll get the very same glitch.
So I wonder, maybe we need a different reset routine altogether?

[Vollbrecht]

well @ivmarkov i already addressed that in the linked PR?

[jobafr]

So I wonder, maybe we need a different reset routine altogether?

What about saving the previous pull-up/down state as a private field of PinDriver on initialization, and restoring that state on drop?

---

I think any unexpected behavior on PinDriver creation could be avoided by making the user of the API choose the pull mode explicitly, e.g. by having a required Pull argument in the input and output methods, or by giving PinDriver a generic argument (with no default value) to the same effect.

The current API enforces at compile time that .set_pull() can only be called on pins that do actually have internal pull resistors available, which is good. I'm not sure whether it'd currently be possible to use the type system so that the user
a) must choose a pull mode for those pins that support one, and
b) must not choose one for the pins that don't, or alternatively, must explicitly "choose" Pull::Floating.

[noahbliss]

There might not always be a previous state however. Personally I would be for having a parameter as part of initializing the pin that requires defining the "reset" and "drop" direction. That is, if dropping a pin/resetting a pin forces the pin to go high or low, we should be able to pick which direction it goes.

[noahbliss]

So it seems this was merged but we are still waiting for it to be released before updating via crates.io? Would pointing to master right now include the fix?

[ivmarkov]

Of course?

[noahbliss]

I guess I am confused then.

If master is treated as a prerelease branch, why is this issue closed? Shouldn't it remain open until the next release since the issue is still present in the latest release of the crate? Just trying to understand dev life cycle here.

Is there an ETA on the next release?

[ivmarkov]

We close the issue as soon as it hits master, or shortly after. That's what a lot of other projects do as well, with GH's simplistic issue tracking system.

As for ETA - perhaps by end of this month latest, if not earlier. Is there any reason, if you really need this ASAP, not to use [patch.crates-io] as most folks do?

[noahbliss]

Cool, thanks for the insight!