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

Discussion in 'Programmer's Corner' started by liquidair, Oct 21, 2013.

  1. liquidair

    Thread Starter Active Member

    Oct 1, 2009
    89
    5
    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):

    Code ( (Unknown Language)):
    1.  
    2. //Debounce
    3. #define DEBOUNCE_TIME 0.2
    4. #define SAMPLE_RATE 20
    5. #define MAXIMUM (DEBOUNCE_TIME * SAMPLE_RATE)
    6.  
    7. //  Prototypes
    8. //****************************************************************************************
    9. void mgpInit(void);
    10. void ioInit(void);
    11. void errorBlink(unsigned char);
    12. void debounce(uint8_t);
    13.  
    14.  
    15. //  Global Variables
    16. //****************************************************************************************
    17.  
    18. //Debounce Variables
    19. uint8_t refNum = 0;
    20. uint8_t interruptInput[4];
    21. uint8_t integrator[4];
    22. uint8_t debounceOutput[4];
    23. uint8_t buttonPressed[4];
    24. uint8_t buttonReleased[4];
    25.  
    26. /*****************************************************************************************
    27. *   This function blinks the Warm LED a set number of times defined by parameter
    28. *   PARAM: number of times the LED will Blink
    29. ******************************************************************************************/
    30. void errorBlink (unsigned char numOfBlinks)
    31. {
    32.     unsigned char temp = 0;
    33.    
    34.     for(temp = 0; temp < numOfBlinks; temp++)
    35.     {
    36.         PORTA ^= 1<<PINA0;
    37.         _delay_ms(500);
    38.         PORTA ^= 1<<PINA0;
    39.         _delay_ms(500);
    40.    
    41.     }
    42. }
    43.  
    44. /*****************************************************************************************
    45. *   This function debounces the switches
    46. *   PARAM: Pin to debounce
    47. *   RETURN: none
    48. *******************************************************************************************/
    49. void debounce (uint8_t refNum)
    50. {
    51.     interruptInput[0]= bit_is_set(PINC,7);      //reads the input of PINC7 and stores it into interruptInput
    52.                                                 //called "interruptInput" since we are actually reading the interrupt
    53.                                                 //output from an IO Expander
    54.    
    55.     //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
    56.     //the variable "integrator" which will constantly be pulled up or down between 0 and 4 (MAXIMUM). MAXIMUM is
    57.     //determined by multiplying the sample rate by the debounce time (see #defines above).
    58.     if (interruptInput[refNum] == 0)             
    59.     {                                          
    60.         if (integrator[refNum] > 0)                    
    61.         {                                      
    62.             integrator[refNum]--;              
    63.         }
    64.     }
    65.     else if (integrator[refNum] < MAXIMUM)
    66.     {
    67.         integrator[refNum] ++;
    68.     }
    69.    
    70.     //Here we test to see if we've reached either threshold.
    71.     if (integrator[refNum] == 0)                //indicates we have had a string of zeros
    72.     {                                          
    73.         debounceOutput[refNum] = 0;             //sets debounced output to 0 (may not need this)
    74.         buttonPressed[refNum] = 1;              //Used to tells main a button has been pressed
    75.         buttonReleased[refNum] = 0;             //Can't be pressed and released
    76.     }
    77.     else if (integrator[refNum] >= MAXIMUM)     //indicates the button hasn't been pressed or is bouncing
    78.     {
    79.         debounceOutput[refNum] = 1;             //sets debounced output to 1 (may not need this)
    80.         integrator[refNum] = MAXIMUM;           //defensive code
    81.         buttonPressed[refNum] = 0;              //Can't be released and pressed
    82.         buttonReleased[refNum] = 1;             //Used to tell main a button has been released
    83.  
    84.     }
    85.    
    86.     //if statement will work if placed here
    87.         /*  if(buttonPressed[0] == 1)
    88.         {
    89.             PORTA ^= 1<<PINA0;
    90.         }*/
    91. }
    92.  
    93.  
    94. /*****************************************************************************************
    95. *   MAIN
    96. ******************************************************************************************/
    97. int main(void)
    98. {
    99.     //  Main Variables
    100.     //**********************************************************************************                   
    101.  
    102.    
    103.    
    104.     //  Initializations
    105.     //**********************************************************************************
    106.     mgpInit();      //Initializes all uC ports and timers.
    107.     i2c_init();     //Initializes the TWI
    108.     ioInit();       //Initializes the IO Expanders
    109.     sei();          //Enables Global Interrupts
    110.    
    111.     //Here is where we would load the previous settings from flash
    112.  
    113.  
    114.    
    115.     //Next step is to turn on all of the LEDs
    116.     i2c_start_wait(EQ_IO_ADDR+I2C_WRITE); //Loads the address of CLN IO into the first byte of the message buffer.
    117.     i2c_write(PCA9535_OUT_PORT_CB);      //Loads the Code to access the output port0 (2) into the command byte of the message buffer.
    118.     i2c_write(0b00000000); //Loads the code to turn the port0 pins to be 0V
    119.     i2c_write(0b00000011); //Tells the output port1 pins to be 0V (except pin7 which is not used).
    120.     i2c_stop(); //sends prepared messageBuf on the TWI bus
    121.    
    122.     //Should have lights on.
    123.     //errorBlink(2);
    124.    
    125.  
    126.    
    127.     while(1)
    128.     {
    129.                 //This will blink the LED
    130.         //errorBlink(buttonPressed[0]);
    131.  
    132.                 //this code does not toggle LED
    133.         if(buttonPressed[0] == 1)
    134.         {
    135.             PORTA ^= 1<<PINA0;
    136.         }
    137.    
    138.     }
    139. }
    140.  
    141.  
    142. /*****************************************************************************************
    143. *   Interrupt Service Routine to debounce switches
    144. *   PARAM: Timer 0 Compare Vector
    145. ******************************************************************************************/
    146. ISR(TIMER0_COMPA_vect)
    147. {
    148.     debounce(refNum);
    149. }
    150.  
     
    Last edited: Oct 21, 2013
  2. ErnieM

    AAC Fanatic!

    Apr 24, 2011
    7,395
    1,607
    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.
     
  3. liquidair

    Thread Starter Active Member

    Oct 1, 2009
    89
    5
    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.
     
  4. liquidair

    Thread Starter Active Member

    Oct 1, 2009
    89
    5
    Original code has been commented.
     
  5. THE_RB

    AAC Fanatic!

    Feb 11, 2008
    5,435
    1,305
    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?
     
  6. liquidair

    Thread Starter Active Member

    Oct 1, 2009
    89
    5
    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!
     
  7. ErnieM

    AAC Fanatic!

    Apr 24, 2011
    7,395
    1,607
    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.
     
  8. THE_RB

    AAC Fanatic!

    Feb 11, 2008
    5,435
    1,305
    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;

    Code ( (Unknown Language)):
    1.  
    2. // (code for debouncing on stable >=30mS)
    3.   // gets here every 1mS (maybe inside an interrupt)
    4.   if(pin1 == pin1_last) pin1_debounce = 0;
    5.   else
    6.   {                
    7.     pin1_debounce++;
    8.     if(pin1_debounce >= 30) pin1_last = ~pin1_last;   // toggle last
    9.   }  
    10.  
    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.
     
  9. liquidair

    Thread Starter Active Member

    Oct 1, 2009
    89
    5
    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:

    Code ( (Unknown Language)):
    1.  
    2.  
    3. //called by a timer tick every 20ms or so
    4. void debounce(void)
    5. {
    6.     static uint8_t ret = 0;         //current return from input port
    7.     static uint8_t last = 1;        //previous return from input port, set to 1 to avoid false read on start up
    8.    
    9.     ret = read_from_input_port;     //stores the current value of the input port pins from I2C read
    10.    
    11.     if (ret == last && (!(ret = 0xFF)))     //if ret is the same as last and the port doesn't equal the default state.
    12.     {
    13.         //check to see which button was pressed and do something
    14.         _delay_ms(500);                     //adjust time until not sluggish
    15.     }
    16.    
    17.     last = ret;
    18.    
    19. }
    20.  
    See anything wrong? Or is it better than what I had?

    Thank you again!
     
  10. liquidair

    Thread Starter Active Member

    Oct 1, 2009
    89
    5
    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!
     
  11. ErnieM

    AAC Fanatic!

    Apr 24, 2011
    7,395
    1,607
    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)))
     
  12. liquidair

    Thread Starter Active Member

    Oct 1, 2009
    89
    5
    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.

    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.
     
  13. ErnieM

    AAC Fanatic!

    Apr 24, 2011
    7,395
    1,607
    Read your line again. Slowly. I believe there is a typo there.

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

    Thread Starter Active Member

    Oct 1, 2009
    89
    5
    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!!
     
  15. THE_RB

    AAC Fanatic!

    Feb 11, 2008
    5,435
    1,305
    I added one line to the code, that only triggers on the / edge, and runs the function do_something().

    Code ( (Unknown Language)):
    1.  
    2. // (code for debouncing on stable >=30mS)
    3.   // gets here every 1mS (maybe inside an interrupt)
    4.   if(pin1 == pin1_last) pin1_debounce = 0;
    5.   else
    6.   {                
    7.     pin1_debounce++;
    8.     if(pin1_debounce >= 30)
    9.     {
    10.       pin1_last = ~pin1_last;   // toggle last
    11.       if(pin1_last != 0) do_something();  // do something on / edge only
    12.     }
    13.   }
    14.  
     
  16. MMcLaren

    Well-Known Member

    Feb 14, 2010
    759
    116
    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.

    Code ( (Unknown Language)):
    1.  
    2.  
    3.   unsigned char swnew = 0;     //
    4.   unsigned char swold = 0;     // switch state latch
    5.   unsigned char flags = 0;     // switch flag bits
    6.  
    7. /*
    8.  *  swnew  ___---___---___---___  sample active hi inputs
    9.  *  swold  ____---___---___---__  switch state latch
    10.  *  swnew  ___-__-__-__-__-__-__  changes, press or release
    11.  *  swnew  ___-_____-_____-_____  filter out 'release' bits
    12.  *  flags  ___------______------  toggle flag bits for leds
    13.  */
    14.   void debounce();             // 20 msec sample intervals
    15.   { swnew = readinputport();   // collect active hi inputs
    16.     swnew ^= swold;            // changes, press or release
    17.     swold ^= swnew;            // update switch state latch
    18.     swnew &= swold;            // filter out 'release' bits
    19.     flags ^= swnew;            // toggle flag bits for leds
    20.     putleds(flags);            // refresh LEDs
    21.   }
    22.  
    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
     
  17. ErnieM

    AAC Fanatic!

    Apr 24, 2011
    7,395
    1,607
    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:

    Code ( (Unknown Language)):
    1. // define these as globals
    2. unsigned char Keys;             // current debounced key state
    3. unsigned char KeyUp;            // bit set when key goes down (main loop responsible to clear)
    4. unsigned char KeyDown;          // bit set when key goes up   (main loop responsible to clear)
    5.  
    6. // define these as local
    7. static unsigned char LastKeys = 0xFF;    // saved value of last key scan
    8. unsigned char RawKeys;                    // current read of keys
    9.  
    10.  
    11. // code fragment. I place this inside an ISR:
    12.     RawKeys = ReadPort();
    13.  
    14.     if (LastKeys != RawKeys)
    15.     {
    16.       // we have a new key pressed, save it to see if it is stable
    17.       LastKeys = RawKeys;
    18.     }
    19.     else if (LastKeys != Keys)  // note the "implied" (LastKeys == RawKeys)
    20.                                 // due to this being the else clause
    21.     {
    22.       // we have a new stable pattern
    23.       KeyUp   |= ( Keys & ~RawKeys)
    24.       KeyDown |= (~Keys &  RawKeys)
    25.       Keys = RawKeys;     // new stable pattern
    26.     }
    27.      
     
  18. liquidair

    Thread Starter Active Member

    Oct 1, 2009
    89
    5
    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!
     
    THE_RB and ErnieM like this.
Loading...