Need help with cleaning up my code and few other things.

Okay first off I’m new to this whole thing like less then a week old, but willing to learn.

I have made a setup with 2 thermistors and a DHT11. One thermistor is on a mini board and covered by a small Tupperware bowl to keep the moving air from ceasing big jumps. The other is on the main big board with the 16*2 LCD that one is not covered. The DHT11 is plugged strait into the Mega 2650 pins both data and the power.

Be that as it may it works very well. What I would like is if some of you could look at my code and help me get it organized better.

The 2nd thing is to see if you could help show me how to make it work with the millis() function and help me understand how it works and why it works so that I can better use it from here on in.

3rd I wanted to see if it’s possible to make a custom library that hold a lot of the stuff in this code that really should be function calls like in a real app would do. If so could you please point me to some good places to start reading on how to go about doing that?

#include <DHT.h>
#include <DHT_U.h>

#define DHTPIN 8
#define DHTTYPE DHT11
DHT dht(DHTPIN, DHTTYPE);

#include <LiquidCrystal.h>

int tim = 50;                       //the value of delay time
// initialize the library with the numbers of the interface pins
LiquidCrystal lcd(4, 6, 10, 11, 12, 13); // set the pins for the LCD
byte customChar[] = { //this is where I put my hand made deg symbol I made
  B00100,
  B01010,
  B01010,
  B00100,
  B00000,
  B00000,
  B00000,
  B00000
};
void getTemp(float * t)
{

  // Converts input from a thermistor voltage divider to a temperature value.
  // The voltage divider consists of thermistor Rt and series resistor R0.
  // The value of R0 is equal to the thermistor resistance at T0.
  // You must set the following constants:
  //                  adcMax  ( ADC full range value )
  //                  analogPin (Arduino analog input pin)
  //                  invBeta  (inverse of the thermistor Beta value supplied by manufacturer).
  // Use Arduino's default reference voltage (5V or 3.3V) with this module.
  //

  const int analogPin = A0; // analog pin for fist thermistor
  const int analogPin2 = A1; //analog pin for 2nd thermistor
  const float invBeta = 1.00 / 3470;   // This is what my datasheat for my thermistor's says what it's beta is

  const  float adcMax = 1024.00; // I know that most say that 1023 is the right setting for this but in the Arduino ref says "The default analogRead() resolution for these boards is 10 bits" so that is why I used the 1024 here
  const float invT0 = 1.00 / 298.15;   // room temp in Kelvin

  int adcVal, i, numSamples = 50; // sets how many reading to get from the thermistor before AVG it to stop big jumps
  float  K, C, F;
  int adcVal2, x, numSamples2 = 50; // sets how many reading to get from the thermistor before AVG it to stop big jumps
  float  K2, C2, F2;

  adcVal = 0;
  adcVal2 = 0;
  for (i = 0; i < numSamples; i++)
  {
    adcVal = adcVal + analogRead(analogPin);
    delay(100);
  }
  for (x = 0; x < numSamples; x++)
  {
    adcVal2 = adcVal2 + analogRead(analogPin2);
    delay(100);
  }
  adcVal = adcVal / 50;
  adcVal2 = adcVal2 / 50;
  K = 1.00 / (invT0 + invBeta * (log ( adcMax / (float) adcVal - 1.00)));
  K2 = 1.00 / (invT0 + invBeta * (log ( adcMax / (float) adcVal2 - 1.00)));
  //  C = K - 273.15;                      // convert to Celsius
  //  F = ((9.0 * C) / 5.00) + 32.00; // convert to Fahrenheit
  F = (K - 273.15) * 9.00 / 5.00 + 32.00; // Just coverted from Kelvin to Farenheit. I don't need Celsius
  F2 = (K2 - 273.15) * 9.00 / 5.00 + 32.00; // Just coverted from Kelvin to Farenheit. I don't need Celsius
  t[2] = (F + F2) / 2.00; // Doing avg on the two thermistors
  t[2] = F2; // Storing the avg in a new varable
  return;
}

void setup()
{
  lcd.begin(16, 2);    // set up the LCD's number of columns and rows:
  lcd.createChar(0, customChar); // Making the custum Charator I made
  lcd.clear();         //Clears the LCD screen and positions the cursor in the upper-left corner
  analogReference(DEFAULT); // used for the thermistors
  Serial.begin(9600);
  dht.begin(); // To get the DHT11 up and going
}

void loop()
{
  float temp[3];
  getTemp(temp);
  float h = dht.readHumidity(); // Reads the DHT11's Humidity
  float t = dht.readTemperature(true); // Reads the DHT's Temp in Farenheit
  float f = (temp[2] + t) / 2; // set a new varable of f to hold the AVG of the AVG of the thermistors and the DHT11's Temp
  float hi = dht.computeHeatIndex(f, h); // have the DHT libary take the varable t and the varable h to get the Feels like or Heat Index
  lcd.setCursor(2, 0); // set the cursor to column 2, line 0
  lcd.print("Temp: "); // prints "Temp: " to the LCD
  lcd.print(f, 1); // prints varable "f" with one point ie 75.5 where the varable may hold 75.53 to LCD
  lcd.write((byte)0); // prints the custum Charator I made to LCD
  lcd.print("F"); // Prints "F" to LCD
  lcd.setCursor(0, 1); // set the to column 0, line 1
  lcd.print("RH: "); // Prints "RH: " to LCD
  lcd.print(h, 0); // Prints varable "h" with not point as the number never has any point numbers to LCD
  lcd.print("%"); // Prints "%" to LCD
  lcd.print(" HI: "); //Prints " HI: " to LCD
  lcd.print(hi, 1); //Prints varable hi with a 1 point res to LCD

  // Serial output so that it can be logged to a text file on deasktop and then be goton over http to use with tasker on my andriod tablet
  Serial.print(f, 1); // prints varable "f" with 1 point place ie 75.54 now shows as 75.5
  Serial.print(","); // prints a "," betwen outputs on the same line
  //  Serial.println(t, 1); // just here for trouble shooting
  Serial.print(h, 0); // prints varable "h" with a whole number no point places ie 44.00 now it just 44
  Serial.print(","); // prints a "," betwen outputs on the same line
  Serial.println(hi, 1); // prints vabale "hi" with one point place ie 75.54 now it just 75.5
  delay(1500); // to give the DHT11 time to get it's next reading before relooping
  return;
}

Thank you for your time and patience with me as I learn and you all teach me.

Welcome to the forums!

+1 karma for actually using code tags on your very first post!

As for your code, you only have 1 other function besides setup() and loop(). Not a lot to optimize. You are declaring an array of 3 temperatures in your

float temp[3]

and passing this reference to getTemp(), but you only every reference the 3rd element. If you never use the first 2 elements, why the array?

As for point #2, look at the BlinkWithoutDelay example (file->examples->02.Digital->BlinkWithoutDelay). Is there a reason why you need to use this?

As for point #3 - don't worry about this as a beginner. Worry about it 5 years from now :)

Do you have programming experience in C++ an other language ? Then you have to read the online documentation a lot what the Arduino functies do. If you have no programming experience then try to copy from good examples (you can ask us which are good examples and which are bad examples).

I will try to put the most important things first:

Your main problem is the DHT11 itself. The DHT11 and DHT22 are not accurate.

There is something going on with ‘int’ and ‘float’.

const float invBeta = 1.00 / 3470;

The compiler sees the “1.00” as a float number, but the “3470” as a integer. I like to be clear and would use “3470.0”, so the compiler sees that as a float as well.

adcVal = 0;
for (i = 0; i < numSamples; i++)
{
  adcVal = adcVal + analogRead(analogPin);
  delay(100);
}

Ouch! the ‘adcVal’ is a integer and that is 16-bits on a Arduino Mega 2560. The ‘numSamples’ is 50. When the analogRead() returns the maximum value of 1023, then it will not fit in a signed integer.
You have to make the adcVal a ‘long’ or ‘unsigned long’, or limit the ‘numSamples’ to 30. Add comment why you use a ‘long’ or why you limit the ‘numSamples’ to 30.

The noise for the analog input is often high frequency noise. You may remove the delay(100) completely. If you want to remove the 50Hz/60Hz mains noise, then you might need a lot more samples. You could also add a low-pass filter in software.

adcVal = adcVal / 50;

When you take many samples, you get extra information. By dividing it with integers, the extra information is lost.
There is a way to keep that extra information by converting the total of ‘adcVal’ to float and divide it by a float number of 50.0, after that use only floats. Example: averageRead().

No one uses ‘return’ at the end of the loop(), even though it is not wrong. You may also remove it from the other function. When nothing is returned, the return value is ‘void’ and the ‘return’-statement is not used.

When you want to try this sketch in a year, it is helpful when you have a header with comment that explains the version of the Arduino IDE, and the used libraries and where they come from. Sometimes there are ten different libraries with the same name (for example LiquidCrystal_I2C). Then it would be useful to know which one you use instead trying all ten of them.

As blh64 already wrote, use the Blink Without Delay example if you want to become an advanced Arduino programmer. For example to check the sensor every 2 seconds. You might even update the display at an other rate (with another software-millis-timer) if you want to display more things. Those displays are slow, perhaps 4 times per second is fast enough.

Don't put code in loop(), write functions that do specific things and call the functions from loop(). In your loop it looks to me like you do 3 things, you get data (temperature etc), you do some calculations and you display stuff. Split this into 3 functions (doesn't have to be 3, whatever seems natural to you) with meaningful names and call the functions from loop().

PerryBebbington: Don't put code in loop(), write functions that do specific things and call the functions from loop().

And whether or no you do that, but especially if you do and end up with loads of functions, it's a good idea to comment the closing brace.

 Serial.println(hi, 1); // prints vabale "hi" with one point place ie 75.54 now it just 75.5
  delay(1500); // to give the DHT11 time to get it's next reading before relooping
  return;
} //loop

I find it easier to keep track of where I am when scrolling through a long sketch.

(In fact, I edited and over-wrote the bare minimum sketch (that appears when you go New) to have those comments at the end of setup() and loop(). )

camgangrel: 3rd I wanted to see if it's possible to make a custom library that hold a lot of the stuff in this code that really should be function calls like in a real app would do.

Organising code in short single purpose functions is good because it makes the program easier to debug and maintain. But I can't see any advantage in creating a library unless you plan to make the code available for use by other people.

...R Planning and Implementing a Program

The code below is the virgin example for a dht11 from the library I use. It has at least the following faults:

  • The delay at the end of loop()
  • everything in loop()

The second code fixes both of those things.

// 
//   FILE:  dht11_test1.pde
// PURPOSE: DHT11 library test sketch for Arduino
//
#include <dht11.h>
dht11 DHT;
#define DHT11_PIN 4

void setup(){
  Serial.begin(9600);
  Serial.println("DHT TEST PROGRAM ");
  Serial.print("LIBRARY VERSION: ");
  Serial.println(DHT11LIB_VERSION);
  Serial.println();
  Serial.println("Type,\tstatus,\tHumidity (%),\tTemperature (C)");
}

void loop(){
  int chk;
  Serial.print("DHT11, \t");
  chk = DHT.read(DHT11_PIN);    // READ DATA
  switch (chk){
    case DHTLIB_OK:  
                Serial.print("OK,\t"); 
                break;
    case DHTLIB_ERROR_CHECKSUM: 
                Serial.print("Checksum error,\t"); 
                break;
    case DHTLIB_ERROR_TIMEOUT: 
                Serial.print("Time out error,\t"); 
                break;
    default: 
                Serial.print("Unknown error,\t"); 
                break;
  }
 // DISPLAT DATA
  Serial.print(DHT.humidity,1);
  Serial.print(",\t");
  Serial.println(DHT.temperature,1);

  delay(1000);
}
// based on....
//   FILE:  dht11_test1.pde
// PURPOSE: DHT11 library test sketch for Arduino
// ... but tidier

#include <dht11.h>
dht11 DHT;
#define DHT11_PIN 4

unsigned long lastDHT;
int dhtReadInterval = 1000;

void setup()
{
  Serial.begin(9600);
  Serial.println("DHT TEST PROGRAM ");
  Serial.print("LIBRARY VERSION: ");
  Serial.println(DHT11LIB_VERSION);
  Serial.println();
  Serial.println("Type,\tstatus,\tHumidity (%),\tTemperature (C)");
}//setup

void loop()
{
  doDHTstuff();
}//loop

void doDHTstuff()
{
  if (millis() - lastDHT >= (unsigned long) dhtReadInterval) //check if time has elapsed
  {
    //if it has, do all the below....
    lastDHT = millis();  //save for next time
    int chk;
    Serial.print("DHT11, \t");
    chk = DHT.read(DHT11_PIN);    // READ DATA
    switch (chk) {
      case DHTLIB_OK:
        Serial.print("OK,\t");
        break;
      case DHTLIB_ERROR_CHECKSUM:
        Serial.print("Checksum error,\t");
        break;
      case DHTLIB_ERROR_TIMEOUT:
        Serial.print("Time out error,\t");
        break;
      default:
        Serial.print("Unknown error,\t");
        break;
    }
    // DISPLAT DATA
    Serial.print(DHT.humidity, 1);
    Serial.print(",\t");
    Serial.println(DHT.temperature, 1);
  }
}//dht stuff

The use of millis() there is pretty much right out of blink without delay, except instead of toggling the led, you do the stuff inside the function.

I’m always in two minds as to where it’s best to put the millis() stuff.

  • Either (like I did there) always call the function from loop() but only do what’s inside if it’s time, or
  • only call the function if it’s time to do so in the first place.

Matter of taste?

Putting the timing in loop() makes it immediately obvious that there is timing, but makes loop() more cluttered, I think.

That would be this:

// based on....
//   FILE:  dht11_test1.pde
// PURPOSE: DHT11 library test sketch for Arduino
// ... but tidier

#include <dht11.h>
dht11 DHT;
#define DHT11_PIN 4

unsigned long lastDHT;
int dhtReadInterval = 1000;

void setup()
{
  Serial.begin(9600);
  Serial.println("DHT TEST PROGRAM ");
  Serial.print("LIBRARY VERSION: ");
  Serial.println(DHT11LIB_VERSION);
  Serial.println();
  Serial.println("Type,\tstatus,\tHumidity (%),\tTemperature (C)");
}//setup

void loop()
{
  if (millis() - lastDHT >= (unsigned long) dhtReadInterval) //check if time has elapsed
  {
    lastDHT = millis();  //save for next time
    doDHTstuff();
  }
}//loop

void doDHTstuff()
{
  int chk;
  Serial.print("DHT11, \t");
  chk = DHT.read(DHT11_PIN);    // READ DATA
  switch (chk) {
    case DHTLIB_OK:
      Serial.print("OK,\t");
      break;
    case DHTLIB_ERROR_CHECKSUM:
      Serial.print("Checksum error,\t");
      break;
    case DHTLIB_ERROR_TIMEOUT:
      Serial.print("Time out error,\t");
      break;
    default:
      Serial.print("Unknown error,\t");
      break;
  }
  // DISPLAT DATA
  Serial.print(DHT.humidity, 1);
  Serial.print(",\t");
  Serial.println(DHT.temperature, 1);
}//dht stuff

I'm always in two minds as to where it's best to put the millis() stuff.

Either (like I did there) always call the function from loop() but only do what's inside if it's time, or only call the function if it's time to do so in the first place.

Matter of taste?

I often find that once per second is right for a lot of stuff, so I create a function everySecond() and put all the once per second function calls in there. On a similar theme you could create a function schedule() (or whatever you like) and put all the timed function calls into it. Call schedule() once from loop().

Yes I was just thinking something along those lines.

So far my method is usually to call the functions unconditionally from loop(), which makes it easy to see that it's for example, reading sensors, actuating actuators, updating an lcd etc. And since it's my code and I'm the only user, I know that there's some timing hidden in those functions.

And anyway, can always put a reminder // in where it's called from loop()

updateLCD(); //every second

So far my method is usually to call the functions unconditionally from loop(), which makes it easy to see that it’s for example, reading sensors, actuating actuators, updating an lcd etc. And since it’s my code and I’m the only user, I know that there’s some timing hidden in those functions.

It’s not just timing, there a a whole load of possible conditions that might go at the start of a function, for example, I might have a function that updates what is on a display, but only if the relevant data has changed, so there will be a flag to indicate the data has changed and a test of the flag in the display function to see if the display should be updated. I usually do what you do, with the tests in the functions not in loop(). For the 1 second stuff I have an everySecond() function.

blomcrestlight: I'm always in two minds as to where it's best to put the millis() stuff.

  • Either (like I did there) always call the function from loop() but only do what's inside if it's time, or
  • only call the function if it's time to do so in the first place.

Matter of taste?

IMHO it's a matter of taste.

I tend to put the timing within the function to keep the code in loop() as clean as possible. I try to make loop() appear as a summary of what the program does - for example

void loop() {
   readButtons();
   updateMotorStates();
   operateMotors();
}

...R

blh64:
Welcome to the forums!

+1 karma for actually using code tags on your very first post!

As for your code, you only have 1 other function besides setup() and loop(). Not a lot to optimize. You are declaring an array of 3 temperatures in your

float temp[3]

As for what that is or doing I have no clue it was part of some code that I found. Without it there the code doesn’t work right so I left it where it is at. ie if your don’t know what it is or does but the car still is working with it but don’t run after it removed put it back till you know what it is doing.

blh64:
and passing this reference to getTemp(), but you only every reference the 3rd element. If you never use the first 2 elements, why the array?

As for point #2, look at the BlinkWithoutDelay example (file->examples->02.Digital->BlinkWithoutDelay). Is there a reason why you need to use this?\

I have all ready done that but it don’t click in my head. That is why I was wanting to see how to move this working code that I do understand. And know is working because it may help me better then some code that don’t make much scents to me to start with. ie why I said I’m learning.

blh64:
As for point #3 - don’t worry about this as a beginner. Worry about it 5 years from now :slight_smile:

Oh okay, thanks just asked because I just want to cut down on lines of code and the understandable of the code. I plain to add more stuff to this build but need to get this working so that I’m not blocking the program from getting other data.

Robin2: IMHO it's a matter of taste.

I tend to put the timing within the function to keep the code in loop() as clean as possible. I try to make loop() appear as a summary of what the program does - for example

void loop() {
   readButtons();
   updateMotorStates();
   operateMotors();
}

...R

Thanks for this that is what I was looking for when it come to cleaning up my cod. I know I wasn't thinking about it the right way.