New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from May 21, 2018

Conversation

Projects
None yet
8 participants
@kivaisan
Contributor

kivaisan commented May 15, 2018

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

@kjbracey-arm kjbracey-arm requested a review from geky May 15, 2018

@0xc0170 0xc0170 requested a review from bulislaw May 15, 2018

@bulislaw bulislaw requested review from SenRamakri and removed request for bulislaw May 15, 2018

@geky

This comment has been minimized.

Member

geky commented May 15, 2018

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.

@geky geky changed the title from Introduce API to query how much time is left for delayed event to events: Introduce API to query how much time is left for delayed event May 15, 2018

@kivaisan

This comment has been minimized.

Contributor

kivaisan commented May 16, 2018

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.

@kivaisan kivaisan force-pushed the kivaisan:eventqueue_time_left branch from 36b4590 to 601604d May 16, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented May 16, 2018

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

This comment has been minimized.

Member

geky commented May 17, 2018

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 geky requested a review from pan- May 17, 2018

*
* @return Remaining time in milliseconds or
* 0 if event is already due to be dispatched or
* -1 if invalid event id.

This comment has been minimized.

@geky

geky May 17, 2018

Member

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.

This comment has been minimized.

@kivaisan

kivaisan May 17, 2018

Contributor

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.

This comment has been minimized.

@geky

geky May 17, 2018

Member

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

This comment has been minimized.

Contributor

cmonr commented May 17, 2018

/morph build

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 17, 2018

@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-arm

This comment has been minimized.

Contributor

kjbracey-arm commented May 17, 2018

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

This comment has been minimized.

mbed-ci commented May 17, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@geky

This comment has been minimized.

Member

geky commented May 17, 2018

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

This comment has been minimized.

Contributor

cmonr commented May 18, 2018

@SenRamakri @pan- Thoughts?

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented May 18, 2018

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

@0xc0170 0xc0170 requested a review from bulislaw May 18, 2018

Introduce API to query how much time is left for delayed event
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.

@kivaisan kivaisan force-pushed the kivaisan:eventqueue_time_left branch from 601604d to 990da08 May 18, 2018

@kivaisan

This comment has been minimized.

Contributor

kivaisan commented May 18, 2018

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

This comment has been minimized.

Member

0xc0170 commented May 20, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 20, 2018

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

geky approved these changes May 20, 2018

Good job 👍

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@adbridge adbridge added ready for merge and removed needs: CI labels May 21, 2018

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented May 21, 2018

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

@cmonr cmonr merged commit 86d04d7 into ARMmbed:master May 21, 2018

13 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 845 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 8808 cycles (-30 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label May 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment