[RFC] Change the way pin types are generated

[hannobraun]

This issue was first opened in the old nrf52-hal repository. Original discussion: jamesmunns/nrf52-hal#8

---

Most of the code in the GPIO module is generated using a macro, including all the types that represent pins. Through my experience working on lpc82x-hal, I've come to regard this as an anti-pattern.

To be clear, I'm not suggesting that we don't generate code using a macro. I'm suggesting that we minimize the amount of generated code, using it only to feed platform-specific bits into generic, hand-written code.

But more on my suggestion later. First, the reasons on why I regard generating everything (specifically all pin types) as an anti-pattern:

1. It makes the code hard to read. If you're unfamiliar with the code and need to make sense of something, you constantly have to correlate the code in the macro, to the macro's argument list, to the macro invocation.
2. The generated pin types represent two different concepts: Which pin this is, and what mode it is in. Combining these two things in the way that is done with the pin types violates the single responsibility principle.

Argument 1. is fairly self-evident, but argument 2. might seem a bit a bit esoteric. It does have real consequences, however. Consider a wrapper type around a pin that is generic over the pin, but specific about the state. Currently, this would require higher-kinded types to express:

struct PinWrapper<T>(T<Output<PushPull>>);

Since higher-kinded types don't exist in Rust, you'd have to use the generic pin type, making the wrapper type less type-safe than it could be:

struct PinWrapper(P0_Pin<Output<PushPull>>);

See dwm1001 for a real-life example of this problem.

I've encountered this same problem with lpc82x-hal and came up with the following solution:

1. Pull as much code as possible out of the macro, into generic, hand-written types.
2. Use traits to abstract over the non-generic parts of the implementation.
3. Use a macro to generate types that implement those traits.

This would look something like this. First, the generic Pin type:

struct Pin<T, Mode> { ... }

Pin relies on traits to do whatever it needs to do:

impl<T> Pin<T, Output> where T: PinTrait {
    pub fn do_stuff(&mut self) {
        T::do_stuff();
    }
}

A code generation macro would still exist, but it would only generate the bare minimum: Types like P0_12 and their trait implementations. lpc82x-hal has several implementations of this pattern: Pin, Function

Now type of pin and mode of pin are cleanly separated, and all combinations can be cleanly expressed:

struct FullyGeneric<T, Mode>(Pin<T, Mode>);
struct TypeGeneric<T>(Pin<T, Output>);
struct ModeGeneric<Mode>(Pin<P0_12, Mode>);
struct FullySpecific(Pin<P0_12, Output);

P0_Pin and degrade would no longer be necessary.

There's one drawback that I never found a good solution for: Whereever you implement this pattern, you end up with a struct, e.g. Thing and a trait that really wants to have the same name. I've been calling these traits ThingTrait instead. This is not pretty, but okay. I'm open to any suggestions!

[jscarrott]

@hannobraun I have to agree with you on this. I found the pin generation macro currently to be very confusing at first. I'll possibly have a look at this, this weekend unless you have already started work on it?

[hannobraun]

@jscarrott Go ahead! I haven't started working on this, and don't currently have any plans to (many things to do).

[nickray]

There's one drawback that I never found a good solution for: Wherever you implement this pattern, you end up with a struct, e.g. Thing and a trait that really wants to have the same name. I've been calling these traits ThingTrait instead. This is not pretty, but okay. I'm open to any suggestions!

@hannobraun One thing I'm trying out in general is collecting traits in separate modules - after all, they should stand on their own (a bit like C++ header files), vs. the usual approach where everything remotely to do with a certain peripheral ends up in one rather long file. And then since they're namespaced, you can of course do

mod traits {    
    pub trait Example {}    
}
struct Example {}
impl Example {}
impl traits::Example for Example {} 

Locally, can use trait::Example as E or as ExampleTrait, or whatever is appropriate.

In the specific case of pins and your PinTrait, I just call it PinId though: https://lpc55-hal.netlify.com/lpc55s6x_hal/drivers/pins/struct.pin. Another option I considered is Which[Pin], but... ;)

One other thing I never understood is why Pins should be a "part" of some "split" peripheral - they're conceptually something else, really. Usually, two or three peripheral register blocks are involved in configuring and driving pins anyway, so who's the natural "owner"?

Generally, I feel like there are a bunch of cross-HAL-generalizable traits that are waiting to be "discovered", for both startup code and (as in the doc example above) "(mini-)driver" code of these non-peripherals "things", such as clocks and pins. I am quite pleased with the fact that there's one place in documentation to both lookup all one can do with a specific pin, or find all pins one can do a specific thing with (e.g. be used as TX for SPI3).

[hannobraun]

Hey @nickray, sorry for the late reply. RustFest messed up my routines pretty badly, and I'm now working through my inbox :-)

I like your suggestion of having traits in a separate module. I have to try that out here or in lpc8xx-hal.

One other thing I never understood is why Pins should be a "part" of some "split" peripheral - they're conceptually something else, really. Usually, two or three peripheral register blocks are involved in configuring and driving pins anyway, so who's the natural "owner"?

I totally agree. In lpc8xx-hal this is one of the most confusing things about the API, currently.

[bradleyharden]

I'm not sure why this issue is still open, but since it is, I thought I would comment. I came here from #215. It looks like the solution I came up with for the ATSAMD HALs is exactly what you're describing. I reviewed my proposal in this post, and the whole module is available here. I thought it ended up being a really elegant solution.

[hannobraun]

I'm not sure why this issue is still open, but since it is, I thought I would comment.

This has never been implemented (or rejected), so why wouldn't it be still open?

I came here from #215. It looks like the solution I came up with for the ATSAMD HALs is exactly what you're describing. I reviewed my proposal in this post, and the whole module is available here. I thought it ended up being a really elegant solution.

I'm not involved with ATSAMD, but for what it's worth, I think the solution you came up with looks great (and the write-up too)!

[bradleyharden]

Thanks.

And I saw a merge that seemed to be related, so I just assumed it had already been implemented. It definitely seems like a good idea.