sys: some simple xtimer->ztimer conversions

[kfessel]

Contribution description

do some simple xtimer->ztimer conversions using 'ztimer/xtimer_compat.h'

These modules only use 32Bit of xtimer and and therefor are able to use ztimer (32) through xtimer_compat.h

Testing procedure

Issues/PRs references

[fjmolinas]

As discussed in the sprint day meeting, to do this the concerned modules should IMO explicitly include ztimer_xtimer_compat, otherwise its too easy to end up with cases with things like:

In file included from uhcpc.c:16:
../../../include/ztimer/xtimer_compat.h:58:18: error: conflicting types for ‘xtimer_t’
   58 | typedef ztimer_t xtimer_t;
      |                  ^~~~~~~~
In file included from ../../../include/net/gnrc/netif/pktq/type.h:27,
                 from ../../../include/net/gnrc/netif.h:63,
                 from ../../../include/net/gnrc.h:294,
                 from ../../gnrc/sock/include/sock_types.h:30,
                 from ../../../include/net/sock/udp.h:685,
                 from uhcpc.c:13:

Otherwise, it's just not a maintainable approach system-wise, it will work here and there but not be a pattern to reproduce.

[kfessel]

in essence this come down to gnrc exposing the timer implementation used internal throughout the stack right into the user from

../../../include/net/gnrc/netif/pktq/type.h:27,
../../../include/net/gnrc/netif.h:63,
../../../include/net/gnrc.h:294,

its is include creep similar to #17574

udcpc should not need to care what timer the pktq uses (it is usesd at netif level and only there) udcpc is miles away from that

right now i am trying to find the point where we can introduce a cut in the include chain in gnrc

[fjmolinas]

in essence this come down to gnrc exposing the timer implementation used internal throughout the stack right into the user from

I'm still not convinced of this approach, not making dependency explicit seems like something that is just prone to break, event_timeout showed this.

The header cleanup I'm all in favor, and is not tied to the other changes right?

0891530 can you explain these changes?

[kfessel]

I'm still not convinced of this approach, not making dependency explicit seems like something that is just prone to break, event_timeout showed this.

Was there an issue with event_timeout.c using ztimer through xtimer_compat.h? - I only remember it working and then we recently removed its xtimer_support entirely.

event_timeout did never expose that it used ztimer through xtimer_compat.h if there was ztimer it used ztimer (even if somthing else uses xtimer on ztimer) the internals

The header cleanup I'm all in favor, and is not tied to the other changes right?

Yes, Separate PR for that first. #17714

0891530 can you explain these changes?

I tried to make these all Test (just with one Board - to find most of the missing includes before uploading to Murdock).
These would not build with native -> need a Board set -> i set a default Board that builds the test. I am sure that this is not connected to the rest of the PR -> Separate PR #17715

[fjmolinas]

I'm still not convinced of this approach, not making dependency explicit seems like something that is just prone to break, event_timeout showed this.

Was there an issue with event_timeout.c using ztimer through xtimer_compat.h? - I only remember it working and then we recently removed its xtimer_support entirely.

It worked because no xtimer_on_ztimer or ztimer_xtimer_compat api is enabled by default, but with #17365 when you start doing you end up with conflicts, multiple definitions of the xtimer api requesting different types.

event_timeout did never exposes that it used ztimer through xtimer_compat.h if there was ztimer it used ztimer (even if somthing else uses xtimer on ztimer) the internals

Yes but not exposing it was the issue, because then if there was a user of ztimer64_xtimer_compat or xtimer_on_ztimer then there was an API conflict that should have been caught from the build-system and not on a compilation error.

[fjmolinas]

See #17693 (comment) #17693 (comment)

[kfessel]

See #17693 (comment) #17693 (comment)

#17726 build successfully

[fjmolinas]

See #17693 (comment) #17693 (comment)

#17726 build successfully

I'm still not in favor of this:

• ifeq dependencies should be avoided
• it hides what module is being used, If I did info-modules I would have no clue, I could see xtimer and ztimer_usec being used
• if xtimer_ztimer_compat is going to be the default module (if sys/xtimer: make xtimer_ztimer_compat default backend #17721 gets in) then there is no need for this one
• if for some reason the user wants xtimer this does not allow it and hides it, and if we don't want to give the user the choice then let's just migrate to ztimer directly

[kfessel]

See #17693 (comment) #17693 (comment)

#17726 build successfully

I'm still not in favor of this:

* `ifeq` dependencies should be avoided

you are right there should only be one ifeq for all these prefer ztimer_usec but fallback to xtimer modules

we can remove the fallback option for these modules if we always have a ztimer

* it hides what module is being used, If I did `info-modules` I would have no clue, I could see `xtimer` and `ztimer_usec` being used

These modules use ztimer and only fallback to xtimer if no one else uses it (kindoff due to the fact the decision is done when the module use is evaluated it might got it worng -> going to change that since no one likes a xtimer that has no job)
These modules will not use xtimer with this PR if ztimer_usec is availible, so if info-modules reports xtimer is being used something else uses it

* if `xtimer_ztimer_compat` is going to be the default module (if [sys/xtimer: make xtimer_ztimer_compat default backend #17721](https://github.com/RIOT-OS/RIOT/pull/17721) gets in) then there is no need for this one

these module are able to use ztimer through its xtimer_compat interface -> they do not need the roundtrip through xtimer.h that is probaly ending at ztimer64_usec, they are perfectly fine working with ztimer_usec even if something else requieres ztimer64 or ztimer64_xtimer_compat
e.g.: newlib may need ztimer64_xtimer_compat, these modules can employ ztimer_usec directly through xtimer_compat sparing them some extra bytes, some extra list handling of ztimer64_usec.

* if for some reason the user wants `xtimer` this does not allow it and hides it, and if we don't want to give the user the choice then let's just migrate to `ztimer` directly

Aren't we doing this right now? seems like we are on a good track to no xtimer in 2-3 months

[fjmolinas]

This one needs a rebase!

[fjmolinas]

This kind of pattern is not used and is not a good idea to use in the build system, it is very very prone to dependency order issues.

Are we sure this is still worth it with xtimer_ztimer_compat being used by default now?

[kfessel]

This is a step to remove the xtimer support from these modules they should be easy conversion to ztimer. We may just remove the xtimer support from these modules entirely and skip this step.

I do not like writing these:

if there is ztimer
   do this 
elseif there is xtimer
   do that

Even reading this kind of code not nice since it distracts from what is the main purpose of that module.

[fjmolinas]

Then instead how about adapting the coccinelle scripts to have two versions a ztimer_xtimer_compat version and a ztimer64_xtimer_compat version, and simply spatch those files and directly make the conversion?

[kfessel]

#17892 has that done (without the coccinelle part)

[kfessel]

This kind of pattern is not used and is not a good idea to use in the build system, it is very very prone to dependency order issues.

somehow i think the xztimer_fallback solves a dependency order issue for modules that support ztimer as well as xtimer through delaying the decision until it is necessary

If the first module in Makefile.dep supports x and z timer the dependancy will be decided at that point and every later module will see the decision done there but in reality the module did not care. - may be this needs another PR

[kfessel]

Alt. Pr.: #17892 made it