Would love feedback on first solo code for project

This is my first Arduino project after reading about the Arduino world and getting hooked.

So my goal was to make a bath tub temperature alarm that would let me know when the water temp got down to the perfect temperature by sounding a small buzzer (that way I didn't have to sit there and keep testing with my foot :P). However, I found that because of the noise and jumpiness of the TC reading, I had to build in some kind of function so the the alarm didn't set until it stabilized under the set temperature. So I just set up a "time" variable that counted whenever the TC is under my set temp, but resets if it jumps back above.

I also build in a double buzz alarm to tell me if the water is below 110F, at which point I need to add more hot water.

Proud to say it works, but since I'm not the most experienced coder, I'd love to get some feedback on my code, as I'm sure there's a more eloquent way to achieve what I did.

Here's the code; any feedback is welcome.

Thanks so much.

New Arduinoid,

EJ1616

// code for bath water temperature alarm. 

#include "max6675.h"

int tempset = 114; //temp alarm setpoint

int buzzer = 9; //buzzer pin

int time = 0; //beginning time

int thermoDO = 4; //these are for MAX6675
int thermoCS = 5;
int thermoCLK = 6;

MAX6675 thermocouple(thermoCLK, thermoCS, thermoDO);
int vccPin = 3;
int gndPin = 2;
  
void setup() {
  
Serial.begin(9600);

  // use Arduino pins
  pinMode(buzzer, OUTPUT);      
  pinMode(vccPin, OUTPUT); digitalWrite(vccPin, HIGH);
  pinMode(gndPin, OUTPUT); digitalWrite(gndPin, LOW);
  
  // wait for MAX chip to stabilize
  delay(500);
}

void loop() {

  // basic readout test, just print the current temp and time variable
  
   Serial.print("F = ");
   Serial.println(thermocouple.readFarenheit());
   Serial.print("Time = ");
   Serial.println(time);
   delay(1000);
 


 
//count how long time is below tempset

   if (thermocouple.readFarenheit() < tempset){
     time=time + 1;  // counts time after TC reaches tempset
   }
   
   if (thermocouple.readFarenheit() > tempset){
      time = 0; //reset time 
   }   
   
//turn on alarm if TC is under temp set for 30 seconds

  if (time > 30){
     digitalWrite(buzzer,HIGH);  //alarms buzzes on and off
     delay(1000);              
     digitalWrite(buzzer, LOW);    
 
  }
  
// Turn on fast alarm if TC is below 110F

  if (thermocouple.readFarenheit() < 110){
     digitalWrite(buzzer,HIGH);  //alarms buzzes on and off
     delay(300);              
     digitalWrite(buzzer, LOW); 
     delay(300);
     digitalWrite(buzzer,HIGH);  //alarms buzzes on and off
     delay(300);              
     digitalWrite(buzzer, LOW);   
 
   }
}
int buzzer = 9; //buzzer pin

This should be

const int buzzer = 9; //buzzer pin

so you don't accidentally overwrite the value in buzzer. Also, I like to see the word pin appear in names of variables that hold pin numbers, like buzzerPin, just so I KNOW that the variable represents a pin number.

int gndPin = 2;
  pinMode(gndPin, OUTPUT); digitalWrite(gndPin, LOW);

Can't tell from this name what this pin is for. If it is really for connecting something to ground, you really should connect it to ground, not to a pin that you pull LOW.

  if (time > 30){
     digitalWrite(buzzer,HIGH);  //alarms buzzes on and off
     delay(1000);             
     digitalWrite(buzzer, LOW);   
 
  }

One second is an annoying long time for a buzzer.

On each pass through loop(), you read the temperature 4 times. Wouldn't one be sufficient? In the few hundred nanoseconds that elapse between the 2 and 4th readings, is the temperature likely to change that much? If it is, I certainly wouldn't be getting in that water.

   if (thermocouple.readFarenheit() < tempset){
     time=time + 1;  // counts time after TC reaches tempset
   }
   
   if (thermocouple.readFarenheit() > tempset){
      time = 0; //reset time
   }

Here, for example, it seems like the if statement should be replaced by an else statement.

What should happen is the temperature equals tempset?

Now that this works, look at the blink without delay example, and learn how to get rid of all the delay()s.

pinMode(vccPin, OUTPUT); digitalWrite(vccPin, HIGH);
  pinMode(gndPin, OUTPUT); digitalWrite(gndPin, LOW);

Are you powering the MAX chip from IO pins?

If so it's a novel approach and should work as the chip only draws about 1mA but it's a little strange.


Rob

http://arduino.cc/playground/Main/RunningAverage

this is a running average library. You can use it to get a running average to smooth out the noise you see on your sensor.

PaulS - thanks, these are great pointers.

Graynomad & PaulS, as for using the MAX6675, I just used what I learned at Adafruit tutorial for thermocouples: Thermocouple sensor tutorial

liudr, love the RunningAverage!

I've incorporated most of everyone's suggestions and it's definitely running way better.

One thing though, Im using a "delay(1000)" because I want to read to the serial port (9600 baud rate). However, I think this delay is messing up my buzzer timing; meaning, it buzzes at 1second intervals rather than the 500milliseconds that I've set in the "interval" constant.

Any clues on how to keep the baud rate, but get my buzzer to buzz at the interval I want?

Thanks!

Code below:

// code for bath water temperature alarm. 

//include Max6675 library
#include "max6675.h"

//include RunningAverage library
#include "RunningAverage.h"


//Constants won't change

const int tempset = 90; //temp alarm setpoint
const int buzzerPin = 9; //buzzer pin

const int thermoDO = 4; //these are for MAX6675
const int thermoCS = 5;
const int thermoCLK = 6;

MAX6675 thermocouple(thermoCLK, thermoCS, thermoDO);
const int vccPin = 3;
const int gndPin = 2;


//Variables will change:
int buzzerState = LOW;    //buzzerState used to set the buzzer
long previousMillis = 0;  // will store last time buzzer was updated

long interval = 500;    // interval at which to buzz buzzer (milliseconds) for primary alarm

RunningAverage myRA(20); //Running Average
int samples = 0;



void setup() {

  Serial.begin(9600);
  myRA.clr(); // explicitly start clean

  // use Arduino pins
  pinMode(buzzerPin, OUTPUT);      
  pinMode(vccPin, OUTPUT); digitalWrite(vccPin, HIGH);
  pinMode(gndPin, OUTPUT); digitalWrite(gndPin, LOW);
  
  // wait for MAX chip to stabilize
  delay(500);
}


void loop() {

   myRA.add(thermocouple.readFarenheit());
   samples++;
   
   Serial.print("Running Average F = ");
   Serial.println(myRA.avg());

   if (samples == 300)
   {
    samples = 0;
    myRA.clr();
   }

   delay(1000);
 

 
//check average temp versus tempset and buzz alarm if so

   if (myRA.avg() < tempset){
       unsigned long currentMillis = millis();
 
      if(currentMillis - previousMillis > interval) {
    
        // save the last time buzzed buzzer 
        previousMillis = currentMillis;   

         // if the buzzer is off turn it on and vice-versa:
         if (buzzerState == LOW)
           buzzerState = HIGH;
         else
            buzzerState = LOW;

      // set the buzzer with the buzzerState of the variable:
      digitalWrite(buzzerPin, buzzerState);
      }
    
   }
   else
        buzzerState = LOW;
 
}

When posting code, please use the # icon in the editor's toolbar.
(You can go back over previous posts, click "modify", highlight the code, then click on the # icon, then click "save")

Not sure what you mean by this:

One thing though, Im using a "delay(1000)" because I want to read to the serial port (9600 baud rate).

But that delay is your problem. If you were reading the serial port, the input would be buffered - you don't need to wait.

If you're thinking you need to pause to let the serial.prints you have complete, you don't. They block until all serial data has been sent.

In short, try it without the delay & see.

RunningAverage myRA(20); //Running Average

   if (samples == 300)
   {
    samples = 0;
    myRA.clr();
   }

You are setting up a buffer to hold twenty values, then clearing it after reading 300 values. Doesn't quite compute for me. The idea behind the class is that it maintains a running average, discarding the oldest value when the internal buffer gets full. Your intervention is not needed.

   if (myRA.avg() < tempset){
   }
   else
        buzzerState = LOW;

Good grief, can you shut the racket off? You set the new state for the buzzer when the average temperature drops below tempset, but you never actually apply it.

long previousMillis = 0;  // will store last time buzzer was updated

This value will be overwritten by an unsigned long value, which can be quite a bit larger than a signed long value, causing previousMillis to contain a negative number. This variable should be unsigned.

long interval = 500;    // interval at which to buzz buzzer (milliseconds) for primary alarm

I guess this is signed to allow for negative intervals.

Im using a "delay(1000)" because I want to read to the serial port (9600 baud rate).

This doesn't make sense to me, either. There are no Serial.read() statements in your code.

If you are trying to prevent flooding the Serial Monitor with messages, a good idea, record when you last sent a value, and send again only after a suitable (unsigned) interval. Get rid of the delay().

PaulS, thanks so much. This has been very helpful. Most of those things I did (like the 300 sample clear out) was because that's what I found in tutorials and stuff, but thanks for pointing it out because I've learned a lot.

I did everything you said, but the temperature will no longer average. Obviously it's something to do with how I've set up recording when I send to Monitor, and then checking the interval of time when since last send to Monitor.

I've pasted here two sets of code. The first one "A" isn't averaging the temps any more and when I look at the serial Monitor, it shows the same temperature for every value. It never changes. The buzzer just buzzes at its interval, but the program isn't really monitoring my temperature any more

The second code "B" has the delay (which I'm trying to get rid of) in it and this one works fine; that is, it's averaging and displaying the changing temps on the Monitor and the buzzer is buzzing when it's supposed to.

I'm really stumped about how to get rid of that delay. I understand the Blind_without_delay principle and have applied to the buzzer, but why is it messing up the program now?

A

// code for bath water temperature alarm. 

//include Max6675 library
#include "max6675.h"

//include RunningAverage library
#include "RunningAverage.h"


//Constants won't change

const int tempset = 90; //temp alarm setpoint
const int buzzerPin = 9; //buzzer pin

const int thermoDO = 4; //these are for MAX6675
const int thermoCS = 5;
const int thermoCLK = 6;

MAX6675 thermocouple(thermoCLK, thermoCS, thermoDO);
const int vccPin = 3;
const int gndPin = 2;


//Variables will change:
int buzzerState = LOW;    //buzzerState used to set the buzzer

unsigned long previousMillis = 0;  // will store last time buzzer was updated
unsigned long previousMillis_Serial = 0;  // will store last time sent to Serial Monitor

unsigned long interval = 750;    // interval at which to buzz buzzer (milliseconds) for primary alarm
unsigned long interval_Serial = 1000;    // interval at which to send to Serial Monitor

RunningAverage myRA(20); //Running Average



void setup() {

  Serial.begin(9600);
  myRA.clr(); // explicitly start clean

  // use Arduino pins
  pinMode(buzzerPin, OUTPUT);      
  pinMode(vccPin, OUTPUT); digitalWrite(vccPin, HIGH);
  pinMode(gndPin, OUTPUT); digitalWrite(gndPin, LOW);
  
  // wait for MAX chip to stabilize
  delay(500);
}


void loop() {

   myRA.add(thermocouple.readFarenheit());
   
   unsigned long currentMillis = millis(); 
  
   if(currentMillis - previousMillis_Serial > interval_Serial) {
    
      // save the last send to Serial 
      previousMillis_Serial = currentMillis;   
   
      Serial.print("Running Average F = ");
      Serial.println(myRA.avg());
       
      } 
   

 
//check average temp versus tempset and buzz alarm if so

   if (myRA.avg() < tempset){

 
      if(currentMillis - previousMillis > interval) {
    
        // save the last time buzzed buzzer 
        previousMillis = currentMillis;   

         // if the buzzer is off turn it on and vice-versa:
         if (buzzerState == LOW)
           buzzerState = HIGH;
         else
            buzzerState = LOW;

      // set the buzzer with the buzzerState of the variable:
      digitalWrite(buzzerPin, buzzerState);
      }
    
   }
   else
        buzzerState = LOW;
  
 
}

B

// code for bath water temperature alarm. 

//include Max6675 library
#include "max6675.h"

//include RunningAverage library
#include "RunningAverage.h"


//Constants won't change

const int tempset = 90; //temp alarm setpoint
const int buzzerPin = 9; //buzzer pin

const int thermoDO = 4; //these are for MAX6675
const int thermoCS = 5;
const int thermoCLK = 6;

MAX6675 thermocouple(thermoCLK, thermoCS, thermoDO);
const int vccPin = 3;
const int gndPin = 2;


//Variables will change:
int buzzerState = LOW;    //buzzerState used to set the buzzer

unsigned long previousMillis = 0;  // will store last time buzzer was updated

unsigned long interval = 1000;    // interval at which to buzz buzzer (milliseconds) for primary alarm

RunningAverage myRA(20); //Running Average




void setup() {

  Serial.begin(9600);
  myRA.clr(); // explicitly start clean

  // use Arduino pins
  pinMode(buzzerPin, OUTPUT);      
  pinMode(vccPin, OUTPUT); digitalWrite(vccPin, HIGH);
  pinMode(gndPin, OUTPUT); digitalWrite(gndPin, LOW);
  
  // wait for MAX chip to stabilize
  delay(500);
}


void loop() {

   myRA.add(thermocouple.readFarenheit());

  
   Serial.print("Running Average F = ");
   Serial.println(myRA.avg());

   delay(1000);
 

 
//check agerage temp versus tempset and buzz alarm if so

   if (myRA.avg() < tempset){
       unsigned long currentMillis = millis();
 
      if(currentMillis - previousMillis > interval) {
    
        // save the last time buzzed buzzer 
        previousMillis = currentMillis;   

         // if the buzzer is off turn it on and vice-versa:
         if (buzzerState == LOW)
           buzzerState = HIGH;
         else
            buzzerState = LOW;

      // set the buzzer with the buzzerState of the variable:
      digitalWrite(buzzerPin, buzzerState);
      }
    
   }
   else
        buzzerState = LOW;
        digitalWrite(buzzerPin, buzzerState);
 
}

The first one "A" isn't averaging the temps any more and when I look at the serial Monitor, it shows the same temperature for every value. It never changes.

Since you don't print the individual readings, how do you know where the problem is? The problem could be that the thermocouple reading never changes. Or, it could be something with the way that the average is computed. You need to determine whether it is a hardware problem (the thermocouple) or a software problem (running average computation). Only then can you hope to fix it.