arkhipenko / TaskScheduler

Cooperative multitasking for Arduino, ESPx, STM32, nRF and other microcontrollers
http://playground.arduino.cc/Code/TaskScheduler
BSD 3-Clause "New" or "Revised" License
1.27k stars 232 forks source link

Obsoleted _TASK_ROLLOVER_FIX #10

Closed JimF42 closed 8 years ago

JimF42 commented 8 years ago

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 #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 #2. using setMillis(-3000) sets the millis() value to 3 seconds before rollover. Task #1 gets an extra catch-up hit, but not Task #2. But, it works the same as the original code though when using setMillis(-3000).

Let me know if you have questions.

arkhipenko commented 8 years ago

You are right. Thanks.

arkhipenko commented 8 years ago

Wow! The execute() loop becomes really lean.. I love it!

JimF42 commented 8 years ago

Hi,

Was I supposed to update the library.properties file too? (I'm new to this...) I noticed that the Ardunio IDE does not see that this project has been updated. What about the readme file and/or the docs in the extras folder?

Thanks, Jim

arkhipenko commented 8 years ago

No, I updated it, just didn't publish yet. Thanks.

On Mon, Nov 30, 2015 at 12:23 PM -0800, "Jim Foster" notifications@github.com wrote:

Hi,

Was I supposed to update the library.properties file too? (I'm new to this...) I noticed that the Ardunio IDE does not see that this project has been updated. What about the readme file and/or the doc folder?

Thanks, Jim


Reply to this email directly or view it on GitHub: https://github.com/arkhipenko/TaskScheduler/pull/10#issuecomment-160750766

JimF42 commented 8 years ago

OK, good to know.

JimF42 commented 8 years ago

OK, stupid question. I just went back to my project and went to update my TaskScheduler library, but I don't see 1.9.1, just 1.9.0. How do I get the Ardunio dev environment see the update?

arkhipenko commented 8 years ago

It's a mystery to me too. I usually just download and replace in the folder directly... Sorry. Can't be more helpful... :(

On Tue, Dec 8, 2015 at 4:44 PM -0800, "Jim Foster" notifications@github.com wrote:

OK, stupid question. I just went back to my project and went to update my TaskScheduler library, but I don't see 1.9.1, just 1.9.0. How do I get the Ardunio dev environment see the update?


Reply to this email directly or view it on GitHub: https://github.com/arkhipenko/TaskScheduler/pull/10#issuecomment-163069361

arkhipenko commented 8 years ago

I had to revert back to 1.9.0 - there is an issue with "delayed" methods if we use this approach.

JimF42 commented 8 years ago

Really? What kind of issue, if I may ask.

arkhipenko commented 8 years ago

Of course. When you delay a task execution for more than a current interval, instead of delaying, it actually starts executing immediately, indefinitely and as if interval is zero.

The reason is that previous millis in that scenario are ahead of current, which means that m-p is always negative, but since it's unsigned, it is always MUSH larger than interval, so it is scheduled for execution immediately, and since interval is added to previous millis after execution, it never stops (p is always ahead of m).

Please compile the following code with 1.9.1 and 1.9.0 and observe the different behavior.

#include <TaskScheduler.h>
// Callback methods prototypes

void t1Callback();

//Tasks
Task t1(1000, -1, &t1Callback);

Scheduler runner;

void t1Callback() {
    Serial.print("t1: ");
    Serial.println(millis());
}

void setup () {
  Serial.begin(115200);
  Serial.println("Scheduler delay TEST");

  runner.init();

  Serial.println("Initialized scheduler");

   runner.addTask(t1);

  Serial.println("added t1");
  delay(5000);

  t1.enableDelayed(2000);
  Serial.println("Enabled t1");
}

void loop () {
  runner.execute();
}

Regards, Anatoli

JimF42 commented 8 years ago

I will try and look at this and see what I can find (but I don't know when that will be with the holidays, wife's birthday in early January, etc.), but my first thought is that the delay code should not be relying on those negative numbers either.

Here's my take without cracking open the code. Shouldn't the delay code be handled mathematically the same way as the interval code? Isn't the delay just a special one-time version of an interval?

Or am I missing something here?

arkhipenko commented 8 years ago

Well, the main decision point is this:

if ( m - p < i ) break;

which assumes that m (millis) should always be later than p (previous millis). In the delay case, p could be pushed beyond m, which would be fine if we were using the negative numbers. My initial approach was to work with the target time, not interval, which by definition must not happen earlier than the previous millis (unless your interval is comparable to 50 days!). So rollover detection is pretty reliable. I think it is safer, but will continue pondering, since now I am not sure it is safe if a delay method causes a rollover... Fun!

            targetMillis = p + i;
            if ( targetMillis < p ) {  // targetMillis rolled over!
                if ( p > ( m - i) )  break;
// basically (p > ( m - i))  is same as (i > m - p) or (m - p < i) - are we back to delayed problem???
            }
            else
                if ( targetMillis > m ) break;

In the end, it only affects systems that run for 50 days non-stop. How many of those are there???

JimF42 commented 8 years ago

I haven't had a chance to look at the code, so these comments are partly speculation, but why would the delay code not work in a way similar to the interval code? If the newer interval code takes care of the rollover inherently, why not adjust the delay code (which I never considered at the time--sorry) to work in a similar way? Isn't the delay just a special one-off "interval"?

arkhipenko commented 8 years ago

Exactly what I thought! Could you please check the 'testing' branch? I think it works (tested with all examples and more). I know you are busy, but appreciate if you could have a look. (Happy birthday to your wife by the way)

JimF42 commented 8 years ago

@arkhipenko, yup, I can look at it and give it a spin. At this point the earliest I can get to it will be Friday evening, but more likely over the weekend.

JimF42 commented 8 years ago

@arkhipenko--I pulled down a fresh copy of "testing" today (1.9.1), and yes, it seems to work just fine. Should this version be called 1.9.2?? Just in case someone else snagged a version of 1.9.1 with the delay issue you found? Your call...

arkhipenko commented 8 years ago

I guess you are right, better avoid confusion.