periph/rtc: move init from auto_init to periph_init

[vincent-d]

It seems that the auto_init code used to initialize rtc was not working.

Using the periph_init seems better.

[smlng]

@vincent-d can you elaborate why (and when) this is an issue? Currently RTC is initialized by auto_init modu[l]e which is enabled by default), however some tests (e.g. bloom_bytes) disable auto_init, hence rtc would be off there.

I'm unsure if moving this to periph_init is the right solution, maybe @haukepetersen can enlighten us: what's the idea of periph_init vs auto_init?

[vincent-d]

I was playing around with my rtc and I discovered that rtc_init() was not called.

In fact, MODULE_RTC is never defined, rtc does not exist as a module nor a pseudomodule.

Then we could replace the #ifdef MODULE_RTC by #ifdef FEATURE_PERIPH_RTC but then I thought it would make more sense to initlialize it from periph_init().

[smlng]

ah MODULE_RTC does not exist, haven't looked at that. Though, your suggestion of using FEATURE_PERIPH_RTC might work, too, it would be inconsistent with other periphs. So, I would either suggest to use RTC_NUMOF like SPI_NUMOF in periph_init or define MODULE_RTC (which would be good anyway) to make it work with auto_init?

[haukepetersen]

Actually, moving the init to periph_init makes much sense: the reason for moving the periph_init out of auto_init is simply the order and the timing of initialization: some periph functions are needed prio to auto_init, and hence the peirph_init is called already during board_init.

The guard should in a future case be something like #ifdef MODULE_PERIPH_RTC, but currently we are not able to specify peripherals used using the make system -> somebody would need to solve #5065...

[smlng]

@vincent-d I just tried tests/periph_rtc and examples/default with current master on board pba-d-01-kw2x. In both cases the RTC is available and working. So set aside that it makes sense to move this into periph_init I still don't see when/where you encounter the problem - please clarify.

[vincent-d]

These are the perfect counterexamples, because these both app call explicitly rtc_init().

But I think rtc_init() should be called implicitly as spi_init() is called.

[haukepetersen]

I am with @vincent-d here, rtc_init() should be called in the (auto)initialization phase. BUT I think we should sort out the issue about selectively building/configuring peripheral drivers first, as we don't want to initialize the rtc every time just because a board provides that feature.

Maybe a periph_rtc pseudomodule (as in #ifdef MODULE_PERIPH_RTC) can do the trick for now?!

[smlng]

These are the perfect counterexamples, because these both app call explicitly rtc_init().

Damn, my bad ... oversaw obvious calls 😞 But then, at least for examples/default this explicit call would be obsolete with your fix - so you add removal to this PR

[smlng]

just tested the following: I removed all rtc stuff from examples/default. Afterwards I compiled and flash the app using master-branch on PhyNode (pba-d-01-kw2x), calling rtc gettime directly cause a kernel panic, calling rtc init first helps. Doing the same with this PR applied, calling rtc gettime first works without kernel panic -> periph_init handles rtc_init(). Nice!

Questions is if that should be default behavior and whether we need an option to disable auto_init of RTC somehow?

[vincent-d]

as we don't want to initialize the rtc every time just because a board provides that feature.

Why? We already do that for SPI, don't we?

But this is not completely clear what the rtc_init should do. I have for instance a Vbackup which provide current to my RTC even when the core is shut down. Then when booting, I want to setup the RTC, but not overwrite the counter. I'm not sure every rtc_init are implemented this way.

[smlng]

yes currently it looks like SPI is auto_init if available ... but that doesn't mean that's a good choice in all cases.

And that's why I asked:

Questions is if that should be default behavior and whether we need an option to disable auto_init of RTC somehow?

If I got @haukepetersen correctly, we agree that we should have switches (macros) to either opt-out or opt-in auto-init of peripheral drivers - the default could be auto-init ON (as now).

[smlng]

convinced by discussion, ACK!

[smlng]

this should be guarded with #if RTC_NUMOF as well.

and in theory the same for the spi header above, too - see #7079

[vincent-d]

It does not work on sam because RTT and RTC are the same periph. The linker complains as soon as you use the RTT.

Maybe using a pseudomodule as @haukepetersen suggested is the best way to go. We definitely need to find a way to compile periph drivers independently!

[vincent-d]

Since #7421 is merged, I guess it becomes as trivial as that!

[vincent-d]

Added shell commands fix and removed rtc init from examples/default (not needed anymore).

[haukepetersen]

now it looks nice: ACK

[haukepetersen]

side question, possibly to be solved somewhere else: should we then also remove the init sub-command from the rtc shell command?

[dylad]

rtc_init function & FEATURE_PERIPH_RTC are still present in the following files :
sys/shell/commands/sc_rtc.c
tests/periph_rtc/main.c
tests/lwmac/main.c
tests/pkg_fatfs/main.c

[dylad]

@haukepetersen that is what I was thinking.
Remove init from the shell and leave a small notice in the shell to warn the user.

[haukepetersen]

notice: tests/lwmac/main.c is fixed in #8022

[vincent-d]

• removed rtc init command from shell
• removed rtc_init calls from tests/periph_rtc/main.c and tests/pkg_fatfs/main.c

[dylad]

I think we're good here. I tested this PR on saml21-xpro using tests/periph_rtc and it's work as expected.
Let's merge it !