sys/ztimer: rename required_pm_mode to block_pm_mode by kfessel · Pull Request #16160 · RIOT-OS/RIOT

kfessel · 2021-03-05T17:42:58Z

Contribution description

the mode that is stored in required_pm_mode would not keep clock running
but one mode higher therefor the timer needs to block that mode, the logic does it this way.

The define keeps it's name for compatiblity but its use should fade, when #15715 is merged,
and should be deprecated in the release after that.

this PR is backward compatible to previous name waiting for it to be deprecated and removed.

Testing procedure

build and test things that use pm_layerd and ztimer

Issues/PRs references

the name was introduced with #13722 maybe #13722 (comment)_ creted the name != function mismatch
this is in conflict with an other PR I am writing on: #15715 i will happily rebase either when the other is merged

maribu

See inline

maribu · 2021-03-10T12:40:06Z

sys/include/ztimer.h

+    union{
    uint8_t required_pm_mode;       /**< min. pm mode required for the clock to run */
+    uint8_t block_pm_mode;          /**< min. pm mode to block for the clock to run */
+    };


I wonder if we really need backward compatibility here. Isn't this an internal API?

But if we really want this:

Suggested change

union{

uint8_t required_pm_mode; /**< min. pm mode required for the clock to run */

uint8_t block_pm_mode; /**< min. pm mode to block for the clock to run */

};

union{

uint8_t required_pm_mode; /**< min. pm mode required for the clock to run */

uint8_t block_pm_mode; /**< min. pm mode to block for the clock to run */

};

i am not sure how internal and not internal API is decided but core/lifo i going through a deprecation cycle for maybe beeing not internal even though no one could name a valid usecase

I personaly think this is only used via the PM_MODE defines and therefor internal but since the require does not take up any space i am ok with deprecation and deletion

Deprecation comes at a cost: This increases complexity and can cause confusion. Also, new users of deprecated API occasionally still pop up even many months after. So, if there is no (valid) external use, we should IMO avoid the effort.

Maybe @kaspar030 and @bergzand have an opinion on this?

sys/ztimer/auto_init.c

sys/include/ztimer.h

fjmolinas · 2021-04-02T07:11:32Z

Deprecation comes at a cost: This increases complexity and can cause confusion. Also, new users of deprecated API occasionally still pop up even many months after. So, if there is no (valid) external use, we should IMO avoid the effort.

Maybe @kaspar030 and @bergzand have an opinion on this?

I think in this case keeping the compatibility is not that expensive, but I don't think the header is clear enough, the union element should have a deprecated tag. But at the same time ztimer is still considered experimental, si I think users should expect the API to change. So, in conclusion, is a similar case would come up and this union solution is applicable I would say why not, but in this case since the API is experimental I would simply change it.

fjmolinas · 2021-04-02T07:11:42Z

This one needs a rebase BTW

kfessel · 2021-04-02T15:10:02Z

rebased (@maribu s sugestions are squash into the new intial commit)

kfessel · 2021-04-02T16:55:19Z

Murdock had some time (no builds running) therefor i added the (CI: ready for build) label: Murdock found no issues.
Please remove if this jumps the Discussion ( afaik it is: no review label )

kfessel · 2021-04-02T16:59:12Z

there is still no response from @kaspar030 and/or @bergzand

should i just remove the union thing?
or
should i add a deprecation marker?

kfessel · 2021-04-04T10:48:31Z

i went with deprecating for now

i would also deprecate the use of CONFIG_ZTIMER_*SEC_REQUIRED_PM_MODE but i do not know how
CONFIG_ZTIMER_(TIMER/RTC/RTT)_BLOCK_PM_MODE should be used. (this honor the change requested by #15911 and implemented by #16172)

kaspar030 · 2021-04-06T09:18:43Z

IMO just renaming would have been fine, but now the deprecation is there, also fine!

What puzzles me more is that the anonymous union actually passed murdock. AFAIK they're c11 only, and our AVR toolchain doesn't support that? Is any ztimer code built on CI for AVR?

kaspar030 · 2021-04-06T09:30:17Z

What puzzles me more is that the anonymous union actually passed murdock. AFAIK they're c11 only, and our AVR toolchain doesn't support that? Is any ztimer code built on CI for AVR?

Seems like AVR is actually on C11, so no blocker here.

So, please squash!

kaspar030

ACK.

kfessel · 2021-04-06T11:43:05Z

Murdock failed with (unrelated i think):
https://ci.riot-os.org/RIOT-OS/RIOT/16160/20ae4bce9e1008ec8193dbdad8b11bcae6cdc9c5/output/makefile_broken.txt
I removed the (CI:ready for build) label.

kaspar030 · 2021-04-06T12:10:36Z

Hm, this doesn't change any makefiles. Could you give this another rebase, then try restarting the build?

maribu · 2021-04-06T12:21:41Z

sys/include/ztimer.h

+    union {
+        uint8_t required_pm_mode;       /**< min. pm mode required for the clock to run
+                                         *   @deprecated name change -> block_pm_mode:
+                                         *               this is used to block a pm_mode
+                                         *               use block_pm_mode instead */
+        uint8_t block_pm_mode;          /**< min. pm mode to block for the clock to run */
+    };


Do we really consider this a user facing API that needs deprecation? IMO this is internal and we should just rename it and be done with it.

Fine with me

so @kfessel, sorry for the confusion, please just remove the deprecation stuff...

I'm also fine with just renaming it. This struct member is just touched by auto_init.c. At least in all my use cases ;)

done (both variants are good from my pov)

kfessel · 2021-04-06T14:59:21Z

i removed the deprecation part

maribu · 2021-04-06T18:38:00Z

@kaspar030 merge?

kfessel · 2021-04-07T09:52:51Z

next stop: round robin scheduling #16126

kfessel requested review from bergzand and kaspar030 as code owners March 5, 2021 17:42

fjmolinas requested a review from jue89 March 6, 2021 13:57

fjmolinas added Area: timers Area: timer subsystems Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Mar 6, 2021

kfessel mentioned this pull request Mar 9, 2021

sys/ztimer: add ZTIMER_SEC, improve auto_init #16172

Merged

maribu reviewed Mar 10, 2021

View reviewed changes

kfessel commented Mar 10, 2021

View reviewed changes

sys/include/ztimer.h Outdated Show resolved Hide resolved

kfessel commented Mar 10, 2021

View reviewed changes

sys/include/ztimer.h Outdated Show resolved Hide resolved

kfessel force-pushed the p-block-required branch from c2ab655 to 56c7239 Compare April 2, 2021 15:08

kfessel added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 2, 2021

kfessel force-pushed the p-block-required branch from 4c8edb0 to 20ae4bc Compare April 6, 2021 09:34

kaspar030 approved these changes Apr 6, 2021

View reviewed changes

kfessel removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 6, 2021

kfessel force-pushed the p-block-required branch from 20ae4bc to 8f0e47a Compare April 6, 2021 12:11

kfessel added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 6, 2021

maribu reviewed Apr 6, 2021

View reviewed changes

sys/ztimer: rename required_pm_mode to block_pm_mode

951fa14

kfessel force-pushed the p-block-required branch from 8f0e47a to 951fa14 Compare April 6, 2021 14:58

kaspar030 merged commit 04b5627 into RIOT-OS:master Apr 7, 2021

kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021

Conversation

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

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

maribu left a comment

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!

Uh oh!

Uh oh!

Uh oh!

fjmolinas commented Apr 2, 2021

Uh oh!

fjmolinas commented Apr 2, 2021

Uh oh!

kfessel commented Apr 2, 2021

Uh oh!

kfessel commented Apr 2, 2021

Uh oh!

kfessel commented Apr 2, 2021

Uh oh!

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

Uh oh!

Uh oh!

kaspar030 commented Apr 6, 2021

Uh oh!

kaspar030 commented Apr 6, 2021

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

kaspar030 commented Apr 6, 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!

kfessel Apr 6, 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!

kfessel commented Apr 6, 2021

Uh oh!

maribu commented Apr 6, 2021

Uh oh!

kfessel commented Apr 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kfessel commented Mar 5, 2021 •

edited

Loading

kfessel commented Apr 4, 2021 •

edited

Loading

kfessel commented Apr 6, 2021 •

edited

Loading

kfessel Apr 6, 2021 •

edited

Loading