Skip to content

sys/ztimer/kconfig: add defaults for backends#16116

Merged
MrKevinWeiss merged 1 commit intoRIOT-OS:masterfrom
leandrolanzieri:pr/sys/ztimer/kconfig_change_default_backend
Mar 9, 2021
Merged

sys/ztimer/kconfig: add defaults for backends#16116
MrKevinWeiss merged 1 commit intoRIOT-OS:masterfrom
leandrolanzieri:pr/sys/ztimer/kconfig_change_default_backend

Conversation

@leandrolanzieri
Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri commented Mar 1, 2021

Contribution description

This PR proposes defaults for the ztimer backends' Kconfig symbols. When ztimer is selected the RTT Timer backend (if supported) will be enabled. If the RTT Timer is not enabled or supported the Timer RTT backend (if supported) is enabled. RTT was given priority, considering low-power as the optimization goal, We could have it the other way around, given that the Timer backend enables more ztimer clocks by default.

As having an RTT is not enough to allow that ztimer backend, the Timer peripheral was given priority.

Testing procedure

  • Check that the defaults make sense
  • Green CI

Issues/PRs references

None

@leandrolanzieri leandrolanzieri added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: Kconfig Area: Kconfig integration labels Mar 1, 2021
@maribu
Copy link
Copy Markdown
Member

maribu commented Mar 1, 2021

Note that ztimer is not supported on every RTT. E.g. when the RTT frequency is 1 Hz, users setting millisecond timeouts will be quite surprised about the lack of accuracy.

Searching for RTT_FREQUENCY reveals quite a few instances of a 1 Hz RTT clock:

boards/common/arduino-due/include/periph_conf.h
79:#define RTT_FREQUENCY       (1U)        /* 1Hz *

boards/ikea-tradfri/include/periph_conf.h
86:#define RTT_FREQUENCY       (1U)

boards/stk3200/include/periph_conf.h
104:#define RTT_FREQUENCY       (1U)

boards/e180-zg120b-tb/include/periph_conf.h
88:#define RTT_FREQUENCY       (1U)

boards/slstk3401a/include/periph_conf.h
117:#define RTT_FREQUENCY       (1U)

cpu/kinetis/include/periph_cpu.h
167:#define RTT_FREQUENCY                (1)

... and many more

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

Note that ztimer is not supported on every RTT. E.g. when the RTT frequency is 1 Hz, users setting millisecond timeouts will be quite surprised about the lack of accuracy.

Then it seems to me that having periph_rtt is not sufficient to use the RTT backend, right? Should we then have something like HAVE_ZTIMER_COMPATIBLE_RTT?

@maribu
Copy link
Copy Markdown
Member

maribu commented Mar 2, 2021

Then it seems to me that having periph_rtt is not sufficient to use the RTT backend, right? Should we then have something like HAVE_ZTIMER_COMPATIBLE_RTT?

That would be awesome!

@kfessel
Copy link
Copy Markdown
Contributor

kfessel commented Mar 3, 2021

i had a short look into the arduino-due manual and saw that the rtt of its MCU capable of running at higher frequencys (just depends on the Prescaler which is independent for rtc(always 32....kHz and rtt (selectable)).
I think this might be the same for many of the incompatible Boards.

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

i had a short look into the arduino-due manual and saw that the rtt of its MCU capable of running at higher frequencys (just depends on the Prescaler which is independent for rtc(always 32....kHz and rtt (selectable)).
I think this might be the same for many of the incompatible Boards.

So, if this would be made configurable, could we do something like the following?

config PRESCALER
    int "Prescaler"

config HAVE_ZTIMER_COMPATIBLE_RTT
    bool
    default y if PRESCALER > SOME_VALUE

@kfessel
Copy link
Copy Markdown
Contributor

kfessel commented Mar 3, 2021

With the new structure for the ztimer peripheral selection process in #15715 a automated decision in the compilation process would be simple to implement.

why not have a ztimer_rtt if there is a rtt but just not use it for ms if it has not enaugh resolution

sec use rtt if rtt_freq>= 1

ms use rtt if rtt_freq>= 1000

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

why not have a ztimer_rtt if there is a rtt but just not use it for ms if it has not enaugh resolution

sec use rtt if rtt_freq>= 1

ms use rtt if rtt_freq>= 1000

Currently there is no information in Kconfig about the RTT frequency. If that information is added then the dependencies of the ms can be changed to add the frequency constraint.

@kfessel
Copy link
Copy Markdown
Contributor

kfessel commented Mar 3, 2021

for the atmega it is done like this ( seems very intransparent)

static inline uint8_t _rtt_div(uint16_t freq)
{
    switch (freq) {
    case 32768: return 0x1;
    case  4096: return 0x2;
    case  1024: return 0x3;
    case   512: return 0x4;
    case   256: return 0x5;
    case   128: return 0x6;
    case    32: return 0x7;
    default   : assert(0);
                return 0;
    }
}
rtt_init(void){
...

    /* 32768Hz / n */
    TCCR2B = _rtt_div(RTT_FREQUENCY);
   ...
}

@kfessel
Copy link
Copy Markdown
Contributor

kfessel commented Mar 3, 2021

does kconfig support lists of options?

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

leandrolanzieri commented Mar 3, 2021

does kconfig support lists of options?

It supports choices. As an example see the clock configuration for STM32 https://github.com/RIOT-OS/RIOT/blob/master/cpu/stm32/kconfigs/Kconfig.clk

For reference: https://www.kernel.org/doc/html/latest/kbuild/kconfig-language.html

@maribu
Copy link
Copy Markdown
Member

maribu commented Mar 3, 2021

It is also a pity that there is no API for querying the supported frequencies for a given RTT. A one size fits all RTT_FREQUENCY define is just not sufficient IMO.

Maybe we could also drop the RTT API altogether and just use periph_timer for all non RTC (with C = clock, not c = counter) timers.

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

Should I then, for now, swap the preference in the defaults for this PR?

@maribu
Copy link
Copy Markdown
Member

maribu commented Mar 3, 2021

Should I then, for now, swap the preference in the defaults for this PR?

That would be nice. But there might still be boards that actually cannot set their RTT to 1kHz / 1.024 kHz - I have no idea. If all boards support 1 kHz / 1.024 kHz, using this by default would be awesome for ztimer.

@leandrolanzieri leandrolanzieri force-pushed the pr/sys/ztimer/kconfig_change_default_backend branch from 55ef586 to 6a0240d Compare March 4, 2021 09:48
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

Changed the default backend and updated the description accordingly.

@leandrolanzieri leandrolanzieri added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 4, 2021
Copy link
Copy Markdown
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Tested with native on posix sleep.

The CI passes.

For now it cleans up some of the app.config.test and allows a bit simpler way of managing kconfig.

As discussed the logic to select the RTC should be dealt with later once the information to make that decision is available in the build system. This is a safer way to move forward.

ACK!

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

If @maribu or @kfessel have any reason to disagree, now is the time.

@maribu
Copy link
Copy Markdown
Member

maribu commented Mar 9, 2021

I'm fine, go ahead!

@kfessel
Copy link
Copy Markdown
Contributor

kfessel commented Mar 9, 2021

Should I then, for now, swap the preference in the defaults for this PR?

That would be nice. But there might still be boards that actually cannot set their RTT to 1kHz / 1.024 kHz - I have no idea. If all boards support 1 kHz / 1.024 kHz, using this by default would be awesome for ztimer.

atm most boards that define some RTT_frequency seem not to be set to ~1kHz they are either (much) higher (~32kHz most) or 1Hz

32k     19
1       12
Max     3 (iotlab is one)
16k     3
1024    2
4096    1 (nucleo-f413zh)

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

atm most boards that define some RTT_frequency seem not to be set to ~1kHz they are either (much) higher (~32kHz most) or 1Hz

I will take that as a future PR will be needed!

@MrKevinWeiss MrKevinWeiss merged commit 9d579a3 into RIOT-OS:master Mar 9, 2021
@kfessel
Copy link
Copy Markdown
Contributor

kfessel commented Mar 9, 2021

atm most boards that define some RTT_frequency seem not to be set to ~1kHz they are either (much) higher (~32kHz most) or 1Hz

I will take that as a future PR will be needed!

you are right i just thought about that when i read it here and did some counting

@kaspar030 kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021
@leandrolanzieri leandrolanzieri added the Release notes: ignored Set on PRs that have been considered for inclusion in the current release's notes but were minor. label Apr 28, 2021
@leandrolanzieri leandrolanzieri deleted the pr/sys/ztimer/kconfig_change_default_backend branch September 8, 2021 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Kconfig Area: Kconfig integration CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Release notes: ignored Set on PRs that have been considered for inclusion in the current release's notes but were minor. 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.

5 participants