Please Critique My First Project

johndey · April 5, 2017, 5:35pm

This is my first Arduino project. The project objective is to remotely monitor the water flow in the irrigation system at my home. I intend to develop the project in 2 phases.

The first phase will be to display the results to a hard wired LED for the purpose of verifying the operation of the monitoring system.

The second phase consists of replacing the LED display with a WiFi module and uploading the output in a continuous stream to a TBD website.

I am now requesting a critique of the hardware setup and the sketch for Phase 1. I have already run the sketch through the Arduino ‘sanity’ verification.

The hardware consists of the following:

Arduino Uno
External power cable for Uno
Geniten Hall-effect water flow sensor
Diymall 4-pin .96 two-color oled display

Connections are as follows:

Jumpers from Uno to a 5V bus and to a 3.3V bus
Sensor power connecttion to 5V bus
Sensor data connect to Uno pin D2

Display power connect to 3.3V bus
Display SDA connect to Uno pin A4
Display SCL connect to Uno pin A5

H2O_Flo_LED

Measures the flow rate of water flowing through a 3/4-inch PVC pipe using a DIGITEN Hall effect sensor and displaying results on a 0.96-inch Diymall 4-pin Oled display.

The sensor output is in the form of the duration in milliseconds of a square wave pulse and the relationship to the water flowrate is:

F = 4.8 * Q ( in liters per minute)

*/

int pin = 2;
double dursec;
double pulsefreq;
double flolitr;
double flogal;
double flocuft;
unsigned long duration;

#include <SPI.h>
#include <Wire.h>
#include <Adafruit_GFX.h>
#include <Adafruit_SSD1306.h>
#define OLED_RESET 4
Adafruit_SSD1306 display(OLED_RESET);

void setup()
{
pinMode(pin, INPUT);

Serial.begin(9600);
display.begin(SSD1306_SWITCHCAPVCC, 0x3C);
//initialize with the I2C addr 0x3C (128x64)
display.clearDisplay();
// clear the display before start
}

void loop()
{
duration = pulseIn(pin, HIGH);
//Pulse duration in microseconds

dursec = duration / 1000000;
//Pulse duration in seconds
pulsefreq = 1 / dursec;
//Pulse frequency in Hz
flolitr = (pulsefreq * 60 / 4.8);
//Flow rate in Liters/hour
flogal = flolitr * .26417;
//Flow rate in Gallons/hour
flocuft = flolitr * .035315;
//Flow rate in cu ft/hour

display.setCursor(22,20);
//x,y coordinates
display.setTextSize(3);
//size of the text
display.println(flogal);
display.setCursor(85,20);
display.setTextSize(3);
display.println("Gal/hr");
//print "Gal/hr" in oled
delay(1000);
// Wait one second
display.clearDisplay();
}

Paul_KD7HB · April 6, 2017, 12:07am

A schematic showing all components and how they are wired is necessary. However! Can you explain the use of a Hall effect sensor to measure water flow, when the sensor description specifically states it measures magnetic fields. If there something in your water that makes it special?

Paul

pert · April 6, 2017, 3:35am

Please use code tags (</> button on the toolbar) when you post code or warning/error messages. The reason is that the forum software can interpret parts of your code as markup (the smiley face in your code above for example), leading to confusion, wasted time, and a reduced chance for you to get help with your problem. This will also make it easier to read your code and to copy it to the IDE or editor. Using code tags and other important information is explained in the How to use this forum post. Please read it.

Please always do a Tools > Auto Format on your code before posting it. This will make it easier for you to spot bugs and make it easier for us to read.

johndey:
I have already run the sketch through the Arduino ‘sanity’ verification.

Does that mean you just checked that it compiles or that you've actually verified that it works correctly?

johndey:
int pin = 2;

Use descriptive variable names. Use const on any variable that isn't supposed to change:
https://www.arduino.cc/en/Reference/Const

double dursec;
double pulsefreq;
double flolitr;
double flogal;
double flocuft;
unsigned long duration;

I realize that your code will eventually be more complex but I suspect that not all of these variables need to be global. Generally you should limit the scope of a variable as much as possible:

dursec = duration / 1000000;

That won't work as you expect it to. duration and 1000000 are both integers and thus integer math will be done. For example if duration is 100000 you would expect dursec to be 0.1 but actually it will be 0.0. To make this math work correctly you need to make one of the numbers floating point:

dursec = duration / 1000000.0;

This is the sort of thing you would have already discovered if you actually tested your code on hardware. The fact that code compiles does not necessarily mean that it works.

delay(1000);

At this point a delay causes no issues but as you add more features to your project you will often find that blocking code causes problems. Doing a big rewrite later to make a sketch non-blocking is much more work than writing it correctly from the start. Please refer to File > Examples > 02.Digital > BlinkWithoutDelay for an example of how you can avoid the use of delay().

johndey · April 6, 2017, 7:28pm

First of all I want to thank Paul_KD7HB and pert for taking the time to review my project and helping me to getting it right. I have incorporated all of pert's suggestions with the exception of eliminating the delay() function in the loop. I do not anticipate adding any features to this sketch that would be affected by the delay function, and I was having difficulty with the logic required for the alternative approach.

I subjected the revised code to both the auto-format and the verify processes provided by Arduino.

Paul, I will provide a schematic some time in the near future when I have the time to construct one.

The revised code is shown below and is wrapped in the </> tags if I can find the button in the preview.

/*
 H2O_Flo_LED

 Measures the flow rate of water flowing through a 3/4-inch PVC pipe using a DIGITEN Hall effect sensor and displaying results on a 0.96-inch Diymall 4-pin Oled display.

 The sensor output is in the form of the duration in milliseconds of a square wave pulse and the relationship to the water flowrate is:

 F = 4.8 * Q  ( in liters per minute)

*/



#include <SPI.h>
#include <Wire.h>
#include <Adafruit_GFX.h>
#include <Adafruit_SSD1306.h>
#define OLED_RESET 4
Adafruit_SSD1306 display(OLED_RESET);



void setup()
{
 const int pin = 2;
 unsigned long dursec;
 float pulsefreq;
 float flolitr;
 float flogal;
 float flocuft;
 unsigned long duration;

 pinMode(pin, INPUT);

 Serial.begin(9600);
 display.begin(SSD1306_SWITCHCAPVCC, 0x3C);
 //initialize with the I2C addr 0x3C (128x64)

 // init done


 // Show image buffer on the display hardware.
 // Since the buffer is intialized with an Adafruit splashscreen
 // internally, this will display the splashscreen.
 display.display();
 delay(2000);



 display.clearDisplay();
 // clear the display before start
}

void loop()
{
 const int pin = 2;
 unsigned long dursec;
 float pulsefreq;
 float flolitr;
 float flogal;
 float flocuft;
 unsigned long duration;

 duration = pulseIn(pin, HIGH);
 //Pulse duration in microseconds

 dursec = duration / 1000000.0;
 //Pulse duration in seconds
 pulsefreq = 1.0 / dursec;
 //Pulse frequency in Hz
 flolitr = (pulsefreq * 60.0 / 4.8);
 //Flow rate in Liters/hour
 flogal = flolitr * .26417;
 //Flow rate in Gallons/hour
 flocuft = flolitr * .035315;
 //Flow rate in cu ft/hour


 display.setCursor(22, 20);
 //x,y coordinates
 display.setTextSize(3);
 //size of the text
 display.println(flogal);
 display.setCursor(85, 20);
 display.setTextSize(3);
 display.println("Gal/hr");
 //print "Gal/hr" in oled
 delay(1000);
 display.clearDisplay();
}

Paul_KD7HB · April 6, 2017, 8:26pm

I cannot find a link to the flow sensor you might be using. There are Hall effect flow sensors for electrically conductive fluids and then there are flow sensors that use a rotating turbine in the fluid to rotate a magnet past a Hall effect sensor. My circuit board washer has the latter to tell the controller wash water is being pumped and the reservoir is not empty.

Is what you are using the latter device?

Paul

johndey · April 6, 2017, 8:44pm

That's correct, Paul, I'm using the type with the turbine. The data output is in the form of a square wave in which the amplitude of the wave is in volts and in which the wave frequency is related to the flow rate of the water flowing past the turbine.

johndey · April 6, 2017, 9:50pm

Schematic requested by Paul is attached.

Schematic.pdf (20.9 KB)

pert · April 6, 2017, 9:52pm

johndey:
void setup()

{
const int pin = 2;

You have made this variable local to setup() and loop(). That's a bad idea because if you wanted to change the value of pin you would need to edit two lines in your code, making it very likely for you to forget one and cause a bug. This was the only one that was definitely appropriate as a global. Also, you forgot my advice on giving it a descriptive name.

johndey:

 unsigned long dursec;

float pulsefreq;
float flolitr;
float flogal;
float flocuft;
unsigned long duration;

You made these local to setup() but they are never used in setup(). They are only used in loop() and thus should be local to loop().

johndey:

 unsigned long dursec;

float pulsefreq;
float flolitr;
float flogal;
float flocuft;
unsigned long duration;

duration = pulseIn(pin, HIGH);
//Pulse duration in microseconds

dursec = duration / 1000000.0;
//Pulse duration in seconds
pulsefreq = 1.0 / dursec;
//Pulse frequency in Hz
flolitr = (pulsefreq * 60.0 / 4.8);
//Flow rate in Liters/hour
flogal = flolitr * .26417;
//Flow rate in Gallons/hour
flocuft = flolitr * .035315;

That works but remember what I was saying about using const whenever the value of a variable won't change. In your code above the value of these variables does change from the uninitialized value to the ones in your flow rate determination code so you can't make them const. However, if you change things a bit it will allow you to make them const and also make the code cleaner:

  const unsigned long duration = pulseIn(pin, HIGH);
  //Pulse duration in microseconds

  const float dursec = duration / 1000000.0;
  //Pulse duration in seconds
  const float pulsefreq = 1.0 / dursec;
  //Pulse frequency in Hz
  const float flolitr = (pulsefreq * 60.0 / 4.8);
  //Flow rate in Liters/hour
  const float flogal = flolitr * .26417;
  //Flow rate in Gallons/hour
  const float flocuft = flolitr * .035315;

johndey:

  unsigned long dursec;

johndey:

  dursec = duration / 1000000.0;

I'm pretty sure you meant to make dursec a float, rather than unsigned long.

johndey · April 7, 2017, 4:54pm

Pert, I followed your suggestions and removed the variable declarations from the setup function. But when I added "const float" to the equations in the loop, I got compilation error messages as shown below:

Arduino: 1.8.1 (Mac OS X), Board: "Arduino/Genuino Uno"

/Users/user/Documents/Arduino/H2O_Flo_LED/H2O_Flo_LED.ino: In function 'void loop()':
H2O_Flo_LED:59: error: conflicting declaration 'const long unsigned int duration'
const unsigned long duration = pulseIn(sensdatapin, HIGH);
^
/Users/user/Documents/Arduino/H2O_Flo_LED/H2O_Flo_LED.ino:57:17: note: previous declaration as 'long unsigned int duration'
unsigned long duration;
^
H2O_Flo_LED:62: error: conflicting declaration 'const float dursec'
const float dursec = duration / 1000000.0;
^
/Users/user/Documents/Arduino/H2O_Flo_LED/H2O_Flo_LED.ino:52:9: note: previous declaration as 'float dursec'
float dursec;
^
H2O_Flo_LED:64: error: conflicting declaration 'const float pulsefreq'
const float pulsefreq = 1.0 / dursec;
^
/Users/user/Documents/Arduino/H2O_Flo_LED/H2O_Flo_LED.ino:53:9: note: previous declaration as 'float pulsefreq'

Here is the complete code:

/*
 H2O_Flo_LED

 Measures the flow rate of water flowing through a 3/4-inch PVC pipe using a DIGITEN Hall effect sensor and displaying results on a 0.96-inch Diymall 4-pin Oled display.

 The sensor output is in the form of the duration in milliseconds of a square wave pulse and the relationship to the water flowrate is:

 F = 4.8 * Q  ( in liters per minute)

*/



#include <SPI.h>
#include <Wire.h>
#include <Adafruit_GFX.h>
#include <Adafruit_SSD1306.h>
#define OLED_RESET 4
Adafruit_SSD1306 display(OLED_RESET);

const int sensdatapin = 2;

void setup()
{



 pinMode(sensdatapin, INPUT);

 Serial.begin(9600);
 display.begin(SSD1306_SWITCHCAPVCC, 0x3C);
 //initialize with the I2C addr 0x3C (128x64)

 // init done


 // Show image buffer on the display hardware.
 // Since the buffer is intialized with an Adafruit splashscreen
 // internally, this will display the splashscreen.
 display.display();
 delay(2000);



 display.clearDisplay();
 // clear the display before start
}

void loop()
{

 float dursec;
 float pulsefreq;
 float flolitr;
 float flogal;
 float flocuft;
 unsigned long duration;

 const unsigned long duration = pulseIn(sensdatapin, HIGH);
 //Pulse duration in microseconds

 const float dursec = duration / 1000000.0;
 //Pulse duration in seconds
 const float pulsefreq = 1.0 / dursec;
 //Pulse frequency in Hz
 const float flolitr = (pulsefreq * 60.0 / 4.8);
 //Flow rate in Liters/hour
 const float flogal = flolitr * .26417;
 //Flow rate in Gallons/hour
 const float flocuft = flolitr * .035315;
 //Flow rate in cu ft/hour


 display.setCursor(22, 20);
 //x,y coordinates
 display.setTextSize(3);
 //size of the text
 display.println(flogal);
 display.setCursor(85, 20);
 display.setTextSize(3);
 display.println("Gal/hr");
 //print "Gal/hr" in oled
 delay(1000);
 display.clearDisplay();
}

When I removed the "const float" etc from the equation (see below), it compiled OK.

duration = pulseIn(sensdatapin, HIGH);
 //Pulse duration in microseconds

dursec = duration / 1000000.0;
 //Pulse duration in seconds
pulsefreq = 1.0 / dursec;
 //Pulse frequency in Hz
flolitr = (pulsefreq * 60.0 / 4.8);
 //Flow rate in Liters/hour
flogal = flolitr * .26417;
 //Flow rate in Gallons/hour
flocuft = flolitr * .035315;
 //Flow rate in cu ft/hour

pert · April 7, 2017, 9:20pm

You should only declare a variable once, your code does it twice thus the errors. My advice was to do the declaration and definition in a single line, which cleans up the code a bit and allows you to make it const since you don't need to redefine the variable. So you would just need to remove the previous declations, which are no longer necessary:

 float dursec;
 float pulsefreq;
 float flolitr;
 float flogal;
 float flocuft;
 unsigned long duration;

Your previous code works, this suggestion is mostly style but making things const whenever possible can really be a lifesaver. For example, lets say you accidentally wrote something like:

if(flogal = 42) {

when actually you meant:

if(flogal == 42) {

easy and common mistake to make. If flogal is const it will fail to compile and you'll get a helpful error message. If flogal is not const it will happily compile and you'll be scratching your head for a while trying to figure out why your device doesn't work.

johndey · April 8, 2017, 2:09pm

Thanks, Pert. I independently deduced the problem was declaration duplication in the loop and removed the duplicates and got the code to compile. For the record, the final code is below.

/*
  H2O_Flo_LED
 
 Measures the flow rate of water flowing through a 3/4-inch PVC pipe using a DIGITEN Hall effect sensor and displaying results on a 0.96-inch Diymall 4-pin Oled display.
 
 The sensor output is in the form of the duration in milliseconds of a square wave pulse and the relationship to the water flowrate is:
 
 F = 4.8 * Q  ( in liters per minute)
 
 */



#include <SPI.h>
#include <Wire.h>
#include <Adafruit_GFX.h>
#include <Adafruit_SSD1306.h>
#define OLED_RESET 4
Adafruit_SSD1306 display(OLED_RESET);

const int sensdatapin = 2;

void setup()
{



  pinMode(sensdatapin, INPUT);

  Serial.begin(9600);
  display.begin(SSD1306_SWITCHCAPVCC, 0x3C);
  //initialize with the I2C addr 0x3C (128x64)

  // init done


  // Show image buffer on the display hardware.
  // Since the buffer is intialized with an Adafruit splashscreen
  // internally, this will display the splashscreen.
  display.display();
  delay(2000);



  display.clearDisplay();
  // clear the display before start
}

void loop()
{



  const unsigned long duration = pulseIn(sensdatapin, HIGH);
  //Pulse duration in microseconds

  const float dursec = duration / 1000000.0;
  //Pulse duration in seconds
  const float pulsefreq = 1.0 / dursec;
  //Pulse frequency in Hz
  const float flolitr = (pulsefreq * 60.0 / 4.8);
  //Flow rate in Liters/hour
  const float flogal = flolitr * .26417;
  //Flow rate in Gallons/hour
  const float flocuft = flolitr * .035315;
  //Flow rate in cu ft/hour


  display.setCursor(22, 20);
  //x,y coordinates
  display.setTextSize(3);
  //size of the text
  display.println(flogal);
  display.setCursor(85, 20);
  display.setTextSize(3);
  display.println("Gal/hr");
  //print "Gal/hr" in oled
  delay(1000);
  display.clearDisplay();
}

Topic		Replies	Views
Project Feasibility and best approach (Arduino beginner) General Guidance	3	950	May 5, 2021
Senior School technology project help General Guidance	14	5927	May 5, 2021
Project suggestion (first timer) General Guidance	32	3085	May 5, 2021
Help with first project General Guidance	13	1240	May 5, 2021
Getting the right hardware & software General Guidance	3	712	May 5, 2021

Please Critique My First Project

Related topics