Digital clock with many functions

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
8
down vote

favorite












I made a digital clock with the use of an RTC (DS3231) and an LCD (16x2).



Current functions:



  • Display time

  • Display date (different formats)

  • Display temperature (Celsius and Fahrenheit)

  • Display day of the week (English and Dutch)

  • Display week number

  • Display number of day in the year

  • Humidity (%)

  • Dew point (Celsius and Fahrenheit)

  • Change summer -> winter time (this will be done automatically, once set correctly)

Future functions:



  • Menu (controlled using the buttons on the LCD shield

  • Alarm (already possible with predefining it at the start)

  • Timer

  • Stopwatch

  • Local time of different locations

I want to implement these future functions as quickly as possible. Please feel free to give feedback and your opinion about what could be better/more efficient.



Github



Clock.ino:



#include <DS3232RTC.h> // https://github.com/JChristensen/DS3232RTC
#include <Timezone.h> // https://github.com/JChristensen/Timezone
#include <TimeLib.h> // https://github.com/PaulStoffregen/Time
#include <Streaming.h> // http://arduiniana.org/libraries/streaming/
#include <Wire.h> // https://www.arduino.cc/en/Reference/Wire
#include <LiquidCrystal.h> // https://www.arduino.cc/en/Reference/LiquidCrystal
#include <DHT.h> // https://github.com/adafruit/DHT-sensor-library
#include "Date.h"

#define DHTPIN 2 // what pin we're connected to
#define DHTTYPE DHT22 // DHT 22 (AM2302)

// initialize the library by associating any needed LCD interface pin
// with the arduino pin number it is connected to
LiquidCrystal lcd(8, 9, 4, 5, 6, 7);

DHT dht(DHTPIN, DHTTYPE); //// Initialize DHT sensor for normal 16mhz Arduino

// constants won't change:
const String days[2][7] = "Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "zo", "ma", "di", "wo", "do", "vr", "za";
const String months[2][12] = "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec", "jan", "feb", "mrt", "mei", "jun", "jul", "aug", "sep", "okt", "nov", "dec";
const String labels[5] = "T", "RH", "DP", "D", "Wk";

/* EXPLANATION DIFFERENT FUNCTIONS FOR CLOCK (WILL ONLY BE USED ON THE HOMEPAGE)
TIME
'h' = hours
'm' = minutes
's' = seconds
'a' = alarm 1 (not atm)
'A' = alarm 2 (not atm)

WEATHER
'T' = temperature
'H' = humidity (no sensor atm)
'P' = dew point (no sensor atm)

DATE
'd' = day of week
'D' = day
'M' = month
'S' = month (short string)
'Y' = year
'w' = weeknumber
'n' = daynumber

MISCELLANEOUS
'l' = current location (need to create a array of enums/strings which can be used in Settings)

DELIMTERS
':' = delimiter
'-' = delimiter
'/' = delimiter
'.' = delimiter
'|' = delimiter
' ' = delimiter
*/
char homepage[16] = "h:m:s", "d D-M-Y", "w n", "S", "T H", "P";

enum languages_t EN, NL;

typedef struct
int hourFormat; // 12 or 24 hour format (AM/PM is not displayed)
uint8_t language; // The language for several labels
char degreesFormat; // Celcius or Fahrenheit
boolean labels; // Display temperature, weeknumber and daynumber with label
long intervalPage1; // interval at which to refresh lcd (milliseconds)
long switchPages; // interval at which to switchPage 1 to 2 (milliseconds)
Settings;

Settings default_settings = 24, NL, 'c', true, 1000, 30000;
Settings settings = 24, NL, 'c', true, 1000, 30000;

TimeChangeRule myDST = "MDT", Last, Sun, Mar, 2, 2 * 60; //Daylight time/Summertime = UTC + 2 hours
TimeChangeRule mySTD = "MST", Last, Sun, Oct, 2, 1 * 60; //Standard time/Wintertime = UTC + 1 hours
Timezone myTZ(myDST, mySTD);
TimeChangeRule *tcr; //pointer to the time change rule, use to get TZ abbrev

unsigned long previousMillis = 0; // will store last time lcd was updated (page 1)
unsigned long oldMillis = 0; // will store last time lcd switched pages
int language_id;
int rowX = 0;
int rowY = 2;
int numberOfPages = (sizeof(homepage) / sizeof(char)) / 32;
float tem;
float hum;
float dew;

void setup()
Serial.begin(9600);

// set up the LCD's number of columns and rows:
lcd.begin(16, 2);

// setSyncProvider() causes the Time library to synchronize with the
// external RTC by calling RTC.get() every five minutes by default.
setSyncProvider(RTC.get);
if (timeStatus() != timeSet) lcd << ("RTC SYNC FAILED");


pinMode(13, OUTPUT);
digitalWrite(13, LOW);



void loop()
// check to see if it's time to refresh the lcd; that is, if the difference
// between the current time and last time you refreshed the lcd is bigger than
// the interval at which you want to refresh the lcd.
unsigned long currentMillis = millis();
defineLanguageId();
tem = dht.readTemperature();
hum = dht.readHumidity();
if (currentMillis - previousMillis >= settings.intervalPage1)
// save the last time you refreshed the lcd
previousMillis = currentMillis;

// display the date and time according to the specificied order with the specified settings
displayPage(rowX, rowY);

if (currentMillis - oldMillis >= settings.switchPages)
oldMillis = currentMillis;

if (rowY == numberOfPages * 2)
rowX = 0;
rowY = 2;
lcd.clear();
else
rowX += 2;
rowY += 2;
lcd.clear();





void displayPage(int rowStart, int rowEnd)

time_t utc, local;
utc = now();

local = myTZ.toLocal(utc, &tcr);

// calculate which day and week of the year it is, according to the current local time
DayWeekNumber(year(local), month(local), day(local), weekday(local));

lcd.setCursor(0, 0);
// for-loop which loops through each row
for (int row = rowStart; row < rowEnd; row++)
// if row == odd, then we are on the second line, so move the cursor of the lcd
if (not(row % 2) == 0) lcd.setCursor(0, 1);
// for-loop which loops through each char in row
for (int pos = 0; pos < 15; pos++)
displayDesiredFunction(row, pos, local);


return;


void displayDesiredFunction(int row, int pos, time_t l)
switch (homepage[row][pos]) '
break;
case ' ':
displayDelimiter(' '); // display ' '
break;



void displayHours(time_t l)
(settings.hourFormat == 24) ? printI00(hour(l)) : printI00(hourFormat12(l));
return;


void displayTemperature()
if (settings.labels)
lcd << labels[0] << ": ";

(settings.degreesFormat == 'c') ? lcd << int(tem) << (char)223 << "C" : lcd << int(((tem * 9) / 5) + 32) << (char)223 << "F";


void displayHumidity()
if (settings.labels)
lcd << labels[1] << ": ";

lcd << int(hum) << "%";


void displayDewPoint()
dew = calculateDewPoint(tem, hum);
if (settings.labels)
lcd << labels[2] << ": ";

(settings.degreesFormat == 'c') ? lcd << int(dew) << (char)223 << "C" : lcd << int(((dew * 9) / 5) + 32) << (char)223 << "F";


float calculateDewPoint(float t, float h)
float a = 17.271;
float b = 237.7;
float temp = (a * t) / (b + t) + log(h * 0.01);
float Td = (b * temp) / (a - temp);
return Td;


void displayWeekday(int val)

lcd << days[language_id][val - 1];
return;


void displayNumber(char val)
if (val == 'd')
if (settings.labels == true)
lcd << labels[3] << ": ";

printI00(DW[0]);

else
if (settings.labels == true)
lcd << labels[4] << ": ";

printI00(DW[1]);

return;


void displayMonthShortStr(int val)
lcd << months[language_id][val];
return;


void displayLocation()
//lcd << settings.location;


void defineLanguageId()
switch (settings.language)
case EN:
language_id = 0;
break;
case NL:
language_id = 1;



void printI00(int val)

if (val < 10) lcd << '0';
lcd << _DEC(val);
return;


void displayDelimiter(char delim)
lcd << delim;
return;


void setTimeRTC(unsigned int hours, unsigned int minutes, unsigned int seconds, unsigned int d, unsigned int m, unsigned int y)
setTime(hours, minutes, seconds, d, m, y);
RTC.set(now());
return;



Date.h:



short DW[2];

void DayWeekNumber(unsigned int y, unsigned int m, unsigned int d, unsigned int w)






share|improve this question

















  • 1




    This is not C code with lcd << labels[0] << ": ";? Recommend tag change.
    – chux
    Jan 17 at 1:25







  • 1




    How is String defined? Is it just another name for std::string?
    – user1118321
    Jan 17 at 3:51










  • This is in fact C code. I use the library Streaming to be able to do lcd << labels[0] < ": ";
    – Sebastiaan Speck
    Jan 17 at 12:33










  • String is a type of C
    – Sebastiaan Speck
    Jan 17 at 12:33
















up vote
8
down vote

favorite












I made a digital clock with the use of an RTC (DS3231) and an LCD (16x2).



Current functions:



  • Display time

  • Display date (different formats)

  • Display temperature (Celsius and Fahrenheit)

  • Display day of the week (English and Dutch)

  • Display week number

  • Display number of day in the year

  • Humidity (%)

  • Dew point (Celsius and Fahrenheit)

  • Change summer -> winter time (this will be done automatically, once set correctly)

Future functions:



  • Menu (controlled using the buttons on the LCD shield

  • Alarm (already possible with predefining it at the start)

  • Timer

  • Stopwatch

  • Local time of different locations

I want to implement these future functions as quickly as possible. Please feel free to give feedback and your opinion about what could be better/more efficient.



Github



Clock.ino:



#include <DS3232RTC.h> // https://github.com/JChristensen/DS3232RTC
#include <Timezone.h> // https://github.com/JChristensen/Timezone
#include <TimeLib.h> // https://github.com/PaulStoffregen/Time
#include <Streaming.h> // http://arduiniana.org/libraries/streaming/
#include <Wire.h> // https://www.arduino.cc/en/Reference/Wire
#include <LiquidCrystal.h> // https://www.arduino.cc/en/Reference/LiquidCrystal
#include <DHT.h> // https://github.com/adafruit/DHT-sensor-library
#include "Date.h"

#define DHTPIN 2 // what pin we're connected to
#define DHTTYPE DHT22 // DHT 22 (AM2302)

// initialize the library by associating any needed LCD interface pin
// with the arduino pin number it is connected to
LiquidCrystal lcd(8, 9, 4, 5, 6, 7);

DHT dht(DHTPIN, DHTTYPE); //// Initialize DHT sensor for normal 16mhz Arduino

// constants won't change:
const String days[2][7] = "Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "zo", "ma", "di", "wo", "do", "vr", "za";
const String months[2][12] = "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec", "jan", "feb", "mrt", "mei", "jun", "jul", "aug", "sep", "okt", "nov", "dec";
const String labels[5] = "T", "RH", "DP", "D", "Wk";

/* EXPLANATION DIFFERENT FUNCTIONS FOR CLOCK (WILL ONLY BE USED ON THE HOMEPAGE)
TIME
'h' = hours
'm' = minutes
's' = seconds
'a' = alarm 1 (not atm)
'A' = alarm 2 (not atm)

WEATHER
'T' = temperature
'H' = humidity (no sensor atm)
'P' = dew point (no sensor atm)

DATE
'd' = day of week
'D' = day
'M' = month
'S' = month (short string)
'Y' = year
'w' = weeknumber
'n' = daynumber

MISCELLANEOUS
'l' = current location (need to create a array of enums/strings which can be used in Settings)

DELIMTERS
':' = delimiter
'-' = delimiter
'/' = delimiter
'.' = delimiter
'|' = delimiter
' ' = delimiter
*/
char homepage[16] = "h:m:s", "d D-M-Y", "w n", "S", "T H", "P";

enum languages_t EN, NL;

typedef struct
int hourFormat; // 12 or 24 hour format (AM/PM is not displayed)
uint8_t language; // The language for several labels
char degreesFormat; // Celcius or Fahrenheit
boolean labels; // Display temperature, weeknumber and daynumber with label
long intervalPage1; // interval at which to refresh lcd (milliseconds)
long switchPages; // interval at which to switchPage 1 to 2 (milliseconds)
Settings;

Settings default_settings = 24, NL, 'c', true, 1000, 30000;
Settings settings = 24, NL, 'c', true, 1000, 30000;

TimeChangeRule myDST = "MDT", Last, Sun, Mar, 2, 2 * 60; //Daylight time/Summertime = UTC + 2 hours
TimeChangeRule mySTD = "MST", Last, Sun, Oct, 2, 1 * 60; //Standard time/Wintertime = UTC + 1 hours
Timezone myTZ(myDST, mySTD);
TimeChangeRule *tcr; //pointer to the time change rule, use to get TZ abbrev

unsigned long previousMillis = 0; // will store last time lcd was updated (page 1)
unsigned long oldMillis = 0; // will store last time lcd switched pages
int language_id;
int rowX = 0;
int rowY = 2;
int numberOfPages = (sizeof(homepage) / sizeof(char)) / 32;
float tem;
float hum;
float dew;

void setup()
Serial.begin(9600);

// set up the LCD's number of columns and rows:
lcd.begin(16, 2);

// setSyncProvider() causes the Time library to synchronize with the
// external RTC by calling RTC.get() every five minutes by default.
setSyncProvider(RTC.get);
if (timeStatus() != timeSet) lcd << ("RTC SYNC FAILED");


pinMode(13, OUTPUT);
digitalWrite(13, LOW);



void loop()
// check to see if it's time to refresh the lcd; that is, if the difference
// between the current time and last time you refreshed the lcd is bigger than
// the interval at which you want to refresh the lcd.
unsigned long currentMillis = millis();
defineLanguageId();
tem = dht.readTemperature();
hum = dht.readHumidity();
if (currentMillis - previousMillis >= settings.intervalPage1)
// save the last time you refreshed the lcd
previousMillis = currentMillis;

// display the date and time according to the specificied order with the specified settings
displayPage(rowX, rowY);

if (currentMillis - oldMillis >= settings.switchPages)
oldMillis = currentMillis;

if (rowY == numberOfPages * 2)
rowX = 0;
rowY = 2;
lcd.clear();
else
rowX += 2;
rowY += 2;
lcd.clear();





void displayPage(int rowStart, int rowEnd)

time_t utc, local;
utc = now();

local = myTZ.toLocal(utc, &tcr);

// calculate which day and week of the year it is, according to the current local time
DayWeekNumber(year(local), month(local), day(local), weekday(local));

lcd.setCursor(0, 0);
// for-loop which loops through each row
for (int row = rowStart; row < rowEnd; row++)
// if row == odd, then we are on the second line, so move the cursor of the lcd
if (not(row % 2) == 0) lcd.setCursor(0, 1);
// for-loop which loops through each char in row
for (int pos = 0; pos < 15; pos++)
displayDesiredFunction(row, pos, local);


return;


void displayDesiredFunction(int row, int pos, time_t l)
switch (homepage[row][pos]) '
break;
case ' ':
displayDelimiter(' '); // display ' '
break;



void displayHours(time_t l)
(settings.hourFormat == 24) ? printI00(hour(l)) : printI00(hourFormat12(l));
return;


void displayTemperature()
if (settings.labels)
lcd << labels[0] << ": ";

(settings.degreesFormat == 'c') ? lcd << int(tem) << (char)223 << "C" : lcd << int(((tem * 9) / 5) + 32) << (char)223 << "F";


void displayHumidity()
if (settings.labels)
lcd << labels[1] << ": ";

lcd << int(hum) << "%";


void displayDewPoint()
dew = calculateDewPoint(tem, hum);
if (settings.labels)
lcd << labels[2] << ": ";

(settings.degreesFormat == 'c') ? lcd << int(dew) << (char)223 << "C" : lcd << int(((dew * 9) / 5) + 32) << (char)223 << "F";


float calculateDewPoint(float t, float h)
float a = 17.271;
float b = 237.7;
float temp = (a * t) / (b + t) + log(h * 0.01);
float Td = (b * temp) / (a - temp);
return Td;


void displayWeekday(int val)

lcd << days[language_id][val - 1];
return;


void displayNumber(char val)
if (val == 'd')
if (settings.labels == true)
lcd << labels[3] << ": ";

printI00(DW[0]);

else
if (settings.labels == true)
lcd << labels[4] << ": ";

printI00(DW[1]);

return;


void displayMonthShortStr(int val)
lcd << months[language_id][val];
return;


void displayLocation()
//lcd << settings.location;


void defineLanguageId()
switch (settings.language)
case EN:
language_id = 0;
break;
case NL:
language_id = 1;



void printI00(int val)

if (val < 10) lcd << '0';
lcd << _DEC(val);
return;


void displayDelimiter(char delim)
lcd << delim;
return;


void setTimeRTC(unsigned int hours, unsigned int minutes, unsigned int seconds, unsigned int d, unsigned int m, unsigned int y)
setTime(hours, minutes, seconds, d, m, y);
RTC.set(now());
return;



Date.h:



short DW[2];

void DayWeekNumber(unsigned int y, unsigned int m, unsigned int d, unsigned int w)






share|improve this question

















  • 1




    This is not C code with lcd << labels[0] << ": ";? Recommend tag change.
    – chux
    Jan 17 at 1:25







  • 1




    How is String defined? Is it just another name for std::string?
    – user1118321
    Jan 17 at 3:51










  • This is in fact C code. I use the library Streaming to be able to do lcd << labels[0] < ": ";
    – Sebastiaan Speck
    Jan 17 at 12:33










  • String is a type of C
    – Sebastiaan Speck
    Jan 17 at 12:33












up vote
8
down vote

favorite









up vote
8
down vote

favorite











I made a digital clock with the use of an RTC (DS3231) and an LCD (16x2).



Current functions:



  • Display time

  • Display date (different formats)

  • Display temperature (Celsius and Fahrenheit)

  • Display day of the week (English and Dutch)

  • Display week number

  • Display number of day in the year

  • Humidity (%)

  • Dew point (Celsius and Fahrenheit)

  • Change summer -> winter time (this will be done automatically, once set correctly)

Future functions:



  • Menu (controlled using the buttons on the LCD shield

  • Alarm (already possible with predefining it at the start)

  • Timer

  • Stopwatch

  • Local time of different locations

I want to implement these future functions as quickly as possible. Please feel free to give feedback and your opinion about what could be better/more efficient.



Github



Clock.ino:



#include <DS3232RTC.h> // https://github.com/JChristensen/DS3232RTC
#include <Timezone.h> // https://github.com/JChristensen/Timezone
#include <TimeLib.h> // https://github.com/PaulStoffregen/Time
#include <Streaming.h> // http://arduiniana.org/libraries/streaming/
#include <Wire.h> // https://www.arduino.cc/en/Reference/Wire
#include <LiquidCrystal.h> // https://www.arduino.cc/en/Reference/LiquidCrystal
#include <DHT.h> // https://github.com/adafruit/DHT-sensor-library
#include "Date.h"

#define DHTPIN 2 // what pin we're connected to
#define DHTTYPE DHT22 // DHT 22 (AM2302)

// initialize the library by associating any needed LCD interface pin
// with the arduino pin number it is connected to
LiquidCrystal lcd(8, 9, 4, 5, 6, 7);

DHT dht(DHTPIN, DHTTYPE); //// Initialize DHT sensor for normal 16mhz Arduino

// constants won't change:
const String days[2][7] = "Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "zo", "ma", "di", "wo", "do", "vr", "za";
const String months[2][12] = "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec", "jan", "feb", "mrt", "mei", "jun", "jul", "aug", "sep", "okt", "nov", "dec";
const String labels[5] = "T", "RH", "DP", "D", "Wk";

/* EXPLANATION DIFFERENT FUNCTIONS FOR CLOCK (WILL ONLY BE USED ON THE HOMEPAGE)
TIME
'h' = hours
'm' = minutes
's' = seconds
'a' = alarm 1 (not atm)
'A' = alarm 2 (not atm)

WEATHER
'T' = temperature
'H' = humidity (no sensor atm)
'P' = dew point (no sensor atm)

DATE
'd' = day of week
'D' = day
'M' = month
'S' = month (short string)
'Y' = year
'w' = weeknumber
'n' = daynumber

MISCELLANEOUS
'l' = current location (need to create a array of enums/strings which can be used in Settings)

DELIMTERS
':' = delimiter
'-' = delimiter
'/' = delimiter
'.' = delimiter
'|' = delimiter
' ' = delimiter
*/
char homepage[16] = "h:m:s", "d D-M-Y", "w n", "S", "T H", "P";

enum languages_t EN, NL;

typedef struct
int hourFormat; // 12 or 24 hour format (AM/PM is not displayed)
uint8_t language; // The language for several labels
char degreesFormat; // Celcius or Fahrenheit
boolean labels; // Display temperature, weeknumber and daynumber with label
long intervalPage1; // interval at which to refresh lcd (milliseconds)
long switchPages; // interval at which to switchPage 1 to 2 (milliseconds)
Settings;

Settings default_settings = 24, NL, 'c', true, 1000, 30000;
Settings settings = 24, NL, 'c', true, 1000, 30000;

TimeChangeRule myDST = "MDT", Last, Sun, Mar, 2, 2 * 60; //Daylight time/Summertime = UTC + 2 hours
TimeChangeRule mySTD = "MST", Last, Sun, Oct, 2, 1 * 60; //Standard time/Wintertime = UTC + 1 hours
Timezone myTZ(myDST, mySTD);
TimeChangeRule *tcr; //pointer to the time change rule, use to get TZ abbrev

unsigned long previousMillis = 0; // will store last time lcd was updated (page 1)
unsigned long oldMillis = 0; // will store last time lcd switched pages
int language_id;
int rowX = 0;
int rowY = 2;
int numberOfPages = (sizeof(homepage) / sizeof(char)) / 32;
float tem;
float hum;
float dew;

void setup()
Serial.begin(9600);

// set up the LCD's number of columns and rows:
lcd.begin(16, 2);

// setSyncProvider() causes the Time library to synchronize with the
// external RTC by calling RTC.get() every five minutes by default.
setSyncProvider(RTC.get);
if (timeStatus() != timeSet) lcd << ("RTC SYNC FAILED");


pinMode(13, OUTPUT);
digitalWrite(13, LOW);



void loop()
// check to see if it's time to refresh the lcd; that is, if the difference
// between the current time and last time you refreshed the lcd is bigger than
// the interval at which you want to refresh the lcd.
unsigned long currentMillis = millis();
defineLanguageId();
tem = dht.readTemperature();
hum = dht.readHumidity();
if (currentMillis - previousMillis >= settings.intervalPage1)
// save the last time you refreshed the lcd
previousMillis = currentMillis;

// display the date and time according to the specificied order with the specified settings
displayPage(rowX, rowY);

if (currentMillis - oldMillis >= settings.switchPages)
oldMillis = currentMillis;

if (rowY == numberOfPages * 2)
rowX = 0;
rowY = 2;
lcd.clear();
else
rowX += 2;
rowY += 2;
lcd.clear();





void displayPage(int rowStart, int rowEnd)

time_t utc, local;
utc = now();

local = myTZ.toLocal(utc, &tcr);

// calculate which day and week of the year it is, according to the current local time
DayWeekNumber(year(local), month(local), day(local), weekday(local));

lcd.setCursor(0, 0);
// for-loop which loops through each row
for (int row = rowStart; row < rowEnd; row++)
// if row == odd, then we are on the second line, so move the cursor of the lcd
if (not(row % 2) == 0) lcd.setCursor(0, 1);
// for-loop which loops through each char in row
for (int pos = 0; pos < 15; pos++)
displayDesiredFunction(row, pos, local);


return;


void displayDesiredFunction(int row, int pos, time_t l)
switch (homepage[row][pos]) '
break;
case ' ':
displayDelimiter(' '); // display ' '
break;



void displayHours(time_t l)
(settings.hourFormat == 24) ? printI00(hour(l)) : printI00(hourFormat12(l));
return;


void displayTemperature()
if (settings.labels)
lcd << labels[0] << ": ";

(settings.degreesFormat == 'c') ? lcd << int(tem) << (char)223 << "C" : lcd << int(((tem * 9) / 5) + 32) << (char)223 << "F";


void displayHumidity()
if (settings.labels)
lcd << labels[1] << ": ";

lcd << int(hum) << "%";


void displayDewPoint()
dew = calculateDewPoint(tem, hum);
if (settings.labels)
lcd << labels[2] << ": ";

(settings.degreesFormat == 'c') ? lcd << int(dew) << (char)223 << "C" : lcd << int(((dew * 9) / 5) + 32) << (char)223 << "F";


float calculateDewPoint(float t, float h)
float a = 17.271;
float b = 237.7;
float temp = (a * t) / (b + t) + log(h * 0.01);
float Td = (b * temp) / (a - temp);
return Td;


void displayWeekday(int val)

lcd << days[language_id][val - 1];
return;


void displayNumber(char val)
if (val == 'd')
if (settings.labels == true)
lcd << labels[3] << ": ";

printI00(DW[0]);

else
if (settings.labels == true)
lcd << labels[4] << ": ";

printI00(DW[1]);

return;


void displayMonthShortStr(int val)
lcd << months[language_id][val];
return;


void displayLocation()
//lcd << settings.location;


void defineLanguageId()
switch (settings.language)
case EN:
language_id = 0;
break;
case NL:
language_id = 1;



void printI00(int val)

if (val < 10) lcd << '0';
lcd << _DEC(val);
return;


void displayDelimiter(char delim)
lcd << delim;
return;


void setTimeRTC(unsigned int hours, unsigned int minutes, unsigned int seconds, unsigned int d, unsigned int m, unsigned int y)
setTime(hours, minutes, seconds, d, m, y);
RTC.set(now());
return;



Date.h:



short DW[2];

void DayWeekNumber(unsigned int y, unsigned int m, unsigned int d, unsigned int w)






share|improve this question













I made a digital clock with the use of an RTC (DS3231) and an LCD (16x2).



Current functions:



  • Display time

  • Display date (different formats)

  • Display temperature (Celsius and Fahrenheit)

  • Display day of the week (English and Dutch)

  • Display week number

  • Display number of day in the year

  • Humidity (%)

  • Dew point (Celsius and Fahrenheit)

  • Change summer -> winter time (this will be done automatically, once set correctly)

Future functions:



  • Menu (controlled using the buttons on the LCD shield

  • Alarm (already possible with predefining it at the start)

  • Timer

  • Stopwatch

  • Local time of different locations

I want to implement these future functions as quickly as possible. Please feel free to give feedback and your opinion about what could be better/more efficient.



Github



Clock.ino:



#include <DS3232RTC.h> // https://github.com/JChristensen/DS3232RTC
#include <Timezone.h> // https://github.com/JChristensen/Timezone
#include <TimeLib.h> // https://github.com/PaulStoffregen/Time
#include <Streaming.h> // http://arduiniana.org/libraries/streaming/
#include <Wire.h> // https://www.arduino.cc/en/Reference/Wire
#include <LiquidCrystal.h> // https://www.arduino.cc/en/Reference/LiquidCrystal
#include <DHT.h> // https://github.com/adafruit/DHT-sensor-library
#include "Date.h"

#define DHTPIN 2 // what pin we're connected to
#define DHTTYPE DHT22 // DHT 22 (AM2302)

// initialize the library by associating any needed LCD interface pin
// with the arduino pin number it is connected to
LiquidCrystal lcd(8, 9, 4, 5, 6, 7);

DHT dht(DHTPIN, DHTTYPE); //// Initialize DHT sensor for normal 16mhz Arduino

// constants won't change:
const String days[2][7] = "Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "zo", "ma", "di", "wo", "do", "vr", "za";
const String months[2][12] = "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec", "jan", "feb", "mrt", "mei", "jun", "jul", "aug", "sep", "okt", "nov", "dec";
const String labels[5] = "T", "RH", "DP", "D", "Wk";

/* EXPLANATION DIFFERENT FUNCTIONS FOR CLOCK (WILL ONLY BE USED ON THE HOMEPAGE)
TIME
'h' = hours
'm' = minutes
's' = seconds
'a' = alarm 1 (not atm)
'A' = alarm 2 (not atm)

WEATHER
'T' = temperature
'H' = humidity (no sensor atm)
'P' = dew point (no sensor atm)

DATE
'd' = day of week
'D' = day
'M' = month
'S' = month (short string)
'Y' = year
'w' = weeknumber
'n' = daynumber

MISCELLANEOUS
'l' = current location (need to create a array of enums/strings which can be used in Settings)

DELIMTERS
':' = delimiter
'-' = delimiter
'/' = delimiter
'.' = delimiter
'|' = delimiter
' ' = delimiter
*/
char homepage[16] = "h:m:s", "d D-M-Y", "w n", "S", "T H", "P";

enum languages_t EN, NL;

typedef struct
int hourFormat; // 12 or 24 hour format (AM/PM is not displayed)
uint8_t language; // The language for several labels
char degreesFormat; // Celcius or Fahrenheit
boolean labels; // Display temperature, weeknumber and daynumber with label
long intervalPage1; // interval at which to refresh lcd (milliseconds)
long switchPages; // interval at which to switchPage 1 to 2 (milliseconds)
Settings;

Settings default_settings = 24, NL, 'c', true, 1000, 30000;
Settings settings = 24, NL, 'c', true, 1000, 30000;

TimeChangeRule myDST = "MDT", Last, Sun, Mar, 2, 2 * 60; //Daylight time/Summertime = UTC + 2 hours
TimeChangeRule mySTD = "MST", Last, Sun, Oct, 2, 1 * 60; //Standard time/Wintertime = UTC + 1 hours
Timezone myTZ(myDST, mySTD);
TimeChangeRule *tcr; //pointer to the time change rule, use to get TZ abbrev

unsigned long previousMillis = 0; // will store last time lcd was updated (page 1)
unsigned long oldMillis = 0; // will store last time lcd switched pages
int language_id;
int rowX = 0;
int rowY = 2;
int numberOfPages = (sizeof(homepage) / sizeof(char)) / 32;
float tem;
float hum;
float dew;

void setup()
Serial.begin(9600);

// set up the LCD's number of columns and rows:
lcd.begin(16, 2);

// setSyncProvider() causes the Time library to synchronize with the
// external RTC by calling RTC.get() every five minutes by default.
setSyncProvider(RTC.get);
if (timeStatus() != timeSet) lcd << ("RTC SYNC FAILED");


pinMode(13, OUTPUT);
digitalWrite(13, LOW);



void loop()
// check to see if it's time to refresh the lcd; that is, if the difference
// between the current time and last time you refreshed the lcd is bigger than
// the interval at which you want to refresh the lcd.
unsigned long currentMillis = millis();
defineLanguageId();
tem = dht.readTemperature();
hum = dht.readHumidity();
if (currentMillis - previousMillis >= settings.intervalPage1)
// save the last time you refreshed the lcd
previousMillis = currentMillis;

// display the date and time according to the specificied order with the specified settings
displayPage(rowX, rowY);

if (currentMillis - oldMillis >= settings.switchPages)
oldMillis = currentMillis;

if (rowY == numberOfPages * 2)
rowX = 0;
rowY = 2;
lcd.clear();
else
rowX += 2;
rowY += 2;
lcd.clear();





void displayPage(int rowStart, int rowEnd)

time_t utc, local;
utc = now();

local = myTZ.toLocal(utc, &tcr);

// calculate which day and week of the year it is, according to the current local time
DayWeekNumber(year(local), month(local), day(local), weekday(local));

lcd.setCursor(0, 0);
// for-loop which loops through each row
for (int row = rowStart; row < rowEnd; row++)
// if row == odd, then we are on the second line, so move the cursor of the lcd
if (not(row % 2) == 0) lcd.setCursor(0, 1);
// for-loop which loops through each char in row
for (int pos = 0; pos < 15; pos++)
displayDesiredFunction(row, pos, local);


return;


void displayDesiredFunction(int row, int pos, time_t l)
switch (homepage[row][pos]) '
break;
case ' ':
displayDelimiter(' '); // display ' '
break;



void displayHours(time_t l)
(settings.hourFormat == 24) ? printI00(hour(l)) : printI00(hourFormat12(l));
return;


void displayTemperature()
if (settings.labels)
lcd << labels[0] << ": ";

(settings.degreesFormat == 'c') ? lcd << int(tem) << (char)223 << "C" : lcd << int(((tem * 9) / 5) + 32) << (char)223 << "F";


void displayHumidity()
if (settings.labels)
lcd << labels[1] << ": ";

lcd << int(hum) << "%";


void displayDewPoint()
dew = calculateDewPoint(tem, hum);
if (settings.labels)
lcd << labels[2] << ": ";

(settings.degreesFormat == 'c') ? lcd << int(dew) << (char)223 << "C" : lcd << int(((dew * 9) / 5) + 32) << (char)223 << "F";


float calculateDewPoint(float t, float h)
float a = 17.271;
float b = 237.7;
float temp = (a * t) / (b + t) + log(h * 0.01);
float Td = (b * temp) / (a - temp);
return Td;


void displayWeekday(int val)

lcd << days[language_id][val - 1];
return;


void displayNumber(char val)
if (val == 'd')
if (settings.labels == true)
lcd << labels[3] << ": ";

printI00(DW[0]);

else
if (settings.labels == true)
lcd << labels[4] << ": ";

printI00(DW[1]);

return;


void displayMonthShortStr(int val)
lcd << months[language_id][val];
return;


void displayLocation()
//lcd << settings.location;


void defineLanguageId()
switch (settings.language)
case EN:
language_id = 0;
break;
case NL:
language_id = 1;



void printI00(int val)

if (val < 10) lcd << '0';
lcd << _DEC(val);
return;


void displayDelimiter(char delim)
lcd << delim;
return;


void setTimeRTC(unsigned int hours, unsigned int minutes, unsigned int seconds, unsigned int d, unsigned int m, unsigned int y)
setTime(hours, minutes, seconds, d, m, y);
RTC.set(now());
return;



Date.h:



short DW[2];

void DayWeekNumber(unsigned int y, unsigned int m, unsigned int d, unsigned int w)








share|improve this question












share|improve this question




share|improve this question








edited Jan 17 at 3:32









user1118321

10.2k11144




10.2k11144









asked Jan 16 at 15:49









Sebastiaan Speck

462




462







  • 1




    This is not C code with lcd << labels[0] << ": ";? Recommend tag change.
    – chux
    Jan 17 at 1:25







  • 1




    How is String defined? Is it just another name for std::string?
    – user1118321
    Jan 17 at 3:51










  • This is in fact C code. I use the library Streaming to be able to do lcd << labels[0] < ": ";
    – Sebastiaan Speck
    Jan 17 at 12:33










  • String is a type of C
    – Sebastiaan Speck
    Jan 17 at 12:33












  • 1




    This is not C code with lcd << labels[0] << ": ";? Recommend tag change.
    – chux
    Jan 17 at 1:25







  • 1




    How is String defined? Is it just another name for std::string?
    – user1118321
    Jan 17 at 3:51










  • This is in fact C code. I use the library Streaming to be able to do lcd << labels[0] < ": ";
    – Sebastiaan Speck
    Jan 17 at 12:33










  • String is a type of C
    – Sebastiaan Speck
    Jan 17 at 12:33







1




1




This is not C code with lcd << labels[0] << ": ";? Recommend tag change.
– chux
Jan 17 at 1:25





This is not C code with lcd << labels[0] << ": ";? Recommend tag change.
– chux
Jan 17 at 1:25





1




1




How is String defined? Is it just another name for std::string?
– user1118321
Jan 17 at 3:51




How is String defined? Is it just another name for std::string?
– user1118321
Jan 17 at 3:51












This is in fact C code. I use the library Streaming to be able to do lcd << labels[0] < ": ";
– Sebastiaan Speck
Jan 17 at 12:33




This is in fact C code. I use the library Streaming to be able to do lcd << labels[0] < ": ";
– Sebastiaan Speck
Jan 17 at 12:33












String is a type of C
– Sebastiaan Speck
Jan 17 at 12:33




String is a type of C
– Sebastiaan Speck
Jan 17 at 12:33










2 Answers
2






active

oldest

votes

















up vote
4
down vote













Just a single thing I noticed:



if (not(row % 2) == 0)


This code works, but only accidentally. You should have written either of these instead:



if (not(row % 2 == 0))
if (row % 2 != 0)


The code you currently have takes the remainder of the division (0 or 1) and then performs a Boolean operation on it (the not). The result of that negation (true or false) is then converted back to an integer (1 or 0) and compared with zero. That's too much conversion between different types.



This pattern only works for comparing with 0. When you compare with 2, it fails:



not(row % 3) == 2


Since not can only ever return true or false, it can never be equal to 2.




And another:



int numberOfPages = (sizeof(homepage) / sizeof(char)) / 32;


This should be:



size_t numberOfPages = sizeof homepage / sizeof homepage[0];





share|improve this answer























  • One could also use std::size() in C++17, or create it, since it shouldn't be hard.
    – Incomputable
    Jan 17 at 10:03










  • I changed the if-statement but the numberOfPages replacement didn't work. See user1118321's answer for a better solution.
    – Sebastiaan Speck
    Jan 17 at 14:26

















up vote
4
down vote













This looks like a really fun project! It's written in a fairly straightforward way, so I think I get most of it. Here are some suggestions for improving it.



Avoid Magic Numbers



You have a lot of bare numbers in your code. Some are obvious, but some are not and there's no explanation of what they are. Furthermore, if you want to change any of them, you'll need to find all instances of them and change each instance. For example, in your constant declarations, you use the numbers 2, 7, 12 and 5. I would make constants for these, like this:



const size_t kNumSupportedLanguages = 2;
const size_t kNumDaysPerWeek = 7;
const size_t kNumMonthsPerYear = 12;
const size_t kNumLabels = 5;


I'd probably do the same for the millisecond values:



const long kOneSecInMS = 1000;
const long kThirtySecsInMS = 30000;


What does the 32 represent in this line:



int numberOfPages = (sizeof(homepage) / sizeof(char)) / 32;


Whatever it is, make a constant for it! Likewise with 9600, 16, 2, and 13 in the setup() function. In fact, if you did that, you wouldn't even need the comment above the call to lcd.begin(). It would just be lcd.begin(kNumLCDCols, kNumLCDRows);. In displayTemperature() you cast the value 233 to a char. Why? What does it represent? If it's the degree symbol, just give it a name.



Simplify With Types



Your Settings struct has several fields that can only have 2 values, but take up more space. I know that embedded applications are often very short on memory, so it's good to keep things small. I'm not sure if that's the case here, but you could easily make a single 8-bit field and use 1 bit for the hour format, 1 bit for language (if you know you won't ever need more languages), 1 bit for the degree format, and 1 bit for whether to show labels or not. That's 7 bytes in your current struct (though probably more due to alignment issues), compressed into a single byte.



Even if you don't do that, I think using an enum similar to the one you have for languages_t would be a good idea for the other 3 fields that only have 2 possible values.



Avoid Global Variables



You have a few dozen global variables in your code. Global variables are a pain to work with because when they get a bad value in them, it can be impossible to track down where it changed, since every part of the code has access to them. I don't see a main() function anywhere in your code, so I'm not sure how this all runs in practice. I would recommend putting all your globals in your main() function (or whatever function eventually calls through to the functions you've written here), and passing them into each function. By passing them into each function you can see the flow of data more easily when debugging.



Don't Declare Variables in Headers



In your Date.h file you declare a global variable DW. This is will cause the variable to be created in every file that #includes that header. And since there are no header guards around the file, if you include the header, and then also include another header that includes Date.h, you'll get a compile error because you'll be redeclaring the variable in the same scope.



Use if When It Would Be More Clear



You've used a ternary operator in a very hard-to-understand way in several places like this:



(w == 0) ? DW[1] = (DW[0] - 7 + 10) / 7 : DW[1] = (DW[0] - w + 10) / 7;


It would be much clearer to just write a simple if statement:



if (w == 0)

DW[1] = (DW[0] - 7 + 10) / 7;

else

DW[1] = (DW[0] - w + 10) / 7;



If you really really want to use a ternary operator for some reason, at least write it in the expected order:



DW[1] = (w == 0) ? (DW[0] - 7 + 10) / 7 : (DW[0] - w + 10) / 7;


Or even:



int subVal = (w == 0) ? 7 : w;
DW [ 1 ] = (DW[0] - subVal + 10) / 7;


Make Functions for Common Tasks



There are some long lines of code you have repeated in several places. This is another area where you have the potential for a bug in only 1 version that will be hard to track down, or you'll change one and need to update all of them. If you just put it in a function, it's easier to read and doesn't suffer from those problems. For example, your conversion from Fahrenheit to Celsius. Just make a function:



int FahrenheitToCelsius(const float celsius)

return static_cast<int>(celsius * 9 / 5 + 32);



That will likely inlined by the compiler, so it shouldn't be any less efficient.



Simplify



The function in Date.h is overly complicated. I would rewrite it like this:



void DayWeekNumber(unsigned int y, unsigned int m, unsigned int d, unsigned int w) 


Also, don't add return statements to the end of void functions. It adds nothing, as the function will return without the statement.






share|improve this answer























  • I replaced the magic numbers as you said. I simpliefied the types (as far as my knowledge reached. I don't know how I can return multiple values with different types, so I put the main code inside a mainloop but the variables are still global. I simplied the Date.h and I made some functions for common tasks. I will post the new code in a new question
    – Sebastiaan Speck
    Jan 17 at 14:25










Your Answer




StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");

StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: false,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);



);








 

draft saved


draft discarded


















StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f185229%2fdigital-clock-with-many-functions%23new-answer', 'question_page');

);

Post as a guest






























2 Answers
2






active

oldest

votes








2 Answers
2






active

oldest

votes









active

oldest

votes






active

oldest

votes








up vote
4
down vote













Just a single thing I noticed:



if (not(row % 2) == 0)


This code works, but only accidentally. You should have written either of these instead:



if (not(row % 2 == 0))
if (row % 2 != 0)


The code you currently have takes the remainder of the division (0 or 1) and then performs a Boolean operation on it (the not). The result of that negation (true or false) is then converted back to an integer (1 or 0) and compared with zero. That's too much conversion between different types.



This pattern only works for comparing with 0. When you compare with 2, it fails:



not(row % 3) == 2


Since not can only ever return true or false, it can never be equal to 2.




And another:



int numberOfPages = (sizeof(homepage) / sizeof(char)) / 32;


This should be:



size_t numberOfPages = sizeof homepage / sizeof homepage[0];





share|improve this answer























  • One could also use std::size() in C++17, or create it, since it shouldn't be hard.
    – Incomputable
    Jan 17 at 10:03










  • I changed the if-statement but the numberOfPages replacement didn't work. See user1118321's answer for a better solution.
    – Sebastiaan Speck
    Jan 17 at 14:26














up vote
4
down vote













Just a single thing I noticed:



if (not(row % 2) == 0)


This code works, but only accidentally. You should have written either of these instead:



if (not(row % 2 == 0))
if (row % 2 != 0)


The code you currently have takes the remainder of the division (0 or 1) and then performs a Boolean operation on it (the not). The result of that negation (true or false) is then converted back to an integer (1 or 0) and compared with zero. That's too much conversion between different types.



This pattern only works for comparing with 0. When you compare with 2, it fails:



not(row % 3) == 2


Since not can only ever return true or false, it can never be equal to 2.




And another:



int numberOfPages = (sizeof(homepage) / sizeof(char)) / 32;


This should be:



size_t numberOfPages = sizeof homepage / sizeof homepage[0];





share|improve this answer























  • One could also use std::size() in C++17, or create it, since it shouldn't be hard.
    – Incomputable
    Jan 17 at 10:03










  • I changed the if-statement but the numberOfPages replacement didn't work. See user1118321's answer for a better solution.
    – Sebastiaan Speck
    Jan 17 at 14:26












up vote
4
down vote










up vote
4
down vote









Just a single thing I noticed:



if (not(row % 2) == 0)


This code works, but only accidentally. You should have written either of these instead:



if (not(row % 2 == 0))
if (row % 2 != 0)


The code you currently have takes the remainder of the division (0 or 1) and then performs a Boolean operation on it (the not). The result of that negation (true or false) is then converted back to an integer (1 or 0) and compared with zero. That's too much conversion between different types.



This pattern only works for comparing with 0. When you compare with 2, it fails:



not(row % 3) == 2


Since not can only ever return true or false, it can never be equal to 2.




And another:



int numberOfPages = (sizeof(homepage) / sizeof(char)) / 32;


This should be:



size_t numberOfPages = sizeof homepage / sizeof homepage[0];





share|improve this answer















Just a single thing I noticed:



if (not(row % 2) == 0)


This code works, but only accidentally. You should have written either of these instead:



if (not(row % 2 == 0))
if (row % 2 != 0)


The code you currently have takes the remainder of the division (0 or 1) and then performs a Boolean operation on it (the not). The result of that negation (true or false) is then converted back to an integer (1 or 0) and compared with zero. That's too much conversion between different types.



This pattern only works for comparing with 0. When you compare with 2, it fails:



not(row % 3) == 2


Since not can only ever return true or false, it can never be equal to 2.




And another:



int numberOfPages = (sizeof(homepage) / sizeof(char)) / 32;


This should be:



size_t numberOfPages = sizeof homepage / sizeof homepage[0];






share|improve this answer















share|improve this answer



share|improve this answer








edited Jan 17 at 6:51


























answered Jan 17 at 6:45









Roland Illig

10.4k11543




10.4k11543











  • One could also use std::size() in C++17, or create it, since it shouldn't be hard.
    – Incomputable
    Jan 17 at 10:03










  • I changed the if-statement but the numberOfPages replacement didn't work. See user1118321's answer for a better solution.
    – Sebastiaan Speck
    Jan 17 at 14:26
















  • One could also use std::size() in C++17, or create it, since it shouldn't be hard.
    – Incomputable
    Jan 17 at 10:03










  • I changed the if-statement but the numberOfPages replacement didn't work. See user1118321's answer for a better solution.
    – Sebastiaan Speck
    Jan 17 at 14:26















One could also use std::size() in C++17, or create it, since it shouldn't be hard.
– Incomputable
Jan 17 at 10:03




One could also use std::size() in C++17, or create it, since it shouldn't be hard.
– Incomputable
Jan 17 at 10:03












I changed the if-statement but the numberOfPages replacement didn't work. See user1118321's answer for a better solution.
– Sebastiaan Speck
Jan 17 at 14:26




I changed the if-statement but the numberOfPages replacement didn't work. See user1118321's answer for a better solution.
– Sebastiaan Speck
Jan 17 at 14:26












up vote
4
down vote













This looks like a really fun project! It's written in a fairly straightforward way, so I think I get most of it. Here are some suggestions for improving it.



Avoid Magic Numbers



You have a lot of bare numbers in your code. Some are obvious, but some are not and there's no explanation of what they are. Furthermore, if you want to change any of them, you'll need to find all instances of them and change each instance. For example, in your constant declarations, you use the numbers 2, 7, 12 and 5. I would make constants for these, like this:



const size_t kNumSupportedLanguages = 2;
const size_t kNumDaysPerWeek = 7;
const size_t kNumMonthsPerYear = 12;
const size_t kNumLabels = 5;


I'd probably do the same for the millisecond values:



const long kOneSecInMS = 1000;
const long kThirtySecsInMS = 30000;


What does the 32 represent in this line:



int numberOfPages = (sizeof(homepage) / sizeof(char)) / 32;


Whatever it is, make a constant for it! Likewise with 9600, 16, 2, and 13 in the setup() function. In fact, if you did that, you wouldn't even need the comment above the call to lcd.begin(). It would just be lcd.begin(kNumLCDCols, kNumLCDRows);. In displayTemperature() you cast the value 233 to a char. Why? What does it represent? If it's the degree symbol, just give it a name.



Simplify With Types



Your Settings struct has several fields that can only have 2 values, but take up more space. I know that embedded applications are often very short on memory, so it's good to keep things small. I'm not sure if that's the case here, but you could easily make a single 8-bit field and use 1 bit for the hour format, 1 bit for language (if you know you won't ever need more languages), 1 bit for the degree format, and 1 bit for whether to show labels or not. That's 7 bytes in your current struct (though probably more due to alignment issues), compressed into a single byte.



Even if you don't do that, I think using an enum similar to the one you have for languages_t would be a good idea for the other 3 fields that only have 2 possible values.



Avoid Global Variables



You have a few dozen global variables in your code. Global variables are a pain to work with because when they get a bad value in them, it can be impossible to track down where it changed, since every part of the code has access to them. I don't see a main() function anywhere in your code, so I'm not sure how this all runs in practice. I would recommend putting all your globals in your main() function (or whatever function eventually calls through to the functions you've written here), and passing them into each function. By passing them into each function you can see the flow of data more easily when debugging.



Don't Declare Variables in Headers



In your Date.h file you declare a global variable DW. This is will cause the variable to be created in every file that #includes that header. And since there are no header guards around the file, if you include the header, and then also include another header that includes Date.h, you'll get a compile error because you'll be redeclaring the variable in the same scope.



Use if When It Would Be More Clear



You've used a ternary operator in a very hard-to-understand way in several places like this:



(w == 0) ? DW[1] = (DW[0] - 7 + 10) / 7 : DW[1] = (DW[0] - w + 10) / 7;


It would be much clearer to just write a simple if statement:



if (w == 0)

DW[1] = (DW[0] - 7 + 10) / 7;

else

DW[1] = (DW[0] - w + 10) / 7;



If you really really want to use a ternary operator for some reason, at least write it in the expected order:



DW[1] = (w == 0) ? (DW[0] - 7 + 10) / 7 : (DW[0] - w + 10) / 7;


Or even:



int subVal = (w == 0) ? 7 : w;
DW [ 1 ] = (DW[0] - subVal + 10) / 7;


Make Functions for Common Tasks



There are some long lines of code you have repeated in several places. This is another area where you have the potential for a bug in only 1 version that will be hard to track down, or you'll change one and need to update all of them. If you just put it in a function, it's easier to read and doesn't suffer from those problems. For example, your conversion from Fahrenheit to Celsius. Just make a function:



int FahrenheitToCelsius(const float celsius)

return static_cast<int>(celsius * 9 / 5 + 32);



That will likely inlined by the compiler, so it shouldn't be any less efficient.



Simplify



The function in Date.h is overly complicated. I would rewrite it like this:



void DayWeekNumber(unsigned int y, unsigned int m, unsigned int d, unsigned int w) 


Also, don't add return statements to the end of void functions. It adds nothing, as the function will return without the statement.






share|improve this answer























  • I replaced the magic numbers as you said. I simpliefied the types (as far as my knowledge reached. I don't know how I can return multiple values with different types, so I put the main code inside a mainloop but the variables are still global. I simplied the Date.h and I made some functions for common tasks. I will post the new code in a new question
    – Sebastiaan Speck
    Jan 17 at 14:25














up vote
4
down vote













This looks like a really fun project! It's written in a fairly straightforward way, so I think I get most of it. Here are some suggestions for improving it.



Avoid Magic Numbers



You have a lot of bare numbers in your code. Some are obvious, but some are not and there's no explanation of what they are. Furthermore, if you want to change any of them, you'll need to find all instances of them and change each instance. For example, in your constant declarations, you use the numbers 2, 7, 12 and 5. I would make constants for these, like this:



const size_t kNumSupportedLanguages = 2;
const size_t kNumDaysPerWeek = 7;
const size_t kNumMonthsPerYear = 12;
const size_t kNumLabels = 5;


I'd probably do the same for the millisecond values:



const long kOneSecInMS = 1000;
const long kThirtySecsInMS = 30000;


What does the 32 represent in this line:



int numberOfPages = (sizeof(homepage) / sizeof(char)) / 32;


Whatever it is, make a constant for it! Likewise with 9600, 16, 2, and 13 in the setup() function. In fact, if you did that, you wouldn't even need the comment above the call to lcd.begin(). It would just be lcd.begin(kNumLCDCols, kNumLCDRows);. In displayTemperature() you cast the value 233 to a char. Why? What does it represent? If it's the degree symbol, just give it a name.



Simplify With Types



Your Settings struct has several fields that can only have 2 values, but take up more space. I know that embedded applications are often very short on memory, so it's good to keep things small. I'm not sure if that's the case here, but you could easily make a single 8-bit field and use 1 bit for the hour format, 1 bit for language (if you know you won't ever need more languages), 1 bit for the degree format, and 1 bit for whether to show labels or not. That's 7 bytes in your current struct (though probably more due to alignment issues), compressed into a single byte.



Even if you don't do that, I think using an enum similar to the one you have for languages_t would be a good idea for the other 3 fields that only have 2 possible values.



Avoid Global Variables



You have a few dozen global variables in your code. Global variables are a pain to work with because when they get a bad value in them, it can be impossible to track down where it changed, since every part of the code has access to them. I don't see a main() function anywhere in your code, so I'm not sure how this all runs in practice. I would recommend putting all your globals in your main() function (or whatever function eventually calls through to the functions you've written here), and passing them into each function. By passing them into each function you can see the flow of data more easily when debugging.



Don't Declare Variables in Headers



In your Date.h file you declare a global variable DW. This is will cause the variable to be created in every file that #includes that header. And since there are no header guards around the file, if you include the header, and then also include another header that includes Date.h, you'll get a compile error because you'll be redeclaring the variable in the same scope.



Use if When It Would Be More Clear



You've used a ternary operator in a very hard-to-understand way in several places like this:



(w == 0) ? DW[1] = (DW[0] - 7 + 10) / 7 : DW[1] = (DW[0] - w + 10) / 7;


It would be much clearer to just write a simple if statement:



if (w == 0)

DW[1] = (DW[0] - 7 + 10) / 7;

else

DW[1] = (DW[0] - w + 10) / 7;



If you really really want to use a ternary operator for some reason, at least write it in the expected order:



DW[1] = (w == 0) ? (DW[0] - 7 + 10) / 7 : (DW[0] - w + 10) / 7;


Or even:



int subVal = (w == 0) ? 7 : w;
DW [ 1 ] = (DW[0] - subVal + 10) / 7;


Make Functions for Common Tasks



There are some long lines of code you have repeated in several places. This is another area where you have the potential for a bug in only 1 version that will be hard to track down, or you'll change one and need to update all of them. If you just put it in a function, it's easier to read and doesn't suffer from those problems. For example, your conversion from Fahrenheit to Celsius. Just make a function:



int FahrenheitToCelsius(const float celsius)

return static_cast<int>(celsius * 9 / 5 + 32);



That will likely inlined by the compiler, so it shouldn't be any less efficient.



Simplify



The function in Date.h is overly complicated. I would rewrite it like this:



void DayWeekNumber(unsigned int y, unsigned int m, unsigned int d, unsigned int w) 


Also, don't add return statements to the end of void functions. It adds nothing, as the function will return without the statement.






share|improve this answer























  • I replaced the magic numbers as you said. I simpliefied the types (as far as my knowledge reached. I don't know how I can return multiple values with different types, so I put the main code inside a mainloop but the variables are still global. I simplied the Date.h and I made some functions for common tasks. I will post the new code in a new question
    – Sebastiaan Speck
    Jan 17 at 14:25












up vote
4
down vote










up vote
4
down vote









This looks like a really fun project! It's written in a fairly straightforward way, so I think I get most of it. Here are some suggestions for improving it.



Avoid Magic Numbers



You have a lot of bare numbers in your code. Some are obvious, but some are not and there's no explanation of what they are. Furthermore, if you want to change any of them, you'll need to find all instances of them and change each instance. For example, in your constant declarations, you use the numbers 2, 7, 12 and 5. I would make constants for these, like this:



const size_t kNumSupportedLanguages = 2;
const size_t kNumDaysPerWeek = 7;
const size_t kNumMonthsPerYear = 12;
const size_t kNumLabels = 5;


I'd probably do the same for the millisecond values:



const long kOneSecInMS = 1000;
const long kThirtySecsInMS = 30000;


What does the 32 represent in this line:



int numberOfPages = (sizeof(homepage) / sizeof(char)) / 32;


Whatever it is, make a constant for it! Likewise with 9600, 16, 2, and 13 in the setup() function. In fact, if you did that, you wouldn't even need the comment above the call to lcd.begin(). It would just be lcd.begin(kNumLCDCols, kNumLCDRows);. In displayTemperature() you cast the value 233 to a char. Why? What does it represent? If it's the degree symbol, just give it a name.



Simplify With Types



Your Settings struct has several fields that can only have 2 values, but take up more space. I know that embedded applications are often very short on memory, so it's good to keep things small. I'm not sure if that's the case here, but you could easily make a single 8-bit field and use 1 bit for the hour format, 1 bit for language (if you know you won't ever need more languages), 1 bit for the degree format, and 1 bit for whether to show labels or not. That's 7 bytes in your current struct (though probably more due to alignment issues), compressed into a single byte.



Even if you don't do that, I think using an enum similar to the one you have for languages_t would be a good idea for the other 3 fields that only have 2 possible values.



Avoid Global Variables



You have a few dozen global variables in your code. Global variables are a pain to work with because when they get a bad value in them, it can be impossible to track down where it changed, since every part of the code has access to them. I don't see a main() function anywhere in your code, so I'm not sure how this all runs in practice. I would recommend putting all your globals in your main() function (or whatever function eventually calls through to the functions you've written here), and passing them into each function. By passing them into each function you can see the flow of data more easily when debugging.



Don't Declare Variables in Headers



In your Date.h file you declare a global variable DW. This is will cause the variable to be created in every file that #includes that header. And since there are no header guards around the file, if you include the header, and then also include another header that includes Date.h, you'll get a compile error because you'll be redeclaring the variable in the same scope.



Use if When It Would Be More Clear



You've used a ternary operator in a very hard-to-understand way in several places like this:



(w == 0) ? DW[1] = (DW[0] - 7 + 10) / 7 : DW[1] = (DW[0] - w + 10) / 7;


It would be much clearer to just write a simple if statement:



if (w == 0)

DW[1] = (DW[0] - 7 + 10) / 7;

else

DW[1] = (DW[0] - w + 10) / 7;



If you really really want to use a ternary operator for some reason, at least write it in the expected order:



DW[1] = (w == 0) ? (DW[0] - 7 + 10) / 7 : (DW[0] - w + 10) / 7;


Or even:



int subVal = (w == 0) ? 7 : w;
DW [ 1 ] = (DW[0] - subVal + 10) / 7;


Make Functions for Common Tasks



There are some long lines of code you have repeated in several places. This is another area where you have the potential for a bug in only 1 version that will be hard to track down, or you'll change one and need to update all of them. If you just put it in a function, it's easier to read and doesn't suffer from those problems. For example, your conversion from Fahrenheit to Celsius. Just make a function:



int FahrenheitToCelsius(const float celsius)

return static_cast<int>(celsius * 9 / 5 + 32);



That will likely inlined by the compiler, so it shouldn't be any less efficient.



Simplify



The function in Date.h is overly complicated. I would rewrite it like this:



void DayWeekNumber(unsigned int y, unsigned int m, unsigned int d, unsigned int w) 


Also, don't add return statements to the end of void functions. It adds nothing, as the function will return without the statement.






share|improve this answer















This looks like a really fun project! It's written in a fairly straightforward way, so I think I get most of it. Here are some suggestions for improving it.



Avoid Magic Numbers



You have a lot of bare numbers in your code. Some are obvious, but some are not and there's no explanation of what they are. Furthermore, if you want to change any of them, you'll need to find all instances of them and change each instance. For example, in your constant declarations, you use the numbers 2, 7, 12 and 5. I would make constants for these, like this:



const size_t kNumSupportedLanguages = 2;
const size_t kNumDaysPerWeek = 7;
const size_t kNumMonthsPerYear = 12;
const size_t kNumLabels = 5;


I'd probably do the same for the millisecond values:



const long kOneSecInMS = 1000;
const long kThirtySecsInMS = 30000;


What does the 32 represent in this line:



int numberOfPages = (sizeof(homepage) / sizeof(char)) / 32;


Whatever it is, make a constant for it! Likewise with 9600, 16, 2, and 13 in the setup() function. In fact, if you did that, you wouldn't even need the comment above the call to lcd.begin(). It would just be lcd.begin(kNumLCDCols, kNumLCDRows);. In displayTemperature() you cast the value 233 to a char. Why? What does it represent? If it's the degree symbol, just give it a name.



Simplify With Types



Your Settings struct has several fields that can only have 2 values, but take up more space. I know that embedded applications are often very short on memory, so it's good to keep things small. I'm not sure if that's the case here, but you could easily make a single 8-bit field and use 1 bit for the hour format, 1 bit for language (if you know you won't ever need more languages), 1 bit for the degree format, and 1 bit for whether to show labels or not. That's 7 bytes in your current struct (though probably more due to alignment issues), compressed into a single byte.



Even if you don't do that, I think using an enum similar to the one you have for languages_t would be a good idea for the other 3 fields that only have 2 possible values.



Avoid Global Variables



You have a few dozen global variables in your code. Global variables are a pain to work with because when they get a bad value in them, it can be impossible to track down where it changed, since every part of the code has access to them. I don't see a main() function anywhere in your code, so I'm not sure how this all runs in practice. I would recommend putting all your globals in your main() function (or whatever function eventually calls through to the functions you've written here), and passing them into each function. By passing them into each function you can see the flow of data more easily when debugging.



Don't Declare Variables in Headers



In your Date.h file you declare a global variable DW. This is will cause the variable to be created in every file that #includes that header. And since there are no header guards around the file, if you include the header, and then also include another header that includes Date.h, you'll get a compile error because you'll be redeclaring the variable in the same scope.



Use if When It Would Be More Clear



You've used a ternary operator in a very hard-to-understand way in several places like this:



(w == 0) ? DW[1] = (DW[0] - 7 + 10) / 7 : DW[1] = (DW[0] - w + 10) / 7;


It would be much clearer to just write a simple if statement:



if (w == 0)

DW[1] = (DW[0] - 7 + 10) / 7;

else

DW[1] = (DW[0] - w + 10) / 7;



If you really really want to use a ternary operator for some reason, at least write it in the expected order:



DW[1] = (w == 0) ? (DW[0] - 7 + 10) / 7 : (DW[0] - w + 10) / 7;


Or even:



int subVal = (w == 0) ? 7 : w;
DW [ 1 ] = (DW[0] - subVal + 10) / 7;


Make Functions for Common Tasks



There are some long lines of code you have repeated in several places. This is another area where you have the potential for a bug in only 1 version that will be hard to track down, or you'll change one and need to update all of them. If you just put it in a function, it's easier to read and doesn't suffer from those problems. For example, your conversion from Fahrenheit to Celsius. Just make a function:



int FahrenheitToCelsius(const float celsius)

return static_cast<int>(celsius * 9 / 5 + 32);



That will likely inlined by the compiler, so it shouldn't be any less efficient.



Simplify



The function in Date.h is overly complicated. I would rewrite it like this:



void DayWeekNumber(unsigned int y, unsigned int m, unsigned int d, unsigned int w) 


Also, don't add return statements to the end of void functions. It adds nothing, as the function will return without the statement.







share|improve this answer















share|improve this answer



share|improve this answer








edited Jan 17 at 12:28









Toby Speight

17.8k13491




17.8k13491











answered Jan 17 at 6:22









user1118321

10.2k11144




10.2k11144











  • I replaced the magic numbers as you said. I simpliefied the types (as far as my knowledge reached. I don't know how I can return multiple values with different types, so I put the main code inside a mainloop but the variables are still global. I simplied the Date.h and I made some functions for common tasks. I will post the new code in a new question
    – Sebastiaan Speck
    Jan 17 at 14:25
















  • I replaced the magic numbers as you said. I simpliefied the types (as far as my knowledge reached. I don't know how I can return multiple values with different types, so I put the main code inside a mainloop but the variables are still global. I simplied the Date.h and I made some functions for common tasks. I will post the new code in a new question
    – Sebastiaan Speck
    Jan 17 at 14:25















I replaced the magic numbers as you said. I simpliefied the types (as far as my knowledge reached. I don't know how I can return multiple values with different types, so I put the main code inside a mainloop but the variables are still global. I simplied the Date.h and I made some functions for common tasks. I will post the new code in a new question
– Sebastiaan Speck
Jan 17 at 14:25




I replaced the magic numbers as you said. I simpliefied the types (as far as my knowledge reached. I don't know how I can return multiple values with different types, so I put the main code inside a mainloop but the variables are still global. I simplied the Date.h and I made some functions for common tasks. I will post the new code in a new question
– Sebastiaan Speck
Jan 17 at 14:25












 

draft saved


draft discarded


























 


draft saved


draft discarded














StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f185229%2fdigital-clock-with-many-functions%23new-answer', 'question_page');

);

Post as a guest













































































Popular posts from this blog

Chat program with C++ and SFML

Function to Return a JSON Like Objects Using VBA Collections and Arrays

Will my employers contract hold up in court?