timer: k_timer_start should be allowed to start start with duration = 0

[youvedeep]

Duration specify time interval before timer expires for first time,
it is measured in milliseconds.
A user may submit an periodic activity starting with ASAP.
So it should allow 0 as value for duration.

So this patch implements adds 0 as valid value for duration.

Jira: ZEP-2497

Signed-off-by: Youvedeep Singh youvedeep.singh@intel.com

[jhedberg]

Adding my view on this here as well: there are use cases where the k_timer_start() API user wants the timer to fire for the first time as soon as possible, and then after that using the given period. In such cases giving K_NO_WAIT seems like the most intuitive thing, i.e. forbidding it makes the API worse (IMO).

[youvedeep]

Hi @jhedberg

There are 2 approaches to this:
Approach 1 (one for which I have submitted patch):- Enforce the condition that Duration should be positive non Zero Value and remove _TICK_ALIGN while calculating final duration. In this respect I have submitted a patch @ #1073

Approach2 (Approach proposed by jhedberg): - Allow duration 0 value passed in the duration to be legal and handle 0 case inside k_timer_start something like :-
Duration = Duration < K_MSEC(1) ? K_MSEC(1) : Duration

But there are some points we should consider against approach 2:-

1. If a user want to fire a timer after 0 duration, then why she will start the timer, instead a function call can be made and this function call can do same action as timer handler.
2. Zephyr Documentation calls out clearly that Duration should be non Zero positive value.
3. Zephyr begin a RTOS should distinguish between Duration of value 0 and 1, If we go by Approach 2 then timer value programmed is going to be same for Duration value 0 and 1, whereas a user expects Duration of 1 is going to me more than 0.

Looking at above points it looks like Approach 1 is more convenient as compared to Approach 2. Also I did grep and found that all instances of k_thread_start as using Non Zero as duration argument (excepy microbit Display). For microbit Display I have submitted a patch #1074.

So technically we put 2 patches
#1073
#1074
net system behavior is going to be same as it is currently and it will confer to Zephyr Documentation too.

[jhedberg]

Hi @youvedeep

Some answers to your points regarding approach 2:

1. One reason is that the user always wants the timer callback to happen in the same context, which may be different from the one that's calling k_timer_start. This is e.g. the case with the micro:bit display driver.

2. Yes, but it's also in conflict with the asserts in the current k_timer_start implementation. One of those is not right, but I wouldn't say it must be the implementation rather than the documentation.

3. We have other "timer" APIs where K_NO_WAIT has the same intuitive "as soon as possible" meaning. One example is passing K_NO_WAIT to k_delayed_work_submit(). In such a case it behaves the same as k_work_submit().

Assuming that the justification in point 1 is valid, if the API user desires "as soon as possible" triggering of the first timer callback, having to pass 1 instead of 0 makes the API usage "weird" in that what you're giving it is a bit arbitrary and not reflecting the exact intention.

[carlescufi]

I agree with @jhedberg here in that, from the point of view of the API user, it is much more understandable to have a K_NO_WAIT parameter passed in when you want the timer to fire as soon as possible, instead of having to artificially hardcode an arbitrary value of "1". What the kernel does internally is an entirely different thing, but "fire timer ~now" is a valid usecase.

[carlescufi]

@youvedeep @galak @jhedberg @cvinayak we just had a similar confusion happen to one of our developers, when he tried to call k_sleep(K_FOREVER) and that just returns immediately. I think K_NO_WAIT and K_FOREVER should be allowed in all kernel APIs or not allowed at all.

[youvedeep]

Hi @carlescufi

I looked at this and here are my comments:-
What Code indicates:- (Do not allow K_FOREVER)
"K_FOREVER" is mapped to -1 and because of this it is returning from k_sleep() immediately without waiting.
__ASSERT(duration != K_FOREVER, "");

What Documentation indicates :- (Do not allow)
(http://zephyr-docs.s3-website-us-east-1.amazonaws.com/online/1.7.0/api/kernel_api.html) :-
void k_sleep(int32_t duration)
Put the current thread to sleep.
This routine puts the current thread to sleep for duration milliseconds.
duration: Number of milliseconds to sleep.

What intuition indicates:- (Allow -1)
if we look at type of parameter this API (int32_t duration), which indicates a signed value, So intuitively -1 (wait forever) should be allowed,

I think this confusion is starting due to this intuition ,

So we should :- "
Either allow "K_NO_WAIT or K_FOREVER" across the APIs
or
Update parameter type and document accordingly and do not allow these.

[youvedeep]

Hi @carlescufi / @jhedberg

can you please help me to verify if my latest patch fixes GFX issue on microbit device.

[carlescufi]

@youvedeep I can do that, as soon as I get back to my office where the hw is

[jhedberg]

Sorry, I didn't realize the patch had been updated (github doesn't send any notification for that). I tested with TICKLESS_KERNEL=y using samples/boards/microbit/display and it works fine. Just one small whitespace issue.

[jhedberg]

Minor thing: redundant empty line here

[youvedeep]

Thanks @jhedberg / @carlescufi for testing quickly :). appreciate your help.
Sure I will incorporate your review comment into next patch version.

[carlescufi]

@jhedberg oh thanks for that