Timer getting stuck

I’m trying to cycle a water pump on and off for an interval of 120 seconds for my hydroponic garden. I’ve run into trouble with the timer getting stuck and leaving the pump on. Here’s the part of the code responsible for this (full code at the bottom):

void Button::cycle(uint8_t interval) {            // Timer issues? *****Gets stuck here*****
  Serial.println("Cycle Begin");
  unsigned long curr = millis();
  while (millis() - curr < interval*1000) {
    if (!this->getState()) {
      this->on();
    }
  }
  this->off();
}

I’m pretty certain that I have the code is right, it does work reliably for shorter intervals and it does work just fine as is sometimes, but usually not. I have the same issues with using delay(120*1000); It gets stuck there far beyond 120 seconds. I’ve caught it malfunctioning on intervals above 37 seconds, but it seems probabilistic.

I tried to catch the problem with a simple test code, but haven’t been able to reproduce it. I may just need to test it longer.
Test code:

unsigned long prev = 0;
unsigned long interval = 1;

void setup() {
  Serial.begin(9600);
  Serial.println("BEGIN");
}

void loop() {
  unsigned long curr = millis();
  if (curr - prev >= interval*1000) {
    prev = curr;
    String out = "Interval: " + String(interval) + "s";
    Serial.println(out);
    interval++;      // add 1s to interval
  }
}

Full code here:

/* 3/22/18
    Hydroponic garden automated controller
    V0.2 3/26/18
      Limited functionality, Timing issue
        Pump stuck on when longer cycle intervals are used (37+ seconds)
      Future versions to include
        Data loging
        Water thermostat control
        Humidity sensor
        OLED display with UI
*/

//#include <DHT.h>
#include <DS3231.h>   // RTC Library

//#define DHTPIN 7
//#define DHTTYPE DHT11
//DHT dht(DHTPIN, DHTTYPE); // Humidity sensor
DS3231  rtc(SDA, SCL);    // Define rtc pins

// Pins
const uint8_t remote[9] = {A1, A0, A3, A2, 2, 3, 6, 5}; //(OFF,ON)

class Button {
  public:
    Button(int, String);
    String device;
    bool getState();
    void on();
    void off();
    void cycle(uint8_t interval);               // on for interval, off
  private:
    uint8_t num;                                // button number
    bool state;
};

Button pump(1, "Pump"), light(2, "Light"), air(4, "Air");

void setup() {
  Serial.begin(9600);
  Serial.println("BOOT UP");

  //  dht.begin();
  rtc.begin();

  // Set pins
  for (uint8_t i = 0; i < 8; i++) {
    pinMode(remote[i], OUTPUT);
    digitalWrite(remote[i], LOW);
    if (i < 4) {                                // press each off button
      digitalWrite(remote[i], HIGH);
      delay(100);
      digitalWrite(remote[i], LOW);
      delay(100);
    }
  }
//  rtc.setTime(, , 0);                         // set current 24h time (hour, min, sec)
//  rtc.setTime(23, 59, 55);                    // Test condition
  air.on();                                     // air always on
  Serial.print("CURRENT TIME: "); displayTime(rtc.getTime());
}

void loop() {
  Time t = rtc.getTime();
  if (t.sec == 0 && t.min == 0) {               // only check on the hour
    if (t.hour == 0 || t.hour == 6 || t.hour == 12 || t.hour == 18){
      pump.cycle(120);                          // cycle pump on these hours
    }
  }
  if (!light.getState() && t.hour >= 6) {       // turn on lights after 6AM
    light.on();
  } else if (light.getState() && t.hour < 6) {
    light.off();
  }
  if (pump.getState()) {                        // pump should always be off outside of cycle
    pump.off();                                 // be sure pump is off
  }
}

/***************** Button Class *****************/
Button::Button(int buttNum, String devName) {
  num = buttNum - 1;
  device = devName;
  state = false;
}

bool Button::getState() {
  return state;
}

void Button::off() {
  digitalWrite(remote[num], HIGH);
  delay(100);
  digitalWrite(remote[num], LOW);
  state = false;
  String out = device + " OFF: ";
  Serial.print(out); displayTime(rtc.getTime());
}

void Button::on() {
  digitalWrite(remote[num + 4], HIGH);
  delay(100);
  digitalWrite(remote[num + 4], LOW);
  state = true;
  String out = device + " ON: ";
  Serial.print(out); displayTime(rtc.getTime());
}

void Button::cycle(uint8_t interval) {            // Timer issues? *****Gets stuck here*****
  Serial.println("Cycle Begin");
  unsigned long curr = millis();
  while (millis() - curr < interval*1000) {
    if (!this->getState()) {
      this->on();
    }
  }
  this->off();
}
/***************** End Button Class *****************/

// Convert 24h to 12 time and display
void displayTime(Time t) {
  String timeString;
  String mereidiem; // AM or PM
  String hmDiv;     // hour / min divider :
  String msDiv;
  uint8_t hours;    // for adjusting to 12h clock

  if (t.hour > 12) {    // adjust to 12h clock
    hours = t.hour - 12;
    mereidiem = " PM";
  } else {
    mereidiem = " AM";
    if (t.hour == 0) {  // midnight is 0 o'clock
      hours = 12;
    } else {
      hours = t.hour;
    }
  }
  if (t.min < 10) { // Keeping the # of digits consistant
    hmDiv = ":0";
  } else {
    hmDiv = ":";
  }
  if (t.sec < 10) {
    msDiv = ":0";
  } else {
    msDiv = ":";
  }

  timeString = String(hours) + hmDiv + String(t.min) + msDiv + String(t.sec) + mereidiem;
  Serial.println(timeString);
  /*
    display.setTextSize(1);
    display.setTextColor(WHITE);
    display.setCursor(0, 0);
    display.print(timeString);
  */
}

DS3231 Library used

interval*1000This is a byte times a 16 bit integer, which can easily overflow the maximum value of 32767 for the default "signed int" data type.

Throughout your code you should be using unsigned long variables with time, e.g. use expressions like

interval*1000UL
if (curr - prev >= interval*1000)

interval is a uint8_t. 1000 is an int. So the result gets stuck into a 16 bit signed int. That means the maximum value that interval can work with is 37. Anything larger overflows this calculation and makes for a negative number. Current time - previous time will always be greater than this negative number because that is all calculated with unsigned long so it's always greater than 0. So you get stuck in an infinite loop.

Try it this way instead:

if (curr - prev >= interval*1000ul)

The ul forces the compiler to treat the 1000 as an unsigned long instead of an int. That fixes the overflow problem.

jremington:
interval*1000This is a byte times a 16 bit integer, which can easily overflow the maximum value of 32767 for the default "signed int" data type.

Throughout your code you should be using unsigned long variables with time, e.g. use expressions like

interval*1000UL

That thought did come to mind, you were right. I tested it at 32767ms and 32769ms but it worked for both, maybe because there was no *1000. I just tried it again with the 1000UL and it worked.

If it is the datatype size, shouldn't it stop sooner than the given interval because it would roll over to a smaller value? What caused it to get stuck?

When you do the math in the sketch, print it to the serial monitor and see what you get.

Always, always, ask yourself if the 'type' you pick for a variable is correct.

And remember, math is done as an 'int' unless you state otherwise.