My led controller

This is about a week and half worth of coding for me. I'm completely new to programming and with lots of reading and help from several members here this is what i have managed to come up with. From my testing and to the best of my knowledge everything is functioning correctly.

My question is, Is there a better or more efficient way to accomplish anything i have done here.

The program controls 2 sets of led's one is called dayLeds the other is moonLeds. It fades them up or down over a 30 minute time frame based on the time of day. It also runs the dayLeds for about 8 hrs and the moonLeds for about 4 hours. Also has the function to print the DS3231 RTC to serial.

AquariumController_2.0.ino (8.54 KB)

int dayLed = 11;
int moonLed = 10;
int dayBrightness = dayLed;
int moonBrightness = moonLed;
int prevBrightness;
int fadeCurrentMillis = 0;
int fadePreviousMillis = 0;
int fadeInterval = 7059;
int x = dayLed;
int y = moonLed;

There is NOTHING magic about the string "millis" in a time variable name. If fact, at some point (about 15 minutes after you see it, in my case), it begins to look stupid.

I really can't tell which of these variables hold pin numbers and which hold pin values. I'd rename all of these to make that crystal clear, so that 3 months later, when you have a "Hey, I want to add XXX", you don't have to guess what the heck you were doing.

How many of these variables use types that are too large? How many use types that are too small?

Time stuff always goes in unsigned long variables.

One letter global variable names are a disaster waiting to happen.

Well, that's enough on that little bit of code. 8)

byte nightHour = 24;

24? Does your clock really tell you that it is 24:45? Mine certainly doesn't.

const long interval = 1000;

In case you later decide that the interval should be -750?

  if (startMorningFade == true) {                        //Sunrise led over 30 minutes
  if (startEveningFade == true) {
  if (startMoonFade == true) {
  if (endMoonFade == true) {

How many of these are going to be true at any given time? If the answer (as I suspect) is 0 or 1, then if/else if is a better structure.

The value in startMorningFade will be either true or false. The result of comparing true to true is true. The result of comparing false to true is false. You can see, then, that the result of the comparison will be whatever the value in the boolean variable is. Therefore, the == true bit is not necessary.

checkDayCycle() also contains a lot if if statements that should be if/else if structured. It is some time. That time can not be within more than one range, so, once the proper range is found, it is not necessary to test the rest.

I like the fact that you have a lot of small functions that can be tested independently, and, once they are known to work, ignored. If it were my project, I'd create another tab, and move the small "I KNOW this works" functions to the other tab. That way, the important stuff, the stuff being developed, is on one tab, and not lost in the clutter of a lot of functions.

PaulS:

byte nightHour = 24;

24? Does your clock really tell you that it is 24:45? Mine certainly doesn't.

I realized this mistake after posting it here and have since changed it

const long interval = 1000;

In case you later decide that the interval should be -750?

  if (startMorningFade == true) {                        //Sunrise led over 30 minutes

if (startEveningFade == true) {
  if (startMoonFade == true) {
  if (endMoonFade == true) {



How many of these are going to be true at any given time? If the answer (as I suspect) is 0 or 1, then if/else if is a better structure.

The value in startMorningFade will be either true or false. The result of comparing true to true is true. The result of comparing false to true is false. You can see, then, that the result of the comparison will be whatever the value in the boolean variable is. Therefore, the == true bit is not necessary.

I will try to implement these changes

checkDayCycle() also contains a lot if if statements that should be if/else if structured. It is some time. That time can not be within more than one range, so, once the proper range is found, it is not necessary to test the rest.

I did try this is with if else but must have been doing something wrong, I will try this again this evening when I get off of work.

I like the fact that you have a lot of small functions that can be tested independently, and, once they are known to work, ignored. If it were my project, I'd create another tab, and move the small "I KNOW this works" functions to the other tab. That way, the important stuff, the stuff being developed, is on one tab, and not lost in the clutter of a lot of functions.

Will it read code from the next tab for use in the first? Say I move the functions to set morning Fade to true and still run the alarm, would it still set it true and work?

I will also revisit my global declarations.

Thanks again

Will it read code from the next tab for use in the first? Say I move the functions to set morning Fade to true and still run the alarm, would it still set it true and work?

When you compile, the IDE will combine all tabs into one cpp file, in some order. By naming the tabs, you can control the order.

I think i have everything covered you mentioned. I changed the checkDayCycle over to use seconds since midnight because the other way was causing quite a bit of trouble, i couldn't get it to work when and how i wanted and at time it would run two or three of them at the same time.

Here are the new if / else's and a new full code download.

  void checkDayCycle() {
    ts = RTC.get() % 86400;              //Gets seconds since midnight
    if (old_ts == 0 || old_ts != ts) {   //Gets current seconds since midnight
      old_ts = ts;

      if (ts >= startSunriseHour + startSunriseMinute && ts <= dayHour + dayMinute) {
        Serial.println(ts);
        startMorningFade = true;
      }
      else if (ts >= dayHour + dayMinute && ts <= startSunsetHour + startSunsetMinute)
      {
        Serial.println("Day Works");
        analogWrite(dayLed, 255);
      }
      else if (ts >= startSunsetHour + startSunsetMinute && ts <= startMoonFadeHour + startMoonFadeMinute)
      {
        Serial.println("Sunset Works");
        startEveningFade = true;
      }
      else if (ts >= startMoonFadeHour + startMoonFadeMinute && ts <= moonLightHour + moonLightMinute)
      {
        Serial.println("Moonrise Works");
        startMoonFade = true;
      }
      else if (ts >= moonLightHour + moonLightMinute && ts <= endMoonFadeHour + endMoonFadeMinute)
      {
        Serial.println("Moon Light Works");
        analogWrite(moonLed, 255);
      }
      else if (ts >= endMoonFadeHour + endMoonFadeMinute && ts <= nightHour + nightMinute)
      {
        Serial.println("Moonset Works");
        endMoonFade = true;
      }
      else if (ts >= nightHour + nightMinute && ts >= startSunriseHour + startSunriseMinute)
      {
        Serial.println("Night Works");
        analogWrite(dayLed, 0);
        analogWrite(moonLed, 0);
      }
    }
  }

and

  //-------------------Sunrise led over 30 minutes----------------------------

  if (startMorningFade && dayLed <= 255) {
    if (fadeLed - prevFadeLed >= fadeInterval) {
      // save the last time sunrise(); was called
      prevFadeLed = fadeLed;
      sunrise();
      Serial.println("dayLed Faded Up");
      dayLed++;
    } else if (dayLed >= 255) {
      startMorningFade = false;
      digitalWrite(dayLed, HIGH);
    }
  }

  //-------------------Sunset led over 30 minutes----------------------------

  else if (startEveningFade && dayLed >= 0) {
    if (fadeLed - prevFadeLed >= fadeInterval) {
      // save the last time sunset(); was called
      prevFadeLed = fadeLed;
      sunset();
      Serial.println("dayLed Faded Down");
      dayLed--;
    } else if (dayLed <= 0) {
      startEveningFade = false;
      digitalWrite(dayLed, LOW);
    }
  }

  //------------------------Moonrise led over 30 minutes--------------------------

  else if (startMoonFade && moonLed <= 255) {
    if (fadeLed - prevFadeLed >= fadeInterval) {
      // save the last time sunrise(); was called
      prevFadeLed = fadeLed;
      moonrise();
      Serial.println("moonLed Faded Up");
      moonLed++;
    } else if (moonLed >= 255) {
      startMoonFade = false;
      digitalWrite(moonLed, HIGH);
    }
  }

  //---------------------------Moonset led over 30 minutes------------------------

  else if (endMoonFade && moonLed >= 0) {
    if (fadeLed - prevFadeLed >= fadeInterval) {
      // save the last time sunset(); was called
      prevFadeLed = fadeLed;
      moonset();
      Serial.println("moonLed Faded Down");
      moonLed--;
    } else if (moonLed <= 0) {
      endMoonFade = false;
      digitalWrite(moonLed, LOW);
    }
  }

AquariumController_2.0.ino (6.64 KB)

It_Works.ino (2.06 KB)

    ts = RTC.get() % 86400;              //Gets seconds since midnight

So it does.

    if (old_ts == 0 || old_ts != ts) {   //Gets current seconds since midnight

It most certainly does NOT.

      if (ts >= startSunriseHour + startSunriseMinute && ts <= dayHour + dayMinute) {

Adding an hour value and a minute value does NOT appear to give a seconds since midnight value. Comparing apples and elephants doesn't make sense.

With these changes, the code no longer appears to make any sense. Either ALL times need to be in seconds since midnight form, or NONE of the times should be in that format.

I had also made these changes to my declared variables, would renaming these help to clear up the confusion?

unsigned long ts;
uint32_t old_ts;
unsigned long uHr = 3600;  //Convert hours to seconds
unsigned long uMn = 60;    //Convert minutes to seconds

//----------------------------ADJUSTABLE TIMES------------------------------------------
//DO NOT CHANGE uHr OR uMn, DO NOT PUT LEADING ZEROS. EXAMPLE - dayHour = 09

//-------Sunrise---------
unsigned long startSunriseHour = 9 * uHr;
unsigned long startSunriseMinute = 1 * uMn;
//-------Day-------------
unsigned long dayHour = 9 * uHr;
unsigned long dayMinute = 30 * uMn;
//-------Sunset----------
unsigned long startSunsetHour = 18 * uHr;
unsigned long startSunsetMinute = 0 * uMn;
//-------Moonrise--------
unsigned long startMoonFadeHour = 18 * uHr;
unsigned long startMoonFadeMinute = 30 * uMn;
//---------Moon----------
unsigned long moonLightHour = 19 * uHr;
unsigned long moonLightMinute = 1 * uMn;
//---------Moonset--------
unsigned long endMoonFadeHour = 23 * uHr;
unsigned long endMoonFadeMinute = 1 * uMn;
//---------Night----------
unsigned long nightHour = 23 * uHr;
unsigned long nightMinute = 30 * uMn;

//----------------------------END ADJUSTABLE TIMES

I had also made these changes to my declared variables, would renaming these help to clear up the confusion?

So, startSunriseHour now contains a number of seconds. Don't you see that as confusing?

Why not create a function, secondsSinceMidnight(), that converts hours and minutes to seconds since midnight, and call that function with variables that contain hours and minutes?

Then, the existing (or previous) code can be easily modified to use the more sensible seconds since midnight values.

You got me here passing variables through functions still has me baffled at the moment. Going to do a little research and see what i can come up with and i'll post back, hopefully sooner than later.

Delta_G:
I don't often write code for you, but I will here because it may cause a eureka moment on how a function works.

Many thanks, i facepalmed after seeing this.

This is what i've got now

byte stopAfterOne;      //added to stop checkDayCycle();
unsigned long ts;
uint32_t old_ts;
unsigned long sRise;
unsigned long dLight;
unsigned long sSet;
unsigned long mRise;
unsigned long mLight;
unsigned long mSet;
unsigned long nTime;

Renamed these for ease of reading

//-------Sunrise---------
byte sunriseHour = 9;
byte sunriseMinute = 1;
//-------Daylight-------------
byte dayHour = 9;
byte dayMinute = 30;
//-------Sunset----------
byte sunsetHour = 18;
byte sunsetMinute = 0;
//-------Moonrise--------
byte moonriseHour = 18;
byte moonriseMinute = 30;
//---------Moonlight----------
byte moonLightHour = 19;
byte moonLightMinute = 1;
//---------Moonset--------
byte moonsetHour = 23;
byte moonsetMinute = 1;
//---------Night----------
byte nightHour = 23;
byte nightMinute = 30;

in loop

  sRise = secondsSinceMidnight(sunriseHour, sunriseMinute);
  dLight = secondsSinceMidnight(dayHour, dayMinute);
  sSet = secondsSinceMidnight(sunsetHour, sunsetMinute);
  mRise = secondsSinceMidnight(moonriseHour, moonriseMinute);
  mLight = secondsSinceMidnight(moonLightHour, moonLightMinute);
  mSet = secondsSinceMidnight(moonsetHour, moonsetMinute);
  nTime = secondsSinceMidnight(nightHour, nightMinute);

  checkDayCycle();         //moved from setup because above not declared until loop and im also using this to display the LED phase to a 16 x 2 LCD
void checkDayCycle() {

  if (stopAfterOne < 1) {
    ts = RTC.get() % 86400;              //Gets seconds since midnight
    if (old_ts == 0 || old_ts != ts) {   //Gets current seconds since midnight
      old_ts = ts;

      if (ts >= sRise && ts <= dLight) {
        Serial.println("Sunrise Works");
        startMorningFade = true;
      }
      else if (ts >= dLight && ts <= sSet)
      {
        Serial.println("Day Works");
        analogWrite(dayLed, 255);
      }
      else if (ts >= sSet && ts <= mRise)
      {
        Serial.println("Sunset Works");
        startEveningFade = true;
      }
      else if (ts >= mRise && ts <= mLight)
      {
        Serial.println("Moonrise Works");
        startMoonFade = true;
      }
      else if (ts >= mLight && ts <= mSet)
      {
        Serial.println("Moon Light Works");
        analogWrite(moonLed, 255);
      }
      else if (ts >= mSet && ts <= nTime)
      {
        Serial.println("Moonset Works");
        endMoonFade = true;
      }
      else if (ts >= nTime)
      {
        Serial.println("Night Works");
        analogWrite(dayLed, 0);
        analogWrite(moonLed, 0);
      }
    }
    stopAfterOne++;
  }
}

Adding another full download with the LCD. Leds use the same code as startup, but the time converts from 24 to 12hr and displays AM and PM - though this may be able to done easier/better as well.

PaulS thanks for all the time and effort you have put into looking at this with me. It is greatly appreciated.

Forgot the attachments.

AquariumController_2.0.ino (5.96 KB)

It_Works.ino (5.05 KB)

in loop

Why? Are the values in sRise, dLight, etc. going to change on every pass through loop()? If not, setup() is the place to assign values to them.

I really do not like ts as a variable name. It doesn't tell me anything. currTSM (current time since midnight) would.

    if (old_ts == 0 || old_ts != ts) {   //Gets current seconds since midnight

That is STILL NOT what the statement is doing.

PaulS thanks for all the time and effort you have put into looking at this with me. It is greatly appreciated.

I was hoping that you would create the secondsSinceMidnight() function, and use it. But, sometimes, it just takes getting a peek at some simple, completed, function to make one say "Oh, that's what he was driving at". It's all good.

PaulS:
Why? Are the values in sRise, dLight, etc. going to change on every pass through loop()? If not, setup() is the place to assign values to them.

I see, moved back to setup and run checkDayCycle () after they are declared.

I really do not like ts as a variable name. It doesn't tell me anything. currTSM (current time since midnight) would.
I will make these changes after work today, I will also try to add in plenty of comments on everything.

    if (old_ts == 0 || old_ts != ts) {   //Gets current seconds since midnight

has been removed, I was using this with the Serial clock print before adding the 1 seconds millis timer. With checkDayCycle it's only run once anyways.

That is STILL NOT what the statement is doing.
I was hoping that you would create the secondsSinceMidnight() function, and use it. But, sometimes, it just takes getting a peek at some simple, completed, function to make one say "Oh, that's what he was driving at". It's all good.

To add to the fun my ds18b20 temp sensor and 4 ch relay should be in tomorrow, the ds18b20 will check the temp of my aquarium water and I will use the temp to control the relay's that will run the heaters.

Ok renamed ts to currTSM for checkDayCycle(); and lcdClockDisplay().

void checkDayCycle() {
  currTSM = RTC.get() % 86400;        //Gets seconds since midnight
  if (currTSM >= sRise && currTSM <= dLight) {
    Serial.println("Sunrise Works");
    startMorningFade = true;
  }
  else if (currTSM >= dLight && currTSM <= sSet)
  {
    Serial.println("Day Works");
    analogWrite(dayLed, 255);
  }
  else if (currTSM >= sSet && currTSM <= mRise)
  {
    Serial.println("Sunset Works");
    startEveningFade = true;
  }
  else if (currTSM >= mRise && currTSM <= mLight)
  {
    Serial.println("Moonrise Works");
    startMoonFade = true;
  }
  else if (currTSM >= mLight && currTSM <= mSet)
  {
    Serial.println("Moon Light Works");
    analogWrite(moonLed, 255);
  }
  else if (currTSM >= mSet && currTSM <= nTime)
  {
    Serial.println("Moonset Works");
    endMoonFade = true;
  }
  else if (currTSM >= nTime)
  {
    Serial.println("Night Works");
    analogWrite(dayLed, 0);
    analogWrite(moonLed, 0);
  }
}

for the LCD

void lcdClockDisplay() {
  currTSMLcd = RTC.get() % 86400;              //Gets seconds since midnight

  lcd.clear();
  lcd.setCursor(0, 0);
  if (hour() < 10 && hour() != 0) {
    lcd.print("0");
  }
  if (hour() > 12) {
    hour() - 12;
    lcd.print(hour() - 12);
  } else if (hour() == 0) {
    lcd.print("12");
  } else {
    lcd.print(hour());
  }

  lcd.print(":");
  if (minute() < 10) {
    lcd.print("0");
  }
  lcd.print(minute());
  lcd.print(":");
  if (second() < 10) {
    lcd.print("0");
  }
  lcd.print(second());
  lcd.print(" ");
  if (hour() > 12) {
    lcd.print("PM");
  } else {
    lcd.print("AM");
  }
  lcd.setCursor(0, 1);
  lcd.print("LED'S = ");

  if (currTSMLcd >= sRise && currTSMLcd <= dLight) {
    lcd.print("SUNRISE");
  }
  else if (currTSMLcd >= dLight && currTSMLcd <= sSet)
  {
    lcd.print("DAY");
  }
  else if (currTSMLcd >= sSet && currTSMLcd <= mRise)
  {
    lcd.print("SUNSET");
  }
  else if (currTSMLcd >= mRise && currTSMLcd <= mLight)
  {
    lcd.print("MOONRISE");
  }
  else if (currTSMLcd >= mLight && currTSMLcd <= mSet)
  {
    lcd.print("MOON");
  }
  else if (currTSMLcd >= mSet && currTSMLcd <= nTime)
  {
    lcd.print("MOONSET");
  }
  else
  {
    lcd.print("NIGHT");
  }
}

I left the arduino running this morning before leaving for work and had it printing to serial the time and also would print eg: "dayLed Faded Up" as it was going through the led fade functions. 4Hrs 4mins and 6 seconds into the program the serial started printing:

dayLed Faded Up
�129:15:37 12 20 2016
�1233338

it froze at
�129:19:36 12 20 2016
�1233577

the ? on the serial monitor is a square.
My 16x2 LCD is also froze with a bunch of gibberish letters, numbers, and symbols.

I'm not sure what could be causing this, the only thing i see as maybe a possibility was i used digitalWrite(dayLed, HIGH) in my sunrise in loop(), but i don't see how that would affect the LCD.
Could this be the issue or are there other issues i need to address.

After a little testing so far i can get the error when any of the leds are fading and i am printing to the lcd, currently testing to see if i can reproduce the error when only fading the leds.

I am wondering if maybe it's not code related and i'm drawing to many mA from the arduino..... running 16x2 lcd with pot and resistor, both leds currently 1 fading w resistor when error happens, and ds3231 rtc, also have a lm35dz wired in but removed the code when testing.

While writing this serial has quit printing dayLed Faded Down while the led is still lit.....

I am wondering if maybe it's not code related

I suspect that it might be code related. At least, make one more change and run another test.

You have a LOT of Serial.print() and lcd.print() statements like:
Serial.println("Moon Light Works");

Use the F() macro to keep the string literals out of SRAM.

Serial.println(F("Moon Light Works"));

Ok I will give this a run tonight, I did find a bug in my fade sequences in loop where at some point I had used dayLed and dayBrightness together. I also made some changes to my fade function not using the global dayBrightness variable for counting anymore. This seems to have debugged the led issues I was having and I was no longer able to reproduce the serial and lcd bug. I will add the f macro and post the changes I've made tonight after work when I have access to my computer again.

Thanks again

After 2 hours, 2 fade cycles, and the changes you suggested everything is still working fine.

The changes i made are in loop fade's - used the pin value instead of the pin numbers.

  if (startMorningFade && dayBrightness <= 255) {
    if (fadeLed - prevFadeLed >= fadeInterval) {
      prevFadeLed = fadeLed;
      sunrise();
      Serial.println(F("dayLed Faded Up"));
      dayBrightness++;
    } else if (dayBrightness >= 255) {
      startMorningFade = false;
      analogWrite(dayLed, 255);
    }
  }

and in the functions - a local variable for counting.

void sunrise() {              //Fade in dayLed value counts up from 0 to 255
  int x = 0;
  if (x <= 255) {
    x++;
    analogWrite(dayLed, dayBrightness);
  }
}

void sunset() {                //Fade out dayLed value counts down from 255 to 0
  int x = 255;
  if (x >= 0) {
    x--;
    analogWrite(dayLed, dayBrightness);
  }
}

void moonrise() {              //Fade in moonLed value counts up from 0 to 255
  int y = 0;
  if (y <= 255) {
    y++;
    analogWrite(moonLed, moonBrightness);
  }
}

void moonset() {                //Fade out moonLed value counts down from 255 to 0
  int y = 255;
  if (y >= 0) {
    y--;
    analogWrite(moonLed, moonBrightness);
  }
}
void sunrise() {              //Fade in dayLed value counts up from 0 to 255
  int x = 0;
  if (x <= 255) {
    x++;
    analogWrite(dayLed, dayBrightness);
  }
}

Every time this function is called, x will be set to 0. 0 is always going to be less than 255, so x, which is not used again, will be set to 1 and the analogWrite() function will be called.

So, the result is no different from:

void sunrise()
{              //Fade in dayLed value counts up from 0 to 255
    analogWrite(dayLed, dayBrightness);
}

The other functions have equally useless constructs.

So at this point calling a function to just analogWrite is pointless right?

To me now it would make more sense to just do it here in loop now.

  if (startMorningFade && dayBrightness <= 255) {
    if (fadeLed - prevFadeLed >= fadeInterval) {
      prevFadeLed = fadeLed;
      analogWrite(dayLed, dayBrightness);
      Serial.println(F("dayLed Faded Up"));
      dayBrightness++;
    } else if (dayBrightness >= 255) {
      startMorningFade = false;
      analogWrite(dayLed, 255);
    }
  }

Then even loosing dayBrightness++; and just using analogWrite(dayLed, dayBrightness++); would work. Would this be a better way of doing this?

  if (startMorningFade && dayBrightness <= 255) {
    if (fadeLed - prevFadeLed >= fadeInterval) {
      prevFadeLed = fadeLed;
      analogWrite(dayLed, dayBrightness++);
      Serial.println(F("dayLed Faded Up"));
    } else if (dayBrightness >= 255) {
      startMorningFade = false;
      analogWrite(dayLed, 255);
    }
  }