multi function chicken coop sketch - all but 1 function works (door) nsw?

howdy,

still a bit of a noob, but i have my automated chicken coop sketch working except for the door open/close function “doCoopDoor();” (which is odd, b/c that particular sketch works fine when it is separated out from the main (multi-function sketch)

within the main sketch i have:

  • thermometer
  • heat
  • fan
  • servo for web cam
  • lcd
  • coop door

would really appreciate someone taking a look and letting me know why “doCoopDoor();” works on its own but not when embedded the main sketch. (both attached below)

also, i’m fairly certain that while i have most of the functionality working within my sketches, (and have studied/read quite a bit) it’s probably done with ~newby/lengthy~ code? if anyone might be able to critique some of my noob-like habits/code, i’d really appreciate it. i’m very interested in becoming better at this and when further experienced, helping others & sharing.

thanks,
//dave

  1. the main chicken coop sketch: (with the coop door function included , but currently that function does not work)
  2. the coop door function alone (which currently works)
    ~both attached~

naves_family_chicken_coop_controller.ino (16.7 KB)

L298N_Motor_Controller_10_6_photocel_millis.ino (8.63 KB)

This is odd:

    photocellReadingRange = 'Dark';

Those single quotes should enclose a single character - you're creating some weird multi-byte character - the compiler accepts it because it has to be able to process unicode.

I don't think this is your issue, but consider replacing that and the others with a single character, enum, const int or a #define'd constant.

After that, I'd comment out all the calls in loop except the one that controls the door and start throwing in debugging serial.prints in there to get some more clues about what's going on.

This sort of thing should be an if/else, not two ifs:

    if (topSwitchPinVal == 0) {                        // check if top switch cicuit is closed
      digitalWrite(ledCoopDoor, LOW);                  // turn coop door led on
      Serial.println("circuit and door are open - danger"); // display "circuit and door are open - danger"
  
    }  
    if (topSwitchPinVal == 1){                         // check if bottom switch cicuit is open
      digitalWrite(ledCoopDoor, HIGH);                 // turn coop door led on
      Serial.println("circuit and door are closed - no danger"); // display "circuit and door are closed - no danger"
     }

I’m disconcerted by the amount of global data you have. It is not at all obvious what they’re each used for, hard to be sure that you’re using the right one in each place and that each one has a sensible lifecycle. This would IMO be easier to follow if you used local statics for data that were only used within a given function. LoopDurationDebounceTopReedSwitch is a good example of a variable that doesn’t need to be global.

The way you’re doing time comparisons is not rollover-safe. Instead of this:

if(currentTime >= (loopDurationDebounceTopReedSwitch + 10)

It would be better to do this, which is logically equivalent but does handle rollover correctly:

if(currentTime - loopDurationDebounceTopReedSwitch  >= 10)

Your variable loopDurationDebounceTopReedSwitch should use enumerated values, not multi-byte character constants. For example:

typedef enum { DARK, DUSK, LIGHT, BRIGHT } LightLevel;
photocellReadingRange = DUSK;

If you made readPhotoCel() a function that returned the current range, the global variable photocellReadingRange wouldn’t be needed at all.

I would suggest applying smoothing to the raw value before returning it.

Since a variable can only have one value at a time, one of these clauses is redundant (similar in several other places):

if (photocellReadingRange == 'Dark' && photocellReadingRange != 'Dusk')

These functions to open aqnd close the coop door would IMO make more sense as a finite state machine. You would have a door state variable (OPEN, CLOSING, CLOSED, OPENING) and a switch statement to detect events relevant to the current state:

void handleDoor()
{
    static int doorState = OPENING; // initial value

    switch (doorState)
    {
        case OPEN:
            if(getLightLevel() < DUSK_THRESHOLD)
            {
                closeDoor();
                doorState = CLOSING;
            }
            break;
        case CLOSING:
            if(isDoorClosed())
            {
                stopDoor();
                doorState = CLOSED;
            }
            break;
        case CLOSED:
            if(getLightLevel() > DAWN_THRESHOLD)
            {
                openDoor();
                doorState = OPENING;
            }
            break;
 }

You get the idea. This way needs no global data and is completely decoupled from anything else that is going on.

thanks for the replies… much appreciated.

wildbill:
This is odd:

    photocellReadingRange = 'Dark';

Those single quotes should enclose a single character - you’re creating some weird multi-byte character - the compiler accepts it because it has to be able to process unicode.

I don’t think this is your issue, but consider replacing that and the others with a single character, enum, const int or a #define’d constant.

After that, I’d comment out all the calls in loop except the one that controls the door and start throwing in debugging serial.prints in there to get some more clues about what’s going on.

ah, ok…pretty sure i understand now. i was getting errors when i didn’t have quotes around those characters. and since i’m still learning about the aforementioned replacements… i was just trying anything that might work. i now have my homework to re-read about data types.

and yeah, good call with commenting out the calls. not really sure how to add debugging serial.prints (except to add what i have back in 1 by 1? or maybe that’s exactly what you’re saying i should do?) with all of the serial printing i currently have going on, i do know that only a few actually appear in the serial monitor when it’s all running.

thanks again, wild bill!


PeterH:
peter h…

This sort of thing should be an if/else, not two ifs:

    if (topSwitchPinVal == 0) {                        // check if top switch cicuit is closed

digitalWrite(ledCoopDoor, LOW);                  // turn coop door led on
      Serial.println(“circuit and door are open - danger”); // display “circuit and door are open - danger”
 
    } 
    if (topSwitchPinVal == 1){                        // check if bottom switch cicuit is open
      digitalWrite(ledCoopDoor, HIGH);                // turn coop door led on
      Serial.println(“circuit and door are closed - no danger”); // display “circuit and door are closed - no danger”
    }



*** ah right. a "duh" on my part.

I'm disconcerted by the amount of global data you have. It is not at all obvious what they're each used for, hard to be sure that you're using the right one in each place and that each one has a sensible lifecycle. This would IMO be easier to follow if you used local statics for data that were only used within a given function. LoopDurationDebounceTopReedSwitch is a good example of a variable that doesn't need to be global.

***ahhh, right... didn't really need to make those global. bad habit of mine! (a waste of ram, yes?)

The way you're doing time comparisons is not rollover-safe. Instead of this:


if(currentTime >= (loopDurationDebounceTopReedSwitch + 10)






It would be better to do this, which is logically equivalent but does handle rollover correctly:


if(currentTime - loopDurationDebounceTopReedSwitch  >= 10)



***oh man! i've been really concerned about rollover (i was taking it step by step just to get everything actually working before dealing with that) and you just saved me a ton of time by posting. thank you!

Your variable loopDurationDebounceTopReedSwitch should use enumerated values, not multi-byte character constants. For example:



typedef enum { DARK, DUSK, LIGHT, BRIGHT } LightLevel;
photocellReadingRange = DUSK;




***i *almost* understand your example... (haven't yet studied typedef nor enum ) 

If you made readPhotoCel() a function that returned the current range, the global variable photocellReadingRange wouldn't be needed at all.

*** makes complete sense. i was having trouble with photocell numerical values coming back and jerking the door motor back and forth around dusk and dawn, so it was my beginner's attempt at creating ranges. (which you've just again made far simpler for me)

I would suggest applying smoothing to the raw value before returning it.

Since a variable can only have one value at a time, one of these clauses is redundant (similar in several other places):


if (photocellReadingRange == ‘Dark’ && photocellReadingRange != ‘Dusk’)




*** yeah, this again was me trying to tell the coop door motor to only trigger the motor when it was dark or light enough (b/c the door was bouncing around (and then jumping past the reed switches) pretty sure part of my problem was that i didn't need to read the photocell nearly as often. after what i've witnessed sitting there during twilight, i really could read the thing every 15 minutes, which would bypass the problem of the photocell reading dark/dusk/dark/dusk. (which begs the question: what is the preferred method of posting 15 minutes? i.e. "900000" or is that lame and inefficient?

These functions to open and close the coop door would IMO make more sense as a finite state machine. You would have a door state variable (OPEN, CLOSING, CLOSED, OPENING) and a switch statement to detect events relevant to the current state:



void handleDoor()
{
    static int doorState = OPENING; // initial value

switch (doorState)
    {
        case OPEN:
            if(getLightLevel() < DUSK_THRESHOLD)
            {
                closeDoor();
                doorState = CLOSING;
            }
            break;
        case CLOSING:
            if(isDoorClosed())
            {
                stopDoor();
                doorState = CLOSED;
            }
            break;
        case CLOSED:
            if(getLightLevel() > DAWN_THRESHOLD)
            {
                openDoor();
                doorState = OPENING;
            }
            break;
}




***dang.. very cool! and you probably just wrote that in 60 seconds or less, too. =) much appreciated! 

You get the idea. This way needs no global data and is completely decoupled from anything else that is going on.

*** yes sir... got it. pulling my head out of the need for globals.

thanks again, peterh!

There's nothing wrong with defining "fifteen minutes" as 900000 milliseconds, but if I were you I wouldn't take readings that infrequently. It would be better IMO to take readings more often (perhaps once per minute, although more often wouldn't hurt) and then smooth them out to get rid of transients. Otherwise there's a risk that a shadow passing over the sensor around dusk will make the door shut prematurely, and then open again at the next sample. There's a really easy smoothing algorithm which just takes a weighted average of the previous and new samples:

smoothed = ((9 * smoothed) + newReading) / 10;

Assuming you have a function that returns the light level, I'd put the logic to decide whether it's time to read the sensor again, and to apply smoothing to the raw value, in that function. (That's what I called getLightLevel() in my snippet earlier.)

PeterH: There's nothing wrong with defining "fifteen minutes" as 900000 milliseconds, but if I were you I wouldn't take readings that infrequently. It would be better IMO to take readings more often (perhaps once per minute, although more often wouldn't hurt) and then smooth them out to get rid of transients. Otherwise there's a risk that a shadow passing over the sensor around dusk will make the door shut prematurely, and then open again at the next sample. There's a really easy smoothing algorithm which just takes a weighted average of the previous and new samples:

smoothed = ((9 * smoothed) + newReading) / 10;

Assuming you have a function that returns the light level, I'd put the logic to decide whether it's time to read the sensor again, and to apply smoothing to the raw value, in that function. (That's what I called getLightLevel() in my snippet earlier.)

more frequent readings... gotcha. makes sense.

"smoothing"... very handy~ obviously what i needed. man, this forum (and helpful folk like yourself) is invaluable. pretty sure i never would've known about smoothing for quite some time.

so i just visited: http://arduino.cc/en/Tutorial/Smoothing and have an 'ok' understanding of what the author is getting across ... but you mentioned "There's a really easy smoothing algorithm." ~smoothed~ is that algorithm? (part of core arduino?) or did you create that as an example? i'm also having difficulty understanding how exactly to apply smoothing into a new getLightLevel().

also (9 * smoothed) what is the 9? the / 10 is to avg 10 readings, yes?

if you'd rather i study more on the subject before asking what i'm sure are simple questions, i understand.

thanks again!

The smoothing algorithm is called a decaying average. The snippet I posted is intended to show the general concept. Here's a slight variation to make the weighting more obvious:

smoothed = ((9 * smoothed) + (1 * newReading)) / 10;

You can see that the new smoothed value is a weighted average of the previous smoothed value, and the new reading. The weightings can be any values you like - the more weight you put on the previous smoothed value the slower it will respond to new readings - just make sure that you divide by the sum of the weights afterwards i.e. 9+1=10 so it divides by 10. If you perform this weighted average for each new reading then the resulting smoothed value produces a decaying average where older samples are less significant and newer ones are more significant. It's much easier than keeping a list of samples and re-averaging them every time a new one arrives.

ahhhhhhhhhhh... makes perfect sense.. thanks for breaking it down.

cheers!

since i’ve had more than a few fellow chicken coop/arduino enthusiasts msg me and ask how this project was going, i thought i’d update this post with solutions and resources.

i finally got the door working exactly as i wanted it. works flawlessly and not hard to understand.

key points:

  • i found a cool timer library “simpleTimer” which, after dealing with too many millis functions, this allowed me to have 1 ref and then call it with different timings
  • after testing many different methods, i chose to check the photocel readings every ten minutes, then check the reed switches and only then turn on/off/stop the door motor. (this was also helpful in dealing with chickens that didn’t come in right away and got locked out. (ps. i’ve found that it’s better to NOT lure the girls in with a light as the ambient light shines outside the coop and keeps them outside longer)

anyway, the final code attached and fritzing diagram linked below.
all i ask is that if your use any of my code or ideas, that you share your progress with me and the rest of the world - i’m big on sharing -hey, it’s open source… makes the world a better place, yes? =)

fritzing diagram:

note:
i’ve been building the coop (el pollo palace) on and off now for about a year and have just started documenting everything on my personal site. (just been very busy with work) it’s a work in progress with much left to do, but please take a look and let me know what you think. (there’s a whole section on what i did with the door - vids, pics, code & diagram)

:slight_smile:

here’s a link to the coop door project: (pics, vids, diagram, parts, & code)

here’s a link to the main coop project (again - much more to document, but maybe it’ll give some ideas)

thanks again for those forum members who helped me here!

cheers,
//d

doCoopDoor.ino (8.53 KB)

finally found the img embed tool. =) The Fritzing Wiring Diagram for the Automatic Chicken Coop Door

cheers!

Hello I've started this thread and still am looking for some help. Can you please help find a door solution for my coop? Here's the link to the thread ----> http://forum.arduino.cc/index.php?topic=311018.0

Thank you