Can I do better

This is my first real project in Arduino, I have played with other peoples code, and modified it, But this my first attempt at writing something myself so be nice.
I programed apple in elementary and middle school, But that is so different I have no idea what i’m doing.

I want to know if I made any head slap mistakes. This code works but I want to learn.

code checks to see if garage door is open at a set time, 9PM, if it is open it hits a relay and closes it.

LCD displays date and time in 12hr format, and open / closed status.

2 leds light also for open / closed status.

#include <Wire.h>
#include "RTClib.h"
#include <LiquidCrystal.h>
RTC_DS1307 rtc;

const int rs = 12, en = 11, d4 = 5, d5 = 4, d6 = 3, d7 = 2;
LiquidCrystal lcd(rs, en, d4, d5, d6, d7);

void setup () {
  

  if (! rtc.begin()) {
    Serial.println("Couldn't find RTC");
    while (1);
  }
  //rtc.adjust(DateTime(__DATE__, __TIME__));    // SET TIME AND DATE: YYYY,MM,DD,HH,MM,SS
  //rtc.adjust(DateTime (2019,12,31,11,59,50));   // test set

  delay(1000);
  pinMode (9,OUTPUT);  // set door relay
  pinMode (8,INPUT);   // set door switch
  pinMode (6,OUTPUT);  // led open
  pinMode (7,OUTPUT);  // led closed
  lcd.begin (16,2);

}
 
void loop ()

{
    int p;
    p=digitalRead(8);  // read door switch
    DateTime now = rtc.now(); // if time target is hit write pin 9 high to triger relay
    if (now.hour() == 21 && now.minute() == 0 && now.second() == 1 && p == 1) // target time
    {
    
    digitalWrite (9,HIGH);  // hit relay high for a bit
    delay (800);
    digitalWrite (9,LOW);
    }
    { 
    int tim = now.hour (); 
    if (tim > 12) tim=tim-12;  // 12 hour time
    if (tim==0) tim=12;        // check if hour = 0
    if (now.month ()< 10)  // write date time to lcd inital 
    // inital is here because without it display was all wrong
    // until secconds rolled over to 0
    { 
    lcd.setCursor (0, 0);
    lcd.print ("0");
    }  
    lcd.print(now.month(), DEC);
    lcd.print('/');
    if (now.day ()< 10)
    { 
    lcd.setCursor (3, 0);
    lcd.print ("0");
    }
    lcd.print(now.day(), DEC);
    lcd.print('/');
    lcd.print(now.year(), DEC);
    lcd.setCursor(0, 1);
    if (tim < 10)
    { 
    lcd.setCursor (0, 1);
    lcd.print ("0");
    }
    
    lcd.print(tim);
    
    lcd.print(':');
    if (now.minute ()< 10)
    { 
    lcd.setCursor (3, 1);
    lcd.print ("0");
    }
    if (now.minute ()< 10)
    { 
    lcd.setCursor (3, 1);
    lcd.print ("0");
    }
    lcd.print(now.minute(), DEC);
    lcd.print(':'); 
}
    
    // write date time to lcd looped
   
    int tim = now.hour (); 
    if (tim > 12) tim=tim-12;  // 12 hour time
    if (tim==0) tim=12;        // check if hour = 0
    if (now.month ()< 10)  // write date time to lcd inital 
    { 
    lcd.setCursor (0, 0);
    lcd.print ("0");
    }  
    lcd.print(now.month(), DEC);
    lcd.print('/');
    if (now.day ()< 10)
    { 
    lcd.setCursor (3, 0);
    lcd.print ("0");
    }
    lcd.print(now.day(), DEC);
    lcd.print('/');
    lcd.print(now.year(), DEC);
    lcd.setCursor(0, 1);
    if (tim < 10)
    { 
    lcd.setCursor (0, 1);
    lcd.print ("0");
    }
    
    lcd.print(tim);
    
    lcd.print(':');
    if (now.minute ()< 10)
    { 
    lcd.setCursor (3, 1);
    lcd.print ("0");
    }
    if (now.minute ()< 10)
    { 
    lcd.setCursor (3, 1);
    lcd.print ("0");
    lcd.print(now.minute(), DEC);
    lcd.print(':'); 
    }
   {
    lcd.setCursor(6, 1);
    if (now.second ()< 10)
    { 
    lcd.setCursor (6, 1);
    lcd.print ("0");
    }
    lcd.print(now.second(), DEC);
    
    lcd.setCursor(9, 1); // check door condition 
    if (p == 1){
      lcd.print (" OPEN  ");  // print lcd open and set open led on
      digitalWrite (7,HIGH);
      digitalWrite (6,LOW);
    }
    if (p == 0) {
      lcd.print (" CLOSED");  // print lcd closed and set closed led on
      digitalWrite (7,LOW);
      digitalWrite (6,HIGH);
    }
    }



    
    delay(100);  
}

welcome to the forums. Thanks for posting your code using code tags!

A couple of comments:

  1. hard to tell if you have a pull-up resistor (or pull-down) on your switch. You will need one since you are not using the internal pull-up resistors

  2. name your pins using constants rather than have random numbers scattered throughout your code.

  3. You have the same code in there twice and several of you if statements (for leading 0s) positioned the cursor but what happens if that if is false? Usually you don’t need those.

  4. Auto format your code (Ctrl-T) and it will line everything up and make it easier to read

#include <Wire.h>
#include "RTClib.h"
#include <LiquidCrystal.h>
RTC_DS1307 rtc;

const int rs = 12, en = 11, d4 = 5, d5 = 4, d6 = 3, d7 = 2;
LiquidCrystal lcd(rs, en, d4, d5, d6, d7);

const int doorRelayPin = 9;
const int doorSwitchPin = 8;
const int ledOpenPin = 6;
const int ledClosedPin = 7;

void setup ()
{
  if (! rtc.begin()) {
    Serial.println("Couldn't find RTC");
    while (1);
  }
  //rtc.adjust(DateTime(__DATE__, __TIME__));    // SET TIME AND DATE: YYYY,MM,DD,HH,MM,SS
  //rtc.adjust(DateTime (2019,12,31,11,59,50));   // test set

  delay(1000);
  pinMode (doorRelayPin, OUTPUT); // set door relay
  pinMode (doorSwitchPin, INPUT);  // set door switch
  pinMode (ledOpenPin, OUTPUT); // led open
  pinMode (ledClosedPin, OUTPUT); // led closed
  lcd.begin (16, 2);
}

DateTime lastTime;

void loop ()
{
  bool doorSwitch = digitalRead(doorSwitchPin); // read door switch
  DateTime now = rtc.now(); // if time target is hit write pin 9 high to triger relay
  if (now.hour() == 21 && now.minute() == 0 && now.second() == 1 && doorSwitch == true) // target time
  {
    digitalWrite (doorRelayPin, HIGH); // hit relay high for a bit
    delay(800);
    digitalWrite (doorRelayPin, LOW);
    delay(200); // wait for more than 1 second so target time is not hit again
  }

  // write date time to lcd if it has changed
  if ( lastTime != now ) {
    lastTime = now;
    int tim = now.hour();
    if (tim > 12) tim = tim - 12; // 12 hour time
    if (tim == 0) tim = 12;    // check if hour = 0
    lcd.setCursor (0, 0);
    if (now.month () < 10) lcd.print ("0");
    lcd.print(now.month(), DEC);
    lcd.print('/');
    if (now.day () < 10) lcd.print ("0");
    lcd.print(now.day(), DEC);
    lcd.print('/');
    lcd.print(now.year(), DEC);
    lcd.setCursor(0, 1);
    if (tim < 10) lcd.print ("0");
    lcd.print(tim);
    lcd.print(':');
    if (now.minute() < 10) lcd.print ("0");
    lcd.print(now.minute(), DEC);
    lcd.print(':');
    if (now.second() < 10) lcd.print ("0");
    lcd.print(now.second(), DEC);

    lcd.setCursor(9, 1); // check door condition
    if (doorSwitch == true) {
      lcd.print (" OPEN  ");  // print lcd open and set open led on
      digitalWrite (ledOpenPin, HIGH);
      digitalWrite (ledClosedPin, LOW);
    } else {
      lcd.print (" CLOSED");  // print lcd closed and set closed led on
      digitalWrite (ledOpenPin, LOW);
      digitalWrite (ledClosedPin, HIGH);
    }
  }
}

I'm learning a ton just comparing these side by side.

Thank you for your version of the code, Looks wonderful.

A few more input:

Would be good to set (or read) status of pins in the setup

Pin are better as const byte rather than const int (but it does not matter too much since the optimizer will get rid of those)

digitalRead() does not return a bool but HIGH or LOW... as you depend on pull-down, ideally if you want to keep a book write it as bool doorSwitch =  (digitalRead(doorSwitchPin) == HIGH);thus way you get a real bool (type consistency) and you express if you have a pull-up (or pull down).

There is no need to write == true for Boolean

If you are very unlucky you might miss 21:00:01, would be best comparing if you are after 21:00 rather than an exact moment (and of course remember if you had closed the door or check it’s status)

No need to explicitly write , DEC in your prints, this is the default value.

You can use the F() macro in your print to save some SRAM (and when print just one char as the leading 0, it is faster and uses less memory to write one char as ‘0’ rather than the cString “0”

J-M-L:
A few more input:

Would be good to set (or read) status of pins in the setup

Pin are better as const byte rather than const int (but it does not matter too much since the optimizer will get rid of those)

digitalRead() does not return a bool but HIGH or LOW... as you depend on pull-down, ideally if you want to keep a book write it as bool doorSwitch =  (digitalRead(doorSwitchPin) == HIGH);thus way you get a real bool (type consistency) and you express if you have a pull-up (or pull down).

HIGH = true = 1
LOW = false = 0
No need for an extra comparison

You have a lot of spurrious braces in this code. They may seem harmless, but you are defining blocks for the scoping of variables and you may well end up with some very confusing errors about things not being defined when it appears that they are. You need to be careful with those braces and understand what they do.

In this code, if it isn’t immediately following the start of a function definition, an if statement, a while or for loop, or a switch statement then it doesn’t need braces. For example:

 if (now.hour() == 21 && now.minute() == 0 && now.second() == 1 && p == 1) // target time
    {
    
    digitalWrite (9,HIGH);  // hit relay high for a bit
    delay (800);
    digitalWrite (9,LOW);
    }
    { 
    int tim = now.hour (); 
    if (tim > 12) tim=tim-12;  // 12 hour time
    if (tim==0) tim=12;        // check if hour = 0
    if (now.month ()< 10)  /

The one before int tim = is not needed and could cause tim to not be seen in other parts of the code if they’re outside that set of braces.

J-M-L:
digitalRead() does not return a bool but HIGH or LOW... as you depend on pull-down, ideally if you want to keep a book write it as

bool doorSwitch =  (digitalRead(doorSwitchPin) == HIGH);

thus way you get a real bool (type consistency) and you express if you have a pull-up (or pull down).

The result of digitalRead() will be either LOW(0) or HIGH(1). When this is the conditional expression for an if(), it WILL be implicitly typecast, correctly, to a "real bool". There is no need to do a comparison to HIGH or LOW. The two snippets are 100% functionally identical.

Regards,
Ray L.

RayLivingston:
The result of digitalRead() will be either LOW(0) or HIGH(1). When this is the conditional expression for an if(), it WILL be implicitly typecast, correctly, to a "real bool". There is no need to do a comparison to HIGH or LOW. The two snippets are 100% functionally identical.

Regards,
Ray L.

I think the concern is that someday the core could be updated so that 1 is LOW and 0 is HIGH and that would break code. So technically it is more correct to compare to the constants that the API expects to be compared to.

However I find that possibility to be highly unlikely. If there ever was a place where abuse of an API seemed safe I think it would be this one.

I agree it delivers the same result technically.

My point was about being SEMANTICALLY correct and offering a self documenting code

1/ it’s a good habit to respect types. True and False are logic value suitable for a Boolean type, 0 and 1 (LOW and HIGH) as being the result of a #define are integer (which is also the type returned by the function)

The compiler thus sees an int being forced into a bool and will add - due to the standard - a comparison and will generate code for you, checking if the value is 0 or not and returning false (0) if it is and true (1) if it is not. Hence the assignment you have is actually transformed into bool doorSwitch =  (digitalRead(doorSwitchPin) == 0 ? false : true);

So not writing the == HIGH does not save you any computation, you just don’t see it / type it.

2/ which brings me to my second point. self documenting code is a good thing. Here you want the Boolean to be true if the digitalRead was to return HIGH. This fact is conditioned by the existence of a PULL-UP or PULLDOWN for that pin and thus writing the == HIGH helps understand that you expect a HIGH for a specific status. And of course the Boolean variable would be better with a meaningful name, such as bool doorIsOpen =  (digitalRead(doorSwitchPin) == HIGH);this way later on when you write your tests you can say

if (doorIsOpen) {
  ...
} else {
  ...
}

which also self documenting

That’s what I was trying to convey - those are important elements to answer OP beginner questions as to improving code.

Seasoned developers will get it without all this, understanding what’s happening in the background (at least for some).

PS: and adding a comment next to the pin constant definition stating there is an external pull-down/up would be good too

It's generally not a good idea to utilize what you happen to know about how something is implemented. This is especially true when using classes that may change over time - stick to the interface that was intended - no using side effects or class members that should have been private but aren't.

This isn't quite the same because I doubt that the definitions of HIGH and LOW will change any time before the heat death of the sun, but it's still a bad habit to get into :wink: