sys/ztimer: add 'ztimer_no_periph_rtt by fjmolinas · Pull Request #17099 · RIOT-OS/RIOT

fjmolinas · 2021-11-02T09:59:56Z

Contribution description

This is a temporary fix for Issue #17060. It allows disabling
auto inclusion of ztimer_periph_rt* in cases where another
module or application requires direct access.

Limitations:

as ifeq are involved order of inclusion matters, therefore
these modules should be included early in the build at the application
level and not in modules Makefile.dep
this does not disallow direct inclusions of ztimer_periph_rt*,
since this only disables auto inclusion of these modules

This is a temporary solution since this is already possible with
Kconfig, but not in make.

This implements the easiest fix proposed in #17060 to be able to move forward with the migration.

Testing procedure

USEMODULE="ztimer_msec ztimer_no_periph_rtt" BOARD=samr21-xpro make -C tests/ztimer_msg/ info-modules  | grep periph_rtt
ztimer_no_periph_rtt

DISABLE_MODULES=ztimer_periph_rtt_auto_select BOARD=samr21-xpro make -C tests/ztimer_msg/ info-modules  | grep periph_rtt

Issues/PRs references

#17060

kfessel · 2021-11-02T12:39:04Z

Why not just remove the automatic rtt rtc selection from makefile.dep?

periph_rtt is no good indicator for an rtt being able to power msec
periph_rtc does conflict with rtt on many devices

why did this change get merged with an KConfig PR #16909?

just revert 34259e1 and remove the Makefile.dep change in c80390f

fjmolinas · 2021-11-02T14:33:28Z

periph_rtt is no good indicator for an rtt being able to power msec

Why not?

periph_rtc does conflict with rtt on many devices

There isl FEATURES_CONFLICTING for that

why did this change get merged with an KConfig PR #16909?

You are surely right about that, I should have asked for a split, at the time I though it was an issue with the Makefile not being inline with Kconfig, but thas was my bad.

just revert 34259e1 and remove the Makefile.dep change in c80390f

I won't revert the change in Makefile.dep but I'll provide a removal of the default rtc seleciton.

benpicco · 2021-11-02T14:46:45Z

sys/include/ztimer.h

 *
+ * If `periph_rtt` or `periph_rtc` is required with direct access by another
+ * MODULE or an application, `ztimer_no_periph_rtt` or `ztimer_no_periph_rtc`
+ * can be included to avoid automatic selection of either `ztimer_periph_rtc`


Suggested change

* can be included to avoid automatic selection of either `ztimer_periph_rtc`

* must be included to avoid automatic selection of either `ztimer_periph_rtc`

i think there should also be a warning and link int the periph_rtt documentation about that automatic rtt selection of ztimer

kfessel

waiting for #17113
after that ztimer will not default to rtc

This no longer needs a Makefile.dep split

fjmolinas · 2021-11-04T08:41:50Z

@benpicco also mentione that a build-system warning would be desirable, I'll take another look on the next sprint

fjmolinas · 2021-11-09T09:48:54Z

@benpicco I added a warning in the periph/rtt.h header, in the ztimer.h header, and when building if both modules are included a warning is shown. Anything else you would suggest?

I also added in d336016 an alternative based on DEFAULT_MODULE

benpicco · 2021-11-09T10:01:37Z

The DEFAULT_MODULE solution sounds ideal, then modules that make use of the RTT API can just DISABLE_MODULE += ztimer_periph_rtt

fjmolinas · 2021-11-09T10:15:20Z

The DEFAULT_MODULE solution sounds ideal, then modules that make use of the RTT API can just DISABLE_MODULE += ztimer_periph_rtt

No, this is not the case. Rember that DEFAULT_MODULE only works if they are disabled before other dependencies are parsed, otherwise they will already be selected. It would need to be disabled in something included in makefiles/defaultmodules.inc.mk or in the application. DEFAULT_MODULE can not be disabled during dependency resolution because at that stage they were already converted to a USEMODULE

kfessel

no need for sys/ztimer/Makefile.ztimer.rtt.dep

sys/ztimer/Makefile.dep

sys/ztimer/Makefile.ztimer.rtt.dep

important no rtc default is adressed

kfessel

one (two) minor static-test-failures.

drivers/include/periph/rtt.h

fjmolinas · 2021-11-25T13:51:29Z

I'll squash if static tests pass.

kfessel · 2021-11-25T14:00:13Z

sys/ztimer/Makefile.dep


+  # If ztimer is used enable rtt autoselection for ztimer_msec, and ztimer_sec.
+  # This will enable inclusion of `ztimer_periph_rtt` if `periph_rtt` is supported.
+  DEFAULT_MODULE += ztimer_periph_rtt_auto_select


this line is not yet evaluated when l 96 is reached -> no auto selection is done

fjmolinas · 2021-11-25T14:01:43Z

sys/ztimer/Makefile.dep


+  # If ztimer is used enable rtt autoselection for ztimer_msec, and ztimer_sec.
+  # This will enable inclusion of `ztimer_periph_rtt` if `periph_rtt` is supported.
+  DEFAULT_MODULE += ztimer_periph_rtt_auto_select


Ajj true, that is why initially I had put in Makefile.default..., couldn't remember why and moved it out :/

I can't think of another solution than ae0bf90... it does mean that a ztimer module will show up even when ztimer is not used...

make -C examples/hello-world/ clean info-modules auto_init board core core_idle_thread core_init core_msg core_panic cpu native_drivers periph periph_common periph_gpio periph_gpio_linux periph_init periph_init_gpio periph_init_gpio_linux periph_init_pm periph_init_uart periph_pm periph_uart stdio_native sys ztimer_periph_rtt_auto_select

fjmolinas · 2021-11-25T14:09:18Z

Rebased because of conflict with master

leandrolanzieri · 2021-11-25T14:16:37Z

sys/include/ztimer.h

+ * If `periph_rtt` or is required with direct access by another MODULE or
+ * application, `ztimer_periph_rtt_auto_select` can be disabled to avoid automatic
+ * selection of `ztimer_periph_rtt` as a backend for ZTIMER_SEC and ZTIMER_MSEC.
+ * i.e.: `DISABLE_MODULE += ztimer_periph_rtt_auto_select`.


Could we maybe invert the logic? Making a module that avoids the default selection if present in the application makefile? then we wouldn't have the issue of the default module always showing up, right?

That was the initial approach I had, I can go back to that.

@kfessel thoughts?

i think the was a problem that lead to abondoning that approach (like could not be kconfig model???)

I split into an alternative the old versio so its easier to compare #17284

@leandrolanzieri @kfessel opinion on what option to prefer this one or #17284?

kfessel

Please Squash and activate ci:ready..

tested with make configuration (using test/ztimer_xsec) ✔️
I also tested with kconfig ✔️

and squash right in:

while reviewing these line threw me in the wrong way would you please remove the S in the make variables (even though these lines are not part of this PR)

sys/ztimer/Makefile.dep
12:  # but RIOT's auto_init scheme expects "auto_init_ztimer" in DEFAULT_MODULES so
13:  # it can be disabled (by adding to DISABLE_MODULES).
please change to 
DEFAULT_MODULE
DISABLE_MODULE

fjmolinas · 2021-11-29T08:16:57Z

sys/include/ztimer.h

 *              if if these are missing it will use same basic timer
 *              as ZTIMER_USEC does.
 *
+ * If `periph_rtt` or is required with direct access by another MODULE or


Suggested change

* If `periph_rtt` or is required with direct access by another MODULE or

* If `periph_rtt` is required with direct access by another MODULE or

fjmolinas · 2021-11-29T08:25:58Z

sys/ztimer/Makefile.dep
12: # but RIOT's auto_init scheme expects "auto_init_ztimer" in DEFAULT_MODULES so
13: # it can be disabled (by adding to DISABLE_MODULES).
please change to
DEFAULT_MODULE
DISABLE_MODULE

This was removed in another PR.

This is a temporary fix for Issue RIOT-OS#17060. It adds a module that enables the auto-inclusion of 'ztimer_periph_rtt'. This module, 'ztimer_periph_rtt_auto_select' is set as a DEFAULT_MODULE and can therefore be deselect at application level of in CPU/BOARDs 'Makefile.default'. Limitations: - This does not allow for modules to disable the auto-selection, e.g.: there can't be a depency where 'rtt_rtc' disables 'ztimer_periph_rtt_auto_select' - This does not disallow direct inclusions of `ztimer_periph_rt*`, since this only disables auto inclusion of these modules This is a temporary solution since this is already possible with Kconfig, but not in make.

fjmolinas · 2021-12-06T21:13:55Z

Superseeded by #17284

fjmolinas requested review from benpicco and kaspar030 November 2, 2021 09:59

fjmolinas requested review from MichelRottleuthner, aabadie, bergzand, cgundogan, jia200x, leandrolanzieri and smlng as code owners November 2, 2021 09:59

github-actions bot added Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework Area: timers Area: timer subsystems labels Nov 2, 2021

fjmolinas mentioned this pull request Nov 2, 2021

sys/ztimer: don't default to rtc for ztimer_sec #17113

Merged

benpicco reviewed Nov 2, 2021

View reviewed changes

kfessel previously requested changes Nov 2, 2021

View reviewed changes

fjmolinas force-pushed the pr_ztimer_no_periph_rt% branch from f2419c3 to acc1b8e Compare November 9, 2021 09:33

fjmolinas requested a review from MrKevinWeiss as a code owner November 9, 2021 09:37

github-actions bot added the Area: drivers Area: Device drivers label Nov 9, 2021

fjmolinas changed the title ~~sys/ztimer: add 'ztimer_no_periph_rt*%'~~ sys/ztimer: add 'ztimer_no_periph_rtt Nov 9, 2021

github-actions bot added the Area: build system Area: Build system label Nov 9, 2021

kfessel reviewed Nov 9, 2021

View reviewed changes

sys/ztimer/Makefile.dep Outdated Show resolved Hide resolved

sys/ztimer/Makefile.dep Outdated Show resolved Hide resolved

sys/ztimer/Makefile.dep Outdated Show resolved Hide resolved

sys/ztimer/Makefile.ztimer.rtt.dep Outdated Show resolved Hide resolved

fjmolinas mentioned this pull request Nov 9, 2021

ztimer_msec conflicts with direct usage of RTT #17060

Closed

fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 25, 2021

fjmolinas force-pushed the pr_ztimer_no_periph_rt% branch from bda703d to abb6c1e Compare November 25, 2021 12:20

github-actions bot removed the Area: build system Area: Build system label Nov 25, 2021

kfessel reviewed Nov 25, 2021

View reviewed changes

drivers/include/periph/rtt.h Outdated Show resolved Hide resolved

drivers/include/periph/rtt.h Show resolved Hide resolved

fjmolinas removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 25, 2021

fjmolinas force-pushed the pr_ztimer_no_periph_rt% branch from a85a19c to b35229c Compare November 25, 2021 13:57

fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 25, 2021

kfessel reviewed Nov 25, 2021

View reviewed changes

fjmolinas removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 25, 2021

fjmolinas commented Nov 25, 2021

View reviewed changes

github-actions bot added the Area: build system Area: Build system label Nov 25, 2021

tests: remove uneeded explicit inclusion of ztimer_periph_rt*

3f2520a

fjmolinas force-pushed the pr_ztimer_no_periph_rt% branch from 11deff0 to 860addb Compare November 25, 2021 14:09

leandrolanzieri reviewed Nov 25, 2021

View reviewed changes

kfessel reviewed Nov 26, 2021

View reviewed changes

fjmolinas commented Nov 29, 2021

View reviewed changes

fjmolinas added 2 commits November 29, 2021 09:26

sys/ztimer: add ztimer_periph_rtt module conflict warning

08d5b9e

fjmolinas force-pushed the pr_ztimer_no_periph_rt% branch from 860addb to 08d5b9e Compare November 29, 2021 08:26

fjmolinas mentioned this pull request Nov 29, 2021

sys/ztimer: add 'ztimer_no_periph_rtt' #17284

Merged

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 30, 2021

kaspar030 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 30, 2021

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

fjmolinas closed this Dec 6, 2021

emmanuelsearch removed this from the Release 2022.01 milestone Jan 27, 2022

fjmolinas deleted the pr_ztimer_no_periph_rt% branch June 9, 2022 09:21

	* can be included to avoid automatic selection of either `ztimer_periph_rtc`
	* must be included to avoid automatic selection of either `ztimer_periph_rtc`

	* If `periph_rtt` or is required with direct access by another MODULE or
	* If `periph_rtt` is required with direct access by another MODULE or

Conversation

fjmolinas commented Nov 2, 2021 • edited by kfessel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

kfessel commented Nov 2, 2021 • edited by fjmolinas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas commented Nov 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel Nov 2, 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 left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Nov 4, 2021

Uh oh!

fjmolinas commented Nov 9, 2021

Uh oh!

benpicco commented Nov 9, 2021

Uh oh!

fjmolinas commented Nov 9, 2021

Uh oh!

kfessel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kfessel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fjmolinas commented Nov 25, 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 commented Nov 25, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel left a comment • 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!

fjmolinas commented Nov 29, 2021

Uh oh!

fjmolinas commented Dec 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

fjmolinas commented Nov 2, 2021 •

edited by kfessel

Loading

kfessel commented Nov 2, 2021 •

edited by fjmolinas

Loading

kfessel Nov 2, 2021 •

edited

Loading

kfessel left a comment •

edited

Loading