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

Feature/hlist pins #748

Closed
wants to merge 6 commits into from
Closed

Feature/hlist pins #748

wants to merge 6 commits into from

Conversation

Ben-PH
Copy link

@Ben-PH Ben-PH commented Aug 22, 2023

Still in draft. Need to expand examples, and check a few things, but the core of the idea is there.
The highlight: compare the traditional way with the hlist-way

Essentially boils down to this:

Instead of managing pins through struct-semantics, it's more like a TupleSet, with some special properties:

  • indexed by type instead of position.
  • all operations are compile-time defined, so scales at O(0) runtime-complexity (to be confirmed)
  • similarly for memory footprint. (To be confirmed)
  • removing/adding entries update its type
    • no partial move issues.
    • Pins are initially owned by the initial collection
    • Pins can be moved moved at the type level to the scope where it's being used.

TODOs

  • Write keyboard example
  • Find name-aliases for names such as Plucker and HList that are more self-documenting.
  • Develop a pattern so that a device can be initialized with the pin-list only, eg, provide the IO struct to USB::hl_new, instead of selecting the correct 3 pins, then calling USB::new
  • Develop some SPI hlist_constructors to determine the burden of implementation (constructor, generic, marker, other approaches)
  • Make pin-generic pin-holders sane to initialize (difficult, probably)
  • Ensure the overhead is compile-time-only
  • Ensure the memory footprint and --release performance of the hlist-based patterns is equivilent to/lighter than the struct-based approach
  • Investigate impact on UX when debugging
  • Investigate impact of barrier-to-entry
  • validate the hypothesis of better performance through reduced need for indirection/dyn-dispatch
  • doc-comment (much) more thoroughly.

Must

  • The code compiles without errors or warnings.
  • All examples work.
  • cargo fmt was run.
  • Your changes were added to the CHANGELOG.md in the proper section.
  • You updated existing examples or added examples (if applicable).
  • Added examples are checked in CI

Nice to have

  • You add a description of your work to this PR.
  • You added proper docs for your newly added features and code.

@bjoernQ
Copy link
Contributor

bjoernQ commented Aug 24, 2023

Just had a very brief look - so just my first impressions, might be inaccurate or not thought through

  • it seems like this is just additional functionality but when using this, I don't see how to create any peripheral driver that takes pins (unless we would change each and every driver to use this)
  • using AnyPin is much more convenient in situations where the user doesn't want or can't use generics
  • this still needs generics in user code and introduces terms like Plucker and HList
  • if we would consider to change each and every driver to use this approach, we probably lose the ability to pass &mut instead of moving the pin into the driver

t.b.h. I don't think we should have this in the HAL (but I am happy to hear other's voices) - the approach is interesting in user code ... if we can do something to create such lists in user code that's certainly fine

As said - just a first impression and I'd like to hear more opinions

@bugadani
Copy link
Contributor

bugadani commented Aug 24, 2023

t.b.h. I don't think we should have this in the HAL

Apart from the tie-in to the macro that generates the initial list, everything should be doable in an external utility crate. That initial list seems to be, however, rather annoying to define outside of the HAL currently.

I only worry that not including this in esp-hal would make the feature practically invisible, as it's unlikely people search for the functionality.

I don't see how to create any peripheral driver that takes pins

It remains possible to pluck a pin and pass it to a peripheral. I believe the example is mostly useful to show an example user-defined wrapper. The HAL drivers could define a "pluck constructor" that take ownership over pins and pass them to the current constructors, but again, I believe this should be doable externally. Also, some peripherals, like SPI have way too many constructors already, doubling the number would be an API nightmare for newcomers to decode.

As I see it, the point is that users don't have to name the pin to move out of the IO struct (i.e. the struct field isn't hardcoded any more). This is a small advantage that is visible when supporting multiple hardware configurations in a single codebase.

@Ben-PH
Copy link
Author

Ben-PH commented Aug 24, 2023

Just had a very brief look - so just my first impressions, might be inaccurate or not thought through

* it seems like this is just additional functionality but when using this, I don't see how to create any peripheral driver that takes  pins (unless we would change each and every driver to use this)

As it currently stands, absolutely correct. I had this question in the periphery of my mind, but now that the initial draft is up, it needs to be front-and center. Something like this, I think, is needed:

    // Previously, USB::new would take 3 specific pins where the pin_list argument is.
    // These three pins are restricted with marker traits/structs, so pins can't get mixed up.
    // For example, esp32s3, the pins must be 18, 19 and 20 in arument 2, 3 and 4 respectively.
    let (usb, pin_list) = USB::hl_new(
        peripherals.USB0,
        pin_list,
        &mut system.peripheral_clock_control,
    );

inside hl_new, the io will pluck_pin, and the type system will understand that the only pins that are to be plucked, are the ones that have the right marker trait/struct generics. An impl might look a bit like this:

impl<'d, S, P, M> USB<'d, S, P, M>
where
    S: UsbSel + Send + Sync,
    P: UsbDp + Send + Sync,
    M: UsbDm + Send + Sync,
{
    pub fn hl_new<T, U, ???>(
        usb0: impl Peripheral<P = peripherals::USB0> + 'd,
        io: IO<T>,
        peripheral_clock_control: &mut PeripheralClockControl,
    ) -> (Self, IO<U>) 
    where 
        T: ???,
        U: ???,

    {
        let (sel_pin, io) = io.pluck_pin();
        let (dp_pin, io) = io.pluck_pin();
        let (dm_pin, io) = io.pluck_pin();
        let res = USB::new(usb0, sel_pin, dp_pin, dm_pin, peripheral_clock_control);
        (res, io)
    }

Currently, the type-system constrains the user such that if they choose the wrong pin-number, there will be a compile failure. The benefit I hope to introduce, is to be able to just pass in the pin-list, and the type-system will automagically select the correct pins, by way of the marker trait/structs, and to raise an error if the pins are unavailable.

* using `AnyPin` is much more convenient in situations where the user doesn't want or can't use generics

From my understanding, a specific pin must always be selected, but it can then be .degrade()ed into an any-pin? There shouldn't be any compatability issues, in that case. instead of selecting a pin based on io.pins.gpioN, you just move the N const into .pluck_pin::<N, _>(). You get access to exactly the same thing, but instead of using N to index by a struct-field, you use it to index into a type-construction.

* this still needs generics in user code [...]

The goal is for generics to only be needed by the user when they want to explicitly state a pin-number, or a pin property.

  • where the pin number can be inferred by the type-system via the way in which the pin is later used, generices are elided entirely (example below)
  • For pin-number, I would argue that .pins.gpioN and .pluck_pin::<N,_>() are semantically equivilent. one encapsulates the const-generic N in a field name, the other, by way of the type-system. if the turbofish syntax is problematic, a work-around that handles the majority of cases should be possible: io.select_pin_no(4), io.select_pin_property(MarkerStruct::default), for example.
  • when pin a pin needs to be selected based on marker trait/struct properties, the hlist approach provides the possibility to bypass the need to specify a pin, as demonstrated in the USB::hl_new example implementation. If you need the one pin that implements the Foo marker trait (which might vary across different chips), you could write let (foo_pin, io) = io.pluck_pin<_, Foo>().
use hal::some_module::UsesPinN

let (pin_n, io) = io.pluck_pin();
let thing = UsesPinN::new(peripheral.SOME_PERIPH, pin_n, &mut system.peripheral_clock_control);

in cross-platform code, where different esp32s use different Ns, this code would just work, bucause the type system would know which GpioPin in the hlist satisfies the type-constraints of the new

and introduces terms like Plucker and HList

I agree that this can be improved. An initial thought is to alias it to Selector and PinListByType for starters. I will try to minimise the more esoteric, and higher-barriar-of-entry concepts that come from functional programming.

* if we would consider to change each and every driver to use this approach, we probably lose the ability to pass `&mut` instead of moving the pin into the driver

the backing crate, frunk, has the ability to obtain mutable references instead of managing ownership through types. When you have a moment, could you please refer me to an example so I can make sure this issue is addressed?

t.b.h. I don't think we should have this in the HAL (but I am happy to hear other's voices) - the approach is interesting in user code ... if we can do something to create such lists in user code that's certainly fine

As said - just a first impression and I'd like to hear more opinions

Thanks for taking the time to respond and critique. I've added some TODO points based on your feedback.

@Ben-PH
Copy link
Author

Ben-PH commented Aug 24, 2023

t.b.h. I don't think we should have this in the HAL

Apart from the tie-in to the macro that generates the initial list, everything should be doable in an external utility crate. That initial list seems to be, however, rather annoying to define outside of the HAL currently.

Correct. I can look into initial list options as I move into the D of R&D

I only worry that not including this in esp-hal would make the feature practically invisible, as it's unlikely people search for the functionality.

Consider this "Vision" example:
#[entry]
fn main() -> ! {
    // `default_init()` could be defined at the bottom of the generated file. The nature of hlist means that it should
    // be inherently cross platform
    // let resource_hlist = default_init();

    // get a type-list collection of resources such as `peripherals`, `clocks`, `system`
    let resource_hlist = user_defined_init();


    // No shared resources unless explicitly required:
    //  - Can move pins into other structs
    //  - Pins can be owned directly by the peripheral device
    //  - This allows for easy seperation of concerns, and other maintain/readibility patterns.
    //
    // AppPeripherals would own each peripheral struct, which in turn would own their respective
    // pins.
    let (
        AppPeripherals {
            uart,
            usb,
            wifi,
            blinker,
            mouse,
        },
        resource_hlist
    ) = AppPeripherals::init_periphs(resource_hlist);

    // The type can be inferred through the pluck constrants, and the way the result is used
    let (io, resource_hlist)/*(IO<hal::InitialPinList>, InitialResources)*/ = resource_hlist.pluck();

    // pins used by these peripherals are owned by them entirely. No needing to satisfy the
    // borrow-checker with shared [mut] references, unless the nature of your app requires it.
    let conns = init_conectivity(uart, usb, wifi);
    run_app(conns, blinker, mouse, Some(io), resource_hlist)
}

^^^ that e.g. is a "far-future" thing, but it's something that could emerge out of efforts such as this one.

Of course, that could all be lsp-inlined, and it would look a lot like the init-code in the examples.

My hope and ambition

Allow a new design/implementation pattern that's considered maintainable, readable, "correct by default", contributes to lowering the barrier-of-entry, fun to use, and results in as significant an impact as it can merit. To give that dream the best chance of reality, I'm aiming for it to be included in the code base on the merit of its features, (potentially as a "proving ground" for upstreaming into embedded-hal). It's a long way off for now,

In the meantime...

I hope it makes sense to live in a draft PR so it can easily be kept up to date, iterated on, commented, etc. I welcome any suggestions.

I don't see how to create any peripheral driver that takes pins

[...] I believe the example is mostly useful to show an example user-defined wrapper.

Precisely. "All that's needed is for the type-system to infer the required pin". In this case: The blinky example approach of explicitly stating a const-generic value. Marker-traits/structs can also be used. Either user, or hal-defined.

The HAL drivers could define a "pluck constructor" that take ownership over pins and pass them to the current constructors, [...]. Also, some peripherals, like SPI have way too many constructors already, doubling the number would be an API nightmare for newcomers to decode.

I anticipate the vast majority of cases doing the following, which is basically just automating the ownership-moves that's currently forced onto the user:

  1. move ownership of the correct pin(s) out of the collection into a dedicated variable with some sort of pluck select_pin() (as opposed to .gpioN)
  2. call the currently implemented constructor with said pins.

I have some ideas to test/validate this, and added to the top-level todo.

As I see it, the point is that users don't have to name the pin to move out of the IO struct (i.e. the struct field isn't hardcoded any more). This is a small advantage that is visible when supporting multiple hardware configurations in a single codebase.

I'm still working on finding the best way to articulate the full set of potential advantages. Here's what I have so far:

  • As you mention, not having to name struct-field for a pin
  • You mention the type isn't hard-coded: Partially true. It's still hardcoded, but that's only to begin with, and only because that's baked into the nature of the hal. By design, it dynamically adapts itself in terms of both its type, and value (at compile-time), based on which pins are being claimed.
  • (tell me if I've missed anything)

There are other potential advantages, which might prove significant:

  • Take full advantage of Rusts (amazingly) powerful type-system through "type-indexed TupleSet-like semantics"
  • More cross-platform code: The option to select by marker-constraints, provides a lot of cross-platform potential.
  • Dependency injection semantics. With the ability to move in-out resources (leaving behind the consumed parts) enables a more idiomatic code-style.
  • Compatible with current code-base. So long as the $gpionum pin-numbers token can be used, there is no burden on the existing code base.

@jessebraham
Copy link
Member

Thank you for the contribution, I really am sorry that this fell off of my radar for so long.

I think at this point we will not move forward with this; the maintainers will soon be meeting to discuss redesigning some of our core APIs, include GPIO, and I think our goals may not necessarily align with these changes.

In the future perhaps this is something that can be revisited, however until we have made some concrete decisions regarding the future of our GPIO driver I do not think this makes sense right now.

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.

4 participants