Long timer question - ON and OFF - functions.

First of all - yes, I’m a total beginner, however, I can’t seem to find the issue going on here.
My project:
4 relays - Temp (ignore it) , Exhaust, Intake and Light.

I want the intake running all the time.
Exhaust and Light run for X hours then stop for Y hours.
The code works fine with seconds - not with minutes/hours though.
Most common problem here would be the usage of int or something that doesn’t support long digits.
It’s not the case here I guess?

Another question is - would this overflow? If this code ran for - say - 5 days, with 10 hours on 10 hours off relays, would that time shift over time? I don’t care about 10 minutes differences, but anything more than that is an issue.

Ignore gatecheck and the temp relay functions, still working on those.

Here’s the code:

// DHT sensor library - Version: Latest
#include <DHT.h>
#include <DHT_U.h>


// Adafruit Unified Sensor - Version: Latest
#include <Adafruit_Sensor.h>


#define DHTPIN 12
#define DHTTYPE DHT11
DHT dht(DHTPIN, DHTTYPE);
int gate = 2;
int rLight = 6;
int rTemp = 7;
int rExhaust = 5;
int rIntake = 4;
unsigned long offTime = 1000 * 15;
unsigned long onTime = 1000 * 15;
unsigned long previousMillis = 0;
unsigned long interval = onTime;
boolean daystate = true;

void setup()
{
  pinMode(rLight, OUTPUT);
  pinMode(rTemp, OUTPUT);
  pinMode(rExhaust, OUTPUT);
  pinMode(rIntake, OUTPUT);
  pinMode(gate, INPUT);
  Serial.begin(9600);
  dht.begin();
}

void loop()
{

  // GATECHECK!
  //while (digitalRead(gate)==LOW) {


  if (daystate == true)
  {
    // Relay ON
    digitalWrite(rLight, LOW);   // Relay Light ON
    digitalWrite(rExhaust, LOW);    // Relay Exhaust ON
    digitalWrite(rIntake, LOW);    // Relay Intake ON
  }
  if (daystate == false)
  {
    // Relay OFF
    digitalWrite(rLight, HIGH);   // Relay Light OFF
    digitalWrite(rExhaust, HIGH);    // Relay Exhaust OFF
    digitalWrite(rIntake, LOW);    // Relay Intake ON
  }

  // Grab snapshot of current time, this keeps all timing
  unsigned long currentMillis = millis();

  // Compare to previous capture to see if enough time has passed
  if ((unsigned long)(currentMillis - previousMillis) >= interval)
  {
    // Change wait interval, based on current relay state
    if (daystate)
    {
      // Relay is currently on, set time to stay off
      interval = offTime;
    }
    else
    {
      // Relay is currently off, set time to stay on
      interval = onTime;
    }
    // Toggle the relay's state
    daystate = !(daystate);
    // Save the current time to compare "later"
    previousMillis = currentMillis;
  }

  //temp control  ------------------------------
  int h = dht.readHumidity();
  int temp = dht.readTemperature();

  Serial.print("Humidity: ");
  Serial.print(" ");
  Serial.print(h);
  Serial.print(" ");
  Serial.print("Temperature: ");
  Serial.print(" ");
  Serial.print(temp);
  Serial.print(" ");

  if (temp <= 16) {
    digitalWrite(rTemp, LOW);
  }
  else {
    digitalWrite(rTemp, HIGH);
  }
  if (temp >= 28) {
    digitalWrite(rTemp, HIGH);
  }
  /*    }//while of gate
    Serial.println("Gate opened, shut down.");
    digitalWrite(rTemp, HIGH);
    digitalWrite(rLight, HIGH);
    digitalWrite(rExhaust, HIGH);     //gate was opened, shut everything down
    digitalWrite(rIntake, HIGH);
    delay(1000*60*60*24*5);*/
}//of loop

I tried to simplify your loop to better see yopur algorithm :

void loop() {
  // GATECHECK!
  //while (digitalRead(gate)==LOW) {
  digitalWrite (rIntake, LOW);    // Relay Intake ON
  digitalWrite (rLight, (daystate) ? LOW : HIGH); // Relay Light
  digitalWrite (rExhaust, (daystate) ? LOW : HIGH); // Relay Exhaust

  // Grab snapshot of current time, this keeps all timing
  unsigned long currentMillis = millis();
  // Compare to previous capture to see if enough time has passed
  if (currentMillis - previousMillis >= interval) {
    interval = (daystate) ? offTime : onTime; // Change wait interval, based on current relay state
    daystate = !(daystate); // Toggle the relay's state
    previousMillis = currentMillis; // Save the current time to compare "later"
  }

  //temp control  ------------------------------
  int h;// = dht.readHumidity();
  int temp;// = dht.readTemperature();

  Serial.print("Humidity:  ");
  Serial.print(h);
  Serial.print(" Temperature:  ");
  Serial.print(temp);
  Serial.print(" ");

  digitalWrite (rTemp, (temp <= 16) ? LOW : HIGH);
  if (temp >= 28) digitalWrite (rTemp, HIGH);
  /*    }//while of gate
    Serial.println("Gate opened, shut down.");
    digitalWrite(rTemp, HIGH);
    digitalWrite(rLight, HIGH);
    digitalWrite(rExhaust, HIGH);     //gate was opened, shut everything down
    digitalWrite(rIntake, HIGH);
    delay(1000*60*60*24*5);*/
}//of loop

I think this line

    interval = (daystate) ? offTime : onTime; // Change wait interval, based on current relay state

should be out of the if block.

Welcome to the forum and congratulations on the use of code tags with your first post.

The code works fine with seconds - not with minutes/hours though.
Most common problem here would be the usage of int or something that doesn't support long digits.
It's not the case here I guess?

There is an issue with the use of integer constants and overflow.
https://www.arduino.cc/reference/en/language/variables/constants/integerconstants/

You need to force these expressions to unsigned long with ul modifiers.

//unsigned long offTime = 1000 * 15;
//unsigned long onTime = 1000 * 15;
unsigned long offTime = 1000ul * 15;
unsigned long onTime = 1000ul * 15;

The way you calculate the number of milliseconds in onTime and offTime (1000 * 15) only works up to 32 seconds. Any higher number of seconds and the int value will overflow into negative numbers. That is because '1000' and '15' are both 'int' and the math will be done in 'int'. The overflowed value gets stored in onTime and offTime. For integer math that goes beyond 32767 you have to used 'unsigned int' and beyond 65535 you have to use 'long int'. The will take you up over two billion.

To make the code easier to read you can define some helpful constants:

const unsigned long SECONDS = 1000;
const unsigned long MINUTES = 60 * SECONDS;
const unsigned long HOURS = 60 * MINUTES;


unsigned long offTime = 15 * SECONDS;
unsigned long onTime = 15 * SECONDS;

Using those constants you should be good for intervals up to about 47 days.

lesept:
I tried to simplify your loop to better see yopur algorithm :

void loop() {

// GATECHECK!
 //while (digitalRead(gate)==LOW) {
 digitalWrite (rIntake, LOW);    // Relay Intake ON
 digitalWrite (rLight, (daystate) ? LOW : HIGH); // Relay Light
 digitalWrite (rExhaust, (daystate) ? LOW : HIGH); // Relay Exhaust

// Grab snapshot of current time, this keeps all timing
 unsigned long currentMillis = millis();
 // Compare to previous capture to see if enough time has passed
 if (currentMillis - previousMillis >= interval) {
   interval = (daystate) ? offTime : onTime; // Change wait interval, based on current relay state
   daystate = !(daystate); // Toggle the relay’s state
   previousMillis = currentMillis; // Save the current time to compare “later”
 }

//temp control  ------------------------------
 int h;// = dht.readHumidity();
 int temp;// = dht.readTemperature();

Serial.print("Humidity:  “);
 Serial.print(h);
 Serial.print(” Temperature:  “);
 Serial.print(temp);
 Serial.print(” ");

digitalWrite (rTemp, (temp <= 16) ? LOW : HIGH);
 if (temp >= 28) digitalWrite (rTemp, HIGH);
 /*    }//while of gate
   Serial.println(“Gate opened, shut down.”);
   digitalWrite(rTemp, HIGH);
   digitalWrite(rLight, HIGH);
   digitalWrite(rExhaust, HIGH);     //gate was opened, shut everything down
   digitalWrite(rIntake, HIGH);
   delay(10006060245);*/
}//of loop



I think this line


interval = (daystate) ? offTime : onTime; // Change wait interval, based on current relay state



should be out of the if block.

Ah totally forgot about the ‘?’ if simplifying, thank you. I’ll see what I can do with that ‘if’ line, I thought it’s best if it’s in there. Thanks for the reply and help!

cattledog:
Welcome to the forum and congratulations on the use of code tags with your first post.

There is an issue with the use of integer constants and overflow.
Integer Constants - Arduino Reference

You need to force these expressions to unsigned long with ul modifiers.

//unsigned long offTime = 1000 * 15;

//unsigned long onTime = 1000 * 15;
unsigned long offTime = 1000ul * 15;
unsigned long onTime = 1000ul * 15;

New to arduino, but not new to the internet!

I added the ‘ul’ modifiers - looks like that fixed the timing issue. I thought that by declaring the correct variables resolves that issue. Guess not - huge thanks!

johnwasser:
The way you calculate the number of milliseconds in onTime and offTime (1000 * 15) only works up to 32 seconds. Any higher number of seconds and the int value will overflow into negative numbers. That is because ‘1000’ and ‘15’ are both ‘int’ and the math will be done in ‘int’. The overflowed value gets stored in onTime and offTime. For integer math that goes beyond 32767 you have to used ‘unsigned int’ and beyond 65535 you have to use ‘long int’. The will take you up over two billion.

To make the code easier to read you can define some helpful constants:

const unsigned long SECONDS = 1000;

const unsigned long MINUTES = 60 * SECONDS;
const unsigned long HOURS = 60 * MINUTES;

unsigned long offTime = 15 * SECONDS;
unsigned long onTime = 15 * SECONDS;





Using those constants you should be good for intervals up to about 47 days.

That explains how the ul modifier works and why my code didn’t work out. So I guess I can either do it with the helpful constants (seconds, minutes etc.) or with the help of the modifier - which I still have to research on how to use since I’ve been only using it the way @cattledog explained.

After simplifying my code and optimizing it a bit, (less unreadable things), do you think this code could cause issues in the long run? Say after 15-20 hours of STOPs and RUNs?

The timer will only be as accurate as the resonator on the Arduino so it may be off a few minutes a day. If that degree of drift is OK then there should be no reason for it to fail, ever.

A little clean-up:

// DHT sensor library - Version: Latest
#include <DHT.h>
#include <DHT_U.h>


// Adafruit Unified Sensor - Version: Latest
#include <Adafruit_Sensor.h>


#define DHTPIN 12
#define DHTTYPE DHT11
DHT dht(DHTPIN, DHTTYPE);


const byte gate = 2;
const byte rLight = 6;
const byte rTemp = 7;
const byte rExhaust = 5;
const byte rIntake = 4;


const unsigned long SECONDS = 1000;
const unsigned long MINUTES = 60 * SECONDS;
const unsigned long HOURS = 60 * MINUTES;


unsigned long offTime = 15 * SECONDS;
unsigned long onTime = 15 * SECONDS;
unsigned long previousMillis = 0;
unsigned long interval = onTime;
boolean daystate = true;


void setup()
{
  pinMode(rLight, OUTPUT);
  pinMode(rTemp, OUTPUT);
  pinMode(rExhaust, OUTPUT);
  pinMode(rIntake, OUTPUT);
  pinMode(gate, INPUT);
  Serial.begin(9600);
  dht.begin();
}


void loop()
{
  // Grab snapshot of current time
  unsigned long currentMillis = millis();


  if (daystate)
  {
    // Everything on
    digitalWrite(rLight, LOW);   // Relay Light ON
    digitalWrite(rExhaust, LOW);    // Relay Exhaust ON
    digitalWrite(rIntake, LOW);    // Relay Intake ON
    interval = onTime;
  }
  else
  {
    // Only Intake on
    digitalWrite(rLight, HIGH);   // Relay Light OFF
    digitalWrite(rExhaust, HIGH);    // Relay Exhaust OFF
    digitalWrite(rIntake, LOW);    // Relay Intake ON
    interval = offTime;
  }


  // Has 'interval' passed?
  if (currentMillis - previousMillis >= interval)
  {
    previousMillis += interval;  // Time when time SHOULD have expired


    daystate = !(daystate);    // Toggle state
  }


  //temp control  ------------------------------
  int h = dht.readHumidity();
  int temp = dht.readTemperature();


  Serial.print("Humidity: ");
  Serial.print(" ");
  Serial.print(h);
  Serial.print(" ");
  Serial.print("Temperature: ");
  Serial.print(" ");
  Serial.print(temp);
  Serial.print(" ");


  // Turn rTemp off (HIGH) if temperature is in range
  digitalWrite(rTemp, temp > 16 &&  temp < 28);


} // end of loop

do you think this code could cause issues in the long run? Say after 15-20 hours of STOPs and RUNs?

I can't speak to the hardware, but the code should be fine since you use the subtraction of unsigned longs in your conditional statements

if ((unsigned long)(currentMillis - previousMillis) >= interval)

You can certainly test your code with large numbers by using micros() instead of millis(). A 20 hour interval will take 72 seconds.

johnwasser:
The timer will only be as accurate as the resonator on the Arduino so it may be off a few minutes a day. If that degree of drift is OK then there should be no reason for it to fail, ever.

A little clean-up:

// DHT sensor library - Version: Latest

#include <DHT.h>
#include <DHT_U.h>

// Adafruit Unified Sensor - Version: Latest
#include <Adafruit_Sensor.h>

#define DHTPIN 12
#define DHTTYPE DHT11
DHT dht(DHTPIN, DHTTYPE);

const byte gate = 2;
const byte rLight = 6;
const byte rTemp = 7;
const byte rExhaust = 5;
const byte rIntake = 4;

const unsigned long SECONDS = 1000;
const unsigned long MINUTES = 60 * SECONDS;
const unsigned long HOURS = 60 * MINUTES;

unsigned long offTime = 15 * SECONDS;
unsigned long onTime = 15 * SECONDS;
unsigned long previousMillis = 0;
unsigned long interval = onTime;
boolean daystate = true;

void setup()
{
  pinMode(rLight, OUTPUT);
  pinMode(rTemp, OUTPUT);
  pinMode(rExhaust, OUTPUT);
  pinMode(rIntake, OUTPUT);
  pinMode(gate, INPUT);
  Serial.begin(9600);
  dht.begin();
}

void loop()
{
  // Grab snapshot of current time
  unsigned long currentMillis = millis();

if (daystate)
  {
    // Everything on
    digitalWrite(rLight, LOW);  // Relay Light ON
    digitalWrite(rExhaust, LOW);    // Relay Exhaust ON
    digitalWrite(rIntake, LOW);    // Relay Intake ON
    interval = onTime;
  }
  else
  {
    // Only Intake on
    digitalWrite(rLight, HIGH);  // Relay Light OFF
    digitalWrite(rExhaust, HIGH);    // Relay Exhaust OFF
    digitalWrite(rIntake, LOW);    // Relay Intake ON
    interval = offTime;
  }

// Has ‘interval’ passed?
  if (currentMillis - previousMillis >= interval)
  {
    previousMillis += interval;  // Time when time SHOULD have expired

daystate = !(daystate);    // Toggle state
  }

//temp control  ------------------------------
  int h = dht.readHumidity();
  int temp = dht.readTemperature();

Serial.print("Humidity: “);
  Serial.print(” “);
  Serial.print(h);
  Serial.print(” ");
  Serial.print("Temperature: “);
  Serial.print(” “);
  Serial.print(temp);
  Serial.print(” ");

// Turn rTemp off (HIGH) if temperature is in range
  digitalWrite(rTemp, temp > 16 &&  temp < 28);

} // end of loop

Thank you for explaining all to me. That’s a super clean code - I’m going to use that! (after I rewrite it to rehearse :stuck_out_tongue: ) Thanks a bunch.

cattledog:
I can’t speak to the hardware, but the code should be fine since you use the subtraction of unsigned longs in your conditional statements

if ((unsigned long)(currentMillis - previousMillis) >= interval)

You can certainly test your code with large numbers by using micros() instead of millis(). A 20 hour interval will take 72 seconds.

Awesome, good to hear. I’ll try the micros to check the drift. Thank you for your help!

RonnieNine:
I'll try the micros to check the drift. Thank you for your help!

If the system clock is off by five minutes in 24 hours (86,400 seconds) it's not going to be off by five minutes in 86.4 seconds when you measure in microseconds. It will still be 5 minutes a day. You will have to run for an extended period (days) against a known good clock to measure the average drift (which will change with temperature and voltage).