Can my program be made better?

Projects Programming
1

My program works fine, uses 3 alarms to turn different lights on and off daily. Just wondering if it could be made better. I am using a Leonardo board which I know is different from an uno board.

/*

 */
 
#include <Time.h>
#include <TimeAlarms.h>
//#include "Wire.h" 
#include <LiquidCrystal.h>

const int MorningAlarmPin1 =  A1;    // Blue Lights ON    
const int EveningAlarmPin1 =  A1;    // Blue Lights OFF
const int MorningAlarmPin2 =  A2;
const int EveningAlarmPin2 =  A2;
const int MorningAlarmPin3 =  A3;
const int EveningAlarmPin3 =  A3;             
const int ledPin = A1;
const int ledPin2 = A2;
const int ledPin3 = A3;
LiquidCrystal lcd( 8, 9, 4, 5, 6, 7);


/*|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||  D E F I N E  :  H E A T E R    O F F |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||*/

void setup()
{
  lcd.begin(16, 2);
  lcd.setCursor(0, 0);
  setTime(12,59,30,2,23,14); // set time to Saturday 8:29:00am Jan 1 2011
  // create the alarms 
  Alarm.alarmRepeat(8,30,0, MorningAlarm);  // Time Blue's come on every day
  Alarm.alarmRepeat(10,32,0,EveningAlarm);   // Time Blue's shut off every day 
  Alarm.alarmRepeat(8,30,0,MorningAlarm2);
  Alarm.alarmRepeat(10,31,0,EveningAlarm2);
  Alarm.alarmRepeat(8,30,0,MorningAlarm3);
  Alarm.alarmRepeat(10,33,0,EveningAlarm3);
  pinMode(MorningAlarmPin1, OUTPUT);     // set the digital pin as output:
  pinMode(EveningAlarmPin1, OUTPUT);     // set the digital pin as output:
  pinMode(MorningAlarmPin2, OUTPUT);
  pinMode(EveningAlarmPin2, OUTPUT);
  pinMode(MorningAlarmPin3, OUTPUT);
  pinMode(EveningAlarmPin3, OUTPUT);
  pinMode(ledPin, OUTPUT); 
  pinMode(ledPin2, OUTPUT);
  pinMode(ledPin3, OUTPUT);
}

void  loop(){
   
  digitalClockDisplay();
  Alarm.delay(1000); // wait one second between clock display
    
}

// functions to be called when an alarm triggers:
void MorningAlarm(){
  lcd.setCursor(0,0);
  lcd.print("BLUE ON ");
  digitalWrite(ledPin,LOW);
    
}

void EveningAlarm(){
  lcd.setCursor(0,0);
  lcd.print("BLUE OFF");
  digitalWrite(ledPin,HIGH);
}
void MorningAlarm2(){
  lcd.setCursor(0,1);
  lcd.print("WHITE ON ");
  digitalWrite(ledPin2,LOW);
    
}

void EveningAlarm2(){
  lcd.setCursor(0,1);
  lcd.print("WHITE OFF");
  digitalWrite(ledPin2,HIGH);
}
void MorningAlarm3(){
  lcd.setCursor(10,1);
  lcd.print("M ON ");
  digitalWrite(ledPin3,LOW);
    
}

void EveningAlarm3(){
  lcd.setCursor(10,1);
  lcd.print("M OFF ");
  digitalWrite(ledPin3,HIGH);
}


  void digitalClockDisplay()
  {
  // digital clock display of the time
  Serial.print(hour());
  printDigits(minute());
  printDigits(second());
  Serial.println(); 
}

void printDigits(int digits)
{
  Serial.print(":");
  if(digits < 10)
    Serial.print('0');
  Serial.print(digits);
}
2

If you have to ask, then then answer will always be "yes" :wink:

Immediately I can see that this is a good candidate for using arrays.

3

Depends what you mean by better.

You say it works and there is a very wise saying "If it works don't fix it".

There may be other ways to write a program that does the same thing which may make the program easier to understand in 6 month's time or which would be a better jumping off point for a more extensive program.

...R

4

Depends what you mean by better.

Agree with this. Are you trying to make it faster, smaller memory footprint or just more maintainable? These are not necessarily compatible and all of them are 'better' in some way from what you have.

5

This is the first program I have tried writing. I should have made the title what could I have done to make it better.

6 
const int MorningAlarmPin1 =  A1;    // Blue Lights ON    
const int EveningAlarmPin1 =  A1;    // Blue Lights OFF
const int MorningAlarmPin2 =  A2;
const int EveningAlarmPin2 =  A2;
const int MorningAlarmPin3 =  A3;
const int EveningAlarmPin3 =  A3;             
const int ledPin = A1;
const int ledPin2 = A2;
const int ledPin3 = A3;

As has already been suggested, these are candidates for making into arrays. Any time you start to use 1, 2, 3 after related variable names, it is usually more efficient to use arrays for storage.

#define MAX_PIN 3  // this can also be a const definition

const int MorningAlarmPin[MAX_PIN] = { A1, A2, A3 };    // Blue Lights ON    
const int EveningAlarmPin[MAX_PIN] =  { A1, A2, A3 };    // Blue Lights OFF
const int ledPin[MAX_PIN] = {A1, A2, A3 };

You can the use loops to process all the pins and simplify your functions to just take the pin number (called an array subscript) and just write one function. The use of a constant or #define also allows you to scale the number of pins easily if you use it consistently as the end conditions for loops and other constructs related to the data, rather than the magic number 3.

If you want to get more advanced, then you can group related information into a structure

#define MAX_PIN 3  // this can also be a const definition

struct PinDefinition_t
{
  int MorningAlarmPin;    // Blue Lights ON    
  int EveningAlarmPin;    // Blue Lights OFF
  int ledPin;
}

This groups the related data into a record of information so that the grouping is more obvious. You would declare and array of structures. This changes the way the functions work but the principle is the same as arrays, using a subscript.

To halve the storage requirements use uint8_t as the variable type (8 bits) instead of int (16 bits). For the range of values you need (pin numbers) this would be enough space.

For maintainability I would add some relevant comments. This should, at a minimum, be what the program does at the start of the file - in 3 months time it won't be so obvious to you. Same goes for any tricky bits of code and at the top of each function.

7

I added some comments

#include <Time.h>
#include <TimeAlarms.h>
//#include "Wire.h" 
#include <LiquidCrystal.h>


// So why do you declare so many constants with the same value?
// especially when they point to a pin. There's only one pin A1.

const int MorningAlarmPin1 =  A1;    // Blue Lights ON    

// why not blueLightPin = A1 ?
// For how these variables get used these aren't good names.
// see setup() for more

const int EveningAlarmPin1 =  A1;    // Blue Lights OFF
const int MorningAlarmPin2 =  A2;
const int EveningAlarmPin2 =  A2;
const int MorningAlarmPin3 =  A3;
const int EveningAlarmPin3 =  A3;             
const int ledPin = A1;
const int ledPin2 = A2;
const int ledPin3 = A3;


LiquidCrystal lcd( 8, 9, 4, 5, 6, 7);


/*|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||  D E F I N E  :  H E A T E R    O F F |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||*/

void setup()
{
  lcd.begin(16, 2);
  lcd.setCursor(0, 0);
  setTime(12,59,30,2,23,14); // set time to Saturday 8:29:00am Jan 1 2011
  // create the alarms 
  Alarm.alarmRepeat(8,30,0, MorningAlarm);  // Time Blue's come on every day
  Alarm.alarmRepeat(10,32,0,EveningAlarm);   // Time Blue's shut off every day 
  Alarm.alarmRepeat(8,30,0,MorningAlarm2);
  Alarm.alarmRepeat(10,31,0,EveningAlarm2);
  Alarm.alarmRepeat(8,30,0,MorningAlarm3);
  Alarm.alarmRepeat(10,33,0,EveningAlarm3);

// Here you use all those variables to set the pinmode on each pin 3 times.

  pinMode(MorningAlarmPin1, OUTPUT);     // set the digital pin as output:
  pinMode(EveningAlarmPin1, OUTPUT);     // set the digital pin as output:
  pinMode(MorningAlarmPin2, OUTPUT);
  pinMode(EveningAlarmPin2, OUTPUT);
  pinMode(MorningAlarmPin3, OUTPUT);
  pinMode(EveningAlarmPin3, OUTPUT);
  pinMode(ledPin, OUTPUT); 
  pinMode(ledPin2, OUTPUT);
  pinMode(ledPin3, OUTPUT);
}

void  loop(){
   
  digitalClockDisplay();
  Alarm.delay(1000); // wait one second between clock display
    
}

// I'm also not sure I would have used function names like this
// I might have used blueOn() blueOff(). The time you set the alarm 
// with reminds you that it's morning.

// functions to be called when an alarm triggers:
void MorningAlarm(){
  lcd.setCursor(0,0);
  lcd.print("BLUE ON ");
  digitalWrite(ledPin,LOW);
    
}

void EveningAlarm(){
  lcd.setCursor(0,0);
  lcd.print("BLUE OFF");
  digitalWrite(ledPin,HIGH);
}
void MorningAlarm2(){
  lcd.setCursor(0,1);
  lcd.print("WHITE ON ");
  digitalWrite(ledPin2,LOW);
    
}

void EveningAlarm2(){
  lcd.setCursor(0,1);
  lcd.print("WHITE OFF");
  digitalWrite(ledPin2,HIGH);
}
void MorningAlarm3(){
  lcd.setCursor(10,1);
  lcd.print("M ON ");
  digitalWrite(ledPin3,LOW);
    
}

void EveningAlarm3(){
  lcd.setCursor(10,1);
  lcd.print("M OFF ");
  digitalWrite(ledPin3,HIGH);
}

// The messages you print to the LCD are all different lengths. 
// Are you having any trouble with stray characters?


  void digitalClockDisplay()
  {
  // digital clock display of the time
  Serial.print(hour());
  printDigits(minute());
  printDigits(second());
  Serial.println(); 
}

void printDigits(int digits)
{
  Serial.print(":");
  if(digits < 10)
    Serial.print('0');
  Serial.print(digits);
}

So the only things I'd change are mostly names and only set pin modes once in setup(). Make a copy and use search and replace. I think it might read better if you moved setup() and loop() to the end.

Oh, there in the, nicely named, printDigits function find out the difference between ":" and '0' Its an important concept.