First project - Garage Door Closer - Would love feedback

Hi All,

I think I’ve just completed my first Arduino project. It’s a simple thing that just makes sure the garage door gets closed. I’d love any feedback people may have on how the code may be better written so that I’ll establish the best coding habits. Also, would it make sense to use sleep to save some power? I think I’ve read that the board will continue using power even if the chip is put into sleep.

/*
  Garage Door Closer
 
Detects when the garage door is open and starts a timer.
When the time elapses, it briefly powers a relay.  
The closed relay contacts are the equivalent of pressing the door switch, closing the door.
The program pauses while the door closes, then returns to detecting door open.
A "Hold Open" button allows the user to increase the timer to 60 minutes.
An LED lights when the timer is active.
Closes the garage door when the time has elapsed.
A buzzer sounds briefly when the door is one minute from closing.

 */
 

// constants won't change. They're used here to 
// set pin numbers:
const int buttonPin = 2;           // the number of the pushbutton pin
const int ledPin =  13;            // the number of the LED pin
const int buzzer = 8;              // the number of the buzzer pin
const int doorSensor = 7;          // the number of the door sensor pin. When closed, the sensor circuit is open.
const int doorTrigger = 4;         // the number of the pin used to power the door 
const int extendedDelay = 3600000; // the amount of time door should stay open after pressing the Hold Open button (3600000)
const int normalDelay = 300000;    // the normal delay time (300000)

// variables will change:
int buttonState = 0;         // variable for reading the pushbutton status
int doorState = 0;           // variable for reading the door's open/close status
long closeTime = 0;           // holds the time at which the door will be closed
                                       // based on millis plus the desired close duration
boolean timerOn = false;    // This variable is for determining whether the timer should be read or ignored
int i = 0;                 // Used for counting buzzer loops



void setup() {
  // initialize the LED pin as an output:
  pinMode(ledPin, OUTPUT);  
  pinMode(doorTrigger, OUTPUT);
  pinMode(buzzer, OUTPUT);
  
  // initialize the pushbutton pin and door senros as inputs:
  pinMode(buttonPin, INPUT);  
  pinMode(doorSensor, INPUT);  
  digitalWrite(doorSensor, HIGH);
  
   //Debug stuff
  Serial.begin(9600);
}

void loop(){
  // Get the button status
  buttonState = digitalRead(buttonPin);
  // Get the door sensor status
  doorState = digitalRead(doorSensor);
 
 
   //if door is closed...
  if(doorState == HIGH) {
    
    //Turn the timer light out
    digitalWrite(ledPin, LOW);
    //Turn off the timer
    timerOn = false;
    
    //Debug
    Serial.println("Door is closed.");
  }
  
  // check if the pushbutton is pressed and the door is open...
  else if (buttonState == HIGH && doorState == LOW) { 
    
    //Debug stuff
    Serial.println("Button Pressed - setting timer");
    Serial.println(buttonState);
     
     // Set timer
    closeTime = millis() + extendedDelay;
    timerOn = true;
    Serial.println("Close time:");
    Serial.println(closeTime);
    Serial.println("millis:");
    Serial.println(millis());
  } 
  
  
  
  // If the timer is active and there is time remaining... 
  else if ( timerOn == true && closeTime > millis() )  {         
 
    //Turn LED on to indicate that the timer is active
    digitalWrite(ledPin, HIGH);
  
    //Debug stuff
    int timeTilClose = closeTime - millis();
    Serial.println(timeTilClose);
    Serial.println("Timer is active");
    
    //If there is less than a minute remaining, buzz the buzzer
    if (closeTime - millis() <= 1000) {
      i = 4;
      do
      {
      digitalWrite(buzzer HIGH);
      delay(100);
      digitalWrite (buzzer, LOW);
      delay(100);
      i = i-1;
      } while (i > 0);
    }      
  }
  
  // If the door is open and the time has elapsed...
  else if ( closeTime < millis() && doorState == LOW && timerOn == true ) {
    
        
    //Debug stuff
    Serial.println("Door open and time elapsed - closing door");
  
    //Turn off the LED since not waiting for timer now
    digitalWrite(ledPin, LOW);
    
    //Trigger  the door switch - power the doorTrigger for a bit of time
    digitalWrite(doorTrigger, HIGH);
    delay(200);
    digitalWrite(doorTrigger, LOW);
    
    //Turn the timer off
    timerOn = false;
    
    //now wait long enough to ensure that the door has finished closing
    delay(10000);
    
    //Debug
    Serial.println("Door should be closed now.");

  }
 
 //  if the door is open and the timer isn't on, it's time to set the timer
  else if (timerOn == false && doorState == LOW) {
    
    //Set the timer to the normal delay time
    closeTime = millis() + normalDelay; 
    timerOn = true;
    
    //Debug stuff
    Serial.println("Setting timer");
    Serial.println(doorState);
    Serial.println(buttonState);
  }
  
else 
//Debug
Serial.println("No arguments were true.");
}

You're using int variables to hold values that are too big for that type. For variables holding time values, unsigned long is usually the appropriate type to use.

Your time comparisons are logically correct in abstract terms but don't handle timer rollover correctly. The suggested way to handle timer overflow when determining whether an interval has elapsed is to subtract the previous time from the current time and compare to the target interval, e.g.:

if(millis() - startMillis > interval)
{
   // the interval has elapsed ...

To implement that throughout would require quite widespread but simple changes to the code dealing with time.

The logic isn't very complex but you do have several state variables as well as the time variables. I think you'll find that the code would be simpler and the control algorithm would be easier to follow if you implemented it as a finite state machine with states such as

CLOSED, WAITING_TO_CLOSE, CLOSING

Using a switch/case statement to handle each state, you can then tests for the events relevant to the current state and also do any timed actions within each state such as blinking LEDs or beeping.

Imagining myself using this, I think I'd prefer the warning beeps to be repeated several times with increasing urgency until eventually the auto close triggered.

I like your coding style. Easy to read, easy to follow, well commented.

I agree with PeterH about the unsigned longs with your timers. Your delay constants are delcared as constant ints, but they’re bigger than 65536, which is the max even an unsigned int can hold. Also, he is correct about rollovers. You might get some strange behavior in 49 days or so?

Imagining myself using this, I think I’d prefer the warning beeps to be repeated several times with increasing urgency until eventually the auto close triggered.

You might get your wish. Well, not the increasing urgency part, but as I read the code, this will happen continuously for one second before the door closes.

    //If there is less than a minute remaining, buzz the buzzer
     if (closeTime - millis() <= 1000) {

Except that 1000 is only one second and not one minute. You might want to use 60000L.

Peter, Tan,

Thanks very much for the detailed feedback. It's extremely helpful. I will make the changes you've suggested.

I also would like to achieve something similar to yours project.

1 magnetic sensor on the door and arduino ethernet shield (control door via web)

I measured the voltage that is now in the button that opens the box and is about 5v.

But I have a question!

  • that relay did you use? If i active the relay stays "on" always
  • how did you get connected? I guess there is a button that opens the garage. is connected in parallel?

Thank thommango
Good project!!!!!!!

:slight_smile: :slight_smile: :slight_smile:

Hi Zeno,

I haven't played with the garage door hardware yet, but here's my theory:

There are two leads from the door opener to a wall-mounted switch. From what I've read, there should be a 16v signal when these leads form a circuit. My plan is to connect the relay to these leads in parallel. I will do some tests to confirm that this will work, of course. And if there's something else needed to activate the door, I will go back to the drawing board.

Cheers,

Thom