Skip to content

cpu/esp[32, 8266, _common]: Move dependency resolutions to Makefile.dep #13685

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
Merged
merged 20 commits into from
Mar 24, 2020
Merged

cpu/esp[32, 8266, _common]: Move dependency resolutions to Makefile.dep #13685

merged 20 commits into from
Mar 24, 2020

Conversation

leandrolanzieri
Copy link
Contributor

@leandrolanzieri leandrolanzieri commented Mar 23, 2020 •
edited
Loading

Contribution description

This PR is an attempt to cleanup the dependency resolution for ESP CPUs. The main idea is to make use of GNU Make conditional expansion to check for USEMODULE variable in Makefile.include files, so the expansion is delayed until after the dependency resolution occurs.

Currently this depends on #12994, so we do not have to deal with GNRC_NETIF_NUMOF in Makefiles. I suggest reviewing this commit per commit.

Testing procedure

  • Using the debug-dependency targets make sure that included modules and features do not change.
  • Inspect the CFLAGS to make sure they are correctly added.

Issues/PRs references

Part of #9913
Depends on #12994

@leandrolanzieri leandrolanzieri added Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation State: waiting for other PR State: The PR requires another PR to be merged first Platform: ESP Platform: This PR/issue effects ESP-based platforms labels Mar 23, 2020
gschorcht
gschorcht reviewed Mar 23, 2020
View reviewed changes
Copy link
Contributor

@gschorcht gschorcht left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great job 👍 Looks good from the first view.

I will investigate whether all dependencies and compiler settings are pulled in correctly. The tests in #PR12971 would help now to compare the ESP specific dependencies. I will do it manually for the moment.

gschorcht
gschorcht reviewed Mar 23, 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.

The change works as expected.

gschorcht
gschorcht reviewed Mar 23, 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.

I have checked all possible compile configurations for ESP specific modules. It works like charm.

@gschorcht
Copy link
Contributor

@leandrolanzieri What is your time schedule for this PR?

From my point of view it looks good and it works as expected. It is very good cleanup. I wish I'd had the idea with the conditionals.

BTW, the merge of PR #13365 produces a conflict. It shouldn't be a problem to move

USEMODULE += pm_layered

from cpu/esp32/Makefile.include to cpu/esp32/Makefile.dep.

aabadie
aabadie reviewed Mar 23, 2020
View reviewed changes
@leandrolanzieri
Copy link
Contributor Author

@leandrolanzieri What is your time schedule for this PR?

It would be nice to have this before the soft feature freeze, but we need #12994 to get rid of GNRC_NETIF_NUMOF

BTW, the merge of PR #13365 produces a conflict.

I'll rebase

@gschorcht
Copy link
Contributor

@leandrolanzieri What is your time schedule for this PR?

It would be nice to have this before the soft feature freeze, but we need #12994 to get rid of GNRC_NETIF_NUMOF

Yeah, but this was just for user convenience. IMHO, we could it for the ESPs. If an application developer enables more than one network interface, he should know that he also has to increase the number of netifs. There are always cases which are not covered by this approach. For example, if you have enabled esp_wifi as netdev_default and use mrf24j40 to add a IEEE802.15.4 interface. So I wouldn't block this PR because of PR#13365.

@leandrolanzieri leandrolanzieri force-pushed the pr/esp/cleanup_makefile_dep_include branch from 0fada52 to 3517151 Compare March 23, 2020 14:50
@leandrolanzieri
Copy link
Contributor Author

@gschorcht rebased to solve the conflict with pm_layered.

Yeah, but this was just for user convenience. IMHO, we could it for the ESPs.

What do you mean?

gschorcht
gschorcht reviewed Mar 23, 2020
View reviewed changes
@gschorcht
Copy link
Contributor

@gschorcht rebased to solve the conflict with pm_layered.

Yeah, but this was just for user convenience. IMHO, we could it for the ESPs.

What do you mean?

IMHO, we could remove it for the ESPs 😎

gschorcht
gschorcht reviewed Mar 23, 2020
View reviewed changes
gschorcht
gschorcht reviewed Mar 23, 2020
View reviewed changes
gschorcht
gschorcht reviewed Mar 23, 2020
View reviewed changes
@gschorcht gschorcht mentioned this pull request Mar 24, 2020
Merged
@gschorcht
Copy link
Contributor

Please squash.

@leandrolanzieri leandrolanzieri force-pushed the pr/esp/cleanup_makefile_dep_include branch from 572f8b0 to 45bc750 Compare March 24, 2020 08:26
@leandrolanzieri leandrolanzieri force-pushed the pr/esp/cleanup_makefile_dep_include branch from 45bc750 to 58320c9 Compare March 24, 2020 08:27
@leandrolanzieri
Copy link
Contributor Author

@gschorcht squashed. I renamed the commit that removes the increase of interfaces.

gschorcht
gschorcht approved these changes Mar 24, 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.

Looks good. Works as expected.

@gschorcht gschorcht added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Mar 24, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Mar 24, 2020
@gschorcht gschorcht merged commit c8a2ff2 into RIOT-OS:master Mar 24, 2020
@gschorcht
Copy link
Contributor

@leandrolanzieri Thanks for this contribution.

@leandrolanzieri leandrolanzieri deleted the pr/esp/cleanup_makefile_dep_include branch March 24, 2020 10:28
@leandrolanzieri
Copy link
Contributor Author

@gschorcht thanks a lot for reviewing and testing

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

@aabadie aabadie aabadie left review comments

@gschorcht gschorcht gschorcht approved these changes

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

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 Platform: ESP Platform: This PR/issue effects ESP-based platforms Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2020.04
Development

Successfully merging this pull request may close these issues.
3 participants
@leandrolanzieri @gschorcht @aabadie