[Solved] Problems adding a value to an integer

In the following, I stay in the while loop until one of the buttons is pressed. When a button is pressed, I want to add 5 (and in the other case subtract 5) to x, and print this new x. I want to stay in the loop after this has been done. So that 3 presses on the button "up" gives 25+5+5+5 = 40 and prints this. Two presses on up and one on down gives 25+5+5-5=30. Right now, a press on up gives me a value above 100. Anyone see the problem? Tell me if you need more of the code. UPDATE: Added the rest of the relevant code in reply #3

int x = 25;
int y;
x += y;
x -= y;
lcd.print(x);               //Prints 25, the starting value
lcd.setCursor(2,1);
lcd.print("g");
while (true)
  {
  state = read_LCD_buttons();
  if (state == 1)                   //When "up" button is pressed
    {
      x += 5;
      lcd.setCursor(0,1);
      lcd.print(x);           
    }
  else if (state == 2)              //When "down" button is pressed
    {
      x -= 5;
      lcd.setCursor(0,1);
      lcd.print(x);
    }

You increment whenever the button is down, not when it becomes pushed down. At least in that code. So it would be better to post the whole thing.

Yep, try using the BOunce2 library and use the button.fell()

Also, skip the while loop. The loop function of Arduino is already running, use that :wink:

Here is the rest of the relevant code

/*-----( Declare Constants )-----*/
#define btnRIGHT  0
#define btnUP     1
#define btnDOWN   2
#define btnLEFT   3
#define btnSELECT 4
#define btnNONE   5

/*-----( Declare Variables )-----*/
int lcd_key       = 0;
int adc_key_in    = 0;
int adc_key_prev  = 0;

void loop() {

adc_key_prev = lcd_key ;
lcd_key = read_LCD_buttons();

int read_LCD_buttons()
{
  adc_key_in = analogRead(0);      // read the value from the sensor 
  delay(5); //switch debounce delay. Increase this delay if incorrect switch selections are returned.
  int k = (analogRead(0) - adc_key_in); //gives the button a slight range to allow for a little contact resistance noise
  if (5 < abs(k)) return btnNONE;  // double checks the keypress. If the two readings are not equal +/-k value after debounce delay, it tries again.
  // my buttons when read are centered at these valies: 0, 144, 329, 504, 741
  // we add approx 50 to those values and check to see if we are close
  if (adc_key_in > 1000) return btnNONE; // We make this the 1st option for speed reasons since it will be the most likely result
  if (adc_key_in < 50)   return btnRIGHT;  
  if (adc_key_in < 195)  return btnUP; 
  if (adc_key_in < 380)  return btnDOWN; 
  if (adc_key_in < 555)  return btnLEFT; 
  if (adc_key_in < 790)  return btnSELECT;   
  return btnNONE;  // when all others fail, return this...
}

That’s gibberish code. Not compiling etc. But yeay, get the point. Doesn’t change a thing. Only you can’t use Bounce2 because of the analog read of switches. Try the find the moment the button became pressed and only act then.

Some comments

while (true)

Like I said, loop is already a infinite loop so use that.

/*-----( Declare Variables )-----*/
int lcd_key       = 0;
int adc_key_in    = 0;
int adc_key_prev  = 0;
  1. It’s more common the write lcdKey, adcKey etc
  2. There all variables only used in one function. Just declare them there!
adc_key_in = analogRead(0);      // read the value from the sensor

Use a variable name for the pin

delay(5); //switch debounce delay. Increase this delay if incorrect switch selections are returned.

Mwaa, try to skip the damn delay(), use millis to not block the program.

I would do something like

const byte AnalogButtonPin = A0;
const byte ButtonDebounceInterval = 5;

byte read_LCD_buttons(){
  static unsigned int pressMillis = 0;
  static byte lastStableKey = btnNONE;
  static byte lastUnstableKey = btnNONE;
  
  byte key = btnNONE;
  unsigned int nowMillis -= millis(); 
  
  int adcKeyIn = analogRead(AnalogButtonPin); // read the value from the button
  
  if(adcKeyIn < 50)
    key = btnRIGHT;
  else if(adcKeyIn < 195)
    key = btnUP;
  else if(adcKeyIn < 380)
    key = btnDOWN;
  else if(adcKeyIn < 555)
    key = btnLEFT;
  else if(adcKeyIn < 790)
    key = btnSELECT;
  
  if(key != lastUnstableKeyKey){
    pressMillis = nowMillis;
    lastUnstableKeyKey = key;
  }
  //last time was the same key
  else if( nowMillis - pressMillis > ButtonDebounceInterval){
    //bounce interval over
    if(lastStableKey != key){
      //only send out when a button got pressed
      lastStableKey = key;
      return key;
  }
  
  return btnNONE;
}

Thanks a lot for the answer. I’m aware that the code isn’t pretty. It’s been 2 years since I last had a course in uni. I’m using this for a 10 week project in school. So it just needs to work, not be pretty :slight_smile:

I solved it by simply inserting a delay in the code

while (true)
  {
  state = read_LCD_buttons();
  if (state == 1)                   //When "up" button is pressed
    {
      x += 5;
      lcd.setCursor(0,1);
      lcd.print(x);           
      delay(1000); <--------------------Solution 
    }
  else if (state == 2)              //When "down" button is pressed
    {
      x -= 5;
      lcd.setCursor(0,1);
      lcd.print(x);
      delay(1000); <--------------------Solution
    }

septillion: delay(5); //switch debounce delay. Increase this delay if incorrect switch selections are returned.

Mwaa, try to skip the damn delay(), use millis to not block the program. [/code]

This sort of thing is where I break my own rule and use delay(), or better yet, delayMicroseconds(). If it's only 5 milliseconds in a program which does not particularly need to be doing anything else just then, I don't see a problem. I consider it a small price to pay for making my code more readable.

Andfj:
Thanks a lot for the answer. I’m aware that the code isn’t pretty. It’s been 2 years since I last had a course in uni. I’m using this for a 10 week project in school. So it just needs to work, not be pretty :slight_smile:

I solved it by simply inserting a delay in the code

while (true)

{
  state = read_LCD_buttons();
  if (state == 1)                  //When “up” button is pressed
    {
      x += 5;
      lcd.setCursor(0,1);
      lcd.print(x);         
      delay(1000); <--------------------Solution
    }
  else if (state == 2)              //When “down” button is pressed
    {
      x -= 5;
      lcd.setCursor(0,1);
      lcd.print(x);
      delay(1000); <--------------------Solution
    }

Not good. Try tapping either button rapidly (more than once per second) to see the problem. Either that or try holding either button down for more than one second.

Hint: check for changes in button state, not just the state itself.

odometer: Not good. Try tapping either button rapidly (more than once per second) to see the problem. Either that or try holding either button down for more than one second.

Hint: check for changes in button state, not just the state itself.

It is actually a really good solution in my case. The user of my screen needs to choose an amount (starting at 25) when I hold a button down, it adds 5 every second. Which is great, since that is the purpose of the button, i.e. adding 5 to an amount. And if it adds to much, you just press the other button. So no problems with the solution.

Andfj: It is actually a really good solution in my case. The user of my screen needs to choose an amount (starting at 25) when I hold a button down, it adds 5 every second. Which is great, since that is the purpose of the button, i.e. adding 5 to an amount. And if it adds to much, you just press the other button. So no problems with the solution.

That will be okay until a time comes when you decide to add other functionality. Then everything will often come to a screeching train wreck halt whenever the button is pushed. Just a heads up for your future projects.

Also, won't it be frustrating for your users, that they can't increment quickly, by pushing the button at normal speeds? That is how modern people expect a button to work.

It wouldn't be the first time a programmer tried to redefine reality to suit the limitations of the program. Bad habit.

aarg:
Also, won’t it be frustrating for your users, that they can’t increment quickly, by pushing the button at normal speeds? That is how modern people expect a button to work.

This.