Skip to content
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

setInterval() Bug #1

Closed
Hannes-III opened this issue Sep 22, 2015 · 3 comments
Closed

setInterval() Bug #1

Hannes-III opened this issue Sep 22, 2015 · 3 comments

Comments

@Hannes-III
Copy link
Collaborator

over night I have been thinking about that.
We implemented the fix it by placing: if (iEnabled) enable(); into Task::setIterations().
I am not sure if that is completely right and if it should not be like:

void Task::setIterations(long aIterations) { 
    if (iEnabled && iIterations == 0) enable();
    iSetIterations = iIterations = aIterations; 
}

You have to decide if that makes sense . My motivation was:

  1. if iterations is not 0 there was no problem, so there was no need to do it.
  2. but more importantly: enable() resets the iPreviousMillis value. But in the case, that the Task is still running, this would destroy the current interval.
    I have a Task running. Its Interval is set to 1o.ooo ms. Every 1o seconds the callback is run.
    Four seconds after the callback was run, I call Task::setIterations() this will reset the Value of iPreviousMillis in a way that the task is due to run immediately. But it should wait another six seconds.

I placed it before the reset of the Variables so that I have the untouched iIterations to compare. I think that is OK too.

You wrote that you have placed the fix into Task::set() too. I Think that was not necessary, before I knew what the Problem was and before there was Task::restart() I used full Task::set() as workaround for Task::setIterations(), because there it worked without any Problems. Although now, looking at the code I cannot understand why.

PS.: great Library, Thank You.

@arkhipenko
Copy link
Owner

Hi Hannes,
That's a good point.Originally, I had tasks automatically disable after last iteration. I should have kept it that way...
setIterations is just a setter method and should not control the execution logic. Next thing you know we will need a setIterationsDelayed() method if immediate execution is not needed.
Maybe I should just go back to that option: it is clean, takes less code and gives programmer explicit responsibility to reactivate the task if s/he so desires... What do you think?
So proposal is to go back to:void Task::setIterations(long aIterations) {
iSetIterations = iIterations = aIterations;
}Drop disableOnLastIteration method altogether, and have the tasks auto-disable after last iteration always.
What do you think?
Anatoli

@arkhipenko
Copy link
Owner

Fixed in the latest push in the master branch.
I will create a separate branch for a version which always disables task on last iteration

@Hannes-III
Copy link
Collaborator Author

Hi Anatoli,
Hmmmmmmmm, this is a question of design, what you expect from your tool.
I think you have the power and decide the direction where this should go. I don't think that a branch would be a good Idea. Makes it too complicated to decide and therefore less attractive (I think).

Actually Me as a user, I could perfectly live with the variant that a finished Task is disabled automatically. Particularly because you introduced the restart() methods. Since those exist, the cases where you have to change the iterations are quite special cases that do not happen too often.

The only important thing is, that the documentation is consistent with the functionality of the code and that the promised functionality runs flawlessly.

Further more, if it saves some Bytes, go for it!
Hannes

PS.: the good documentation was one of the reasos why I started to work with your library formerly.

JimF42 added a commit to JimF42/TaskScheduler that referenced this issue Nov 29, 2015
Based upon http://arduino.stackexchange.com/a/12588/10648, the extra
code for _TASK_ROLLOVER_FIX should not be needed if the math is done
slightly different. I updated the code and did some quick tests and it
appears correct. Example arkhipenko#6 returns the same values for IDLE, but a few
less ms when not compiled with IDLE. Using the setMillis() function in
the SO posting, I did some quick tests with Example arkhipenko#2. using
setMillis(-3000) sets the millis() value to 3 seconds before rollover.
Task arkhipenko#1 gets an extra catch-up hit, but not Task arkhipenko#2. But, it works the
same as the original code though when using setMillis(-3000).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants