[riot-notifications] [RIOT-OS/RIOT] sys/evtimer: introduce ZTIMER_MSEC as timer backend (#13661)

Marian Buschsieweke notifications at github.com
Thu May 14 14:33:46 CEST 2020


@maribu commented on this pull request.

Some comments inline. I was about to implement this myself - luckily I checked for open PRs first :-)

> @@ -1,6 +1,7 @@
 include ../Makefile.tests_common
 
 USEMODULE += evtimer
+USEMODULE += xtimer

This should not be needed. `xtimer` is (for now) still the default high level timer API an should be pulled in as dependency, unless the user explicitly wishes to do differently.

> @@ -38,6 +38,12 @@ ifneq (,$(filter xtimer_on_ztimer,$(USEMODULE)))
   PSEUDOMODULES += xtimer_on_ztimer
 endif
 
+# make evtimer use ztimer_msec as low level timer
+ifneq (,$(filter evtimer_on_ztimer,$(USEMODULE)))
+  USEMODULE += ztimer_msec
+  PSEUDOMODULES += evtimer_on_ztimer

This should be moved to `makefiles/pseudomodules.mk`

> +# make evtimer use ztimer_msec as low level timer
+ifneq (,$(filter evtimer_on_ztimer,$(USEMODULE)))
+  USEMODULE += ztimer_msec
+  PSEUDOMODULES += evtimer_on_ztimer
+endif
+

This should be moved next to where the other `evtimer` dependencies are handled.

> +  ifeq (,$(filter evtimer_on_ztimer,$(USEMODULE)))
+    USEMODULE += xtimer
+  endif

Please move the other dependency handling (see below) here, so that all `evtimer` related dependency management comes directly below `ifneq (,$(filter evtimer,$(USEMODULE)))` 

> @@ -27,7 +27,7 @@ endif
 ifneq (,$(filter ztimer,$(USEMODULE)))
   ifneq  (,$(filter xtimer,$(USEMODULE)))
     ifeq (,$(filter ztimer_xtimer_compat,$(USEMODULE)))
-        USEMODULE += xtimer_on_ztimer
+      USEMODULE += xtimer_on_ztimer

Can you split this out into a separate commit, as this fix of indent is unrelated? Keeping unrelated stuff into separate commits makes life much easier when after merging some bug pops up and specific commits need to be reverted until a long term solution is found.

(Actually, unrelated stuff should be moved into its own PR. But I personally have strong sympathy for not wanting to create a PR for trivial single line fixes.)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/13661#pullrequestreview-411738208
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20200514/68a9f7ec/attachment.htm>


More information about the notifications mailing list