Issues with millis timers

I am trying to implement a millis timer as found on the arduino avoid delay playground page. http://playground.arduino.cc/Code/AvoidDelay

The issue that I am having is that my code will not work as posted.... compiles fine but doesnt work. The real issue is my statement BoilerDelay = millis() and PumpDelay = millis(). If i quote these lines out the code works (doesnt delay properly but the outputs work) so I must not be making the correct comparison. Some help would be appreciated

#define Boiler_Pin 3
#define Pump_Pin 4
#define Input_Pin 5

boolean BoilerOn = false;
boolean PumpOn = false; 
unsigned long BoilerDelay;
unsigned long PumpOnDelay;
unsigned long PumpOffDelay;


void setup() {
  digitalWrite(Boiler_Pin, HIGH);
  digitalWrite(Pump_Pin, HIGH);

  pinMode(Boiler_Pin, OUTPUT);
  pinMode(Pump_Pin, OUTPUT);
  pinMode(Input_Pin, INPUT_PULLUP);
  
  Serial.begin(9600);
}

void loop() {
  if (digitalRead(Input_Pin) == LOW)
  {
   if (not BoilerOn && digitalRead(Pump_Pin) == LOW)
   {
    BoilerDelay = millis() , digitalWrite(Boiler_Pin,HIGH), BoilerOn = false;
    if (not BoilerOn && millis()-BoilerDelay >= 15000)
    {
      digitalWrite(Boiler_Pin,LOW), BoilerOn = true;
      Serial.println("boiler on");
    }
   }
   if (not PumpOn && digitalRead(Input_Pin) == LOW)
   {
     PumpOnDelay = millis() , digitalWrite(Pump_Pin,HIGH), PumpOn = false;
    if (not PumpOn && millis()-PumpOnDelay >= 15000)
    {
      digitalWrite(Pump_Pin,LOW), PumpOn = true;
      Serial.println("pump on");
    }
   }
  }
  else
  {
   digitalWrite(Boiler_Pin,HIGH);
   Serial.println("Boiler Off");
   if (digitalRead(Input_Pin == HIGH) && PumpOn)
   {
    PumpOffDelay = millis(), digitalWrite(Pump_Pin,LOW);
    if (millis()-PumpOffDelay >= 15000)
    {
      digitalWrite(Pump_Pin,HIGH), PumpOn = false;
      Serial.println("pump off");
    }
   }
  }
  delay (1000);
}

When you compare values you should use parenthesis in your code.

I never know how && and == will be evaluated in which sequence.

Your code:

(not PumpOn && digitalRead(Input_Pin) == LOW)

should be changed to

((not PumpOn) && (digitalRead(Input_Pin) == LOW))

This way the compiler knows if it has to do the comparison (==) or AND (&&) first.

I have not analyzed your code in detail, however, in my case this sometimes helped.

I dont have any error codes..... This is not the issue, I have now removed all commas and put in semicolons.... Still not working. I followed the code example on the playground, I have also used commas extensively throughout lots of code and they seem to work fine. Run this, then this, then this; and so on.

This is from Wikipedia

In the C and C++ programming languages, the comma operator is a binary operator that evaluates its first operand and discards the result, and then evaluates the second operand and returns this value (and type).

I think that this is something new for most of us. Including me.

I do not really understand what this sentence wants to tell us, however, using commas instead of semicolons might change the results in an unexpected way.

I did remove all the commas and instead use semicolons - still the same result, the timer does not elapse.

Its a horrible thing, but is used quite a lot, for example this is valid and does what you'd expect it to:

int var1 = 2, var2 = 5, var3 = 10;  // declares and initialises in one go 
int var4, var5, var6;  // only declares.

I'm glad everyone is learning about commas and semicolons but its really not helping me get my project any close to working. I have never learned to code properly so for me I find code and edit it to make it work for my needs. Like i stated in my original post I found this source code on the arduino playground to avoid delay, it doesn't seem to work and I'm not sure why.

The issue still remains that the timer does not elapse with the PumpOnDelay = millis() in place. Is it possible that the code is cycling to fast and it continually updates the millis time and never allows the timer to elapse? I dont know, just a guess.

Could you try that latest change, and if it doesn't fix it re-post your latest code with all your fixes so far (ie getting rid of commas, and sorting that bracket issue)?

#define Boiler_Pin 3
#define Pump_Pin 4
#define Input_Pin 5

boolean BoilerOn = false;
boolean PumpOn = false; 
unsigned long BoilerDelay;
unsigned long PumpOnDelay;
unsigned long PumpOffDelay;


void setup() {
  digitalWrite(Boiler_Pin, HIGH);
  digitalWrite(Pump_Pin, HIGH);

  pinMode(Boiler_Pin, OUTPUT);
  pinMode(Pump_Pin, OUTPUT);
  pinMode(Input_Pin, INPUT_PULLUP);
  
  Serial.begin(9600);
}

void loop() {
  if (digitalRead(Input_Pin) == LOW)
  {
   if (not BoilerOn && digitalRead(Pump_Pin) == LOW)
   {
    BoilerDelay = millis();
    digitalWrite(Boiler_Pin,HIGH);
    BoilerOn = false;
    if ((not BoilerOn) && (millis()-BoilerDelay >= 15000))
    {
      digitalWrite(Boiler_Pin,LOW), BoilerOn = true;
      Serial.println("boiler on");
    }
   }
   if (not PumpOn && digitalRead(Input_Pin) == LOW)
   {
     PumpOnDelay = millis();
     digitalWrite(Pump_Pin,HIGH);
     PumpOn = false;
    if ((not PumpOn) && (millis()-PumpOnDelay >= 15000))
    {
      digitalWrite(Pump_Pin,LOW), PumpOn = true;
      Serial.println("pump on");
    }
   }
  }
  else
  {
   digitalWrite(Boiler_Pin,HIGH);
   Serial.println("Boiler Off");
   if ((digitalRead(Input_Pin) == HIGH) && PumpOn)
   {
    PumpOffDelay = millis();
    digitalWrite(Pump_Pin,LOW);
    if (millis()-PumpOffDelay >= 15000)
    {
      digitalWrite(Pump_Pin,HIGH);
      PumpOn = false;
      Serial.println("pump off");
    }
   }
  }
  delay (1000);
}

If you see any other brackets or commas that I've missed please let me know and I'll change it.

he issue still remains that the timer does not elapse with the PumpOnDelay = millis() in place.

This is a snippet of your code :

PumpOnDelay = millis();
digitalWrite(Pump_Pin, HIGH);
PumpOn = false;
if ((not PumpOn) && (millis() - PumpOnDelay >= 15000))

Lets read through it. Lets assume 300 ms have passed and will be returned by millis();

Set PumpOnDelay to millis(), so it now is equal to 300. Set PumpOn to false. Check if PumpOn is false (not needed, always will be at this stage!), and check that millis() - pumpOnDelay is greater than 15 seconds.

Well millis will return 300, pumpOnDelay is 300, 300 - 300 = 0 and will never get in there

Thank you tammytan, finally some help on this. I kind of figured that this was the problem and that it was resetting every loop. I am unsure of how to write this correctly though.... the input on pin 5 could come 25 minutes into the sketch or 25 days and i need it to continue to work multiple times not just once. Think of PIN 5 as a thermostat bringing on a pump and boiler, your thermostat cycles many times in an hour.

The demo several things at a time illustrates how to use millis() to manage timing.

You should only record pumpOnDelay at the moment when the pump is turned on. I think the logic would be more obvious if you call that variable pumpStartMillis

...R

Thanks to those of you that posted useful information… With a lot of trial and error i finally got it figured out and have the code working. I added in some serial.prints to help debug the problems i was having and the comparisons required to make this work correctly.

#define Boiler_Pin 3
#define Pump_Pin 4
#define Input_Pin 5

boolean BoilerOn = false;
boolean PumpOn = false; 
unsigned long BoilerDelay = 15000UL;
unsigned long PumpOnDelay = 15000UL;
unsigned long PumpOffDelay;
unsigned long Interval = 15000UL;
unsigned long Interval2 = 15000UL;
unsigned long Interval3 = 10000UL;


void setup() {
  digitalWrite(Boiler_Pin, HIGH);
  digitalWrite(Pump_Pin, HIGH);

  pinMode(Boiler_Pin, OUTPUT);
  pinMode(Pump_Pin, OUTPUT);
  pinMode(Input_Pin, INPUT_PULLUP);
  
  Serial.begin(9600);
}

void loop() {
  if (digitalRead(Input_Pin) == LOW)
  {
    PumpOffDelay = Interval3 + millis();
   if (not BoilerOn && digitalRead(Pump_Pin) == LOW)
   {
    digitalWrite(Boiler_Pin,HIGH);
    BoilerOn = false;
    if ((not BoilerOn) && (millis()-BoilerDelay <= Interval2))
    {
      digitalWrite(Boiler_Pin,LOW), BoilerOn = true;
      Serial.println("boiler on");
    }
   }
   if (not PumpOn && digitalRead(Input_Pin) == LOW)
   {
     digitalWrite(Pump_Pin,HIGH);
     PumpOn = false;
    if ((not PumpOn) && (millis()-PumpOnDelay <= Interval))
    {
      digitalWrite(Pump_Pin,LOW), PumpOn = true;
      Serial.println("pump on");   
    }
   }
  }
  else
  {
   digitalWrite(Boiler_Pin,HIGH);
   BoilerOn = false;
   Serial.println("Boiler Off");
   if ((digitalRead(Input_Pin) == HIGH) && PumpOn)
   {
   // PumpOffDelay = millis();
    digitalWrite(Pump_Pin,LOW);
    if (millis()-PumpOffDelay <= Interval3)
    {
      digitalWrite(Pump_Pin,HIGH);
      PumpOn = false;
      Serial.println("pump off");
    }
   }
    PumpOnDelay = Interval + millis();
    BoilerDelay = (Interval + Interval2 + millis());
  }
  Serial.print("Interval - ");
  Serial.println(Interval);
  Serial.print("Interval2 - ");
  Serial.println(Interval2);
  Serial.print("Interval3 - ");
  Serial.println(Interval3);
  Serial.print("BoilerDelay - ");
  Serial.println(BoilerDelay);
  Serial.print("PumpOnDelay - ");
  Serial.println(PumpOnDelay);
  Serial.print("PumpOffDelay - ");
  Serial.println(PumpOffDelay);
  Serial.print("CurrentMillis - ");
  Serial.println(millis());
  Serial.println();
  delay (1000);
}

tammytam: Its a horrible thing, but is used quite a lot, for example this is valid and does what you'd expect it to:

int var1 = 2, var2 = 5, var3 = 10;  // declares and initialises in one go 
int var4, var5, var6;  // only declares.

Those commas are not the comma operator. They're separators in a variable declaration list. There are actually at least 5 different uses for the comma in C that I can think of off the top of my head.