events: Introduce API to query how much time is left for delayed event

[kivaisan]

Description

If user has initiated a delayed event (either with call_in or call_every),
user might need to know how much time is left until the event is
due to be dispatched.

Added time_left() function can be used to get the remaining time.

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

[geky]

Interesting interesting interesting.

The implementation looks great (probably the best pr I've seen in a while). But I'm concerned this will encourage users to write racey code. As soon as you call equeue_timeleft, the result could be out of date. For a similar reason we haven't provided a "has my event executed?" function, which this indirectly provides.

What is this function used for? Is it something that could be handled outside of the EventQueue class? Looking at the LoraWAN pr I'm not sure this is used.

[kivaisan]

What is this function used for? Is it something that could be handled outside of the EventQueue class? Looking at the LoraWAN pr I'm not sure this is used.

In LoRa use case we use EventQueue to delay an event when TX is limited by a duty cycle. When duty cycle limits the TX, we calculate a backoff time when next TX is possible and it can be something between seconds to few minutes. User might want to know when this next TX is possible so in our case it is the same as the time when this delayed event is going to be dispatched.
Of course this could be implemented in other way but it would require more code. I see this EventQueue solution as a very simple and generic solution for our use case.

[kjbracey]

This isn't necessary, as someone can always monitor it themselves (at least if they're not using call_every). But it does save them having their own tracking storage, as it's something that the event queue must be tracking and can give a definitive answer to.

The race is fundamentally the same as cancel, I believe - it should be no more or less safe. I believe it is racey at the instant of execution, like cancel. One thing it is important to know - will it return 0 while the event is actually executing? I don't know what cancel does.

[geky]

Hmmm, Ok, you've convinced me. Lets try to get this guy in 👍

@kjbracey-arm, cancel doesn't tell you if it cancelled an event or not, after cancel you can just assume the event will not execute.

[geky]

Do you think we should actually return 0 for an invalid id? Otherwise, when exactly this value changes from 0 -> -1 isn't well defined.

[kivaisan]

I was thinking that return value of invalid id should be somehow separated from 0 which is a valid value.
But actually the "invalid event id" seems to be a little problematic. Checking id 0 is clear but a random id is not. I don't think this check works properly (I just followed how cancel was implemented):
if (e->id == id >> q->npw2)
Just a quick test with id 99 gives following:
e->id = 0, id = 99, q->npw2 = 10, id >> q->npw2 = 0
so it is comparing 0 == 0 and therefore starts calculating diff from an invalid event.

[geky]

well, same with cancel, the user isn't allowed to pass an arbitrary int, only int ids given from a post function or the NULL event id 0.

For 99, the lookup in the array will point to arbitrary memory.

[cmonr]

/morph build

[cmonr]

@kjbracey-arm, cancel doesn't tell you if it cancelled an event or not, after cancel you can just assume the event will not execute.

So cancel is similar to what a stop function for a thread might be?

[kjbracey]

after cancel you can just assume the event will not execute.

Can you though? The event could have just started executing the event, but not reached the point where the user hits its own state mutex. I think you need some protection to be sure:

 start executing event
 === thread switch to non-event thread
 * app_mutex_lock
 * if (event_pending) { cancel_event(); event_pending = false; }
 * app_mutex_unlock
 === thread switch back to event thread
 * app_mutex_lock
 * if (!event_pending) return; // << Must have some sort of "should I be running?" check
 * do processing
 * app_mutex_unlock

So cancel is similar to what a stop function for a thread might be?

Similar, but not quite so scary. Stop on a thread could kill you almost anywhere while executing - cancel on an event can't stop you mid-event.

Otherwise, when exactly this value changes from 0 -> -1 isn't well defined.

And the detection of invalid IDs is not reliable - it's just an emergency backstop, I guess?

So, what's the correct usage semantics for this and cancel? I think it has to be legal to call either while the event is still executing - you can't totally avoid that, as per the illustration above. The event code can't change state to prevent it until part-way through. But it's illegal to call it after the event has executed, right? Completion of the event execution must be assumed to invalidate the ID.

Should we be asserting in cases where we can confirm invalidity? I'd suggest yes. And maybe we don't define the return code for "invalid event ID" - it's undefined behaviour, asserting in debug builds.

[mbed-ci]

Build : SUCCESS

Build number : 2040
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6901/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

[mbed-ci]

Test : SUCCESS

Build number : 1856
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6901/1856

[mbed-ci]

Exporter Build : SUCCESS

Build number : 1689
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6901/

[geky]

Ah you're right, although you do get the garauntee if you call cancel on the same thread of dispatch.

Note that canceling an event inside itself is valuable if the event is periodic.

I keep going back and forth, but now I'm thinking sticking with -1 is a good idea. For the timing functions it represents an "infinite" timeout. And we would have the same race condition with periodic events flipping from 0 -> next timeout.

[cmonr]

@SenRamakri @pan- Thoughts?

[hasnainvirk]

Could this go through after Chris's review ? This is the baseline for another pending PR.

[kivaisan]

Updated the descriptions for cancel() and time_left() about invalid event id and updated the test to check time_left when event is executing.
cc @kjbracey-arm

[0xc0170]

/morph build

[mbed-ci]

Build : SUCCESS

Build number : 2072
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6901/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

[geky]

Good job 👍

[mbed-ci]

Test : SUCCESS

Build number : 1884
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6901/1884

[mbed-ci]

Exporter Build : SUCCESS

Build number : 1719
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6901/

[hasnainvirk]

@cmonr Could this go in ? Other stuff in the pipeline is clogged.