Problem with delay()

Projects Programming
1

Hi,
I am sure this is just a bug in my code and not an actual issue with the board, but I cannot see the mistake. I am using a MEGA2560 to control an array of 20 air intake solenoid valves and two output solenoid valves. Each intake valve should open for 5 minutes (with the next value in the series simultaneously opening to prime the following sample). The airflow from the solenoids goes into an sampling instrument hooked to a datalogger, which records the sample value at the end of each 5 minute period.

My issue is that the Arduino controller ends up being out of sync with the datalogger. After each loop (totaling 100 minutes) the controller is ~13 seconds behind the datalogger (compounding with each successive loop). It looks like this occurs during the last cycle of the solenoid array within the loop (activation/deactivation of pin 32).

Can anyone see whether this is a bug in my code? Any suggestions on how to fix would be much appreciated.

/*
 Control of intake solenoids and output solenoids

solPins: intake solenoids
soloPins: output solenoids
 */

int solPins[] = { 
  53, 41, 49, 43, 47, 39, 51, 35, 45, 37, 52, 24, 50, 26, 48, 28, 46, 30, 44, 32};       // an array of pin numbers to which intake solenoids are attached
int pinCount = 20;           // the number of pins (array length)

void setup() {
 
  //initialize intake solenoids
  for (int thisPin = 0; thisPin < pinCount; thisPin++)  {
    pinMode(solPins[thisPin], OUTPUT);      
  }

  //initialize output solenoids
  for (int soloPin = 12; soloPin < 14; soloPin++) {
  pinMode(soloPin, OUTPUT);
  }
}

void loop() {				 //loop through the solenoid array, switch back and forth between the two output solenoids

  for (int thisPin = 0; thisPin < pinCount; thisPin++) {

       if(solPins[thisPin] > 43) {	//pins >43 activate output solenoid #1, deactivate output solenoid #2
       digitalWrite(13, LOW);
       digitalWrite(12, HIGH);
     }
     else {				//pins <43 activate output solenoid #2, deactivate output solenoid #1
       digitalWrite(13, HIGH);
       digitalWrite(12, LOW);
     }
     
       digitalWrite(solPins[thisPin], HIGH);
       digitalWrite(solPins[thisPin + 1], HIGH);
       delay(300000); 					 //5 min = 300000
       digitalWrite(solPins[thisPin], LOW);
       digitalWrite(solPins[thisPin + 1], LOW);
 
     }
 }
2

The thing to remember is that delay() is at best approximate. It doesn't deal with time in interrupt routines or other stuff your program does.... and the tolerances of the crystals mean that clocks will drift slightly. A 13 second drift over 100 minutes is ~0.2%, which is pretty good.

The way I would approach this problem is the make the switch from one solenoid to the next actively synchronous with the datalogger.. i.e. have the switch be accompanied by a signal to the datalogger. How you implement the rest is up to you, but the simplest might be to record that signal in the data stream and then figure out the details in post-processing.

If the exact period of sampling is critical, then I would suggest using an RTC (real-time-clock) module rather than delay(), and also calibrate that RTC clock for it's drift.

-- Mitch

3

Instead of using delay, use "blink without delay"...

http://www.arduino.cc/en/Tutorial/BlinkWithoutDelay

...with a few corrections...

unsigned long previousMillis = 0;

unsigned long interval = 300000;

void setup()
{
// YOUR CODE GOES HERE
}

void loop()
{
unsigned long currentMillis = millis();

if ( currentMillis - previousMillis >= interval )
{
previousMillis = currentMillis;

// YOUR CODE GOES HERE
}
}

The biggest problem is that your Sketch does not consider the time consumed performing the digitalWrite calls. Using "blink without delay" solves this problem.

4

dmarvs:
...bug in my code ...

There is an incidence of undefined behavior, which is a bug that may or may not manifest itself in observable misbehavior.

The last time through the loop the value of thisPin is 19 and there are 20 pins in the array. The statements that use thisPin+1 as the array index are addressing memory beyond the end of the array.

        digitalWrite(solPins[thisPin], HIGH);
        digitalWrite(solPins[thisPin + 1], HIGH); // Out of bounds when thisPin is equal to 19
        delay(300000); 					 //5 min = 300000
        digitalWrite(solPins[thisPin], LOW);
        digitalWrite(solPins[thisPin + 1], LOW); // Out of bounds when thisPin is equal to 19

I really doubt that this "undefined behavior" causes the timing drift that you observe. See Footnote.

However...

Even if the undefined behavior does not cause the timing problem (or other observable problems), you really gotta fix it.

Besides getting rid of undefined behavior (by not accessing the 21st element in a 20-element array), you have to ask yourself what it is really supposed to do here. (Since there is no valve number 20, when thisPin is equal to 19, should you be dealing with valves 19 and 0? Or what?)

Regards,

Dave

Footnote:
The 0.22% timing error of 13 seconds every 100 minutes seems kind of high to me, but maybe that's the best you can expect if a ceramic resonator (instead of a crystal) is supplied on the Mega board. (And the amount of time for executing 120 digitalWrite functions every 100 minutes accounts for something less than half a millisecond error, so it doesn't seem to me that a more accurate accounting for timing on the Mega can affect things with anything like the drift you are observing.)

The bottom line is that even with a more accurate clock on the Mega and taking the time for digitalWrite into account, the time reference will eventually be noticeably different from that on the datalogger unless there is some way to synchronize them, as Mitch suggested.

You are observing a drift of 13 seconds every 200 minutes, or 0.65 seconds each time through the loop.

How about this:

If you have determined experimentally that the drift is, say, 0.65 seconds each time through the loop, then you can try changing the argument to the delay function to a value 650 milliseconds lower or higher, whichever direction is needed. Better yet, use the scheme outlined by Coding Badly and adjust the value of the interval constant so that the time tracking is "good enough."

Or some such thing.

This manual calibration might not be acceptable for a commercial product, since each device will have to be calibrated individually. (There are ways to determine clock frequency that don't require to run your loop for 100 minutes, but individual calibration is still very expensive on a production line for high volumes.) But, it might be OK for a particular application or for building a small number of devices. (What is the measured accuracy of the datalogger clock? Is it "frequency-standard" accurate, or what?)

5

@davekw7x: yeah i realized this was a problem with the last valve in my array. i am sampling the last valve but would not be simultaneously priming the first valve in the array. I am not sure how to implement this properly. Putting aside the delay vs. interval for a moment, should I wrap the code for switching the main pins in an if()else() statement like this:

 if(ledPins[thisPin] != 32) {    // pin 32 is final pin in the array
       digitalWrite(ledPins[thisPin], HIGH);
       digitalWrite(ledPins[thisPin] + 1,HIGH);
       delay(300000);                                   
       digitalWrite(ledPins[thisPin], LOW);
       digitalWrite(ledPins[thisPin] + 1, LOW);
     }
     else {
      digitalWrite(32, HIGH);
      digitalWrite(53, HIGH);  // pin 53 is first pin in the array
      delay(300000);
      digitalWrite(32, LOW);
      digitalWrite(53, LOW);
     }

or is there a far more elegant/proper way to implement (especially considering my switch to the interval as shown below)? Also, thank you for the suggestion to just account for the drift in the timing. I just extended the full time interval on my datalogger by ~13 seconds and this keeps the drift between the two timers down to a manageable amount.

@Coding Badly: Thanks for the suggested switch to interval. Is this the correct way to modify my code to use the blink without delay?

unsigned long previousMillis = 0;
unsigned long interval = 300000;  //5min = 300000 

// an array of pin numbers to which intake solenoids are attached
int solPins[] = {53, 41, 49, 43, 47, 39, 51, 35, 45, 37, 52, 24, 50, 26, 48, 28, 46, 30, 44, 32};
// the number of pins (array length)
int pinCount = 20;          

void setup() 
{
   //initialize intake solenoids
  for (int thisPin = 0; thisPin < pinCount; thisPin++)  {
    pinMode(solPins[thisPin], OUTPUT);      
  }
  //initialize output solenoids
  for (int soloPin = 12; soloPin < 14; soloPin++) {
  pinMode(soloPin, OUTPUT);
  }
}

//loop through the solenoid array, switch back and forth between the two output solenoids
void loop()
{
   unsigned long currentMillis = millis(); 
   
     //loop through the solenoid array, and switch between pump solenoids with each iteration
  for (int thisPin = 0; thisPin < pinCount; thisPin++) {
    
      digitalWrite(solPins[thisPin], HIGH);
      digitalWrite(solPins[thisPin + 1], HIGH);
       
       //pins >43 activate output solenoid #1, deactivate output solenoid #2
       if(solPins[thisPin] > 43) {
       digitalWrite(13, LOW);
       digitalWrite(12, HIGH);
       }
        //pins <43 activate output solenoid #2, deactivate output solenoid #1
       else {                 
       digitalWrite(13, HIGH);
       digitalWrite(12, LOW);
       }
     
      if (currentMillis - previousMillis >= interval) {
            
        previousMillis = currentMillis;  //reset timer to 0
        digitalWrite(solPins[thisPin], LOW);
        digitalWrite(solPins[thisPin + 1], LOW);
     }
     
  }
}
6

or is there a far more elegant/proper way to implement (especially considering my switch to the interval as shown below)?

You know how big the array is, and you know the value of "thisPin" - why not use that, instead of the contents of the last element (which may change)?

Counting up, the modulo operation is useful for this sort of thing.
Counting down...not so.

7

@Coding Badly: Thanks for the suggested switch to interval.

You are welcome.

Is this the correct way to modify my code to use the blink without delay?

No.

Give this (untested) a try...

unsigned long interval = 300000;

unsigned long previousMillis = (unsigned long)(-interval);

const int solPins[] = { 
  53, 41, 49, 43, 47, 39, 51, 35, 45, 37, 52, 24, 50, 26, 48, 28, 46, 30, 44, 32};       // an array of pin numbers to which intake solenoids are attached
const int pinCount = 20;           // the number of pins (array length)

void setup()
{
  // YOUR CODE GOES HERE
}

void loop()
{
  static int thisPin;
  static int previousPin = -1;
  unsigned long currentMillis = millis();
 
  if ( currentMillis - previousMillis >= interval )
  {
    previousMillis = currentMillis;  

    // YOUR CODE GOES HERE

    if ( previousPin != -1 )
    {
      digitalWrite(solPins[previousPin], LOW);
      digitalWrite(solPins[previousPin + 1], LOW);  // WARNING: This reads past the end of the array.
    }

    if( solPins[thisPin] > 43 )
    {
      digitalWrite(13, LOW);
      digitalWrite(12, HIGH);
    }
    //pins <43 activate output solenoid #2, deactivate output solenoid #1
    else 
    {
      digitalWrite(13, HIGH);
      digitalWrite(12, LOW);
    }

    digitalWrite(solPins[thisPin], HIGH);
    digitalWrite(solPins[thisPin + 1], HIGH);  // WARNING: This reads past the end of the array.
    
    previousPin = thisPin;
    
    ++thisPin;
    
    if ( thisPin >= pinCount )
    {
      thisPin = 0;
    }
  }
}
8

In the blink without delay example, I have been using

previousMillis += 300000; // or whatever interval

instead of

previousMillis = currentMillis;

to prevent a milli tic from sneaking in just before the variable assignment.

Good idea?

9

Er, yes if you mean what I think you do. Say you want to do something every 30 minutes, then you are better adding 30 each time to whenever-you-started, than adding 30 to the current time, as you may have spent a minute doing whatever.

10

nelsone:
.I have been using

previousMillis += 300000; // or whatever interval

If the interval is 300000, I think a better idea would be to write the following, so that you only have to change one statement in the code if you want to use it with a different interval:

previousMillis += interval;

As far as that particular example, since nothing else is done in the loop, there is no way that a mysterious tick can sneak in, but if the remainder of the loop takes something like a millisecond (or more), then there could be an extra tick (or more) before the next toggle of the LED. See Footnote.

Furthermore...

It is easy to imagine situations where the remainder of the loop has conditional code that takes longer some times than others so that the example code could result in events that might not be evenly spaced.

Bottom line: I think your method is superior for getting evenly spaced events with the duration that you specify.

Regards,

Dave

Footnote:
If you take the example and print out the value of millis() each time you toggle the LED you will find that, indeed, the time between events is 1001 milliseconds and not 1000 milliseconds. It's not because of something sneaky, but because of the condition:

  if(currentMillis - previousMillis > interval) {
    previousMillis = currentMillis;

With the value of interval equal to 1000, you get print values of 1001, 2002, 3003, 4004, ...

If you want the events (in that particular example) to occur every 1000 milliseconds, change the condition to

  if(currentMillis - previousMillis >= interval) {
    previousMillis = currentMillis; // Or, maybe better: previousMillis += interval;

Now you get 1000, 2000, 3000, ...