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

fjmolinas · 2021-11-29T08:31:46Z

Contribution description

This PR splits the initial approach in #17099 to its own PR to be easier to compare. Instead of DEFAULT_MODULE a module can be included to disable auto-inclusion. The advantage being that there is no ztime_auto_select_rtt module used even where ztimeris not used.

    sys/ztimer: add 'ztimer_no_periph_rtt'
    
    This is a temporary fix for Issue #17060. It allows to disable
    auto inclusion of `ztimer_periph_rtt` 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 application
      level and not in modules `Makefile.dep`
    - this does not disallow direct inclusions of `ztimer_periph_rtt`,
      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.

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

Alternative to #17099

leandrolanzieri · 2021-11-30T13:18:21Z

this does not disallow direct inclusions of ztimer_periph_rtt, since this only disables auto inclusion of these modules

We could run a sanity check after dependency resolution in this case, right? To see if someone requested too late not to use RTT periph.

fjmolinas · 2021-11-30T13:50:17Z

We could run a sanity check after dependency resolution in this case, right? To see if someone requested too late not to use RTT periph.

Ah yes it could be added to the warning.

kfessel · 2021-11-30T13:51:24Z

I am with either of this or #17099

(I was just waiting for Murdock (and a minor issue (solved)) to approve it, while waiting this one was created)

This one seems a bit simpler and easier to understand (I my self did not use DISABLE_MODULE before i tested 17099)

Reading this seems: OK

I would test this (same way I tested 17099) if you like it one more.

(I would only approve one of them because they are in conflict (solving the same thing))

kfessel

It test as i did with #17099 (using test/ztimer_xsec)

USEMODULE=USEMODULE=ztimer_no_periph_rtt

tested with make configuration ✔️
tested with kconfig ✔️

in both cases i check the result with enabled an disabled "do not auto select rtt"

it think this PR is easier to understand

Please decide which one you prefer

I would like the kconfig option to be a little more visible see below

Other than that I approve this xor #17099

kfessel · 2021-12-01T15:42:54Z

sys/ztimer/Kconfig

+config MODULE_ZTIMER_NO_PERIPH_RTT
+    bool "Do not auto include RTT peripheral"
+


Suggested change

config MODULE_ZTIMER_NO_PERIPH_RTT

bool "Do not auto include RTT peripheral"

#move this up

config MODULE_ZTIMER_NO_PERIPH_RTT

bool "do not auto include ztimer_periph_rtt"

help

This module disables the auto-inclusion of ztimer_periph_rtt as a backend

for ztimer_msec and ztimer_sec if those are selected and periph_rtt is

available

I think these line could be somelines up (not hidden in a hidden by default submenu

#17099 has the equivalent option in line 10

fjmolinas · 2021-12-01T16:01:49Z

It test as i did with #17099 (using test/ztimer_xsec)

Lets go with this one, I don't like having an unused module show up. Let me know if its OK to squash.

fjmolinas · 2021-12-01T16:03:00Z

It test as i did with #17099 (using test/ztimer_xsec)

Lets go with this one, I don't like having an unused module show up. Let me know if its OK to squash.

In the fix-ups I added a warning if ztimer-periph-rtt ztimer-no-periph-rtt are included, as well as fixing the Kconfig placement as you suggested.

kfessel

please squash

kfessel · 2021-12-01T17:20:17Z

sys/ztimer/Makefile.include

+  MODULES_ZTIMER_ON_RTT_CONFLICTING = $(filter $(MODULES_ZTIMER_ON_RTT_CONFLICT),$(USEMODULE))
+  ifneq (0,$(words $(MODULES_ZTIMER_ON_RTT_CONFLICTING)))
+    $(info $(COLOR_YELLOW)WARNING! The following modules conflict with 'ztimer_periph_rtt': '$(MODULES_ZTIMER_ON_RTT_CONFLICTING)')
+    $(info To disable ztimer periph_rtt auto-inclusion add 'ztimer_no_periph_rtt' to 'USEMODULE'$(COLOR_RESET))


I am not sure if a warning should be triggert by ztimer_no_periph_rtt:
Especialy if the warning suggest to do the very thing "add 'ztimer_no_periph_rtt' to 'USEMODULE'" that triggert it

WARNING! The following modules conflict with 'ztimer_periph_rtt': 'ztimer_no_periph_rtt' To disable ztimer periph_rtt auto-inclusion add 'ztimer_no_periph_rtt' to 'USEMODULE'

also having ztimer_no_periph_rtt is not in conflict with ztimer_periph_rtt (since the option is more like ztimer_no_auto_periph_rtt)

$ BOARD=nucleo-f767zi USEMODULE='ztimer_periph_rtt ztimer_no_periph_rtt' make info-modules WARNING! The following modules conflict with 'ztimer_periph_rtt': 'ztimer_no_periph_rtt' To disable ztimer periph_rtt auto-inclusion add 'ztimer_no_periph_rtt' to 'USEMODULE' ... ztimer_no_periph_rtt ztimer_periph_rtt ztimer_periph_timer ...

Having another think about this: I am sure that this warning should not betriggert since ztimer_no_periph_rtt does not override ztimer_periph_rtt

It does not but if you include it in the wrong place (not application or BOARD level) then you would get a warning. But Ok will remove.

kfessel · 2021-12-01T17:30:20Z

sys/ztimer/Makefile.include

  PSEUDOMODULES += xtimer
 endif

+MODULES_ZTIMER_ON_RTT_CONFLICT = ztimer_no_periph_rtt rtt_rtc gnrc_lwmac gnrc_gomach


Suggested change

MODULES_ZTIMER_ON_RTT_CONFLICT = ztimer_no_periph_rtt rtt_rtc gnrc_lwmac gnrc_gomach

MODULES_ZTIMER_ON_RTT_CONFLICT = rtt_rtc gnrc_lwmac gnrc_gomach

This is a temporary fix for Issue RIOT-OS#17060. It allows to disable auto inclusion of `ztimer_periph_rtt` 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 application level and not in modules `Makefile.dep` - this does not disallow direct inclusions of `ztimer_periph_rtt`, 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 requested review from kfessel and leandrolanzieri November 29, 2021 08:31

fjmolinas requested review from MichelRottleuthner, MrKevinWeiss, aabadie, bergzand, cgundogan, jia200x, kaspar030 and smlng as code owners November 29, 2021 08:31

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

fjmolinas mentioned this pull request Nov 29, 2021

sys/ztimer: add 'ztimer_no_periph_rtt #17099

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 29, 2021

kfessel reviewed Dec 1, 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 Dec 1, 2021

kfessel approved these changes Dec 1, 2021

View reviewed changes

fjmolinas force-pushed the pr_ztimer_no_periph_rtt branch from e1dd6fd to 9b88539 Compare December 2, 2021 06:54

github-actions bot added Area: boards Area: Board ports Area: CI Area: Continuous Integration of RIOT components labels Dec 2, 2021

fjmolinas added 3 commits December 2, 2021 07:54

tests: remove uneeded explicit inclusion of ztimer_periph_rt*

84ba92a

sys/ztimer: add ztimer_periph_rtt module conflict warning

638373e

fjmolinas force-pushed the pr_ztimer_no_periph_rtt branch from 9b88539 to 638373e Compare December 2, 2021 06:54

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

github-actions bot removed Area: CI Area: Continuous Integration of RIOT components Area: boards Area: Board ports labels Dec 2, 2021

fjmolinas changed the title ~~sys/ztimer: add 'ztimer_no_periph_rtt~~ sys/ztimer: add 'ztimer_no_periph_rtt' Dec 2, 2021

aabadie merged commit b6cc070 into RIOT-OS:master Dec 2, 2021

leandrolanzieri mentioned this pull request Dec 10, 2021

sys/ztimer: fix backend selection #17372

Merged

fjmolinas added this to the Release 2022.01 milestone Jan 21, 2022

fjmolinas deleted the pr_ztimer_no_periph_rtt branch January 31, 2022 09:00

kfessel mentioned this pull request Feb 18, 2022

ztimer_msec conflicts with direct usage of RTT #17060

Closed

		config MODULE_ZTIMER_NO_PERIPH_RTT
		bool "Do not auto include RTT peripheral"

-config MODULE_ZTIMER_NO_PERIPH_RTT
-    bool "Do not auto include RTT peripheral"
+#move this up
+config MODULE_ZTIMER_NO_PERIPH_RTT
+    bool "do not auto include ztimer_periph_rtt"
+    help
+        This module disables the auto-inclusion of ztimer_periph_rtt as a backend
+        for ztimer_msec and ztimer_sec if those are selected and periph_rtt is
+        available

	MODULES_ZTIMER_ON_RTT_CONFLICT = ztimer_no_periph_rtt rtt_rtc gnrc_lwmac gnrc_gomach
	MODULES_ZTIMER_ON_RTT_CONFLICT = rtt_rtc gnrc_lwmac gnrc_gomach

Conversation

fjmolinas commented Nov 29, 2021

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

leandrolanzieri commented Nov 30, 2021

Uh oh!

fjmolinas commented Nov 30, 2021

Uh oh!

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

Uh oh!

Uh oh!

kfessel left a comment

Choose a reason for hiding this comment

Uh oh!

kfessel Dec 1, 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!

fjmolinas commented Dec 1, 2021

Uh oh!

fjmolinas commented Dec 1, 2021

Uh oh!

kfessel left a comment

Choose a reason for hiding this comment

Uh oh!

kfessel Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

kfessel Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

kfessel Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

fjmolinas Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

kfessel Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kfessel commented Nov 30, 2021 •

edited

Loading

kfessel Dec 1, 2021 •

edited

Loading