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

sys/ztimer: auto-select ztimer_no_periph_rtt only for samd21 #17786

Merged
merged 1 commit into from
Mar 17, 2022

Conversation

fjmolinas
Copy link
Contributor

@fjmolinas fjmolinas commented Mar 10, 2022

Contribution description

As seen in #14031 sam3(EDIT: only samd21) does to suffer from the same clock domain synchronization issues (or at least much less), see #17786 (comment) #17786 (comment) for esting results.

Testing procedure

I I just ran USEMODULE=ztimer_periph_rtt IOTLAB_NODE=samr30-1.saclay.iot-lab.info BOARD=samr30-xpro make -C tests/bench_ztimer/ for samr30-xpro and it looks OK:

main(): This is RIOT! (Version: 2022.04-devel-798-g8fa26-HEAD)
ztimer benchmark application.

                     set() one   121995 / 1000 = 121
                  remove() one     1544 / 1000 = 1
          set() + remove() one   243996 / 1000 = 243
  set() many increasing target   138007 / 1000 = 138
               re-set()  first   122001 / 1000 = 122
               re-set() middle   122100 / 1000 = 122
               re-set()   last   122192 / 1000 = 122
       remove() + set()  first   244009 / 1000 = 244
       remove() + set() middle   243983 / 1000 = 243
       remove() + set()   last   243982 / 1000 = 243
      remove() many decreasing   203144 / 1000 = 203
                  ztimer_now()     7506 / 1000 = 7
              sizeof(ztimer_t)    16000 / 1000 = 16
done.

@dylad or @jue89 maybe have a saml21-xpro?

@fjmolinas fjmolinas requested a review from jue89 March 10, 2022 13:15
@github-actions github-actions bot added Area: sys Area: System Area: timers Area: timer subsystems labels Mar 10, 2022
@fjmolinas
Copy link
Contributor Author

BOARD=samr30-xpro USEMODULE=ztimer_msec make -C tests/ztimer_msg/ info-modules | grep rtt
periph_init_rtt
periph_rtc_rtt
periph_rtt
ztimer_periph_rtt

@fjmolinas
Copy link
Contributor Author

BTW would be good to see if other sam% can be filtered-out, @benpicco do you have a samd5x to test out? Maybe @dylad has some as well?

Copy link
Contributor

@jue89 jue89 left a comment

Choose a reason for hiding this comment

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

@dylad or @jue89 maybe have a saml21-xpro?

No, I just have a samr30-xpro at hand.

You should mention why the saml21 is filtered-out in the comment above.

@dylad
Copy link
Member

dylad commented Mar 10, 2022

I should be able to test SAML10, SAML11, SAML21 and SAME4 tonight. Worse case tomorrow evening.

@dylad
Copy link
Member

dylad commented Mar 10, 2022

For SAML21-XPRO:
USEMODULE=ztimer_periph_rtt BOARD=saml21-xpro make -C tests/bench_ztimer/ flash

Output:

2022-03-10 21:22:28,685 # START
2022-03-10 21:22:28,692 # main(): This is RIOT! (Version: 2022.04-devel-799-g5cf714-pr_ztimer_rtt_sam3)
2022-03-10 21:22:28,694 # ztimer benchmark application.
2022-03-10 21:22:28,694 # 
2022-03-10 21:22:28,821 #                      set() one   121961 / 1000 = 121
2022-03-10 21:22:28,828 #                   remove() one     2100 / 1000 = 2
2022-03-10 21:22:29,078 #           set() + remove() one   243943 / 1000 = 243
2022-03-10 21:22:29,220 #   set() many increasing target   138031 / 1000 = 138
2022-03-10 21:22:29,347 #                re-set()  first   121963 / 1000 = 121
2022-03-10 21:22:29,474 #                re-set() middle   122062 / 1000 = 122
2022-03-10 21:22:29,601 #                re-set()   last   122153 / 1000 = 122
2022-03-10 21:22:29,850 #        remove() + set()  first   243939 / 1000 = 243
2022-03-10 21:22:30,099 #        remove() + set() middle   243945 / 1000 = 243
2022-03-10 21:22:30,347 #        remove() + set()   last   243945 / 1000 = 243
2022-03-10 21:22:30,555 #       remove() many decreasing   203071 / 1000 = 203
2022-03-10 21:22:30,568 #                   ztimer_now()     7673 / 1000 = 7
2022-03-10 21:22:30,572 #               sizeof(ztimer_t)    16000 / 1000 = 16
2022-03-10 21:22:30,573 # done.

@dylad
Copy link
Member

dylad commented Mar 10, 2022

With this patch:

---
 sys/ztimer/Makefile.dep | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sys/ztimer/Makefile.dep b/sys/ztimer/Makefile.dep
index 757e3649c6..2f4924fb1e 100644
--- a/sys/ztimer/Makefile.dep
+++ b/sys/ztimer/Makefile.dep
@@ -85,7 +85,7 @@ endif
 # the counter is read, this propagates and leads to timing errors
 # on ztimer_msec that are higher than > +-1msec.
 # The same goes for the fe310 rtt.
-ifneq (,$(filter-out saml21,$(filter sam% fe310,$(CPU))))
+ifneq (,$(filter-out saml21 saml1x samd5x,$(filter sam% fe310,$(CPU))))
   USEMODULE += ztimer_no_periph_rtt
 endif
 
-- 
2.17.1

SAML11-XPRO:
USEMODULE=ztimer_periph_rtt BOARD=saml11-xpro make -C tests/bench_ztimer flash

2022-03-10 21:30:30,994 # START
2022-03-10 21:30:31,001 # main(): This is RIOT! (Version: 2022.04-devel-799-g5cf714-pr_ztimer_rtt_sam3)
2022-03-10 21:30:31,003 # ztimer benchmark application.
2022-03-10 21:30:31,004 # 
2022-03-10 21:30:31,161 #                      set() one   151628 / 1000 = 151
2022-03-10 21:30:31,171 #                   remove() one     4270 / 1000 = 4
2022-03-10 21:30:31,481 #           set() + remove() one   303254 / 1000 = 303
2022-03-10 21:30:31,492 #   set() many increasing target     6583 / 100 = 65
2022-03-10 21:30:31,650 #                re-set()  first   151627 / 1000 = 151
2022-03-10 21:30:31,808 #                re-set() middle   151666 / 1000 = 151
2022-03-10 21:30:31,966 #                re-set()   last   151703 / 1000 = 151
2022-03-10 21:30:32,276 #        remove() + set()  first   303264 / 1000 = 303
2022-03-10 21:30:32,586 #        remove() + set() middle   303254 / 1000 = 303
2022-03-10 21:30:32,896 #        remove() + set()   last   303251 / 1000 = 303
2022-03-10 21:30:32,918 #       remove() many decreasing    16830 / 100 = 168
2022-03-10 21:30:32,938 #                   ztimer_now()    14823 / 1000 = 14
2022-03-10 21:30:32,943 #               sizeof(ztimer_t)     1600 / 100 = 16
2022-03-10 21:30:32,943 # done.

SAME54-XPRO:
USEMODULE=ztimer_periph_rtt BOARD=same54-xpro make -C tests/bench_ztimer flash

2022-03-10 21:34:23,568 # START
2022-03-10 21:34:23,575 # main(): This is RIOT! (Version: 2022.04-devel-799-g5cf714-pr_ztimer_rtt_sam3)
2022-03-10 21:34:23,578 # ztimer benchmark application.
2022-03-10 21:34:23,578 # 
2022-03-10 21:34:23,674 #                      set() one    91551 / 1000 = 91
2022-03-10 21:34:23,679 #                   remove() one      531 / 1000 = 0
2022-03-10 21:34:23,866 #           set() + remove() one   183104 / 1000 = 183
2022-03-10 21:34:23,923 #   set() many increasing target    52184 / 1000 = 52
2022-03-10 21:34:24,019 #                re-set()  first    91555 / 1000 = 91
2022-03-10 21:34:24,115 #                re-set() middle    91593 / 1000 = 91
2022-03-10 21:34:24,211 #                re-set()   last    91623 / 1000 = 91
2022-03-10 21:34:24,399 #        remove() + set()  first   183085 / 1000 = 183
2022-03-10 21:34:24,587 #        remove() + set() middle   183092 / 1000 = 183
2022-03-10 21:34:24,775 #        remove() + set()   last   183093 / 1000 = 183
2022-03-10 21:34:24,899 #       remove() many decreasing   119677 / 1000 = 119
2022-03-10 21:34:24,904 #                   ztimer_now()     1077 / 1000 = 1
2022-03-10 21:34:24,909 #               sizeof(ztimer_t)    16000 / 1000 = 16
2022-03-10 21:34:24,910 # done.

@fjmolinas
Copy link
Contributor Author

So outside of samd21 which we know presents the issue it seem only sam3 is additionally being blacklisted:

git grep "CPU = sam" | grep -vE "saml21|samd5x|saml1x|samd21"
boards/common/arduino-due/Makefile.features:CPU = sam3

I schemed through the datasheet of ATSAM3X8E and found no mention to the clock synchronization issue, so I also removed it.

@fjmolinas fjmolinas changed the title sys/ztimer: auto-select ztimer_periph_rtt for saml21 sys/ztimer: auto-select ztimer_no_periph_rtt only for samd21 Mar 11, 2022
@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 Mar 11, 2022
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Mar 12, 2022
@dylad dylad added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Mar 12, 2022
@dylad dylad added this to the Release 2022.04 milestone Mar 12, 2022
@dylad dylad added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Mar 15, 2022
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.

ACK on my side. Re-test on the same platforms, got the same results.
@ju89 any more comments on this ?

Copy link
Contributor

@jue89 jue89 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 samr30-xpro. ACK.

@fjmolinas fjmolinas merged commit 114e61c into RIOT-OS:master Mar 17, 2022
@fjmolinas fjmolinas deleted the pr_ztimer_rtt_sam3 branch March 17, 2022 11:32
@fjmolinas
Copy link
Contributor Author

Thanks for testing everyone!

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: 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 Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants