Skip to content

boards/common/stm32: clean up LED definitions#18415

Merged
maribu merged 2 commits intoRIOT-OS:masterfrom
maribu:boards/common/stm32
Aug 25, 2022
Merged

boards/common/stm32: clean up LED definitions#18415
maribu merged 2 commits intoRIOT-OS:masterfrom
maribu:boards/common/stm32

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Aug 8, 2022

Contribution description

Let boards only define the port and pin number of each LEDs. The common definitions in stm32_leds.h will provide LED<x>_ON, LED<x>_OFF, LED<x>_TOGGLE, LED<x>_PIN, LED<x>_MASK and LED<x>_PORT.

In addition to code de-duplication, this also makes it easier to use LEDs in GPIO LL, which can be beneficial for super low overhead debugging output - e.g. when a bug is timing sensitive and DEBUG() would spent to much time for stdio to reproduce a bug.

Testing procedure

The LED macros should not change to master. Hence, the (except for debug symbols) the binaries generated should not change compared to master.

One could also test if controlling LEDs still works for all the changed boards...

Issues/PRs references

None

@maribu maribu added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: boards Area: Board ports labels Aug 8, 2022
@github-actions github-actions bot added the Area: CI Area: Continuous Integration of RIOT components label Aug 8, 2022
maribu
maribu commented Aug 8, 2022
View reviewed changes
@maribu maribu changed the title Boards/common/stm32 boards/common/stm32: clean up LED definitions Aug 8, 2022
@maribu maribu mentioned this pull request Aug 8, 2022
Merged
3 tasks
@krzysztof-cabaj
Copy link
Copy Markdown
Contributor

krzysztof-cabaj commented Aug 8, 2022

If I well understand your code You assumed that LED is ON when the port is at HIGH state. I don't know how many boards, but at least old stm32f469i-disco use swap polarity - LOW state ONs the LED - see #18041 for more details.

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Aug 8, 2022

Thanks for the pointer. I will add support for inverted LEDs to the common header, so that boards with inverted LEDs just have to toss in an #define LED0_IS_INVERTED 1. I will also double check that this doesn't add regressions for other boards where I didn't notice that they have inverted LEDs.

@maribu
boards/common/stm32: clean up LED definitions
1fd9913 
Let boards only define the port and pin number of each LEDs. The common
definitions in `stm32_leds.h` will provide `LED<x>_ON`, `LED<x>_OFF`,
`LED<x>_TOGGLE`, `LED<x>_PIN`, `LED<x>_MASK` and `LED<x>_PORT`.

In addition to code de-duplication, this also makes it easier to use
LEDs in GPIO LL, which can be beneficial for super low overhead
debugging output - e.g. when a bug is timing sensitive and `DEBUG()`
would spent to much time for stdio to reproduce a bug.
@maribu maribu force-pushed the boards/common/stm32 branch from eaadcff to 1fd9913 Compare August 8, 2022 21:35
@github-actions github-actions bot removed the Area: CI Area: Continuous Integration of RIOT components label Aug 8, 2022
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Aug 8, 2022

Should be fixed now. If my regex-fu is not too bad, I should also have tracked down all other STM32 boards that were configured to use inverted LEDs, so that this hopefully does not cause regressions.

@maribu maribu removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Aug 8, 2022
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Aug 10, 2022

I successfully tested this on the STM32F103 bluepill via tests/leds. So it does indeed work with STM32F1 and with inverted LEDs.

@maribu maribu requested a review from benpicco August 10, 2022 15:46
@krzysztof-cabaj
Copy link
Copy Markdown
Contributor

krzysztof-cabaj commented Aug 11, 2022

I checked your PR with tests/leds on my old STM32F469I-DISCO (boards with inverted LEDs) - I could confirm that everything works fine!

@MrKevinWeiss MrKevinWeiss mentioned this pull request Aug 12, 2022
Closed
benpicco
benpicco approved these changes Aug 22, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a nice convenience improvement.

@github-actions github-actions bot added the Area: tools Area: Supplementary tools label Aug 22, 2022
@maribu maribu force-pushed the boards/common/stm32 branch from 6c284c2 to 5e9d3c3 Compare August 22, 2022 10:47
@maribu maribu force-pushed the boards/common/stm32 branch from 5e9d3c3 to db3c51d Compare August 22, 2022 10:55
@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 22, 2022
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Aug 22, 2022

I replaced a few thousands of LED[0-9]_FOO and BTN[0-9]_FOO regexes in exclude_patterns with a handful of hand-crafted ones. That should also make sure that future additions of these macros will not be frowned upon from the CI.

@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 25, 2022
@maribu maribu enabled auto-merge August 25, 2022 14:08
@maribu maribu merged commit 8d14769 into RIOT-OS:master Aug 25, 2022
@maribu maribu deleted the boards/common/stm32 branch August 26, 2022 05:24
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Aug 26, 2022

Thx :)

@maribu maribu mentioned this pull request Sep 8, 2022
Merged
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@kaspar030 kaspar030 Awaiting requested review from kaspar030

Assignees

No one assigned

Labels

Area: boards Area: Board ports Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

Release 2022.10

Development

Successfully merging this pull request may close these issues.

3 participants

@maribu @krzysztof-cabaj @benpicco