Alarm not in tune with time

I have built a controller for my garden to water and turn on and off lights, the lights have one on and one off per day, the pump turns on and off three times per day. Everything is working well, and I have tested all my hardware to make sure that it is all doing what it should. The problem I'm having is when I set times for the pump or light to come on, they don't come on quite the way they should. They will come on after the set times and won't turn off at the set times, it seems almost random. The code is a series of while statements that allow the setting of alarms, and then at the very bottom is where the set times are being called on with if statements. I know that I could have used switch/case, but I was pretty far along by the time I had learned about them. If anyone has any suggestions on my methods or if I have done anything flat out wrong, please let me know. Thanks!

P.S. I have removed six of the while statements to fit into the forum, in addition to the six if statements for the alarm on and off time.

// Thanks to Alec Robinson, otherwise known as alecnotalex for supplying the code for setmode
// Thanks to Arduino.cc forums for the huge amount of knowledge that helped to build this code
// Created by Gerald @ www.myplayboat.com


#include <LiquidCrystal.h>
#include <Wire.h>
#include "RTClib.h"
#include <EEPROM.h>
LiquidCrystal lcd(7, 8, 9, 10, 11, 12);
RTC_DS1307 RTC;
int setupbutton = 2, adjusthours = 3, adjustminutes = 4; //Creates a variable connected to pins 2,3,4
int setmode = 0; //Sets the Mode to 0 for the current time ready to change to any of the other modes
int pump = 5; //Digital pin for the pump relay
int light = 6; //Digital pin for the light relay

//Below this line is where the controller loads the alarm times into the sketch in the event of lost power, this way the alarms wont have to be set again
int pumponhoura = EEPROM.read(110);    // First Pump schedule ON
int pumponminutea = EEPROM.read(111);  // First Pump schedule ON
int pumpoffhoura = EEPROM.read(112);   // First Pump schedule OFF
int pumpoffminutea = EEPROM.read(113); // First Pump schedule OFF
int pumponhourb = EEPROM.read(120);    // Second pump schedule ON
int pumponminuteb = EEPROM.read(121);  // Second pump schedule ON
int pumpoffhourb = EEPROM.read(122);   // Second pump schedule OFF
int pumpoffminuteb = EEPROM.read(123); // Second pump schedule OFF
int pumponhourc = EEPROM.read(130);    // Third pump schedule ON
int pumponminutec = EEPROM.read(131);  // Third pump schedule ON
int pumpoffhourc = EEPROM.read(132);   // Third pump schedule OFF
int pumpoffminutec = EEPROM.read(133); // Third pump schedule OFF
int lightonhour = EEPROM.read(140);    // Light schedule ON
int lightonminute = EEPROM.read(141);  // Light schedule ON
int lightoffhour = EEPROM.read(142);   // Light schedule OFF
int lightoffminute = EEPROM.read(143); // Light schedule OFF


void setup () {
    
    lcd.begin(16, 2);
    Wire.begin();
    RTC.begin();
    
    
    pinMode(pump, OUTPUT);
    pinMode(light, OUTPUT);
    pinMode(setupbutton, INPUT);
    pinMode(adjusthours, INPUT);
}
 
void loop () {
 
 if (digitalRead(setupbutton) == HIGH) //If setbutton is pressed
  {
    setmode++; //Changes the mode
    delay(200); //wait so as not to move through settings too fast
    if (setmode == 9) //Resets it if the user has scrolled through all settings
    {
      setmode = 0;
    }
  }
  
  if (setmode == 0) // If the user is not in an alarm setting mode
  {
    lcd.setCursor(0, 0);
  lcd.print("Current Time    ");
  }
  
  while(setmode == 1) // User is setting the time of the first pump schedule ON
  {
    if (digitalRead(setupbutton) == HIGH) //If setbutton is pressed
    {
      setmode++;
      delay(200);
    }
    lcd.setCursor(0,0);
    lcd.print("Flood CycleA ON "); // Setting an alarm on time
    if (pumponhoura < 10)
    {
    lcd.setCursor(0,1);
    lcd.print("0");
    lcd.setCursor(1,1);
    lcd.print(pumponhoura);
    }
    else
    {
      lcd.setCursor(0,1);
      lcd.print(pumponhoura);
    }
    if (digitalRead(adjusthours) == HIGH) //If adjustbutton is pressed
    {
      pumponhoura++;
      delay(200);
    }
    if (pumponhoura > 24)
    {
      pumponhoura = 0;
    }
    lcd.setCursor(2,1);
    lcd.print(":");
    if (pumponminutea < 10)
    {
      lcd.setCursor(3,1);
      lcd.print("0");
      lcd.setCursor(4,1);
      lcd.print(pumponminutea);
    }
    else
    {
      lcd.setCursor(3,1);
      lcd.print(pumponminutea);
    }
    lcd.setCursor(5,1);
    lcd.print("   ");
    
    if (digitalRead(adjustminutes) == HIGH) //If adjustbutton is pressed
    {
      pumponminutea++;
      delay(200);
    }
    if (pumponminutea > 59)
    {
      pumponminutea = 0;
    }
   EEPROM.write(110, pumponhoura); //Write the new settings to EEPROM
   EEPROM.write(111, pumponminutea); //Write the new settings to EEPROM
  }
  
  while(setmode == 2) // User is setting the time of the first pump schedule OFF
  {
    if (digitalRead(exitmode) == HIGH) // If exit button is pressed
    {
      setmode = 0; // Set the mode to 0 to exit to the current time
    }
    if (digitalRead(setupbutton) == HIGH) //If setbutton is pressed
    {
      setmode++;
      delay(200);
    }
    lcd.setCursor(0,0);
    lcd.print("Flood CycleA OFF"); // Setting an alarm on time
    if (pumpoffhoura < 10)
    {
    lcd.setCursor(0,1);
    lcd.print("0");
    lcd.setCursor(1,1);
    lcd.print(pumpoffhoura);
    }
    else
    {
      lcd.setCursor(0,1);
      lcd.print(pumpoffhoura);
    }
    if (digitalRead(adjusthours) == HIGH) //If adjustbutton is pressed
    {
      pumpoffhoura++;
      delay(200);
    }
    if (pumpoffhoura > 24)
    {
      pumpoffhoura = 0;
    }
    lcd.setCursor(2,1);
    lcd.print(":");
    if (pumpoffminutea < 10)
    {
      lcd.setCursor(3,1);
      lcd.print("0");
      lcd.setCursor(4,1);
      lcd.print(pumpoffminutea);
    }
    else
    {
      lcd.setCursor(3,1);
      lcd.print(pumpoffminutea);
    }
    lcd.setCursor(5,1);
    lcd.print("   ");
    
    if (digitalRead(adjustminutes) == HIGH) //If adjustbutton is pressed
    {
      pumpoffminutea++;
      delay(200);
    }
    if (pumpoffminutea > 59)
    {
      pumpoffminutea = 0;
    }
   EEPROM.write(112, pumpoffhoura); //Write the new settings to EEPROM
   EEPROM.write(113, pumpoffminutea); //Write the new settings to EEPROM
  }

//This code removed to fit the forum
//--------------------------------------------------------------------------------------------------
//--------------------------------------------------------------------------------------------------
//This code removed to fit the forum  

  
  // The code below this line is where the LCD will display the time
  // and is what the user will see when there are no alarms being set.
  
  setmode = 0;
    DateTime now = RTC.now();
    
  if (now.hour() < 10) //Starts the test to see if a space needs to be inserted before the hours
  {
    lcd.setCursor(0,1); //Sets the cursor for a blank
    lcd.print(" ");   //Blanks the first digit
    lcd.setCursor(1,1); //Sets the cursor for hour
    lcd.print(now.hour(), DEC); //Prints the hour  
  }
  else
  {
    lcd.setCursor(0,1); //Sets the cursor for hours
    lcd.print(now.hour(), DEC); //Prints the hour
  }
  
  lcd.setCursor(2,1); //Sets the cursor for first colon
  lcd.print(":");
  
  if (now.minute() < 10) // Check if a zero is needed when the minute is less than 10
  {
    lcd.setCursor(3,1); 
    lcd.print("0");
    lcd.setCursor(4,1);
    lcd.print(now.minute(), DEC);
  }
  else
  {
  lcd.setCursor(3,1);  //Prints the minutes when its over 9 without a zero before it
  lcd.print(now.minute(), DEC);
  }
  
  lcd.setCursor(5,1); //Sets the cursor for colon
  lcd.print(":");
  lcd.setCursor(6,1); //Sets the cursor for seconds
  lcd.print(now.second(), DEC);    
    
 if (now.second() == 0) // blanks out the space after a single digit "second"
 {
   lcd.setCursor(7,1);
   lcd.print(" ");
 }
 // Pump on and off "A"
 // The following two if statements check to see if the real
 // time is equal to the alarm time, then puts the pin with
 // the pump to a HIGH state followed by a LOW state.
 if (now.hour() == pumponhoura)
 {
   if (now.minute() == pumponminutea)
   {
     digitalWrite(3, HIGH);
   }
 }
 if (now.hour() == pumpoffhoura)
 {
   if (now.minute() == pumpoffminutea)
   {
     digitalWrite(3, LOW);
   }
 }

//Code removed to fit forums--------------------------------------
//code removed to fit forums -------------------------------------

}

I suggest you first re-factor the code. Move all the UI code (setting alarm times) into a separate function
or functions.

If any routine is more than a page long then its a candidate for splitting up into smaller logical units - otherwise
you'll end up with spaghetti code than noone can understand clearly.

Having done that repost the code if its still misbehaving.

You have checked that

  1. the RTC is actually telling the right time?
  2. the code isn't stuck in one of the many while loops of your UI code? (those are a bad idea,
    its easy to get stuck in a loop so that the rest of the system is unresponsive - use if statements,
    remember that loop() keeps getting called - so long as no function called by loop can take too
    long to execute (or get stuck in a loop), then every part of the system will get attention regularly.

The code looks to me like it should work. Except that your digitalWrites() are using hard coded 3 instead of pump, which was set to 5.

To track down why it isn't turning on stuff at the times you are expecting, you could put some debug statements in there. Probably the RTC is working fine, since it is displaying the time when the setmode == 0, and it would be pretty obvious if the time were wrong. Maybe you could also display pumponhoura and pumponminutea and pumpoffhoura and pumpoffminutea, just to make sure they're what you think they should be.

One concern, and it isn't the problem you're seeing, and it should work anyway, as long as your loop is being executed often enough, but...

The code that turns the pump on will continuously turn on the pump as long as the rtc clock returns the hour and the minute of the preset. Which means, for one minute, it's banging that bit high. If, for some reason, you don't happen to run that code during that minute (maybe you're accidentally in set mode, or you got stuck in on of those while loops MarkT mentioned) the pump won't turn on. Maybe it would be better to check if the pump should be on (time >= pumpon and < pumpoff) and check that against the actual pump status (digitalRead(pump)). If they don't match up, then you send the digitalWrite().

You can't store ints in EEPROM directly. So, why are you storing the byte that you read from EEPROM in an int? Surely, you are not expecting to turn the lights on at 15000:14829:32813 are you?

Thanks for all the great replies.
So it is pretty clear to me now that the code needs to be cleaned up a lot, thanks MarkT for pointing out the functions part, I had no knowledge on adding in functions until now, and I have found a few great tutorials on writing functions. I'll start with that.

TanHadron, I am thinking that you are right where I have used the hard coded digitalWrites() and that may have been the most likely cause of the problem, I'll also try out using debug statements and send them to serial to make sure things are happening the way I think they are.
Also I'll use your statement to see if the pump should be on, instead of telling it to be on regardless.

PaulS, I had thought that I could store the minute or hour to the EEPROM directly, It seems like it is storing the presets and retrieving them correctly on the LCD screen. Is there something that I'm not seeing? when I write EEPROM.write(112, pumpoffhoura); does that not store the digit as I would see it, ie: 12 or 05?

Thanks again for the help!

Is there something that I'm not seeing?

The EEPROM.read() function returns a byte. Storing that 8 bit byte in a 16 bit int is a waste of space.

The EEPROM.write() function takes a byte. Passing it an int does not necessarily result in defined behavior. That the values you were passing it as truncated to byte, with no loss of data, is the only thing that saved you.

Using the correct type of variable as input to, and to hold output from, EEPROM is better.