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.

Sign up for GitHub

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

Jump to bottom

sys/auto_init: add auto_init_leds, drop LED init code from boards #17584

Merged
merged 7 commits into from
Feb 18, 2022
Merged

sys/auto_init: add auto_init_leds, drop LED init code from boards #17584

merged 7 commits into from
Feb 18, 2022

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Jan 28, 2022 •
edited
Loading

Contribution description

Testing procedure

Issues/PRs references

split off #17008 (comment)

@github-actions github-actions bot added Area: boards Area: Board ports Area: build system Area: Build system Area: sys Area: System Platform: native Platform: This PR/issue effects the native platform labels Jan 28, 2022
@fjmolinas fjmolinas mentioned this pull request Feb 9, 2022
Merged
@fjmolinas
Copy link
Contributor

@benpicco why is this a draft?

@benpicco
Copy link
Contributor Author

I think this depends on #17632 😉
AFAIR the problem was with the 'disable LED0 init on nucleo boards if SPI is used'

@fjmolinas
Copy link
Contributor

This one needs a rebase!

@fjmolinas fjmolinas 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 Feb 15, 2022
@fjmolinas
Copy link
Contributor

@benpicco I forced push to your branch rebasing, I'll split 6fd4785 later. It's still missing Kconfig though.

@github-actions github-actions bot added the Area: Kconfig Area: Kconfig integration label Feb 15, 2022
@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed Area: Kconfig Area: Kconfig integration labels Feb 15, 2022
@github-actions github-actions bot added the Area: Kconfig Area: Kconfig integration label Feb 15, 2022
@benpicco benpicco marked this pull request as ready for review February 15, 2022 14:17
@github-actions github-actions bot added Area: drivers Area: Device drivers Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools labels Feb 17, 2022
@fjmolinas
Copy link
Contributor

I doped the dependency to #17632 as its not moving as fast as I thought it would.

@benpicco
Copy link
Contributor Author

@fjmolinas thank you for pushing this forward - if this were your PR I'd give it an ACK 😉

dylad
dylad reviewed Feb 17, 2022
View reviewed changes
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

Why limit the number of LEDs to 8 ?
That's a "good enough" number but I do have some custom boards with more. Shouldn't we let this number configurable at least ?

@benpicco
Copy link
Contributor Author

Let's make leds_int() a weak function then - this allows to overwrite it with custom / cheaper implementations.

e.g. on mcb2388 the custom leds_init() saves 284 bytes compared to the generic function.

@fjmolinas
Copy link
Contributor

@benpicco I think we can consider that we are both ACK'ing each other changes no?

fjmolinas
fjmolinas approved these changes Feb 18, 2022
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK please squash!

@fjmolinas fjmolinas merged commit dd150fe into RIOT-OS:master Feb 18, 2022
@benpicco benpicco deleted the auto_init_leds branch February 18, 2022 18:43
@fjmolinas fjmolinas mentioned this pull request Feb 21, 2022
Merged
@benpicco benpicco mentioned this pull request Feb 26, 2022
Merged
@OlegHahm OlegHahm added this to the Release 2022.04 milestone Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylad dylad dylad left review comments

@fjmolinas fjmolinas fjmolinas approved these changes

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

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

@cgundogan cgundogan Awaiting requested review from cgundogan

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

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

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

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

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

@smlng smlng Awaiting requested review from smlng

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

Assignees
No one assigned
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework 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 Platform: native Platform: This PR/issue effects the native platform
Projects
None yet
Milestone
Release 2022.04
Development

Successfully merging this pull request may close these issues.
4 participants
@benpicco @fjmolinas @dylad @OlegHahm