[PATCH 1/2] cyclic: Fix rollover every 72 min on 32 bits platforms
On 32 bits platforms, timer_get_us() returns an unsigned long which is a 32 bits. timer_get_us() wraps around every 72 minutes (2 ^ 32 / 1000000 =~ 4295 sec =~ 72 min).
So the test "if time_after_eq64(now, cyclic->next_call)" is no more true when cyclic->next_call becomes above 32 bits max value (4294967295).
At this point after 72 min, no more cyclic function are executed included watchdog one.
Instead of using timer_get_us(), use get_timer_us() which returns a uint64_t, this allows a rollover every 584942 years.
Signed-off-by: Patrice Chotard patrice.chotard@foss.st.com ---
common/cyclic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/cyclic.c b/common/cyclic.c index 196797fd61e..e3f03a19d55 100644 --- a/common/cyclic.c +++ b/common/cyclic.c @@ -61,7 +61,7 @@ static void cyclic_run(void) * Check if this cyclic function needs to get called, e.g. * do not call the cyclic func too often */ - now = timer_get_us(); + now = get_timer_us(0); if (time_after_eq64(now, cyclic->next_call)) { /* Call cyclic function and account it's cpu-time */ cyclic->next_call = now + cyclic->delay_us;
Replace delay_ns by delay_us which is the field name used into struct cyclic_info.
Signed-off-by: Patrice Chotard patrice.chotard@foss.st.com ---
include/cyclic.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/cyclic.h b/include/cyclic.h index c6c463d68e9..56190db0599 100644 --- a/include/cyclic.h +++ b/include/cyclic.h @@ -20,7 +20,7 @@ * * @func: Function to call periodically * @name: Name of the cyclic function, e.g. shown in the commands - * @delay_ns: Delay is ns after which this function shall get executed + * @delay_us: Delay is ns after which this function shall get executed * @start_time_us: Start time in us, when this function started its execution * @cpu_time_us: Total CPU time of this function * @run_cnt: Counter of executions occurances
On 08.01.25 16:09, Patrice Chotard wrote:
Replace delay_ns by delay_us which is the field name used into struct cyclic_info.
Signed-off-by: Patrice Chotard patrice.chotard@foss.st.com
include/cyclic.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/cyclic.h b/include/cyclic.h index c6c463d68e9..56190db0599 100644 --- a/include/cyclic.h +++ b/include/cyclic.h @@ -20,7 +20,7 @@
- @func: Function to call periodically
- @name: Name of the cyclic function, e.g. shown in the commands
- @delay_ns: Delay is ns after which this function shall get executed
- @delay_us: Delay is ns after which this function shall get executed
The comment still references "ns" above. No need to re-send, I can change this when committing.
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
Hello Patrice,
Am Wed, Jan 08, 2025 at 04:09:39PM +0100 schrieb Patrice Chotard:
On 32 bits platforms, timer_get_us() returns an unsigned long which is a 32 bits. timer_get_us() wraps around every 72 minutes (2 ^ 32 / 1000000 =~ 4295 sec =~ 72 min).
So the test "if time_after_eq64(now, cyclic->next_call)" is no more true when cyclic->next_call becomes above 32 bits max value (4294967295).
At this point after 72 min, no more cyclic function are executed included watchdog one.
Instead of using timer_get_us(), use get_timer_us() which returns a uint64_t, this allows a rollover every 584942 years.
This should be long enough. ;-)
Signed-off-by: Patrice Chotard patrice.chotard@foss.st.com
common/cyclic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/cyclic.c b/common/cyclic.c index 196797fd61e..e3f03a19d55 100644 --- a/common/cyclic.c +++ b/common/cyclic.c @@ -61,7 +61,7 @@ static void cyclic_run(void) * Check if this cyclic function needs to get called, e.g. * do not call the cyclic func too often */
now = timer_get_us();
now = get_timer_us(0);
Acked-by: Alexander Dahl ada@thorsis.com
Greets Alex
if (time_after_eq64(now, cyclic->next_call)) { /* Call cyclic function and account it's cpu-time */ cyclic->next_call = now + cyclic->delay_us;-- 2.25.1
On 08.01.25 16:09, Patrice Chotard wrote:
On 32 bits platforms, timer_get_us() returns an unsigned long which is a 32 bits. timer_get_us() wraps around every 72 minutes (2 ^ 32 / 1000000 =~ 4295 sec =~ 72 min).
So the test "if time_after_eq64(now, cyclic->next_call)" is no more true when cyclic->next_call becomes above 32 bits max value (4294967295).
At this point after 72 min, no more cyclic function are executed included watchdog one.
Instead of using timer_get_us(), use get_timer_us() which returns a uint64_t, this allows a rollover every 584942 years.
Signed-off-by: Patrice Chotard patrice.chotard@foss.st.com
common/cyclic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/cyclic.c b/common/cyclic.c index 196797fd61e..e3f03a19d55 100644 --- a/common/cyclic.c +++ b/common/cyclic.c @@ -61,7 +61,7 @@ static void cyclic_run(void) * Check if this cyclic function needs to get called, e.g. * do not call the cyclic func too often */
now = timer_get_us();
now = get_timer_us(0);
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
On 1/9/25 08:56, Stefan Roese wrote:
On 08.01.25 16:09, Patrice Chotard wrote:
On 32 bits platforms, timer_get_us() returns an unsigned long which is a 32 bits. timer_get_us() wraps around every 72 minutes (2 ^ 32 / 1000000 =~ 4295 sec =~ 72 min).
So the test "if time_after_eq64(now, cyclic->next_call)" is no more true when cyclic->next_call becomes above 32 bits max value (4294967295).
At this point after 72 min, no more cyclic function are executed included watchdog one.
Instead of using timer_get_us(), use get_timer_us() which returns a uint64_t, this allows a rollover every 584942 years.
Signed-off-by: Patrice Chotard patrice.chotard@foss.st.com
common/cyclic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/cyclic.c b/common/cyclic.c index 196797fd61e..e3f03a19d55 100644 --- a/common/cyclic.c +++ b/common/cyclic.c @@ -61,7 +61,7 @@ static void cyclic_run(void) * Check if this cyclic function needs to get called, e.g. * do not call the cyclic func too often */ - now = timer_get_us(); + now = get_timer_us(0); if (time_after_eq64(now, cyclic->next_call)) { /* Call cyclic function and account it's cpu-time */ cyclic->next_call = now + cyclic->delay_us;
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
Hi Stefan
I found another issue regarding cyclic. I didn't notice it due to debug trace i activated to find this issue, but after 72 min , i got :
ERROR: cyclic function watchdog took too long: -4294967293us vs 1000us max
This is due to remaining usage of timer_get_us() in cyclic_register() and in cyclic_run(). they also need to be replaced by get_timer_us(0).
I will send a v2 for that.
Patrice
participants (4)
-
Alexander Dahl -
Patrice CHOTARD -
Patrice Chotard
-
Stefan Roese