-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sys/ztimer: initial import #11874
sys/ztimer: initial import #11874
Conversation
10af470
to
4b12eb5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is disabling type checking by force casting at many instances. Most of the casts are not needed and can be replaced by code that does not disable type checking. Converting from a generic to a specific (e.g. ztimer_dev_t *
to ztimer_periph_t
) could be moved to a static inline function. That will make refactoring easier, e.g. if the structure layout of ztimer_periph_t
changed that ztimer_dev_t
no longer is the first member.
Is there any case where a ztimer_base_t *
does not point to a ztimer_t
? If not, what is the reason to have ztimer_base_t
?
Would the already existing |
uint32_t period); | ||
|
||
/** | ||
* @brief Put the calling thread to sleep for the specified number of ticks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably also valid for the other functions: This probably doesn't sleep exactly the number of ticks (or seconds) on every platform. Does this sleep at least ticks
or at most ticks
. Furthermore, is there a way to get the other behaviour, for example, sleep at least $duration to allow for a peripheral to initialize or sleep at most $duration to not miss a deadline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I bet that this is impossible, as e.g. a thread with higher priority might get the CPU at the very moment the caller wants to wake up. As far as I know, RIOT never had the goal to provide hard real time features, but soft real time features instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this sleep at least
ticks
or at mostticks
.
Well, the implementation sleeps "at least x ticks". I guess this information has to be added everywhere.
As far as I know, RIOT never had the goal to provide hard real time features, but soft real time features instead.
I think that is what the website states. Actually, it is more complicated. Parts of RIOT are fully hard real-time capable, some parts are not, some parts are only if certain constraints are honored. With care, it is perfectly possible to do hard real time with RIOT. but, you really have to know the internals, as documentation on the matter is severley lacking.
Still, I like that doing hard real-time with RIOT is possible.
The good thing with the static inline funcitons is, if e.g. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. The general architecture and the user facing API are fine. It adds no more complexity than required to allow different timer bases, and the advantages of this are obvious and outweigh the cost of one additional parameter by means.
Having the proposed plan in mind to bring this to master, so that it can be tested and gradually improved to the point it can replace xtimer
, I'd say this PR is generally in the shape to get merged. (One exception: The TODO in the user facing API, see comment below. The function should also be documented properly, as it is part of the user facing API.)
Please let's not have this discussion here. As implemented here, this is idiomatic C. I'd rather keep the style as is, and concentrate on more pressing issues. (I will fix all the unnececessary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor issues I've found so far
I think only four things are left so that this can be merged:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just found some documentation that's not to clear to me. I did not read the corresponding code yet.
I've put quite some work into this. Please see the updated PR description. There's also a more detailed document on why we actually started this and why I think it should replace xtimer: https://github.com/RIOT-OS/RIOT/wiki/ztimer-problem-statement-and-design-document |
Maybe also advertise this on the devel mailinglist. |
ok |
Co-authored-by: Joakim Nohlgård <joakim.nohlgard@eistec.se>
This commit adds logic to make xtimer use ztimer_usec as backend (instead of periph_timer). This allows ztimer_usec and xtimer to coexist. It also allows xtimer to profit from eventually implemented power mode blocking in ztimer's periph_timer backend.
The SAMR30 looks fine!
Results
|
Thanks for running these! Did you somehow export |
Good to know. I set an environment variable accordingly. (BTW: Good test if the compat layer is actually used :D)
Results
|
Re ran with compile_and_test_for_board.py . BOARD --with-test-only --incremental -j0 --applications="tests/xtimer*pba-d-01-kw2x/failuresummaryFailures during compilation: nucleo-f207zg/failuresummaryFailures during compilation: hifive1b/failuresummaryFailures during compilation: nrf52dk/failuresummaryFailures during compilation: nrf52840dk/failuresummaryFailures during compilation: iotlab-m3/failuresummaryFailures during compilation: frdm-k64f/failuresummaryFailures during compilation: nucleo-l152re/failuresummaryFailures during compilation: b-l475e-iot01a/failuresummaryFailures during compilation: nucleo-f410rb/failuresummaryFailures during compilation: arduino-uno/failuresummaryFailures during compilation: nucleo-f767zi/failuresummaryFailures during compilation: arduino-mega2560/failuresummaryFailures during compilation: i-nucleo-lrwan1/failuresummaryFailures during compilation: openmote-b/failuresummaryFailures during compilation: z1/failuresummaryFailures during compilation: nucleo-f091rc/failuresummaryFailures during compilation: arduino-zero/failuresummaryFailures during compilation: nucleo-f103rb/failuresummaryFailures during compilation: b-l072z-lrwan1/failuresummaryFailures during compilation: nucleo-f303re/failuresummaryFailures during compilation: slstk3402a/failuresummaryFailures during compilation: nucleo-l073rz/failuresummaryFailures during compilation: Only compilation failures for:
|
So the other xtimer tests pass. They don't really check for quality of the results. This basically means that the difficult corner cases that are checked don't hang or crash with ztimer (which is nice). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ztimer is fully optional and not intrusive. Default RIOT behavior is not affected. Tests showed no issues and the code is in good shape. Documentation is in good enough shape for this to be merged.
ACK!
Contribution description
This PR contains a new timer subsystem meant to eventually replace xtimer. It has been in development for quite some time by @gebart and me.
Please refer to this document for a more detailed problem statement and implementation details.
The PR provides some unittests using a mock clock. This has been used successfully to find quite some bugs.
The PR does not enable or use ztimer for anything but tests. The idea is to get feedback on concept and API, fill the holes, see how it works. Eventually, if deemed better than xtimer, we can convert the codebase. There's an initial coccinelle script that can automatically do that (only fails on 64bit xtimer API calls for lack of ztimer alternative).
ztimer tries to re-use the same defaults and configuration as xtimer. With "USEMODULE += xtimer_on_ztimer", a large part of the code can be tested with ztimer as backend and xtimer on top. *update: this is selected automatically if xtimer and ztimer are in USEMODULE.
Update 2020-02-20
Update 2019-12-09:
Update 2019-12-10:
Testing procedure
review API and concept
try
"USEMODULE += xtimer_on_ztimer""USEMODULE=ztimer make ..." on your board, with some of the xtimer testsIssues/PRs references