Skip to content

drivers/Makefile.dep: remove inappropriate use of FEATURES_PROVIDED for rtt_rtc#14497

Merged
aabadie merged 2 commits intoRIOT-OS:masterfrom
aabadie:pr/drivers/makefile_dep_cleanup
Jul 20, 2020
Merged

drivers/Makefile.dep: remove inappropriate use of FEATURES_PROVIDED for rtt_rtc#14497
aabadie merged 2 commits intoRIOT-OS:masterfrom
aabadie:pr/drivers/makefile_dep_cleanup

Conversation

@aabadie
Copy link
Contributor

@aabadie aabadie commented Jul 10, 2020

Contribution description

As seen when reviewing #14496, there's no reason to use FEATURES_PROVIDED in a Makefile.dep. The dependency resolution can be done later.
Any cpu already providing periph_rtt but not periph_rtc could simply add:

ifneq (,$(filter periph_rtc,$(USEMODULE)))
  USEMODULE += rtt_rtc
endif

in its Makefile.dep and add

FEATURES_PROVIDED += periph_rtc

in its Makefile.features

and that would work.

Maybe this breaks other platforms, let's see what Murdock has to say.

Testing procedure

Test periph_rtc on nrf5x platforms with #14496 and it'll still work.

Issues/PRs references

Found while reviewing #14496

@aabadie aabadie added Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 10, 2020
@aabadie aabadie requested a review from benpicco July 10, 2020 16:20
@benpicco
Copy link
Contributor

benpicco commented Jul 10, 2020

Ok with this line removed, #14496 becomes necessary.

% make BOARD=nrf52840dk -C tests/periph_rtc 
make: Entering directory '/home/benpicco/dev/RIOT/tests/periph_rtc'
There are unsatisfied feature requirements: periph_rtc
/home/benpicco/dev/RIOT/tests/periph_rtc/../../Makefile.include:886: *** You can let the build continue on expected errors by setting CONTINUE_ON_EXPECTED_ERRORS=1 to the command line.  Stop.
make: Leaving directory '/home/benpicco/dev/RIOT/tests/periph_rtc'

whereas on master

% make BOARD=nrf52840dk -C tests/periph_rtc
make: Entering directory '/home/benpicco/dev/RIOT/tests/periph_rtc'
Building application "tests_periph_rtc" for "nrf52840dk" with MCU "nrf52".

"make" -C /home/benpicco/dev/RIOT/boards/nrf52840dk
"make" -C /home/benpicco/dev/RIOT/boards/common/nrf52xxxdk
"make" -C /home/benpicco/dev/RIOT/core
"make" -C /home/benpicco/dev/RIOT/cpu/nrf52
"make" -C /home/benpicco/dev/RIOT/cpu/cortexm_common
"make" -C /home/benpicco/dev/RIOT/cpu/cortexm_common/periph
"make" -C /home/benpicco/dev/RIOT/cpu/nrf52/periph
"make" -C /home/benpicco/dev/RIOT/cpu/nrf5x_common
"make" -C /home/benpicco/dev/RIOT/cpu/nrf5x_common/periph
"make" -C /home/benpicco/dev/RIOT/drivers
"make" -C /home/benpicco/dev/RIOT/drivers/periph_common
"make" -C /home/benpicco/dev/RIOT/drivers/rtt_rtc
"make" -C /home/benpicco/dev/RIOT/sys
"make" -C /home/benpicco/dev/RIOT/sys/auto_init
"make" -C /home/benpicco/dev/RIOT/sys/div
"make" -C /home/benpicco/dev/RIOT/sys/isrpipe
"make" -C /home/benpicco/dev/RIOT/sys/newlib_syscalls_default
"make" -C /home/benpicco/dev/RIOT/sys/stdio_uart
"make" -C /home/benpicco/dev/RIOT/sys/test_utils/interactive_sync
"make" -C /home/benpicco/dev/RIOT/sys/tsrb
"make" -C /home/benpicco/dev/RIOT/sys/xtimer
   text	   data	    bss	    dec	    hex	filename
  14264	    128	   2448	  16840	   41c8	/home/benpicco/dev/RIOT/tests/periph_rtc/bin/nrf52840dk/tests_periph_rtc.elf

@aabadie
Copy link
Contributor Author

aabadie commented Jul 10, 2020

There are unsatisfied feature requirements: periph_rtc

And this is normal since the cpu doesn't provide that feature.

@aabadie
Copy link
Contributor Author

aabadie commented Jul 10, 2020

If one wants to use the RTT emulated RTC then just add USEMODULE += rtt_rtc in the application Makefile.

@benpicco
Copy link
Contributor

If you want I can also include this commit in #14496

Copy link
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.

If the feature is provided by the board / CPU, this is not needed here.

@benpicco benpicco added the State: waiting for other PR State: The PR requires another PR to be merged first label Jul 10, 2020
@aabadie aabadie requested a review from haukepetersen as a code owner July 10, 2020 19:58
@aabadie aabadie changed the title drivers/Makefile.dep: remove useless FEATURES_PROVIDED for rtt_rtc drivers/Makefile.dep: remove inappropriate FEATURES_PROVIDED for rtt_rtc Jul 14, 2020
@aabadie aabadie changed the title drivers/Makefile.dep: remove inappropriate FEATURES_PROVIDED for rtt_rtc drivers/Makefile.dep: remove inappropriate use of FEATURES_PROVIDED for rtt_rtc Jul 14, 2020
@fjmolinas
Copy link
Contributor

@aabadie what is this PR waiting for?

@benpicco
Copy link
Contributor

benpicco commented Jul 15, 2020

I added the label because IMHO we should merge #14496 first to not lose the functionality.
Then there was disagreement if we should have this by default in the first place.

I wanted RTC emulation enabled automatically for targets that only have a RTT, but @aabadie noted that a user could then also call rtt_ functions without a warning which would mess with the RTC emulation / RTC emulation would mess with the behavior of RTT.

@aabadie
Copy link
Contributor Author

aabadie commented Jul 15, 2020

what is this PR waiting for?

Normally, nothing. if @benpicco agrees, we can (not to say should) remove the label.

@fjmolinas
Copy link
Contributor

#14496 Has some issues regarding integration with our current build system, lets go ahead with this one as is.

@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 State: waiting for other PR State: The PR requires another PR to be merged first CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 20, 2020
@aabadie aabadie merged commit f013be5 into RIOT-OS:master Jul 20, 2020
@aabadie aabadie deleted the pr/drivers/makefile_dep_cleanup branch July 20, 2020 11:05
Comment on lines -1 to -3
ifneq (,$(filter periph_rtc,$(USEMODULE)))
USEMODULE += rtt_rtc
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

We could at least have left this in place, it's only used if periph_rtc is explicitly requested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes no sense if the board doesn't provide that feature.

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

Labels

Area: build system Area: Build system Area: drivers Area: Device drivers 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

Development

Successfully merging this pull request may close these issues.

3 participants