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

Move pin af definitions to a universal pin_mappings file #29

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

david-sawatzke
Copy link
Member

@david-sawatzke david-sawatzke commented Jan 1, 2019

This makes the peripheral files really peripheral specific and moves the glue into a separate file. I'd like to compare it to all the datasheets to make sure there is no typo in there.

I'm not sure if we should keep the analog mappings in the adc.rs file.

@david-sawatzke david-sawatzke changed the title Move pin af definitions to a universal pin_mappings file [WIP] Move pin af definitions to a universal pin_mappings file Jan 1, 2019
@therealprof
Copy link
Member

Nice idea!

@HarkonenBade
Copy link
Member

Looks awesome!

@HarkonenBade
Copy link
Member

hmm, I have a potential thought for this. Would it be preferable to just have a single definition of all the pins for each micro variant? It will result in some duplication, but it will also make it much clearer what the total mapping for any given micro is.

@therealprof
Copy link
Member

hmm, I have a potential thought for this. Would it be preferable to just have a single definition of all the pins for each micro variant? It will result in some duplication, but it will also make it much clearer what the total mapping for any given micro is.

I was thinking the same as this would make things much easier to change and verify.

@jessebraham
Copy link
Contributor

jessebraham commented Jan 1, 2019

Just my $0.02, I like @HarkonenBade 's idea of splitting out the pin definitions by variant; as mentioned by @therealprof it would simplify adding/modifying/verifying things, IMO. In regards to potential duplication, we may be able to refactor out some common configuration as we find it, but I see the pros of this change outweighing the cons.

@david-sawatzke
Copy link
Member Author

Would it be preferable to just have a single definition of all the pins for each micro variant?
I split it up so its (everything) -> (family) -> (specific micro). I think it's the best way.

@HarkonenBade
Copy link
Member

Yeah, I'm just wondering if a flat single macro for each supported chip feature would be easier to keep track of. Also I'd be down to make a python script that takes the pin mapping table from a datasheet in a sane way and outputs the macro def.

@david-sawatzke
Copy link
Member Author

f. Also I'd be down to make a python script that takes the pin mapping table from a datasheet in a sane way and outputs the macro def.

Pdf parsing sounds somewhat broken and insane, but you could maybe see what stm32cubemx uses internally. It's probably something xml-y.

@HarkonenBade
Copy link
Member

f. Also I'd be down to make a python script that takes the pin mapping table from a datasheet in a sane way and outputs the macro def.

Pdf parsing sounds somewhat broken and insane, but you could maybe see what stm32cubemx uses internally. It's probably something xml-y.

That is fair, currently pulling some of their MCU defs to take a look at what the file format is like.

@therealprof
Copy link
Member

@HarkonenBade PDF parsing is indeed tricky. Most likely you'll only find all the fancy tipos and minor changes in typography in all the different data sheets.

@HarkonenBade
Copy link
Member

Well, I've managed to hack together something that can read the cubemx database files and spit out a pins! macro definition for a particular mcu.

@therealprof
Copy link
Member

@HarkonenBade Cool stuff, potentially even more accurate than the SVD files or datasheets/documentation.

@HarkonenBade
Copy link
Member

https://github.com/HarkonenBade/cube-parse
There is the current state, the code is super messy and potentially fragile, but it seems to function. I'll try and grind some of the rough edges off if it seems useful.

@david-sawatzke
Copy link
Member Author

Looks good. Does anyone know what the license of these files is? I liked having a guaranteed subset, but it's way better and less error prone this way.

@HarkonenBade
Copy link
Member

Looks good. Does anyone know what the license of these files is? I liked having a guaranteed subset, but it's way better and less error prone this way.

I believe that it is under the same licence applied to all of cubeMX which is this: https://www.st.com/resource/en/license_agreement/dm00218346.pdf . I'm not 100% certain if derivative use would be permitted, but I also am not an expert at reading licence agreements.

@david-sawatzke
Copy link
Member Author

use, reproduction or redistribution of this software package partially or totally may be done in any manner that would subject this software package to any Open Source Terms (as defined below)
is software package or any part thereof, including modifications and/or derivative works of this software package, must be used and execute solely and exclusively on or in combination with a microcontroller or a microprocessor devices manufactured by or for STMicroelectronics.

Doesn't seem very compatible. We should probably store them in a separate repo akin to stm32-rs and just use the end result. I wouldn't depend on them not completely changing under us. Having a separate crate doesn't seem possible/easy, so i think we just need to copy-paste.

@HarkonenBade
Copy link
Member

hmm, I'm not sure that would be directly a terrible restriction, as is the stm32f0xx-hal really going to be used on anything other than ST devices? Also if we were copy pasting then we would still fall foul of the licence I suspect.

@david-sawatzke
Copy link
Member Author

Nah, we're only using the technical data, that's not copyright, while the original files may be.
According to Wikipedia

Copyright does not cover ideas and information themselves, only the form or manner in which they are expressed

So we're probably safe, but I'm no lawyer

@therealprof
Copy link
Member

@david-sawatzke

Doesn't seem very compatible.

What do you mean by that? Using the result should be no problem. Redistributing parts of CubeMX is of course a bad idea but it's likely they're part of the family packs and in that case it might be even possible to retrieve them on the fly.

I wouldn't depend on them not completely changing under us.

I would not change anything from how we're working today: if we need the pins, get the files, run the tool put the output into this crate and done. We'll not notice if they change anyway but unless they're buggy I'm also not expecting any major changes.

@HarkonenBade
Copy link
Member

@david-sawatzke

Doesn't seem very compatible.

What do you mean by that? Using the result should be no problem. Redistributing parts of CubeMX is of course a bad idea but it's likely they're part of the family packs and in that case it might be even possible to retrieve them on the fly.

They are sadly only distributed as part of the actual cubemx application. The family packs just contain hal sources and templates/examples. The issue is if the pin defs count as 'derived works' from the cubemx software package.

@david-sawatzke
Copy link
Member Author

Using the result seems to be no problem

Yes, but i personally like the way stm32-rs does it, generating final files as needed and using original files as the source of truth. But we can't do it this way in the repo

@therealprof
Copy link
Member

Yes, but i personally like the way stm32-rs does it, generating final files as needed and using original files as the source of truth. But we can't do it this way in the repo

There're two obvious ways how to achieve the same (and a few more non-obvious ones):

  • Ask for permission to include the source files in the GH repo
  • Download them on the fly

@HarkonenBade
Copy link
Member

Yes, but i personally like the way stm32-rs does it, generating final files as needed and using original files as the source of truth. But we can't do it this way in the repo

There're two obvious ways how to achieve the same (and a few more non-obvious ones):

  • Ask for permission to include the source files in the GH repo
  • Download them on the fly

The latter will require working out how to extract them from the STM32CubeMX installer without running it however.

@david-sawatzke
Copy link
Member Author

david-sawatzke commented Jan 1, 2019

Getting permission would be great, but i wouldn't download them on the fly, since we could experience random breakage with newer versions and stm32cubemx is pretty large.

EDIT: And st wants people to give their email to download it, although the aur also has a direct link.

@HarkonenBade
Copy link
Member

Slow progress on autogeneration still being made in https://github.com/HarkonenBade/cube-parse, the latest output looks something like https://gist.github.com/HarkonenBade/5ae17d4f85e5f83faeb05407fd5a653c

@david-sawatzke david-sawatzke changed the title [WIP] Move pin af definitions to a universal pin_mappings file Move pin af definitions to a universal pin_mappings file Jan 11, 2019
@david-sawatzke
Copy link
Member Author

Rebased this on master, but haven't added the new pinbindings, because I hope we'll use generated mappings

@HarkonenBade
Copy link
Member

HarkonenBade commented Jan 24, 2019

I'll try taking a bit more of a look at that soon. A profitable route may be using the features of the F0-hal crate to narrow the feature gate declarations. (and I could then flag if there are any conflicts between the current feature gates and the ones that the pin def would require)

@dbrgn

This comment has been minimized.

@dbrgn

This comment has been minimized.

@dbrgn
Copy link
Contributor

dbrgn commented Feb 18, 2020

Quick update, after checking @HarkonenBade's approach with cube-parse again a bit in more depth, this definitely looks like the cleaner approach than the hack I did above. We had a lengthy discussion tonight in the rust-embedded Matrix channel with @adamgreig and @therealprof about grouping of features and ended up with the conclusion that using the "GPIO IP Versions" as features is probably the best approach.

I've forked cube-parse for now and added a lot of documentation on how the STM32CubeMX database is structured, and why grouping features by "GPIO IP Version" probably makes sense.

In the case of the STM32F0, the following five GPIO versions...

STM32F031_gpio_v1_0_Modes.xml
STM32F042_gpio_v1_0_Modes.xml
STM32F051_gpio_v1_0_Modes.xml
STM32F052_gpio_v1_0_Modes.xml
STM32F091_gpio_v1_0_Modes.xml

...would result in the following features:

  • io-STM32F031
  • io-STM32F042
  • io-STM32F051
  • io-STM32F052
  • io-STM32F091

The feature names could still be bikeshedded. A possible alternative would be removing the STM32 prefix so that the feature name is io-F031. I think some kind of prefix (like io) makes sense though, to indicate that these are not necessarily prefixes of your MCU (see below).

Note that sometimes those GPIO peripheral versions don't match the name. For example the STM32F091_gpio_v1_0 version is shared among the following MCUs:

#[cfg(feature = "io-STM32F091")]
// STM32F030CCTx
// STM32F030RCTx
// STM32F091C(B-C)Tx
// STM32F091C(B-C)Tx
// STM32F091C(B-C)Ux
// STM32F091C(B-C)Ux
// STM32F091R(B-C)Tx
// STM32F091R(B-C)Tx
// STM32F091RCHx
// STM32F091RCYx
// STM32F091V(B-C)Tx
// STM32F091V(B-C)Tx
// STM32F091VCHx
// STM32F098CCTx
// STM32F098CCUx
// STM32F098RCHx
// STM32F098RCTx
// STM32F098RCYx
// STM32F098VCHx
// STM32F098VCTx

This includes some F098xxxx MCUs as well as some STM32F030xxxx. However, other STM32F030xxxx MCUs are using other GPIO versions (e.g. STM32F030C6Tx -> io-STM32F031).

We'll probably have to solve this issue with documentation (and maybe a lookup table from MCU to feature).

The alternative would be adding a feature per MCU and then adding a dependency on the proper GPIO version, but @therealprof objected to having many features 🙂 I'm still unsure whether having a feature per MCU would be a good idea or not.

(The MCU feature dependencies could look something like this: STM32F098CCUx = ["io-STM32F091"].)

@rnestler
Copy link

I'm still unsure whether having a feature per MCU would be a good idea or not.

I think the feature per IO IP version makes more sense (if properly documented) since which IO IP the MCU has is a feature of it.

On the other hand as soon as we need to differentiate between USART, I2C, etc. IP versions, it would maybe be easier to just select the MCU and all the features get selected correctly. But that could also happen via some code generation, i.e. creating from a template for a given MCU.

@david-sawatzke
Copy link
Member Author

@dbrgn I haven't looked at this issue & repo in quite a while, but I think, as long as it's all generated, it's fine to have a feature for each cpu.

I prefer it over the approach with the IP features, since having the human do the lookup is quite error-prone & takes more effort to check, especially when "only" switching the variant. I also can't see many disadvantages to this approach, since it just consists of adding something generated to the cargo.toml, especially since this is gonna have to be done either in the cargo.toml or in the documentation. I've skimmed the conversation in matrix and based on my understanding, the major objection here is the need to test each & every feature.

But a complete test "just" needs to test all the combinations that are used, not each stm32f42 part if they're all the same. This doesn't differ from what needs to be done if this lookup table is just documentation, since all combinations that exist as a part still need to be tested. (I'm also not sure if we can't just rely on testing all the combinations when updating the generated files and otherwise ignoring multiple gpio mappings)

For the "unnecessary" variants: It's also more obvious to the user, if each feature they need is named in a somewhat consistent way, not the thing we currently do, where the "granularity" of the features differ based on which family you're using.

TLDR: The difference is whether the lookup table is in the documentation or exists as features. Removing the human element seems like a good idea.

(BTW: I have very little skin in the game, feel free to ignore this)

@dbrgn
Copy link
Contributor

dbrgn commented Feb 19, 2020

I tend to agree with you, especially with regard to only having to test one mcu-feature per gpio version. However, I still do see value in mainly using gpio version based features for cfg guards in the code because it allows using newer MCUs with older HALs without having to re-generate all the pin mappings first, as long as the gpio version of the new MCU is already covered.

@therealprof would you also object to per-mcu features if they're purely aliases for a gpio version feature?

mcu-STM32F098CCUx = ["io-STM32F091"]

As aliases, they would not need to be tested in CI.

@dimpolo
Copy link
Contributor

dimpolo commented May 13, 2020

I might be missing something, but it seems as though this mapping is not found by cube-parse.

#[cfg(any(feature = "stm32f030xc", feature = "stm32f042", feature = "stm32f048"))]
i2c_pins! {
    I2C1 => {
        scl => [gpiob::PB13<Alternate<AF5>>],
        sda => [gpiob::PB14<Alternate<AF5>>],
    }
}

stm32f042c4.pdf does have this mapping and it does show up in CubeMX.

However GPIO-STM32F042_gpio_v1_0_Modes.xml (or any other F0 file for that matter) does not include it.

Not sure what's going on there or how CubeMX still knows about the mapping

@dbrgn
Copy link
Contributor

dbrgn commented May 13, 2020

Hm, this is weird indeed. These MCUs all use the STM32F042_gpio_v1_0 definitions:

STM32F042T6Yx.xml:	<IP ConfigFile="GPIO-STM32F0xx" InstanceName="GPIO" Name="GPIO" Version="STM32F042_gpio_v1_0"/>
STM32F042K(4-6)Ux.xml:	<IP ConfigFile="GPIO-STM32F0xx" InstanceName="GPIO" Name="GPIO" Version="STM32F042_gpio_v1_0"/>
STM32F042G(4-6)Ux.xml:	<IP ConfigFile="GPIO-STM32F0xx" InstanceName="GPIO" Name="GPIO" Version="STM32F042_gpio_v1_0"/>
STM32F042K(4-6)Tx.xml:	<IP ConfigFile="GPIO-STM32F0xx" InstanceName="GPIO" Name="GPIO" Version="STM32F042_gpio_v1_0"/>
STM32F042F6Px.xml:	<IP ConfigFile="GPIO-STM32F0xx" InstanceName="GPIO" Name="GPIO" Version="STM32F042_gpio_v1_0"/>
STM32F042F4Px.xml:	<IP ConfigFile="GPIO-STM32F0xx" InstanceName="GPIO" Name="GPIO" Version="STM32F042_gpio_v1_0"/>
STM32F042C(4-6)Ux.xml:	<IP ConfigFile="GPIO-STM32F0xx" InstanceName="GPIO" Name="GPIO" Version="STM32F042_gpio_v1_0"/>
STM32F042C(4-6)Tx.xml:	<IP ConfigFile="GPIO-STM32F0xx" InstanceName="GPIO" Name="GPIO" Version="STM32F042_gpio_v1_0"/>

However, that IP file only seems to contain GPIO_AF5_I2C1 mappings for PA11/PA12.

At the same time I can confirm that PB13 / PB14 have the I2C1 functionality available in STM32CubeMX... 😕

Maybe some "common" functionality that's inherited from another file?

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.

7 participants