Delay yield

[vbextreme]

While writing a port official scheduler I realized that the delay() function has a bug.
this is official delay() function

void delay(unsigned long ms)
{
    uint16_t start = (uint16_t)micros();

    while (ms > 0) {
        yield();
        if (((uint16_t)micros() - start) >= 1000) {
            ms--;
            start += 1000;
        }
    }
}

This code assumes that the task called from delay lasts less than a millisecond , otherwise you can get up to an infinite loop.
simple solution:

void myDelay(unsigned long ms)
{
      if ( 0 == ms ) return;

      uint32_t sm = micros();
      uint32_t sM = millis();

      while( millis() - sM < ms - 1 ) yield();
      ms *= 1000;
      while( micros() - sm < ms - 1 );
}

It works with the same precision but should be free of bugs.

or

void myDelay(unsigned long ms)
{
    uint32_t start = micros();

    while (ms > 0) {
        yield();
        while ( ms > 0 && (micros() - start) >= 1000) {
            ms--;
            start += 1000;
        }
    }
}

[Testato]

+1

[cmaglie]

good catch,

I prefer the second version (using uint32 instead of uint16 and the while instead of the if) because the sequence:

      uint32_t sm = micros();
      uint32_t sM = millis();

it's still subject to race conditions (millis() may advance when the execution is in the middle of the two instruction). Would you like to make a pull request for this one?

[matthijskooijman]

I also prefer the second one (I think you edited your post to add it? I don't remember seeing it in the e-mail notification).

it's still subject to race conditions (millis() may advance when the execution is in the middle of the two instruction). Would you like to make a pull request for this one?

What race condition exactly? You mention millis(), but the second version doesn't use millis() at all? I can't see any race condition here?

[cmaglie]

What race condition exactly? You mention millis(), but the second version doesn't use millis() at all? I can't see any race condition here?

The second version if fine, I'm referring to the first version.

If the millis() update ISR fires exactly in the middle of the two instruction:

      uint32_t sm = micros();
      uint32_t sM = millis();

you can get something like:
sm == 100999
sM == 101

while you really wanted:
sm == 100999
sM == 100

or:
sm == 101000
sM == 101

[matthijskooijman]

I don't actually think that your example would cause any problem? In the end, the micros() value defines the exact moment of returning. I do think that the second loop should be < ms instead of < ms - 1, though. In any case, I think the other implementation is better, also because the first one is limited to 2^32 us, while the second can do 2^32 ms.

[vbextreme]

if you have five task running,which consume two milliseconds can this be

start = micros()/*0*/;

while( ms()/*2ms*/ )
{
    yield();/* run another 4 task, ellapse 8ms*/
    if ( micros()/*> 8ms*/ - start/*0*/ > 1/*ms*/ )
    {
        --ms;/*1*/
        start += 1 /*ms*/;
        /* big problem! ellapse 8 ms but remove olny one ms and increment start for only one milliseconds.*/ 
    }
} /*LOOP*/

The start losing information causing long waits unexpected.
If the yield is more long, for example approximate 65ms can have a infinite loop because it goes to fall back always near the start.
Both issues are real, a task or set of tasks can last more than a ms.
Since the function delayMicroseconds() does not implement the functionality of the yield() a task can get to have lasting (~ 0L) us, therefore a time of 65ms is acceptable.

The first solution that I posted is simple and subbivide the problem into two subproblems.
We will use Mircos() to try to get a delay() relatively accurately, and millis() just as extra time to address other tasks.
Then we're going to pick the exact start in (us) and a monitoring time (ms).
Now we enter the loop until it has elapsed the time right -1, in this way the subsequent loops will have the time necessary to increase the precision, the -1 in this case assumes a (us) lost in this whole process, in reality need a -4.
No race condition in the millis (), is not critical.

The second code I posted it at a later time, trying to elimiare the bug but using the "old" code delay() so as not to disrupt anything.

Theoretically it is more precise and secure the first version but in this context I think are equivalent and therefore I would opt for the second code fragment.

If confirmed execute the pull request .
These days I have little time , sorry for the delayed response

[shiftleftplusone]

Iis this delay thing also an issue for Due Scheduler threads ?
Or to which scheduler is this topic related ?

[vbextreme]

You have a DUE?
Create One task and use micros for get real time delay(2), add second task and use delayMicroseconds(65536).
After exit on delay in first task send on serial real time of delay.

we look what happens.

[vbextreme]

try this code

#include <Scheduler.h>

void loop2() {
  uint32_t ts,te;

  Serial.print("delay(1000) == ");
  digitalWrite(13, HIGH);

  ts = micros();
  delay(1000);
  te = micros();

  digitalWrite(13, LOW);
  Serial.print(te-ts);
  Serial.print("us == ");
  Serial.print((te-ts)/1000);
  Serial.println("ms");
  delayMicroseconds(800);
}

void loop1() {

  digitalWrite(13, LOW);
  delayMicroseconds(65535);
  digitalWrite(13, HIGH);
  yield();
}

void setup() {
  Serial.begin(9600);
  pinMode(13, OUTPUT);

  Scheduler.startLoop(loop1);
  Scheduler.startLoop(loop2);
}

void loop() {
  yield();
}