Code review

Attached is my code. I am interested in any feedback in general, on the use of datatypes (efficiency and conflicts).

I have one concern regarding some math requiring division and the use of a float datatype. does this present a problem, etc. and do I need to cast the result as a smaller datatype before passing to LCD.


  #include <PID_v1.h>
  #include <avr/io.h>
  #include <avr/interrupt.h>
  #include "DHT.h"
  #include <LiquidCrystal.h>

// Pin assignments

  #define DETECT 2  //zero cross detect
  #define GATE 3    //triac gate
  #define motorPin 4    //Motor on/off    
  #define DHTPIN 5    //DHT sensor input
  #define dayIncrement 6    //DHT sensor input

  #define dRs 8    //Display register S  
  #define dEnable 9    //Display enable
  #define dB4 10    //Display B7
  #define dB5 11    //Display B6  
  #define dB6 12    //Display B5
  #define dB7 13    //Display B4
// Specify the LCD
LiquidCrystal lcd(dRs, dEnable, dB4, dB5, dB6, dB7);

// Specify for testing inputs
//  #define TEST_INPUT A0  //for test input
  int triacPot = 0;    // initialize to an off condition, test only

// Specify MOC/triac pulse and counts in a 1/2 cycle of 60hz for mapping
  int phaseLow = 416;    //208 counts to 416 counts is a roughly linear region except at low power, and represents 10% power to 100% power with full on being 120% power
  int phaseHigh = 208;
  #define PULSE 4

// Specify timing
  unsigned long turnDays = 420000;       // 19 days in milliseconds  =19*24*60*60*1000   1641600000
  unsigned long dayCount =  0;
  unsigned long oneDay = 108000;    //86400000
  unsigned long motorTime = 0; // initialize with 0; the time when the motor was turned on or off
  unsigned long timeOn = 3000; // however long the motor should be on */;   15000
  unsigned long timeOnError =  30000;
  unsigned long timeOff = 6000;// however long the motor should be off */;  3585000
  unsigned long timeOffError = 30000;
  unsigned long measureTime = 0; // initialize with 0; the time when the last measurement was made
  unsigned long displayTime = 0; // initialize with 0; the time when the display was last refreshed
  unsigned long testTime = 0;
  bool motorRunning = false; // start with the motor off
  bool motorError = false; 
  bool heaterError = false; 
  bool DHTerror = false;

// Initialize DHT sensor.
  #define DHTTYPE DHT22   // DHT 22  (AM2302), AM2321
  float h;
  float t;
  float f;
//Specify the PID
  double Setpoint, Input, Output;
  double Kp=18, Ki=1, Kd=1;
  PID myPID(&Input, &Output, &Setpoint, Kp, Ki, Kd, DIRECT);
  float percentHeat;

void setup(){


  lcd.begin(20, 4);
//  pinMode(TEST_INPUT, INPUT);   //test purposes potentiometer
  pinMode(DHTPIN, INPUT);   //DHT sensor input
  pinMode(DETECT, INPUT);     //zero cross detect   INPUT_PULLUP
  pinMode(GATE, OUTPUT);      //triac gate control
  pinMode(motorPin, OUTPUT);   //motor control

  pinMode(dEnable, OUTPUT);   //display
  pinMode(dRs, OUTPUT);   //display
  pinMode(dB7, OUTPUT);   //display
  pinMode(dB6, OUTPUT);   //display
  pinMode(dB5, OUTPUT);   //display
  pinMode(dB4, OUTPUT);   //display

// set up Timer1    ATMEGA 328 data sheet pg 134)
  OCR1A = 510;      //initialize the comparator to an off/low condition
  TIMSK1 = 0x03;    //enable comparator A and overflow interrupts
  TCCR1A = 0x00;    //timer control registers set for
  TCCR1B = 0x00;    //normal operation, timer disabled

// set up zero crossing interrupt
  attachInterrupt(digitalPinToInterrupt(DETECT),zeroCrossingInterrupt, RISING);  //IRQ0 is pin 2. Call on rising signal

//Set up the PID loop
  //initialize the variables we're linked to
    Input = 70;
    Setpoint =100;    // 99 to 101 F is recommended range

//Interrupt Service Routines

void zeroCrossingInterrupt(){ //zero cross detect   
  TCCR1B=0x04; //start timer with divide by 256 input
  TCNT1 = 0;   //reset timer - count from zero     TCNT1 is the Timer/counter Register for timer 1

ISR(TIMER1_COMPA_vect){ //comparator match
  digitalWrite(GATE,HIGH);  //set triac gate to high
  TCNT1 = 65536-PULSE;      //trigger pulse width

ISR(TIMER1_OVF_vect){ //timer1 overflow
  digitalWrite(GATE,LOW); //turn off triac gate
  TCCR1B = 0x00;          //disable timer stopd unintended triggers

void loop(){

// manage the egg turner
    unsigned long now = millis();
    dayCount = millis() / oneDay;
    if (millis() <= turnDays || motorRunning ==true)  //enter this block only if time is less than 19 days or the motor is running 
        // The motor is running. 
        // Is it time to turn it off?        
        if(now - motorTime >= timeOn)
          digitalWrite(motorPin, LOW);   // turn off motor
          motorTime = now;
          motorRunning = false;

         // The motor is not running. 
         // Is it time to turn it on?
        if(now - motorTime >= timeOff)
            digitalWrite(motorPin, HIGH);   // turn on motor
            motorTime = now;
            motorRunning = true;

// Reading temperature or humidity takes about 250 milliseconds!    
    if(now - measureTime >= 2000)        // Data is only updated about every 2 seconds, and disables the dimmer interrupts
      // It is time to measure temperature
        h = dht.readHumidity();
        //t = dht.readTemperature();
        f = dht.readTemperature(true);  //(isFahrenheit = true)
        // Check if any reads failed and exit early (to try again).
        if (isnan(h) || isnan(t) || isnan(f)) {
          DHTerror = true; 
          //Serial.println("Failed to read from DHT sensor!");
        measureTime = now;

// Determine heater setting    
  Input = f;
  OCR1A = map(Output,0,255, phaseLow, phaseHigh);   //set the compare register; phaseLow is least amount

// if(now - testTime >= 20000)    ///-------------------------------------------for testing purpose
//    {
//        Input =  Input + 100;   //  analogRead(TEST_INPUT);
//        if(OCR1A > 499)
//        {
//          Input = 0;
//        }
//        OCR1A = map(Input,0,1023, phaseLow, phaseHigh); //map(Output,0,255, phaseLow, phaseHigh);   //set the compare register brightness desired.
//        testTime = now;
//    }

//Send info to LCD display
    if(now - displayTime >= 1000)
//      lcd.clear();
      //Day count
      lcd.setCursor(0, 0);
      lcd.print("Day: ");
      lcd.print("  ");   //print 2 blanks in case the number changes from 3 to 2 digits
      lcd.setCursor(5, 0);      //go back to the start of the number  
      lcd.setCursor(0, 1);
      lcd.print("Temp: ");
      lcd.print("         ");   //print 9 blanks in case the number changes from 3 to 2 digits
      lcd.setCursor(6, 1);      //go back to the start of the number  
      lcd.print(" ");
      lcd.print("F");    //   \t

      lcd.setCursor(0, 2);
      lcd.print("Hum: ");
      lcd.print("        ");   //print 8 blanks in case the number changes from 3 to 2 digits
      lcd.setCursor(5, 2);      //go back to the start of the number  
      lcd.print(" %");
      //humidity/temperature warning
      //lcd.setCursor(0, 0);
      //heater percent on
      lcd.setCursor(10, 0);
      lcd.print("Heat: ");
      lcd.print("    ");   //print 3 blanks in case the number changes from 3 to 2 digits
      lcd.setCursor(16, 0);      //go back to the start of the number  
      lcd.print(percentHeat); // 

      //Motor on/off
      lcd.setCursor(0, 3);
         lcd.print("Motor: On ");
         lcd.print("Motor: Off");  

      //Display Millis()
      lcd.setCursor(11, 3);
      displayTime = now;


Preprocessor directives start at column 1 - they are not indented. It doesn't matter with modred compilers, but back in the day the preprocessor checked for a hash in column 1 and that's how it worked.

I think that const variables are better practise than using preprocessor directives … although it's a "feel" thing and tricky to explain why. Preprocessor directives work by messing with your code before it gets to the compiler. Using const variables or enums lets the compiler do its job.

I prefer

const unsigned long turnDays = 19L*24L*60L*60L*1000L; // 19 days

Note that we have to explicitly decare that the factors are longs (the L suffix on the number) Why not let the compiler do the multiplication? That way, you don't get weird bugs where the number in your code (420000) doesn't match what the comment says it is. In fact - 420000 is 7 minutes! Nowhere near 19 days! I suppose this is a test value, but that's the point - having the multiplication there means that I don't have to divide by 60 to work out what's going on.

I like to suffix names that hold measurements with the unit of measurement. This makes "crashing your spaceship into Mars" errors less likely. So 'turnDays' would be 'turnMs' or 'turn_ms', because it holds a number of milliseconds not a number of days.

I'd be inclined to break some of that stuff in the loop() into sub-functions. The display stuff in particular is better in a function, because that makes it easy to swap in a different function that - for instance - spits stuff out to serial instead of to an LCD.

  #define dB4 10    //Display B7
  #define dB5 11    //Display B6 
  #define dB6 12    //Display B5
  #define dB7 13    //Display B4

Reasonable names would not need comments to explain the statement.

  int triacPot = 0;    // initialize to an off condition, test only

What the hell does that comment mean? If you have to explain comments, then the comments you have are useless.

The only place that variable is used is in a commented out Serial.print() call. Exercise your delete key!

  unsigned long oneDay = 108000;    //86400000

So, clearly, oneDay is not one day. Stupid name, then.

The real purpose of comments is to explain WHY, not WHAT. It is obvious what the code is doing. Why it is doing it is not at all obvious. Since it isn't obvious what you are trying to accomplish, it's hard to say if there are better ways.

PaulMurrayCbr: yes, the smaller numbers were meant for testing purposes. I should have mentioned that in my post. I like the idea of putting the LCD in a function and your recommendation on name suffixes.

PaulS: those dB# are pins on the LCD, so I felt they were descriptive enough. But for anyone not using the typical LCDs, I can see how it does not make sense. I can put some more 'why' in my code.

Thanks for the comments. If anyone sees any glaring issues, or has recommendations on handling the floating point math related to percentHeat variable, please let me know (e.g. I think I need to use cast before passing it to the LCD function).

I think I need to use cast before passing it to the LCD function


class LiquidCrystal : public Print {

and Print sure as heck knows how to print a float.

I would learn s(n)printf() it is a lot cleaner to look at than stuff like code below, though your using floats will admittedly cause problems there. You have a small sketch, so using it may not be an issue.

Side Note: Since the DHT22 isn't all that precise in whole (C) degrees, I often ask myself why I bother with orders of magnitude below 0 in Fahrenheit? I could place any random number right of the decimal and appear to be just as accurate! Well, I stopped using those sensors anyways... They are even worse at humidity...

      lcd.setCursor(0, 2);
      lcd.print("Hum: ");
      lcd.print("        ");   //print 8 blanks in case the number changes from 3 to 2 digits
      lcd.setCursor(5, 2);      //go back to the start of the number  
      lcd.print(" %");

and I'd get back into the habit of using the F() macro...