Skip to content

Makefile.dep: periph_init based on USEMODULE#13639

Merged
gschorcht merged 1 commit intoRIOT-OS:masterfrom
fjmolinas:pr_usemodule_periph_init
Mar 16, 2020
Merged

Makefile.dep: periph_init based on USEMODULE#13639
gschorcht merged 1 commit intoRIOT-OS:masterfrom
fjmolinas:pr_usemodule_periph_init

Conversation

@fjmolinas
Copy link
Contributor

@fjmolinas fjmolinas commented Mar 16, 2020

Contribution description

In #13511 @gschorcht pointed out the later PR changed the behaviour when adding periph_% MODULEs via the commandline. Since periph_init check was based on FEATURES_USED it didn't take into account if it was added directly as a USEMODULE.

This PR now make the rule check USEMODULE instead of FEATURES_USED to restore the previous behavior.

Testing procedure

USEMODULE=periph_rtc make -C tests/shell/ info-debug-variable-USEMODULE --no-print-directory | grep rtc
app_metadata auto_init board core core_init core_msg core_panic cpu native-drivers periph periph_common periph_gpio periph_init periph_init_common periph_init_gpio periph_init_init periph_init_pm periph_init_rtc periph_init_uart periph_pm periph_rtc periph_uart ps shell shell_commands stdin stdio_native sys

only periph_rtc in master:

USEMODULE=periph_rtc make -C tests/shell/ info-debug-variable-USEMODULE --no-print-directory | grep rtc
app_metadata auto_init board core core_init core_msg core_panic cpu native-drivers periph periph_common periph_gpio periph_init periph_init_gpio periph_init_pm periph_pm periph_rtc periph_uart ps shell shell_commands stdin stdio_native s

NOTE: the right way to add periph depdencies should still be through FEATURES_REQUIRED, but since it is not how most MODULEs are added It made sense adding this handling.

Issues/PRs references

#13511

@fjmolinas fjmolinas added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 16, 2020
@fjmolinas fjmolinas requested a review from gschorcht March 16, 2020 08:06
@fjmolinas fjmolinas requested a review from aabadie as a code owner March 16, 2020 08:06
gschorcht
gschorcht approved these changes Mar 16, 2020
View reviewed changes
Copy link
Contributor

@gschorcht gschorcht left a comment

Choose a reason for hiding this comment

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

Works now with USEMODULE=periph_rtc and FEATURES_RQUIRED=periph_rtc.

@gschorcht gschorcht 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 Mar 16, 2020
@gschorcht gschorcht merged commit 90815c2 into RIOT-OS:master Mar 16, 2020
@gschorcht
Copy link
Contributor

gschorcht commented Mar 16, 2020

@fjmolinas Thanks for contributing the change.

@fjmolinas fjmolinas deleted the pr_usemodule_periph_init branch March 16, 2020 12:49
@fjmolinas
Copy link
Contributor Author

fjmolinas commented Mar 16, 2020

Thanks for the review!

@maribu
Copy link
Member

maribu commented Mar 16, 2020

Now modules periph_init_init and periph_init_commo are added in addition...

@maribu maribu mentioned this pull request Mar 16, 2020
Merged
@fjmolinas
Copy link
Contributor Author

fjmolinas commented Mar 16, 2020

Now modules periph_init_init and periph_init_commo are added in addition...

Damn, sorry about this.

@gschorcht
Copy link
Contributor

gschorcht commented Mar 16, 2020

Now modules periph_init_init and periph_init_commo are added in addition...

Indeed, I didn't realize that. I just focused on the modules I was looking for. Hm. Maybe it is better to revert this PR. I can live with the FEATURES_REQUIRED approach.

@fjmolinas
Copy link
Contributor Author

fjmolinas commented Mar 16, 2020

Indeed, I didn't realize that. I just focused on the modules I was looking for. Hm. Maybe it is better to revert this PR. I can live with the FEATURES_REQUIRED approach.

It could be just filtered out as @maribu did here https://github.com/RIOT-OS/RIOT/blob/ea5122e7e466483a136f2cec2bc50b9686ea4e86/Makefile.features_modules.

@fjmolinas
Copy link
Contributor Author

fjmolinas commented Mar 16, 2020

Indeed, I didn't realize that. I just focused on the modules I was looking for. Hm. Maybe it is better to revert this PR. I can live with the FEATURES_REQUIRED approach.

I'm not familiar with the revert policy, but I did not test this well enough, so maybe a revert is better.

@maribu
Copy link
Member

maribu commented Mar 16, 2020

I'm not familiar with the revert policy, but I did not test this well enough, so maybe a revert is better.

I would just create a PR to add filtering this out.

As those are pseudo-modules anyway, no harm is done. It is just a cosmetic issue.

fjmolinas added a commit to fjmolinas/RIOT that referenced this pull request Mar 17, 2020
@fjmolinas
Makefile.dep: cleanup extra periph_init_%
0014983 
PR RIOT-OS#13639 cused periph_init_common and periph_init_init to be added
to USEMODULES, this PR fixes it.
@fjmolinas fjmolinas mentioned this pull request Mar 17, 2020
Merged
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Mar 19, 2020
@leandrolanzieri leandrolanzieri added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Mar 19, 2020
@fjmolinas
Copy link
Contributor Author

fjmolinas commented Mar 26, 2020 •
edited
Loading

Figured out another issue caused by this one, quite a big increase decrease (takes longer) in performance in info-boards-supported.

@maribu
Copy link
Member

maribu commented Mar 26, 2020

If the FEATURES_REQUIRED_ANY PR gets in, the performance issue get mitigated ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gschorcht gschorcht gschorcht approved these changes

@aabadie aabadie Awaiting requested review from aabadie

Assignees

No one assigned

Labels

Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2020.04

Development

Successfully merging this pull request may close these issues.

4 participants

@fjmolinas @gschorcht @maribu @leandrolanzieri