Programming Efficiency

Using Arduino Programming Questions
1

Hey everyone, so over the past few weeks I've managed to build a pumping project, replace many different components for better more reliable ones....and make who knows how many changes to my program, while learning as much as I can.

This is my first Arduino project, so it's all been a bit of a learning experience for me...it's been very fun though.

I'm going to post my code in hopes to receive some critiques on how to make it more efficient, smarter ways to accomplish tasks, etc.
As of now the project works, so I'm happy....I'd just love to learn smarter ways to handle some of this stuff with my next project.

There are some items that aren't used that I never deleted....I was attempting to remove all delays for the millis method but was having issues with some of them, so there are some remnants from that.

I'm not positive if my method of level detection works currently, but I'll find that out very soon.

// Pin 8 = Recirc Flow
// Pin 7 = Normal Flow
// pin 6 = Pumps
// Pin 5 = Level LCD
// Pin 3 = Flow LCD
// Pin 2 = Flow Meter
// Analog 3 = Pressure

#include "Arduino.h"
#include <SoftwareSerial.h>

SoftwareSerial Serial1 = SoftwareSerial(0, 3);
SoftwareSerial Serial2 = SoftwareSerial(0, 5);

const long displayinterval = 10000;
const long newdisplaytime = 21000;
const long flowinterval = 60000;
const long onesecond = 1000;
const long interval = 18000000;
const long thirtysecond = 30000;
const long tensecond = 10000;
unsigned long displaystart;
unsigned long rfms;
unsigned long fms;
unsigned long cms = 0;
unsigned long newdisplay;
unsigned long pms = 0;
unsigned long pfms = 0;
unsigned long dpms = 0;
unsigned long ost = 0;
unsigned long tst = 0;
unsigned long tenst = 0;


int counter;
int totalFlow;
float flowRate;
volatile int NbTopsFan;
int Calc;
int PCalc;
int Level;
int PrevLevel;
int ChangeLevel;

void setup() {
  Serial.begin(9600);
  Serial1.begin(9600);
  Serial2.begin(9600);
  pinMode(5, OUTPUT);
  pinMode(6, OUTPUT);
  pinMode(7, OUTPUT);
  pinMode(8, OUTPUT);
  pinMode(3, OUTPUT);
  pinMode(A3, INPUT);
  pinMode(2, INPUT);
  attachInterrupt(digitalPinToInterrupt(2), rpm, RISING);
  rlcd1();
  rlcd2();
  Serial1.print("Prototype Pump");
  Serial1.write(0xFE);
  Serial1.write(0x80 + 64);
  Serial1.print("Working Smarter");
  Serial2.print("Prototype Pump");
  Serial2.write(0xFE);
  Serial2.write(0x80 + 64);
  Serial2.print("Working Smarter");
}

void loop() {
  cms = millis();
  if (cms <= 1000) {
    Serial.println("Initializing System");
    digitalWrite(7, HIGH);
    delay(7500);
    digitalWrite(6, HIGH);
  }
  while (flowRate >= 0.25) {
    cms = millis();
    if (cms - tenst >= displayinterval) {
      Serial.println(cms);
      Serial.println("Flow Rate > 0.1");
      Serial.println("Running Normal Flow Path");
      tenst += displayinterval;
      Serial.println(tenst);
    }
    if (cms - dpms >= newdisplaytime) {
      Serial.println("Update Displays");
      UpdateDisplays();
      dpms += newdisplaytime;
      Serial.println(dpms);
    }
    if (cms - pfms >= flowinterval) {
      pfms += flowinterval;
      Serial.println("Time to Read New Flows");
      ReadFlow();
      Serial.println(pfms);
    }
    if (cms - pms >= interval) {
      Serial.println("Starting Level Calculation");
      Serial.println("Putting Valves Into Recycle Path");
      digitalWrite(8, HIGH);
      digitalWrite(7, LOW);
      delay(7500);
      Serial.println("Wait 30 Seconds");
      delay(30000);
      Serial.println("Locking In Pressure");
      digitalWrite(8, LOW);
      delay(5500);
      digitalWrite(6, LOW);
      delay(2000);
      Serial.println("Wait 10 Seconds For Pressure Stabilization");
      delay(10000);
      Serial.println("Calculate Level");
      ReadLevel();
      pms += interval;
      Serial.println(pms);
    }
  }
  while (flowRate < 0.25) {
    Serial.println("There Is No Flow Detected");
    Serial.println("Wait 10 Seconds To Verify Flow");
    delay(10000);
    Serial.println("Verify Flow");
    ReadFlow();
    Serial.println("There Is Still No Flow Detected");
    for (counter = 0; counter < 10; ++counter) {
      digitalWrite(7, LOW);
      digitalWrite(8, HIGH);
      delay(7500);
      digitalWrite(6, HIGH);
      delay(10000); // Wait for 10 seconds
      digitalWrite(7, HIGH);
      digitalWrite(8, LOW);
      delay(7500);
      delay(30000); // Wait for 30 Seconds
      ReadFlow();
      if (flowRate >= 0.25) {
        return;
      } else {
        delay(18000000);
      }
    }
  }
}
void UpdateDisplays() {
  rlcd1();
  Serial1.print("Gal Per Minute");
  Serial1.write(0xFE);
  Serial1.write(0x80 + 64);
  Serial1.print(flowRate);
  Serial.println("Gal Per Minute");
  Serial.println(flowRate);
  rlcd2();
  Serial2.print("Feet of Water");
  Serial2.write(0xFE);
  Serial2.write(0x80 + 64);
  Serial2.print(Level);
  Serial.println("Feet of Water");
  Serial.println(Level);
  delay(10000);
  rlcd2();
  Serial2.print("Level Change");
  Serial2.write(0xFE);
  Serial2.write(0x80 + 64);
  Serial2.print(ChangeLevel);
  Serial.println("Change In Level");
  Serial.println(ChangeLevel);
  rlcd1();
  Serial1.print("Total Gallons");
  Serial1.write(0xFE);
  Serial1.write(0x80 + 64);
  Serial1.print(totalFlow);
  Serial.println("Total Gallons");
  Serial.println(totalFlow);
}
void rpm () {
  NbTopsFan++;
}

void ReadFlow() {
  noInterrupts();
  NbTopsFan = 0;
  interrupts();
  delay (1000);
  noInterrupts();
  flowRate = (NbTopsFan / 11 * 0.2642);
  interrupts();
  totalFlow += flowRate * 60;
  DisplayFlow();
}

void DisplayFlow() {
  Serial.println("Display Flow");
  rlcd1();
  Serial1.print("Gal Per Minute");
  Serial1.write(0xFE);
  Serial1.write(0x80 + 64);
  Serial1.print(flowRate, DEC);
  Serial.println("Gal Per Minute");
  Serial.println(flowRate);
  delay(10000);
  rlcd1();
  Serial1.print("Total Gallons");
  Serial1.write(0xFE);
  Serial1.write(0x80 + 64);
  Serial1.print(totalFlow, DEC);
  Serial.println("Total Gallons");
  Serial.println(totalFlow);
}

void ReadLevel() {
  PCalc = (((analogRead(A3) / 5) - 0.1) / 0.75) * 145.038;
  Level = ((PCalc * 27.71) / 12);
  Serial.println("Calculating Level");
  Serial.println(Level);
  ChangeLevel = Level - PrevLevel;
  Serial.println("Calculating Change In Level");
  Serial.println(ChangeLevel);
  Level = PrevLevel;
  Serial.println("Display Level");
  DisplayLevel();
}

void DisplayLevel() {
  rlcd2();
  Serial2.print("Feet of Water");
  Serial2.write(0xFE);
  Serial2.write(0x80 + 64);
  Serial2.print(Level);
  Serial.println("Feet of Water");
  Serial.println(Level);
  delay(10000);
  rlcd2();
  Serial2.print("Level Change");
  Serial2.write(0xFE);
  Serial2.write(0x80 + 64);
  Serial2.print(ChangeLevel);
  Serial.println("Change In Level");
  Serial.println(ChangeLevel);

}

void rlcd1() {
  Serial1.write(0xFE);
  delay(10);
  Serial1.write(0x01);
  delay(10);
}

void rlcd2() {
  Serial2.write(0xFE);
  delay(10);
  Serial2.write(0x01);
  delay(10);
}
2

So, right out of the gate, you should never use integer pin numbers in anything but a tiny demo program. So instead of just commenting them, like

// Pin 8 = Recirc Flow

assign meaningful names to the pins, e.g.

const int recircFlow_pin = 8
[...]
  pinMode(recircFlow_pin, OUTPUT);
[...]
      digitalWrite(recircFlow_pin, HIGH);

See how that improves the readability?

This one is a "whopper"...

        delay(18000000);

If you can afford to freeze all processing for that long, fine, but... you may regret that later.

3

aarg:
So, right out of the gate, you should never use integer pin numbers in anything but a tiny demo program. So instead of just commenting them, like

// Pin 8 = Recirc Flow

assign meaningful names to the pins, e.g.

const int recircFlow_pin = 8

[...]
  pinMode(recircFlow_pin, OUTPUT);
[...]
      digitalWrite(recircFlow_pin, HIGH);




See how that improves the readability?

Aside from readability is there any purpose in that?
I can see where it would make the potential for errors down the road less likely for sure.

aarg:
This one is a "whopper"...
Code: [Select]
delay(18000000);

If you can afford to freeze all processing for that long, fine, but... you may regret that later.

The goal for that is basically to be turned off. If it gets that far then that means after 10 attempts at getting flow there hasn't been any luck.

4

DrakeLewis:
Aside from readability is there any purpose in that?
I can see where it would make the potential for errors down the road less likely for sure.

Yes, there are many additional reasons. A major one is that you can re-assign the pin number that you used in many different places in the program, by editing only one line - and without the risk of goofing it up somewhere by forgetting some or making a typo. It makes it easier to port the code to different hardware as well.

Also, calculations of this sort invite confusion, you should document the math and/or electronics formulas that stand behind it:

  PCalc = (((analogRead(A3) / 5) - 0.1) / 0.75) * 145.038;
  Level = ((PCalc * 27.71) / 12);
5

SoftwareSerial takes almost all the processing power of an Arduino Uno.
You have a attachInterrupt() and two SoftwareSerial, that makes it unreliable.
Try to avoid SoftwareSerial.

I read that SoftwareSerial on a ESP8266 is not so demanding.

When a variable is only used locally, then you can create it local instead of global.
For example:

void loop()
{
  unsigned long currentMillis = millis();
  ...
}

You have two lines with "cms = millis();", that is confusing. With good code, only one is needed.

Often there are a few things to do in the loop().
Don't use delay or long while-loops in the loop() function. Let the loop() run as often as possible.

Do not use this if not needed:

tenst += displayinterval;

Use this:

tenst = currentMillis;

To add the interval is only needed when the timing must be in pace with the real time. If there is a delay in the loop() that is consistent larger than the interval, then the second one is safer.

All the delays should be removed. Blinking leds can be done with millis().

Waiting for a sensor can be done with millis(), perhaps even a Finite State Machine can be used together with millis().

When the compiler sees "5" in a calculation, then it uses the 5 as a integer.
When the compiler sees "5.0" in a calculation, then it uses the 5.0 as a float number.
What do you think the compiler will to here:

PCalc = (((analogRead(A3) / 5) - 0.1) / 0.75) * 145.038;

It is better when you decide if the calculation should be done with integers or float. That is better than just hoping that the compiler does it the way you want.

There should be comments, explaining the "0.1", the "0.75", the "145.038" and so on.

There are many tutorials and example sketches with millis(). I have also made some. Scroll down halfway this page for the explanation.

When you want to make changes, then you can keep this sketch and start a new sketch with a new name. Then you have something to fall back to. You could store a sketch before each large change. The Arduino IDE does not have a version control, so you have to remember to keep in-between-versions yourself.

6

Actually this is kind of strange:

  PCalc = (((analogRead(A3) / 5) - 0.1) / 0.75) * 145.038;

Because the range of analogRead()/5 is always much greater than 0.1, for example if the ADC reads 5, the sum is 1.0+0.1 = 1.1. But the ADC is not supposed to be operate in such a low range - midrange for the full S/N is 512 so 512/5 = 102, 102.0+0.1 = 102.1 so the adjustment was about 1/1000. About one ADC step. Hmmmm.

7

aarg:
Actually this is kind of strange:

  PCalc = (((analogRead(A3) / 5) - 0.1) / 0.75) * 145.038;

Because the range of analogRead()/5 is always much greater than 0.1, for example if the ADC reads 5, the sum is 1.0+0.1 = 1.1. But the ADC is not supposed to be operate in such a low range - midrange for the full S/N is 512 so 512/5 = 102, 102.0+0.1 = 102.1 so the adjustment was about 1/1000. About one ADC step. Hmmmm.

I might have done this in correctly. The goal of the PCalc and Level is to use the pressure transducer to determine the level in a water column. It involves going from MPa to PSI, PSI to inH20, then converted to feet.
Linked is the pressure sensor I'm using....If I'm doing that incorrectly I wouldn't be surprised.

Pressure Sensor

8

I'm curious, what type of displays are you using? Don't often see LCDs with a serial interface on here.

9

You have two lines with "cms = millis();", that is confusing. With good code, only one is needed.

Often there are a few things to do in the loop().
Don't use delay or long while-loops in the loop() function. Let the loop() run as often as possible.

Should I avoid the use of the while loop and just go with if statements? The while loop just seemed like the logical thing for me to do since there are basically two states my project can be in...either with flow, or without flow.

Without getting rid of the while-loops i have to use two lines of "cms = millis();" in order for my timers to work, or do I? from my reading it seemed that if I never called for cms to update with millis(); that none of my timers would ever meet their criteria.

All the delays should be removed. Blinking leds can be done with millis().

Waiting for a sensor can be done with millis(), perhaps even a Finite State Machine can be used together with millis().

There are still a few delays I need to replace for sure...but in situations where I actually want nothing else being done a delay is fine...right?

I'm new to this, so I'll have to do some reading on whatever a Finite State Machine is haha.

When the compiler sees "5" in a calculation, then it uses the 5 as a integer.
When the compiler sees "5.0" in a calculation, then it uses the 5.0 as a float number.
What do you think the compiler will to here:
Code: [Select]
PCalc = (((analogRead(A3) / 5) - 0.1) / 0.75) * 145.038;

It is better when you decide if the calculation should be done with integers or float. That is better than just hoping that the compiler does it the way you want.

There should be comments, explaining the "0.1", the "0.75", the "145.038" and so on.

If there is a float used anywhere in a calculation then the Arduino converts the result into a float..right? I understand your point though.

I definitely need additional comments for my math though, you are correct.

david_2018:
I'm curious, what type of displays are you using? Don't often see LCDs with a serial interface on here.

Here you go David. Granted this is my first go at this stuff, but I'm pretty impressed with their ease.

LCD Display

10

I sometimes use scalar variables for conversion, it's not my idea, it is already in the language for example:

#define DEG_TO_RAD 0.017453292519943295769236907684886

is already in the Arduino core
so you can say

float root2 = 2.0*sin(45.0 * DEG_TO_RAD);

You could do the same kind of thing with "MPa to PSI" and the others.

11

I'm a bit confused by the data sheet for the pressure sensor.
The equation for the output voltage is stated as Vout = Vcc ( 0.75 P + 0.1 )
The stated output range is 0.5V to 4.5V with Vcc of 5.0V, and pressure from 0 to 1.2Mpa.
The problem is, at P = 1.2Mpa, the equation would be
Vout = 5.0 ( .75 * 1.2 + 0.1 )
Vout = 5.0 ( 0.9 + 0.1 )
Vout = 5.0 ( 1.0 )
Vout = 5.0 volts
This does not match the stated output range, or the graph showing 4.5V out at 1.2Mpa.

12

david_2018:
I'm a bit confused by the data sheet for the pressure sensor.
The equation for the output voltage is stated as Vout = Vcc ( 0.75 P + 0.1 )
The stated output range is 0.5V to 4.5V with Vcc of 5.0V, and pressure from 0 to 1.2Mpa.
The problem is, at P = 1.2Mpa, the equation would be
Vout = 5.0 ( .75 * 1.2 + 0.1 )
Vout = 5.0 ( 0.9 + 0.1 )
Vout = 5.0 ( 1.0 )
Vout = 5.0 volts
This does not match the stated output range, or the graph showing 4.5V out at 1.2Mpa.

Wow....I just assumed that the provided equation would be correct without ever verifying. I suppose I'll have to do some testing to figure out what the scaling actually is.

aarg:
I sometimes use scalar variables for conversion, it's not my idea, it is already in the language for example:

#define DEG_TO_RAD 0.017453292519943295769236907684886

is already in the Arduino core
so you can say

float root2 = 2.0*sin(45.0 * DEG_TO_RAD);

You could do the same kind of thing with "MPa to PSI" and the others.

I did not realize that functionality was there!
Posting here has definitely been worth while

13

PCalc = (((analogRead(A3) / 5) - 0.1) / 0.75) * 145.038;

That calculation may be incorrect, from the original equation in the data sheet

Vout = Vcc ( 0.75 P + 0.1 )

Solving for P gives

P = ( (Vout / Vcc) - 0.1 ) / 0.75

and with Vcc = 5

P = ( (Vout / 5) - 0.1 ) / 0.75

If that is what you are intending, note that analogRead(A3) is a number from 0 to 1023, not a voltage.
To convert the analog input to a voltage, divide by 1024 and multiply by the analog reference voltage (Vcc in this case)
(and you can look up the discussions on whether to use 1023 or 1024 as the divisor)
Vout = ( analogRead() / 1024.0 ) * 5.0 //this forces the compiler to use floating point numbers

The equation then becomes

P = ( ( ( ( analogRead() / 1024.0 ) * 5.0) / 5) - 0.1 ) / 0.75

P = ( ( analogRead() / 1024.0 ) - 0.1 ) / 0.75

14

DrakeLewis:
Aside from readability is there any purpose in that?
I can see where it would make the potential for errors down the road less likely for sure.

Readability is a big thing for language.

Additionally, it will help searching when your code gets larger. Some numerical values are used way more often than others. So searching for 1 when you want to change pin1 will give you a lot of hits.

Using good names can help you find bugs. e.g.

pinMode( BUTTON_PIN, INPUT);

looks OK, but might make you or a code reviewer wonder whether you need to enable a pull up resistor.

pinMode( BUTTON_PIN, INPUT_PULLUP);

A widely used naming convention is to use CAPITAL_WITH_UNDERSCORE for constants e.g.

const long DISPLAY_INTERVAL = 10000;

This helps reading larger amounts of code especially when you are not the owner or you have not read your code for a while. You do not need to wonder what that is. Its just a number and cannot have different values. You have been using a few of these in your code already(LOW, OUTPUT, DEG_TO_RAD, ...)

I would also recommend to redefining Serials when you have more than the one for Serial Monitor. e.g.

#define serialLcd Serial1
#define gps Serial2

DrakeLewis:
Should I avoid the use of the while loop and just go with if statements? The while loop just seemed like the logical thing for me to do since there are basically two states my project can be in...either with flow, or without flow.

I try to avoid while loops as much as possible. They are unpredictable especially when your code needs to react fast to some events. As you are adding functionality you can run into timing issues a lot faster. Try to run the loop function as often as you can and do only little things each time you run trough it. Then you can add new steps to the loop without breaking the timing of what you already have.

15

DrakeLewis:
but in situations where I actually want nothing else being done a delay is fine...right?

No. When there is nothing to be done, the loop() should run over and over again without doing something.

Suppose you want to blink a led, regardless what the rest is doing. Then you want the loop() function to run as often as possible, as Klaus_K already wrote.

A sketch with millis() could be this:

  • Get the sensor values every 2 seconds and put it in variables.
  • Update the display four times per second (LCD is slow).
  • Blink a led, the blinking is turned on and off from the sketch.
  • Run a sequence from a table or a Finite State Machine.
  • and so on.

Have you tried a few examples from my Fun_with_millis examples ? It starts with the millis_basic_demo.ino. That example is doing three things. As soon as one of them would use a (large) delay, then it all falls apart.

A sketch using millis() is probably a lot larger and might not even look like an old sketch with delay(). I suggest you start with simple examples, because rewriting your sketch for millis() is not easy. Sometimes additional programming techniques are needed, for example the Finite State Machine that I wrote about earlier.

16

Hi,

#include <SoftwareSerial.h>

SoftwareSerial Serial1 = SoftwareSerial(0, 3);
SoftwareSerial Serial2 = SoftwareSerial(0, 5);

What do the last two lines do?
Pin 0 is already used for programming and the IDE Monitor.
You cannot use the same Pin for all the serial ports, they must have separate pairs.

This example may help;

Tom... :slight_smile:

17

If there is a float used anywhere in a calculation then the Arduino converts the result into a float..right?

Wrong. If floats are used in the calculation, then integer values will be converted to floats eventually, at some point in the calculation, but not before it absolutely has to. For example in your code line:

PCalc = (((analogRead(A3) / 5) - 0.1) / 0.75) * 145.038;

The first part to be evaluated will be the part "(analogRead(A3) / 5)". The numbers before and after the "/" are integers, so integer division will be done. If analogRead(A3) was 19, the result would be 3, not 3.8, so you loose some accuracy. The next part to the evaluated would be "(3 - 0.1)". At this point, one value is integer and the other is float, so floating subtraction is done, giving 2.9, and not the 3.7 you might have wanted. This is what Koepel was trying to explain earlier.

18

Everyone,

Thank you for the replies and information! I'll be making some adjustments in the field to my program once I do a little rewriting.

There is a lot for me to learn, but clearly this is an active community with people who enjoy (or don't mind) providing constructive feedback.

Thank you everyone!