Fix a period callback error and a tolerance error#31
Fix a period callback error and a tolerance error#31LiyouZhou wants to merge 1 commit intoARMmbed:masterfrom
Conversation
1. The first callback of a period callback now happens immediately after postcallback() instead of one period interval later. 2. The tolerance is stored as double sided tolerance but used as single sided, with effect of have double the tolerance as user intended. This has been fixed.
| if(dispatch_tree.get_num_elements() > 0) { | ||
| CallbackNode *root = dispatch_tree.get_root(); | ||
| now_plus_tolerance = wrapTime(now + root->tolerance); | ||
| now_plus_tolerance = wrapTime(now + root->tolerance/2); |
There was a problem hiding this comment.
It is a bit weird that initially we're doubling the tolerance, only to divide it by 2 later. Being consistent is probably a better idea.
There was a problem hiding this comment.
That's why I asked about the intention of double sided tolerance. When we store it we times it by 2 and every time we use it we divide. This is not intuitive unless there is a higher design purpose for this. This fix is the minimum amount of code change, but I can equally change all the other place to use single sided tolerance. What do you think?
There was a problem hiding this comment.
I think that the original purpose of the "double sided tolerance" was to allow an event's tolerance to be applied in each direction relative to the event's scheduled time: [scheduled_time - tolerance, scheduled_time + tolerance]. Since that's not implemented currently, I don't think that multiplying the initial tolerance by 2 gives us any advantage. The proper fix for this would be to actually apply and check the tolerance in both directions.
Cc @autopulated
There was a problem hiding this comment.
The current algorithm only consider one side of the tolerance: scheduled_time - tolerance. But if we consider both side, what can we do if (now > scheduled_time + tolerance)?
There was a problem hiding this comment.
@bremoran, didn't you look at this a while ago?
There was a problem hiding this comment.
I did, however what I looked at was a scenario where, under heavy load, minar would stop scheduling periodic callbacks.
There was a problem hiding this comment.
OK, I don't think that playing with the tolerance is a good idea for now. When we fix it, we'll fix it properly. Let's leave the code as it is for now.
|
I would expect a periodic callback to not trigger immediately only if it also had a delay, otherwise I don't see why it wouldn't just trigger after being scheduled. What if I want to schedule a callback with a period equal to one second, but I want it to run as soon as possible?
Thanks. It looks like just using |
|
While I appreciate the semantics you're looking for, @bogdanm, changing the behaviour of the API should require a major version bump. I would not recommend doing this right before a release. |
Completely agreed with the major version bump. Also, fair point. I'll postpone this. |
|
While I closed this PR because of the upcoming release, feel free to continue disccusing these changes, as we'll revisit it after the release. |
|
I wouldn't suggest closing this PR, rather just leave it open with a DO NOT MERGE label |
4 participants
immediately after postcallback() instead of one period
interval later.
but used as single sided, with effect of have double
the tolerance as user intended. This has been fixed.
Test result:
Note that minar036 and 027 are expected to fail
@bogdanm @0xc0170 Please review