Millis don't seem to be counting properly . . .

When I run this program with millis that represent 1 second or 2.4 seconds, etc. i.e. short periods of time, the Arduino Nano has no problem calculating the off time and the on time.

But with the program as it stands, all the pins turn on, and then only pins 10 and 12 turn off and on. Pins 11 and 13 stay on constantly. Also, when pins 10 and 12 turn off and on, it's definitely not for the minutes of frequency that I have in the program. For example, Pin 12 has a frequency of about 10 seconds off and 16 seconds on.

I'm not sure what's happening. Any help would be very appreciated.

//========================================

// ----------LIBRARIES--------------



// --------CONSTANTS (won't change)---------------

const int pump3 = 13;      // the pin numbers for the LEDs
const int pump2 = 12;
const int pump1 = 11;
const int pump0 = 10;


const int pump3off = 6600000; // number of millisecs between blinks
const int pump2off = 4800000;
const int pump1off = 3000000;
const int pump0off = 1200000;

const int pumpon = 600000; // number of millisecs that Led's are on - all three leds use this


//------------ VARIABLES (will change)---------------------

byte pump3State = LOW;             // used to record whether the LEDs are on or off
byte pump2State = LOW;           //   LOW = off
byte pump1State = LOW;
byte pump0State = LOW;

unsigned long currentMillis = 0;    // stores the value of millis() in each iteration of loop()
unsigned long previouspump3Millis = 0;   // will store last time the LED was updated
unsigned long previouspump2Millis = 0;
unsigned long previouspump1Millis = 0;
unsigned long previouspump0Millis = 0;

//========================================

void setup() {

  Serial.begin(115200);
  Serial.println("pumptimer2.ino");  
  pinMode(pump3, OUTPUT);
  pinMode(pump2, OUTPUT);
  pinMode(pump1, OUTPUT);
  pinMode(pump0, OUTPUT);
}

//========================================

void loop() {
  currentMillis = millis();   
  updatepump3();
  updatepump2();
  updatepump1();
  updatepump0();
  switchpumps();

}

//========================================

void updatepump3() {

  if (pump3State == LOW) {
    if (currentMillis - previouspump3Millis >= pump3off) {
       pump3State = HIGH;
       previouspump3Millis += pump3off;
     }
  }
  else {
    if (currentMillis - previouspump3Millis >= pumpon) {
       pump3State = LOW;
       previouspump3Millis += pumpon;
    } 
  }
}

//========================================

void updatepump2() {

  if (pump2State == LOW) {
    if (currentMillis - previouspump2Millis >= pump2off) {
       pump2State = HIGH;
       previouspump2Millis += pump2off;
    }
  }
  else {
    if (currentMillis - previouspump2Millis >= pumpon) {
       pump2State = LOW;
       previouspump2Millis += pumpon;
    } 
  }    
}

//========================================

void updatepump1() {

  if (pump1State == LOW) {
    if (currentMillis - previouspump1Millis >= pump1off) {
       pump1State = HIGH;
       previouspump1Millis += pump1off;
    }
  }
  else {
    if (currentMillis - previouspump1Millis >= pumpon) {
       pump1State = LOW;
       previouspump1Millis += pumpon;
    }
  }    
}

//========================================

void updatepump0() {

  if (pump0State == LOW) {
    if (currentMillis - previouspump0Millis >= pump0off) {
       pump0State = HIGH;
       previouspump0Millis += pump0off;
    }
  }
  else {
    if (currentMillis - previouspump0Millis >= pumpon) {
       pump0State = LOW;
       previouspump0Millis += pumpon;
    }
  }    
}

//========================================

void switchpumps() {

  digitalWrite(pump3, pump3State);
  digitalWrite(pump2, pump2State);
  digitalWrite(pump1, pump1State);
  digitalWrite(pump0, pump0State);
}

You should have a look in the reference section (under learning above) of this site at the entry for int. See what kinds of values it can and cannot hold. See what's the biggest number there.

You have a lot of this:

previouspump3Millis += pump3off;

Which should probably have been this:

previouspump3Millis = currentMillis;

That's not an issue. Adding one interval is how you do it if you want to make sure the timing still lines up even if one tick isn't late. By setting to millis then if one interval is late they all become late. It enforces a minimum time limit.

If you're trying to synchronize many things then the add an interval works best. Just make sure you don't add to something beyond the current time or rollover issues come into play.

The OPs real issue is that his timing variables don't hold the numbers he thinks they do.

The op has to learn the sizes of the variaboes.
Char -127<x<127
Byte 0<=x<=256
Int -2^15<x<2^15
long -2^31<x<2^31
Unsigned long=millis() values.

Use the littlest variable type possible for every use

Silente:
The op has to learn the sizes of the variaboes.
Char -127<x<127
Byte 0<=x<=256
Int -2^15<x<2^15
long -2^31<x<2^31

Or, more intuitively:
char (aka int8_t) -128 to 127
byte (aka uint8_t) 0 to 255
int (aka int16_t) -32768 to 32767
long (aka int32_t) -2147483648 to 2147483647
unsigned long (aka uint32_t) 0 to 4294967295

(Try not to use the XOR operator when expressing powers of numbers)

Danois90:
You have a lot of this:

previouspump3Millis += pump3off;

Which should probably have been this:

previouspump3Millis = currentMillis;

The first way ...

previouspump3Millis += pump3off;

is actually the better way as it avoids small errors accumulating.

...R

Thank you for this advice. I have never scaled a constant like this, so I wasn't looking for problems like what size of variable I was using.

While I feel foolish, it's a valuable lesson learned.

Thanks again!

Danois90:
You have a lot of this:

previouspump3Millis += pump3off;

Which should probably have been this:

previouspump3Millis = currentMillis;

Adding the increment rather than using currentMillis is more accurate.

PaulMurrayCbr:
Adding the increment rather than using currentMillis is more accurate.

Yes, as long as the intervals exceed the time it takes to "loop()" :slight_smile:

Danois90:
Yes, as long as the intervals exceed the time it takes to "loop()" :slight_smile:

The code would need to be badly organized for that to happen and, whichever way the value is updated, if loop() is that slow the timing will be wrong.

...R

PaulMurrayCbr:
Adding the increment rather than using currentMillis is more accurate.

That depends on the situation. If I want the timing to line up even then you are right. But if I want to enforce some minimum time then I might be more interested in catching when it actually happened.

Both are valid and which one you might choose depends on how you want it to behave. Neither one is always right or wrong.

Robin2:
The code would need to be badly organized for that to happen and, whichever way the value is updated, if loop() is that slow the timing will be wrong.

...R

Yes, but if the interval is added and the loop is slow for a period and then becomes faster again, then the timed block of code would be executed with a faster rate than the wanted interval until "previousInterval" has "cought up with millis()". Agreed, this would be the result of bad coding, but there are a lot of "brainless coders" asking in these forums, so.. :wink:

Robin2:
The code would need to be badly organized for that to happen and, whichever way the value is updated, if loop() is that slow the timing will be wrong.

Meh, it can happen if something is blocking - waiting on IO or the network. What happens in that case is that if you use the more accurate method, the sketch will flick though the skipped steps real fast and then settle down back in sync. This is perhaps good if you are displaying video, and bad if you are trying to run a robot. As with everything, it depends on what you are trying to do.

First of all people have to remember that, if they use miplos timing (best way) they CAN'T use delay timing. And so the code can be quicker.
But I have never understood why use a variable correntmillis and don't use millis() directly

PaulMurrayCbr:
Meh, it can happen if something is blocking - waiting on IO or the network.

That's exactly what I mean by "badly organized" :slight_smile:

And. yes, I can see that there are occasions when a "catch-up" system would be unsuitable. I did not get the impression that that was relevant in the OP's project.

...R

Silente:
But I have never understood why use a variable correntmillis and don't use millis() directly

It ensures that all the tests in a single iteration of loop() use the same time value.

...R