Skip to content

Makefile.include: include Makefile.features after $(CPU)/Makefile.include#11424

Closed
fjmolinas wants to merge 1 commit intoRIOT-OS:masterfrom
fjmolinas:pr_reorder_makefile.include
Closed

Makefile.include: include Makefile.features after $(CPU)/Makefile.include#11424
fjmolinas wants to merge 1 commit intoRIOT-OS:masterfrom
fjmolinas:pr_reorder_makefile.include

Conversation

@fjmolinas
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas commented Apr 19, 2019 •
edited
Loading

Contribution description

Currently in base Makefile.include, Makefile.fatures for a BOARD is included after $(RIOTCPU)/$(CPU)/Makefile.include.

RIOT/Makefile.include

Lines 247 to 258 in 9a64731

# process provided features
include $(RIOTBOARD)/$(BOARD)/Makefile.features
# mandatory includes!
include $(RIOTMAKE)/pseudomodules.inc.mk
include $(RIOTMAKE)/defaultmodules.inc.mk
# Include Board and CPU configuration
INCLUDES += $(addprefix -I,$(wildcard $(RIOTBOARD)/$(BOARD)/include))
include $(RIOTBOARD)/$(BOARD)/Makefile.include
INCLUDES += -I$(RIOTCPU)/$(CPU)/include
include $(RIOTCPU)/$(CPU)/Makefile.include

This causes the below statement to have no effect since KINETIS_SERIES isn't defined at this point. But more generaly means none of the variables defined are in $(RIOTCPU)/$(CPU)/Makefile.include are yet available.

RIOT/cpu/kinetis/Makefile.features

Lines 5 to 9 in 9a64731

ifeq (EA,$(KINETIS_SERIES))
FEATURES_PROVIDED += periph_ics
else
FEATURES_PROVIDED += periph_mcg
endif

Moving Makfile.features below include $(RIOTCPU)/$(CPU)/Makefile.include. would allow using the SERIES, FAMILY, etc. to include some features for the specific group.

I'm not sure of the impact of this change, I would like @cladmi insight on the matter. Based on my inspection of the build system there would be no negative impact since FEATURES_PROVIDED are first used by Makefile.dep. Also Makefile.features files only reference themselves so this doesn't affect the include $(RIOTBOARD)/$(BOARD)/Makefile.include and $(RIOTCPU)/$(CPU)/Makefile.include.

Testing procedure

All applications for all boards should still compile as expected.

To see this fixes #11423 add $(info Kinetis series: $(KINETIS_SERIES)) just before the above the code below and call make -C examples/hello-world/ BOARD=pba-d-01-kw2x.

RIOT/cpu/kinetis/Makefile.features

Lines 5 to 9 in 9a64731

ifeq (EA,$(KINETIS_SERIES))
FEATURES_PROVIDED += periph_ics
else
FEATURES_PROVIDED += periph_mcg
endif

Without this PR:

make: Entering directory '/home/francisco/workspace/RIOT/examples/hello-world'
Kinetis series: W
Building application "hello-world" for "pba-d-01-kw2x" with MCU "kinetis".

Without this PR:

make: Entering directory '/home/francisco/workspace/RIOT/examples/hello-world'
Kinetis series:
Building application "hello-world" for "pba-d-01-kw2x" with MCU "kinetis".

Issues/PRs references

Could fix #11423.
Related to #8713

@fjmolinas fjmolinas requested a review from cladmi April 19, 2019 10:21
@fjmolinas fjmolinas added Area: build system Area: Build system Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) and removed Impact: major The PR changes a significant part of the code base. It should be reviewed carefully labels Apr 19, 2019
@fjmolinas fjmolinas mentioned this pull request Apr 19, 2019
Merged
@fjmolinas fjmolinas changed the title Makefile.include: features after cpu Makefile Makefile.include: include Makefile.features after $(CPU)/Makefile.include Apr 19, 2019
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Apr 19, 2019 via email

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Apr 23, 2019 •
edited
Loading

The issue is also present in the efm32 cpu which depends on a variable defined by Makefile.include

RIOT/cpu/efm32/Makefile.features

Lines 9 to 11 in d16d911

ifeq (1,$(EFM32_TNRG))
FEATURES_PROVIDED += periph_hwrng
endif
(where there is even a typo in TRNG but fixing it changes nothing).

The problem should however be solved the other way around, with these variables all defined by Makefile.features.

It cannot currently be done as CPU is defined by board/*/Makefile.include, I described the issue in #9913 where the next step would be to move all the CPU/CPU_FAM/CPU_FAMILY/CPU_MODEL variables to Makeflie.features.
I welcome feedback in the tracking issue if I should PR a change in this direction or how to handle it otherwise.

And indeed, there is also this file https://github.com/RIOT-OS/RIOT/blob/master/makefiles/info-global.inc.mk#L10 that does the evaluation in a different way for getting the dependencies.
Moving these BSP specific variables would require to list them for cleaning them between each evaluation or change this handling but may not be directly possible.

The two way of doing the parsing already leads to inconsistencies:

make info-boards-supported | tr ' ' '\n' | grep mips-malta
mips-malta

BOARD=mips-malta make
There are unsatisfied feature requirements: periph_uart


EXPECT ERRORS!

@fjmolinas
Copy link
Copy Markdown
Contributor Author

fjmolinas commented Apr 23, 2019

@cladmi @kaspar030 Thanks for your insight into the build system!

I understand even though this is a valid problem this PR is not the propper way of fixing the issue. I will close this PR and raise the issue in #9913 (I had missed it when opening this PR).

@fjmolinas fjmolinas mentioned this pull request Apr 23, 2019
Open
22 tasks
@fjmolinas fjmolinas closed this Apr 30, 2019
@fjmolinas fjmolinas deleted the pr_reorder_makefile.include branch August 7, 2019 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cladmi cladmi Awaiting requested review from cladmi

Assignees

No one assigned

Labels

Area: build system Area: Build system Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cpu/kinetis: features provided not properly defined according to series

3 participants

@fjmolinas @kaspar030 @cladmi