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!