-
Notifications
You must be signed in to change notification settings - Fork 2k
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
cpu/stm32: de-duplicate ifdefs #20609
cpu/stm32: de-duplicate ifdefs #20609
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for undertaking this cleanup!
Some comments:
@MrKevinWeiss do you have some of the hardware in your test rack to give this a whirl? |
Good idea... I ripped everything apart but can set it back up tomorrow I think! |
I can confirm that the blinky example is compiling and working on the following Nucleos: My NUCLEO-WB55 is at home, perhaps I can test it tomorrow. |
The following boards look good!
|
Also ran the periph test shelid on a few and it worked... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the cleanup @Enoch247 and thank you @MrKevinWeiss and @crasbe for testing!
Please squash.
Please squash |
abc97e1
to
465e4fe
Compare
Squashed. Please note I snuck in a fix to the includes, in commit 465e4fe. |
Head branch was pushed to by a user without write access
sorry found an incorrect comment. fixed now. |
You can directly squash such minor fixes (if you don't rebase onto master while doing so) |
6a2faa9
to
d5f3544
Compare
Squashed, and ready for merge now. |
d5f3544
to
06e7da0
Compare
return &APB1_PERIPH_LP_EN; | ||
#endif | ||
#ifdef APB12_PERIPH_LP_EN | ||
#define HAS_LP_MODE 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like this can be defined in line 37 ?
would make this function more likely to fit on screen
maybe or not have an extra check here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#define HAS_LP_MODE 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consider doing it that way during development, but ultimately decided that it must be done as I did it to avoid potential for human error when porting to new processors. The goal was to minimize the amount of code one must touch or understand to extend support to a new MCU.
#endif | ||
break; | ||
#ifdef APB1_PERIPH_DIS | ||
#define RCC_REG_IS_ATOMIC 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invert the logic for less repititon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#define RCC_REG_IS_ATOMIC 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate? I don't understand.
defined(CPU_FAM_STM32F4) || \ | ||
defined(CPU_FAM_STM32F7) || \ | ||
defined(CPU_FAM_STM32L1) | ||
#define APB1_PERIPH_LP_EN RCC->APB1LPENR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#define APB1_PERIPH_LP_EN RCC->APB1LPENR | |
#define HAS_LP_MODE 1 | |
#define APB1_PERIPH_LP_EN RCC->APB1LPENR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above
This patch consolidates mutliple conditional compile blocks. This is done to simplify adding new STM32 CPU's and ease maintenance of existing ports.
Following best practice, this patch adds the module's header as its first include. Resulting compiler errors are also fixed by adding the header's missing include of cpu_conf.h.
06e7da0
to
6c91865
Compare
Contribution description
This patch consolidates mutliple conditional compile blocks. This is done to simplify adding new STM32 CPU's and ease maintenance of existing ports. This PR is also part of my on-going effort to port RIOT to the STM32H7 family.
Also, note that this PR makes functions that I re-wrote thread safe. I do not believe that the previous implementation was.
Testing procedure
Run the script below:
Issues/PRs references
This is an alternate approach to my first attempt (PR #20532) at cleaning up this part of the codebase.