C for AVR Application - ISR Repetition

Question

I'm trying to get a simple interrupt routine to work on an ATMega328P. There's an LED connected to PD6 and a built-in button on PB7. The LED should blink normally until the button is pressed and then go solid for 1.5s before going back to blinking. Here's the code:

#define F_CPU 16000000UL
#include <avr/io.h>
#include <avr/interrupt.h>
#include <util/delay.h>

int main(void)
{
    // Enable pull-ups and set pin directions
    MCUCR |= (1<<PUD);
    PORTD &= ~(1<<PORTD6);
    DDRD |= (1<<DDD6);
    PORTB |= (1<<PORTB7);
    DDRB &= ~(1<<DDB7);

    // Enable pin change interrupt
    PCICR = 0x01;
    PCMSK0 = 0x80;
    sei();

    while (1) 
    {
        // Blink LED at standard rate
        _delay_ms(500);
        PORTD ^= (1<<PORTD6);
        _delay_ms(500);
        PORTD ^= (1<<PORTD6);
    }
}

ISR(PCINT0_vect,ISR_BLOCK)
{
    PORTD &= ~(1<<PORTD6);
    _delay_ms(500);
    PORTD |= (1<<PORTD6);
    _delay_ms(1500);
    PORTD &= ~(1<<PORTD6);
}

The interrupt triggers correctly, however the ISR routine loops twice. I imagine it's some sort of button bounce issue but I'm unfamiliar with how to deal with it. I tried introducing the 500ms delay at the beginning and I've also tried clearing the pin change interrupt flag within the ISR so that it doesn't trigger again but it still does. Thanks in advance for any help!

Please don't put a delay in an interrupt handler, which should be as quick as possible, and might prevent other interrupts from being serviced. Better is to record a time count of when the button was pressed. If a button is pressed too soon after a previous, record the time but ignore it. Control the LED from a lower level, with information set by the interrupt handler. — Weather Vane
Simplest would be to have the ISR only set a button_pressed flag and exit. Then put a if (button_pressed) {delay(1000); button_pressed=false;} after you turn the light on in the main loop. In general though, embedded code should avoid delay loops altogether, and replace them with clock checks and/or timer interrupts. — AShelly
I have definitely heard that avoiding delay() is preferable, I'll look for some examples to replace the delay() functions with timer controls. — Eric

Tim Tim · Accepted Answer · 2018-11-03T16:34:49

Let's work on the basis that you are happy to ignore any button presses while the LED is on for 1.5 seconds. You could write your interrupt handler like this:

ISR(PCINT0_vect,ISR_BLOCK)
{
    button_pressed = 1;
}

and put this next the top of your code:

volatile int button_pressed = 0;

(See this page for information on what volatile is all about and why it's needed here.)

Then your main loop can look like this:

while (1) 
{
    // Blink LED on and off

    PORTD |= (1<<PORTD6);   // Turn LED on.
    if (button_pressed) {
        _delay_ms(1500);    // Long delay if button was pressed.
        button_pressed = 0;
    } else {
        _delay_ms(500);     // Regular delay otherwise.
    }

    PORTD &= ~(1<<PORTD6);  // Turn LED off.
    _delay_ms(500);
}

Notes for advanced readers:

volatile int button_pressed = 0; could actually be just volatile int button_pressed;, as static ints at file scope are initialised to 0, but it's a lot clearer to initialise explicitly.
C programs often use for (;;) as the idiom for "loop forever" instead of while (1).

C for AVR Application - ISR Repetition

1 Answers