Merged
Conversation
kaspar030
added
Discussion: RFC
github-actions
bot
added
Area: build system
22 tasks
Contributor
Author
|
I've updated the API: Here's a table for absolute variants:
please take a look and let me know if this makes sense. I've also implemented The "tick" for updating base timer offset is still missing. |
Contributor
|
i would prefer all absolute be |
maribu
reviewed
Nov 15, 2021
| ztimer64_offset2absolute(clock, timeout)); | ||
| } | ||
|
|
||
| #define MSG_ZTIMER64 0xc83f /**< msg type used by ztimer64_msg_receive_timeout */ |
Member
There was a problem hiding this comment.
Chosen by fair dice roll?
Image embedded from https://xkcd.com/221/ - all credits to xkcd.
Contributor
Author
There was a problem hiding this comment.
hah! someone noticed. MSG_ZTIMER was created with the define generator that's PR'ed somewhere.
I just used that value, changed a bit, and removed the comment.
Contributor
Author
|
I've renamed |
Contributor
Author
|
Seems like ztimer64 comes in at roughly 500b extra: |
github-actions
bot
added
Area: doc
Contributor
Author
|
The "tick" is now implemented and seems to work. I think this is ready for review! Ideally the API from ztimer64/util.c gets hammered a bit. |
Contributor
|
I do not like that this introduces a concept of absolute time next to ztimer (which forces RIOT to keep the timers running all the time - one use of ZTIMER64_USEC will disable power-management even at times it isn't used). |
Contributor
Author
What about >49days worth of milliseconds on periph_rtt? |
Contributor
|
Having timeouts/alarm/callbacks/msg... >49days in the future does not depend on absolute time it depends on relative time being able to track more than 49 days and if no one wants to know about that, you do not need to track time (that time domain) saving energy. |
Contributor
Author
|
But there are application that would like to keep track of time, possibly synchronized over the network, for more than 49 days. Keeping 64bit absolute time requires some wakeups. If the RTT can do 32bit, there's a wakeup every ~25 days. Is that your issue? |
Member
|
His is issue is that when oneoccasionally uses I don't see this an issue. Other than running We could add a check ala build system sanity check to enfore |
kaspar030
requested review from
aabadie,
bergzand,
fjmolinas and
leandrolanzieri
as code owners
fjmolinas
reviewed
Dec 7, 2021
fjmolinas
reviewed
Dec 7, 2021
| static void _ztimer64_update(ztimer64_clock_t *clock); | ||
|
|
||
| /* debug aid */ | ||
| void ztimer64_clock_print(const ztimer64_clock_t *clock); |
Contributor
There was a problem hiding this comment.
Is this something you want to be able to insert around?
Contributor
Author
There was a problem hiding this comment.
Yup, let's keep it around. :)
fjmolinas
reviewed
Dec 7, 2021
Contributor
|
Seems like CI complaints are basically Kconfig related, I'll take a look |
fjmolinas
reviewed
Dec 7, 2021
fjmolinas
reviewed
Dec 7, 2021
fjmolinas
reviewed
Dec 7, 2021
fjmolinas
reviewed
Dec 7, 2021
fjmolinas
reviewed
Dec 7, 2021
Contributor
|
Murdock is happy, but there are a couple of nitpicks and static checks comments remaining... |
Co-authored-by: Francisco Molina <[email protected]> Co-authored-by: Marian Buschsieweke <[email protected]>
Co-authored-by: Francisco Molina <[email protected]>
Contributor
Author
|
Contributor
|
I ran unit tests on a series of BOARDs, all good: Details |
Contributor
|
The ztimer64_msg test failed on two BOARD's I'll take a closer look: Details |
Contributor
|
Can you add the following patch @kaspar030: diff --git a/cpu/atmega_common/include/cpu_conf.h b/cpu/atmega_common/include/cpu_conf.h
index d20bea4eb2..cd7dfa8164 100644
--- a/cpu/atmega_common/include/cpu_conf.h
+++ b/cpu/atmega_common/include/cpu_conf.h
@@ -25,6 +25,7 @@
#ifndef CPU_CONF_H
#define CPU_CONF_H
+#include "kernel_defines.h"
#include "atmega_regs_common.h"
#ifdef __cplusplus
@@ -49,8 +50,8 @@ extern "C" {
* to avoid not printing of debug in interrupts
*/
#ifndef THREAD_STACKSIZE_IDLE
-#ifdef MODULE_XTIMER
-/* xtimer's 64 bit arithmetic doesn't perform well on 8 bit archs. In order to
+#if IS_USED(MODULE_XTIMER) || IS_USED(MODULE_ZTIMER64)
+/* 64 bit arithmetic doesn't perform well on 8 bit archs. In order to
* prevent a stack overflow when an timer triggers while the idle thread is
* running, we have to increase the stack size then
*/ |
github-actions
bot
added
Area: cpu
bergzand
approved these changes
Dec 9, 2021
Member
bergzand
left a comment
bergzand
left a comment
There was a problem hiding this comment.
Also an ACK from my side! and go!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contribution description
This PR contains my work on a 64bit ztimer. As discussed before, this is a new module that sits on top of ztimer(32), and uses that as backend.
Pros:
Cons:
Envisioned usage:
Each ztimer64 instance needs a ztimer(32) backend clock. Initialization is simple:
ztimer64_init(ZTIMER64_MSEC, ZTIMER_MSEC);I'd suggest mirroring the ztimer32 ones, e.g.,
there'd be ZTIMER64_MSEC, ZTIMER64_USEC, ..., with corresponding ztimer64_Xsec pseudomodules that modules/apps can depend on.
A module needing 64bit times could then:
USEMODULE += ztimer64_msecDesign choices:
Status:
probably we should rename the current to be a new API using absolute targets, then re-create relative on top of that?
Testing procedure
Issues/PRs references