sys/evtimer: introduce ZTIMER_MSEC as timer backend#13661
sys/evtimer: introduce ZTIMER_MSEC as timer backend#13661jue89 merged 5 commits intoRIOT-OS:masterfrom
Conversation
b8d8159 to
6a45ced
Compare
|
Since #13666 has been merged, I get back to this PR. I rebased this PR on master and included a ztimer implementation for Furthermore, I've tested |
maribu
left a comment
There was a problem hiding this comment.
Some comments inline. I was about to implement this myself - luckily I checked for open PRs first :-)
|
Btw: It is now called |
they both exist. "xtimer_on_ztimer" includes full xtimer, but makes xtimer use ztimer as backend. "ztimer_xtimer_compat" is a wrapper (xtimer calls get wrapped to ztimer calls). The latter doesn't support the complete xtimer api. |
|
I think everything that should be in this PR has been implemented, @maribu. This PR:
Follow-up PR 1:
Follow-up PR 2:
Does this sound like a plan? If so I would rebase and clean-up this PR if you don't mind. It became kinda messy ;) |
We could also go the other way around. There's no harm in Makefile.dep adding to pseudomodules. We could move them from pseudomodules.mk to the Makefile.deps, so they're close to the other module dependencies. |
31ec32f to
e61e5c8
Compare
|
Rebased and ready for review ;) |
I think @maribu's point is the recursive dependency resolution by including all This does work but takes some time. But in case of the conditional Or have I got something wrong? |
Could it work if you declare a pseudomodule (did not test this, just brainstorming) |
Oh yes, a module this is only conditionally a pseudo-module is indeed a special case. The approach @fjmolinas suggested would certainly work. Maybe it could be simplified to this: ifneq (,$(filter xtimer_on_xtimer,$(USEMODULE)))
NO_AUTO_SRC = 1
endifAlternatively, this special case IMO justifies an exception. A small comment above the I don't think that de-duplication of |
|
Really good idea! But I would put this in a follow-up PR. |
Can we quantify this? The recursion stops when there are no changes (additions) to the checked variables. Adding to PSEUDOMODULES doesn't even cause deeper recursion. And if having a module in there multiple times probably won't have any measurable effect on performance. Neither would a You can certainly "fix" this, but IMO there's no issue. |
|
@kaspar030 only squashing is missing here, are you ok with the change? |
|
Note: This won't change generated machine code, unless someone explicitly choose to use |
Fine with me 👍 |
|
The testbed is still alive :) Does anyone mind if I squash the fixups? (@kaspar030?) |
|
yes, please squash! |
1daef0e to
c3a20f8
Compare
|
Looks like this uncovered a missing |
Yip. I pushed a commit fixing that. I think this one-liner doesn't need its own PR? |
|
Should we update the documentation in this PR or postpone to a followup? It is currently pretty specific with explaining that xtimer is used as backend... |
|
Hmm, I'll push some lines the evtimer's documentation pointing out that it may be based on Postponing documentation to a later point in time is likely to never happen :D |
|
I added a quick note to refer to |
IMO, this is good as it as. Let's rerun Murdock. (One build failed due to connectivity issues of the worker.) |
|
@kaspar030: All green ;-) |
|
Am I allowed to press that green shiny Merge button? :) |
|
Thank you all for reviewing! |
| # make evtimer use ztimer_msec as low level timer | ||
| ifneq (,$(filter evtimer_on_ztimer,$(USEMODULE))) | ||
| USEMODULE += ztimer_msec | ||
| USEMODULE += ztimer_now64 |
There was a problem hiding this comment.
speaking about that ztimer_now64
Contribution description
Inspired by the implementation of
xtimer_on_ztimer, I tried the same thing forevtimer.According to evtimer's description, it has been built to allow for long running timers. Thus, I made an adaption to the ZTIMER_MSEC clock which should perfectly meet this requirement.
This PR is still WIP ... not all tests are green.
Testing procedure
Run
tests/evtimer_msgwith the following command line:USEMODULE="ztimer_usec evtimer_on_ztimer" make all test(
tests/evtimer_underflowdoesn't work onnative.)Issues/PRs references
Requires #13666