Skip to content

boards: provide RTC feature for boards with the RTT feature#14496

Closed
benpicco wants to merge 4 commits intoRIOT-OS:masterfrom
benpicco:boards/nrf5x_rtc
Closed

boards: provide RTC feature for boards with the RTT feature#14496
benpicco wants to merge 4 commits intoRIOT-OS:masterfrom
benpicco:boards/nrf5x_rtc

Conversation

@benpicco
Copy link
Contributor

@benpicco benpicco commented Jul 10, 2020

Contribution description

If those boards all provide the RTT feature, they will also provide the RTC feature.

Testing procedure

Run tests/periph_rtc on a nrf51/nrf52 board.

Issues/PRs references

@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: boards Area: Board ports labels Jul 10, 2020
@benpicco benpicco requested a review from bergzand July 10, 2020 15:11
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Tested on iot-lab on nrf52832-mdk, nrf51dk, nrf52840dk. All are working.

I haven't followed all this in detail but can you explain why it is working although there's no rtc.c for nrf ?

@benpicco
Copy link
Contributor Author

We now have a RTT based software RTC implementation in drivers/rtt_rtc.
It wakes up each time time RTT overflows to update the counter and relies on backup RAM to keep the time across Deep Sleeps (if available)

@aabadie
Copy link
Contributor

aabadie commented Jul 10, 2020

It seems this has something related to the rtt_rtc module and greping it I found:

FEATURES_PROVIDED += periph_rtc

in a Makefile.dep. I think it was supposed to provide the feature if the module is used. But anyway this has nothing to do here and should be removed.

Apparently for nrf, there's already some logic to add the rtt_rtc module when the rtc feature is used:

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

@aabadie
Copy link
Contributor

aabadie commented Jul 10, 2020

We now have a RTT based software RTC implementation in drivers/rtt_rtc

Can you point to the original PR ?

@benpicco
Copy link
Contributor Author

benpicco commented Jul 10, 2020

Apparently for nrf, there's already some logic to add the rtt_rtc module when the rtc feature is used:

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

Now I remember this works with FEATURES_REQUIRED += periph_rtc but not FEATURES_OPTIONAL += periph_rtc - thinking about the emulated nature of the RTC, maybe it's better that way 🤔

Can you point to the original PR ?

#13519

@aabadie
Copy link
Contributor

aabadie commented Jul 10, 2020

Both rtc and rtt are based on the same peripheral so I think the features should at least be declared as conflicting features on nrf (it's the case on sam0).

@aabadie
Copy link
Contributor

aabadie commented Jul 10, 2020

thinking about the emulated nature of the RTC, maybe it's better that way

which way ?

@benpicco
Copy link
Contributor Author

Both rtc and rtt are based on the same peripheral so I think the features should at least be declared as conflicting features on nrf (it's the case on sam0).

That won't work as rtt_rtc depends on periph_rtt

thinking about the emulated nature of the RTC, maybe it's better that way

which way ?

The way it is without this PR: RTC is available with FEATURES_REQUIRED += periph_rtc but not FEATURES_OPTIONAL += periph_rtc

@aabadie
Copy link
Contributor

aabadie commented Jul 10, 2020

That won't work as rtt_rtc depends on periph_rtt

Do you think that both rtt and rtc could be used at the same time ? If it's not the case, then there's an issue in the build system. If they can be used together, then I'm fine with this PR.

@benpicco
Copy link
Contributor Author

benpicco commented Jul 10, 2020

Do you think that both rtt and rtc could be used at the same time?

No that's not possible.
(Although now that you mention it, nrf52 does implement two RTT peripherals. So we could indeed use the second one for RTC. But only one is used in RIOT right now)

there's an issue in the build system

Yes, it's the old issue that an interface can have multiple implementations. AFAIK we don't have a clean solution for that.

@benpicco
Copy link
Contributor Author

I just realized I have opened a can of works 😩
With #14497 I now have to update all cpu_cc2538|cpu_esp32|cpu_esp8266 boards too that provide periph_rtt.

Now in theory this would be easy with

FEATURES_REQUIRED += periph_rtt
FEATURES_REQUIRED_ANY += cpu_cc2538|cpu_esp32|cpu_esp8266

but make info-boards-supported does not print all boards 😟

@benpicco
Copy link
Contributor Author

nvm this actualy does print all boards and esp8266/esp32 already provide the feature on CPU level

@benpicco benpicco requested a review from MrKevinWeiss as a code owner July 10, 2020 16:45
@benpicco benpicco changed the title boards/common/nrf5x: provide RTC feature boards: provide RTC feature for boards with the RTT feature Jul 10, 2020
@aabadie
Copy link
Contributor

aabadie commented Jul 10, 2020

Yes, it's the old issue that an interface can have multiple implementations. AFAIK we don't have a clean solution for that.

If we start providing the rtc feature for boards/cpus which already provide rtt, there will be spurious issues for users. If we can't find a clean solution in the build system to warn the users, I'm not sure I want this in.

@benpicco
Copy link
Contributor Author

Hm but without this I wouldn't want #14497 in

@aabadie
Copy link
Contributor

aabadie commented Jul 10, 2020

Hm but without this I wouldn't want #14497 in

As I understand the rtt_rtc module, it provides a software wrapper that mimics the periph/rtc.h API but on top of an RTT hardware component. So yes, somehow the module act as if the periph_rtc feature is provided. The problem is that in the build system, features can only be determined from the hardware itself (cpu or board). The list of features provided should be known (with Makefile.features files) before starting the dependency resolution (with Makefile.dep files).

So #14497 is completely valid alone. It's just that if someone wants to use periph_rtc on a board that is already providing periph_rtt but has no hardware RTC, then this person just has to add USEMODULE += rtt_rtc somewhere in its application Makefile.

Here we are facing the same issue we already have for the possible netif feature that can only be known once drivers module are added.

So as a conclusion, I wouldn't add now FEATURES_PROVIDED += periph_rtc at CPU level when there's only periph_rtt and thanks to the rtt_rtc module. We must first find a solution in the build system. Maybe Kconfig will provide that solution for free when it's in.

@aabadie
Copy link
Contributor

aabadie commented Jul 10, 2020

this person just has to add USEMODULE += rtt_rtc somewhere in its application Makefile.

I tested a build of tests/periph_rtc for nrf in #14497 with the following change:

diff --git a/tests/periph_rtc/Makefile b/tests/periph_rtc/Makefile
index ecb6a6a796..54df5bdee3 100644
--- a/tests/periph_rtc/Makefile
+++ b/tests/periph_rtc/Makefile
@@ -1,7 +1,8 @@
 include ../Makefile.tests_common
 
-FEATURES_REQUIRED = periph_rtc
+# FEATURES_REQUIRED = periph_rtc
 
+USEMODULE += rtt_rtc
 USEMODULE += xtimer
 
 include $(RIOTBASE)/Makefile.include

=> no build error

@benpicco benpicco removed the request for review from leandrolanzieri November 9, 2020 17:27
@benpicco benpicco closed this Nov 9, 2020
@benpicco benpicco deleted the boards/nrf5x_rtc branch November 9, 2020 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: boards Area: Board ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants