Why is timer interrupt on PIC MCU not behaving as desired?

Question

I am using PIC16F627A with PICKit 3. MPLAB X IDE v5.40 (and associated IPE) with XC8 compiler v2.41.

PIC16F627A datasheet

My intention is to flash TOWER_LED in response to a closure of pushbutton PB_OPEN and to stop flashing the LED in response to a closure of pushbutton PB_CLOSE. Push pushbuttons are attached to port B pins with weak pull-ups enabled. The ambiguous "LED" is lit while the program is paused. I am trying to use TMR2 interrupts to achieve the flashing of TOWER_LED. TOWER_LED_ON_OFF is a global flag to indicate whether the ISR needs to toggle the state of TOWER_LED.

The results I am getting are that the "LED" works as expected and the TOWER_LED lights constantly on and constantly off in reponse to the pushbuttons. But it does not flash.

Simple question: why not?

Code as follows:

// CONFIG
#pragma config FOSC = INTOSCIO  // Oscillator Selection bits (INTOSC 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)
#pragma config PWRTE = OFF       // Power-up Timer Enable bit (PWRT enabled)
#pragma config MCLRE = ON       // RA5/MCLR/VPP Pin Function Select bit (RA5/MCLR/VPP pin function is MCLR)
#pragma config BOREN = OFF      // Brown-out Detect Enable bit (BOD disabled)
#pragma config LVP = OFF        // Low-Voltage Programming Enable bit (RB4/PGM pin has digital I/O function, HV on MCLR must be used for programming)
#pragma config CPD = OFF        // Data EE Memory Code Protection bit (Data memory code protection off)
#pragma config CP = OFF         // Flash Program Memory Code Protection bit (Code protection off)

#include <xc.h>

#define GPIO_OUTPUT 0
#define GPIO_INPUT 1

#define _XTAL_FREQ 4000000
#define TMR2PRESCALE 4

#define TOWER_LED   PORTAbits.RA0 
#define LED         PORTBbits.RB1
#define PB_OPEN     PORTBbits.RB5
#define PB_CLOSE    PORTBbits.RB4

// Timed LED values
int TOWER_LED_counter = 0;
int TOWER_LED_THRESH = 316;       // ~ 1 Hz with postscale @ 1 
int TOWER_LED_ON_OFF = 0;
int COUNTER_MAX_VALUE = 20000;  // some sort of cutoff

void MCU_setup(void) {
    CMCON = 0x07;  // disable the comparator module because PORTA muxed comparator
    OPTION_REGbits.nRBPU = 0;  // enable all PORTB weak pull-ups
    OPTION_REGbits.T0CS = 1;   // disable TMR0 on pin RA4
    TMR1ON = 0;           // disable TMR0
    T0IE = 0;             // disable TMR0 interrupt
    PIE1bits.TMR1IE = 0;  // disable TMR1 interrupt
    PIE1bits.TMR2IE = 1;  // ENABLE TMR2 interrupt
    INTCONbits.GIE = 1;   // Set the Global Interrupt Enable
    
    // GPIO directions
    TRISA0 = GPIO_OUTPUT; // TOWER_LED is output   
    TRISB1 = GPIO_OUTPUT; // TEST_LED is output
    TRISB4 = GPIO_INPUT;  // open push button is input
    TRISB5 = GPIO_INPUT;  // close push button is input 
}

void TMR2_setup() {  
    TOUTPS3 = 0; TOUTPS2 = 0; TOUTPS1 = 0; TOUTPS0 = 0;  // postscaler is 1:1
                                 // Therefore interrupts occur  each PWM_period
    PIR1bits.TMR2IF = 0;         // Clear the Timer 2 interrupt flag
    PIE1bits.TMR2IE = 1;         // Enable Timer 2 interrupts   
    T2CKPS1 = 0 ;T2CKPS0 = 1;    // Timer 2 prescaler is 4    
    TMR2ON = 1;                  // Switch on TMR2
}

void __interrupt() isr(void) {
    if (TMR2IF == 1) {
        if (TOWER_LED_ON_OFF == 1) {
            if (TOWER_LED_counter == TOWER_LED_THRESH) {
                TOWER_LED = ~TOWER_LED;
                TOWER_LED_counter = 0;
            }
        }
        TOWER_LED_counter++;
        if (TOWER_LED_counter >= COUNTER_MAX_VALUE) {
            TOWER_LED_counter = 0;
        }
    }
    PIR1bits.TMR2IF = 0;        //Clear the Timer 2 interrupt flag
}

void main(void) {
    MCU_setup();
    TMR2_setup();
    TOWER_LED = 1;
    LED = 1;
    __delay_ms(1000);
    LED = 0;

    while (1) {
        while (!PB_OPEN) {           
            LED = 1;
            TOWER_LED = 1;
            TOWER_LED_ON_OFF = 1;
            __delay_ms(1000);
            LED = 0;
        }
        while (!PB_CLOSE) {
            LED = 1;
            TOWER_LED = 0;
            TOWER_LED_ON_OFF = 0;
            __delay_ms(1000);
            LED = 0;
        }
    } 
}

Answer 1

Not a complete answer, but too much for a comment.

First of all, as @hobbs mentioned in the comments, you should declare all variables that are manipulated within the ISR as volatile. Otherwise the optimizer might optimize too much.

The next code block could lead to unexpected behavior.

void __interrupt() isr(void) {
    if (TMR2IF == 1) {
        if (TOWER_LED_ON_OFF == 1) {
            if (TOWER_LED_counter == TOWER_LED_THRESH) {
                TOWER_LED = ~TOWER_LED;
                TOWER_LED_counter = 0;
            }
        }
        TOWER_LED_counter++;
        if (TOWER_LED_counter >= COUNTER_MAX_VALUE) {
            TOWER_LED_counter = 0;
        }
    }
    PIR1bits.TMR2IF = 0;        //Clear the Timer 2 interrupt flag
}

Consider TOWER_LED_ON_OFF = 0:

• TOWER_LED_counter keeps on incrementing, regardless of the state of TOWER_LED_ON_OFF.
• If TOWER_LED_ON_OFF increments past TOWER_LED_THRESH, it'll take a WHILE until it overflows back to 0. During that period (TOWER_LED_counter == TOWER_LED_THRESH) will stay false, even if it's well past the threshold value.

For one, you should probably change the condition to (TOWER_LED_counter >= TOWER_LED_THRESH).
You should also consider only to increment TOWER_LED_counter whenever (TOWER_LED_ON_OFF == 1).
And reset TOWER_LED_counter to 0, when TOWER_LED_ON_OFF is 0, so that the counter restarts from zero when TOWER_LED_ON_OFF is enabled.

Answer 2

Assuming your comments are correct, then your ISR wants to toggle the LED at approximately 1Hz. Your main loop, while seeing the button pressed, repeatedly turns on the LED and waits for one second. That is bit problematic itself but is not your main problem. The main problem is in the ISR. When TOWER_LED_ON_OFF is true, the LED will be inverted only if TOWER_LED_counter is 316. TOWER_LED_counter counts between 0 and COUNTER_MAX_VALUE (20000) during normal LED off times, so if the main code sets TOWER_LED_ON_OFF true when TOWER_LED_counter is at 317, the LED will not flash until TIMER_LED_counter counts past 20000.

Answer 3

As well as the points made in the answers, the other problem in my code is that I had not set the PEIE bit in the INTCON register.

Answer 4

If I remember right, XC8 define int as uint8_t. Therefore, 2000 is way to big to enter in an 8 bit. I think, that might be your issue.