Detecting long button presses using timers

[newtekuser]

I have three buttons hooked up to RB0-2 to control a user menu and the ISR handles the button press detection, but now I'd like it to detect when the button connected to RB2 is pressed more than let's say 1 second to lit an LED connected to RC0.  Various posts and articles I've read indicate usage of timers.
For my purpose I've tried using the Timer1 function in my PIC16F887 (driving it off the 8mhz internal oscillator), but my approach is not working correctly and now the LED gets lit when I press any of the three buttons regardless of the time passed.

Code: [Select]

void main(void)
{
    OSCCON =  0x70;     
    ANSEL = 0b0000000; 
    ANSELH = 0b0000000;
   
    INTCONbits.INTE = 1;       
    OPTION_REGbits.INTEDG = 0; 
    INTCONbits.GIE = 1;         
    INTCONbits.PEIE = 1;       
    INTCONbits.RBIE = 1;
    IOCBbits.IOCB0 = 1;   
    IOCBbits.IOCB1 = 1;   
    IOCBbits.IOCB2 = 1;   
   
    TRISA = 0x0;
    TRISC = 0x0;   
    PORTA = 0x0;
    PORTC = 0x0;
   
    TRISBbits.TRISB0 = 1;
    TRISBbits.TRISB1 = 1;
    TRISBbits.TRISB2 = 1;
    TRISCbits.TRISC0 = 0;

    T1CONbits.TMR1CS = 0;   
    T1CONbits.T1CKPS = 0b11;

    while(1)
    {

// displaying menu, etc. code here
    }

}


void __interrupt() isr(void) {
    if (INTCONbits.INTF)
    {       
        // do stuff when RB0 button pressed

        INTCONbits.INTF = 0;
    }
   
    if (INTCONbits.RBIF)
    {
        static unsigned char pb_last;             
        unsigned char pb_now = PORTB;
       
        unsigned char pb_changed = (pb_last ^ pb_now) & pb_last;
        pb_last = pb_now;   

        if (pb_changed & 0b10)
        {

            // do stuff when RB1 button pressed
        }

        if (pb_changed & 0b100)
        {   
           
            // do stuff when RB2 button pressed

            T1CONbits.TMR1ON = 1;  // start timer
        }
        else
        {
           // handle the long button press when RB2 button is pressed for more than x seconds
           T1CONbits.TMR1ON = 0; // Stop Timer1
           unsigned int timer1_value;
           timer1_value = (TMR1H << 8) | TMR1L;
           if (timer1_value >= 64535)
             RC0 = 1; // LIT LED
        }       
       
        INTCONbits.RBIF = 0;
    }
}
Thank you in advance!

[PlainName]

A while since I did PICs, so just to help us out and help yourself this time next year when you've forgotten what it all meant, just add a comment on the end of those register settings to say wtf they do. Sometimes you'll find that as you write the comment you're realise that's not what the value is indicating and fix a bug early.

As I see the ISR, you press some button (tip: #define 0b100 to a meaningful name) and that kicks off the timer. But how do you detect a release? If there is no interrupt (not sure if you're triggering on both edges or just one) then nothing happens because the ISR isn't triggered, and if you do interrupt on a button action then the timer stuff is ignored because the button change circumvents it (the 'if (pb_change & 0b100)' is never false, unless a different button is pressed).

Next, there is no timer interrupt so all that happens is you read a random value from the timer (if you even hit that part of the code) and since it wraps in about 16ms with an 8MHz clock (or is there some divider?) I don't think anything you get from it is meaningful. The window for testing >= 64535 is 250us.

The code is fixable, but I think you need a different approach since there is no debounce with this version. If you add debounce stuff then short, long, extra long and stuck detections are relatively simple extensions of that.

and now the LED gets lit when I press any of the three buttons regardless of the time passed

That might be because pressing a button other than the 0b100 causes the timer part of the code to run, which may light the LED.

[MikeK]

My preference for a button press ISR is to setup a timer to interrupt every, let's say, 5ms (or choose your own value).  In the ISR you check if the button is pressed.  If pressed, you increment a counter.  If not pressed, you zero the counter.  Then you choose what value of the counter as a threshold.

[PlainName]

That's reasonably simple, but the ISR is running all the time (not literally, of course, but with whatever timer period), and since the vast majority of time nothing is happening it's wasted cycles. A middle way would be to have the button ISR start the timer interrupts, which would then turn off once the relevant timing periods have completed. Thus the timer polling would only occur when it's actually being used for something.

[nctnico]

That's reasonably simple, but the ISR is running all the time (not literally, of course, but with whatever timer period), and since the vast majority of time nothing is happening it's wasted cycles. A middle way would be to have the button ISR start the timer interrupts, which would then turn off once the relevant timing periods have completed. Thus the timer polling would only occur when it's actually being used for something.

The downside with having a button cause interrupts in one form on another is that it is easy to setup a system that gets bogged down by a button going bad or getting filled with water. Using a fixed timer interrupt is a better solution as this results in a constant load on the system. This in turn makes it far easier to prove correct functionality of the product under all circumstances.

[DavidAlfa]

Yeah, the running the timer interrupt is a much better approach, keeping the ISR small shouldn't impact the performance too much.

(I left this as excercise ) Configure Timer1 to overflow every 1ms, enable interrupts (GIE and PIE), start timer.
Timing accuracy is not really required for simple button management, if you get 1,024ms or 0,975ms it's just fine.
Then try this code:

Code: [Select]

void __interrupt() isr(void) {
  if(PIR1bits.TMR1IF)
  {
    static unsigned int RB1_count, RB2_count;
    static unsigned char pb_last;   
    unsigned char pb_now = PORTB;   
    unsigned char pb_changed = (pb_last ^ pb_now) & pb_last;
    pb_last = pb_now;
   
    PIR1bits.TMR1IF = 0;
   
    /********************************************************************************************/
    /*            Instant press actions                                                         */
    /********************************************************************************************/
    if (pb_changed & 0b10)                        /* RB1 pressed */
    {
      RB1_count = 1;                              /* Start RB1 counter */
      // do stuff when RB1 button pressed
    }
    else                                          /* Not pressed */
    {
      RB1_count = 0;                              /* Disable RB1 counter */
    }

    if (pb_changed & 0b100)                       /* RB2 pressed */
    {
      RB2_count = 1;                              /* Start RB2 counter */
      // do stuff when RB2 button pressed
    }
    else                                          /* Not pressed */
    {
      RB2_count = 0;                              /* Disable RB2 counter */
    }
   
   
    /********************************************************************************************/
    /*            Long press actions                                                            */   
    /********************************************************************************************/
    if (RB1_count && ++RB1_count>1000)            /* RB1 counter enabled, increase, if higher than 1000ms */
    {
      RB1_count = 0;                              /* Disable counter */
      // do stuff when RB1 button is long pressed
    }
   
    if (RB2_count && ++RB2_count>1000)            /* RB2 counter enabled, increase, if higher than 1000ms */
    {
      RB2_count = 0;                              /* Disable counter */
      // do stuff when RB2 button is long pressed
    }
  }
}


[nctnico]

Polling buttons at 20ms intervals is OK. Typically I want to see a 'push' that lasts at around 80ms to 100ms. Beyond 100ms you start to notice a delay.

[PlainName]

That's reasonably simple, but the ISR is running all the time (not literally, of course, but with whatever timer period), and since the vast majority of time nothing is happening it's wasted cycles. A middle way would be to have the button ISR start the timer interrupts, which would then turn off once the relevant timing periods have completed. Thus the timer polling would only occur when it's actually being used for something.

The downside with having a button cause interrupts in one form on another is that it is easy to setup a system that gets bogged down by a button going bad or getting filled with water. Using a fixed timer interrupt is a better solution as this results in a constant load on the system. This in turn makes it far easier to prove correct functionality of the product under all circumstances.

Can be, yes. But you'd use the button ISR to kick off the timers and disable the button interrupts. Only once the timer stuff has ended does the button interrupt get reenabled.

[newtekuser]

Yeah, the running the timer interrupt is a much better approach, keeping the ISR small shouldn't impact the performance too much.

(I left this as excercise ) Configure Timer1 to overflow every 1ms, enable interrupts (GIE and PIE), start timer.
Timing accuracy is not really required for simple button management, if you get 1,024ms or 0,975ms it's just fine.
Then try this code:

Code: [Select]

void __interrupt() isr(void) {
  if(PIR1bits.TMR1IF)
  {
    static unsigned int RB1_count, RB2_count;
    static unsigned char pb_last;   
    unsigned char pb_now = PORTB;   
    unsigned char pb_changed = (pb_last ^ pb_now) & pb_last;
    pb_last = pb_now;
   
    PIR1bits.TMR1IF = 0;
   
    /********************************************************************************************/
    /*            Instant press actions                                                         */
    /********************************************************************************************/
    if (pb_changed & 0b10)                        /* RB1 pressed */
    {
      RB1_count = 1;                              /* Start RB1 counter */
      // do stuff when RB1 button pressed
    }
    else                                          /* Not pressed */
    {
      RB1_count = 0;                              /* Disable RB1 counter */
    }

    if (pb_changed & 0b100)                       /* RB2 pressed */
    {
      RB2_count = 1;                              /* Start RB2 counter */
      // do stuff when RB2 button pressed
    }
    else                                          /* Not pressed */
    {
      RB2_count = 0;                              /* Disable RB2 counter */
    }
   
   
    /********************************************************************************************/
    /*            Long press actions                                                            */   
    /********************************************************************************************/
    if (RB1_count && ++RB1_count>1000)            /* RB1 counter enabled, increase, if higher than 1000ms */
    {
      RB1_count = 0;                              /* Disable counter */
      // do stuff when RB1 button is long pressed
    }
   
    if (RB2_count && ++RB2_count>1000)            /* RB2 counter enabled, increase, if higher than 1000ms */
    {
      RB2_count = 0;                              /* Disable counter */
      // do stuff when RB2 button is long pressed
    }
  }
}


Thanks DavidAlfa! Unfortunately with the modified code, now no button presses are detected. Am I setting the timer wrong?

Code: [Select]

void main(void)
{
    OSCCON =  0x70;     
    ANSEL = 0b0000000;
    ANSELH = 0b0000000;
   
    INTCONbits.INTE = 1;       
    OPTION_REGbits.INTEDG = 0;
    INTCONbits.GIE = 1;         
    INTCONbits.PEIE = 1;       
    INTCONbits.RBIE = 1;
    IOCBbits.IOCB0 = 1;   
    IOCBbits.IOCB1 = 1;   
    IOCBbits.IOCB2 = 1;   
   
    TRISA = 0x0;
    TRISC = 0x0;   
    PORTA = 0x0;
    PORTC = 0x0;
   
    TRISBbits.TRISB0 = 1;
    TRISBbits.TRISB1 = 1;
    TRISBbits.TRISB2 = 1;
    TRISCbits.TRISC0 = 0;

    unsigned int PR1 = (0.001 * 8000000) / (4 * 8);

    PIE1bits.TMR1IE = 1;
    TMR1H = (PR1 >> 8) & 0xFF;
    TMR1L = PR1 & 0xFF;
    T1CONbits.TMR1CS = 0;   
    T1CONbits.T1CKPS = 0b11;
    T1CONbits.TMR1ON = 1;

    while(1)
    {

// displaying menu, etc. code here
    }

}


void __interrupt() isr(void) {
    if (INTCONbits.INTF)
    {       
        // do stuff when RB0 button pressed
        Lcd_Goto(1,1);
        Lcd_Print("Button RB0 pressed");

        INTCONbits.INTF = 0;
    }
   
    void __interrupt() isr(void) {
 
    if(PIR1bits.TMR1IF)
  {
    static unsigned int RB1_count, RB2_count;
    static unsigned char pb_last;   
    unsigned char pb_now = PORTB;   
    unsigned char pb_changed = (pb_last ^ pb_now) & pb_last;
    pb_last = pb_now;
   
    PIR1bits.TMR1IF = 0;
   
    /********************************************************************************************/
    /*            Instant press actions                                                         */
    /********************************************************************************************/
    if (pb_changed & 0b10)                        /* RB1 pressed */
    {
      RB1_count = 1;                              /* Start RB1 counter */
      // do stuff when RB1 button pressed
      Lcd_Goto(1,1);
      Lcd_Print("Button RB1 pressed");
    }
    else                                          /* Not pressed */
    {
      RB1_count = 0;                              /* Disable RB1 counter */
    }

    if (pb_changed & 0b100)                       /* RB2 pressed */
    {
      RB2_count = 1;                              /* Start RB2 counter */
      // do stuff when RB2 button pressed
      Lcd_Goto(1,1);
      Lcd_Print("Button RB2 pressed");
    }
    else                                          /* Not pressed */
    {
      RB2_count = 0;                              /* Disable RB2 counter */
    }
   
   
    /********************************************************************************************/
    /*            Long press actions                                                            */   
    /********************************************************************************************/
    if (RB1_count && ++RB1_count>1000)            /* RB1 counter enabled, increase, if higher than 1000ms */
    {
      RB1_count = 0;                              /* Disable counter */
      RC0 = 1; // lit LED
    }
   
    if (RB2_count && ++RB2_count>1000)            /* RB2 counter enabled, increase, if higher than 1000ms */
    {
      RB2_count = 0;                              /* Disable counter */
      // do stuff when RB2 button is long pressed
      RC0 = 1; // lit LED
    }
  }
}

}

[DavidAlfa]

Its seems you pasted it wrong. Interrupt inside interrupt? 

[DavidAlfa]

Simulating it, it was in an endless interrupt loop because of this:

Code: [Select]

INTCONbits.INTE = 1;       
OPTION_REGbits.INTEDG = 0;
INTCONbits.RBIE = 1;Either INT or RBIE interrupts were firing, but flags not cleared.
You shouldn't enable interrupts you're not managing!

When initializing follow this order:
- Init peripherals, pins
- Enable peripheral interrupts, clear flags
- Enable GIE/ PEIE.

Not at the beginning like you're doing! This can cause unexpected behaviour, interrupts firing while writing to the registers, etc.


Adding decimals is a bad idea! Might get you into trouble, rounded to 0 by the compiler, or adding heavy float libraries into the system.
The formula is also wrong.

Code: [Select]

unsigned int PR1 = (0.001 * 8000000) / (4 * 8);

Timer1 doesn't auto-reload. It just counts up, lacking compare match.
So you must always reload the counter value within the ISR, stopping the timer first as this is not an atomic operation.
Then it'll keep counting up until 0xFFFF, overflowing to 0 and triggering next ISR.
So for calculating the period, the correct formula is:

Code: [Select]

0xFFFF - (8000000/(4*8*1000))

But as it adds the delay from the interrupt until the manual reload, the final timebase becomes larger, about 1.02ms.
We can compensate for this by slighly increasing the default preload, so the timer overflows earlier:

Code: [Select]

#define PR1 ((unsigned int)0xFFFF - (unsigned int)(8000000/(4*8*1000)) + 5)     /* +5 for calibrating reload delay, etc */
This gives between 1.000ms and  0.9995ms (500ns variation = 1 cpu clock). This can be tweaker further by adding few NOP().

This was wrong too:.
You're ANDing with pb_last, while you should do it with pb_now to detect which button was just pressed.

Code: [Select]

unsigned char pb_changed = (pb_last ^ pb_now) & pb_last;
Fix:

Code: [Select]

unsigned char pb_changed = (pb_last ^ pb_now) & pb_now;


Also: Learn to properly make interrupts. You shouldn't call any slow process inside them, and LCD commands are very slow!
Instead, use flags. Set them in the interrupt, process them in the main program.
Try this:

Code: [Select]

#define PR1 ((unsigned int)0xFFFF - (unsigned int)(8000000/(4*8*1000)) + 5)     /* +5 for calibrating reload delay, etc */

volatile unsigned char press_RB1, press_RB2, long_press_RB1, long_press_RB2;

void main(void)
{
    OSCCON =  0x70;     
    ANSEL = 0b0000000;
    ANSELH = 0b0000000;
     
    TRISA = 0x0;
    TRISC = 0x0;   
    PORTA = 0x0;
    PORTC = 0x0;
   
    TRISBbits.TRISB0 = 1;
    TRISBbits.TRISB1 = 1;
    TRISBbits.TRISB2 = 1;
    TRISCbits.TRISC0 = 0;

    TMR1H = PR1 >> 8;
    TMR1L = PR1 & 0xFF;
    T1CONbits.TMR1CS = 0;   
    T1CONbits.T1CKPS = 0b11;
    T1CONbits.TMR1ON = 1;   
    PIE1bits.TMR1IE = 1;
   
    INTCONbits.GIE = 1;         
    INTCONbits.PEIE = 1;

    while(1)
    {
        if(press_RB1)
        {
            press_RB1 = 0;
            Lcd_Goto(1,1);
            Lcd_Print("Button RB1 pressed");
        }
        if(press_RB2)
        {
            press_RB2 = 0;
            Lcd_Goto(1,1);
            Lcd_Print("Button RB2 pressed");           
        }
        if(long_press_RB1)
        {
            long_press_RB1 = 0;
            Lcd_Goto(1,1);
            Lcd_Print("Button RB1 L_pressed");           
        }
        if(long_press_RB2)
        {
            long_press_RB2 = 0;
            Lcd_Goto(1,1);
            Lcd_Print("Button RB2 L_pressed");           
        }
        // displaying menu, etc. code here
    }

}


 void __interrupt() isr(void) {
  if(PIR1bits.TMR1IF)
  {
    static unsigned int RB1_count, RB2_count;
    static unsigned char pb_last;   
    unsigned char pb_now = PORTB;
    unsigned char pb_changed = (pb_last ^ pb_now) & pb_now;
    pb_last = pb_now;
   
    T1CONbits.TMR1ON = 0;
    TMR1H = PR1 >> 8;                             /* Preload timer */
    TMR1L = PR1 & 0xFF;
    NOP();                                        /* For best calibration of 1ms */
    T1CONbits.TMR1ON = 1;
   
    PIR1bits.TMR1IF = 0;                          /* Reset flag */
   
    /********************************************************************************************/
    /*            Instant press actions                                                         */
    /********************************************************************************************/
    if (pb_changed & 0b10)                        /* RB1 pressed */
    {
      RB1_count = 1;                              /* Start RB1 counter */
      press_RB1 = 1;
    }
    else if(!(pb_last & 0b10))                    /* Not pressed */
    {
      RB1_count = 0;                              /* Disable RB1 counter */
    }

    if (pb_changed & 0b100)                       /* RB2 pressed */
    {
      RB2_count = 1;                              /* Start RB2 counter */
      press_RB2 = 1;
    }
    else if(!(pb_last & 0b100))                   /* Not pressed */
    {
      RB2_count = 0;                              /* Disable RB2 counter */
    }
   
   
    /********************************************************************************************/
    /*            Long press actions                                                            */   
    /********************************************************************************************/
    if (RB1_count && ++RB1_count>1000)            /* RB1 counter enabled, increase, if higher than 1000ms */
    {
      RB1_count = 0;                              /* Disable counter */
      long_press_RB1 = 1;
    }
   
    if (RB2_count && ++RB2_count>1000)            /* RB2 counter enabled, increase, if higher than 1000ms */
    {
      RB2_count = 0;                              /* Disable counter */
      long_press_RB2 = 1;
    }
  }
}


[newtekuser]

Simulating it, it was in an endless interrupt loop because of this:

Code: [Select]

INTCONbits.INTE = 1;       
OPTION_REGbits.INTEDG = 0;
INTCONbits.RBIE = 1;Either INT or RBIE interrupts were firing, but flags not cleared.
You shouldn't enable interrupts you're not managing!

When initializing follow this order:
- Init peripherals, pins
- Enable peripheral interrupts, clear flags
- Enable GIE/ PEIE.

Not at the beginning like you're doing! This can cause unexpected behaviour, interrupts firing while writing to the registers, etc.


Adding decimals is a bad idea! Might get you into trouble, rounded to 0 by the compiler, or adding heavy float libraries into the system.
The formula is also wrong.

Code: [Select]

unsigned int PR1 = (0.001 * 8000000) / (4 * 8);

Timer1 doesn't auto-reload. It just counts up, lacking compare match.
So you must always reload the counter value within the ISR, stopping the timer first as this is not an atomic operation.
Then it'll keep counting up until 0xFFFF, overflowing to 0 and triggering next ISR.
So for calculating the period, the correct formula is:

Code: [Select]

0xFFFF - (8000000/(4*8*1000))

But as it adds the delay from the interrupt until the manual reload, the final timebase becomes larger, about 1.02ms.
We can compensate for this by slighly increasing the default preload, so the timer overflows earlier:

Code: [Select]

#define PR1 ((unsigned int)0xFFFF - (unsigned int)(8000000/(4*8*1000)) + 5)     /* +5 for calibrating reload delay, etc */
This gives between 1.000ms and  0.9995ms (500ns variation = 1 cpu clock). This can be tweaker further by adding few NOP().

This was wrong too:.
You're ANDing with pb_last, while you should do it with pb_now to detect which button was just pressed.

Code: [Select]

unsigned char pb_changed = (pb_last ^ pb_now) & pb_last;
Fix:

Code: [Select]

unsigned char pb_changed = (pb_last ^ pb_now) & pb_now;


Also: Learn to properly make interrupts. You shouldn't call any slow process inside them, and LCD commands are very slow!
Instead, use flags. Set them in the interrupt, process them in the main program.
Try this:

Code: [Select]

#define PR1 ((unsigned int)0xFFFF - (unsigned int)(8000000/(4*8*1000)) + 5)     /* +5 for calibrating reload delay, etc */

volatile unsigned char press_RB1, press_RB2, long_press_RB1, long_press_RB2;

void main(void)
{
    OSCCON =  0x70;     
    ANSEL = 0b0000000;
    ANSELH = 0b0000000;
     
    TRISA = 0x0;
    TRISC = 0x0;   
    PORTA = 0x0;
    PORTC = 0x0;
   
    TRISBbits.TRISB0 = 1;
    TRISBbits.TRISB1 = 1;
    TRISBbits.TRISB2 = 1;
    TRISCbits.TRISC0 = 0;

    TMR1H = PR1 >> 8;
    TMR1L = PR1 & 0xFF;
    T1CONbits.TMR1CS = 0;   
    T1CONbits.T1CKPS = 0b11;
    T1CONbits.TMR1ON = 1;   
    PIE1bits.TMR1IE = 1;
   
    INTCONbits.GIE = 1;         
    INTCONbits.PEIE = 1;

    while(1)
    {
        if(press_RB1)
        {
            press_RB1 = 0;
            Lcd_Goto(1,1);
            Lcd_Print("Button RB1 pressed");
        }
        if(press_RB2)
        {
            press_RB2 = 0;
            Lcd_Goto(1,1);
            Lcd_Print("Button RB2 pressed");           
        }
        if(long_press_RB1)
        {
            long_press_RB1 = 0;
            Lcd_Goto(1,1);
            Lcd_Print("Button RB1 L_pressed");           
        }
        if(long_press_RB2)
        {
            long_press_RB2 = 0;
            Lcd_Goto(1,1);
            Lcd_Print("Button RB2 L_pressed");           
        }
        // displaying menu, etc. code here
    }

}


 void __interrupt() isr(void) {
  if(PIR1bits.TMR1IF)
  {
    static unsigned int RB1_count, RB2_count;
    static unsigned char pb_last;   
    unsigned char pb_now = PORTB;
    unsigned char pb_changed = (pb_last ^ pb_now) & pb_now;
    pb_last = pb_now;
   
    T1CONbits.TMR1ON = 0;
    TMR1H = PR1 >> 8;                             /* Preload timer */
    TMR1L = PR1 & 0xFF;
    NOP();                                        /* For best calibration of 1ms */
    T1CONbits.TMR1ON = 1;
   
    PIR1bits.TMR1IF = 0;                          /* Reset flag */
   
    /********************************************************************************************/
    /*            Instant press actions                                                         */
    /********************************************************************************************/
    if (pb_changed & 0b10)                        /* RB1 pressed */
    {
      RB1_count = 1;                              /* Start RB1 counter */
      press_RB1 = 1;
    }
    else if(!(pb_last & 0b10))                    /* Not pressed */
    {
      RB1_count = 0;                              /* Disable RB1 counter */
    }

    if (pb_changed & 0b100)                       /* RB2 pressed */
    {
      RB2_count = 1;                              /* Start RB2 counter */
      press_RB2 = 1;
    }
    else if(!(pb_last & 0b100))                   /* Not pressed */
    {
      RB2_count = 0;                              /* Disable RB2 counter */
    }
   
   
    /********************************************************************************************/
    /*            Long press actions                                                            */   
    /********************************************************************************************/
    if (RB1_count && ++RB1_count>1000)            /* RB1 counter enabled, increase, if higher than 1000ms */
    {
      RB1_count = 0;                              /* Disable counter */
      long_press_RB1 = 1;
    }
   
    if (RB2_count && ++RB2_count>1000)            /* RB2 counter enabled, increase, if higher than 1000ms */
    {
      RB2_count = 0;                              /* Disable counter */
      long_press_RB2 = 1;
    }
  }
}


Unfortunately there's no change, button presses are still not detected with this code.
I also have a button0 to RB0 which I am handling with INTE option, which I see no longer see being enabled with this code. What do I need to change to also handle this?
Also, what happens with the interrupts on change? I see that IOC is no longer enabled either, or is that now handled with the timer interrupts?
(sorry for the stupid questions... that may explain why you were wondering about why I nested the interrupts )
Thank you for the suggestions especially w.r.t handling flags only in the ISR.

[DavidAlfa]

How are you wiring the buttons?
Yeah I removed everything so we center on the specific function.

However I found a weird bug. I simplified the code to the maximum.

Code: [Select]

void __interrupt() isr(void) {
  if(PIR1bits.TMR1IF)
  {
    static unsigned char pb_last;
    unsigned char pb_now = PORTB & 0b110;                                            /* Filter values, taking only RB1 & RB2 */
    unsigned char pb_changed = (pb_last ^ pb_now) & pb_now;
   
    if(pb_last != pb_now)
        NOP();
   
    if(pb_changed)
        NOP();
   
    pb_last = pb_now;
   
    T1CONbits.TMR1ON = 0;
    TMR1H = PR1 >> 8;                             /* Preload timer */
    TMR1L = PR1 & 0xFF;   
    PIR1bits.TMR1IF = 0;                          /* Reset flag */
    T1CONbits.TMR1ON = 1;   
  }
}

Whenever RB1 or RB2 is kept high, pb_now or pb_last magically changes every 2 seconds, triggering pb_changed.


This also happens when removing the interrupts, usign polling:

Code: [Select]

void pb_handle(void){
    static unsigned char pb_last;
    unsigned char pb_now = PORTB & 0b110;
   
    if(pb_last != pb_now)
        NOP();
   
    pb_last = pb_now;   
}

void main(void)
{
    OSCCON =  0x70;     
    ANSEL = 0b0000000;
    ANSELH = 0b0000000;
     
    TRISA = 0x0;
    TRISC = 0x0;   
    PORTA = 0x0;
    PORTC = 0x0;
   
    TRISBbits.TRISB0 = 1;
    TRISBbits.TRISB1 = 1;
    TRISBbits.TRISB2 = 1;
    TRISCbits.TRISC0 = 0;

    while(1)
    {
        pb_handle();
    }
}

I thought it could be a Proteus bug. But it also happens in MplabX sim! I don't have any physical 16F887 to test this for real.

[DavidAlfa]

So how are you developing this? Are you doind it blindly? Programming, then see what happens?
Don't you have a pickit to debug it? That's the way to go! Stepping the code, setting brepoints, finding what your code does, what doesn't.

[NorthGuy]

Whenever RB1 or RB2 is kept high, pb_now or pb_last magically changes every 2 seconds, triggering pb_changed.

You may have a watchdog timer on, which resets the PIC periodically.

[newtekuser]

So how are you developing this? Are you doind it blindly? Programming, then see what happens?
Don't you have a pickit to debug it? That's the way to go! Stepping the code, setting brepoints, finding what your code does, what doesn't.

Pretty much. I do have a pickit4, but I gave up on using the mplab debugger as I can't make it do the basic things I'm expecting from it. i.e.: stepping over functions throws a "Stepover() failed. No breakpoint is available for the Step Over function" error and in other cases I get "no hardware breakpoints available". Not sure if these are a limitation of the PIC I'm using or simply not understanding the flow of debugging on MCUs. I am not a noob at using debuggers though as I've been using them for almost 20 years, but only recently with HW,

[newtekuser]

Whenever RB1 or RB2 is kept high, pb_now or pb_last magically changes every 2 seconds, triggering pb_changed.

You may have a watchdog timer on, which resets the PIC periodically.

The watchdog is turned off. Here's my config:

Code: [Select]

#pragma config FOSC = INTRC_NOCLKOUT// Oscillator Selection bits (INTOSCIO oscillator: I/O function on RA6/OSC2/CLKOUT pin, I/O function on RA7/OSC1/CLKIN)
#pragma config WDTE = OFF       // Watchdog Timer Enable bit (WDT disabled and can be enabled by SWDTEN bit of the WDTCON register)
#pragma config PWRTE = OFF      // Power-up Timer Enable bit (PWRT disabled)
#pragma config MCLRE = OFF      // RE3/MCLR pin function select bit (RE3/MCLR pin function is digital input, MCLR internally tied to VDD)
#pragma config CP = OFF         // Code Protection bit (Program memory code protection is disabled)
#pragma config CPD = OFF        // Data Code Protection bit (Data memory code protection is disabled)
#pragma config BOREN = OFF      // Brown Out Reset Selection bits (BOR disabled)
#pragma config IESO = ON        // Internal External Switchover bit (Internal/External Switchover mode is enabled)
#pragma config FCMEN = ON       // Fail-Safe Clock Monitor Enabled bit (Fail-Safe Clock Monitor is enabled)
#pragma config LVP = OFF        // Low Voltage Programming Enable bit (RB3 pin has digital I/O, HV on MCLR must be used for programming)

// CONFIG2
#pragma config BOR4V = BOR40V   // Brown-out Reset Selection bit (Brown-out Reset set to 4.0V)
#pragma config WRT = OFF        // Flash Program Memory Self Write Enable bits (Write protection off)

[DavidAlfa]

Indeed it was the WDT! I simply hadn't set anything, assuming it was off by default.
This works fine!

Code: [Select]

#include <xc.h>

#pragma config FOSC = INTRC_NOCLKOUT// Oscillator Selection bits (INTOSCIO oscillator: I/O function on RA6/OSC2/CLKOUT pin, I/O function on RA7/OSC1/CLKIN)
#pragma config WDTE = OFF       // Watchdog Timer Enable bit (WDT disabled and can be enabled by SWDTEN bit of the WDTCON register)
#pragma config PWRTE = OFF      // Power-up Timer Enable bit (PWRT disabled)
#pragma config MCLRE = OFF      // RE3/MCLR pin function select bit (RE3/MCLR pin function is digital input, MCLR internally tied to VDD)
#pragma config CP = OFF         // Code Protection bit (Program memory code protection is disabled)
#pragma config CPD = OFF        // Data Code Protection bit (Data memory code protection is disabled)
#pragma config BOREN = OFF      // Brown Out Reset Selection bits (BOR disabled)
#pragma config IESO = ON        // Internal External Switchover bit (Internal/External Switchover mode is enabled)
#pragma config FCMEN = ON       // Fail-Safe Clock Monitor Enabled bit (Fail-Safe Clock Monitor is enabled)
#pragma config LVP = OFF        // Low Voltage Programming Enable bit (RB3 pin has digital I/O, HV on MCLR must be used for programming)

// CONFIG2
#pragma config BOR4V = BOR40V   // Brown-out Reset Selection bit (Brown-out Reset set to 4.0V)
#pragma config WRT = OFF        // Flash Program Memory Self Write Enable bits (Write protection off)


#define PR1 ((unsigned int)0xFFFF - (unsigned int)(8000000/(4*8*1000)) + 5)     /* +5 for calibrating reload delay, etc */


volatile unsigned char press_RB1, press_RB2, long_press_RB1, long_press_RB2;


void main(void)
{
    OSCCON =  0x70;     
    ANSEL = 0b0000000;
    ANSELH = 0b0000000;
     
    TRISA = 0x0;
    TRISC = 0x0;   
    PORTA = 0x0;
    PORTC = 0x0;
   
    TRISBbits.TRISB0 = 1;
    TRISBbits.TRISB1 = 1;
    TRISBbits.TRISB2 = 1;
    TRISCbits.TRISC0 = 0;

    TMR1H = (PR1 >> 8) & 0xFF;
    TMR1L = PR1 & 0xFF;
    T1CONbits.TMR1CS = 0;   
    T1CONbits.T1CKPS = 0b11;
    T1CONbits.TMR1ON = 1;   
    PIE1bits.TMR1IE = 1;
   
    INTCONbits.GIE = 1;         
    INTCONbits.PEIE = 1;

    while(1)
    {
        if(press_RB1)
        {
            press_RB1 = 0;
            Lcd_Goto(1,1);
            Lcd_Print("Button RB1 pressed");
        }
        if(press_RB2)
        {
            press_RB2 = 0;
            Lcd_Goto(1,1);
            Lcd_Print("Button RB2 pressed");           
        }
        if(long_press_RB1)
        {
            long_press_RB1 = 0;
            Lcd_Goto(1,1);
            Lcd_Print("Button RB1 L_pressed");           
        }
        if(long_press_RB2)
        {
            long_press_RB2 = 0;
            Lcd_Goto(1,1);
            Lcd_Print("Button RB2 L_pressed");           
        }
    }
}


void __interrupt() isr(void)
{
  if(PIR1bits.TMR1IF)
  {
    static unsigned int RB1_count, RB2_count;
    static unsigned char pb_last;
    unsigned char pb_now = PORTB;
    unsigned char pb_changed = (pb_last ^ pb_now) & pb_now;

   
    T1CONbits.TMR1ON = 0;
    TMR1H = PR1 >> 8;                             /* Preload timer */
    TMR1L = PR1 & 0xFF;
    PIR1bits.TMR1IF = 0;
    NOP();                                        /* For 1ms calibration */
    T1CONbits.TMR1ON = 1;                         /* Reset flag */
   
    pb_last = pb_now;
   
    /********************************************************************************************/
    /*            Short press actions                                                         */
    /********************************************************************************************/
    if (pb_changed & 0b10)                        /* RB1 pressed */
    {
      RB1_count = 1;                              /* Start RB1 counter */
    }
    else if(!(pb_last & 0b10))                    /* Not pressed, or released */
    {
      if(RB1_count >50 && RB1_count <500 )
      {
        press_RB1 = 1;                            /* Was pressed for a short time */
      }
      RB1_count = 0;                              /* Disable RB1 counter */
    }

    if (pb_changed & 0b100)                       /* RB2 pressed */
    {
      RB2_count = 1;                              /* Start RB2 counter */   
    }
    else if(!(pb_last & 0b100))                   /* Not pressed, or released */
    {
      if(RB2_count >50 && RB2_count <500 )
      {
        press_RB2 = 1;                            /* Was pressed for a short time */     
      }
      RB2_count = 0;                              /* Disable RB2 counter */
    }   
   
    /********************************************************************************************/
    /*            Long press actions                                                            */   
    /********************************************************************************************/
    if (RB1_count && ++RB1_count>1000)            /* RB1 counter enabled, increase, if higher than 1000ms */
    {
      RB1_count = 0;                              /* Disable RB1 counter */
      long_press_RB1 = 1;
    }   
    if (RB2_count && ++RB2_count>1000)            /* RB2 counter enabled, increase, if higher than 1000ms */
    {
      RB2_count = 0;                              /* Disable RB2 counter */
      long_press_RB2 = 1;
    }
  }
}

This code doesn't instantly react when button is pushed, instead, it starts the counter.
When released, it will trigger a short press it it was held between 50-500ms, providing some debouncing.
You can change this as you want, if you want instant response then the press_RBx flag inside if (pb_changed & 0bxx).
Whenever a button is hold for longer than 1s, it sets the long press flag.

[PartialDischarge]

For keys I always use interrupts activated by edge not level, AND a counter loop inside the interrupt that polls the key state so I can know how long the key was pressed for, thus allowing for 2 functions on one key: long press and short press.

[newtekuser]

Indeed it was the WDT! I simply hadn't set anything, assuming it was off by default.
This works fine!

Code: [Select]

#include <xc.h>

#pragma config FOSC = INTRC_NOCLKOUT// Oscillator Selection bits (INTOSCIO oscillator: I/O function on RA6/OSC2/CLKOUT pin, I/O function on RA7/OSC1/CLKIN)
#pragma config WDTE = OFF       // Watchdog Timer Enable bit (WDT disabled and can be enabled by SWDTEN bit of the WDTCON register)
#pragma config PWRTE = OFF      // Power-up Timer Enable bit (PWRT disabled)
#pragma config MCLRE = OFF      // RE3/MCLR pin function select bit (RE3/MCLR pin function is digital input, MCLR internally tied to VDD)
#pragma config CP = OFF         // Code Protection bit (Program memory code protection is disabled)
#pragma config CPD = OFF        // Data Code Protection bit (Data memory code protection is disabled)
#pragma config BOREN = OFF      // Brown Out Reset Selection bits (BOR disabled)
#pragma config IESO = ON        // Internal External Switchover bit (Internal/External Switchover mode is enabled)
#pragma config FCMEN = ON       // Fail-Safe Clock Monitor Enabled bit (Fail-Safe Clock Monitor is enabled)
#pragma config LVP = OFF        // Low Voltage Programming Enable bit (RB3 pin has digital I/O, HV on MCLR must be used for programming)

// CONFIG2
#pragma config BOR4V = BOR40V   // Brown-out Reset Selection bit (Brown-out Reset set to 4.0V)
#pragma config WRT = OFF        // Flash Program Memory Self Write Enable bits (Write protection off)


#define PR1 ((unsigned int)0xFFFF - (unsigned int)(8000000/(4*8*1000)) + 5)     /* +5 for calibrating reload delay, etc */


volatile unsigned char press_RB1, press_RB2, long_press_RB1, long_press_RB2;


void main(void)
{
    OSCCON =  0x70;     
    ANSEL = 0b0000000;
    ANSELH = 0b0000000;
     
    TRISA = 0x0;
    TRISC = 0x0;   
    PORTA = 0x0;
    PORTC = 0x0;
   
    TRISBbits.TRISB0 = 1;
    TRISBbits.TRISB1 = 1;
    TRISBbits.TRISB2 = 1;
    TRISCbits.TRISC0 = 0;

    TMR1H = (PR1 >> 8) & 0xFF;
    TMR1L = PR1 & 0xFF;
    T1CONbits.TMR1CS = 0;   
    T1CONbits.T1CKPS = 0b11;
    T1CONbits.TMR1ON = 1;   
    PIE1bits.TMR1IE = 1;
   
    INTCONbits.GIE = 1;         
    INTCONbits.PEIE = 1;

    while(1)
    {
        if(press_RB1)
        {
            press_RB1 = 0;
            Lcd_Goto(1,1);
            Lcd_Print("Button RB1 pressed");
        }
        if(press_RB2)
        {
            press_RB2 = 0;
            Lcd_Goto(1,1);
            Lcd_Print("Button RB2 pressed");           
        }
        if(long_press_RB1)
        {
            long_press_RB1 = 0;
            Lcd_Goto(1,1);
            Lcd_Print("Button RB1 L_pressed");           
        }
        if(long_press_RB2)
        {
            long_press_RB2 = 0;
            Lcd_Goto(1,1);
            Lcd_Print("Button RB2 L_pressed");           
        }
    }
}


void __interrupt() isr(void)
{
  if(PIR1bits.TMR1IF)
  {
    static unsigned int RB1_count, RB2_count;
    static unsigned char pb_last;
    unsigned char pb_now = PORTB;
    unsigned char pb_changed = (pb_last ^ pb_now) & pb_now;

   
    T1CONbits.TMR1ON = 0;
    TMR1H = PR1 >> 8;                             /* Preload timer */
    TMR1L = PR1 & 0xFF;
    PIR1bits.TMR1IF = 0;
    NOP();                                        /* For 1ms calibration */
    T1CONbits.TMR1ON = 1;                         /* Reset flag */
   
    pb_last = pb_now;
   
    /********************************************************************************************/
    /*            Short press actions                                                         */
    /********************************************************************************************/
    if (pb_changed & 0b10)                        /* RB1 pressed */
    {
      RB1_count = 1;                              /* Start RB1 counter */
    }
    else if(!(pb_last & 0b10))                    /* Not pressed, or released */
    {
      if(RB1_count >50 && RB1_count <500 )
      {
        press_RB1 = 1;                            /* Was pressed for a short time */
      }
      RB1_count = 0;                              /* Disable RB1 counter */
    }

    if (pb_changed & 0b100)                       /* RB2 pressed */
    {
      RB2_count = 1;                              /* Start RB2 counter */   
    }
    else if(!(pb_last & 0b100))                   /* Not pressed, or released */
    {
      if(RB2_count >50 && RB2_count <500 )
      {
        press_RB2 = 1;                            /* Was pressed for a short time */     
      }
      RB2_count = 0;                              /* Disable RB2 counter */
    }   
   
    /********************************************************************************************/
    /*            Long press actions                                                            */   
    /********************************************************************************************/
    if (RB1_count && ++RB1_count>1000)            /* RB1 counter enabled, increase, if higher than 1000ms */
    {
      RB1_count = 0;                              /* Disable RB1 counter */
      long_press_RB1 = 1;
    }   
    if (RB2_count && ++RB2_count>1000)            /* RB2 counter enabled, increase, if higher than 1000ms */
    {
      RB2_count = 0;                              /* Disable RB2 counter */
      long_press_RB2 = 1;
    }
  }
}

This code doesn't instantly react when button pushed. Instead, it starts the counter.
When a button is released, it will trigger a short press it it was held between 50-500ms, providing some debouncing.
You can change this as you want, if you want instant response then the press_RBx flag inside if (pb_changed & 0bxx).
Whenever a button is hold for longer than 1s, it sets the long press flag.

Indeed, this version is working great, thanks DavidAlfa!

[DavidAlfa]

If you can disclosure it, what are you doing with this? 

[NorthGuy]

Not sure if these are a limitation of the PIC I'm using or simply not understanding the flow of debugging on MCUs. I am not a noob at using debuggers though as I've been using them for almost 20 years, but only recently with HW,

The PIC you're using is also 20 years old. The whole new generation of humans grew up since then. There are much better PIC16s now. Look at the models with part number PIC16F1xxxx. They have 3 hardware breakpoints, lots of useful periphery, better oscillators, and so on.

The problems you're having are more from the general approach. You write the program first and then you start testing it. It is much better to start very small, then add things in very small increments. After adding a small part, you test, you see what happens, and you fix it right away. If something breaks, you know that the bug is in these few lines of code you added last. When you know where the bug is, it's much easier to fix.

[newtekuser]

If you can disclosure it, what are you doing with this? 

I'm working on an alarm clock that can be silenced by simply covering up a light sensor. The clock is driven by a DS3231 but I'm trying to make it analogue using two small stepper motors to actuate the hour and minute indicators... I thought I could learn a lot about micro controllers this way

[newtekuser]

Not sure if these are a limitation of the PIC I'm using or simply not understanding the flow of debugging on MCUs. I am not a noob at using debuggers though as I've been using them for almost 20 years, but only recently with HW,

The PIC you're using is also 20 years old. The whole new generation of humans grew up since then. There are much better PIC16s now. Look at the models with part number PIC16F1xxxx. They have 3 hardware breakpoints, lots of useful periphery, better oscillators, and so on.

The problems you're having are more from the general approach. You write the program first and then you start testing it. It is much better to start very small, then add things in very small increments. After adding a small part, you test, you see what happens, and you fix it right away. If something breaks, you know that the bug is in these few lines of code you added last. When you know where the bug is, it's much easier to fix.

I know my PIC is old. I started building on top of the videos and articles I found online so I've stuck with it, but I do have a PIC18F46K40 as well as a PIC16F18877T which I'm planning to experiment with.

[DavidAlfa]

Pretty much. I do have a pickit4, but I gave up on using the mplab debugger as I can't make it do the basic things I'm expecting from it. i.e.: stepping over functions throws a "Stepover() failed. No breakpoint is available for the Step Over function" error and in other cases I get "no hardware breakpoints available". Not sure if these are a limitation of the PIC I'm using or simply not understanding the flow of debugging on MCUs. I am not a noob at using debuggers though as I've been using them for almost 20 years, but only recently with HW,

Yeah, you can set only a few breakpoints. Can't remember how many, but definitely not more than 3. Over this you'll get the "No breakpoints available" error.
So you must remove the old ones and set the new ones all the time.
Always halt the PIC when doing so, they can't do so on the fly, but MplabX should warn you about this if you try.

The 16F877 should debug fine, however there're some limitations. Not being able to step the code inside an interrupt is a bummer.
https://microchipdeveloper.com/debug-limits:8bit-devices-pic16f887

The 16F183xx series work really well with debuggers! But I gave up on Microchip 8-bitters, they were getting expensive for their value.
Having 32-bit ARM MCUs for less than 30 cents, once you get into them the PIC16/18 will look like toys!
Microchip kept trying 8-bit but industry is moving!

[MikeK]

Regarding debugging...Rest assured that most people don't need a debugger.  Plenty of us older programmers get by very well by just turning on an LED at a strategic point or sending a value out the UART.  I've done a lot of microcontroller development and only a couple of times have I used the built-in debugger to step through code.  Overwhelmingly, I find it easier and faster to do the LED or UART method.

[MikeK]

For keys I always use interrupts activated by edge not level, AND a counter loop inside the interrupt that polls the key state so I can know how long the key was pressed for, thus allowing for 2 functions on one key: long press and short press.

If you're looping inside an ISR, waiting for a long keypress, you've developed bad programming habits.  That is not what an ISR is for.

[PartialDischarge]

[If you're looping inside an ISR, waiting for a long keypress, you've developed bad programming habits.  That is not what an ISR is for.

0.6s max, and for the applications that I do is perfectly fine, the CPU has nothing else to do.
*If* the CPU had something else to do, you could simply exit the ISR and run this code outside via some flag, but in my case it's not necessary

[snarkysparky]

Create some tools first

where you are polling the key create

1   debounce using the history of the input.    Last 4 polled values is what i use

2  rising edge detection acting on the debounced signal

3  falling edge detection acting on the debounced signal

Now after you have debugged these use them like so


rising edge of keypress
{   
    zero and start the timer

}


falling edge of keypress
{

   stop timer
}


timer exceeding preset level
{
     do stuff you want if this happens
}

[DavidAlfa]

0.6s max, and for the applications that I do is perfectly fine, the CPU has nothing else to do.

I know it might work but it's a bad idea anyways .
That's why timer interrupts are usually used. You only need to read the button state, increase some counters, set few flags when required.
Though I've used ISR to run some heavy code (No delays, but plenty of processing) in a sort of higher priority task, while the main did the graphics or other stuff which had no priority at all.
Still, it was under 5ms execution, but I consider that a lot for a ISR! But this was in a STM32, with plenty of IRQ priorities, so if done well you won't stall anything else.
600ms ISR is almost terrorism! The programmer army will find you in the night!

[PartialDischarge]

while the main did the graphics or other stuff which had no priority at all.

Graphics? lol, my CPU does nothing until a key gets pressed

600ms ISR is almost terrorism! The programmer army will find you in the night!

I know, I'm already regretting posting, an avalanche of purists and ANSI bible programmers will start to appear

[nctnico]

[If you're looping inside an ISR, waiting for a long keypress, you've developed bad programming habits.  That is not what an ISR is for.

0.6s max, and for the applications that I do is perfectly fine, the CPU has nothing else to do.
*If* the CPU had something else to do, you could simply exit the ISR and run this code outside via some flag, but in my case it's not necessary

Or use nested interrupts which interrupt the long interrupt. Nothing wrong with that. It is a whole lot cleaner compared to trying to mimic an OS with flags that may, or may not be set and all the associated process synchronisation that would be involved.

[newtekuser]

Pretty much. I do have a pickit4, but I gave up on using the mplab debugger as I can't make it do the basic things I'm expecting from it. i.e.: stepping over functions throws a "Stepover() failed. No breakpoint is available for the Step Over function" error and in other cases I get "no hardware breakpoints available". Not sure if these are a limitation of the PIC I'm using or simply not understanding the flow of debugging on MCUs. I am not a noob at using debuggers though as I've been using them for almost 20 years, but only recently with HW,

Yeah, you can set only a few breakpoints. Can't remember how many, but definitely not more than 3. Over this you'll get the "No breakpoints available" error.
So you must remove the old ones and set the new ones all the time.
Always halt the PIC when doing so, they can't do so on the fly, but MplabX should warn you about this if you try.

The 16F877 should debug fine, however there're some limitations. Not being able to step the code inside an interrupt is a bummer.
https://microchipdeveloper.com/debug-limits:8bit-devices-pic16f887

The 16F183xx series work really well with debuggers! But I gave up on Microchip 8-bitters, they were getting expensive for their value.
Having 32-bit ARM MCUs for less than 30 cents, once you get into them the PIC16/18 will look like toys!
Microchip kept trying 8-bit but industry is moving!

Just curious, what 32-bit MCUs can be had at that price? I didn't look too extensively, but the cheapest I found were north of $1.40.

[DavidAlfa]

PY32F002A - 11 cents
HK32F030M - 24 cents
STM32G030 - 35 cents

[pqass]

My brain hurts looking at that ISR.  Ya gotta keep it simple.

See attached for an updated version of my debounce function from here.
This updated version hasn't been tested, although, it does compile at least.
It's for AVR but I'm sure you can port it fairly easily; the register setup is localized or is straight-forward.

The gist of it is...
a. setup a free-running 5ms timer interrupt.
b. in the timer ISR, call debouncePin(current pin value, struct var *) for every pin that needs debouncing. Increment a tick counter var.
   debouncePin() keeps track of the last 8 pin states. If the last 8 are HIGH, the debounced var.state is set to HIGH. Same for LOW.
   Also, while in the LOW state, var.time is incremented to contain the number of timer interval counts it spent in the LOW state.
c. your main-line code just checks if var.state==HIGH and if var.time==0 or some other value.
   if it's the latter, do whatever you need doing then set var.time=0 to stop if from re-executing until another keypress.

[DavidAlfa]

Sure, do you thing using magic Arduino crap "debouncePin((PINA>>(PA7)), &attn_key);" is simpler? 
It's just you're not seeing what it's doing behind the walls.
keeps track of the last 8 pin states? Why? More bloated functions.

[pqass]

It's not "magic Arduino crap".  I define the function!    Look at the code.
In fact, there is no Arduino library calls at all; except setup() and loop().

[DavidAlfa]

My bad, I didn't see it so I assumed the usual!
Anyways, I kept the programming simple when people struggle with it. Like avoiding structures, pointers...
Also PIC architecture is pretty bad, not efficient at addressing so I avoid pointers there when possible.

[pqass]

Anyways, I kept the programming simple when people struggle with it. Like avoiding structures, pointers...
Also PIC architecture is pretty bad, not efficient at addressing so I avoid pointers there when possible.

Honestly, structures help to keep the data compartmentalized.
If you don't want to do pointers, then you can repackage as an array of structures or even array of arrays.  And just pass indexes around.

Why 8?  Because it represents 40ms of time; long enough for a bounce to settle.
If in that time the pin didn't change states (bounce), then you can say it has settled and change the debounced state flag to that value.
Also, I use the .tmp structure element which happens to be a uint8_t and every timer tick it is shifted left by 1 OR'd with the current pin state.

[tridac]

Same method here, 10mS or so tick timer, isr as a little state machine that validates a keypress if a key has been seen for two or more ticks, 20-30Ms is usually enough for contact debounce. Isr just a reads a port, x & y, 4 bits each to handle 16 possible keys. Then use a simple lookup table to translate that into a key value. Table has all the valid single key values only, with anything else ignored, to reject multi key presses. Just a few lines of code...

[DavidAlfa]

As you see adding more buttons will need more code, eventually getting larger and making development messier.

To make it dynamic, at first it needs more complex code, but then it'll be a lot better.
Using pointers is the usual way when the architecture allows it, but PIC16 are not meant for this, neither their ecosystem/header definitions.
The terrible memory bank switching messes everything up, so manually making a pointer to 0x05 could be PORTA, TRISA, WDTCON or  SRCON depending on the selected bank.
Ports are declared as RAM volatile chars, so adding pointer to them will cause a "operand not constant" error by the compiler.
The only solution I found was to hardcode the PORT addresses, manually set the bank and use indirect addressing through FSR / INDF.
I recycled the debouncing code from here, but barely resembles it now!

Now this code is way better! Plus you can use getTick() to read the system time for other timing you need.
Definitely 10ms is better suited here, a 32bit timer will overflow after 49 days at 1KHz, 497 days at 100Hz.

Keep in mind I made this code for buttons to be active-low!
If you're using them active-high, you need to invert the definitions in button_physical_state_t.
When a short/long press was detected, the code won't update the button anymore until the software reads it using readButtonLogicState, this will reset the button state and resume the sampling.
This will give the software the time it needs to process the events. Of course one button won't affect the rest.

Code: [Select]

#include <xc.h>

#pragma config FOSC = INTRC_NOCLKOUT// Oscillator Selection bits (INTOSCIO oscillator: I/O function on RA6/OSC2/CLKOUT pin, I/O function on RA7/OSC1/CLKIN)
#pragma config WDTE = OFF       // Watchdog Timer Enable bit (WDT disabled and can be enabled by SWDTEN bit of the WDTCON register)
#pragma config PWRTE = OFF      // Power-up Timer Enable bit (PWRT disabled)
#pragma config MCLRE = OFF      // RE3/MCLR pin function select bit (RE3/MCLR pin function is digital input, MCLR internally tied to VDD)
#pragma config CP = OFF         // Code Protection bit (Program memory code protection is disabled)
#pragma config CPD = OFF        // Data Code Protection bit (Data memory code protection is disabled)
#pragma config BOREN = OFF      // Brown Out Reset Selection bits (BOR disabled)
#pragma config IESO = ON        // Internal External Switchover bit (Internal/External Switchover mode is enabled)
#pragma config FCMEN = ON       // Fail-Safe Clock Monitor Enabled bit (Fail-Safe Clock Monitor is enabled)
#pragma config LVP = OFF        // Low Voltage Programming Enable bit (RB3 pin has digital I/O, HV on MCLR must be used for programming)

// CONFIG2
#pragma config BOR4V = BOR40V   // Brown-out Reset Selection bit (Brown-out Reset set to 4.0V)
#pragma config WRT = OFF        // Flash Program Memory Self Write Enable bits (Write protection off)


#define TIMEBASE 10                                                             // Timebase in ms (100Hz timer)
#define PR1 ((const unsigned int)(0xFFFFU - (8000000U/(4UL*8*(1000/TIMEBASE))) + 4))// +4 for compensating interrupt latency and reload delay

typedef enum { button_pressed=0, button_released=1 }button_physical_state_t;    // Physical states. Adjust as required if inverted.
typedef enum { button_idle, button_short, button_long, button_wait_release }button_logic_state_t;    // Logical states
typedef enum {                                                                  // Have to do this because PIC addressing/bank switching and/or compiler sucks
            PORT_A=0x5, PORT_B=0x6, PORT_C=0x7,
            PORT_D=0x8, PORT_E=0x9, PORT_F=0xA
}port_addr_t;

typedef enum { button_RB1=0, button_RB2=1 }button_t;                            // Button index definition, just to make it easier

typedef struct {                                                                // Input filter structure
    union{
        volatile uint8_t d;
        struct{
            unsigned pin         :3;                                            // Pins: 0-7
            unsigned logic_state :2;                                            // idle, short, long, wait_release
            unsigned last        :1;                                            // last reading
            unsigned state       :1;                                            // current stable state
            unsigned stable      :1;                                            // stable flag
        };
    };
    uint8_t  port;                                                              // input port (port_addr_t))
    uint32_t timer;                                                             // Time store for debouncing, events
}inputFilter_t;

volatile inputFilter_t buttons[] = {                                            // Button structure initialization
  { .port = PORT_B, .pin = 1, .state = button_released },                       // RB1
  { .port = PORT_B, .pin = 2, .state = button_released },                       // RB2
};

volatile uint32_t tick;                                                         // System tick

uint32_t GetTick(void);
unsigned readButtonValue(button_t b);
uint8_t readButtonLogicState(button_t b, button_logic_state_t state);

void main(void)
{
    OSCCON =  0x70;     
    ANSEL = 0b0000000;
    ANSELH = 0b0000000;
     
    TRISA = 0x0;
    TRISC = 0x0;   
    PORTA = 0x0;
    PORTC = 0x0;
   
    TRISBbits.TRISB0 = 1;
    TRISBbits.TRISB1 = 1;
    TRISBbits.TRISB2 = 1;
    TRISCbits.TRISC0 = 0;

    TMR1H = (PR1 >> 8) & 0xFF;
    TMR1L = PR1 & 0xFF;
    T1CONbits.TMR1CS = 0;   
    T1CONbits.T1CKPS = 0b11;
    T1CONbits.TMR1ON = 1;   
    PIE1bits.TMR1IE = 1;
   
    INTCONbits.GIE = 1;         
    INTCONbits.PEIE = 1;

    while(1)
    {
        if(readButtonLogicState(button_RB1, button_short) {
            Lcd_Goto(1,1);
            Lcd_Print("Button RB1 pressed");
        }
        if(readButtonLogicState(button_RB1, button_long) {
            Lcd_Goto(1,1);
            Lcd_Print("Button RB1 L_pressed");
        }
        if(readButtonLogicState(button_RB2, button_short) {
            Lcd_Goto(1,1);
            Lcd_Print("Button RB2 pressed");
        }
        if(readButtonLogicState(button_RB2, button_long) {
            Lcd_Goto(1,1);
            Lcd_Print("Button RB2 L_pressed");
        }
    }
}


uint32_t GetTick(void){                                                         // returns system time in tens of ms (1=10ms, 100=1000ms)
    return tick;
}


uint8_t readButtonValue(button_t b) {                                           // Read filtered button state
  return (buttons[b].state == button_pressed);                                  // Return true if pressed
}

uint8_t readButtonLogicState(button_t b, button_logic_state_t state) {          // Read logical button states
    button_logic_state_t s = buttons[b].logic_state;
    if(s==button_wait_release){                                                 // treat button_wait_release as idle
        s=button_idle;
    }
    if(s==state) {                                                              // If state matching request
        if(s==button_short || s==button_long) {                                 // If we have a pending state to be read
            buttons[b].logic_state =  button_wait_release;                      // Set flag to wait for release
        }
        return 1;                                                               // Return true
    }
    return 0;                                                                   // Return false
}

#define BUTTON_COUNT (sizeof(buttons)/sizeof(inputFilter_t))
#define BUTTON_DEBOUNCE (20/TIMEBASE)                                           // Debounce time
#define BUTTON_LONG_MIN (1000/TIMEBASE)                                         // Min long press time

void updateButtons(void) {                                                      // To be called periodically by main or some interrupt every few ms
  uint32_t now = GetTick();
  for(uint8_t b=0; b<BUTTON_COUNT; b++) {                                       // Scan buttons
    STATUSbits.IRP = 0;                                                         // Select lower memory bank (For PORT registers) - PIC addressing is ancient!
    FSR = buttons[b].port;                                                      // Use indirect addressing, load port address into FSR
    uint8_t current = (INDF >> buttons[b].pin) & 1;                             // Read value, convert to boolean
   
    if(buttons[b].last != current){                                             // If button changed
      buttons[b].stable = 0;                                                    // Changed, not stable
      buttons[b].last = current;                                                // Update last state
      buttons[b].timer = now;                                                   // Restart timer
    }
    else if(!buttons[b].stable){                                                // If not stable
      if((now-buttons[b].timer)>BUTTON_DEBOUNCE ) {                             // Check if button was stable for the specified time
        if(buttons[b].state != current) {                                       // New state
            if(     current==button_released &&                                 // If button released,
                    buttons[b].logic_state == button_idle &&                    // logic state was idle
                    buttons[b].state == button_pressed) {                       // and previous button state was pressed
                buttons[b].logic_state = button_short;                          // Short press
            }           
            buttons[b].state = current;                                         // Update state
        }
        buttons[b].stable = 1;                                                  // Set stable condition
      }
    }
    else {                                                                      // Button stable
        if(current==button_pressed) {                                           // If pressed
            if(buttons[b].logic_state == button_idle) {                         // And logic state idle
                if(now - buttons[b].timer > BUTTON_LONG_MIN) {                  // If enough time passed
                    buttons[b].logic_state = button_long;                       // Long press
                }
            }
        }
        else if(buttons[b].logic_state == button_wait_release) {                // If released and logic button state was waiting
            buttons[b].logic_state = button_idle;                               // Set idle
        }
    }
  }
}

void __interrupt() isr(void)
{
  if(PIR1bits.TMR1IF) {
    T1CONbits.TMR1ON = 0;
    TMR1H = PR1 >> 8;                                                           // Preload timer
    TMR1L = PR1 & 0xFF;
    T1CONbits.TMR1ON = 1;
    PIR1bits.TMR1IF = 0;                                                        // Reset flag
   
    tick++;   
    updateButtons();
  }
}