Debouncing problems with AVR...(likely a misuse of variable).

Thread Starter

liquidair

Joined Oct 1, 2009
192
Hi all-

I'm currently trying to debounce some buttons in an AVR project with Kenneth Kuhn's debounce algorithm. The problem is that I can't seem to get "IF buttonpressed, do something" to work. This is likely a simple problem, here's what I'm doing.

20 times a second a timer interrupt calls the debounce algorithm. Since there's 4 buttons (I'm only testing 1 currently), I've created array variables (buttonInput[4], integrator[4], etc.)to store the data required by the debounce function. If the debounceOutput variable is a 0, i set buttonPressed to 1 and buttonReleased to 0 (and vice versa). Both are global variables.

Then in main, I check to see if the button was pressed. Since I'm learning, I keep it simple and just toggle an LED. But I can't get it to work.

The funny thing is, if I move the "if statement" to the debounce function, it works. I have another function I wrote to check for errors that will simply blink the same LED any number of times based on the integer passed into the function. If I pass buttonPressed[0] into errorBlink from main, it blinks once. So in other words there is a 1 stored into buttonPressed[0] when the button is pressed.

The statement " if(buttonPressed[0] == 1), PORTA ^= 1<<PINA0;" does not work in main, but again, it works in the debounce function.

Any clue what I'm doing wrong?

Here's my code (minus the initialization functions to make the code smaller):

Rich (BB code):
//Debounce
#define DEBOUNCE_TIME 0.2
#define SAMPLE_RATE 20
#define MAXIMUM (DEBOUNCE_TIME * SAMPLE_RATE)

//	Prototypes
//****************************************************************************************
void mgpInit(void);
void ioInit(void);
void errorBlink(unsigned char);
void debounce(uint8_t);


//	Global Variables
//****************************************************************************************

//Debounce Variables
uint8_t refNum = 0;
uint8_t interruptInput[4];
uint8_t integrator[4];
uint8_t debounceOutput[4];
uint8_t buttonPressed[4];
uint8_t buttonReleased[4];

/*****************************************************************************************
*	This function blinks the Warm LED a set number of times defined by parameter
*	PARAM: number of times the LED will Blink
******************************************************************************************/
void errorBlink (unsigned char numOfBlinks)
{
	unsigned char temp = 0;
	
	for(temp = 0; temp < numOfBlinks; temp++)
	{
		PORTA ^= 1<<PINA0;
		_delay_ms(500);
		PORTA ^= 1<<PINA0;
		_delay_ms(500);
	
	}
}

/*****************************************************************************************
*	This function debounces the switches 
*	PARAM: Pin to debounce
*	RETURN: none
*******************************************************************************************/
void debounce (uint8_t refNum)
{
	interruptInput[0]= bit_is_set(PINC,7);		//reads the input of PINC7 and stores it into interruptInput
												//called "interruptInput" since we are actually reading the interrupt
												//output from an IO Expander
	
	//Here we start by reading the input. Since we know a bounce will be a random set of 1's and 0's, we will use
	//the variable "integrator" which will constantly be pulled up or down between 0 and 4 (MAXIMUM). MAXIMUM is
	//determined by multiplying the sample rate by the debounce time (see #defines above).
	if (interruptInput[refNum] == 0)			 
	{											
		if (integrator[refNum] > 0)						
		{										
			integrator[refNum]--;				
		}
	}
	else if (integrator[refNum] < MAXIMUM)
	{
		integrator[refNum] ++;
	}
	
	//Here we test to see if we've reached either threshold.
	if (integrator[refNum] == 0)				//indicates we have had a string of zeros
	{											
		debounceOutput[refNum] = 0;				//sets debounced output to 0 (may not need this)
		buttonPressed[refNum] = 1;				//Used to tells main a button has been pressed
		buttonReleased[refNum] = 0;				//Can't be pressed and released
	}
	else if (integrator[refNum] >= MAXIMUM)		//indicates the button hasn't been pressed or is bouncing
	{
		debounceOutput[refNum] = 1;				//sets debounced output to 1 (may not need this)
		integrator[refNum] = MAXIMUM;			//defensive code
		buttonPressed[refNum] = 0;				//Can't be released and pressed
		buttonReleased[refNum] = 1;				//Used to tell main a button has been released

	}
	
	//if statement will work if placed here
        /*  if(buttonPressed[0] == 1)
		{
			PORTA ^= 1<<PINA0;
		}*/
}


/*****************************************************************************************
*	MAIN
******************************************************************************************/
int main(void)
{
	//	Main Variables
	//**********************************************************************************					

	
	
	//	Initializations
	//**********************************************************************************
	mgpInit();		//Initializes all uC ports and timers.
	i2c_init();		//Initializes the TWI
	ioInit();		//Initializes the IO Expanders
	sei();			//Enables Global Interrupts
	
	//Here is where we would load the previous settings from flash


	
	//Next step is to turn on all of the LEDs
	i2c_start_wait(EQ_IO_ADDR+I2C_WRITE); //Loads the address of CLN IO into the first byte of the message buffer.
	i2c_write(PCA9535_OUT_PORT_CB);		 //Loads the Code to access the output port0 (2) into the command byte of the message buffer.
	i2c_write(0b00000000); //Loads the code to turn the port0 pins to be 0V
	i2c_write(0b00000011); //Tells the output port1 pins to be 0V (except pin7 which is not used).
	i2c_stop(); //sends prepared messageBuf on the TWI bus
	
	//Should have lights on.
	//errorBlink(2);
	

	
	while(1)
	{
                //This will blink the LED 
		//errorBlink(buttonPressed[0]);

                //this code does not toggle LED
		if(buttonPressed[0] == 1)
		{
			PORTA ^= 1<<PINA0;
		}
	
	}
}


/*****************************************************************************************
*	Interrupt Service Routine to debounce switches
*	PARAM: Timer 0 Compare Vector
******************************************************************************************/
ISR(TIMER0_COMPA_vect)
{
	debounce(refNum);
}
 
Last edited:

ErnieM

Joined Apr 24, 2011
8,377
I really tried to follow the debounce code, but there is too much going on without any comments (and I already have a headache).

Copying code is a poor way to learn. Reading good code for ideas about how to write your own code is far better, especially when you re-read your own code and hate it and re-write it next week.

There's probably 1,000 ways to debounce a button in code. It is not a hard, you just want to make sure it has a stable pattern over some period of time.
 

Thread Starter

liquidair

Joined Oct 1, 2009
192
Ah, I didn't even realize I forgot to comment all the debounce code. I really liked it for it's simplicity. I'll work on editing it now so it's easier to follow.

That said, I'm not tied to using that exact code. The problem I am finding is that there's 100's of debounce examples but very few actually show you the implementation of it. For the beginner, this is pretty hard especially if you understand what the code is doing.

You'll be happy to know I actually modified it a bit, but I doubt I could write it any better.

I hope you feel better Ernie.
 

THE_RB

Joined Feb 11, 2008
5,438
The code looks very poor to me. Debouncing needs some timing control, and it looks as though the timing control is entirely from the interrupt timing.

How many buttons do you need to debounce, and does it have to occur in an interrupt? What is the application?
 

Thread Starter

liquidair

Joined Oct 1, 2009
192
You are correct it is through interrupt timing.

I technically have 26 buttons to debounce, but since they are spread over 5 IO expanders each with an interrupt output, I figured I could connect the 5 interrupt outputs to microcontroller pins and poll these pins. If we debounce these outputs, then we can simply read the expander which sent the message and get which button was pressed.

It does not have to occur in an interrupt, I was just trying to not waste processor cycles. The truth however, that the entire application is basically read a button, toggle an LED (write to the IO expanders), and repeat. I could likely get away with debouncing using delays, but I'm wanting to set myself up with good programming techniques for harder and more complicated things I might try in the future.

Thank you for your reply!
 

ErnieM

Joined Apr 24, 2011
8,377
Sorry I was rushing out the door this morning and had to cut my post short.

In my apps I frequently have several buttons... not as many as you have but the principles to debounce work the same.

All debouncing means is the button you read has the same state "a while" later, where "a while" depends on how good your button is. (I've seen some really bad buttons). I've found 25 mS works well for me for "tactile" type buttons (and they are el-cheapo buttons to be sure).

All I do is:

1 - Read buttons

2 - see if the current reading is same as last reading

3 - if same, post new button state

4 - save current reading as last reading

5 - repeat after delay


The code for that isn't hard, maybe a dozen lines I put inside the ISR. In your case I would call each extender at the key tick rate and query for a value.
 

THE_RB

Joined Feb 11, 2008
5,438
I use a similar method to ErnieM but since my programs normally have a 1mS or 10mS timed event, I check for >X contiguous states of the input pin;

Rich (BB code):
// (code for debouncing on stable >=30mS)
  // gets here every 1mS (maybe inside an interrupt)
  if(pin1 == pin1_last) pin1_debounce = 0;
  else 
  {                 
    pin1_debounce++;
    if(pin1_debounce >= 30) pin1_last = ~pin1_last;   // toggle last
  }
At any time the value of pin1_last should represent the last official debounced value of that pin.

It safely debounces both / and \ edges, in either case the pin needs to be stable (in that new state) for at least 30 contiguous samples over 30 mS to be recorded as the official "last" value.
 

Thread Starter

liquidair

Joined Oct 1, 2009
192
Sorry I was rushing out the door this morning and had to cut my post short.
No worries! I'm just glad to have someone helping me!

So, the original problem was that i didn't declare buttonPressed as "volatile".

I thought about the replies thus far and changed my code quite a bit (the "poor" code was my mods), and got the code working pretty good until I actually tried to read the actual I/O expander when a button was pressed. This actually rebounced the button (the interrupt output follows the button pressed until read, in which case it looks like another press)!!

So I scrapped that and tried to work on your post, ErnieM. And, it worked!! I can read at least 2 of the buttons (all I have hooked up right now) but it should theoretically be able to debounce 8 buttons at the same time. Here's what I did:

Rich (BB code):
//called by a timer tick every 20ms or so
void debounce(void)
{
	static uint8_t ret = 0;			//current return from input port
	static uint8_t last = 1;		//previous return from input port, set to 1 to avoid false read on start up
	
	ret = read_from_input_port;		//stores the current value of the input port pins from I2C read
	
	if (ret == last && (!(ret = 0xFF)))		//if ret is the same as last and the port doesn't equal the default state.
	{
		//check to see which button was pressed and do something
		_delay_ms(500);						//adjust time until not sluggish
	}
	
	last = ret;
	
}
See anything wrong? Or is it better than what I had?

Thank you again!
 

Thread Starter

liquidair

Joined Oct 1, 2009
192
THE_RB, I like that! I didn't see your post because I was typing mine.

One of the things I've been struggling with the fact that we only want to do the "do something" code once per full press. The delay gets around this but it doesn't have a way to deal with a stuck button or a long press. That's what I was trying to do in my original post.

It looks like since yours picks up the rising edge, you could say something like "if we've "done this before" and not "released", don't do it again. If "released" we reset "done this before". Is this correct?

Thank you for your help!
 

ErnieM

Joined Apr 24, 2011
8,377
See anything wrong? Or is it better than what I had?
Looks much better (I like short direct sections of code).

Seems you have a comment instead of an action when a key is down. Looks like you got your "edge trigger" implemented.

I would put edge triggering in a different category then debouncing. Similar, but one depends on the other. Coding together is fine.

The comment:
//called by a timer tick every 20ms or so

doesn't work with this line:
_delay_ms(500); //adjust time until not sluggish


I do not fully understand what this line does:
if (ret == last && (!(ret = 0xFF)))
 

Thread Starter

liquidair

Joined Oct 1, 2009
192
I do not fully understand what this line does:
if (ret == last && (!(ret = 0xFF)))
It checks to see if the return from the i2c read is the same as it was last time AND that it doesn't equal the default state of the port we read, which would all be high since no button has been pressed. Otherwise, the if would be true on a not pressed condition as well.

The comment:
//called by a timer tick every 20ms or so

doesn't work with this line:
_delay_ms(500); //adjust time until not sluggish
I probably didn't understand where to put the delay in your steps, but it works to stop the if (ret==last && ...) from repeating over and over every 20ms, assuming that no press will last that long. So after we delay the 500ms (too long), the program will resume reading 0xFF (not pushed) every 20ms. I don't like this bit tho.

I feel like we could use a counter variable to ensure it only goes through the "do something" code once, and reset the counter when we detect the release.
 

ErnieM

Joined Apr 24, 2011
8,377
It checks to see if the return from the i2c read is the same as it was last time AND that it doesn't equal the default state of the port we read, which would all be high since no button has been pressed. Otherwise, the if would be true on a not pressed condition as well.
Inego Montoya said:
You keep using that word. I do not think it means what you think it means.
Read your line again. Slowly. I believe there is a typo there.

Hint: it's the most common C typo involving the "if" statement.
 

Thread Starter

liquidair

Joined Oct 1, 2009
192
Oh, there's 2 actually! It should be:

if((ret == last) && (!(ret == 0xFF)))

Funny thing is that the actual function I wrote has it correct, I made a pseudocode version below main to both sketch and copy to paste on here. Good pick up!!
 

THE_RB

Joined Feb 11, 2008
5,438
...
It looks like since yours picks up the rising edge, you could say something like "if we've "done this before" and not "released", don't do it again. If "released" we reset "done this before". Is this correct?
...
I added one line to the code, that only triggers on the / edge, and runs the function do_something().

Rich (BB code):
// (code for debouncing on stable >=30mS)
  // gets here every 1mS (maybe inside an interrupt)
  if(pin1 == pin1_last) pin1_debounce = 0;
  else
  {                
    pin1_debounce++;
    if(pin1_debounce >= 30) 
    {
      pin1_last = ~pin1_last;   // toggle last
      if(pin1_last != 0) do_something();  // do something on / edge only
    }
  }
 

MMcLaren

Joined Feb 14, 2010
861
One of the things I've been struggling with the fact that we only want to do the "do something" code once per full press. The delay gets around this but it doesn't have a way to deal with a stuck button or a long press.
This is a simple switch state management problem. You've got a switch state latch variable so use it to detect a change in state and filter out all but the desired "new press" or "new release" state for up to eight switches in parallel.

Rich (BB code):
  unsigned char swnew = 0;     //
  unsigned char swold = 0;     // switch state latch
  unsigned char flags = 0;     // switch flag bits

/*
 *  swnew  ___---___---___---___  sample active hi inputs
 *  swold  ____---___---___---__  switch state latch
 *  swnew  ___-__-__-__-__-__-__  changes, press or release
 *  swnew  ___-_____-_____-_____  filter out 'release' bits
 *  flags  ___------______------  toggle flag bits for leds
 */
  void debounce();             // 20 msec sample intervals
  { swnew = readinputport();   // collect active hi inputs
    swnew ^= swold;            // changes, press or release
    swold ^= swnew;            // update switch state latch
    swnew &= swold;            // filter out 'release' bits
    flags ^= swnew;            // toggle flag bits for leds
    putleds(flags);            // refresh LEDs
  }
If switch data is active low and you want to filter for the "new press" state, use swnew = ~readinputport();.

If switch data is active low and you want to filter for the "new release" state, use swnew = readinputport();.

If switch data is active high and you want to filter for the "new release" state, use swnew = ~readinputport();.

Hope this helps.

Cheerful regards, Mike
 

ErnieM

Joined Apr 24, 2011
8,377
The last umteen PIC projects I've done used a series of 5 buttons for input. These fit nicely inside a byte wide variable, which is relatively safe to read or compare. I had the idea once to add some Windows-like events to these buttons, being able to sense key-down and key-up.

I implemented a method to flag these events, but never actually used it so I don't know the pitfalls. The scheme was for the keyscan code (inside an ISR) would set a bit for an event, and the app code would reset the bit when it handled (consumed) the event. I am not so sure the app code should twiddle the bits directly, so some wrapper code would be better. But here's how I detected events:

Rich (BB code):
// define these as globals
unsigned char Keys;             // current debounced key state
unsigned char KeyUp;            // bit set when key goes down (main loop responsible to clear)
unsigned char KeyDown;          // bit set when key goes up   (main loop responsible to clear)

// define these as local
static unsigned char LastKeys = 0xFF;    // saved value of last key scan
unsigned char RawKeys;                    // current read of keys


// code fragment. I place this inside an ISR:
    RawKeys = ReadPort();

    if (LastKeys != RawKeys)
    {
      // we have a new key pressed, save it to see if it is stable
      LastKeys = RawKeys;
    }
    else if (LastKeys != Keys)  // note the "implied" (LastKeys == RawKeys)
                                // due to this being the else clause
    {
      // we have a new stable pattern
      KeyUp   |= ( Keys & ~RawKeys)
      KeyDown |= (~Keys &  RawKeys)
      Keys = RawKeys;     // new stable pattern
    }
 

Thread Starter

liquidair

Joined Oct 1, 2009
192
I realized I had got carried away with things working well that I didn't come back and say "Thank you" to both ErnieM and THE_RB! I was surprised to see new posts, and I really liked seeing some of the cleverness in the new code examples. Mike, you deserve a "Thank you" as well.

It's funny to me that when I took c++ in school I was just ok, occasionally I'd get the rare flash of brilliance but to the most part I didn't really get most of what was going on, just enough to do the assignments.

10 years later, and I pick up uC's and it all starts to click. I am able to get some of the subtlety of what your code does and why c++ is so cool. So thank you gentlemen for helping me get a footing!
 
Top