code not compiling... can't figure out why DS18B20

Hi everybody, this is my first Arduino project and I’m attempting to start by just modifying someone else’s code, but am having some issues right out of the gate.

Please understand that I have very limited background in this area and my only real programming knowledge is a very basic understanding of VBA.

The part that’s giving me grief is related to getting the temperature readings from a ds18b20 temperature sensor. I’ve spent hours searching the internet and have found several different code examples that will compile, but they’re so similar that I can’t actually figure out the differences that are responsible for some compiling while others won’t.

I’ll post the complete code as i have it so far at the end of this post for reference, but to start specifically the exact error I’m getting is “exit status 1_ ‘temp’ was not declared in this scope” and this is the section of code that it errors out on…

// if hot, turn on cooling fans
 
void doCoopHVACCool() {
 
  float temperature = getTemp();                    // create temperature variable
  //float tempF = (temperature * 9.0) / 5.0 + 32.0;   // convert celcius to fahrenheit
 
 
  if ((unsigned long)(millis() - lastTempCheckTime) > TempCheckDelay) {    // check temperature every 10 minutes
    lastTempCheckTime = millis();
 
    if (temp >= 30) {                                      // if temp rises above 30C turn on cooling fan(s) relay
      digitalWrite(relayFan, HIGH);
    }
 
    else if (temp < 30) {
      digitalWrite(relayFan, LOW);
    }
    if (SerialDisplay) {
      Serial.print(" Coop Temperature:");             // print out coop temperature
[color=pink]      Serial.println(temp);                          // print out the temperature
[/color]      Serial.println(" Coop Exhaust is on");           // print out Coop Exhaust is on
    }
  }
}

The “getTemp” function is almost a carbon copy of most of the examples I’ve found online about how to read from these sensors and is in the full code below… as far as I understand it, the getTemp code is like a function that is called or executed from other processes or functions rather than just looping through and executing repetitively. I would like to have 2 of these sensors hooked up and I have found their ROM numbers, but can’t get the code to compile. One code example that does compile and I was hoping would provide some indication as to what changes I may need to make is…

#include <OneWire.h>
  
int DS18S20_Pin = 2; //DS18S20 Signal pin on digital 2
  
//Temperature chip i/o
OneWire ds(DS18S20_Pin);  // on digital pin 2
  
void setup(void) {
  Serial.begin(9600);
}
  
void loop(void) {
  float temperature = getTemp();
  Serial.println(temperature);
    
  delay(100); //just here to slow down the output so it is easier to read
    
}
  
  
float getTemp(){
  //returns the temperature from one DS18S20 in DEG Celsius
  
  byte data[12];
  byte addr[8];
  
  if ( !ds.search(addr)) {
      //no more sensors on chain, reset search
      ds.reset_search();
      return -1000;
  }
  
  if ( OneWire::crc8( addr, 7) != addr[7]) {
      Serial.println("CRC is not valid!");
      return -1000;
  }
  
  if ( addr[0] != 0x10 && addr[0] != 0x28) {
      Serial.print("Device is not recognized");
      return -1000;
  }
  
  ds.reset();
  ds.select(addr);
  ds.write(0x44,1); // start conversion, with parasite power on at the end
  
  byte present = ds.reset();
  ds.select(addr);    
  ds.write(0xBE); // Read Scratchpad
  
    
  for (int i = 0; i < 9; i++) { // we need 9 bytes
    data[i] = ds.read();
  }
    
  ds.reset_search();
    
  byte MSB = data[1];
  byte LSB = data[0];
  
  float tempRead = ((MSB << 8) | LSB); //using two's compliment
  float TemperatureSum = tempRead / 16;
    
  return TemperatureSum;
    
}

I figure the difference must be in the DS18S20 vs the DS18B20, and I’ve read the information on the Arduino Playground, but either i’m doing it wrong, don’t understand it or both…

Anyways, I’ve pretty much tried everything that is within my limited understanding of things to try and I’m stumped. I’m really hoping someone here can either point me in the right direction or explain it to me in a way that makes sense :slight_smile:

The full code that I am currently in the process of working on will be in the following post as i’m getting the too many characters error as I’m trying to post this… you’ll also probably be able to spot some of my failed attempts that i’ve just commented out… :confused: I’ve tried to highlight the error line in pink in this code as well as the first code section above…

Any assistance would be greatly appreciated!

Thanks,
Joe

ok so i guess it’s still too many characters… I’ve tried attaching it to this post…

Chicken_Coop_Mod1.ino (20.9 KB)

Your variable in the first code that you posted is called 'temperature', not 'temp'.

You can scroll through the output window; the first error is shown first and will probably point to this line ;)

    if (temp >= 30) {                                      // if temp rises above 30C turn on cooling fan(s) relay

The IDE is a bit stupid and often highlights the last error

Your full error message is probably like

C:\Users\sterretje\Documents\Arduino\sketch_apr27a\sketch_apr27a.ino: In function 'void doCoopHVACCool()':

sketch_apr27a:31: error: 'temp' was not declared in this scope

     if (temp >= 30) {                                      // if temp rises above 30C turn on cooling fan(s) relay

         ^

sketch_apr27a:40: error: 'temp' was not declared in this scope

       Serial.println(temp);                          // print out the temperature

                      ^

exit status 1
'temp' was not declared in this scope

First learn how to read the DS18B20 before including anything else. The tutorial from Hacktronix is good.

Once you’ve fixed the problem with temp/temperature there are two more to handle.

This may not convert properly:

  float tempRead = ((MSB << 8) | LSB); //using two's compliment

The number should first be stored in a 16-bit signed integer just to be sure.

  int16_t tempRead = ((MSB << 8) | LSB); //using two's compliment

Then you can convert it to float.

The other problem is that you must delay an appropriate amount of time between starting the temperature conversion and reading the result. For a 12-bit precision result (default) you must delay at least 850 milliseconds. Fewer bits of precision require less delay. See the datasheet.

  ds.write(0x44,1); // start conversion, with parasite power on at the end
  delay(850);

Pete

hey guys, thanks for the info! :slight_smile:

So i finally got the two temperature probes working and displaying on the LCD! ended up using a bit of a different code than the original, but it seems to work so far. I figured the next step would be easy, which was to get the photoresistor up and running and displaying on the LCD as well, but this has proven to be much more of a challenge than I’d anticipated… In the original code that I’m trying to modify, the photoresitor readings are segregated into bands for “dark”, “twighlight” and “light”, but all i’m getting on my display is “0”

I’ve attempted to separate out the photocell code and seem to have it working by itslef on the serial monitor, but not with the LCD… not sure where my problem is here… One thing I’ve noticed that i’ve never seen before is the code: if(SerialDisplay)…

So for example in the code below…

if (SerialDisplay) {
        Serial.println(" Photocel Reading Level:");
        Serial.println(" - Dark");

does this mean if the display is the serial monitor then print “Photocell Reading Level: - Dark”?

or is this looking for an argument from SerialDisplay? …(assuming this is some sort of temporary memory notepad or something?)

I’ve found a similar bit of code on the ardafruit site that also denotes rages of cell readings into descriptive bands, but in this case, this is a function that is supposed to return numeric values and corresponding alphanumeric values for a range of light levels so that the other functions can utilize the numeric values in their processes and the alphanumeric labels descriptions such as “dark” or “twighlight” are displayed on the LDC screen.

It’s not critical to have both, but since it was already in the code i’m borrowing, i thought it would be a neat feature to keep.

If anyone can tell me why my [photocellrReadingLevel] is displaying as “0” and not “Light”, “Twighlight” or “Dark”, it would be much appreciated. The original full version of the code was attached my my second post and I’ve attached the current sketch I’m working on, which is just the working temp probes with the LCD and the not working photocell code. I’ve also attached another sketch where I seem to have the photocell code working on the serial monitor. It seems like I’ve tried a million different things and obviously this code doesn’t show everything I’ve tried, but I’m stumped and this is where I’m at. :o

one other question… before I started the attempt to add the photocell, the working code did not contain [Serial.begin(9600);] I added this in an attempt to get the photocell code working, but not sure if maybe I’ve overlooked something else related to this?

Finally, the following is just an example of how the photocell readings will be utilized by another function in the program… just including it for reference :slight_smile:

// do the coop door
void doCoopDoor() {
  if (photocellReadingLevel  == '1') {              // if it's dark
    if (photocellReadingLevel != '2') {             // if it's not twilight
      if (photocellReadingLevel != '3') {           // if it's not light
        debounceTopReedSwitch();                    // read and debounce the switches
        debounceBottomReedSwitch();
        closeCoopDoorMotorB();                      // close the door
      }
    }
  }
  if (photocellReadingLevel  == '3') {              // if it's light
    if (photocellReadingLevel != '2') {             // if it's not twilight
      if (photocellReadingLevel != '1') {           // if it's not dark
        debounceTopReedSwitch();                    // read and debounce the switches
        debounceBottomReedSwitch();
        openCoopDoorMotorB();                       // Open the door
      }
    }
  }
}

EDIT: I just realized after i posted that i have my light level readings and descriptions backwards… but everything else still applies… I changed my <= to >= :confused:

As always, any wisdom you can share is greatly appreciated!

Thanks,
Joe

LCD_with_TempProbes___Photocell.ino (5.9 KB)

PhotoCell_only.ino (2.51 KB)

This snippet (there are at least two like this) caught my eye.

  if (photocellReadingLevel  == '3') {              // if it's light
    if (photocellReadingLevel != '2') {             // if it's not twilight
      if (photocellReadingLevel != '1') {           // if it's not dark

If the photocellReadinglevel equals '3', it will never be equal to '2' or '1'; the second and third 'if' always result in true and therefore can just as well be left out.

For the SerialDisplay, this is near the top of your code

// print debug messages or not to serial
const boolean SerialDisplay = true;

It's simply a flag; if true, send debug data over serial port, if false, don't send debug data over serial port.

I'm not quite sure about your light/dark/twilight question. Is that this piece of code (from the attached file)?

  lcd.setCursor(0,2); //Start at character 0 on line 2
  lcd.print("Light Level: ");
  lcd.print(photocellReadingLevel);  // "Dark", "Twilight", or "Light"

If so, your levels are just characters, not strings like Dark, Twilight and Light. Look at the code that does the serial printing; it prints the strings based on value of photocellReadingLevel, not the value itself.

This is an alternative that achieves what you want (or what I think that you want) to achieve

lcd.print(photocellReadingLevel=='1'?"Dark":photocellReadingLevel=='2'?"Twilight":"Light");

See it as a shortform for if / else if /else. If you don't understand it, just use if / else if / else

Thanks sterretje, I tried your suggestion and it works on the serial monitor, but still doesn't seem to display correctly on the LCD. It displays "Light" on the LCD regardless of the sensor reading, which is a step up from "0" :)

I'm not sure if the values 1, 2 and 3 are being passed through, but i'm assuming if it's displaying properly on the Serial Monitor then it probably is :)

anyways, the reason for the thresholds is to operate a motor, which opens a door when it's light, closes it when it's dark and ignores twighlight so that the doors don't open at dusk and close at dawn. So the numerical values are passed to the motor function and the light level description is displayed on the LCD.

hopefully that explains a little bit better what the desired result is.

thanks in advance!

cheers, Joe

With your old code, did it display 1, 2 and 3 as expected?

running the code “LCD_with_TempProbes__Photocell” :on the lcd it would display “Light Level: 0” no matter what, now it displays “Light Level: Light” no matter what.

Running the code “PhotoCell_only” on the serial monitor i get the following:

Photocell Analog Reading =
958
Photocell Reading Level:

  • Dark
    Photocell Analog Reading =
    953
    Photocell Reading Level:
  • Light
    Photocell Analog Reading =
    506

So it does change when i cover the sensor and displays the description correctly on the serial monitor and i’d just copied this code into the code with my temp probes and LCD so there shouldn’t be any difference… except for the instruction to print to LCD

I’m not sure if i’m telling it to display on the lcd correctly… should this command be in the function that reads the sensor or outside of it?

I’ve reattached the two sketches again as they are now…

thanks,
joe

PhotoCell_only.ino (2.68 KB)

LCD_with_TempProbes___Photocell.ino (5.92 KB)

One other question... in the source code i'm borrowing, the temperature variable is defined as... [int temperature = getTemp();]

However in the code I have working at the moment, I have excluded this variable definition and it seems to be working as expected... is this because the variable "tempC" is already included in the DallasTemperature library?

For example in my current working code...

[float tempC = sensors.getTempC(deviceAddress);] and [float outsidetempC = sensors.getTempC(outsideThermometer);]

both seem to work without the "int" definition... is this a rookie "bad practice"? or am I overlooking something by not including it as in the original? or is this "int" definition redundant?

Thanks, joe

In your combined code, you never call doReadPhotoCell(); so the value of photocellReadingLevel will always be zero ;)

With regards to reply #10: I'm not sure what you're asking.

'float' and 'int' are different types of variables. If getTemp() returns an int, you basically should use an int; 'converting' to float does not add anything (and wastes two bytes in the scarce arduino memory). If getTemp() returns a float, you should basically use a float. You can also 'cast' to an int if you don't need the accuracy. I don't know if you posted a link to the library and sensors that you're using; please post (again) and I will have a look to see if I can advise.

Ok, so I’ve made some pretty good progress over the last couple of days! :relaxed: It’s probably not perfect, but relays are clicking, lights are blinking, sensors are sensing and motors are turning!

I do still have a few questions thought that I’m hoping you can help me out with…

  1. With regards to the question of float vs int, I think sterretje, you’ve confirmed what I was thinking that float and int are two different types of variables.

sterretje:
With regards to reply #10: I’m not sure what you’re asking.

‘float’ and ‘int’ are different types of variables. If getTemp() returns an int, you basically should use an int; ‘converting’ to float does not add anything (and wastes two bytes in the scarce arduino memory). If getTemp() returns a float, you should basically use a float. You can also ‘cast’ to an int if you don’t need the accuracy. I don’t know if you posted a link to the library and sensors that you’re using; please post (again) and I will have a look to see if I can advise.

I am using the DS18B20 temperature probes with the ONEWIRE and DALLASTEMPERATURE libraries…

If I’m not mistaken, every instance in this sketch that uses or calls for the tempC variable or value is utilizing float, for example…

void displayTemperature(DeviceAddress deviceAddress)
{

float tempC = sensors.getTempC(deviceAddress);

   if (tempC == -127.00) // Measurement failed or no device found
   {
    lcd.print("Temperature Error");
   } 
   else
   {
   lcd.print(tempC);
   lcd.print(" C");

   }
}// End printTemperature

So defining the variable as int would be pointless, but should it be defined at the beginning in the variables section as float? or is the definition of variable type for this variable completely unnecessary?

  1. With regards to the photocell, I found another thread here… http://forum.arduino.cc/index.php?topic=328761.15 where someone else was trying to modify the same code I’m working with and was also having some issues with the photocell part. I’ve attempted to replicate his solution and it seems to work, but I’m not sure if I’ve left the door open for problems down the road as the solution involved removing some code, which sterretje had initially pointed out as suspect; (see below)
 if (photocellReadingLevel  == '3') {              // if it's light
    if (photocellReadingLevel != '2') {             // if it's not twilight
      if (photocellReadingLevel != '1') {           // if it's not dark

but was created for the following purpose…

dhnaves:
howdy,

if interested, the reason i did this was because during the twighlight hour, the photocel reading were jumping all over the place (very quickly) which caused the door motor to jump back and forth (sometimes jumping past the reed switches and then locking up tight in the wrong direction)

so instead of smoothing the readings, i just made sure the reading was a ~consistent~ value of before actually triggering the door motor. i’m sure there are more/better ways to do it, but this has been working flawlessly now for quite some time.

thanks again for all of the input,

cheers,
//d

Can anyone see any issues with my revisions in the attached code?

And finally,

  1. The following function controls a light that turns on during “Twighlight” levels if the door status is “open” and then turns off when the door status changes to “closed”.
//  turn on interior lights at dusk and turn off after door shuts

void doCoopInteriorLightDusk() {

  if ((unsigned long)(millis() - lastTwilightTime) > TwilightDelay) {     // delay 5 mins

    lastTwilightTime = millis();


 //   doReadPhotoCell();
    bottomSwitchPinVal = digitalRead(bottomSwitchPin);
    if (bottomSwitchPinVal == 1) {                          // if bottom reed switch circuit is open (door is open)
      if (photocellReadingLevel = '2') {                         // if it's twilight
     //   if (photocellReading <= 100) {
          digitalWrite (relayInteriorLight, LOW);          // turn on interior light relay
          if (SerialDisplay) {
            Serial.println(" Interior Light: On");
          }
        }
      }
    }
    else if (bottomSwitchPinVal == 0) {                    // if bottom reed switch circuit is closed (door is closed)
      digitalWrite (relayInteriorLight, HIGH);              // turn off? interior light relay
      if (SerialDisplay) {
        Serial.println(" Interior Light: Off");
      }
    }
  }

I was wondering if it’s possible to use the delay function with this to check the light levels again in 12 hours and turn the light on again if it’s “Dark” and then off again when it’s “Light”? Or would it make more sense to create a separate function for this and if so, how would I initiate the delay from the last “Twighlight” reading level? Would something like this require the addition of a realtime clock? Also, I haven’t hooked up a light yet to test, but do the relay LOW and HIGH values look mixed up to anyone else with respect to on and off?

Thanks again for all your help!
Cheers,
Joe

totally unrelated, but does anyone know why i don’t get notified of replies even though i have checked “Notify me of replies”?

retest_daves_code.ino (20 KB)

Comparing floats with a single hard-coded value is in general a bad idea. Floats are never 100% accurate. It might work in this case (because it seems to be an error condition), but in general one would use a range.

if(tempC <= -126.00)
{
}

or

if(tempC < -126 && tempC > -128)
{
}

Please provide a link to the dallastemperature library.

Basically you should declare variables where you need them. If you only need them in one function, declare them in that function. Of you need them in multiple functions, declare them global (outside any function). If you declare them global, there is a risk that you change them by accident due to an error / typo in your code.

tempC is only used in displayTemperature() so it should be there and not global.

photocellReadingLevel is used in multiple functions; you set it in readPhotoCell() and read it in multiple other functions. So I would keep that global (as it is).

photocellReading is used in two functions so global is more-or-less OK. Personally I would change it slightly but you can keep it as is.

Also be aware that global variables permanently occupy memory. If you make tempC and watertempC global, it will always use 2x 4 bytes, even if not used at that moment. As it is now, you call doWaterHeat() and use 4 bytes for watertempC. When doWaterHeat() is finished the four bytes are ‘released’. Next you call displayTemperature() and use 4 bytes for tempC. Once displayTemeperature() is finished, those 4 bytes are ‘released’. So instead of using 8 bytes for those two variables, you only occasionally use 4 bytes.

Probably ,ore than you wanted to know :wink:

I see that AWOL passed the same comment in that thread as I did now. Even after the comment that you quoted it does not make sense; it probably does not apply to the exact section. I copied the latest code in that thread (reply #15) and it does not contain the below code.

 if (photocellReadingLevel  == '3') {              // if it's light
    if (photocellReadingLevel != '2') {             // if it's not twilight
      if (photocellReadingLevel != '1') {           // if it's not dark

I’m not sure if I understand; the ‘main’ condition is the photocellReadingLevel. That will automatically happen in N hours when you enter Twilight again (so it will happen both when going from light to dark and from dark to light).
And you should NOT use delay(). If will prevent any code from being executed for the delay period.

If you explain it differently, I might understand what you want and can think about it.

Thanks sterretje, the tip about using a range for float makes sense and the explanation regarding the scope of variables and their declaration was also very helpful!

yes Awol had identified the same issue that you had, and I used the solution posted in that thread (#15) to modify my code. I quoted the original code’s author to explain his intended function of the original code in case the solution in post (#15) and now in my revised code presented any potential issues that I’m overlooking. I’m assuming that if you like the solution from #15 then it’s probably all good!

Here’s a link to the DallasTemperature library on GitHub… is this the link you asked for or am I not understanding what you’re looking for? (again, first project, new to everything… libraries are a new concept, sorry if I’m missing it)

DallasTemperature link

With regards to the question 3, sorry for the poor explanation, I’ll try to be more clear…

So the current function (see previous post) operates a light that comes on at dusk if the coop door is open and shuts off after another function closes the door.

My question was; in addition to the above function (light on at dusk…) I would also like the light to turn back on again 12 hrs later (in the morning) IF it is still dark outside. The reason being that in the winter we have shorter days (I live up north), and by turning the lights on for a few hours in the morning before the sun comes up to artificially create a longer day, we get more eggs :slight_smile: Since I don’t have a Real Time Clock, I was wondering if there was a function like delay that I could use, but now that I realize delay would delay the processing of all the other functions as well, I understand this is not the solution… so I’m guessing the only way to do this is to get and incorporate a real time clock, and use it as a timer to countdown 12 hrs after dusk to turn the light back on and then turn it off again when the sun comes up. Does this make sense?

Thanks,
Joe

You want the light to go on for e.g. 2 hours, 12 hours after a transition from light to dark.

So your condition is a timing that is started by the ‘twilight’ condition (you can implement this 12 hours using millis()) and the previous (or current? not sure) light level must be dark. I think that you must start keeping track of the light level so the system can determine if the ‘time’ is ‘evening’ (light to dark transition) or ‘morning’ (still dark or dark to light transistion).

As you don’t have conditions (if statements) in your loop, it looks like you need a new function that implements this functionality.

If you're interested, I'm working on a fake RTC (for an Uno); it simulates a RTC in software. It's not complete yet and it will obviously loose the date/time after a power failure; it's currently missing a function for this.

Ok I think I get it! so if I were to use something like the following… (assuming I’ve defined TwighlightDelay12 as 12 hrs)

  //  turn on interior lights at dusk and turn off after door shuts

void doCoopInteriorLightDusk() {

  if ((unsigned long)(millis() - lastTwilightTime) > TwilightDelay) {     // delay 5 mins

    lastTwilightTime = millis();

    bottomSwitchPinVal = digitalRead(bottomSwitchPin);
    if (bottomSwitchPinVal == 1) {                          // if bottom reed switch circuit is open (door is open)
      if (photocellReadingLevel <=> '1') {                         // if it's not dark

          digitalWrite (relayInteriorLight, HIGH);          // turn off interior light relay
          if (SerialDisplay) {
            Serial.println(" Interior Light: OFF");
          }
        }
      }
    }
 
 
    else if (bottomSwitchPinVal == 1) {                          // if bottom reed switch circuit is open (door is open)
      if (photocellReadingLevel = '2') {                         // if it's twilight
   
          digitalWrite (relayInteriorLight, LOW);          // turn on interior light relay
          if (SerialDisplay) {
            Serial.println(" Interior Light: On");
          }
        }
      }
    }
    else if (bottomSwitchPinVal == 0) {                    // if bottom reed switch circuit is closed (door is closed)
      digitalWrite (relayInteriorLight, HIGH);              // turn off? interior light relay
      if (SerialDisplay) {
        Serial.println(" Interior Light: Off");
      }
    }
  }

  if ((unsigned long)(millis() - lastTwilightTime) > TwilightDelay12hrs) {     // delay 12 hrs

    lastTwilightTime = millis();

    bottomSwitchPinVal = digitalRead(bottomSwitchPin);
    if (bottomSwitchPinVal == 0) {                          // if bottom reed switch circuit is closed (door is closed)
      if (photocellReadingLevel = '1') {                         // if it's Dark
     //   if (photocellReading <= 100) {
          digitalWrite (relayInteriorLight, LOW);          // turn on interior light relay
          if (SerialDisplay) {
            Serial.println(" Interior Light: On");
          }
        }
      }
    }

Then it would check the light levels and door status again in 12 hrs and turn on the light if the door is closed and it’s dark out. After this the original code would continue to run on its original 5 min delay until the initial conditions were met (dusk and door open turns on light; next check when door’s closed, turns light off) before this last section was called again? or would this second delay mess up the first one? (ie. prevent the check every 5 min and pause it for another 12 hrs?)

Thanks,
Joe

You have some misplaced curlies by the looks of it. I tried to fix it, but might have done it wrong.

void doCoopInteriorLightDusk() {

    if ((unsigned long)(millis() - lastTwilightTime) > TwilightDelay) 
    { // delay 5 mins

        lastTwilightTime = millis();

        bottomSwitchPinVal = digitalRead(bottomSwitchPin);
        if (bottomSwitchPinVal == 1)
        { // if bottom reed switch circuit is open (door is open)
            if (photocellReadingLevel <=> '1')
            { // if it's not dark

                digitalWrite (relayInteriorLight, HIGH);          // turn off interior light relay
                if (SerialDisplay)
                {
                    Serial.println(" Interior Light: OFF");
                }
            }
        }
    }
    else if (bottomSwitchPinVal == 1)
    { // if bottom reed switch circuit is open (door is open)
        if (photocellReadingLevel = '2')
        { // if it's twilight
            digitalWrite (relayInteriorLight, LOW);          // turn on interior light relay
            if (SerialDisplay)
            {
                Serial.println(" Interior Light: On");
            }
        }
    }
    else if (bottomSwitchPinVal == 0)
    { // if bottom reed switch circuit is closed (door is closed)
        digitalWrite (relayInteriorLight, HIGH);              // turn off? interior light relay
        if (SerialDisplay)
        {
            Serial.println(" Interior Light: Off");
        }
    }

    if ((unsigned long)(millis() - lastTwilightTime) > TwilightDelay12hrs)
    { // delay 12 hrs
        lastTwilightTime = millis();

        bottomSwitchPinVal = digitalRead(bottomSwitchPin);
        if (bottomSwitchPinVal == 0)
        { // if bottom reed switch circuit is closed (door is closed)
            if (photocellReadingLevel = '1')
            { // if it's Dark
                //   if (photocellReading <= 100) {
                digitalWrite (relayInteriorLight, LOW);          // turn on interior light relay
                if (SerialDisplay)
                {
                    Serial.println(" Interior Light: On");
                }
            }
        }
    }
}

I don’t know if the above is what you intended. If so, you will have a problem.

    if ((unsigned long)(millis() - lastTwilightTime) > TwilightDelay) 
    { // delay 5 mins

        lastTwilightTime = millis();

The last line is executed every 5 minutes. So the below will never be true

    if ((unsigned long)(millis() - lastTwilightTime) > TwilightDelay12hrs)
    { // delay 12 hrs

You will have to rethink your strategy :wink: A lastTwilightTime12 variable will be one step. I can’t say for sure if the actions for each ‘timing’ will interfere with each other.

You also have some mistakes; <=> should be != and if (photocellReadingLevel = ‘2’) will assign a value to photocellReadingLevel instead of comparing.

There are some improvements to be made in general.

Values like ‘1’, ‘2’ and ‘3’ for the photocellReadingLevel are quite meaningless; you have to remember what each one means (OK, it’s basically documented using a comment). You can use #define to create meaningful names.

#define DARK '1'
#define TWILIGHT '2'
#define LIGHT '3'

Basically place this at the top of your file. Next you can replace all ‘1’, ‘2’ and ‘3’ on your code by their respective names.

E.g.

//  set photocel threshholds
void readPhotoCell() {
  if (photocellReading >= 900) {
    photocellReadingLevel = '1';

can change to

//  set photocel threshholds
void readPhotoCell() {
  if (photocellReading >= 900) {
    photocellReadingLevel = DARK;

And a line like

      if (photocellReadingLevel == '2') {                         // if it's twilight

can change to

      if (photocellReadingLevel == 'TWILIGHT') {                         // if it's twilight

The same for the status of the door contacts but you might have to think a little about that as you have opposite states for the top switch and the bottom switch.

Thanks Sterretje! That is awesome!!! I will totally redefine those states, and I also noticed the opposite states for the switches and I’m not sure if that’s how it’s supposed be or if that’s an error… I guess I’ll find out when I assemble everything and see if it works as expected or not. Also, I figured the delays might interfere with eachother… I guess i’ll have to put in an RTC, and just add come code for the light to turn on at 6 am if it’s not already light outside… probably the easiest way to go…

One other question about the switches that you may know… do I need to put in pull up (or down? how do you tell if it’s up or down anyway? just depending which side of the switch it’s on?) resistors in addition to enabling the Arduino’s built in pull up resistors? I’ve attached a drawing of how I was planning to hook them up, but I’ve tested them a little bit this way and they almost seem to work better without the resistors, so I got to thinking, maybe connecting them this way is for when you’re not enabling the internal pull ups?

Thanks again,
Joe

reed swich diag.jpg