2
\$\begingroup\$

I've created a program to read temperature from 4 sensors, get an average and close/open 4 relays according to min/max values. Right now I'm trying to add a 16x2 LCD display to show those temperatures live instead of having to connect the Arduino to the serial bus and I wrote this:

// Wire - Version: Latest - Serial bus I2C communication Library
#include <Wire.h>

// Newliquidcrystal_1.3.5 - Version: Latest // LCD Library. Downloaded from https://bitbucket.org/fmalpartida/new-liquidcrystal/downloads/
#include <LiquidCrystal_I2C.h>

LiquidCrystal_I2C lcd(0x27, 2, 1, 0, 4, 5, 6, 7, 3, POSITIVE);  // Set the LCD I2C address

const byte tempPin[] = {A1, A2, A5, A4}; //Analog Pins. defining the LM35 temp sensors input pins as constant byte to avoid changing it. Value from 0 to 255
const byte relayPin[] = {6, 7, 8, 9}; //Digital Pins. defining the relay output pins as constant byte to avoid changing it. Value from 0 to 255

// hysteresis = upperLimit - lowerLimit
const byte lowerLimit = 28;
const byte upperLimit = 32;

float tempC[4]; //creating the 4 temp inputs inside an array. Float to deal with decimals
const int numReadings = 25; // defining the number of readings to create an average with
word printInterval = 1000; // One second minimum print delay value
unsigned long lastPrintTime = 0; //this is set to unsigned long because it will equal millis() which always returns an unsigned long value
byte row = 0; //Row value for the LCD display
byte column = 0; //Column value for the LCD display


void setup()
{
  analogReference(INTERNAL); //Using the internal 1.1v reference for better resolution on the analog pin readings
  Serial.begin(115200); // Setting 115200 baud ratio for communication
  for (int i = 0; i < 4; i++) { //starting with i = 0 and as long as i is less than 4, go through the loop and then add 1 to i value
    digitalWrite(relayPin[i], HIGH); // For most optocoupler relays, HIGH means Open and Low means Closed. In here I initially set the relays to HIGH/Open to avoid a short LOW/Closed period before the loop starts.
    pinMode(relayPin[i], OUTPUT); // sets the relay pins as outputs, defaults state HIGH, relay OFF
  }
  lcd.begin(16, 2); // Sets the size of the LCD display in columns and rows
  lcd.clear(); // Moves the cursor to 0,0
}

void loop()
{
  // readings and control
  for (int i = 0; i < 4; i++) {
    float raw_temp = analogRead(tempPin[i]) / 9.31; // 1024 analog resolution. 1.1 Internal Voltage reference = 1.0742 millivolts for each analog step. LM35 1 degree Celsius per 10 millivolts. 10/1.0742 = one degree change every 9.31.
    tempC[i] += (raw_temp - tempC[i])  / numReadings; //Exponential moving average. This means each loop, the TempC will be corrected by the average of the last 25 values, which are condensed in the TempC value from the previous loop.
    if (tempC[i] < lowerLimit) {
      digitalWrite(relayPin[i], LOW);   //relay ON
    } //If the TempC value is lower than the const lowerlimit value, the relay will close and turn on the heatpads
    else if (tempC[i] > upperLimit) {
      digitalWrite(relayPin[i], HIGH);  // relay OFF
    } //If the TempC value is higher than the const upperlimit value, the relay will close and turn on the heatpads
  }

  if (millis() - lastPrintTime >= printInterval) { //If the difference between the time the program has been running and the last time the program printed the values is bigger than printInterval value, it will print again. A fancy print delay
    for (int i = 0; i < 4; i++) {
      Serial.print("tempC");
      Serial.print(i + 1); // Just to avoid showing TempC number 0 due array indexing
      Serial.print(" ");
      Serial.println(tempC[i]);

      lcd.setCursor(column,row);
      lcd.print("T");
      column++;
      lcd.setCursor(column,row);
      lcd.print(i + 1);
      column+=2;
      lcd.setCursor(column,row);
      lcd.print(tempC[i],1); //prints the float value with only 1 decimal
      column++;
      if (i >= 2 ) //If the TempC array index is 2 or 3 (last two TempC Values), this if starts
      {
        if ( i == 2) //If the TempC array index is 2 (first TempC Value), the column value will be reseted to 0 to start from the first line
        {
         column = 0;   
        }
       row = 1;
      }
    }


    Serial.println();
    lastPrintTime = millis(); // sets LastPrintTime to the current program running time during that loop (millis)

  }
}

It works, But I think the code is messy and needs optimization. However I'm struggling to find how and where I can optimize the code, and which parts can become functions. I'm a beginner programmer so i'm still trying to understand when, where and how to use functions. Can you give me a hand? Thanks in advance!

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Do you have the source and/or documentation for Wire.h and LiquidCrystal_I2C.h? If so please post it to help reviewers understand what you are using from those headers. \$\endgroup\$
    – Null
    Apr 20 '18 at 13:40
  • \$\begingroup\$ Oh, those two are libraries for controlling and communication between the Arduino Board and the LCD 16x2 Display. Let me edit the code with comments \$\endgroup\$ Apr 20 '18 at 13:43
  • \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to give it a different title if there is something more appropriate. \$\endgroup\$ Apr 20 '18 at 14:11
1
\$\begingroup\$

This looks like an interesting project! I've never had a chance to work with such peripherals. Here are some things that I see:

Comments

There are way too many comments. Many of them are completely unnecessary, like this:

for (int i = 0; i < 4; i++) { //starting with i = 0 and as long as i is less than 4, go through the loop and then add 1 to i value

Any reader of your code should already know what a for loop does. There's no need to comment on it.

Some of your other comments give good information like this one:

// 1024 analog resolution. 1.1 Internal Voltage reference = 1.0742 millivolts for each analog step. LM35 1 degree Celsius per 10 millivolts. 10/1.0742 = one degree change every 9.31.

The problem is that it makes the code harder to read because of where it is. I recommend making a constant for 9.31 and putting the comment where the constant is created rather than in the code which uses it. I think it could even be shortened by removing the 1024 analog resolution since it's not clear what relevance that has.

Avoid Magic Numbers

You have dozens of magic numbers in your code. Values like 115200, 16, and 2 should have names like kSensorDataRate, kNumLCDColumns, and kNumLCDRows, respectively. This goes for just about every bare number in your code. (Usually 1s and 0s are self explanatory.)

In particular, you're assuming that you'll only ever have 4 temperature readings. What if you buy a new sensor in the future and it allows 8 readings? Right now you have to go through your entire program and change every 4 to an 8. If you used a named constant like kMaxTempReadings to define array sizes and loop end conditions, you'd only have to change a single value to update it.

Naming

Why is the loop() function named loop? It has 2 loops in it, but its purpose is not to loop. Its purpose appears to be to print out the temperature readings to the LCD display. So why not call it something like printTempsToLCD()?

Simplify

This if statement is a little confusing to read:

  if (i >= 2 ) //If the TempC array index is 2 or 3 (last two TempC Values), this if starts
  {
    if ( i == 2) //If the TempC array index is 2 (first TempC Value), the column value will be reseted to 0 to start from the first line
    {
     column = 0;   
    }
   row = 1;
  }

It looks like you're resetting the column and row when you reach the end of the first row. You could simplify it and also make it work with a wider display by doing this:

column++;
if (column >= kMaxLCDColumns) 
{
    row = (row + 1) % kMaxLCDRows;
}
column = column % kMaxLCDColumns;

Now both values get set appropriately on every loop iteration, and this will continue to work if you have a wider or short LCD display.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you for your reply. Will do on the comments Great advice on the "magic numbers". However wouldn't declaring so many constants use more memory space? Arduino requires two main functions to work. setup() and loop(), so sadly I can't change that. I changed the if statement for a function: code void lcdreading(byte i, byte column, byte row){ lcd.setCursor(column,row); //16 columns x 2 rows on a 16x2 LCD display lcd.print("T"); lcd.print(i+1); lcd.print(" "); lcd.print(tempC[i],1); //temp reading, 1 decimal }code \$\endgroup\$ Apr 23 '18 at 12:07
  • \$\begingroup\$ For constants taking up space - it may depend on the architecture that you're compiling to. I believe most modern CPU architectures have immediate instructions where the constant loaded ends up being part of the instruction or an argument to the instruction, so it takes no additional memory. I'm not sure whether that applies in this case, but I would be surprised if it made much difference. Good idea on making the if into a function! \$\endgroup\$ Apr 23 '18 at 17:54
  • \$\begingroup\$ See the Parts of an Instruction section of the Wikipedia page on instruction set architecture for a description of immediate values. In particular, the image to the right shows an example. \$\endgroup\$ Apr 23 '18 at 17:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.