ztimer: fix required_pm_mode initialization by vincent-d · Pull Request #15911 · RIOT-OS/RIOT

vincent-d · 2021-02-02T09:49:04Z

Contribution description

This might be flawed, but from my understanding:

the required_pm_mode should be applied to the 'base' clock (i.e. the periph_timer or periph_rtt clock)
'convert' clocks do not have pm requirement

This PR sets required_pm_mode on ZTIMER_USEC_BASE and ZTIMER_MSEC_BASE. Setting this field is also moved before those clocks are used as they can be used in ztimer_convert_frac_init().
I also changed 'convert' clocks init so they set their internal required_pm_mode to ZTIMER_CLOCK_NO_REQUIRED_PM_MODE instead of nothing (which makes it 0, thus blocking a power mode by error).

As a last minor change, I fixed the initialization of 'adjust' parameter for the ZTIMER_MSEC clock as the field was renamed.

Testing procedure

I tested with our main application and checked that pm was behaving as expected. Before this change, it could never go to sleep.

For ZTIMER_USEC and ZTIMER_MSEC, the required_pm_mode should be set on the base clock (ZTIMER_USEC_BASE and ZTIMER_MSEC_BASE respectively) as they represent the hardware clocks that have pm constraints.

convert and convert_frac clocks do not have om constraints, thus setting their required_pm_mode to ZTIMER_CLOCK_NO_REQUIRED_PM_MODE.

adjust do not exist anymore and have been replaced by adjust_set and adjust_sleep.

kaspar030 · 2021-02-02T10:01:45Z

@jue89 I think this change is valid, could you double check?

jue89 · 2021-02-02T11:49:55Z

Good catch! I never unblocked PM mode 0 before ... so this bug never showed up on my application.

Code-wise this looks good to me, too.
Unfortunately, I have no Board at hand, currently. I could test tomorrow on a samr30-xpro.

Furthermore, no board use CONFIG_ZTIMER_.*_ADJUST, no fixes required there as well.

kfessel · 2021-02-05T15:53:46Z

#15715 or this one has to be merged first the other has to be modified

my first impression is that #15715 should go first and this should modifie ZTIMER_SEC after rebase

also this PR should investigate if "ZTIMER_CLOCK_NO_REQUIRED_PM_MODE" is a good(safe) default

kfessel · 2021-02-05T16:02:12Z

it is not ztimer(which scale ever) that needs a specific pm but its base timer there for the configuration value should be
(CONFIG_ZTIMER_RTT_REQUIRED_PM_MODE or CONFIG_ZTIMER_RTC_REQUIRED_PM_MODE)

kfessel · 2021-02-05T17:18:15Z

I am also very fine with rebaseing and editing #15715 after this is merged. If this is faster.
The safe default investigation can be part of another PR. (pm is blocked by default therefor the basic default is safe until unblocked).
Maybe the change of the configuration logic even thoug this touches it should be part of an other PR.
I am not sure how to rename the ZTIMER_USEC_REQUIRED_PM_MODE maybe it is ZTIMER_TIM(ER0)_REQUIRED_PM_MODE

jue89

As advertised, I've tested this PR using the samr30-xpro. Sorry that this took so long ...

Power-savings are still achieved - so I'm fine with this PR. Just some minor suggestions.

Hacked-together test based on the hello-world example

diff --git a/examples/hello-world/Makefile b/examples/hello-world/Makefile
index 258d8e9baf..b72257d9ff 100644
--- a/examples/hello-world/Makefile
+++ b/examples/hello-world/Makefile
@@ -12,6 +12,16 @@ RIOTBASE ?= $(CURDIR)/../..
 # development process:
 DEVELHELP ?= 1
 
+USEMODULE += gnrc_netdev_default
+USEMODULE += auto_init_gnrc_netif
+
+USEMODULE += ztimer ztimer_usec ztimer_msec ztimer_periph_rtt ztimer_xtimer_compat
+USEMODULE += pm_layered
+
+CFLAGS += '-DCONFIG_ZTIMER_MSEC_REQUIRED_PM_MODE=0'
+CFLAGS += '-DCONFIG_ZTIMER_USEC_REQUIRED_PM_MODE=1'
+CFLAGS += '-DPM_BLOCKER_INITIAL=0x00000000'
+
 # Change this to 0 show compiler invocation lines by default:
 QUIET ?= 1
 
diff --git a/examples/hello-world/main.c b/examples/hello-world/main.c
index f51bf8c0a0..fd915fb6b5 100644
--- a/examples/hello-world/main.c
+++ b/examples/hello-world/main.c
@@ -21,6 +21,14 @@
 
 #include <stdio.h>
 
+#include "net/netif.h"
+#include "ztimer.h"
+#include "timex.h"
+
+static void callback (void * arg) {
+       puts((char*) arg);
+}
+
 int main(void)
 {
     puts("Hello World!");
@@ -28,5 +36,20 @@ int main(void)
     printf("You are running RIOT on a(n) %s board.\n", RIOT_BOARD);
     printf("This board features a(n) %s MCU.\n", RIOT_MCU);
 
+    /* The SAM-R30 board boots with its netif enabled and draws several mA of current. */
+    /* Send it to sleep mode ... */
+    netif_t *iface = netif_iter(NULL);
+    netopt_state_t state = NETOPT_STATE_SLEEP;
+    netif_set_opt(iface, NETOPT_STATE, 0, &state, sizeof(netopt_state_t));
+
+    /* 1. Run a callback after 3s */
+    static ztimer_t cb_timer = {.callback = callback, .arg = "Hello World"};
+    ztimer_set(ZTIMER_USEC, &cb_timer, 3 * US_PER_SEC);
+
+    /* 2. Sleep the current thread for 60s */
+    ztimer_sleep(ZTIMER_MSEC, 60 * MS_PER_SEC);
+
+    while (true) {}
+
     return 0;
 }

jue89 · 2021-02-08T10:17:51Z

sys/ztimer/auto_init.c

+    LOG_DEBUG("ztimer_init(): ZTIMER_MSEC setting adjust_sleep value to %i\n",
+              CONFIG_ZTIMER_USEC_ADJUST_SLEEP );


Suggested change

LOG_DEBUG("ztimer_init(): ZTIMER_MSEC setting adjust_sleep value to %i\n",

CONFIG_ZTIMER_USEC_ADJUST_SLEEP );

LOG_DEBUG("ztimer_init(): ZTIMER_MSEC setting adjust_sleep value to %i\n",

CONFIG_ZTIMER_USEC_ADJUST_SLEEP);

jue89 · 2021-02-08T11:46:01Z

sys/ztimer/auto_init.c

+#  ifdef MODULE_PM_LAYERED
+    LOG_DEBUG("ztimer_init(): ZTIMER_USEC setting required_pm_mode to %i\n",
+              CONFIG_ZTIMER_USEC_REQUIRED_PM_MODE);
+    ZTIMER_USEC_BASE->required_pm_mode = CONFIG_ZTIMER_USEC_REQUIRED_PM_MODE;
+#  endif


This section can cause errors during compilation if CONFIG_ZTIMER_USEC_REQUIRED_PM_MODE is configured with the wrongly typed value:

In file included from /home/jue/Projects/RIOT/sys/ztimer/auto_init.c:48: /home/jue/Projects/RIOT/sys/ztimer/auto_init.c: In function 'ztimer_init': /home/jue/Projects/RIOT/sys/ztimer/auto_init.c:115:15: error: format '%i' expects argument of type 'int', but argument 2 has type 'long unsigned int' [-Werror=format=] 115 | LOG_DEBUG("ztimer_init(): ZTIMER_USEC setting required_pm_mode to %i\n", | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This happens, if I use the defines made by the cpu vendor.

I'd say the easy solution is to set ZTIMER_USEC_BASE->required_pm_mode and then print that value. If I'm familiar enough with GCC and clang this should fix casting issues due to implicit casting.

Suggested change

# ifdef MODULE_PM_LAYERED

LOG_DEBUG("ztimer_init(): ZTIMER_USEC setting required_pm_mode to %i\n",

CONFIG_ZTIMER_USEC_REQUIRED_PM_MODE);

ZTIMER_USEC_BASE->required_pm_mode = CONFIG_ZTIMER_USEC_REQUIRED_PM_MODE;

# endif

# ifdef MODULE_PM_LAYERED

ZTIMER_USEC_BASE->required_pm_mode = CONFIG_ZTIMER_USEC_REQUIRED_PM_MODE;

LOG_DEBUG("ztimer_init(): ZTIMER_USEC setting required_pm_mode to "PRIu8"\n",

ZTIMER_USEC_BASE->required_pm_mode);

# endif

The same goes with the respective MSEC section.

What do you think?
Maybe @kaspar030 has an opinion on this, too?

If you apply this, please use the past tense in the debug message. (setting -> set)

kfessel · 2021-02-09T21:59:16Z

@vincent-d: would you please check if current #15715 also fixes this.
I moved the PM_MODE configuration to the ZTIMER hardware bases.
basically I did what I wrote in my comments.

fjmolinas · 2021-03-30T14:04:57Z

ping @vincent-d :)

kfessel · 2021-04-04T10:53:35Z

@vincent-d: #16172 should have solved fix this.

vincent-d · 2021-04-14T10:24:41Z

@kfessel sorry for the long delay. Indeed, it should fix the initialization order issue. But if I'm not mistaken, the convert clock pm mode is still not initialized, though.

kfessel · 2021-04-14T11:17:13Z

@vincent-d: I think we don't need to init the convert_clocks (highlevel like *SEC) pm_mode_block if we init the base clocks (rtt timer or rtc) pm_mode_block

maribu · 2021-04-14T11:47:04Z

I also changed 'convert' clocks init so they set their internal required_pm_mode to ZTIMER_CLOCK_NO_REQUIRED_PM_MODE instead of nothing (which makes it 0, thus blocking a power mode by error).

This makes a lot of sense to me. I didn't check to confirm that this does actually cause issues. But IMO purely virtual clocks should use ZTIMER_CLOCK_NO_REQUIRED_PM_MODE just because this is the most sensible value there - so even if there is currently no code relying on their value being sensible.

maribu · 2021-04-14T11:47:39Z

@vincent-d: Care to rebase and update?

kfessel · 2021-04-14T12:19:56Z

ztimer/convert.c
ztimer/convert_frac.c
ztimer/convert_muldiv64.c
ztimer/convert_shift.c

need their init fuctions

super.super.block_pm_mode = ZTIMER_CLOCK_NO_REQUIRED_PM_MODE,

it just worked cause i never activated pm_mode 0

fjmolinas · 2021-06-11T07:57:26Z

Any updates here?

fjmolinas · 2021-06-15T10:27:05Z

ztimer/convert.c
ztimer/convert_frac.c
ztimer/convert_muldiv64.c
ztimer/convert_shift.c
need their init fuctions
super.super.block_pm_mode = ZTIMER_CLOCK_NO_REQUIRED_PM_MODE,
it just worked cause i never activated pm_mode 0

@kfessel can you provide a patch for what you identified is missing here?

kfessel · 2021-06-18T16:46:56Z

see #16573

fjmolinas · 2021-06-28T08:04:26Z

see #16573

This PR is now addressed with #16573 being in.

Vincent Dupont added 3 commits February 2, 2021 10:36

ztimer: fix required_pm_mode initialization

bb0257a

For ZTIMER_USEC and ZTIMER_MSEC, the required_pm_mode should be set on the base clock (ZTIMER_USEC_BASE and ZTIMER_MSEC_BASE respectively) as they represent the hardware clocks that have pm constraints.

ztimer/convert: set required_pm_mode for convert clocks

4c1b4f2

convert and convert_frac clocks do not have om constraints, thus setting their required_pm_mode to ZTIMER_CLOCK_NO_REQUIRED_PM_MODE.

ztimer: fix adjust_* init for ZTIMER_MSEC

97be6f5

adjust do not exist anymore and have been replaced by adjust_set and adjust_sleep.

vincent-d added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: timers Area: timer subsystems labels Feb 2, 2021

vincent-d requested review from bergzand and kaspar030 as code owners February 2, 2021 09:49

vincent-d requested a review from maribu February 2, 2021 09:49

jue89 self-requested a review February 2, 2021 15:13

maribu mentioned this pull request Feb 5, 2021

sys/ztimer: add ZTIMER_SEC #15715

Closed

jue89 requested changes Feb 8, 2021

View reviewed changes

kfessel mentioned this pull request Mar 9, 2021

sys/ztimer: add ZTIMER_SEC, improve auto_init #16172

Merged

kfessel mentioned this pull request Apr 4, 2021

sys/ztimer: rename required_pm_mode to block_pm_mode #16160

Merged

kfessel mentioned this pull request Jun 18, 2021

sys/ztimer: convert clock do not require pm #16573

Merged

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

fjmolinas closed this Jun 28, 2021

fjmolinas removed this from the Release 2021.07 milestone Jun 28, 2021

		LOG_DEBUG("ztimer_init(): ZTIMER_MSEC setting adjust_sleep value to %i\n",
		CONFIG_ZTIMER_USEC_ADJUST_SLEEP );

Conversation

vincent-d commented Feb 2, 2021

Contribution description

Testing procedure

Uh oh!

kaspar030 commented Feb 2, 2021

Uh oh!

jue89 commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfessel commented Feb 5, 2021

Uh oh!

kfessel commented Feb 5, 2021

Uh oh!

kfessel commented Feb 5, 2021

Uh oh!

jue89 left a comment

Choose a reason for hiding this comment

Uh oh!

jue89 Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

jue89 Feb 8, 2021 • 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!

maribu Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

kfessel commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas commented Mar 30, 2021

Uh oh!

kfessel commented Apr 4, 2021

Uh oh!

vincent-d commented Apr 14, 2021

Uh oh!

kfessel commented Apr 14, 2021

Uh oh!

maribu commented Apr 14, 2021

Uh oh!

maribu commented Apr 14, 2021

Uh oh!

kfessel commented Apr 14, 2021

Uh oh!

fjmolinas commented Jun 11, 2021

Uh oh!

fjmolinas commented Jun 15, 2021

Uh oh!

kfessel commented Jun 18, 2021

Uh oh!

fjmolinas commented Jun 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jue89 commented Feb 2, 2021 •

edited

Loading

jue89 Feb 8, 2021 •

edited

Loading

kfessel commented Feb 9, 2021 •

edited

Loading