Trying to simplify Clocktower Flashing Light Program

I have a working program that acts as a Clocktower substituting flashing lights for the bell sounds. There are 4 lights.

At the 1/4 hour the lights flash in sequence one time. At the 1/2 hour the lights flash in sequence twice. At the 3/4 hour the lights flash in sequence 3 times. At the top of the hour the lights flash in sequence 4 times and the flash on and off in unison the count of the hours.

I wrote the convoluted code below with help from articles on the internet and this forum. There are places that I repeat myself and am not sure how to simplify the code.

Any help or guidance is appreciated.

/*-----( Import needed libraries )-----*/
#include <Wire.h>
#include "RTClib.h"

/*-----( Declare Constants and Pin Numbers )-----*/
const int lightPin1 = 4;    // Light #1 30
const int lightPin2 = 5;    // Light #2 31
const int lightPin3 = 6;    // Light #3 32
const int lightPin4 = 7;    // Light #4 33

/*-----( Declare objects )-----*/
RTC_DS1307 rtc;    // Create a RealTimeClock object

int has15run = 0;
int has30run = 0;
int has45run = 0;
int has00run = 0;
int hasHourRun = 0;
int blinkCount = 4;
int hr = 2;

unsigned long currentMillis = 1;
unsigned long FlashTime = 1;
unsigned long BlinkTime = 1;
unsigned long NightTime = 1;
unsigned long PrintTime = 1;


enum LightStates { OneOn, TwoOn, ThreeOn, FourOn, FourOff, StandbyStrings, BlinkAll, NightCheck };
enum BlinkStates { AllOn, AllOff, StandbyBlinkAll };
BlinkStates bState = StandbyBlinkAll;
LightStates lState = NightCheck;


void setup() {
  pinMode (A3, OUTPUT);     // Plug the clock board into pins A2 through A5
  digitalWrite (A3, HIGH);  // Use this line as a supply voltage to the clock board
  pinMode (A2, OUTPUT);     // This pin can't be left floating if it is used as Ground for the RTC
  digitalWrite (A2, LOW);   // Set this pin low so that it acts as Ground for the RTC
  Serial.begin(57600);
  Wire.begin();
  rtc.begin();
  //rtc.adjust(DateTime(F(__DATE__), F(__TIME__))); // sets the RTC to the date & time this sketch was compiled
  //rtc.adjust(DateTime(2015, 1, 21, 3, 0, 0));  // sets the RTC with an explicit date & time

  pinMode(lightPin1, OUTPUT);      // sets the digital pins as output
  pinMode(lightPin2, OUTPUT);
  pinMode(lightPin3, OUTPUT);
  pinMode(lightPin4, OUTPUT);

  digitalWrite(lightPin1, HIGH);        // Prevents relays from starting up engaged
  digitalWrite(lightPin2, HIGH);
  digitalWrite(lightPin3, HIGH);
  digitalWrite(lightPin4, HIGH);

  bState = StandbyBlinkAll; //we start out in this machine state
  lState = NightCheck;  //we start out in this machine state
  
  PrintTime = millis();
  FlashTime = millis();
  BlinkTime = millis();
  NightTime = millis();

}

void loop() {
 currentMillis = millis();
 Blink15Min();
 Blink4Times();
}

//Delay time expired function
boolean CheckTime(unsigned long &lastMillis, unsigned long wait)
{
  //is the time up for this task?
  if (currentMillis - lastMillis >= wait)
  {
    lastMillis += wait;  //get ready for the next iteration
    return true;
  }
  return false;
}
//END of CheckTime()

void Blink15Min() {
 DateTime now = rtc.now();
  if (now.minute() == 15 && has15run == 0) {
    blinkCount = 4;
    lState = OneOn;
    has15run = 1;
    has00run = 0;
  }
  else if (now.minute() == 30 && has30run == 0) {
    blinkCount = 3;
    lState = OneOn;
    has30run = 1;
    has15run = 0;
  }
  else if (now.minute() == 45 && has45run == 0) {
    blinkCount = 2;
    lState = OneOn;
    has45run = 1;
    has30run = 0;
  }
  else if (now.minute() == 0 && has00run == 0) {
    blinkCount = 1;
    lState = OneOn;
    has00run = 1;
    hasHourRun = 0;
    has45run = 0;

  }
}

void Blink4Times() {
  switch (lState) {
    
    case OneOn:
      turnOnOne ();

      if (CheckTime(FlashTime, 1000UL))  {
        FlashTime = millis(); //initialize the next wait time
        lState = TwoOn;
      }
      break;

    case TwoOn:
      turnOnTwo ();
      if (CheckTime(FlashTime, 1000UL)) {
        FlashTime = millis(); //initialize the next wait time
        lState = ThreeOn;
      }
      break;

    case ThreeOn:
      turnOnThree ();
      if (CheckTime(FlashTime, 1000UL)) {
        FlashTime = millis(); //initialize the next wait time
        lState = FourOn;
      }
      break;

    case FourOn:
      turnOnFour ();
      if (CheckTime(FlashTime, 1000UL)) {
        FlashTime = millis(); //initialize the next wait time
        lState = FourOff;
      }
      break;

    case FourOff:
      if (CheckTime(FlashTime, 1000UL)) {
        FlashTime = millis(); //initialize the next wait time
        digitalWrite(lightPin4, HIGH);
        blinkCount++;
        if (blinkCount <= 4) {
          lState = OneOn;
        }
        else if (blinkCount >= 5 && hasHourRun == 0) {
          blinkHour();
          lState = BlinkAll;
        }
        else {
          lState = NightCheck;
        }
      }
      break;

    case StandbyStrings:
      if (CheckTime(FlashTime, 500UL)) {
        //Serial.println("Standby Strings");
      }
      break;

              case BlinkAll:
                {
                  switch (bState)
                  {
                    case AllOn:
                      turnOffLights ();
                      if (CheckTime(BlinkTime, 1000UL))
                      {
                        BlinkTime = millis(); //initialize the next wait time
                        turnOnLights ();
                        bState = AllOff;
                      }
                      break;
          
                    case AllOff:
                      if (CheckTime(BlinkTime, 1000UL))
                      {
                        BlinkTime = millis(); //initialize the next wait time
                        turnOffLights ();
                        hr = hr - 1;
                        if (hr > 0) {
                          bState = AllOn;
                        }
                        else {
                          bState = StandbyBlinkAll;
                          lState = NightCheck;
                          hasHourRun = 1;
                        }
                      }
                      break;
                    case StandbyBlinkAll:
                      break;
                  }
                  break;

      case NightCheck:
        nightLights();
        break;

      default:
        // this is just to help debugging
        //Serial.println("Unexpected BlinkALL step number!");
        break;
      }
  }
}

void blinkHour() {
 DateTime now = rtc.now();
  hr = (now.hour());

  if (hr > 12) {
    hr = hr - 12;
  }
  lState = BlinkAll;
  bState = AllOn;
}

void nightLights() {
  DateTime now = rtc.now();
  if ( now.hour() >= 16 && now.hour() <= 24) {
    turnOnLights ();
  }
  else if ( now.hour() >= 0 && now.hour() <= 6) {
    turnOnLights ();
  }
  else {
    turnOffLights ();
  }
}


void turnOffLights () {
  digitalWrite(lightPin1, HIGH); // Make sure all lights are off
  digitalWrite(lightPin2, HIGH); 
  digitalWrite(lightPin3, HIGH);
  digitalWrite(lightPin4, HIGH); 
}

void turnOnLights () {
  digitalWrite(lightPin1, LOW); // Make sure all lights are on
  digitalWrite(lightPin2, LOW); 
  digitalWrite(lightPin3, LOW); 
  digitalWrite(lightPin4, LOW);
}

void turnOnOne () {
  digitalWrite(lightPin1, LOW);
  digitalWrite(lightPin2, HIGH);
  digitalWrite(lightPin3, HIGH);
  digitalWrite(lightPin4, HIGH);
}

void turnOnTwo () {
  digitalWrite(lightPin1, HIGH);
  digitalWrite(lightPin2, LOW);
  digitalWrite(lightPin3, HIGH);
  digitalWrite(lightPin4, HIGH);
}

void turnOnThree () {
  digitalWrite(lightPin1, HIGH);
  digitalWrite(lightPin2, HIGH);
  digitalWrite(lightPin3, LOW);
  digitalWrite(lightPin4, HIGH);
}

void turnOnFour () {
  digitalWrite(lightPin1, HIGH);
  digitalWrite(lightPin2, HIGH);
  digitalWrite(lightPin3, HIGH);
  digitalWrite(lightPin4, LOW);
}
void loop() {
 currentMillis = millis();
 Blink15Min();
 Blink4Times();
}

It seems pointless to me to call Blink10Min() and Blink4Times() and have each of them determine if it is time to do something.

What I would do is have loop() get the time from the RTC, and have loop() determine if it is time to call Blink() with a value of 0, 1, 2, or 3. (1 for 15 after, 2 for 30 after, 3 for 45 after, and 0 for on the hour).

Then, Blink() could determine what to do based on the input value (and a switch statement with 4 cases).

An array of pin numbers would eliminate the need for 3 of the 4 functions to turn pins on.

Since the Arduino is not doing anything except blinking LEDs, delay() is perfectly OK to use, and simplifies the logic considerably.

PaulS:
It seems pointless to me to call Blink10Min() and Blink4Times() and have each of them determine if it is time to do something.

I agree, just can't get my head around how to combine them.

PaulS:
What I would do is have loop() get the time from the RTC, and have loop() determine if it is time to call Blink() with a value of 0, 1, 2, or 3. (1 for 15 after, 2 for 30 after, 3 for 45 after, and 0 for on the hour).

Then, Blink() could determine what to do based on the input value (and a switch statement with 4 cases).

Do you mean something like this? I understand how to asign a value of 0-3 but I don't know how to trigger the event only once per cycle. That is why I have the has15run check.

Do I make another if statement to check the value of determineBlink and then set a state in the state machine?

What happens if the blinking finishes before the minute changes?

void loop() {
 currentMillis = millis();
 DateTime now = rtc.now();
 if (now.minute() == 15 && has15run == 0) {
 determineBlink = 1;
  }
  else if (now.minute() == 30 && has30run == 0) {
 determineBlink = 2;
  }
  else if (now.minute() == 45 && has45run == 0) {
 determineBlink = 3;
  }
  else if (now.minute() == 0 && has00run == 0) {
 determineBlink = 0;
  }
 
 //Blink15Min();
 //Blink4Times();
}

PaulS:
An array of pin numbers would eliminate the need for 3 of the 4 functions to turn pins on.

I will look this up and try to figure out how to use an array. It seems doable with my limited skill set. Thank you for the suggestion.

PaulS:
Since the Arduino is not doing anything except blinking LEDs, delay() is perfectly OK to use, and simplifies the logic considerably.

This will be part of a larger program that has motor controls but I am trying to simplify this portion first so it does not get unwieldy too soon.

Do you mean something like this?

Something like that. But not exactly that.

bool doneBlinking = false;
void loop()
{
   DateTime now = rtc.now();
   if(now.minute() % 15 == 0)
   {
      if(!doneBlinking)
      {
         doneBlinking = true;
         Blink(now.minute() / 15);
      }
   }
   else
      doneBlinking = false;
}

Here, the time will be read on every pass through loop(). At n:00, n:15, n:30, and n:45, now.minute() % 15 will be 0, so it will be time to do something, if it hasn’t already been done.

So, we check if it has been done. If not, we do it, and record that it has been done.

If it is not n:00, n:15, n:30, or n:45, then we clear the “been there, done that” flag.

Calling Blink() with now.minute() / 15 will result in calling it with 0 (at n:00), 1 (at n:15), 2 (at n:30), or 3 (at n:45).

This will be part of a larger program that has motor controls but I am trying to simplify this portion first so it does not get unwieldy too soon

To actually bang a gong? If that is the case, then, still, most of the time, the Arduino won't be doing anything, so it is perfectly reasonable to use delay().

Yeah, yeah, I know that some will consider that heresy, but, really, the right tool for the job is best. If you need to do something else, delay() isn't the right tool. If not, it is.

PaulS:
Something like that. But not exactly that.

bool doneBlinking = false;

void loop()
{
  DateTime now = rtc.now();
  if(now.minute() % 15 == 0)
  {
      if(!doneBlinking)
      {
        doneBlinking = true;
        Blink(now.minute() / 15);
      }
  }
  else
      doneBlinking = false;
}



Here, the time will be read on every pass through loop(). At n:00, n:15, n:30, and n:45, now.minute() % 15 will be 0, so it will be time to do something, if it hasn't already been done.

So, we check if it has been done. If not, we do it, and record that it has been done.

If it is not n:00, n:15, n:30, or n:45, then we clear the "been there, done that" flag.

Calling Blink() with now.minute() / 15 will result in calling it with 0 (at n:00), 1 (at n:15), 2 (at n:30), or 3 (at n:45).

Ok, I understand how it counts and determines if it has be done or not. I don’t understand how calling Blink() with 0 - 3 tells the Blink function what to do. It seems that I need an if statement in the Blink() function to see if it is 0 or 1 or 2 or 3 and then do something based on that.

Ok, I understand how it counts and determines if it has be done or not. I don't understand how calling Blink() with 0 - 3 tells the Blink function what to do.

void Blink(int whatToDo)
{
   switch(whatToDo)
   {
      case 0:
         // It's n:00
         break;
      case 1:
         // It's n:15
         break;
      case 2:
         // It's n:30
         break;
      case 3:
         // It's n:45
         break;
   }
}

It seems that I need an if statement in the Blink() function to see if it is 0 or 1 or 2 or 3 and then do something based on that.

Or a switch statement with cases (as I mentioned earlier).

That makes sense.

When is it case 0 how do I use an array or simplify blinking through the Leds pausing 1 second and then turning the one off and moving to the next?

Cases 1-3 are more confusing to me. I need the array to blink the 4 leds in sequence and then I need it to repeat 1, 2 or 3 times.

I think a for loop would work but when I use a for loop and the millis counter it runs through all of the i+ iterations without doing anything.

For now I have this:

void Blink (int whatToDo) {
   switch(whatToDo)
   {
      case 0:
         // It's n:00
            blinkCount = 1;
            lState = OneOn;
            Blink4Times();
        }
         break;
      case 1:
         // It's n:15
            blinkCount = 4;
            lState = OneOn;
            Blink4Times();
         break;
      case 2:
         // It's n:30
            blinkCount = 3;
            lState = OneOn;
            Blink4Times();
         break;
      case 3:
         // It's n:45
            blinkCount = 2;
            lState = OneOn;
            Blink4Times();
         break;
   }
}

That did not work because when case0: calls Blink4Times it only runs once since Blink4Times is not in the loop.

I want

case 0: to blink the 4 leds in sequence once

case 1: to blink the 4 leds in sequence twice

case 2: to blink the 4 leds in sequence three times

case 3: to blink the 4 leds in sequence 4 times and THEN blink all four leds simultaneously the number of times relating to what hour it is.

I just can't figure it out.

That did not work because when case0: calls Blink4Times it only runs once since Blink4Times is not in the loop.

It DID work, in that it called Blink4Times(). Blink4Times() did not do what you want, but that simply means that you need to rewrite Blink4Times(), like so:

void BlinkManyTimes(int numberOfTimes)
{
   for(int b=0; b<numberOfTimes; b++)
   {
      BlinkAllThePinsInOrder();
      delay(someTime);
   }
}

Ok. I added that but the for loop seems to finish before it blinks the lights. I still have the lState State Machine. I want to simplify that it should be a simple array but I can’t get it to work.

The code below just triggers the first led and then stops.

void Blink (int whatToDo) {
   switch(whatToDo)
   {
      case 0:
         // It's n:00
            Blink4Times(4);
         break;
      case 1:
         // It's n:15
            Blink4Times(1);
         break;
      case 2:
         // It's n:30
            Blink4Times(2);
         break;
      case 3:
         // It's n:45
            Blink4Times(3);
         break;
   }
}

  
void Blink4Times( int numberOfTimes) {
  
  for(int b=0; b<numberOfTimes; b++)
  {
  lState = OneOn;  
  switch (lState) {
    
    case OneOn:
      turnOnOne ();
      if (CheckTime(FlashTime, 1000UL))  {
        FlashTime = millis(); //initialize the next wait time
        lState = TwoOn;
      }
      break;

    case TwoOn:
      turnOnTwo ();
      if (CheckTime(FlashTime, 1000UL)) {
        FlashTime = millis(); //initialize the next wait time
        lState = ThreeOn;
      }
      break;

    case ThreeOn:
      turnOnThree ();
      if (CheckTime(FlashTime, 1000UL)) {
        FlashTime = millis(); //initialize the next wait time
        lState = FourOn;
      }
      break;

    case FourOn:
      turnOnFour ();
      if (CheckTime(FlashTime, 1000UL)) {
        FlashTime = millis(); //initialize the next wait time
        lState = FourOff;
      }
      break;

    case FourOff:
      if (CheckTime(FlashTime, 1000UL)) {
        FlashTime = millis(); //initialize the next wait time
        digitalWrite(lightPin4, HIGH);
      }
      break;
  } 
  }
}

I give up. You are welded to the idea of using millis() instead of delay. While normally this is a good thing, having the Arduino do nothing for 15 minutes at a time while you eschew the use of delay() is just an unnecessary complication. So, leave your code the way it was.

Thank you for the help. I really appreciate it. I know that I could use delay but need/want to use millis since this flashing sequence is part of a larger program that has powerful motors moving objects that need to stop at the right time. Using delay risks the Arduino flashing lights on the hour, 11 o'clock takes a few seconds, that the motor would not stop in time.

I will use your code suggestions as a base and see if I can get an array of leds to light in sequence without delay.

Thanks again.