I have two buttons on my breadboard connected to PC4 and PC5 and I have them both hooked up to an interrupt. When PC5 fires, it seems to only run the correct part, although when PC4 fires, it seems to be running both parts. Can someone please let me know what I'm doing wrong.

ISR(PCINT1_vect)
{
    if (PINC & (1 << PC5))
    {
        // Do PC5 specific stuff
    }

    if ((PINC & (1 << PC4)) == 0)
    {
        // Do PC4 specific stuff
    }
}

void setup() 
{
    // setup Timer0:
    // CTC (Clear Timer on Compare Match mode)
    // TOP set by OCR0A register
    TCCR0A |= (1 << WGM01);

    // clocked from CLK/1024
    // which is 14745600/1024, or 14400 increments per second
    TCCR0B |= (1 << CS02) | (1 << CS00);

    // set TOP to 143
    // because it counts 0, 1, 2, ... 142, 143, 0, 1, 2 ...
    // so 0 through 143 equals 144 events
    OCR0A = 143;

    // enable interrupt on compare event
    // (14400 / 144 = 100 per second)
    TIMSK0 |= (1 << OCIE0A);

    //Set the button pins as input
    DDRC &= ~(1 << PC5); 
    DDRC &= ~(1 << PC4);

    // turn on the internal resistors for the pins
    PORTC |= (1 << PC5);
    PORTC |= (1 << PC4);

    //Enable PIN Change Interrupt 1 - This enables interrupts on pins
    //PCINT14...8 see p70 of datasheet
    PCICR |= (1 << PCIE1);

    PCMSK1 |= ((1 << PCINT13)|(1 << PCINT12));
}

A couple of issues (at least to my understanding).

1. If one of your pins are triggered, it must still be in that same state when the program enters the interrupt otherwise it will be unknown which pin triggered it.
2. Why did you use two different statements to check pin status in the interrupt?
3. If you want a sure fire way to trigger two different pins, use different interrupt vectors. If you have to keep them on the same port, you are on the right path, you should use the syntax of your first (PINC & (1<<PC5)).
   
4. Buttons should be between the pins and ground.

5. Pins should be enabled as inputs with internal pull ups on.

   DDRC &= ~(1<<PC5);
   PORTC |= (1<<PC5);
   

   Rick

HI Metallic_Cloud,

I think the problem might be that you are checking for the pin to be high inside the interrupt handler, and not necessarily checking for a pin transition. What is most likely happening in your code is that when the PC5 interrupt fires (any transition on the pin) PC4 happens to be high, even though it might have not just actively transitioned. Checking for the current state of the pin is a good practice when you only have one pin on the bank changing but if you have two pins that are moving independently you probably want to have some sort of global state of your own for each of the pins (that is managed inside the interrupt). Then when an interrupt is fired you can check the direction of the transition and act on it as required. Hope let me know if something I said doesn't make sense.

Humberto

Hi Guys,

Thanks for your help.

I currently have it as the following

ISR(PCINT1_vect)
{
    if ((PINC & (1 << PC5)) == 0)
    {
        // Do PC5 specific stuff
    }

    if ((PINC & (1 << PC4)) == 0)
    {
        // Do PC4 specific stuff
    }
}

and it seems to be working, although I haven't had much time to properly test it.

Humberto, are you saying I should have something like the following...

int pc5State = 0;

ISR(PCINT1_vect)
{
    if (pc5State != PINC & (1 << PC5))
    {
        pc5State = PINC & (1 << PC5);

        // Do PC5 specific stuff
    }
}

and have the same thing for PC4, or did I not understand correctly?

Cheers

Ideally you wouldn't use PC4 and PC5 because they both use interrupt 1. If you instead use pins that are on different interrupts it becomes easier to discern the cause of the interrupt.

If you want to use the same interrupt for some reason, then yes, it's better to keep track of the state and check if it changes. I'd do it a bit differently, so that you don't read PINC multiple times in the ISR (it could change each time you read it):

#define MYPIN (1<<PC5)

volatile uint8_t pcState;

ISR(PCINT1_vect)
{
    uint8_t newState = PINC;
    uint8_t changedPins = newState ^ pcState;
    pcState = newState;

    if (changedPins & MYPIN)
    {
        // PC5 changed
        if (pcState & MYPIN)
        {
            // it went high
        }
        else
        {
            // it went low
        }
    }
}

int main()
{
    DDRC &= ~MYPIN;
    PORTC |= MYPIN;
    pcState = PINC;
    // etc.
}

If a pin changes state and then changes back again before the ISR gets past the preamble stuff and into your code, the transition won't be seen in "changedPins" so you may want to do things differently depending on whether you need to catch that case or not.

Thanks,

I think I will change my structure to be more like yours. I have almost finished my project, and I will post the results when it's done.

The reason I am checking which pin sent the interrupt, instead of using different interrupts is because I will end up with 4 or 5 buttons, so either way there will needs to be more than one button per interrupt.

Cheers

Hey guys, I'm trying to connect 7 push-buttons to avr, so I have to use same interrupt. You all are saying that if pin changes while ISR is still running, there will be trouble... But code that you wrote will be executed in less then one microsecond, and there is no way human fingers can press and release the button within 1 microsecond.

My project is messing up with the buttons too right now, so you must be right somewhere, but I don't understand why.

A single human-generated button press results in several (potentially dozens) of digital logic transitions. There are two reasons for this. One is that the switch components physically rebound against each other (bounce) and take a while to settle down, and the second is that even in the absence of physical bouncing many switches take some time to transition from a high-impedence state (open circuit) to a low-impendence state (closed circuit) and the switch voltage spends some time in the digital "gray area" which is neither clearly 0V nor 5V and this can cause logic levels to jump around. Both reasons together are called bouncing, and you probably need to do debouncing, either in hardware or software.

can I just put a little delay in the interrupt and read PINx after that?

As a general rule you don't want to do delays inside interrupts because new interrupts can occur during the delay, and complicated stuff can happen depending on whether the MCU holds onto the interrupt for you or whether it throws it away or whether you've re-enable interrupts inside your handler, etc.

I would read this to familiarize yourself with the problem and some of the many different possible solutions.

thank you :D That helped a lot! I guess pushbuttons and pin-change interrupts just don't go too well together. I just happened to have a timer in my app running every millisecond, so I just read the button values in that timer, and xor it with previous state to see what changed. Still get a bounce every 40 presses, but that's not bad at all for my system.

Instead of using just two samples you can easily incorporate 8 samples by doing this (using PC4 for example):

volatile uint8_t samples;

ISR(whatever)
{
    uint8_t pinBit = (PINC >> PC4) & 1;
    samples = (samples << 1) | pinBit;

    if (samples == 0x80)
    {
        // pin was high but has been low for 7 tests
    }
    else if (samples == 0x7F)
    {
        // pin was low but has been high for 7 tests
    }
}