k_timer_remaining_get returns incorrect value on long timers

[ljaynes87]

Describe the bug
k_timer_remaining_get returns incorrect value when reading time remaining on long timers. I am not sure on the exact timer length but setting a timer for 48 hours will cause this issue

To Reproduce
Steps to reproduce the behavior:

1. Start a timer over 48 hours. (Could be shorter. 24 hours seems to work but 48 does not)
2. Read the timer with k_timer_remaining_get.
3. Timer value will return incorrect value for time remaining.

Expected behavior
Timer is expected to return the correct time remaining in milliseconds.

Environment:

• OS: Windows
• Toolchain nRF Connect Toolchain manager version 1.4.0

Additional context
Changing k_timer_remaining_get to use k_ticks_to_ms_floor64 instead of k_ticks_to_ms_floor32 seems to remedy this issue. It looks like k_ticks_to_ms_floor32 does not allow the tick count to be large enough.

[andyross]

k_timer_remaining_get() is a legacy function that returns a value in milliseconds as a 32 bit quantity. It does its internal math to 32 bit precision simply because it always has, and is thus subject to overflow bugs. It's true that we could do better than we do by computing a 64 bit intermediate result, but it's always going to be skewed relative to what you can achieve with the underlying timeout representation (which is 64 bit by default now, but can be configured to 32 bits).

Use k_timer_remaining_ticks() to return a k_ticks_t value in the full internal precision that you can convert however you like. I'm not opposed to trying to augment the internal precision of the legacy function, I'm just not sure that it's very useful (given that this bug has been present forever AFAICT).

[andyross]

Drop priority just because there's a trivial workaround

[andyross]

Thinking harder on this, I think I'm just going to close this. This particular function is really a legacy holdover from the era when we had millisecond timeouts everywhere. At that point, we only had 32 bit timeout precision internally and so a 64 bit intermediate results wasn't even feasible. Code written to that API clearly won't be expecting the full precision of the output space, and new code should be written to the ticks variant of the call which gives full internal precision by design.

[TjazVracko]

Hi,

I stumbled upon this issue in the current project I am coding since I had to use k_timer_remaining_get to implement a task scheduler. Some timers are configured to run with intervals of multiple weeks.

@andyross Your explanation above is very good, but the Zephyr API docs are not clear on this at all:
https://docs.zephyrproject.org/latest/kernel/services/timing/timers.html#c.k_timer_remaining_get

Based on the docstring and the function signature, If I configure a timer with an interval smaller than UINT32_MAX ms, I would expect k_timer_remaining_get to return the correct value, which it does not.

Could the documentation be updated to reflect the actual behaviour(if the duration in ticks is above UINT32_MAX, this function will overflow)? Or maybe mark it as "legacy", "deprecated" or something.

---

I would suggest also maybe adding k_timer_remaining_get_64?

The implementation is straightforward:

static inline int64_t k_timer_remaining_get_64(struct k_timer *timer)
{
	return k_ticks_to_ms_floor64(k_timer_remaining_ticks(timer));
}

While adding this to my code is simple, it requires me to copy-paste it across projects and to warn my colleagues of the pitfalls of k_timer_remaining_get whenever it is used.