Hi there !
I have problem with the following sketch. This sketch is meant to start and stop the pump and to display the three levels of the Tank. Cut-in level is 1 and cut-off level is 3.
Cut-off level3 is working OK but pump starts when the level2 is achieved instead of level1 whereas the condition assigned in void startstoppump() is if (level < 2) digitalWrite(Pumppin, HIGH );
The complete code is given below. Kindly help me to identify the problem and suggest to improve the
code.
Thanks
#include <EEPROM.h>
// INPUT PINS
#define Button1 3 //Tank1 Level1
#define Button2 4 //Tank1 Level2
#define Button3 5 //Tank1 Level3
#define Auto_Man_Button 2 //Auto/manual button
// OUTPUT PINS
#define LED1 8 //Tank1 Level1 Display
#define LED2 9 //Tank1 Level2 Display
#define LED3 11 //Tank2 Level1 Display
#define Auto_Man_Led 10 //Auto/manual operation
#define Pumppin 13 // Pin for water pump activation
// EEPROM MEMORY POSITIONS
#define memposL1 0 // set constant eeprom position 0 to save Level1
// variables
int state=0;
int level = 0;
int lastlevel = 0;
void setup (){
// INPUTS
pinMode(Auto_Man_Button,INPUT); // default mode is INPUT
pinMode(Button1,INPUT); // default mode is INPUT
pinMode(Button2,INPUT); // default mode is INPUT
pinMode(Button3,INPUT); // default mode is INPUT
digitalWrite(Auto_Man_Button, HIGH); // Turn on the internal pull-up resistor, default state is HIGH
digitalWrite(Button1, HIGH); // Turn on the internal pull-up resistor, default state is HIGH
digitalWrite(Button2, HIGH); // Turn on the internal pull-up resistor, default state is HIGH
digitalWrite(Button3, HIGH); // Turn on the internal pull-up resistor, default state is HIGH
// OUTPUTS
pinMode(Pumppin, OUTPUT);
pinMode(LED1, OUTPUT);
pinMode(LED2, OUTPUT);
pinMode(Auto_Man_Led, OUTPUT);
pinMode(LED3, OUTPUT);
// Restore tank water Level from eeprom
level = EEPROM.read(memposL1); // Read from eeprom memPos1 the value of Level1
lastlevel = level; // also update the according lastLevel variable
if ((level == 0)||(level>3)) { // if eeprom has illegal value
level=1; // set level to 1 (minimum)
updateprom(); // call updateprom function to set eeprom memory value to minimum too.
}
setleds(); // restore the reed states from EEPROM stored value.
delay(1000);
startstoppump();
}
// FUNCTION THAT SETS THE LEDS ACCORDING TO LEVEL VARIABLE WHENEVER CALLED
void setleds() {
digitalWrite(LED1, LOW );
digitalWrite(LED2, LOW );
digitalWrite(LED3, LOW );
if (level > 0) digitalWrite(LED1, HIGH );
if (level > 1) digitalWrite(LED2, HIGH );
if (level > 2) digitalWrite(LED3, HIGH );
}
// FUNCTION THAT CONTROLS THE PUMP OPERATION ACCORDING TO WATER LEVEL AND AUTO_MAN SETTING
void startstoppump(){
if (level < 2) digitalWrite(Pumppin, HIGH );
if ((level == 3) & (state == 0)) {
digitalWrite(Pumppin, LOW );
}
else {
digitalWrite(Pumppin, HIGH );
}
}
// FUNCTION THAT UPDATES EEPROM AT EACH LEVEL CHANGE
void updateprom() {
// UPDATE LEVEL 1
lastlevel = level; // update lastLevel variable
EEPROM.write(memposL1, level); // and store new Level value to eeprom
}
// MAIN LOOP
void loop () {
// TOGLE AUTO/MANUAL MODE
if ( (digitalRead(Auto_Man_Button) == LOW) & (state == 0) )
{ // Button pushed
state=1;
digitalWrite(Auto_Man_Led, state ); // Turn on the LED
startstoppump();
delay(400);
}
if ( (digitalRead(Auto_Man_Button) == LOW) & (state == 1) ) { // Button pushed
state=0;
digitalWrite(Auto_Man_Led, state ); // Turn off the LED
startstoppump();
delay(400);
}
//END OF TOGLE AUTO/MANUAL MODE
// CHECK WATER LEVEL
if ( digitalRead(Button1) == LOW ) { // Button pushed
level = 1;
}
if ( digitalRead(Button2) == LOW ) { // Button pushed
level = 2;
}
if ( digitalRead(Button3) == LOW ) { // Button pushed
level = 3;
}
//END OF CHECK WATER LEVEL
// IF WATER LEVEL CHANGED AND CALL THE NECESSARY FUNCTIONS.
if (level != lastlevel) {
setleds();
startstoppump();
updateprom();
}
} // END OF MAIN LOOP
That code is nearly impossible to read. The variable names are poor. The indenting is atrocious.
Button1 means tank level 1? How? Variables that contain pin numbers should have pin in the name, so its clear that the variable represents a pin number.
It is quite likely that your problem will become obvious when you use meaning names for variables and use the Tools + Auto Format menu option to make that mess readable.
If not, post the revised code and we'll look at it.
Your text does NOT go inside the code tags.
Please reformat the code as Paul has suggested. Blank lines between functions, each { on its own line and Tools/Auto Format would be a start.
As to the problem, the startstoppump() function will start the pump if level is 2 because you have only tested and acted upon the case where it is < 2 and == 3 so that if it is 2 the else clause is executed and the pump turns on.
Your code looks like this
void startstoppump(){
if (level < 2) digitalWrite(Pumppin, HIGH );
if ((level == 3) & (state == 0)) {
digitalWrite(Pumppin, LOW );
}
else {
digitalWrite(Pumppin, HIGH );
}
}
a tidy veresion looks like this
// FUNCTION THAT CONTROLS THE PUMP OPERATION ACCORDING TO WATER LEVEL AND AUTO_MAN SETTING
void startstoppump()
{
if (level < 2)
{
digitalWrite(Pumppin, HIGH );
}
if ((level == 3) & (state == 0))
{
digitalWrite(Pumppin, LOW );
}
else
{
digitalWrite(Pumppin, HIGH );
}
}
It is much easier to see what is going on in the tidy version, don't you think ?
Thanks a lot Mr. PaulS and UKHeliBob
I have done the auto format and reformat
Removed the else clause from the void startstoppump(), It's working OK.
Do you think this code is good or need more corrections ? Please suggest to improve the same.
Thanks
#include <EEPROM.h>
// CONSTANTS
// INPUT PINS
#define Level1 3
#define Level2 4
#define Level3 5
// OUTPUT PINS
#define LED1 8
#define LED2 9
#define LED3 11
#define Pumppin 13
// EEPROM MEMORY POSITIONS
#define memposL1 0 // set constant eeprom position 0 to save Level1
// variables
int state= 0;
int level = 0;
int lastlevel = 0;
void setup ()
{
// INPUTS
pinMode(Level1,INPUT); // default mode is INPUT
pinMode(Level2,INPUT); // default mode is INPUT
pinMode(Level3,INPUT); // default mode is INPUT
digitalWrite(Level1, HIGH); // Turn on the internal pull-up resistor, default state is HIGH
digitalWrite(Level2, HIGH); // Turn on the internal pull-up resistor, default state is HIGH
digitalWrite(Level3, HIGH); // Turn on the internal pull-up resistor, default state is HIGH
// OUTPUTS
pinMode(Pumppin, OUTPUT);
pinMode(LED1, OUTPUT);
pinMode(LED2, OUTPUT);
pinMode(LED3, OUTPUT);
// Restore tank water Level from eeprom
level = EEPROM.read(memposL1); // Read from eeprom memPos1 the value of Level1
lastlevel = level; // also update the according lastLevel variable
if ((level == 0)||(level>3))
{ // if eeprom has illegal value
level=1; // set level to 1 (minimum)
updateprom(); // call updateprom function to set eeprom memory value to minimum too.
}
setleds(); // restore the reed states from EEPROM stored value.
delay(1000);
startstoppump();
}
// FUNCTION THAT SETS THE LEDS ACCORDING TO LEVEL VARIABLE WHENEVER CALLED
void setleds()
{
digitalWrite(LED1, LOW );
digitalWrite(LED2, LOW );
digitalWrite(LED3, LOW );
if (level > 0) digitalWrite(LED1, HIGH );
if (level > 1) digitalWrite(LED2, HIGH );
if (level > 2) digitalWrite(LED3, HIGH );
}
//FUNCTION THAT CONTROL THE START AND TOP OF THE PUMP
void startstoppump()
{
if (level == 1)
{
digitalWrite(Pumppin, HIGH );
}
if ((level == 3) & (state == 0))
{
digitalWrite(Pumppin, LOW );
}
}
// FUNCTION THAT UPDATES EEPROM AT EACH LEVEL CHANGE
void updateprom()
{
lastlevel = level; // update lastLevel variable
EEPROM.write(memposL1, level); // and store new Level value to eeprom
}
// MAIN LOOP
void loop ()
{
// CHECK WATER LEVEL
if ( digitalRead(Level1) == LOW )
{
level = 1;
}
if ( digitalRead(Level2) == LOW )
{
level = 2;
}
if ( digitalRead(Level3) == LOW )
{
level = 3;
}
//END OF CHECK WATER LEVEL
// IF WATER LEVEL CHANGED AND CALL THE NECESSARY FUNCTIONS.
if (level != lastlevel)
{
setleds();
startstoppump();
updateprom();
}
} // END OF MAIN LOOP
Please suggest to improve the same.
Stylistic changes, yes.
// INPUTS
pinMode(Level1,INPUT); // default mode is INPUT
pinMode(Level2,INPUT); // default mode is INPUT
pinMode(Level3,INPUT); // default mode is INPUT
digitalWrite(Level1, HIGH); // Turn on the internal pull-up resistor, default state is HIGH
digitalWrite(Level2, HIGH); // Turn on the internal pull-up resistor, default state is HIGH
digitalWrite(Level3, HIGH); // Turn on the internal pull-up resistor, default state is HIGH
// OUTPUTS
pinMode(Pumppin, OUTPUT);
pinMode(LED1, OUTPUT);
pinMode(LED2, OUTPUT);
pinMode(LED3, OUTPUT);
Input pin handling is separated from the comment, but there is no separation before or after the next comment. I'd prefer:
// INPUTS
pinMode(Level1,INPUT); // default mode is INPUT
pinMode(Level2,INPUT); // default mode is INPUT
pinMode(Level3,INPUT); // default mode is INPUT
digitalWrite(Level1, HIGH); // Turn on the internal pull-up resistor, default state is HIGH
digitalWrite(Level2, HIGH); // Turn on the internal pull-up resistor, default state is HIGH
digitalWrite(Level3, HIGH); // Turn on the internal pull-up resistor, default state is HIGH
// OUTPUTS
pinMode(Pumppin, OUTPUT);
pinMode(LED1, OUTPUT);
pinMode(LED2, OUTPUT);
pinMode(LED3, OUTPUT);
I like variable names that contain pin numbers to have pin in the name. LEDPin1, LevelPin1, etc.
But, if the code is working, and you don't want to mess with a good thing, I won't be offended.
Thanks lot Mr. PaulS
I shall sincerely act upon your advice to improve my coding in near future.