Gpio Alternate Functions

[DrTobe]

Most peripherals require pins to be configured in a specific alternate function configuration with a specific output type (push-pull vs. open-drain). Until now, these requirements have been specified next to the peripheral implementation itself, e.g. in serial.rs or i2c.rs. This has been fine until now but this would get messier over time as soon as we start to properly work on #29. Therefore, I have thought about how to restructure this and this PR contains the best solution I could come up with so far.

The new alternate_functions module now contains all the traits that the pins for the different peripherals depend on, e.g. SclPin, SckPin, ... . Additionally, gpio.rs now contains two traits AlternatePP and AlternateOD which are obviously implemented on pins in Alternate<AFx, PushPull> and Alternate<AFx, OpenDrain> configuration. The peripherals then specify requirements for their pins by combining those two. For example, the I2c<I2C1, (SCL, SDA)> peripheral requires SCL: SclPin<I2C1> + AlternateOD. Before, most peripheral implementations impl'ed their traits directly on a pin with a specific output type. This was ok before but this change now allows us to handle all mappings between pin-af-combinations and peripheral traits in a consistent way.

Now that all alternate function mappings can be handled consistently, this allows to implement those in a central place (the alternate_function module and its feature-gated submodules). To do so comfortably, I have played with Rust's macro system until I was able to impl a whole table of alternate function mappings (as found in the datasheets of the different MCUs) in a single macro call. The bunch of macros to do so is not the most readable code I have ever seen and I do not know if there is a simpler solution to do so, but at least this is only done once now and only in a single place. Feature-gating more devices is extremely simple now because it just requires us to:

1. Add a new feature to Cargo.toml
2. Copy the alternate function tables from the datasheets
3. Make sure that the peripherals are enabled

The last step still requires us to go through all peripheral modules but especially the second step has been simplified a lot. If someone only wants to contribute by copying the alternate function tables, they can do so without breaking anything.

So please let me know what you think about this approach and if you see any potential for improvement, especially for the macros. If you are fine with eventually merging this, I will try to at least copy the trait impls for the "fallback features" so that we do not lose anything there and clean up all the files where the old stuff is currently still kept for reference.

[DrTobe]

Hi, I would really like to get some feedback on this topic to either finish or abandon it. If there is anything missing to evaluate the idea of the approach, please let me know.

[korken89]

I think we should look at merging toward #263 maybe.
It seems like the const generics interface is a lot simpler.

[DrTobe]

So you mean finalizing #263 first and revise the alternate function requirements for peripherals afterwards?

[DrTobe]

I will wait for the outcome of the discussions of #261 and #266 before adjusting this code for the newest changes again.

[korken89]

I think with the const generics in this might be closed now?

[DrTobe]

I have updated the code so it builds again at least.

Although this pull request has been heavily affected by the changes introduced with the const generics, its goals are different: What I am trying to achieve here is to replace the complicated alternate-function mappings spread over all the different peripheral implementations (for example what happens in i2c.rs) by a consistent approach with a single module for each MCU, following the tabular approach from the datasheet (this has been shown in stm32l452.rs).

I think that this is still valuable, especially now that we have a single device feature for each MCU. This allows to "just copy" the tables from the datasheet for each MCU which seems to be much easier and less error prone than the previous approach.

It is still a considerable amount of work to reach just the previous state because I have currently not implemented any alternate-function mappings for any other device. But I still do not know if this would finally be accepted (it changes a lot of the design and uses hard-to-understand macro-magic) so I hope to get your opinion on this.

[korken89]

I'm having issues with understanding the main benefits, is there someplace special I should look to gain better understanding?

I gave the traits and bounds a cursory look, and I directly found something that is an issues that does not have a direct solution to me.
When looking at the trait bounds I commonly find that the pin type is specified (e.g AlternatePP), but in many cases we can't make this bound.
For example PWM, we don't know if the user needs it to work in Push Pull our Open Drain, but the interface forces us to use Push Pull here:

pub fn channel1(self, _ch1_pin: impl PwmCh1<TIM> + AlternatePP)

Is there some way to get around this design choice other than having many different new?

[DrTobe]

Is there some way to get around this design choice other than having many different new?

Yes, we could just remove the push-pull/open-drain requirement. So the following would allow to call the method either with the Alternate<OpenDrain, _> or the Alternate<PushPull, _> pin:

pub fn channel1(self, _ch1_pin: impl PwmCh1<TIM>)

I'm having issues with understanding the main benefits

I was inspired to implement this, when I tried to add alternate-function pin-mappings for the stm32l452, which we have a local fork for. To add the pin-peripheral-combinations that are additionally supported by a specific chip, one needs (at least) to add those

• in i2c.rs with this macro
• in serial.rs with this macro
• in qspi.rs with this macro
• in spi.rs with this macro
• in tsc.rs without any macro
• and in can.rs with this macro

This is scattered over six different files while even using different approaches sometimes. With each new device we support there, each of these files gets messier. To be honest, I have not found a good approach to find out which pin mappings are missing for my specific device, so just adding a few of these has been a considerable amount of work without any easy way to verify that a) nothing is missing and b) every pin assignment is right.

As an alternative, I suggest adding one module per device as shown in stm32l452.rs which does all this for a single device, and only for this device. Then, we would need one person per device to copy the alternate function mapping table from the datasheet and this person would not have to work in six different files with different macros. This would simplify supporting those different device features a lot, in my opinion.

[Crzyrndm]

I will lend my support to having all of the alternate function mappings in one location per device. The current layout of in module definitions requires mixing #[cfg(...)] in with the impl declarations which is not immediately clear when reading (i2c.rs really stands out in my memory for some reason, but the same applies to the others).

The syntax involved I'm less sure on (take with a grain of salt, only using the web view so far) but I can see the rationale of trying to match what is in the data sheets. Without the knowledge to see what can be done stylistically with macros, I will refrain from bikeshedding.

[DrTobe]

Agreed, the syntax is not so important. It appeared to be reasonable to match the tables from the datasheet because I thought this would simplify verification for reviewers. In general, I would definitely be open to other syntaxes but I do not think that this is the important issue here. Rather, this is more about the decision where and how (by peripheral or by device) those alternate function mappings will be grouped.

[Crzyrndm]

I've been mucking around with this for a bit today and I feel confident now in saying that the table format is a notable downside because there's no strong link between the alternate function index (AFx) and the mapped alternate function. Because of this, the table has to be manually formatted to make any sense (and this occurs frequently while editing). More than once I let the formatting get out of sync and then placed the alternate function mapping in the wrong column, and I have only done GPIOA for a single MCU so far.

My conclusion from this is that any macro should focus on either a single column (AF2), row (PA0), or peripheral (ADC1 / ADCx). Peripheral and column end up being almost the same thing so after a bit of twiddling this is what I came up with
https://github.com/Crzyrndm/stm32l4xx-hal/blob/gpio-alternate-l476/src/alternate_functions/stm32l476.rs#L22-L91

The result is significantly more line verbose, however is both rustfmt compatible and has a strong tie between the AF index and the mapping which (for me) makes it easier to edit and review (just run down the relevant column in the datasheet)

(the peripheral version I'm not 100% sold on. It could remove a lot of copy-paste and the issues this presents edit/review, but at the same time it feels too far removed from the trait. Leaving it mostly for potential future inspiration)

It would of course be quite easy to do a row(pin) based version of ^^, however the way I see this coming up most often is by peripheral (what pins can I use to make do what I want). The datasheet does have a pin to function table, however it doesn't specify the index of those functions which would make it a real pain to maintain/review with. provising both row and column is just going to end up with conflicting definitions so doing both is probably only going to end in frustration

[Crzyrndm]

Taking a bit of a step back, either approach involves a very significant amount of manual work (25 MCUs by avg 100 pins by say 5 AF per pin). That leads to the simple conclusion of generated AF mappings (for which macros are just fluff)

These mappings are not in the SVD files, from what I know the only official source is the matching datasheet (which is a pain for a variety of reasons...). . However @Dirbaio has (again) been doing interesting things and has a script which extracts a lof of datasheet info (including AF mappings) here: https://github.com/embassy-rs/stm32-data (STM32L476RG as example)

Using these as inputs, writing out the two lines for each AF mapping will be a relatively trivial task

EDIT: Ignoring L412/22, AF mapping counts are between 300 and 800 per MCU (mostly 500+). Definitely not something you want to be doing by hand. I have a very rough script which rips through the data linked ^^ and creates files that look like

(The trait name "CAN_RX" being a simple combination of the peripheral and signal names)

[DrTobe]

In my opinion, this last proposal is what we should definitely do. I just did not know that there are already machine-readable files available which is just the precondition to avoid all this cumbersome manual work.

I am currently wondering, what would be the best way to organize this? Auto-generating these mappings resembles what svd2rust does. So would it be reasonable to have these mappings generated in their own package which stm32l4xx-hal will depend on?

[Crzyrndm]

pin definitions are currently part of the HAL, not PAC, so that might prove difficult (at a rough guess, you'd have to break off the gpio module, not just the AF mappings)

[DrTobe]

You are right, that might be at least difficult. I was just thinking that this would seem cleaner to me than to have a separate script lying around here. So let's assume it was possible, would you prefer that kind of separation?

[Crzyrndm]

my lazy take would be to throw a generator script in the build.rs under an undocumented feature gate with a decent block of documentation (it seems unlikely it'll need to be run again, but keep it around...). Could also drop it in tools/ (and document that you need to add the --target=<host> option when building)
my "this is the way I would like to do it" would probably indeed break GPIO off into it's own crate (along with several other modules. not much point in doing just one and getting PAC -> GPIO -> HAL). That does create publishing/maintenance issues though since you can't (yet?) publish a workspace as a single public crate, and the generate script is still going to end up in build.rs or similar

[DrTobe]

Today, I have tried out if it is possible to only define the peripheral pin traits (e.g. I2cScl, I2cSda, SpiScl, ...) and their pin-af-combinations in a dependency library and having the actual pin structs/types in another crate which uses that library. To do so, I introduced pin number traits (e.g. pub trait PA1) and alternate function number traits (e.g. pub trait AF1) in that library. The idea was to let this dependency library be mostly generated and the other crate implement those traits for their pin types (when they are in the right state). Unfortunately, this fails because this leads to possibly conflicting trait implementations:

   |
21 | impl<Pin: PA9 + AF4> I2cScl<I2C1> for Pin {}
   | ----------------------------------------- first implementation here
...
24 | impl<Pin: PB10 + AF4> I2cScl<I2C1> for Pin {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation

But if you are curious, here is how the code looked like:

use stm32l4::stm32l4x2::*;

// Peripheral pin traits
pub trait I2cScl<I2C> {}
pub trait I2cSda<I2C> {}

// Pin traits
pub trait PA9 {}
pub trait PB6 {}
pub trait PB10 {}
// ...

// Alternate function number traits
pub trait AF1 {}
// ...
pub trait AF4 {}
pub trait AF5 {}

// Trait implementations for all Pins which fulfill
// the requirements, i.e. right pin + af combination.
// Especially this part was intended to be generated.
impl<Pin: PA9 + AF4> I2cScl<I2C1> for Pin {}
/* commenting this in produces a "conflicting implementation" error
impl<Pin: PB6 + AF4> I2cScl<I2C1> for Pin {}
impl<Pin: PB10 + AF4> I2cScl<I2C1> for Pin {}
// ...
impl<Pin: PB6 + AF5> I2cScl<I2C4> for Pin {}
*/

Currently, I have no other (nice and beautiful) idea how to separate the alternate function relationships into their own library. So I guess your last proposal is currently the best.