Button logic

I'm having trouble using buttons in my sketch. This is my first attempt at using them for anything other than turning on an led.

The problem im having is the loop messing with my brain.

pretty simple setup. 2 buttons. Mode Button to change modes. StopGo button to start or stop whatever mode you selected. And a pot to set the level for one of the modes.

I'm currently able to select my mode and start it but that's where things start to fall apart.
I feel like I need a bunch of If/Else statements to keep things flowing properly when it loops. Is that the way its usually done? I searched but found mainly simple press the button to turn on the led or huge menu systems I couldn't quite wrap my head around.

One other issue is my checkflow function doesnt work. When i run it in a standalone sketch it works fine. I suspect the way its looping is off.

It would be great if somebody can take a quick look at my sketch and tell me anything obviously wrong and/or if I'm even heading in the right direction.

thanks

// reading liquid flow rate using Seeeduino and Water Flow Sensor from Seeedstudio.com
// Code adapted by Charles Gantt from PC Fan RPM code written by Crenn @thebestcasescenario.com
// http:/themakersworkbench.com http://thebestcasescenario.com http://seeedstudio.com
#include "SparkSoftLCD.h"
#define LCD_TX 3
SparkSoftLCD lcd = SparkSoftLCD(LCD_TX);
int incomingByte = -1;
int val = 0; 
char code[10]; 
int bytesread = 0;
volatile int NbTopsFan; //measuring the rising edges of the signal
int Calc;  
int hallsensor = 2;    //The pin location of the sensor
float totaldispensed;
int newdevicemode = 0;
int devicemode = 0;
int devicemodebuttonstate;
int devicemodebuttonpin = 4;
int devicemodelastButtonState = 0;

int stopgobutton;
int stopgobuttonstate;
int stopgobuttonpin = 5;
int stopgolastButtonState = 0;

int autofillamount = 0;

int amigoing = 0;

int potPin = 1;
int potval = 0;

void rpm ()     //This is the function that the interupt calls
{
  NbTopsFan++;  //This function measures the rising and falling edge of the hall effect sensors signal
}
// The setup() method runs once, when the sketch starts
void setup() //
{
  pinMode(hallsensor, INPUT); //initializes digital pin 2 as an input
  Serial.begin(9600); //This is the setup function where the serial port is initialised,
  attachInterrupt(0, rpm, RISING); //and the interrupt is attached

  // setup lcd

  pinMode(LCD_TX, OUTPUT);
  lcd.begin(9600);
  lcd.clear();
  Serial.begin(9600);
  // hidden cursor
  lcd.cursor(0);
}
// the loop() method runs over and over again,
// as long as the Arduino has power
void loop ()   
{

  checkbuttons();
  if (amigoing==1){
    if (devicemode==2) { 
      autofill(); 
    }
    if (devicemode==1){ 
      checkflow(); 
    }
  }

}
void checkbuttons()
{


  if (devicemode == 0) {
    lcd.clear();
    lcd.print ("Waiting for your ");
    lcd.cursorTo(2,1);
    lcd.print ("command sir...");
  }
  if (devicemode == 1) {
    //   lcd.clear();
    if (amigoing != 1){
    lcd.cursorTo(1,1);
    lcd.print ("Counter Mode");
  }}
  if (devicemode == 2) {
    //  lcd.clear();
    lcd.cursorTo(1,1);
    lcd.print ("Autofill Mode");
    lcd.cursorTo(2,1);
    lcd.print (autofillamount);
  }
  delay (500);
  devicemodebuttonstate = digitalRead(devicemodebuttonpin);


  // compare the buttonState to its previous state

  if (devicemodebuttonstate != devicemodelastButtonState) {

    // if the state has changed, increment the counter

    if (devicemodebuttonstate == HIGH)

    {
      if (devicemode == 0){
        newdevicemode = 1;
        lcd.clear();
        lcd.print ("Counter Mode");
        delay (500);
        autofillamount = 0;
        // checkflow();
      } 
      if (devicemode == 1) {
        newdevicemode = 2;
        lcd.clear();
        lcd.print ("AutoFill Mode");
        delay (500);
        autofillamount = 0;
        //   autofill();
      }
      if (devicemode == 2) {
        newdevicemode = 0;
        lcd.clear();
        lcd.print ("Turning Off");
        delay (500);
        lcd.clear();
        autofillamount = 0;
      }  
      devicemode = newdevicemode;

    }

    delay(50);

  }

  stopgobuttonstate = digitalRead(stopgobuttonpin);  
  if (stopgobuttonstate != stopgolastButtonState) {

    // if the state has changed, do something

    if (stopgobuttonstate == HIGH)
    { 
      if (amigoing == 0) {

        if (devicemode == 1) {
          lcd.clear();
          lcd.print ("going to checkflow");
          lcd.cursorTo(2,1);
          amigoing = 1;
          delay (5000);
          checkflow();
        }
        if (devicemode == 2) {
          lcd.cursorTo(1,1);
          lcd.print ("AutoFill Amount");
          lcd.cursorTo(2,1);
          lcd.print (autofillamount);
          amigoing = 1;
          autofill();
        }

      }

      else {
        amigoing = 0;

      }
    }


    delay(50);

  }   
  // save the current state as the last state,
  //for next time through the loop
  stopgolastButtonState = stopgobuttonstate;

  // read the pot to get fill amount: 
  potval = analogRead(potPin);
  //map fill to 200 gallon max
  autofillamount = map(potval, 0,1023,1,200);
}



void autofill(){
//  lcd.print("blah");
 // delay(5000);
  lcd.cursorTo(1,1);
  lcd.print ("Filling to:");
  lcd.cursorTo(2,1);
  lcd.print (autofillamount);


}

void checkflow()
{ 
  //lcd.print("counting");
//  delay(5000);

  NbTopsFan = 0;   //Set NbTops to 0 ready for calculations
  sei();      //Enables interrupts
  delay (1000);   //Wait 1 second
  cli();      //Disable interrupts
  lcd.clear();
 /// lcd.print("counting123");
 // delay(5000);

  // block-style blinking cursor
  lcd.cursor(0);

  Calc = (NbTopsFan * 60 / 7.5); //(Pulse frequency x 60) / 7.5Q, = flow rate in L/hour
  lcd.print (Calc, DEC); //Prints the number calculated above
  lcd.print (" L/hour"); //Prints "L/hour" and returns a  new line
  lcd.cursorTo(2,1);

  totaldispensed +=((float)Calc / 3600);
  lcd.print ("total: ");
  delay(5000);

  int intValue = (int)totaldispensed; // convert float PHValue to tricky int combination
  float diffValue = totaldispensed - (float)intValue;
  int anotherIntValue = (int)(diffValue * 1000.0);


  lcd.print (intValue);
  lcd.print (".");
  lcd.print (anotherIntValue);
  lcd.print ("L");
  delay(5000);
}

Your using delay()'s in your code - That's wrong! You may get the result you are looking for if you keep your finger on the button for 10 or 20 seconds.

Other wise start by looking at blink with out delay!.

Mark

Try simplifying some of your statements to make the code more readable. For example

    if (stopgobuttonstate == HIGH)
    { 
      if (amigoing == 0) {

is the same as

    if (stopgobuttonstate == HIGH) &&  (amigoing == 0) 
  {

Try simplifying some of your statements to make the code more readable. For example...is the same as...

True, but the nested if test is better, in my opinion. Concentrate on one action at a time.

pretty simple setup. 2 buttons.

Perhaps you need a slightly more complicated setup that involves some switches, instead of shirt buttons, and some wires, too. You might also tell us exactly how those switches and wires are connected to the Arduino, and why you are not using the internal pullup resistors.

it looks something like this.

I'll rework it to use millis and see what happens.

hey just removing all the delays and cleaning things up seems to have helped a bunch. my button presses seem to be recognized better now too. i dont have to hold the buttons down so long. still a little erratic ill keep working on it.
my checkflow function seems to be working again as well. :wink:
I removed all the delays except in the interupt. ill try removing that one next. I also fixed some places that were clearing the lcd and making it blink really fast.

// reading liquid flow rate using Seeeduino and Water Flow Sensor from Seeedstudio.com
// Code adapted by Charles Gantt from PC Fan RPM code written by Crenn @thebestcasescenario.com
// http:/themakersworkbench.com http://thebestcasescenario.com http://seeedstudio.com
#include "SparkSoftLCD.h"
#define LCD_TX 3
SparkSoftLCD lcd = SparkSoftLCD(LCD_TX);
int incomingByte = -1;
int val = 0; 
char code[10]; 
int bytesread = 0;
volatile int NbTopsFan; //measuring the rising edges of the signal
int Calc;  
int hallsensor = 2;    //The pin location of the sensor
float totaldispensed;
int newdevicemode = 0;
int devicemode = 0;
int devicemodebuttonstate;
int devicemodebuttonpin = 4;
int devicemodelastButtonState = 0;

int stopgobutton;
int stopgobuttonstate;
int stopgobuttonpin = 5;
int stopgolastButtonState = 0;

int autofillamount = 0;

int amigoing = 0;

int potPin = 1;
int potval = 0;

long previousMillis = 0;    
long interval = 1000;
void rpm ()     //This is the function that the interupt calls
{
  NbTopsFan++;  //This function measures the rising and falling edge of the hall effect sensors signal
}
// The setup() method runs once, when the sketch starts
void setup() //
{
  pinMode(hallsensor, INPUT); //initializes digital pin 2 as an input
  Serial.begin(9600); //This is the setup function where the serial port is initialised,
  attachInterrupt(0, rpm, RISING); //and the interrupt is attached

  // setup lcd

  pinMode(LCD_TX, OUTPUT);
  lcd.begin(9600);
  lcd.clear();
  Serial.begin(9600);
  // hidden cursor
  lcd.cursor(0);
}
// the loop() method runs over and over again,
// as long as the Arduino has power
void loop ()   
{
  unsigned long currentMillis = millis();
  checkbuttons();
  if (amigoing==1){
    if (devicemode==2) { 
      autofill(); 
    }
    if (devicemode==1){ 
      checkflow(); 
    }
  }
  else {
    if (devicemode == 0) {
      lcd.cursorTo(1,1);
      lcd.print ("Waiting ");

    }
    if (devicemode == 1) {
      if (amigoing != 1){
        lcd.cursorTo(1,1);
        lcd.print ("Counter Mode");
      }
    }
    if (devicemode == 2) {

      lcd.cursorTo(1,1);
      lcd.print ("Autofill Mode");
      lcd.cursorTo(2,1);
      lcd.print (autofillamount);
    }
  }
}
void checkbuttons()
{

  devicemodebuttonstate = digitalRead(devicemodebuttonpin);


  // compare the buttonState to its previous state

  if (devicemodebuttonstate != devicemodelastButtonState) {

    // if the state has changed, increment the counter

    if (devicemodebuttonstate == HIGH)

    {
      if (devicemode == 0){
        newdevicemode = 1;
        lcd.clear();
        lcd.print ("Counter Mode");
        autofillamount = 0;

      } 
      if (devicemode == 1) {
        newdevicemode = 2;
        lcd.clear();
        lcd.print ("AutoFill Mode");
        autofillamount = 0;

      }
      if (devicemode == 2) {
        newdevicemode = 0;
        lcd.clear();
        lcd.print ("Turning Off");
        lcd.clear();
        autofillamount = 0;
      }  
      devicemode = newdevicemode;

    }



  }

  stopgobuttonstate = digitalRead(stopgobuttonpin);  
  if (stopgobuttonstate != stopgolastButtonState) {

    // if the state has changed, do something

    if (stopgobuttonstate == HIGH)
    { 
      if (amigoing == 0) {

        if (devicemode == 1) {
          amigoing = 1;
          checkflow();
        }
        if (devicemode == 2) {
          amigoing = 1;
          autofill();
        }

      }

      else {
        amigoing = 0;
        devicemode = 0;
        lcd.clear();

      }
    }


  }   
  // save the current state as the last state,
  //for next time through the loop
  stopgolastButtonState = stopgobuttonstate;

  // read the pot to get fill amount: 
  potval = analogRead(potPin);
  //map fill to 200 gallon max
  autofillamount = map(potval, 0,1023,1,200);
}



void autofill(){

  lcd.cursorTo(1,1);
  lcd.print ("Filling to:  ");
  lcd.cursorTo(2,1);
  lcd.print (autofillamount);


}

void checkflow()
{ 

  NbTopsFan = 0;   //Set NbTops to 0 ready for calculations
  sei();      //Enables interrupts
  delay (1000);   //Wait 1 second
  cli();      //Disable interrupts
  lcd.clear();

  // block-style blinking cursor
  lcd.cursor(0);

  Calc = (NbTopsFan * 60 / 7.5); //(Pulse frequency x 60) / 7.5Q, = flow rate in L/hour
  lcd.print (Calc, DEC); //Prints the number calculated above
  lcd.print (" L/hour"); //Prints "L/hour" and returns a  new line
  lcd.cursorTo(2,1);

  totaldispensed +=((float)Calc / 3600);
  lcd.print ("total: ");

  int intValue = (int)totaldispensed; // convert float PHValue to tricky int combination
  float diffValue = totaldispensed - (float)intValue;
  int anotherIntValue = (int)(diffValue * 1000.0);


  lcd.print (intValue);
  lcd.print (".");
  lcd.print (anotherIntValue);
  lcd.print ("L");

}

I removed all the delays except in the interupt.

There should be absolutely NO delays in an interrupt service routine. None. Nada. Zilch. Zip.

Not that there are any in yours.

Paul sometimes your little comments are enough to get the brain going in the right direction without giving away the answer. This is not one of those times...

I tried removing the delay from what I think is the interupt. It stopped working.
It currently works with the one delay but the buttons are still erratic.
this is what I tried instead of using a delay.

void checkflow()
{ 

  NbTopsFan = 0;   //Set NbTops to 0 ready for calculations
  sei();      //Enables interrupts
  unsigned long currentMillis = millis();
 //delay(1000);
  if(currentMillis - previousMillis > interval) {
    // save the last time you blinked the LED 
    previousMillis = currentMillis;   
  cli();      //Disable interrupts
  
  // block-style blinking cursor
  lcd.cursor(0);
lcd.cursorTo(1,1);
  Calc = (NbTopsFan * 60 / 7.5); //(Pulse frequency x 60) / 7.5Q, = flow rate in L/hour
  lcd.print (Calc, DEC); //Prints the number calculated above
  lcd.print (" L/hour"); //Prints "L/hour" and returns a  new line
  }
  lcd.cursorTo(2,1);

  totaldispensed +=((float)Calc / 3600);
  lcd.print ("total: ");

  int intValue = (int)totaldispensed; // convert float PHValue to tricky int combination
  float diffValue = totaldispensed - (float)intValue;
  int anotherIntValue = (int)(diffValue * 1000.0);


  lcd.print (intValue);
  lcd.print (".");
  lcd.print (anotherIntValue);
  lcd.print ("L");

}

I didnt get any compile errors but it stopped counting.
What should i be looking at to rid myself of the last delay and to start figuring out why button presses sometimes skip around and sometimes have to hold the button down for a while to get it to recognize the press.

Paul sometimes your little comments are enough to get the brain going in the right direction without giving away the answer. This is not one of those times...

Let's recap. You said:

I removed all the delays except in the interupt.

and I said:

There should be absolutely NO delays in an interrupt service routine. None. Nada. Zilch. Zip.

Not that there are any in yours.

So, clearly, there was nothing wrong with your interrupt handler.

I don't understand this, though:

  sei();      //Enables interrupts
  delay (1000);   //Wait 1 second
  cli();      //Disable interrupts

Interrupts are not allowed to happen, for ANY interrupts, including the clock ticking, except while in this function. Not a good idea.

You need to rethink how you check the flow/interrupts per unit time. The blink without delay example bears looking at.

You really need to stop disabling interrupts.

ok i need to step back and verify i know what this part of the code is actually doing.
This is my first time using interupts. That whole part of the code just came from the demo sketch with the device. Then I frankensteined the rest on.

rpm() just takes the variable NbTopsFan and increments it by one.

NbTopsFan is a Volatile variable so it gets saved to ram since it can be changed outside the part of the code its in.

attachInterrupt(0, rpm, RISING); I think this means that when it detects an rising interrupt on Pin 2 to run rpm() which then increments NbTopsFan.

so what i think is happening is
the interrupt is enabled
it waits for 1 second
when the interrupt is enabled rpm() is called and NbTopsFan starts being incremented. Im fuzzy on what it is actually counting based on the RISING.
the interrupt is disabled
calculation to get flow
display flow on lcd

a problem i see with this method is that I think all timing including millis and delay dont work properly when interrupts are disabled. this doesnt really matter if all the program is doing is looping and displaying the flow. But it does affect me since I do have other stuff happening in the sketch and once i disable the interrupts that stuff starts acting funny, in laymans terms.

So I suppose the question I should be asking is whats the best way to count the RISING without disabling interrupts as part of the process.
Or how do I reenable interrupts without calling rpm().
Or maybe I should be trying to do it without ever disabling the interrupts.

Am I heading down the right rabbit hole now?

a problem i see with this method is that I think all timing including millis and delay dont work properly when interrupts are disabled.

True.

Instead of enabling and disabling interrupts the way that you are, what you need to do is record when you last collected data each time you collect data. Then, on each pass through loop(), you see if is time to collect data again. If it is, then you disable interrupts, copy and reset the count, copy and reset the last time, and re-enable interrupts. Total lost rpm interrupts will be the time needed to copy an int (or long), set an int (or long), and copy a long, and set a long by calling millis().

Then, use the copies and the time that the copies were made to determine the time during which counting occurred, and then use count and counting time to determine rpm.

jointtech:
hey just removing all the delays and cleaning things up seems to have helped a bunch. my button presses seem to be recognized better now too. i dont have to hold the buttons down so long. still a little erratic ill keep working on it.
my checkflow function seems to be working again as well. :wink:
I removed all the delays except in the interupt. ill try removing that one next. I also fixed some places that were clearing the lcd and making it blink really fast.

There are a couple of things I can see in the code. But there is a larger issue that I will get to below.

First, you don't need to, and should not, use cli/sei. Don't Do That. There are only a few very special scenarios where you need to disable interrupts, and this isn't one of them.

Second, you could probably eliminate all the floating point operations by using a little algebra.
For example, a * 60 / 7.5 is equivalent to a * 120 / 15. There's more that can be done to completely convert to integer math, but I won't get into that here. For what you are doing, it probably works fine.

Also, the delay(1000) in checkflow is going to cause you some grief. During that 1 second
your sketch can't do anything else, like check the buttons. That may be OK in this case, but
it's going to slow down the response time. Do you want to wait 1 second to see the results of a button press? I myself would get impatient and maybe press the button again.
Instead of using a fixed delay, take a timestamp (millis()) and clear the NbTopsFan counter
during setup. Then when doing checkflow, you have the elapsed time and the counter value, and can calculate the rate. After doing the calculation, clear the counter and reset the timestamp, for the next time. One thing to watch out for is overflow of either millis or the counter. You can avoid that by resetting them both in loop() whenever they get too large.

However, the biggest issue that I see is that your buttons are not debounced. That means that in some cases you are going to press the button once, and it will act like it was pressed several times.
Fortunately for you, this is a problem that has been extensively studied and there are many solutions. Your task is going to be to integrate them into your sketch.
An excellent essay on debouncing can be found here Debouncing Contacts and Switches.
And there are libraries in the playground made for deboucing switches just like this.
Arduino Playground - Bounce
Arduino Playground - SoftwareDebounce

Putting it all together... well that's the fun part.

thanks for the comments guys. Ill digest and do some more research and see what I come up with.
Wow never new something as seemingly simple as a button could cause so much grief regarding bouncing. Maybe I should just put in a switch and be done with it. But where would the fun be there ? :wink:

Maybe I should just put in a switch and be done with it

Switch, button doesn't matter - anything that involves making and breaking a circuit using mechanical contacts will bounce.

bah my dreams are dashed :wink: I spent a few minutes thinking about using a switch but it wont work like I want anyway. So on to debouncing. That first article referenced above is about 200 level stuff and I'm still taking the intro classes but I'll slog on and figure it out like I usually do.