Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ztimer with two instances of ztimer_periph_timer #17611

Closed
jue89 opened this issue Feb 3, 2022 · 7 comments · Fixed by #17654
Closed

ztimer with two instances of ztimer_periph_timer #17611

jue89 opened this issue Feb 3, 2022 · 7 comments · Fixed by #17654
Assignees
Labels
Area: timers Area: timer subsystems Type: question The issue poses a question regarding usage of RIOT

Comments

@jue89
Copy link
Contributor

jue89 commented Feb 3, 2022

Description

I'm using ztimer on the efm32 family. This family has two different types of periph_timer: One which makes use of the EFM32 TIMER peripheral and one that utilizes the EFM32 LETIMER peripheral. The latter one may be clocked by a low-frequency crystal and stays active during lower power modes.

I'd like to use a LETIMER for the ZTIMER_MSEC and one TIMER for the ZTIMER_USEC. This combination isn't supported by ztimer_init. (I have reasons for not using periph_rtt: it conflicts with the internal radio in my case.) I see two different options for this:

  1. Make ztimer_init() weak and provide an own ztimer_init() for the EFM32 family.
  2. Somehow bring this combination into the current ztimer_init()

I'd say the first option is the better one. The current ztimer_init() implementation is already very complex. I'd say introducing more complexity for this corner case isn't worth it.

What do you think?

Useful links

@jue89 jue89 added Area: timers Area: timer subsystems Type: question The issue poses a question regarding usage of RIOT labels Feb 3, 2022
@kfessel
Copy link
Contributor

kfessel commented Feb 9, 2022

Would it be possible to provide letimer as an alternative rtt (if someone uses radio)?

@kaspar030
Copy link
Contributor

kaspar030 commented Feb 9, 2022

auto_init_ztimer is a "default module", meaning it should be able to disable it with DISABLE_MODULE += auto_init_ztimer.
But, then you'd have to manually initialize all needed clocks, early (in auto_init.c).

I'm not a fan of weak symbols, people get surprised often by functions being overridden in the linking step.
I wish we'd have xfa based auto_init already, it would be simple to put some custom initialization to replace ztimer_init().

@jue89
Copy link
Contributor Author

jue89 commented Feb 9, 2022

Would it be possible to provide letimer as an alternative rtt (if someone uses radio)?

I guess. But I don't like to go that way - periph_rtt isn't an instance based driver. And there will be people that may want to use LETIMER and RTT at the same time.

auto_init_ztimer is a "default module", meaning it should be able to disable it with DISABLE_MODULE += auto_init_ztimer. But, then you'd have to manually initialize all needed clocks, early (in auto_init.c).

I'm not a fan of weak symbols, people get surprised often by functions being overridden in the linking step. I wish we'd have xfa based auto_init already, it would be simple to put some custom initialization to replace ztimer_init().

Thank you for your advise!

Yes, xfa based auto_init would be very nice to have. But I guess the setup of ztimer during board_init() should work for me ...

@chrysn
Copy link
Member

chrysn commented Feb 12, 2022

If it turns out that initializing the ztimer during the auto_init phase is necessary (say, because a later auto_init'ed driver needs it running), there would still be the option of putting the auto initialization into auto init with a board-selective (or, efm32-common selective, or even more precisely named like auto_init_ztimer_efm32mixed) guard into the auto init section.

(A precisely named (pseudo)module for this would also allow users who disable the auto_init_ztimer module to disable auto_init_ztimer_efm32mixed, though I doubt there are good reasons to disable ztimer auto init unless one provides equivalent functionality as proposed here. If it does turn out to be relevant, the current auto_init_ztimer could still be renamed to auto_init_ztimer_default, and auto_init_ztimer becomes a pseudomodule like netdev_default that just selects whatever is the default ztimer setup for that board -- but as said, likely not necesary).

@kfessel
Copy link
Contributor

kfessel commented Feb 12, 2022

It would also be possible to add a second timer to the ztimer auto config ->
this will also enable us in future to abandon the special rtt interface and just use an low power timer instead (on avr and on stm they have more in common than they are different)

@chrysn
Copy link
Member

chrysn commented Feb 12, 2022

I think that's what jue's "2. Somehow bring this combination into the current ztimer_init()" meant, which would add complexity to an already complex setup.

@jue89
Copy link
Contributor Author

jue89 commented Feb 14, 2022

If it turns out that initializing the ztimer during the auto_init phase is necessary (say, because a later auto_init'ed driver needs it running), there would still be the option of putting the auto initialization into auto init with a board-selective (or, efm32-common selective, or even more precisely named like auto_init_ztimer_efm32mixed) guard into the auto init section.

In the first attempt, I try not to mix-in too much EFM32 Series 2 related stuff into the core and auto_init system. I'd say reviewing initial patches for a new cpu family is a time-intensive task, anyway. Thus, I try to allow reviewers to convince themselves that no other parts of RIOT are touched and side-effects aren't to be expected.

board_init() is called before auto_init(), which is called during kernel_init() (cf. vectors_cortexm.c). I'd say ztimer initialization should work properly during board_init().

Integrating and generalizing stuff (as #17654 proposes) should be the second step after the family has landed in RIOT.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: timers Area: timer subsystems Type: question The issue poses a question regarding usage of RIOT
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants