Allow Pull Without Output Requirement

[noahbliss]

Being able to pull up or down doesn't necessarily require being able to output. This change allows better migration from other hal projects such as esp-hal where this kind of requirement is similar. While this does relax the gate on what kinds of pins can pull, it allows developers to write stricter functions when passing pins around.

[Vollbrecht]

This is not a correct solution, as it would only allow it to set if its in the state InputPin. We need a solution where both are possible

[noahbliss]

Hmm darn. It looks like esp-hal just has separate function implementations for this inside the pin. Honestly I much prefer the approach they've taken with pins as drivers.

[noahbliss]

Are there any pins under the "Pin" trait that aren't GPIO capable?

[Vollbrecht]

If you ask to use it as a pullup / pulldown indicator, not every pin has pullup, pulldowns, but pin is generally used as a marker trait for all gpio's

[noahbliss]

Playing devil's advocate, but wouldn't the current logic of "all pins capable of input or output" be equally as capable of including pins without pull capabilities?

This change is valuable for the reasons listed in the first message as well as for making the downgrade_input() and downgrade_output() result types useful. I'm not really sure what the point of them is otherwise.

That is to say, the type AnyInputPin, isn't something really worth using currently since functions that would be typically used against that type (like set_pull) don't work against it presently.

I changed the code to require the trait of "Pin" and at least in my code, am experiencing the intended functionality with no ill-effects.

[ivmarkov]

Being able to pull up or down doesn't necessarily require being able to output. This change allows better migration from other hal projects such as esp-hal where this kind of requirement is similar. While this does relax the gate on what kinds of pins can pull, it allows developers to write stricter functions when passing pins around.

Do you have a concrete problem at hand which does not work with the current code?
The reason the code is implemented this way is because - at least on the ESP32 - there is no pin which has pullup/pulldown resistors and which is input only. Or to rephrase - all pins wich are input+output capable have pullup/pulldown, and the other way around.

If the other MCUs are not like that, we should revisit though.

As for your relaxed : Pin - that's not OK, because this way you'll be "pretending" to set pull up / pull down on the input-only ESP32 pins that don't have these resistors: https://linuxhint.com/esp32-pull-up-pins/

This change is valuable for the reasons listed in the first message as well as for making the downgrade_input() and downgrade_output() result types useful. I'm not really sure what the point of them is otherwise.

That is to say, the type AnyInputPin, isn't something really worth using currently since functions that would be typically used against that type (like set_pull) don't work against it presently.

... which has the problem I outlined above though - you can call set pullup/pulldown on pins that don't have these.

Why are you just not using AnyIOPin?

[noahbliss]

Yeah, my issue is that I have a function which needs to accept any and only "inputs" as a parameter, and I am trying to change the pull of this pin as part of the function. If someone were to inadvertently pass this function an output, things would break. I'd like to reiterate that I don't understand the purpose of even having the AnyInputPin trait if functions which are supported by input pins cannot be run against pins which have this trait.

Additionally, while I understand and agree that it is a problem that set_pull can be called against pins which cannot internally pull, this is already a bug that exists in the current unchanged code, for example, against gpio1 in your link. My change would potentially expand that existing peoblem to include input only non pulling pins, but does at least allow AnyInputPin to behave closer to expectations.

To recap, there are two issues:

1. AnyInputPin pin can't do things it should be able to. This change helps this.

2. set_pull can be run against pins that can't pull. This bug already exists in the current code and is not resolved by mine.

[ivmarkov]

Yeah, my issue is that I have a function which needs to accept any and only "inputs" as a parameter, and I am trying to change the pull of this pin as part of the function. If someone were to inadvertently pass this function an output, things would break.

I do not understand your reasoning. On one hand, you want the type to enforce that the pin is input only, but on the other hand, you seem to be fine that - with your change - folks can call pullup/pulldown functions on pins that don't really have that functionality.

I think we would be much better off by introducing two additional traits: PullUp and PullDown (or maybe just a combined one as the ESPs seem to always have none or both resistors), and then using these (this) traits in your type contracts. BTW and see below - you are trying to (ab)use AnyIOPin and AnyInputPin where you should be using generics IMO.

I'd like to reiterate that I don't understand the purpose of even having the AnyInputPin trait if functions which are supported by input pins cannot be run against pins which have this trait.

AnyIOPin AnyInputPin only exist so you can take a bunch of these in an array/vec. They don't have other purpose in life.

Also, you can use AnyInputPin as long as you don't have to change the pullup/pulldown setup of the pin. Or to put it another way, AnyIOPin models pins that are all of input + output + pull up/dpown-capable. AnyInputPin models just pins which are input-capable, and where we are uncertain if they have pullups/pulldowns.

I think at the bottom of all of this is you trying to write your function as:

fn my_f(pin: AnyInputPin)

... and somehow have AnyInputPin modeling all the aspects of input pins.

... where you probably want

fn my_f(pin: impl InputPin + PullUpDown) // The new trait

Additionally, while I understand and agree that it is a problem that set_pull can be called against pins which cannot internally pull, this is already a bug that exists in the current unchanged code, for example, against gpio1 in your link.

Nit-picking? gpio1 is missing from the above pinout, but seems to be listed as a regular, all-capable pin here.

Hence I've included it as a "normal" pin. If it is not, the easiest thing is to special-case it.

[noahbliss]

fn my_f(pin: Impl InputPin + PullUpDown) // The new trait

This would be acceptable.

[ivmarkov]

fn my_f(pin: Impl InputPin + PullUpDown) // The new trait

This would be acceptable.

If you want it sooner rather than later, you can re-arrange your PR in this direction then.

[noahbliss]

It seems I could possibly avoid this as well by requiring pindrivers as the function inputs rather than pins themselves. Like below:

async fn hwio(
    dri_input: PinDriver<_, AnyIOPin, Input>,
    dri_output: PinDriver<_, AnyIOPin, Output>,
) {

Would this approach be the preferred way to distinguish between Inputs and Outputs for the sake of functions?

Unfortunately since I am running embassy which requires #![feature(type_alias_impl_trait)] it appears this will not work for my use-case as the PinDriver includes _. I haven't found a way around this yet.

[ivmarkov]

It seems I could possibly avoid this as well by requiring pindrivers as the function inputs rather than pins themselves. Like below:

async fn hwio(
    dri_input: PinDriver<_, AnyIOPin, Input>,
    dri_output: PinDriver<_, AnyIOPin, Output>,
) {

Of course. Or better yet - and with generics - dri_input: impl InputPin (where InputPin is the e-hal1 "driver" trait, NOT the esp-idf-hal "peripheral" trait). In the end, you have to choose whether you want/need access to the peripheral or just to the driver on top of it. And you have to distinguish these two concepts. In the embassy HALs, they are well-distinguished, also for GPIO. Ditto for esp-idf-hal. Not so sure about esp-hal.

It is another topic that embassy-executor - as far as I'm aware - has issues taking generics. For good reasons. It is another reason topic why you insist on using ermbassy-executor on top of ESP-IDF which does allocate. And you seem to be using esp-idf-hal types directly in your code (i.e. no effort to abstract anything). So if you (a) are OK with allocations (b) are hard-wiring all of code to esp-idf-hal/svc, why are you insisting on embassy-executor which has these restrictions w.r.t. generics precisely because it is no-alloc?

Would this approach be the preferred way to distinguish between Inputs and Outputs for the sake of functions?

See above. It boils down to whether you want the peripheral, or the driver on top. e-hal BTW abstracts "drivers", not "peripherals".

Unfortunately since I am running embassy which requires #![feature(type_alias_impl_trait)] it appears this will not work for my use-case as the PinDriver includes _. I haven't found a way around this yet.

Just use 'static where you currently put _.

[noahbliss]

I appreciate the insight you put into this reply, I'll more carefully plan the approach here. In this case, should we close this PR? Anecdotally there may still be a purpose for this, but it would likely be a pretty niche-case right?

[ivmarkov]

I appreciate the insight you put into this reply, I'll more carefully plan the approach here. In this case, should we close this PR? Anecdotally there may still be a purpose for this, but it would likely be a pretty niche-case right?

Yeah. See, we used to have PullUp/PullDown back in time and at some point I removed those. Don't remember why. Turns out these could be useful if you want to constrain your peripheral to be only input + pullable, right?

But not the most important thing in the world; definitely niche. I'm fine either way. Perhaps you can try to experiment a bit in your code, see how it goes and only then decide if you want to resurrect something like this.

[noahbliss]

I'll close this for now and rework my approach. It'll be here in the closed PRs/search if someone else gets to it before I do, either way, this code is getting tossed and the trait would need to be added. Thanks!