Skip to content

Conversation

vincent-d
Copy link
Member

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

Using the periph_init seems better.

@miri64 miri64 added Area: drivers Area: Device drivers Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Apr 13, 2017
@smlng
Copy link
Member

smlng commented Apr 13, 2017

@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
Copy link
Member Author

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

smlng commented Apr 14, 2017

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

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

smlng commented Jun 7, 2017

@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
Copy link
Member Author

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

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

smlng commented Jun 7, 2017

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

smlng commented Jun 7, 2017

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
Copy link
Member Author

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

smlng commented Jun 7, 2017

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).

Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

convinced by discussion, ACK!

@smlng smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 8, 2017
@@ -19,6 +19,7 @@
*/

#include "periph/spi.h"
#include "periph/rtc.h"
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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
Copy link
Member Author

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

@vincent-d
Copy link
Member Author

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

Copy link
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

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

now it looks nice: ACK

@haukepetersen
Copy link
Contributor

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

Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

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

dylad commented Nov 13, 2017

@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
Copy link
Contributor

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

@vincent-d
Copy link
Member Author

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

@dylad
Copy link
Member

dylad commented Nov 13, 2017

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 !

@dylad dylad merged commit 67048ea into RIOT-OS:master Nov 13, 2017
@toonst toonst deleted the pr/init_rtc branch August 21, 2018 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants