sys/sema: allow to use ztimer and/or xtimer by haukepetersen · Pull Request #15782 · RIOT-OS/RIOT

haukepetersen · 2021-01-15T15:10:13Z

Contribution description

On the winding road to get NimBLE running without including xtimer or periph_timer in the build, the sema and event modules are the last ones that pull in these dependencies currently. So I am desperately looking for way to free these modules of xtimer deps.

The code in this PR, still very prototypic, illustrates one possible solution to decouple the sema module from xtimer and at the same time adding ztimer support. However, this might not be the most elegant solution, so I am happy to hear any alternative ideas on this task!

Regarding what IMO should be possible:

per default, build with an untouched API, so no existing code ever breaks
allow to enable the ztimer API in parallel to the existing API -> allows for mixing legacy code with new ztimer code
allow to disable the xtimer support and use the ztimer support only -> allows to throw out xtimer

Testing procedure

not in focus, yet.

Issues/PRs references

none

haukepetersen · 2021-01-15T15:11:37Z

Forgot to mention: remodeling the event module (event_wait_timeout()) presents the same class of problem, so a unified concept that can be applied to both would be needed here.

haukepetersen · 2021-01-18T17:13:23Z

Did another take on this, and I think with the API part I am quite happy now. The new take applies the exact same style than we use for the sys/event module (see also #15789). The sema_post_timed() function(s) are simply only available if the fitting timer backend was compiled in.

The only thing that needs more work is the implementation part. Here we need some smarter approach to prevent code duplication I guess...

sys/sema/sema.c

sys/include/sema.h

haukepetersen · 2021-03-31T08:43:54Z

Cleaned up the API part slightly. The more I think about this, I come to the conclusion that having two, although in large parts identical, implementations of the _sema_wait_x() functions seems to be the cleanest solution for now. At some point we will remove the xtimer part anyway, right? Then all we need to do is remove the one code path an map the sema_wait_timed to the ztimer call and we are all good.

fjmolinas · 2021-03-31T09:54:08Z

Cleaned up the API part slightly. The more I think about this, I come to the conclusion that having two, although in large parts identical, implementations of the _sema_wait_x() functions seems to be the cleanest solution for now. At some point we will remove the xtimer part anyway, right? Then all we need to do is remove the one code path an map the sema_wait_timed to the ztimer call and we are all good.

Sure lets got with it as is, seems like there is actually no taste for sema atm no?

fjmolinas

@haukepetersen there is a sema test in master now, lets keep it simple and simply use the ztimer version in one of the wait scenario:

diff --git a/tests/sema/Makefile b/tests/sema/Makefile
index ce88d28105..0d0af60982 100644
--- a/tests/sema/Makefile
+++ b/tests/sema/Makefile
@@ -1,5 +1,7 @@
 include ../Makefile.tests_common
 
 USEMODULE += sema
+USEMODULE += xtimer
+USEMODULE += ztimer_usec
 
 include $(RIOTBASE)/Makefile.include
diff --git a/tests/sema/main.c b/tests/sema/main.c
index 295c434103..5f0fdd2fa4 100644
--- a/tests/sema/main.c
+++ b/tests/sema/main.c
@@ -26,6 +26,7 @@
 #include "sema.h"
 #include "thread.h"
 #include "xtimer.h"
+#include  "ztimer.h"
 
 #define TEST_TIME_US 1000
 #define TEST_ITERATIONS 4
@@ -48,7 +49,7 @@ static void *second_thread(void *arg)
         printf("THREAD ERROR: sema_wait_timed()");
         *thread_success = -1;
     }
-    if (sema_wait_timed(&test_sema2, TEST_TIME_US) != 0) {
+    if (sema_wait_timed_ztimer(&test_sema2, ZTIMER_USEC, TEST_TIME_US) != 0) {
         printf("THREAD ERROR: sema_wait_timed()");
         *thread_success = -1;
     }

(please rebase to get the test in)

fjmolinas

Some final comments.

sys/sema/sema.c

sys/include/sema.h

wrong button when commenting

fjmolinas · 2021-04-07T10:26:44Z

@haukepetersen there is a sema test in master now, lets keep it simple and simply use the ztimer version in one of the wait scenario:

Actually, maybe its better to simply duplicate the test

fjmolinas · 2021-04-08T07:41:36Z

@haukepetersen there is a sema test in master now, lets keep it simple and simply use the ztimer version in one of the wait scenario:

Actually, maybe its better to simply duplicate the test

With #15782 (comment) in mind I think just the patch in #15782 (review) is enough. So if you remove this newline #15782 (comment) then I think this one is OK!

haukepetersen · 2021-04-12T09:03:37Z

done:

added call to sema_wait_timed_ztimer() to the test application
removed the newline
fixed a dependency problem when building without any active timers

fjmolinas · 2021-04-12T10:08:18Z

Murdock is green please squash @haukepetersen!

fjmolinas

ACK!

haukepetersen · 2021-04-12T11:09:06Z

squashed

haukepetersen · 2021-04-12T12:44:27Z

All green -> go

haukepetersen force-pushed the opt_sema_ztimer branch from a5a758d to 8c04066 Compare January 18, 2021 17:10

fjmolinas reviewed Feb 28, 2021

View reviewed changes

sys/sema/sema.c Outdated Show resolved Hide resolved

fjmolinas reviewed Mar 1, 2021

View reviewed changes

sys/sema/sema.c Outdated Show resolved Hide resolved

fjmolinas reviewed Mar 1, 2021

View reviewed changes

sys/include/sema.h Outdated Show resolved Hide resolved

fjmolinas reviewed Mar 1, 2021

View reviewed changes

sys/include/sema.h Show resolved Hide resolved

kfessel mentioned this pull request Mar 5, 2021

[Tracking] modules depending on 64-bit xtimer implementation #13667

Open

22 tasks

fjmolinas reviewed Apr 7, 2021

View reviewed changes

fjmolinas previously approved these changes Apr 7, 2021

View reviewed changes

sys/sema/sema.c Show resolved Hide resolved

sys/include/sema.h Show resolved Hide resolved

sys/include/sema.h Show resolved Hide resolved

haukepetersen force-pushed the opt_sema_ztimer branch from 0b81ecf to c650335 Compare April 12, 2021 09:02

haukepetersen requested review from MichelRottleuthner, aabadie, leandrolanzieri and smlng as code owners April 12, 2021 09:02

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

fjmolinas approved these changes Apr 12, 2021

View reviewed changes

haukepetersen added 2 commits April 12, 2021 13:08

sys/sema: allow to use ztimer and/or xtimer

b339e91

tests/sema: add test for sema_wait_timed_ztimer()

4cd9e96

haukepetersen force-pushed the opt_sema_ztimer branch from c650335 to 4cd9e96 Compare April 12, 2021 11:09

haukepetersen merged commit 9926e33 into RIOT-OS:master Apr 12, 2021

haukepetersen deleted the opt_sema_ztimer branch April 12, 2021 12:44

haukepetersen mentioned this pull request Apr 12, 2021

pkg/nimble: migrate to ztimer #16317

Merged

kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021

Conversation

haukepetersen commented Jan 15, 2021

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

haukepetersen commented Jan 15, 2021

Uh oh!

haukepetersen commented Jan 18, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

haukepetersen commented Mar 31, 2021

Uh oh!

fjmolinas commented Mar 31, 2021

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fjmolinas commented Apr 7, 2021

Uh oh!

fjmolinas commented Apr 8, 2021

Uh oh!

haukepetersen commented Apr 12, 2021

Uh oh!

fjmolinas commented Apr 12, 2021

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Apr 12, 2021

Uh oh!

haukepetersen commented Apr 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants