lib: posix: make sleep() and usleep() standards-compliant #52519
Conversation
cfriedt
added
bug
cfriedt
force-pushed
the
issue/51776/posix-api-is-not-portable-across-arches
branch
10 times, most recently
from
f530927 to
06f7ee8
Compare
cfriedt
force-pushed
the
issue/51776/posix-api-is-not-portable-across-arches
branch
2 times, most recently
from
315ac77 to
5ef8d70
Compare
|
nit: in commit msg for (update usleep() to follow the POSIX spec) typo s/becuase/because |
| @@ -38,7 +36,7 @@ ZTEST(posix_apis, test_posix_clock) | |||
| } | |||
|
|
|||
| /*TESTPOINT: Check if POSIX clock API test passes*/ | |||
| zassert_equal(secs_elapsed, (2 * SLEEP_SECONDS), | |||
| zassert_equal(secs_elapsed, SLEEP_SECONDS, | |||
There was a problem hiding this comment.
maybe this test update should be part of the commit that changes usleep behavior? Otherwise, this test will fail for a couple commits in git history
There was a problem hiding this comment.
I might just reorder the commits. Normally we should keep test commits and code commits separate.
Good catch though 👍
Using `usleep()` for >= 10000000 microseconds results in an error, so this test was kind of defective, having explicitly called `usleep()` for seconds. Also, check the return values of `clock_gettime()`. Signed-off-by: Chris Friedt <cfriedt@meta.com>
In the case that `sleep()` is interrupted, the POSIX spec requires it to return the number of "unslept" seconds (i.e. the number of seconds requested minus the number of seconds actually slept). Since `k_sleep()` already returns the amount of "unslept" time in ms, we can simply use that. Signed-off-by: Chris Friedt <cfriedt@meta.com>
The original implementation of `usleep()` was not compliant to the POSIX spec in 3 ways. - calling thread may not be suspended (because `k_busy_wait()` was previously used for short durations) - if `usecs` > 1000000, previously we did not return -1 or set `errno` to `EINVAL` - if interrupted, previously we did not return -1 or set `errno` to `EINTR` This change addresses those issues to make `usleep()` more POSIX-compliant. Signed-off-by: Chris Friedt <cfriedt@meta.com>
Previously, there was no test coverage for `sleep()` and `usleep()`. This change adds full test coverage. Signed-off-by: Chris Friedt <cfriedt@meta.com>
cfriedt
force-pushed
the
issue/51776/posix-api-is-not-portable-across-arches
branch
from
5ef8d70 to
d58c4ef
Compare
Fixed! |
| rem = k_sleep(K_SECONDS(seconds)); | ||
| __ASSERT_NO_MSG(rem >= 0); | ||
|
|
||
| return rem / MSEC_PER_SEC; |
There was a problem hiding this comment.
question: does the spec require rounding up? ie, if remaining is 500ms, should we return 0 or 1?
There was a problem hiding this comment.
But the return value of 0 must mean "the requested time has elapsed", no?
There was a problem hiding this comment.
Correct - rem must be non-negative
| k_msleep(useconds / USEC_PER_MSEC); | ||
| int32_t rem; | ||
|
|
||
| if (useconds >= USEC_PER_SEC) { |
There was a problem hiding this comment.
nit
It says useconds >= USEC_PER_SEC but the commit message says:
if
usecs> 1000000,
instead of >=.
There was a problem hiding this comment.
It's the spec that's important.
| rem = k_sleep(K_SECONDS(seconds)); | ||
| __ASSERT_NO_MSG(rem >= 0); | ||
|
|
||
| return rem / MSEC_PER_SEC; |
There was a problem hiding this comment.
But the return value of 0 must mean "the requested time has elapsed", no?
| @@ -14,8 +14,12 @@ | |||
| */ | |||
| unsigned sleep(unsigned int seconds) | |||
There was a problem hiding this comment.
Should we test the seconds? We can't take 0xffffff because this has special meaning for Zephyr, K_TICKS_FOREVER.
int32_t z_impl_k_sleep(k_timeout_t timeout)
{
k_ticks_t ticks;
__ASSERT(!arch_is_in_isr(), "");
SYS_PORT_TRACING_FUNC_ENTER(k_thread, sleep, timeout);
/* in case of K_FOREVER, we suspend */
if (K_TIMEOUT_EQ(timeout, K_FOREVER)) {
k_thread_suspend(_current);
SYS_PORT_TRACING_FUNC_EXIT(k_thread, sleep, timeout, (int32_t) K_TICKS_FOREVER);
return (int32_t) K_TICKS_FOREVER;There was a problem hiding this comment.
At the very least, I would be quite impressed to see a Zephyr system made today still running 136 years from now, or a system that could maintain power for that long..
Philosophically speaking though, if we waited forever, it would still be at least 2^32 - 1 seconds, so the POSIX spec is still satisfied
cfriedt
deleted the
issue/51776/posix-api-is-not-portable-across-arches
branch
4 commits:
sleep()should report unslept time in secondsusleep()to follow the POSIX specsleep()andusleep()See individual commit messages for details.
Fixes #51776
Fixes #52517
Fixes #52518