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

cpu: do not auto-select ztimer_periph_rtt for sam0, fe310 #17395

Merged
merged 3 commits into from Dec 15, 2021

Conversation

fjmolinas
Copy link
Contributor

Contribution description

The sam0 rtt busy loops for 180us every time an alarm is set or
the counter is read, this propagates and leads to timing errors
on ztimer_msec that are higher than > +-1msec.

The same foes for fe310.

Testing procedure

ztimer_periph_rtt is not selected for those boards

BOARD=samr21-xpro USEMODULE=ztimer_msec make -C examples/hello-world/ info-modules | grep ztimer_periph_rtt
# empty
BUILD_IN_DOCKER=1 BOARD=hifive1b USEMODULE=ztimer_msec make -C examples/hello-world/ info-modules | grep ztimer_periph_rtt
# empty

The lacking performance can be seen with #17391:

  • hifive1b
2021-12-14 13:30:49,021 # START
2021-12-14 13:30:49,027 # main(): This is RIOT! (Version: 2022.01-devel-1235-g86ba6-pr-17391)
2021-12-14 13:30:49,030 # xtimer benchmark application.
2021-12-14 13:30:49,030 #
2021-12-14 13:30:49,767 #                      set() one   732483 / 1000 = 732
2021-12-14 13:30:49,771 #                   remove() one      732 / 1000 = 0
2021-12-14 13:30:51,240 #           set() + remove() one  1464843 / 1000 = 1464
2021-12-14 13:30:51,263 #   set() many increasing target    18860 / 100 = 188
2021-12-14 13:30:51,999 #                re-set()  first   732422 / 1000 = 732
2021-12-14 13:30:52,735 #                re-set() middle   732422 / 1000 = 732
2021-12-14 13:30:53,472 #                re-set()   last   732422 / 1000 = 732
2021-12-14 13:30:54,941 #        remove() + set()  first  1464843 / 1000 = 1464
2021-12-14 13:30:56,410 #        remove() + set() middle  1464843 / 1000 = 1464
2021-12-14 13:30:57,879 #        remove() + set()   last  1464844 / 1000 = 1464
2021-12-14 13:30:57,955 #       remove() many decreasing    73242 / 100 = 732
2021-12-14 13:30:58,142 #                   ztimer_now()   183106 / 1000 = 183
2021-12-14 13:30:58,147 #               sizeof(ztimer_t)     1600 / 100 = 16
2021-12-14 13:30:58,147 # done.
  • samr21-xpro
2021-12-14 13:32:08,172 # main(): This is RIOT! (Version: 2022.01-devel-1235-g86ba6-pr-17391)
2021-12-14 13:32:08,175 # xtimer benchmark application.
2021-12-14 13:32:08,175 #
2021-12-14 13:32:08,912 #                      set() one   732211 / 1000 = 732
2021-12-14 13:32:08,919 #                   remove() one     2304 / 1000 = 2
2021-12-14 13:32:10,389 #           set() + remove() one  1464447 / 1000 = 1464
2021-12-14 13:32:10,697 #   set() many increasing target   303791 / 1000 = 303
2021-12-14 13:32:11,434 #                re-set()  first   732220 / 1000 = 732
2021-12-14 13:32:12,172 #                re-set() middle   732315 / 1000 = 732
2021-12-14 13:32:12,909 #                re-set()   last   732374 / 1000 = 732
2021-12-14 13:32:14,379 #        remove() + set()  first  1464415 / 1000 = 1464
2021-12-14 13:32:15,848 #        remove() + set() middle  1464524 / 1000 = 1464
2021-12-14 13:32:17,318 #        remove() + set()   last  1464616 / 1000 = 1464
2021-12-14 13:32:18,134 #       remove() many decreasing   810893 / 1000 = 810
2021-12-14 13:32:18,322 #                   ztimer_now()   183080 / 1000 = 183
2021-12-14 13:32:18,327 #               sizeof(ztimer_t)    16000 / 1000 = 16
2021-12-14 13:32:18,327 # done.

Realted Issues

Found while teseting #17357

@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 14, 2021
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: timers Area: timer subsystems Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms labels Dec 14, 2021
sys/ztimer/Makefile.dep Outdated Show resolved Hide resolved
The sam0 rtt busy loops for 180us every time an alarm is set or
the counter is read, this propagates and leads to timing errors
on ztimer_msec that are higher than > +-1msec.

The same goes for fe310.
@kfessel
Copy link
Contributor

kfessel commented Dec 14, 2021

every read of any timer in these things has to be syncronised 👀

@fjmolinas
Copy link
Contributor Author

every read of any timer in these things has to be syncronised eyes

Yes but its only on slow timers that it really hurts

@dylad
Copy link
Member

dylad commented Dec 14, 2021

@fjmolinas If you're not in a hurry, I can have at SAMD21 to see if there is a way to speed up the sync process instead ?

@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Dec 14, 2021
@fjmolinas
Copy link
Contributor Author

@fjmolinas If you're not in a hurry, I can have at SAMD21 to see if there is a way to speed up the sync process instead ?

Not in a hurry other than wanting this one in the release, and I'm also on vacation tomorrow for the next month, I think it's ok to get this in after soft feature freeze, but you would have to push it forward for me :)

@fjmolinas
Copy link
Contributor Author

@dylad do you mind if we get this in and you can re-add/revert if you figure out a solution?

Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

I read: plausible ✔️
I checked the results of info-module of master and pr with without kconfig: ✔️
I trust the test results by the run by @fjmolinas ✔️
I think this is a good temporary fix (workround) for an issue due to automatic rtt use which the sam and fe310 periph implementation isn't yet good for .

@fjmolinas
Copy link
Contributor Author

Offline @dylad agreed with merging since #17403 still needs some work.

@fjmolinas fjmolinas merged commit 03a004e into RIOT-OS:master Dec 15, 2021
@fjmolinas fjmolinas deleted the pr_cpu_no_rtt branch December 15, 2021 13:05
@fjmolinas fjmolinas added this to the Release 2022.01 milestone Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework Area: timers Area: timer subsystems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants