more Any* types

[dragazo]

Adds some more Any* types that I needed for dynamically-assigned peripherals in my current project. Note that some traits were changed from fn () -> T to fn (&self) -> T to facilitate this, so some of this is a breaking change.

[Vollbrecht]

thanks for the PR ! my question that i just throw in the room - Including the Input -& Output variants on the pin opens the scope a lot for the AnyPin. Wouldn't it maybe nicer if we just create something like an "AnyPeripheralPin" that can contain the specific Peripheral variants it is usable with, but gates the usage of such pin for normal gpio usage where the user should use AnyIoPin ?

[dragazo]

As in an enum over AnyInputPin, AnyOutputPin, etc.? I think that would be fine for the current version, but wouldn't include extra info like ADC/DAC/Touch since those are IO pins with extra features. I would have included stuff for those as well in this PR, but I couldn't find anything that actually used those - tbh I'm still not sure how to do those with this crate, which would be really helpful lol

[Vollbrecht]

could you elaborate a bit more what you mean by "dynamically-assigned peripherals in my current project"

[dragazo]

I just mean the peripherals, like i2c, gpio, pwm channels, timers, etc. in my project are configured at runtime based on a json file stored in nvs. I basically needed a pool mapping each index to a gpio pin, pwm channel, etc. that I can take from at runtime. But there didn't seem to be anything for that (for these things) since every individual pwm channel, timer, etc. are distinct types and cannot be boxed as their overarching trait object (and even if they could, it'd be less efficient). So I added more generic concrete types for them based on the AnyIntputPin and AnyOutputPin types that almost did what I needed (but only for the gpio pins).

The pool approach I used might be useful for others, but I figured that might be a bit against the grain for the rest of the crate. It's basically a [Option<T>; N] wrapper where T is a generic concrete type like AnyPin, AnyPwmChannel, etc. that binds all of said type of peripheral. It's basically like the existing Pins peripheral collection, but can be indexed at runtime instead of only compile time. I just construct a pool from the Pins collection directly, so it's like an opt-in dynamic indexing scheme.

[Vollbrecht]

I just mean the peripherals, like i2c, gpio, pwm channels, timers, etc. in my project are configured at runtime based on a json file stored in nvs. I basically needed a pool mapping each index to a gpio pin, pwm channel, etc. that I can take from at runtime. But there didn't seem to be anything for that (for these things) since every individual pwm channel, timer, etc. are distinct types and cannot be boxed as their overarching trait object (and even if they could, it'd be less efficient). So I added more generic concrete types for them based on the AnyIntputPin and AnyOutputPin types that almost did what I needed (but only for the gpio pins).

The pool approach I used might be useful for others, but I figured that might be a bit against the grain for the rest of the crate. It's basically a [Option<T>; N] wrapper where T is a generic concrete type like AnyPin, AnyPwmChannel, etc. that binds all of said type of peripheral. It's basically like the existing Pins peripheral collection, but can be indexed at runtime instead of only compile time.

hmmm, i feel like it would be nicer if you have some deserializer that directly crates the pins from your input.json, you are not bound to use the Peripheral::take() approach, technically you can unsafely create them by yourself if you made sure you didn't used them beforehand, and for the rest just use them as normal.

[dragazo]

But for a deserializer to work I'd still have to have a concrete type that any pin, pwm, timer, etc. can map to, so I'd still need the stuff in this PR anyway. Also even if I didn't use them elsewhere, the json could contain the same pin multiple times - it's user-provided, and the users are middle/high school students, so I can't trust them to configure things safely. So I'd have to do extra work after the fact to make sure each is unique, and checking that is similar to the pool approach anyway. But that aside, I also just like the pool approach since it involves no unsafe code (my project so far has no unsafe code - I'm actually pretty impressed Rust has so much safe tooling for embedded).

[Vollbrecht]

i definitely see your problem, and i am not saying that your proposal is a bad one. I just want to explore the complete option space, and think about all alternatives.
The main pain point is that peripherals like gpio's i2c's etc are not collections but field's in a struct correct? What if we create something like an alternative take() something like a take_as_collection() that returns something
that holds your proposed Collections but not as Options. For example all Gpios would live in an [AnyIOPin, N] and would be created by the take_as_collection call. Same with other peripheral. On creation of the array all gpio::new() variants would be created with gpioX::new().downgrade() and collected. This way we still have only one origin of all the peripherals, the cost would be that it is an either or decision. You would have the peripherals normally or as an peripheral_collection.
https://github.com/esp-rs/esp-idf-hal/blob/master/src/peripherals.rs#L149

This way you could itterate over the collections and find the pins you need. Than you could create your own System Peripheral Struct at runtime out of that

[dragazo]

That's kind of what I was thinking, but maybe slightly different. I was thinking the Peripherals object could be refactored to contain Pins (like it already does) for all gpio pins, and new structs called LedcChannels for all ledc channels, Timers for all timers, etc. (currently pins are all together like this, but others are just in the Peripherals root object). And these new structs would all be field-based, so it's equivalent to just regrouping existing fields into new organizational structs. But from there, each of these would have an into_dynamic(self) function that converts the entire collection of a specific kind (gpios, ledc channels, timers, etc.) into their dynamic version. So you could still Peripherals::take() all peripherals, and you can opt-in to making specific types of those peripherals dynamic (but it consumes all of that kind of peripheral to do so). For instance peripherals.pins.into_dynamic() or peripherals.timers.into_dynamic(). That's basically how my current version works.

It'd be great if we could just do the array version for everything like peripherals.pins[5] instead of peripherals.pins.gpio5. But sadly last I checked partial move semantics for arrays is not quite good enough for this (but fields work fine), so arrays will have to be opt-in.

[Vollbrecht]

That's kind of what I was thinking, but maybe slightly different. I was thinking the Peripherals object could be refactored to contain Pins (like it already does) for all gpio pins, and new structs called LedcChannels for all ledc channels, Timers for all timers, etc. (currently pins are all together like this, but others are just in the Peripherals root object). And these new structs would all be field-based, so it's equivalent to just regrouping existing fields into new organizational structs. But from there, each of these would have an into_dynamic(self) function that converts the entire collection of a specific kind (gpios, ledc channels, timers, etc.) into their dynamic version. So you could still Peripherals::take() all peripherals, and you can opt-in to making specific types of those peripherals dynamic (but it consumes all of that kind of peripheral to do so). For instance peripherals.pins.into_dynamic() or peripherals.timers.into_dynamic(). That's basically how my current version works.

It'd be great if we could just do the array version for everything like peripherals.pins[5] instead of peripherals.pins.gpio5. But sadly last I checked partial move semantics for arrays is not quite good enough for this (but fields work fine), so arrays will have to be opt-in.

I think instead of using an array we may should use something like an heapless vec https://docs.rs/heapless/latest/heapless/struct.Vec.html

[dragazo]

That would work and then we can take elements out of it without having to do the Option<T> thing. But it could make it a bit confusing in some cases - like if you do pins.remove(5).unwrap(), that's gpio5, but if you then do pins.remove(7).unwrap() that's gpio8 now. Of course, if we're not trying to act as a pool and just making it a collection that users can use to make their own pool struct if needed, then that's not a problem we have to address.

[ivmarkov]

Sorry for the delay here, I plan to look at this over the upcoming weekend.

[ivmarkov]

@dragazo Sorry for the delay. I'm finally looking into this as I (also) might need to introduce an AnyAdcPin instance (in addition to the existing AnyIOPin, AnyInputPin, AnyOutputPin ones) so as to make the usage of the upcoming ADC DMA driver more ergonomic.

What I don't understand is the following:
What is the notion of AnyPin you've introduced buying us? As in, why do we need it? The way I see it, currently the downgrading logic works as follows:

• your concrete pin -> downgrade_XXX() -> a generic AnyXXXPin, where XXX is IO, Input or Output

I think you suggest (in addition to what we already have):

• your concrete pin -> downgrade_any() -> AnyPin (your new type) -> try_downgrade_XXX() -> a generic AnyXXXPin, where XXX is IO, Input or Output

So my question is - what is the value of the AnyPin struct from your suggestion? It does not implement any of the peripheral traits (InputPin / OutputPin, ADCPin etc.)? In a way, it is only (a) delaying a potential future downgrade operation, and (b) weakening the downgrade operation to try_downgrade operation, because when the AnyPin instance is created, you inevitably lose the compile-time information as to what the original typed pin is capable of, hence the need to have the downgrades fallible, as the compile-type capability info becomes a run-tme capability info.

So if you can explain this use case as to why you need this extra level of indirection?

[ivmarkov]

Here's one speculation I have by reading the earlier comments: you are essentially implementing an ESPHome or - like - Tasmota - if you like - except in Rust. What these firmwares do is "delaying" the hardwiring of concrete hardware to concrete pins until runtime, because they are serving different hardware configurations (which the firmware user might have) with a single firmware.

Now this is a great use case I agree, yet a niche one. For all other use cases (and I would assume, these are like 99.9% of the use cases), the wiring of the external hardware to the ESP32 is a compile time thing, because the firmware is very specific to the use case. Now, the existing AnyInputPin, AnyOutputPin and AnyIOPin do not help you at all for delaying the wiring to runtime. They just simplify the firmware code for the few cases where you need to pass multiple input pins, or multiple output pins to your hard-wired peripheral. Your suggestion is not an extension of this idea I think; it rather plays in a league of its own - as in (I guess I'm repeating myself) - "let's delay the whole wiring thing for runtime".

If you can confirm that.

[dragazo]

Right, it's just so that we can opt into runtime checking. As it stands, that was impossible despite all the underlying APIs just using pin numbers under the hood. This PR just takes the concrete pins and wraps up their permissions/capabilities so that we can still have all the same checking as AnyXXXPin but at runtime.

[ivmarkov]

Is it possible to implement this functionality outside of esp-idf-hal? Even if it uses a little hidden unsafe {} here and there in its implementation? I fear that if we do it inside esp-idf-hal we have to do it for everything, which is quite a change, and quite a baggage...

[dragazo]

It would be possible but annoying to do outside of esp-idf-hal because of the permissions checking part. I added logic to the already-existing macros that generate the concrete types to just also take the permissions and use them to generate the dynamic wrapper. If you did that outside of esp-idf-hal you'd have to at least duplicate all of the permissions for every pin on every esp chip. That would basically come down to copying all of the macros from esp-idf-hal and replacing the existing definitions with a template to generate the dynamic one with permissions. And of course that would need to be updated for new chips independently of esp-idf-hal for any new targets down the road.

If rust would ever add generic specialization to stable there would be a better story for this, but currently it would be very messy.

[ivmarkov]

But your solution still seems incomplete. For one, I bet you currently have a very large match statement... in fact - multiple large match statements - one for each MCU - that match on an integer pin number, and then - depending on what the pin number is - moving a concrete pin instance out of the pins peripheral and then into- ing it into AnyPin.

I mean... if you have these large match-es, why bother with all the type safety and not just forget about the peripherals.pin stuff, construct your AnyPin out of thin air, and then hope that ESP IDF which is underlying the PinDriver anyway will catch and return an EspError if you try to e.g. set your AnyPin to input_output while it only supports output mode?

[dragazo]

Actually I don't have any kind of big matches for that. All I do is convert the concrete pins into AnyPin and store them in a collection: https://github.com/dragazo/netsblox-vm-esp32/blob/638de2e35f1c6da37b1150ae2882cb79b47eaf47/src/platform.rs#L134-L178. But even that would be avoidable if esp-idf-hal supported a dynamic version of Pins as mentioned before (but this PR doesn't address that).

I'll also clarify I'm perfectly fine just using my fork if this isn't something we want in esp-idf-hal proper. I was just going to contribute it back if anyone wanted it.

[ivmarkov]

Actually I don't have any kind of big matches for that. All I do is convert the concrete pins into AnyPin and store them in a collection: https://github.com/dragazo/netsblox-vm-esp32/blob/638de2e35f1c6da37b1150ae2882cb79b47eaf47/src/platform.rs#L134-L178. But even that would be avoidable if esp-idf-hal supported a dynamic version of Pins as mentioned before (but this PR doesn't address that).

Yeah. 48 lines of pins.insert. Same story. :)

I'll also clarify I'm perfectly fine just using my fork if this isn't something we want in esp-idf-hal proper. I was just going to contribute it back if anyone wanted it.

Sure. I'm just thinking loud if there is a way to not merge your PR (as it is very big yet a niche use case), yet with no or minimal changes allow you to get rid of these 48 lines, plus whatever you do for the other peripherals. And most importantly, avoid the need to fork esp-idf-hal, as keeping that fork up to date would be a constant hassle for you.

I think the absolute minimum for all things not GPIO is to ensure, that all Peripheral traits' methods do have a &self parameter, as you've patched it here: https://github.com/esp-rs/esp-idf-hal/pull/247/files#diff-fba755c8c1d90e3a768fa35f15d5eebfe2da5a47632f9ea06b755c4918f05be1R552

But once you have that, I'm still thinking you should be creating your peripherals out of thin air. Perhaps you can even create a DynamicPeripherals struct that creates these peripherals, and on constructor, takes the non-dynamic Peripherals one from esp-idf-hal. This way, for the external consumer of your dynamic peripherals those would be completely safe, as one won't be able to construct a dynamic version of - say - ADC1, while the static Peripherals::adc1 is still around.

[ivmarkov]

It'd be great if we could just do the array version for everything like peripherals.pins[5] instead of peripherals.pins.gpio5. But sadly last I checked partial move semantics for arrays is not quite good enough for this (but fields work fine), so arrays will have to be opt-in.

The other problem with an array of pins, i.e. peripherals.pins[] is that all pin structs in that array have to have the same type. In other words, you won't be able to statically distinguish between input-only capable pin vs input-output-capable pin anymore.

[dragazo]

Yeah. 48 lines of pins.insert. Same story.

Well an array isn't going to construct itself when this crate doesn't support that...

The other problem with an array of pins, i.e. peripherals.pins[] is that all pin structs in that array have to have the same type.

Well that's the point of this PR, as an opt-in option. Without this PR this is flat out impossible currently.

But once you have that, I'm still thinking you should be creating your peripherals out of thin air.

Well like I said, doing this outside of the macros in this crate would be an unmaintainable nightmare. Unless you're suggesting I forego even runtime capability checking, which I would never do.

This way, for the external consumer of your dynamic peripherals those would be completely safe, as one won't be able to construct a dynamic version of - say - ADC1, while the static Peripherals::adc1 is still around.

This PR already does that (not with ADC specifically, but the ones that I added like AnyPin etc.).

[ivmarkov]

Alright. I'll give it one last shot. :)

Yeah. 48 lines of pins.insert. Same story.

Well an array isn't going to construct itself when this crate doesn't support that...

But your PR does not address that either! Even with your PR applied, people in your situation still have to do the "48 insert lines" dance. Moreover, they'll have to do it per chip (i.e. the dance multiplied per the number of MCUs you have to support).

Hopefully now it is clearer why I was saying below that your patch is incomplete - because even with it applied, there are these boilerplate lines of insert that you are still doing and everybody willing to go the dynamic route has to do:

(My words earlier) But your solution still seems incomplete. For one, I bet you currently have a very large match statement... in fact - multiple large match statements - one for each MCU - that match on an integer pin number, and then - depending on what the pin number is - moving a concrete pin instance out of the pins peripheral and then into- ing it into AnyPin

The other problem with an array of pins, i.e. peripherals.pins[] is that all pin structs in that array have to have the same type.

Well that's the point of this PR, as an opt-in option. Without this PR this is flat out impossible currently.

(I'll skip that as this is not so important really and we'll just digress.)

But once you have that, I'm still thinking you should be creating your peripherals out of thin air.

Well like I said, doing this outside of the macros in this crate would be an unmaintainable nightmare. Unless you're suggesting I forego even runtime capability checking, which I would never do.

ESP IDF already does capability checking at runtime and will return an ESP_ERR_INVALID_ARG if you - say - call gpio_output_enable on a pin which is not output-capable. Maybe this is the case for everything else (ADC pins etc.). (I thought I have written that but maybe I missed?) I'm saying - you can create your pins out of thin air - nothing wrong with that - and then when you try to e.g. set a pin as an e.g. output pin and it is not capable of doing so, you'll anyway get a nice ESP error code which is your runtime check.

This way, for the external consumer of your dynamic peripherals those would be completely safe, as one won't be able to construct a dynamic version of - say - ADC1, while the static Peripherals::adc1 is still around.

This PR already does that (not with ADC specifically, but the ones that I added like AnyPin etc.).

Of course it does. I'm not saying it doesn't. Read the whole paragraph. I'm saying that you don't need the full scope of this PR to build a "dynamic", safe API. And one aspect of building this dynamic but safe API is that you have to swallow the typed Peripherals as input in your API, and produce a dynamic, untyped Peripherals equivalent as a result.

[dragazo]

Well I was planning on just adding the core AnyPin feature in this PR and someone else would add the dynamic pins collection however you would want it if you wanted it at all. But whatever, I just added a commit for that too. It's just a fn(self) -> [AnyPin; _] method on Pins. This also has the side effect of reducing redundancy since Pins is now defined by the same macro that defines the individual pins. We could do a heapless::Vec instead, but there was not precedent for it, so I opted for a standard array. Or we could do a smarter collection that has things like fn take(&mut self, pin: i32) -> Option<AnyPin> that does searching by pin number. These are the decisions I was trying to avoid making myself. But regardless, any collection is easy to make from an array.

[gmalette]

I'm finally looking into this as I (also) might need to introduce an AnyAdcPin

Until this happens, how does one create an array of ADCPins on the same ADC? Is it possible at all?

[dragazo]

@gmalette I believe without something like this PR, that is currently impossible. Granted, I don't think I included anything for ADC specifically (I kind of just added what I needed at the time), but it would probably be an easy addition. I can look into adding it when I get some time, but probably as a separate PR if this one gets accepted.

[ivmarkov]

I have mentioned in the comments above that this PR has a much bigger scope than AnyXXXPin. A scope that is niche nevertheless. I have suggested an alternative which has a smaller change surface, but the author does not seem interested.

AnyAdcPin can be introduced separately.

[dragazo]

Well by "something like this PR" I just meant another PR that just adds Any* stuff, as this one does, but this PR is not needed for that. @ivmarkov I actually addressed your most recent points (like the manual inserts thing) in the most recent code changes. It's not that I'm not interested in getting this accepted, but anything less than a completely generic AnyPin doesn't work for my use case. I'll also note that this isn't a breaking change (aside from the minor trait changes I mentioned in the first comment, but those would be needed for any more specific AnyXXXPin anyway, which this library already supports in some, but not all, cases), and indeed this PR just adds more dynamic options should users need them.