GPIO features #129
GPIO features #129
Conversation
|
Btw, the PAC modules matter only because we have functions that give out access to the shared GPIO registers and we need to name their return types. That wouldn't be the case if we used the CS approach (discussed in at length #37) to access those registers, but I didn't want to make this a full refactor of the GPIO code, to keep the changes manageable. And the PAC should be fixed anyway. |
| use crate::dma; | ||
| use cortex_m::interrupt; | ||
| } | ||
| } |
There was a problem hiding this comment.
I am not completely happy with using cfg_if to apply feature gates to multiple items. I guess it beats repeating yourself, but there is quite a lot of added indentation now.
|
|
||
| unsafe impl TxPin<USART2> for gpioa::PA2<AF7> {} | ||
| // unsafe impl TxPin<USART2> for gpioa::PA14<AF7> {} | ||
| // unsafe impl TxPin<USART2> for gpiob::PB3<AF7> {} |
There was a problem hiding this comment.
I don't know why these pins were commented out. I just added them in again, with the features derived from the available AF mappings according to the CubeMX DB.
|
All checks pass but this is only because the CI is actually broken. I created a PR to fix it: #130 In reality, the compilation still does not work for the stm32f398 feature. The reason is that the PAC is missing the enable bits for GPIOG and GPIOH ( |
|
That second issue with the PAC has been fixed and this PR compiles not for all MCU sub-families. |
|
I've looked at most of the relevant datasheets and it looks like all pin and AF mappings that are present in the datasheets are also present in the auto-generated mappings. Which means the CubeMX database is likely complete. Some MCU families omit some of the theoretically available AFs in their datasheet. That is because these AFs relate to peripherals not available on the respective MCUs. For example, on the F301 and the F318, the CAN-related AFs are missing. Once we have feature gates for all peripherals, we might be able to use those to decide which AFs are not actually useful on which MCU and exculde them. However, this sounds like a lot of complexity for little gain. There is not much harm in allowing the user to set pins into an AF mode that is useless on the particular MCU. I think it's fine to just ignore those superfluous AFs. In some MCUs (e.g. F318, F378) the pin Finally, the F318 omits whole GPIO ports. According to the alternate functions documentation in the datasheet, ports C and D are missing. I'm not sure if this is quite correct since other sections in the data sheet mention a GPIOC, but GPIOD is definitely not there. According to the RM this MCU family still has the respective RCC enable and reset bits for both ports, so again no harm should be done if we just ignore the fact that those ports are missing. TLDR: We are not missing any mappings but for MCUs with fewer peripherals or smaller packages we sometimes have too many. We could avoid that with more feature gates but I'd expect that to make the code significantly more complex and that is probably not worth it. It should be fine to have a few too many mappings on some MCUs since the worst thing that can arise from users making use of those is useless code that does nothing. |
|
Thanks for looking into it. |
|
I'm currently operating under the assumption that the F318 has the exact same GPIO implementation as, say, the F302x6, just with no usable pins on the GPIOD port. This assumption is purely based on what the CubeMX DB says, I don't see any way to actually verify it, unfortunately. But if it is true that means writing to the memory-mapped registers of this port on the F318 still configures the GPIO normally, it just doesn't have any effect. So that should be totally safe. What we can easily do is have a look at the datasheets to see what is actually mapped at the missing port's memory. For the F318 GPIOD would be mapped at Then again I know nothing about the underlying hardware details and my mental image of how this stuff works might be totally wrong. If you have any idea how we could verify or falsify this more, that would help :) |
I haven't (really) checked any datasheets but the one related to the f303xc before - so thanks for doing that. While my mental image of how this stuff works might be totally wrong, that pretty much resolves my concern. |
There was a problem hiding this comment.
Wow, this will take a while to read through. Thanks for all that work.
I'll try to go through it file by file. Hope that works.
A note on testing (disclosure: I'm a big fan of TDD):
I see why there is not a single test for the HAL, as a test would most of the time simply reproduce the code written (or at least I don't get how to test a proper register setup). Also property based testing (like idempotency tests) doesn't really feel in place here. If there is something else that makes sense on hardware on the low level of a HAL, I'd like to learn about it and even use it here, but I don't know of any.
However this seems not to be the case for this codegen subproject. If I grasp the scope of this codegen project correctly, it provides a very good starting point to replace a lot of repetetive tasks in a lot of the modules already written and to be wirtten. So establishing an easy to refactor codebase seems like a good idea.
however I'm just starting to read all of this, so stop me if I'm ahead of myself ;)
|
@jamesmunns actually made a blog post for testing on embedded, might be interesting to you: https://jamesmunns.com/blog/hardware-ci-overview/. I don't know too much about testing embedded code myself. I think it is a good idea to try to separate low-level hardware-specific code (register accesses) from hardware-independent algorithmic code. Put the algorithmic code into a separate ( A HAL is by definition hardware-specific code. I can't think of anything we could decouple enough from the hardware to write unit tests for. So we are left with making good use of Rust's type checking and writing examples (we could definitely use more of those). It would be interesting to explore options of having unit tests that run directly on the MCU. You are right that That all said, I'm not against adding tests to |
.github/workflows/ci.yml
Outdated
| @@ -18,6 +18,8 @@ jobs: | |||
| - stm32f302xc | |||
| - stm32f302xd | |||
| - stm32f302xe | |||
| - stm32f302x6 | |||
| - stm32f302x8 | |||
There was a problem hiding this comment.
I don't know how travis works. I was under the impression that we're only building for stm32f303xc (it also says so in the include part)
If that's the case, why list all the mcus here?
If that's not the case (and hey - thats better), we should extend the note in the README to also update this file.
There was a problem hiding this comment.
There is no Travis involved, we are using Github Actions now :)
Here is the relevant documentation for matrices: https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idstrategy
Basically, it runs the job for each combination of all specified features. In our case it means, every MCU is checked with the "rt" feature. The "include" field is special and allows you to add new entries to the matrix or overwrite existing ones. We use that to specify that the Check for the stm32f303xc MCU should use the features "rt,stm32-usbd" instead of "rt". But all other MCUs are still tested. You can actually see that in the names of the checks executed for a PR.
Not sure which note in the README you mean? There is one in the Cargo.toml, that already mentions the ci.yml file:
# Any Changes here should be mirrored in README.md, src/lib.rs, and
# .github/workflows/ci.yml.There was a problem hiding this comment.
There is no Travis involved, we are using Github Actions now :)
Here is the relevant documentation for matrices: https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idstrategyBasically, it runs the job for each combination of all specified features. In our case it means, every MCU is checked with the "rt" feature. The "include" field is special and allows you to add new entries to the matrix or overwrite existing ones. We use that to specify that the Check for the stm32f303xc MCU should use the features "rt,stm32-usbd" instead of "rt". But all other MCUs are still tested. You can actually see that in the names of the checks executed for a PR.
Thanks for taking the time to explain this. I like it.
Not sure which note in the README you mean? There is one in the Cargo.toml, that already mentions the ci.yml file:
# Any Changes here should be mirrored in README.md, src/lib.rs, and # .github/workflows/ci.yml.
In the root README there is a comment on line 50 I was talking about:
[comment]: # (Any changes here should be mirrored in src/lib.rs)
I'm not sure if adding the instructions to all places where one can possibly think about makes sense or if it makes more sense to add a central note in the README on "contributing" and reference that.
There was a problem hiding this comment.
Ha, good catch. Somehow I totally missed this comment in the README.
I'm not sure if adding the instructions to all places where one can possibly think about makes sense or if it makes more sense to add a central note in the README on "contributing" and reference that.
Probably a good idea. I'm also not a huge fan of having to repeat all the features so often. I think it would be better to just tell the user to pick one feature from the Cargo.toml. But that discussion doesn't really fit here I guess.
How about simply removing this comment in the README? IMO having it only in the Cargo.toml is enough, because if you want to add a new feature you will obviously do it there first. Nobody (I hope) will think that just adding a new line to the README will magically make a new feature appear in the code. Plus the README is targeted to users anyway, not developers.
teskje
force-pushed
the
gpio-features
branch
2 times, most recently
from
4b4cb76
to
4dd3e83
Compare
This also adds two MCU versions (stm32f302x{6,8}) that we missed
previously.
This commit switches to the current nightly version of the stm32f3 PAC to make use of the fixes regarding the F3 GPIO peripherals. We also need to adjust the codegen of the GPIO pin mappings a bit to account for the fact that the F373 GPIO is still special in that the PAC contains an additional `gpiod` module for it.
Setting the memory and peripheral addresses of a DMA channel has been marked unsafe in the newest version of the stm32f3 HAL. This commit adjusts the DMA abstraction code accordingly.
teskje
force-pushed
the
gpio-features
branch
2 times, most recently
from
b90e56f
to
209eef3
Compare
teskje
force-pushed
the
gpio-features
branch
from
209eef3
to
d3dfa2e
Compare
|
Updated the stm32f3 crate to version 0.12. This is ready for merging now. |
codegen/src/main.rs
Outdated
| fn parse_args() -> ArgMatches<'static> { | ||
| App::new("codegen") | ||
| .about("Code generation for the stm32f3xx-hal crate") | ||
| .setting(AppSettings::SubcommandRequiredElseHelp) | ||
| .subcommand( | ||
| SubCommand::with_name("gpio") | ||
| .about("Generate GPIO mappings from an STM32CubeMX database") | ||
| .arg( | ||
| Arg::with_name("db_path") | ||
| .help("Path of the STM32CubeMX MCU database (.../db/mcs/)") | ||
| .required(true), | ||
| ), | ||
| ) | ||
| .get_matches() | ||
| } |
There was a problem hiding this comment.
It might be work it to just use structopt
There was a problem hiding this comment.
You're right, that makes the code a bit nicer :)
|
Sorry, I lack the time for a throughout code review. If @dfrankland approves it I'm fine with merging it (and possibly building on top of it) |
This PR is an attempt to replace the mess that is our current GPIO pin mapping code with auto-generated mappings extracted from the STM32CubeMX database. It's inspired by a similar effort done for the
stm32l0xx-hal: stm32-rs/stm32l0xx-hal#82.Parsing of that database is done by executing the new
codegencrate and pointing it at the database path, e.g.:This command yields feature-gated
gpio!macro invocations that can be copy-pasted at the end ofgpio.rs.Based on this parsing of the CubeMX database, this PR introduces 5 new features for the 5 different GPIO peripheral versions used between the F3 MCUs. Each MCU feature selects the correct GPIO version, so users don't have to do anything. I've added two new MCU features too, for
stm32f302x6andstm32f302x8, which we previously missed.Apart from the GPIO code itself, I had to touch a few other modules in cases where code used feature-gates that were too broad. For example, code would gate the definition of a UART pin behind a
stm32f302feature gate, but the pin isn't available on all F302 MCUs. For the I2C and serial modules I was able to switch to the newgpio-*features and I'm reasonably confident that this code is correct (or at least more correct than before). For PWM I only did the minimal changes required to make it compile again because, honestly, this code is a mess and I don't understand what it's doing. We need to refactor that too at some point.In the long term, the idea is to only use peripheral version features like the
gpio-*ones as feature gates in the code. MCU features (stm32f3*) should only select the correct peripheral versions and nothing more. I think to achieve this we'll have to extend the codegen to more peripherals first.Currently, the code doesn't compile for most MCUs. It looks like that is because of bugs in the PAC. The PAC contains different GPIO modules (gpio{a,b,c}), which seems to be due to different reset values between the different GPIO ports (port A and B have debug pins that are set toAF0by default). As far as I can tell, all GPIO versions have the same debug pins and therefore the same reset values, so they should have the same GPIO modules in the PAC too. This is not the case for some reason. Thestm32f303PAC is lackinggpioc, for example. I plan to open an issue on thestm32-rsrepo and hopefully get that fixed.In summary, what's still to do here:
codegen