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

don't touch pullups on reset while init PinDriver #344

Merged
merged 5 commits into from
Jan 31, 2024

Conversation

Vollbrecht
Copy link
Collaborator

addresses #343

@ivmarkov
Copy link
Collaborator

ivmarkov commented Dec 8, 2023

@Vollbrecht My only concern here is that in the ESP IDF code, they state that they don't leave the pin floating "for powersafe reasons" (whatever that means). Yet here, we would be doing exactly that. Thoughts?

@Vollbrecht
Copy link
Collaborator Author

we don't do that here, we use the normal Drop function for power safety - with a pull-up if we actually drop the Pin. But on transition /creating a new driver we use the function (that is essentially a replica) but don't touch the pullup's

@ivmarkov
Copy link
Collaborator

ivmarkov commented Dec 8, 2023

we don't do that here, we use the normal Drop function for power safety - with a pull-up if we actually drop the Pin. But on transition /creating a new driver we use the function (that is essentially a replica) but don't touch the pullup's

OK. So:

  • On driver initialisation, no pullup, no "glitch" and no "power-safety", but we probably don't need "power safety" when the driver is operational.
  • On driver drop - glitch

Are we OK with the glitch on driver drop?

@Vollbrecht
Copy link
Collaborator Author

Vollbrecht commented Dec 8, 2023

Well its depatable -

we don't do that here, we use the normal Drop function for power safety - with a pull-up if we actually drop the Pin. But on transition /creating a new driver we use the function (that is essentially a replica) but don't touch the pullup's

OK. So:

* On driver initialisation, no pullup, no "glitch" and no "power-safety", but we probably don't need "power safety" when the driver is operational.

* On driver drop - glitch

Are we OK with the glitch on driver drop?

Yeah in C land you are not forced to call the function at all - in that case the user has the choice what is happening. Here we kinda dictate how things should be run. So yes in that eye it probably would be nice as a user to have control over what kind of drop one wants.
I think for most users its a good default with the pullup enabled at the drop, if they plan to drop it and not reuse the pin shortly after. I am fine with it how it currently is, making it configurable would mean we need either a generic/trait or a runtime value we add so to differentiate here right?

@Vollbrecht
Copy link
Collaborator Author

Vollbrecht commented Dec 8, 2023

Though from an overall perspective it would only makes sense if the droped value has an owned Pin and not a ref Pin, because in that case we would allow the user to create a new driver, and he would potentially have the same problem again. That might be the case for someone or might not, and its hard to tell for every case. Definitely an opinionated choice here

@noahbliss
Copy link

noahbliss commented Dec 13, 2023

Just out of curiosity, if pins are pulled an opinionated direction by esp-idf when they are reset for "power saving reasons", why aren't they pulled an opinionated direction on boot? I only have this problem with pins I have specifically chosen to use/invoke the driver of.

Would it make sense to, in addition to this fix which fixes the bug on pindriver init, to make "drop" go into an undefined state (no bug, no pull), and add something like "drop_into_pullup" and "drop_into_pulldown" as other means of destruction?

EDIT: Realized drop seems to be something standardized outside of this crate. Perhaps this would be better specified as part of the pindriver initialization as an optional argument? Where the default behavior is to pull-up on drop (like the default esp-idf behavior) but an initialized driver could have a specified opinion on which way it pulls when it drops defined when it is initialized.

@ivmarkov
Copy link
Collaborator

@Vollbrecht Would you be willing to resolve the conflict so that we can merge this? (I think conflicts are due to the removal of the riscv-ulp-hal feature.)

ivan i am a sinner

clippy + fmt

clippy

more sinning in RTC
@ivmarkov ivmarkov merged commit 307bd3b into esp-rs:master Jan 31, 2024
8 checks passed
@ivmarkov
Copy link
Collaborator

Looks great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants