Timer turns on twice a day instead of once.

Projects Programming
1

So i have gotten myself stuck. i have an LED light fixture with 6 channels that my arduino is controlling. Im using a ds3231 RTC. My set times and fading seems to be functioning fine, except one thing. i have my lights set to turn on at 2pm and off at 11pm. but when midnight rolls around it turns itself back on. im not sure what time they turn off but when i get up in the mornings about 7 they are back off again.

If someone could take the time to browse my code and see if they can spot my error, i would be much obliged. As far as i can tell, this should only be coming on between my setpoints.

#include "Adafruit_PWMServoDriver.h"
#include <TimeLib.h>                   // https://github.com/PaulStoffregen/Time
#include <Wire.h>
#include <DS3232RTC.h>                 // https://github.com/JChristensen/DS3232RTC
#include "RTClib.h"

Adafruit_PWMServoDriver pwm = Adafruit_PWMServoDriver();          // Initialize PWM Generator object
RTC_DS3231 rtc;                                                   // initialize Real time clock object

int currentBright = 0;             // Tracks current brightness
int targetBright = 0;              // What is our target to fade to
int maxBright = 75;                // Maximum Value of brightness - TODO: Map() fades to max brightness as 100%

// Light Variables
unsigned long currentTime;                 // current time in millis
unsigned long prevFadeTime = 0;            // Pevious event millis
unsigned long timeBetweenFade = 250;       // How long to wait between fade steps

time_t timeOn;                    // Time to turn light on
time_t timeOff;                   // Time to turn light Off

bool lightIsOn = false;           // Track whether light is on or not

void setup() {
  // put your setup code here, to run once:
  Serial.begin(9600);         // Start serial
  Wire.setClock(400000);
  if (! rtc.begin()) {                      // try to startup RTC
    Serial.println("Couldn't find RTC");
    while (1);                              // if you cant find it, fix and wait for reset
  }
  if (rtc.lostPower()) {                    // check if battery died on RTC, if so lets set it to something
    Serial.println("RTC lost power, lets set the time!");

    // following line sets the RTC to the date & time this sketch was compiled
    rtc.adjust(DateTime(F(__DATE__), F(__TIME__)));

    // This line sets the RTC with an explicit date & time, for example to set
    // January 21, 2014 at 3am you would call:
    //rtc.adjust(DateTime(2019, 4, 30, 23, 52, 0));
  }

  setSyncProvider(RTC.get);         //Sets our time keeper as the RTC
  // setTime(RTC.get);              // Sets system time to RTC Time
  setSyncInterval(5);               // number of seconds to go before requesting re-sync
  if (timeStatus() != timeSet)
    Serial.println("Unable to sync with the RTC");
  else
    Serial.println("RTC has set the system time");

  pwm.begin();
  pwm.setOutputMode(false);           // Set PWM Driver to open Drain for LEDs
  pwm.setPWMFreq(400);                // Set PWM Hz to reduce buzzing
  for (int i = 0; i < 6; i++) {
    pwm.setPin(i, currentBright, 0); // turn all channels off
  }
  timeOn = 1565532000;        // Time to turn this channel on
  timeOff = 1565564400;        // Time to turn this channel off
}

void loop() {
  // put your main code here, to run repeatedly:
  currentTime = millis();                     // Update current time
  unsigned long SSM = getSSM(now());          // Find out how many seconds since midnight it has been
  Serial.print("Seconds Since Midnight");
//  Serial.println(SSM);
//  Serial.println(getSSM(timeOn));
//  Serial.println(getSSM(timeOff));

  // during light on times
  if ((SSM >= getSSM(timeOn)) && (SSM < getSSM(timeOff))) {

    // Serial.println(F("Turning Light On"));
    targetBright = maxBright;               // Set light On
  } else {
    // Serial.println(F("Turning Light Off"));
    targetBright = 0;                       // Set light off
  }

  // only allow setBrightness to run occasionally to lengthen the fade
  if (currentTime >= prevFadeTime + timeBetweenFade) {
    //Serial.println(F("Lets go set Brightness"));
    setBrightness();
  }

}

// Set brightness, then auto adjust brightness.
// use current brightness and target brightness and
//   increment steps one by on with a non-blocking
//   delay to adjust until we are at our target level

void setBrightness () {

  if (currentBright != targetBright) {          // See if we need to adjust brightness
    if (targetBright > currentBright) {         // Are we ramping up
      if (currentBright < 4096) {               // Make sure we havent exceeded bounds
        currentBright++;                        // Lets get brighter

      }
    } else if (targetBright < currentBright) {  // Are we ramping down
      if (currentBright > 0) {                  // Make sure we havent exceeded bounds
        currentBright--;                        // Lets get dimmer
      }
    }

    prevFadeTime = currentTime;                 // Reset timer to wait for next fade step



    for (int i = 0; i < 6; i++) {                   // Lets apply the brightness to all pins
      //Serial.print("Adjusting Brightness to ");
      //Serial.println(currentBright);
      pwm.setPin(i, (currentBright * 40), 0);
      //Serial.print("On Channel ");
      //Serial.println(i);

    }
  }
}


unsigned long getSSM (time_t getTime) {         // Calculate seconds since midnight for timers
  unsigned long timeSecs = (hour(getTime) * 60 * 60);
  timeSecs += (minute(getTime) * 60);
  timeSecs += second(getTime);

//  static unsigned long previousTime = timeSecs;
//  if (timeSecs != previousTime) {
//    Serial.print("Seconds Since Midnight: ");
//    Serial.println(timeSecs);
//    previousTime != timeSecs;
//  }
  return timeSecs;
}
2 
  unsigned long timeSecs = (hour(getTime) * 60 * 60);

Even though the final result of the multiplication will be an unsigned long, the calculation will be done using ints. (This is just one of many traps you can fall into when programming Arduino/C++).

Instead try:

  unsigned long timeSecs = (hour(getTime) * 60UL * 60UL);

in which the UL tells the compiler "This number is supposed to be an unsigned long integer."

3

Why all the trouble of storing the on and off times as time_t to only convert it to some sort of seconds from midnight thing? Why not compare it directly?

And why have the used times in the middle of the code and not nice and easy where you defined the variables?

But:

unsigned long timeSecs = (hour(getTime) * 60 * 60);

hour() returns an int, 60 is a int literal aka math is done in int. But 23 * 60 * 60 is larger than an int can hold. Force math to an unsigned long:

unsigned long timeSecs = (hour(getTime) * 60UL * 60);
4

if (currentTime >= prevFadeTime + timeBetweenFade)

Use subtraction:

if (currentTime - prevFadeTime >= timeBetweenFade)

5

Thanks for the quick replies! That make sense! I was expecting the math to be UL since the final product was a UL.

@septillion I started with time_t for everything but was running into problems and it was recommended to use minutes since midnight, i need a resolution of seconds however for other sensors i plan to add, so I went with seconds since midnight. I still need to go through and do some house keeping. :wink:

I will make the changes mentioned above and report back! Thanks again guys

6

Not that it's a bad thing to use time since midnight (although I think a nice function would make it nicer) but why start with a time_t still? Why not define (at the top!) the switch times in time since midnight?

const unsigned long SecondsPerHour = 60UL * 60;
const unsigned long SecondsPerMinuut = 60;
const unsigned long OnTime = 23 * SecondsPerHour;
const unsigned long OffTime = 1 * SecondsPerHour + 30 * SecondsPerMinuut;
7

Ok so i made the proposed changes and all seems well. i have hooked it back up to the light and haven't noticed any erratic behavior. Thanks so much for everyone's help!

@larryd Sorry, i missed your comment earlier. good catch on the millis overflow. got that corrected now

@septillion I used time_t because i will be receiving a UNIX timestamp from an android app via Bluetooth, which is how i will set the RTC time. i wanted to get the rest of the project up and running first though. Its looking like it will be easier to use UL for most of my arduino side and then just use the timestamp to set the RTC though. Was this what you meant by making a function out of it? or did i misunderstand what you meant.

I have this declared at the top:

const unsigned long SecondsPerHour = 60UL * 60;
const unsigned long SecondsPerMinute = 60;

Then i added this function:

unsigned long getTimeInSeconds(int hours, int minutes) {
  return ((hours * SecondsPerHour) + (minutes * SecondsPerMinute));
}