kernel: Clarify timeout API regarding negative inputs #20439
Conversation
nordic-krch
requested review from
andyross,
pabigot,
andrewboie and
carlescufi
kernel/sched.c
Outdated
| @@ -975,6 +977,8 @@ s32_t z_impl_k_sleep(int ms) | |||
| { | |||
| s32_t ticks; | |||
|
|
|||
| __ASSERT(ms >= 0, "Only positive values are accepted."); | |||
There was a problem hiding this comment.
Can't do this. Existing code passes K_FOREVER which will fail. That needs to get cleaned up.
There was a problem hiding this comment.
Yes. Ten examples in-tree, probably more outside. k_sleep(K_FOREVER) is an obvious thing to do, even though it doesn't work as people expect unless it's within a while (true) loop.
There was a problem hiding this comment.
moved it to #20448.
Misinterpretation? |
Perhaps. But more likely they might expect code that was working as desired before #19591 would continue to behave the same way in v2.1. |
That's for sure one thing. You can ask author of that piece of code if that was his intention: zephyr/lib/cmsis_rtos_v2/kernel.c Line 152 in 8e55968 Another is that user may expect that he can perform arithmetic operations on timeout or sleep and don't care about negative result which will be interpreted (and currently is, at least until #19591) as the past. |
nordic-krch
force-pushed
the
clarify_timeout_api
branch
from
ba18adb to
07d3b40
Compare
nordic-krch
changed the title
|
@pabigot can you take one more look? |
Timeout and use s32_t as an argument but only positive values are accepted (or special value like K_FOREVER). It was not specified in the description which may lead to misinterpretation. Signed-off-by: Krzysztof Chruscinski <[email protected]>
kernel/sched.c
Outdated
| @@ -368,6 +368,7 @@ static void pend(struct k_thread *thread, _wait_q_t *wait_q, s32_t timeout) | |||
| } | |||
|
|
|||
| if (timeout != K_FOREVER) { | |||
| __ASSERT(timeout >= 0, "Only positive values are accepted."); | |||
There was a problem hiding this comment.
Non-negative, not positive.
nordic-krch
force-pushed
the
clarify_timeout_api
branch
2 times, most recently
from
0f82f83 to
711fbb1
Compare
|
All checks passed. Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages. |
Add assert when negative (except K_FOREVER) is passed as timeout. Add negative timeout correction to 0. Signed-off-by: Krzysztof Chruscinski <[email protected]>
nordic-krch
force-pushed
the
clarify_timeout_api
branch
from
711fbb1 to
1c52cff
Compare
|
@andrewboie feel free to merge or wait for @andyross 's review |
|
@carlescufi let me ping @andyross just to be sure |
|
|
||
| if (timeout < 0) { | ||
| timeout = 0; | ||
| } |
There was a problem hiding this comment.
What's the value of having both an assert and a runtime clamp? I'd leave the former in and take this out. Note that this code gets changed in the timeout patch anyway, where conversion happens in the initialization of the k_timeout_t before we get here.
There was a problem hiding this comment.
agree with andyross here, @andrewboie merged while I was commenting, so we will deal with this later when doing the overall runtime checks
Timeout API use s32_t as an argument but only positive
values are accepted (or special value like K_FOREVER). It was
not specified in the description which may lead to misinterpretation.
Fixes #20438.