event/timeout: remove forced ZTIMER_USEC dependency [alternative] by fjmolinas · Pull Request #16958 · RIOT-OS/RIOT

fjmolinas · 2021-10-06T16:39:54Z

Contribution description

An alternative to #16892

Basically, instead of adding a new module for the legacy API, make event_timeout_ztimer a module not depending on event_timeout and if only event_timeout_ztimer is selected only the new api is implemented (no forced backend). If event_timeout and event_timeout_ztimer are selected then ztimer_usec is pulled in for a drop in replacement.

Testing procedure

Since this is just changing dependencies, the code should still compile. On examples/nimble_heart_rate_sensor no usec timer is pulled in.

in tests/events xtimer is pulled in by default, if ztimer and ztimer_usec are also pulled in the xtimer_on_ztimer is added. If event_timeout_ztimer is also added then xtimer is not pulled in but ztimer_usec:

make -C tests/events clean info-modules 
auto_init
auto_init_xtimer
board
core
core_idle_thread
core_init
core_msg
core_panic
core_thread_flags
cpu
div
event
event_callback
event_timeout
event_timeout_xtimer
native_drivers
periph
periph_common
periph_gpio
periph_gpio_linux
periph_init
periph_init_gpio
periph_init_gpio_linux
periph_init_pm
periph_init_timer
periph_init_uart
periph_pm
periph_timer
periph_uart
stdin
stdio_native
sys
test_utils_interactive_sync
xtimer

USEMODULE="ztimer ztimer_usec" make -C tests/events clean info-modules 
auto_init
auto_init_xtimer
auto_init_ztimer
board
core
core_idle_thread
core_init
core_msg
core_panic
core_thread_flags
cpu
div
event
event_callback
event_timeout
event_timeout_xtimer
frac
native_drivers
periph
periph_common
periph_gpio
periph_gpio_linux
periph_init
periph_init_gpio
periph_init_gpio_linux
periph_init_pm
periph_init_timer
periph_init_uart
periph_pm
periph_timer
periph_uart
stdin
stdio_native
sys
test_utils_interactive_sync
xtimer
xtimer_on_ztimer
ztimer
ztimer_auto_init
ztimer_convert
ztimer_convert_frac
ztimer_convert_shift
ztimer_core
ztimer_extend
ztimer_periph_timer
ztimer_usec

USEMODULE="event_timeout_ztimer" make -C tests/events clean info-modules 
auto_init
auto_init_ztimer
board
core
core_idle_thread
core_init
core_msg
core_panic
core_thread_flags
cpu
event
event_callback
event_timeout
event_timeout_ztimer
frac
native_drivers
periph
periph_common
periph_gpio
periph_gpio_linux
periph_init
periph_init_gpio
periph_init_gpio_linux
periph_init_pm
periph_init_timer
periph_init_uart
periph_pm
periph_timer
periph_uart
stdin
stdio_native
sys
test_utils_interactive_sync
ztimer
ztimer_auto_init
ztimer_convert
ztimer_convert_frac
ztimer_convert_shift
ztimer_core
ztimer_extend
ztimer_periph_timer
ztimer_usec

Issues/PRs references

haukepetersen · 2021-10-06T17:03:53Z

Looks good to me, I like it better than the approach in #16892. So lets roll with it!

fjmolinas · 2021-10-07T09:53:37Z

Looks good to me, I like it better than the approach in #16892. So lets roll with it!

To add Kconfig for this I had to adapt the dependency a bit so that there is no circular dependency with Kconfig. To avoid using xtimer, then event_timeout_ztimer needs to be included. Otherwise, if ztimer is included then the user will have to live with the (in most cases negligible) overhead off xtimer_on_ztimer.

fjmolinas · 2021-10-07T09:59:42Z

Added Kconfig and updated test procedure

kfessel · 2021-10-08T19:29:05Z

should n't
USEMODULE="ztimer ztimer_usec" make -C tests/events clean info-modules
make use of

event_timeout_ztimer

fjmolinas · 2021-10-26T07:09:00Z

should n't USEMODULE="ztimer ztimer_usec" make -C tests/events clean info-modules make use of
event_timeout_ztimer

The PR description was wrong, ztimer ztimer_usec do not trigger automatic inclusion of event_timeout_ztimer. This only leads to xtimer_on_ztimer being used.

kfessel · 2021-10-26T09:19:09Z

wouldn't xtimer_compat be the better option in case of USEMODULE="ztimer ztimer_usec"?
afaik the event API does not profit from the out of compat specifics of xtimer but xtimer_on_ztimer has non small overhead

fjmolinas · 2021-10-26T09:26:38Z

xtimer_compat

xtimer_compat is not a drop in replacement and currently there is a warning in the build system:

# "ztimer_xtimer_compat" is a wrapper of the xtimer API on ztimer_used
# (it is currently incomplete). Unless doing testing, use "xtimer_on_ztimer".

kfessel · 2021-10-29T11:41:25Z

I actually just use the header #include ztimer/xtimer_compat.h as an interface to ztimer_usec (read: do not include the xtimer_compat-module), this is totaly safe since it only applays to that compile step. That way i can be sure even though anything else is needing xtimer the thing that i added the header to is just using ztimer_usec with the very nice xtimer-api.

fjmolinas · 2021-10-29T12:27:39Z

I actually just use the header #include ztimer/xtimer_compat.h as an interface to ztimer_usec (read: do not include the xtimer_compat-module), this is totaly safe since it only applays to that compile step. That way i can be sure even though anything else is needing xtimer the thing that i added the header to is just using ztimer_usec with the very nice xtimer-api.

How would that work? xtimer_on_ztimer gets selected automatically if ztimer and xtimer are used. And event if I make this module not pull xtimer in, someone else is already doing it.

kfessel · 2021-10-29T14:05:27Z

in case of USEMODULE="ztimer ztimer_usec" you would not need to depend on any *timer, as ztimer_usec is allready there and ready to be used e.g.: via ztimer/xtimer_compat.h even without any extra (pseudo)modules.
The (pseudo)module xtimer_compat is just the thing that redirects xtimer.h to xtimer_compat.h and this is also the thing that the warning is for since xtimer_compat.h has not everything of xtimer.h, but just most of it, but xtimer_compat.h is there without the redirection.

fjmolinas · 2021-10-29T15:23:04Z

in case of USEMODULE="ztimer ztimer_usec" you would not need to depend on any *timer, as ztimer_usec is allready there and ready to be used e.g.: via ztimer/xtimer_compat.h even without any extra (pseudo)modules. The (pseudo)module xtimer_compat is just the thing that redirects xtimer.h to xtimer_compat.h and this is also the thing that the warning is for since xtimer_compat.h has not everything of xtimer.h, but just most of it, but xtimer_compat.h is there without the redirection.

I'm still not quite sure I understand, so please correct me if I'm wrong, you are suggesting something like this?

ifneq (,$(filter event_timeout_ztimer,$(USEMODULE)))
  USEMODULE += ztimer
endif

ifneq (,$(filter event_timeout_xtimer,$(USEMODULE)))
  ifeq (,$(filter ztimer_usec,$(USEMODULE)))
    USEMODULE += xtimer
  endif
endif

ifneq (,$(filter event_timeout,$(USEMODULE)))
  ifneq (,$(filter event_timeout_ztimer,$(USEMODULE)))
    USEMODULE += ztimer_usec
  else
    USEMODULE += event_timeout_xtimer
  endif
endif

and in sys/event/timeout_xtimer.c

#include "kernel_defines.h"
#if IS_USED(MODULE_ZTIMER_USEC)
#include "xtimer_compat.h"
#else
#include "xtimer.h"
#endif
#include "event/timeout.h"

kfessel · 2021-10-29T15:33:19Z

yes -- sys/event/timeout_xtimer.c:

...
#include "ztimer/xtimer_compat.h"
...

fjmolinas · 2021-10-29T16:36:17Z

yes -- sys/event/timeout_xtimer.c:
...
#include "ztimer/xtimer_compat.h"
...

I can try it out, but I fear a circular dependency...

fjmolinas · 2021-11-09T09:05:39Z

A lot of issues popping up

fjmolinas · 2021-11-09T09:07:16Z

Ok ci showed that this is still quite messy...

fjmolinas · 2021-11-16T08:39:22Z

I consider event_timeout_xtimer as purely internal for dependency resolution of how to implement event_timeout therefore its specific name is ignored. Every explicit use of event_timeout_xtimer is a wrong.

please squash and rocket

It seems like having event_timeout_xtimer made things weird since with different orders of inclusion both could be included, I could have used 8025c44 to exclude the c file as well, but having both modules show up as selected seemed weird. Instead, I removed event_timeout_xtimer and unselected the event_timeout c file when ztimer_usec and event_timeout_ztimet are selected. If Murdock passes then I'll split the build system change.

fjmolinas · 2021-11-16T09:08:25Z

Seems there is a circular dependency...

fjmolinas · 2021-11-16T09:19:29Z

@kfessel I don't think the changes to use the xtimer_compat header I don't think I can express something like:

    select MODULE_XTIMER if !MODULE_ZTIMER_USEC

without a dependency loop, which is the equivalent of:

    ifeq (,$(filter ztimer_usec,$(USEMODULE)))
      USEMODULE += xtimer
    endif

fjmolinas · 2021-11-16T09:41:19Z

Trying something else... lets see...

fjmolinas · 2021-11-16T13:36:17Z

Murdock is finally passing here, Ok to squash @kfessel? Does you ACK still stand?

kfessel · 2021-11-16T14:44:04Z

yes, please squash

fjmolinas · 2021-11-16T17:44:10Z

If Murdock passes then I'll split the build system change.

Splited.

This PR makes `event_timeout` and `event_timeout_ztimer` two distinct pseudomodules, where the only api difference is in the init function. If only `event_timeout_ztimer` is selected then no default ZTIMER backend is selected and the old init function is not implemented. If only `event_timeout` is selected then `xtimer` is used unless `ztimer_usec` is included. In which case the `xtimer` wrapper on top of `ztimer` is used and `xtimer` is not directly selected. This allows for the legacy api to be supported with `ztimer_usec` as a drop-in replacement. If `event_timeout` and `event_timeut_ztimer` are selected then `event_timeout` SRC file is excluded from compilation.

fjmolinas · 2021-11-17T09:16:04Z

Rebased

fjmolinas · 2021-11-17T13:01:45Z

@kfessel all green, since there was a rebase I let you hit the button!

fjmolinas · 2021-11-17T14:06:43Z

Thanks!

fjmolinas requested a review from kaspar030 as a code owner October 6, 2021 16:39

github-actions bot added Area: build system Area: Build system Area: examples Area: Example Applications Area: sys Area: System labels Oct 6, 2021

fjmolinas marked this pull request as draft October 6, 2021 16:40

fjmolinas changed the title ~~Wip/event timeout ztimer no usec~~ event/timeout: remove forced ZTIMER_USEC dependency [alternative] Oct 6, 2021

haukepetersen mentioned this pull request Oct 6, 2021

event/timeout: remove forced ZTIMER_USEC dependency #16892

Closed

github-actions bot added the Area: Kconfig Area: Kconfig integration label Oct 7, 2021

fjmolinas requested a review from leandrolanzieri October 7, 2021 09:53

fjmolinas marked this pull request as ready for review October 7, 2021 10:01

fjmolinas requested review from cgundogan and jia200x as code owners October 7, 2021 10:01

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

fjmolinas requested review from MichelRottleuthner, aabadie and smlng as code owners November 4, 2021 08:28

github-actions bot added the Area: tests Area: tests and testing framework label Nov 4, 2021

kfessel mentioned this pull request Nov 5, 2021

net/gcoap: port to ztimer #17141

Merged

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

fjmolinas force-pushed the wip/event_timeout_ztimer_no_usec branch from 728caf3 to a2e7cc6 Compare November 16, 2021 09:41

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

fjmolinas force-pushed the wip/event_timeout_ztimer_no_usec branch from a2e7cc6 to fcdac23 Compare November 16, 2021 17:36

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

fjmolinas force-pushed the wip/event_timeout_ztimer_no_usec branch from fcdac23 to 09b66e9 Compare November 16, 2021 17:43

fjmolinas mentioned this pull request Nov 16, 2021

Makefile.base: add SUBMODULES_NO_SRC to excluded a selected SUBMODULE *.c #17222

Merged

fjmolinas added 4 commits November 17, 2021 10:15

examples/nimble_heart_rate_sensor: use event_timeout_ztimer

39b896e

tests/events: add Kconfig

73babe5

tests/event_ztimer: add missing ztimer_usec dependency

c74e35c

fjmolinas force-pushed the wip/event_timeout_ztimer_no_usec branch from 09b66e9 to c74e35c Compare November 17, 2021 09:15

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

kfessel merged commit 5a84a25 into RIOT-OS:master Nov 17, 2021

fjmolinas deleted the wip/event_timeout_ztimer_no_usec branch November 17, 2021 14:06

fjmolinas added this to the Release 2022.01 milestone Nov 18, 2021

kfessel mentioned this pull request Feb 18, 2022

event/timeout: fixed dependency on ZTIMER_USEC preventing low-power operation #16891

Closed

Conversation

fjmolinas commented Oct 6, 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!

haukepetersen commented Oct 6, 2021

Uh oh!

fjmolinas commented Oct 7, 2021

Uh oh!

fjmolinas commented Oct 7, 2021

Uh oh!

kfessel commented Oct 8, 2021

Uh oh!

fjmolinas commented Oct 26, 2021

Uh oh!

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

Uh oh!

Uh oh!

fjmolinas commented Oct 26, 2021

Uh oh!

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

Uh oh!

Uh oh!

fjmolinas commented Oct 29, 2021

Uh oh!

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

Uh oh!

Uh oh!

fjmolinas commented Oct 29, 2021

Uh oh!

kfessel commented Oct 29, 2021

Uh oh!

fjmolinas commented Oct 29, 2021

Uh oh!

fjmolinas commented Nov 9, 2021

Uh oh!

fjmolinas commented Nov 9, 2021

Uh oh!

fjmolinas commented Nov 16, 2021

Uh oh!

fjmolinas commented Nov 16, 2021

Uh oh!

fjmolinas commented Nov 16, 2021

Uh oh!

fjmolinas commented Nov 16, 2021

Uh oh!

fjmolinas commented Nov 16, 2021

Uh oh!

kfessel commented Nov 16, 2021

Uh oh!

fjmolinas commented Nov 16, 2021

Uh oh!

fjmolinas commented Nov 17, 2021

Uh oh!

fjmolinas commented Nov 17, 2021

Uh oh!

fjmolinas commented Nov 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fjmolinas commented Oct 6, 2021 •

edited

Loading

kfessel commented Oct 26, 2021 •

edited

Loading

kfessel commented Oct 29, 2021 •

edited

Loading

kfessel commented Oct 29, 2021 •

edited

Loading