Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sys/ztimer: add 'ztimer_no_periph_rtt' #17284

Merged
merged 3 commits into from Dec 2, 2021

Conversation

fjmolinas
Copy link
Contributor

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

@github-actions 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 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
@leandrolanzieri
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

kfessel commented Nov 30, 2021

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

Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 26 to 28
config MODULE_ZTIMER_NO_PERIPH_RTT
bool "Do not auto include RTT peripheral"

Copy link
Contributor

@kfessel kfessel Dec 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kfessel kfessel added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Dec 1, 2021
@fjmolinas
Copy link
Contributor Author

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 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
@fjmolinas
Copy link
Contributor Author

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.

Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please squash

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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -4,6 +4,8 @@ ifneq (,$(filter ztimer_xtimer_compat,$(USEMODULE)))
PSEUDOMODULES += xtimer
endif

MODULES_ZTIMER_ON_RTT_CONFLICT = ztimer_no_periph_rtt rtt_rtc gnrc_lwmac gnrc_gomach
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@github-actions github-actions bot added Area: boards Area: Board ports Area: CI Area: Continuous Integration of RIOT components labels Dec 2, 2021
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 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 github-actions bot removed Area: CI Area: Continuous Integration of RIOT components Area: boards Area: Board ports labels Dec 2, 2021
@fjmolinas fjmolinas changed the title sys/ztimer: add 'ztimer_no_periph_rtt sys/ztimer: add 'ztimer_no_periph_rtt' Dec 2, 2021
@aabadie aabadie merged commit b6cc070 into RIOT-OS:master Dec 2, 2021
@fjmolinas fjmolinas added this to the Release 2022.01 milestone Jan 21, 2022
@fjmolinas fjmolinas deleted the pr_ztimer_no_periph_rtt branch January 31, 2022 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants