boards/samr30-xpro: add ztimer configuration by jue89 · Pull Request #14031 · RIOT-OS/RIOT

jue89 · 2020-05-06T12:08:31Z

Contribution description

This PR adds ztimer configuration for the samr30-xpro board. This should reduce its power consumption when no timer is running on the ZTIMER_USEC clock.

Testing procedure

make -C tests/ztimer_pm_layered BOARD=samr30-xpro flash term

Expected output:

2020-05-06 13:42:13,558 # main(): This is RIOT! (Version: 2020.07-devel-446-g8ffd34)
2020-05-06 13:42:13,565 # This test application lets the CPU sleep for 5 seconds using the clocks 
2020-05-06 13:42:13,568 # ZTIMER_MSEC and ZTIMER_USEC alternating.
2020-05-06 13:42:13,569 # 
2020-05-06 13:42:13,573 # Unblocking all pm modes related to ztimer clocks ...
2020-05-06 13:42:13,577 # pm_unblock(CONFIG_ZTIMER_USEC_REQUIRED_PM_MODE)
2020-05-06 13:42:13,578 # 
2020-05-06 13:42:13,580 # Entering the sleepy loop ...
2020-05-06 13:42:13,585 # Elapsed time - ZTIMER_MSEC:   27ms, ZTIMER_USEC:   27ms
2020-05-06 13:42:13,588 # ztimer_sleep(ZTIMER_MSEC, 5000)
2020-05-06 13:42:18,593 # Elapsed time - ZTIMER_MSEC: 5008ms, ZTIMER_USEC:    8ms
2020-05-06 13:42:18,597 # ztimer_sleep(ZTIMER_USEC, 5000000)
2020-05-06 13:42:23,607 # Elapsed time - ZTIMER_MSEC: 5014ms, ZTIMER_USEC: 5008ms
2020-05-06 13:42:23,610 # ztimer_sleep(ZTIMER_MSEC, 5000)
2020-05-06 13:42:28,616 # Elapsed time - ZTIMER_MSEC: 5008ms, ZTIMER_USEC:    8ms
2020-05-06 13:42:28,619 # ztimer_sleep(ZTIMER_USEC, 5000000)

make -C tests/ztimer_overhead BOARD=samr30-xpro flash term

Expected output:

2020-05-06 14:07:25,183 # main(): This is RIOT! (Version: 2020.07-devel-448-ga4511b-feature/board_samr30-xpro_ztimer)
2020-05-06 14:07:25,185 # ZTIMER_USEC->adjust = 16
2020-05-06 14:07:26,232 # min=0 max=0 avg_diff=0

Issues/PRs references

#13722

benpicco · 2020-06-17T17:49:22Z

Murdock is not happy.
Maybe it's better to split this into two PRs.

fjmolinas · 2020-08-26T11:01:17Z

ping @jue89!

benpicco · 2020-10-21T22:18:48Z

ping 😉

fjmolinas · 2021-04-20T09:01:20Z

ping @jue89!

jue89 · 2021-04-20T09:25:42Z

Whoops. This PR slipped through.
I'll split this PR into test and samr30-xpro configuration.

fjmolinas · 2021-11-18T08:59:41Z

boards/samr30-xpro/Makefile.dep

+
+ifneq (,$(filter ztimer_msec,$(USEMODULE)))
+  USEMODULE += ztimer_periph_rtt
+endif


this one can get in by simply dropping this change @jue89, care to rebase?

I can't remember ... what was the problem with this change?

Since a while periph_rtt is the default ztimer_msec backend, except for some BOARDs, among them sam0 BOARDs since it was shown that the rtt overhead was to high to be able to implement a good enough msec timer by default (see #17395):

RIOT/sys/ztimer/Makefile.dep

Lines 81 to 90 in f298d0a

# NOTE: select the module here and not in the CPU so that order of inclusion

# does not have periph_rtt selected earlier that it should be while at the same

# time avoiding the module 'ztimer-no_periph_rtt' being included unecesarily.

# 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 the fe310 rtt.

ifneq (,$(filter sam% fe310,$(CPU)))

USEMODULE += ztimer_no_periph_rtt

endif

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

So it looks like the sam% blacklist was a bit too strict and we can remove sam3.

Well, that patch completely breaks our application. We need ZTIMER_MSEC running on periph_rtt. Yes, high RTT_MIN_OFFSETS are required, but with that in place, the application just works fine even with all these busy loops. We don't have another choice for battery applications.

Note that ztimer_no_periph_rtt does not impede including ztimer_periph_rtt it just does not enable it by default. So I don't see why that would break your application.

I also ran RTT_MIN_OFFSET for the BOARD and it yiels:

RTT_MIN_OFFSET for samr30-xpro over 1024 samples: 8

So quite OK, I think we should re-enable it.

So quite OK, I think we should re-enable it.

Ah wait this is already configured

So I don't see why that would break your application.

That line made me saying this ;-)

RIOT/sys/ztimer/Makefile.dep

Lines 88 to 90 in df80860

ifneq (,$(filter sam% fe310,$(CPU)))

USEMODULE += ztimer_no_periph_rtt

endif

Yes but it only prevents "auto-inclusion", it doesn't forbid an application from till requesting, so you can have:

USEMODULE += ztimer_no_periph_rtt ztimer_periph_rtt.

I'll open a PR re-enabling sam3

OlegHahm · 2022-03-09T12:31:02Z

@jue89, ping!

fjmolinas

I rebased, and re-ran tests/ztimer_overhead and adapted the values, @jue89 care to confirm?

main(): This is RIOT! (Version: 2022.04-devel-780-gdda35-feature/board_samr30-xpro_ztimer)
ZTIMER_USEC auto_adjust params:
    ZTIMER_USEC->adjust_set = 19
    ZTIMER_USEC->adjust_sleep = 23
ZTIMER_USEC auto_adjust params cleared
zitmer_overhead_set...
min=19 max=19 avg_diff=19
zitmer_overhead_sleep...
min=23 max=23 avg_diff=23
ZTIMER_USEC adjust params for samr30-xpro:
    CONFIG_ZTIMER_USEC_ADJUST_SET    19
    CONFIG_ZTIMER_USEC_ADJUST_SLEEP  23

jue89 · 2022-03-10T12:00:18Z

Thank you for pinging me and rebasing on master!

I think this PR need some rework:

Basically, I would drop the test. The brute-force dropping of acquired PM levels is ... bad habit ... to put it kindly. Furthermore, it makes use of ztimer_now() we already identified this to be bad habit, as well. See ztimer: add ztimer_ondemand module for implicit power management #17607.
I'd would configure a PM_BLOCKER_INITIAL of 0x00000001.

fjmolinas · 2022-03-10T12:34:55Z

Thank you for pinging me and rebasing on master!

I think this PR need some rework:

* Basically, I would drop the test. The brute-force dropping of acquired PM levels is ... bad habit ... to put it kindly. Furthermore, it makes use of `ztimer_now()` we already identified this to be bad habit, as well. See [ztimer: add ztimer_ondemand module for implicit power management #17607](https://github.com/RIOT-OS/RIOT/pull/17607).

* I'd would configure a `PM_BLOCKER_INITIAL` of `0x00000001`.

Then maybe we can just split the adjust parameters?

jue89 · 2022-03-10T12:46:51Z

Okay, I can split the PR.

But we gave up on the direct interaction of pm_layered and ztimer. It's a source of so many quirks and bugs during the last years just because of timers sometimes stalling ZTIMER_USEC and the use of ztimer_now(). We are now heading towards #17607.

fjmolinas · 2022-04-07T13:35:36Z

@jue89 should we split the adjust parameters and close this one then?

jue89 · 2022-11-01T11:09:49Z

I'd love to just close this in favor of:

Would someone object?

dylad · 2022-11-04T16:27:50Z

Would someone object?

I think you can go ahead.

jue89 requested review from MichelRottleuthner, aabadie, dylad, fjmolinas, leandrolanzieri and smlng as code owners May 6, 2020 12:08

benpicco requested a review from kaspar030 May 6, 2020 12:15

benpicco added Area: boards Area: Board ports Area: timers Area: timer subsystems Type: new feature The issue requests / The PR implemements a new feature for RIOT labels May 6, 2020

jue89 mentioned this pull request May 7, 2020

sys/ztimer/Makefilde.dep: periph_rtt as FEATURES_OPTIONAL #14013

Closed

jue89 force-pushed the feature/board_samr30-xpro_ztimer branch from 13b819c to 962825a Compare May 8, 2020 11:38

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 18, 2020

MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

fjmolinas reviewed Nov 18, 2021

View reviewed changes

fjmolinas added this to the Release 2022.01 milestone Nov 18, 2021

emmanuelsearch modified the milestones: Release 2022.01, Release 2022.04 Jan 27, 2022

OlegHahm added the State: waiting for author State: Action by the author of the PR is required label Mar 9, 2022

tests/ztimer_pm_layered: initial commit

f0c3672

fjmolinas force-pushed the feature/board_samr30-xpro_ztimer branch from 962825a to d6af2c4 Compare March 9, 2022 14:09

github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework labels Mar 9, 2022

github-actions bot removed the Area: timers Area: timer subsystems label Mar 9, 2022

fjmolinas force-pushed the feature/board_samr30-xpro_ztimer branch from d6af2c4 to dda35f4 Compare March 9, 2022 14:10

boards/samr30-xpro: added ztimer configuration

96ac811

fjmolinas force-pushed the feature/board_samr30-xpro_ztimer branch from dda35f4 to 96ac811 Compare March 9, 2022 14:13

fjmolinas reviewed Mar 9, 2022

View reviewed changes

fjmolinas mentioned this pull request Mar 10, 2022

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

Merged

4 tasks

jue89 closed this Nov 4, 2022

	# NOTE: select the module here and not in the CPU so that order of inclusion
	# does not have periph_rtt selected earlier that it should be while at the same
	# time avoiding the module 'ztimer-no_periph_rtt' being included unecesarily.
	# 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 the fe310 rtt.
	ifneq (,$(filter sam% fe310,$(CPU)))
	USEMODULE += ztimer_no_periph_rtt
	endif

Conversation

jue89 commented May 6, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

benpicco commented Jun 17, 2020

Uh oh!

fjmolinas commented Aug 26, 2020

Uh oh!

benpicco commented Oct 21, 2020

Uh oh!

fjmolinas commented Apr 20, 2021

Uh oh!

jue89 commented Apr 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjmolinas Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlegHahm commented Mar 9, 2022

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

jue89 commented Mar 10, 2022

Uh oh!

fjmolinas commented Mar 10, 2022

Uh oh!

jue89 commented Mar 10, 2022

Uh oh!

fjmolinas commented Apr 7, 2022

Uh oh!

jue89 commented Nov 1, 2022

Uh oh!

dylad commented Nov 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fjmolinas Mar 10, 2022 •

edited

Loading