k_uptime_get_32() does not behave as documented #18739
Comments
|
I'm going to work up a patch that marks it deprecated, removes all uses from master, and documents the misbehavior. |
This function does not return the low 32 bits of k_uptime_get() as suggested by it's documentation; it returns the number of milliseconds represented by the low 32-bits of the underlying system clock. The truncation before translation results in discontinuities at every point where the system clock increments bit 33. Refine the documentation to describe what it actually does, mark it deprecated, and replace all in-tree uses with an expression that is slower but does the right thing. Closes zephyrproject-rtos#18739. Signed-off-by: Peter Bigot <[email protected]>
|
Doesn't #17155 already fix this, though? The resolution to that discussion was that the 32 bit conversion results should be correct to the full precision of the output word, and that's what the new conversion utilities implement and test for. |
@pabigot might want to weigh in, but I think the reason for this is the fact that we can get a fix in time for 2.0, whereas #17155 will only be merged in time for 2.1. |
|
That's fair. Mostly I'm just whining about extra work when rebasing, I guess. I agree with the fix on the whole. |
|
My take-away from TSC is that nobody thought this was important enough to fix for 2.0 (otherwise TSC would have had to vote to approve it). So it's not going in. |
|
@pabigot we've had this issue for a few releases now, so that might be why there was no vote. Just not deemed urgent enough given the amount of time this has been in the tree. |
|
Bug tracking accounting. Update milestone to match PR |
@carlescufi That may be due to a misunderstanding of the significance of the problem, either by me or by others. Until the 2.0 development series most Zephyr targets had a 100 Hz tick rate, which meant a 32-bit system clock wouldn't wrap until just over 497 days runtime. Now with #16782 most targets have a 10 kHz tick rate, which wraps the system clock after less than 5 days runtime. As I noted, durations based on subtracting Or so I calculate. I haven't had lunch yet, so somebody should probably check that math. |
|
@pabigot that's a very good point. |
|
@carlescufi @ioannisg OK, there's now #18771 for this, intended for 2.0 merge as a "documentation fix". It's a fine line between documenting the serious problems with this API to suggest people not to use it, while it's still used for many timeout operations within the kernel. BTW: Accepting this for 2.0 probably implies deprecating the function in 2.1, since we don't want the documented behavior of an API function to change multiple times in successive releases. |
ioannisg
added
priority: high
The current implementation does not return the low 32 bits of k_uptime_get() as suggested by it's documentation; it returns the number of milliseconds represented by the low 32-bits of the underlying system clock. The truncation before translation results in discontinuities at every point where the system clock increments bit 33. Reimplement it using the full-precision value, and update the documentation to note that this variant has little value for long-running applications. Closes zephyrproject-rtos#18739. Signed-off-by: Peter Bigot <[email protected]>
The current implementation does not return the low 32 bits of k_uptime_get() as suggested by it's documentation; it returns the number of milliseconds represented by the low 32-bits of the underlying system clock. The truncation before translation results in discontinuities at every point where the system clock increments bit 33. Reimplement it using the full-precision value, and update the documentation to note that this variant has little value for long-running applications. Closes #18739. Signed-off-by: Peter Bigot <[email protected]>
The current implementation does not return the low 32 bits of k_uptime_get() as suggested by it's documentation; it returns the number of milliseconds represented by the low 32-bits of the underlying system clock. The truncation before translation results in discontinuities at every point where the system clock increments bit 33. Reimplement it using the full-precision value, and update the documentation to note that this variant has little value for long-running applications. Closes #18739. Signed-off-by: Peter Bigot <[email protected]>
The current implementation does not return the low 32 bits of k_uptime_get() as suggested by it's documentation; it returns the number of milliseconds represented by the low 32-bits of the underlying system clock. The truncation before translation results in discontinuities at every point where the system clock increments bit 33. Reimplement it using the full-precision value, and update the documentation to note that this variant has little value for long-running applications. Closes #18739. Signed-off-by: Peter Bigot <[email protected]>
It was recently pointed out on slack that
k_uptime_get_32()does not returnThe lower 32-bits of the elapsed time since the system booted, in milliseconds.It returns instead the number of milliseconds corresponding to the low 32 bits of the tick counter.
The values are significantly different when you look at a 32-bit rollover of the tick clock:
This explains #17155 (comment) which had claimed that the current implementation followed an algorithm that I noted was incorrect. Apparently it does.
The text was updated successfully, but these errors were encountered: