Advice for my code

Hi friends, I want to realize an automation for my aquarium. I want to check

  1. The illumination
  2. The temperature
  3. And then other sensors for some parameters, but this part I will make it later.

I have written this code, can you check it? Before that I load it in Arduino I'd like to know your opinion. Thanks.

#include <Wire.h>
#include <LiquidCrystal.h>

LiquidCrystal lcd(12,11,10,4,3,2);

int rele_out1 = 5;
int rele_out2 = 6;
int pulsantepiu = 0;
int pulsantemeno = 1;
int pulsanteok = 7;
int pulsantenext = 8;
int tmax;
int temperatura;
int piu=0;
int meno=0;
int ok=0;
int next=0;
int posizione=0;
int secondi;
int minuti;
int ore;
int seconds1;
int minutes1;
int hours1;
int seconds2;
int minutes2;
int hours2;
char* menu_principale[6] = {"Pagina iniziale","Temperatura max","Temperatura min","Orario on","Orario off","Imposta ora"};

byte grado[8] = {
0b00010,
0b00101,
0b00010,
0b00000,
0b00000,
0b00000,
0b00000,
0b00000,
};

void setup(){
Wire.begin();
Serial.begin(9600);
temperatura = analogRead(A3)*0.48828125; //Coversione temperatura rilevata dal sensore LM35
lcd.createChar(0, grado);
lcd.begin(16,2);
lcd.print( menu_principale[posizione] );
pinMode(rele_out1, OUTPUT);
pinMode(rele_out2, OUTPUT);
pinMode(pulsantepiu, INPUT);
pinMode(pulsantemeno, INPUT);
pinMode(pulsanteok, INPUT);
pinMode(pulsantenext, INPUT);
secondi = 00;
minuti = 26;
ore = 17;
tmax=1;
seconds1 = 00;
minutes1 = 00;
hours1 = 00;
seconds2 = 00;
minutes2 = 00;
hours2 = 00;
}

void loop(){
get_time();
display_time();
change_menu();
check_temp();
}

void change_menu(){
next=digitalRead(pulsantenext);
if(next == HIGH){
posizione = posizione ++;
if(posizione == 1){
display_time();
display_temp();
}
else if(posizione == 2){
chose_temp();
led_on();
}
} else if(posizione == 3){
} else if(posizione == 4){
setMinutes_on();
setHour_on();
set_time_on();
led_on();
} else if(posizione == 5){
setMinutes_off();
setHour_off();
set_time_off();
led_off();
} else if(posizione == 6){
setMinutes();
setHour();
set_time();
} else if(posizione > 6){
posizione == 1;
}
}

void led_on(){
if(seconds1 == secondi && minutes1 == minuti && hours1 == ore){
lcd.clear();
lcd.print("Led on!");
digitalWrite(rele_out2, HIGH);
} else{
digitalWrite(rele_out2, LOW);
}
}

void led_off(){
if(seconds2 == secondi && minutes2 == minuti && hours2 == ore){
lcd.clear();
lcd.print("Led off!");
digitalWrite(rele_out2, LOW);
} else{
digitalWrite(rele_out2, HIGH);
}
}

void chose_temp(){
piu = digitalRead(pulsantepiu);
meno = digitalRead(pulsantemeno);
ok = digitalRead(pulsanteok);
if (piu == HIGH){
tmax = tmax ++;
lcd.clear();
lcd.print("Temperatura di");
lcd.setCursor(0, 1);
lcd.print("soglia: ");
lcd.setCursor(10, 1);
lcd.print(tmax,DEC);
lcd.write((byte)0);
lcd.print("C");
delay(1000);
}
if (meno == HIGH){
tmax = tmax --;
lcd.clear();
lcd.print("Temperatura di");
lcd.setCursor(0, 1);
lcd.print("soglia: ");
lcd.setCursor(10, 1);
lcd.print(tmax,DEC);
lcd.write((byte)0);
lcd.print("C");
delay(1000);
}
}

void check_temp(){
if(temperatura > (tmax + 1)){
lcd.clear();
lcd.print("Termostato spento!");
digitalWrite(rele_out1, LOW);
} else{
digitalWrite(rele_out1, HIGH);
}
}

void display_temp(){
lcd.setCursor(0,2);
lcd.print("Temp: ");
lcd.print(temperatura, DEC);
}

void display_time(){
char buf[12];

lcd.setCursor(0, 0);

if(ore == 0)
{
lcd.clear();
}

lcd.print("Orario ");
lcd.print(itoa(ore, buf, 10));
lcd.print(":");

if(minuti < 10)
{
lcd.print("0");
}
lcd.print(itoa(minuti, buf, 10));

lcd.print(":");

if(secondi < 10){
lcd.print("0");
}
lcd.print(itoa(secondi, buf, 10));

}

void initChrono()
{
set_time();
set_time_on();
set_time_off();
}

void set_time()
{
Wire.beginTransmission(104);
Wire.write(0);
Wire.write(decToBcd(secondi));
Wire.write(decToBcd(minuti));
Wire.write(decToBcd(ore));
Wire.endTransmission();
}

void get_time()
{
Wire.beginTransmission(104);
Wire.write(0);//set register to 0
Wire.endTransmission();
Wire.requestFrom(104, 3);//get 3 bytes (seconds,minutes,hours);
secondi = bcdToDec(Wire.read() & 0x7f);
minuti = bcdToDec(Wire.read());
ore = bcdToDec(Wire.read() & 0x3f);
}

void set_time_on()
{
Wire.beginTransmission(104);
Wire.write(0);
Wire.write(decToBcd(seconds1));
Wire.write(decToBcd(minutes1));
Wire.write(decToBcd(hours1));
Wire.endTransmission();
}

void get_time_on()
{
Wire.beginTransmission(104);
Wire.write(0);//set register to 0
Wire.endTransmission();
Wire.requestFrom(104, 3);//get 3 bytes (seconds,minutes,hours);
seconds1 = bcdToDec(Wire.read() & 0x7f);
minutes1 = bcdToDec(Wire.read());
hours1 = bcdToDec(Wire.read() & 0x3f);
}

void set_time_off()
{
Wire.beginTransmission(104);
Wire.write(0);
Wire.write(decToBcd(seconds2));
Wire.write(decToBcd(minutes2));
Wire.write(decToBcd(hours2));
Wire.endTransmission();
}

void get_time_off()
{
Wire.beginTransmission(104);
Wire.write(0);//set register to 0
Wire.endTransmission();
Wire.requestFrom(104, 3);//get 3 bytes (seconds,minutes,hours);
seconds2 = bcdToDec(Wire.read() & 0x7f);
minutes2 = bcdToDec(Wire.read());
hours2 = bcdToDec(Wire.read() & 0x3f);
}

void setHour()
{
ore++;
if(ore > 23)
{
ore = 0;
secondi = 0;
minuti = 0;
}
set_time();
}

void setMinutes()
{
minuti++;
if(minuti > 59)
{
minuti = 0;

}
secondi=0;

set_time();
}

void setHour_on()
{
hours1++;
if(hours1 > 23)
{
hours1 = 0;
seconds1 = 0;
minutes1 = 0;
}
set_time_on();
}

void setMinutes_on()
{
minutes1++;
if(minutes1 > 59)
{
minutes1 = 0;

}
seconds1 = 0;

set_time_on();
}

void setHour_off()
{
hours2++;
if(hours2 > 23)
{
hours2 = 0;
seconds2 = 0;
minutes2 = 0;
}
set_time_off();
}

void setMinutes_off()
{
minutes2++;
if(minutes2 > 59)
{
minutes2 = 0;

}
seconds2 = 0;

set_time_off();
}

byte decToBcd(byte val)
{
return ( (val/10*16) + (val%10) );
}

byte bcdToDec(byte val)
{
return ( (val/16*10) + (val%16) );
}

Please use code tags, not quote tags.

Initial glance through it:

  1. Lose some of the ints. If you aren't storing anything outside the range 0-255 you should use a byte not an int.
  2. Make your pin definitions const to save RAM.
  3. Be wary of using leading 0's for your numbers, they signify octal, not decimal. 00 is ok as 0 is 0 in every base, but 08 and 09 are not numbers, and 010 is not 10, but 8.

Thank you for your advices, but I don't understand your first point :cold_sweat:..
Can you explain me please? These are my changes:

#include <Wire.h>  
#include <LiquidCrystal.h>

LiquidCrystal lcd(12,11,10,4,3,2);

const int rele_out1 = 5;
const int rele_out2 = 6;
const int pulsantepiu = 0;
const int pulsantemeno = 1;
const int pulsanteok = 7;
const int pulsantenext = 8;
int tmax;
int temperatura;
int piu = 00;
int meno = 00;
int ok = 00;
int next = 00;
int posizione = 00;
int secondi;
int minuti;
int ore;
int seconds1;
int minutes1;
int hours1;
int seconds2;
int minutes2;
int hours2;
char* menu_principale[6] = {"Pagina iniziale","Temperatura max","Temperatura min","Orario on","Orario off","Imposta ora"};

byte grado[8] = {     
  0b00010,
  0b00101,
  0b00010,
  0b00000,
  0b00000,
  0b00000,
  0b00000,
  0b00000,
  };

void setup(){
  Wire.begin();          
  Serial.begin(9600);
  temperatura = analogRead(A3)*0.48828125;  //Coversione temperatura rilevata dal sensore LM35
  lcd.createChar(0, grado);
  lcd.begin(16,2);
  lcd.print( menu_principale[posizione] );
  pinMode(rele_out1, OUTPUT);
  pinMode(rele_out2, OUTPUT);
  pinMode(pulsantepiu, INPUT);
  pinMode(pulsantemeno, INPUT);
  pinMode(pulsanteok, INPUT);
  pinMode(pulsantenext, INPUT);
  secondi = 00;
  minuti = 26;
  ore = 17;
  tmax = 01;
  seconds1 = 00;
  minutes1 = 00;
  hours1 = 00;
  seconds2 = 00;
  minutes2 = 00;
  hours2 = 00;
  }
  
void loop(){
  get_time();
  display_time();
  change_menu();
  check_temp();
  }

void change_menu(){
  next=digitalRead(pulsantenext);
  if(next == HIGH){
    posizione = posizione ++;
        if(posizione == 1){
          display_time();
          display_temp();
        }
        else if(posizione == 2){
               chose_temp();
               led_on();
      }
      } else if(posizione == 3){
      } else if(posizione == 4){
               setMinutes_on();
               setHour_on();
               set_time_on();
               led_on();
      } else if(posizione == 5){
               setMinutes_off();
               setHour_off();
               set_time_off();
               led_off();
      } else if(posizione == 6){
               setMinutes();
               setHour();
               set_time();
      } else if(posizione > 6){
                posizione == 1;
                }        
}

void led_on(){
  if(seconds1 == secondi && minutes1 == minuti && hours1 == ore){
    lcd.clear();
    lcd.print("Led on!");
    digitalWrite(rele_out2, HIGH);
    } else{
      digitalWrite(rele_out2, LOW);
  }
}

void led_off(){
  if(seconds2 == secondi && minutes2 == minuti && hours2 == ore){
    lcd.clear();
    lcd.print("Led off!");
    digitalWrite(rele_out2, LOW);
    } else{
      digitalWrite(rele_out2, HIGH);
  }
}

void chose_temp(){
  piu = digitalRead(pulsantepiu);
      meno = digitalRead(pulsantemeno);
      ok = digitalRead(pulsanteok);
      if (piu == HIGH){
        tmax = tmax ++;
        lcd.clear();
        lcd.print("Temperatura di");
        lcd.setCursor(0, 1);
        lcd.print("soglia: ");
        lcd.setCursor(10, 1);
        lcd.print(tmax,DEC);
        lcd.write((byte)0);
        lcd.print("C");
        delay(1000);
        }
      if (meno == HIGH){
        tmax = tmax --;
        lcd.clear();
        lcd.print("Temperatura di");
        lcd.setCursor(0, 1);
        lcd.print("soglia: ");
        lcd.setCursor(10, 1);
        lcd.print(tmax,DEC);
        lcd.write((byte)0);
        lcd.print("C");
        delay(1000);
        }
      }
      
void check_temp(){
  if(temperatura > (tmax + 1)){
    lcd.clear();
    lcd.print("Termostato spento!");
    digitalWrite(rele_out1, LOW);
    } else{
      digitalWrite(rele_out1, HIGH);
}
}

void display_temp(){
  lcd.setCursor(0,2);
  lcd.print("Temp: ");
  lcd.print(temperatura, DEC);
  }
  
void display_time(){
  char buf[12];
 
  lcd.setCursor(0, 0);
 
  if(ore == 0)
  {
    lcd.clear();
  }
 
  lcd.print("Orario ");  
  lcd.print(itoa(ore, buf, 10));
  lcd.print(":");
 
  if(minuti < 10)
  {
    lcd.print("0");
  }
  lcd.print(itoa(minuti, buf, 10));
 
  lcd.print(":");
 
  if(secondi < 10){
    lcd.print("0");
  }
  lcd.print(itoa(secondi, buf, 10));

}

void initChrono()
{
  set_time();
  set_time_on();
  set_time_off();
}

void set_time()
{
   Wire.beginTransmission(104);
   Wire.write(0);
   Wire.write(decToBcd(secondi));
   Wire.write(decToBcd(minuti));
   Wire.write(decToBcd(ore));
   Wire.endTransmission();
}

void get_time()
{
  Wire.beginTransmission(104);
  Wire.write(0);//set register to 0
  Wire.endTransmission();
  Wire.requestFrom(104, 3);//get 3 bytes (seconds,minutes,hours);
  secondi = bcdToDec(Wire.read() & 0x7f);
  minuti = bcdToDec(Wire.read());
  ore = bcdToDec(Wire.read() & 0x3f);
}

void set_time_on()
{
   Wire.beginTransmission(104);
   Wire.write(0);
   Wire.write(decToBcd(seconds1));
   Wire.write(decToBcd(minutes1));
   Wire.write(decToBcd(hours1));
   Wire.endTransmission();
}

void get_time_on()
{
  Wire.beginTransmission(104);
  Wire.write(0);//set register to 0
  Wire.endTransmission();
  Wire.requestFrom(104, 3);//get 3 bytes (seconds,minutes,hours);
  seconds1 = bcdToDec(Wire.read() & 0x7f);
  minutes1 = bcdToDec(Wire.read());
  hours1 = bcdToDec(Wire.read() & 0x3f);
}

void set_time_off()
{
   Wire.beginTransmission(104);
   Wire.write(0);
   Wire.write(decToBcd(seconds2));
   Wire.write(decToBcd(minutes2));
   Wire.write(decToBcd(hours2));
   Wire.endTransmission();
}

void get_time_off()
{
  Wire.beginTransmission(104);
  Wire.write(0);//set register to 0
  Wire.endTransmission();
  Wire.requestFrom(104, 3);//get 3 bytes (seconds,minutes,hours);
  seconds2 = bcdToDec(Wire.read() & 0x7f);
  minutes2 = bcdToDec(Wire.read());
  hours2 = bcdToDec(Wire.read() & 0x3f);
}

void setHour()
{
  ore++;
  if(ore > 23)
  {
   ore = 0;
   secondi = 0;
   minuti = 0;
  }
  set_time(); 
}

void setMinutes()
{
  minuti++;  
  if(minuti > 59)
  {
   minuti = 0;
   
  }
  secondi=0;
 
  set_time(); 
}

void setHour_on()
{
  hours1++;
  if(hours1 > 23)
  {
   hours1 = 0;
   seconds1 = 0;
   minutes1 = 0;
  }
  set_time_on();
}

void setMinutes_on()
{
  minutes1++;  
  if(minutes1 > 59)
  {
   minutes1 = 0;
   
  }
  seconds1 = 0;
 
  set_time_on();
}

void setHour_off()
{
  hours2++;
  if(hours2 > 23)
  {
   hours2 = 0;
   seconds2 = 0;
   minutes2 = 0;
  }
  set_time_off();
}

void setMinutes_off()
{
  minutes2++;  
  if(minutes2 > 59)
  {
   minutes2 = 0;
   
  }
  seconds2 = 0;
 
  set_time_off();
}

byte decToBcd(byte val)
{
  return ( (val/10*16) + (val%10) );
}

byte bcdToDec(byte val)
{
  return ( (val/16*10) + (val%16) );
}

What majenko means is that a byte can store a number from 0 to 255, while an int stores up to 32767. An int uses twice as much space as a byte, so if you don't need numbers over 255, use bytes and save space.

Okok thank you, I have changed all "int" with "byte" :wink: because it has to assume a value less 25.
Are there other advice? Can you check the function and sub-function too, please? :slight_smile:

int meno = 00;

Expressing constants in octal can be a bad habit to get into.
Fortunately, zero isn't affected.

I have written this code, can you check it? Before that I load it in Arduino I'd like to know your opinion.

I am very surprised that you wrote all this and appear not to have tried running it. If that really is the case then I would suggest testing some of the functionality in isolation to make sure that it works.

You do not appear to be using the menu_principale array apart from its first entry. Are you planning to extend the program further ?

You have no code for the case when posizione equals 3, but I suspect that was deliberate.

    posizione = posizione ++;This is an odd way to increment a variable when    posizione++;would be simpler and less prone to misinterpretation.

On a personal note, I find switch/case easier to read and understand rather than several else if combinations but you may feel differently.

Hi, I was about to mention the ++ thing and the switch case but Bob beat me to it!

So well done! As Bob says, if you try to run it as-is, it may be difficult to track down any bugs. If you have those problems, comment out as much code as you can to help find each bug.

There is already a degrees symbol in the character set of these lcd displays, it is code 0xDF, so no need to define your own. Strictly speaking, its considered more correct to omit the degrees symbol altogether these days and just show "25C".

It appears to me that many of your functions are very similar to each other. For example, there are get_time(), get_time_on() and get_time_0ff(). Is this an error? If not, all 3 functions could be replaced with a single copy:

void get_time(byte &seconds, byte &minutes, byte &hours)
{
  Wire.beginTransmission(104);
  Wire.write(0);//set register to 0
  Wire.endTransmission();
  Wire.requestFrom(104, 3);//get 3 bytes (seconds,minutes,hours);
  seconds = bcdToDec(Wire.read() & 0x7f);
  minutes = bcdToDec(Wire.read());
  hours = bcdToDec(Wire.read() & 0x3f);
}

Note the "&" symbols against the parameters. These make the parameters act as "references" to variables used when the function is called. So you can replace "get_time()" with "get_time(secondi, minuti, ore)" and replace "get_time_on()" with "get_time(seconds1, munutes1, hours1)" and so on.

Paul

Okok thank you for your advice. Yes I want to extend my project in the future and I have to study like I can use "switch case".
Ok I shall examine in depth your advice PaulRB.