PIN CHANGE INTERRUPTS

Go To Last Post
11 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I have this issue and can't solve it, it in pin  change interrupts in AVR?

 

I have setup pin change interrupt on PB1

 

<

DDRB &~(1<<1); //Make PB1 Input

PORTB |=(1<<); //Pullup PB1

PCICR |=(1<<0); // Enable Interrupts on PB0 - PB7

PCMSK0 |=(1<<1); // Enable interrupt PCINT0 - PCINT7

sei();

>

 

then in my ISR

 

This will not work.

ISR(PCINT0_vect)

{

  if(PINB &(1<<1) callback();   //Check to ensure the interrupt from PB1

}

 

//This is working perfectly WHY?

ISR(PCINT0_vect)

{

      callback(); ///Check to ensure the interrupt from PB1

}

This topic has a solution.
Last Edited: Wed. Oct 13, 2021 - 10:14 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Post the exact code please - this won't compile due to missing parenthesis:

ISR(PCINT0_vect)
{
  if(PINB &(1<<1) callback();   //Check to ensure the interrupt from PB1
}

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

What is connected to PB1?

#1 Hardware Problem? https://www.avrfreaks.net/forum/...

#2 Hardware Problem? Read AVR042.

#3 All grounds are not created equal

#4 Have you proved your chip is running at xxMHz?

#5 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand."

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

please see Tip #1 in my signature, below, for how to properly post source code:

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 2

Would have helped if you had said which AVR this was! Also I can't help thinking this:

DDRB &~(1<<1); //Make PB1 Input
PORTB |=(1<<); //Pullup PB1
PCICR |=(1<<0); // Enable Interrupts on PB0 - PB7
PCMSK0 |=(1<<1); // Enable interrupt PCINT0 - PCINT7
sei();

would be more readable as:

DDRB   &= ~(1 << 1);     // Make PB1 Input
PORTB  |= (1 << 1);      // Pullup PB1
PCICR  |= (1 << PCIE0);  // Enable Interrupts on PB0 - PB7
PCMSK0 |= (1 << PCINT1); // Enable interrupt PCINT0 - PCINT7
sei();

The first two lines had obvious typographical errors and the latter two will benefit from use of the symbolic names (PCIE0, PCINT1)

 

(I've assumed a 48/88/168/328 - perhaps I'm wrong?)

 

PS forgot to say: don't become addicted to using |= and &=~ all the time! Only use those to add bit settings into an already configured registered - for the first setting of a register just use plain old =

Last Edited: Wed. Oct 13, 2021 - 08:36 AM
This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

 

Thank you all for your helping hands.

 

I was able to get the code working, see attached below.

 

What I am doing? 

I am using ATMEGA328P, 

I want to have a Pinchange Interrupt. 

I should be able to set the tracking level HIGH or LOW, since the PIN change state trigers interrupt. 

I also want to select single or multiple pins when needed.  

 

Each pin has function callback address stored in array (array of function pointer).

Whenver void ISR_PIN_ATTACH( void (*Fnc)(void), uint8_t bits, uint8_t track_state); is call

You need to pass address of the callback function, the PIN, and the level. 

 

#include <avr/io.h>
#include <string.h>
#include <avr/interrupt.h>
#include <util/delay.h>

 

typedef void (*FNCptr)(void);
FNCptr PCIINT[24]={nullptr};
uint8_t TRACK_LOGIC[24]={1};

ISR(PCINT0_vect)
{
    if((PINB &(1<<0))==TRACK_LOGIC[0]) PCIINT[0]();    //PB0
    else if((PINB &(1<<1))&&TRACK_LOGIC[1]) PCIINT[1]();    //PB1
    else if((PINB &(1<<2))&&TRACK_LOGIC[2]) PCIINT[2]();    //PB2
    else if((PINB &(1<<3))&&TRACK_LOGIC[3]) PCIINT[3]();    //PB3
    else if((PINB &(1<<4))&&TRACK_LOGIC[4]) PCIINT[4]();    //PB4
    else if((PINB &(1<<5))&&TRACK_LOGIC[5]) PCIINT[5]();    //PB5
    else if((PINB &(1<<6))&&TRACK_LOGIC[6]) PCIINT[6]();    //PB6 XTAL
    else if((PINB &(1<<7))&&TRACK_LOGIC[7]) PCIINT[7]();    //PB7 XTAL
}

void ISR_PIN_ATTACH( void (*Fnc)(void), uint8_t bits, uint8_t track_state)
{
    PCIINT[bits]=Fnc;
    TRACK_LOGIC[bits]=track_state;
    PINB &=~(1<< bits);
    DDRB |= (1<< bits);
    PCICR |= (1<< PCIE0);
    PCMSK0 |= (1<< bits);
}

void BlinkLed()
{
    PORTB ^=(1<<5);
    _delay_ms(50);
}

int main(void)
{
    DDRB |=(1<<5);    //PB5 OUTPUT
    DDRB &=~(1<<1); //PB1 Input
    PORTB |=(1<<1);    //Pull UP PB1

    TRACK_LOGIC[1]=false;
    ISR_PIN_ATTACH(&BlinkLed, 1, 1);
    sei();

    while(1)
    {

    }
    return 0;
}
 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You are very correct (ATMEGA328P)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1
FNCptr PCIINT[24]={nullptr};

then:

if((PINB &(1<<0))==TRACK_LOGIC[0]) PCIINT[0]();

You don't seem to be checking to see if a function pointer has been assigned - if PCINT[0] (BTW don't use all upper case for C symbols!) has not been assigned you will call nullptr and effectively reset the AVR.

 

I thought your TRACK_LOGIC (again don't use all upper case - it looks like a pre-pro macro!) was a protection again this but you seem to have that "upside down" - you assign 1's to the entries and set "false" when one is used but your test in the ISR() is for 1/true not 0/false ??

 

Also don't do this:

ISR(PCINT0_vect)
{
    if((PINB &(1<<0))==TRACK_LOGIC[0]) PCIINT[0]();    //PB0
    else if((PINB &(1<<1))&&TRACK_LOGIC[1]) PCIINT[1]();    //PB1
    else if((PINB &(1<<2))&&TRACK_LOGIC[2]) PCIINT[2]();    //PB2
    else if((PINB &(1<<3))&&TRACK_LOGIC[3]) PCIINT[3]();    //PB3
    else if((PINB &(1<<4))&&TRACK_LOGIC[4]) PCIINT[4]();    //PB4
    else if((PINB &(1<<5))&&TRACK_LOGIC[5]) PCIINT[5]();    //PB5
    else if((PINB &(1<<6))&&TRACK_LOGIC[6]) PCIINT[6]();    //PB6 XTAL
    else if((PINB &(1<<7))&&TRACK_LOGIC[7]) PCIINT[7]();    //PB7 XTAL
}

where you read PINB multiple times - by the last read the stimulating input might have changed state - do something more like:

ISR(PCINT0_vect)
{
    uint8_t readB = PINB;
    if((readB &(1<<0))==TRACK_LOGIC[0]) PCIINT[0]();    //PB0
    else if((readB &(1<<1))&&TRACK_LOGIC[1]) PCIINT[1]();    //PB1
    else if((readB &(1<<2))&&TRACK_LOGIC[2]) PCIINT[2]();    //PB2
    else if((readB &(1<<3))&&TRACK_LOGIC[3]) PCIINT[3]();    //PB3
    else if((readB &(1<<4))&&TRACK_LOGIC[4]) PCIINT[4]();    //PB4
    else if((readB &(1<<5))&&TRACK_LOGIC[5]) PCIINT[5]();    //PB5
    else if((readB &(1<<6))&&TRACK_LOGIC[6]) PCIINT[6]();    //PB6 XTAL
    else if((readB &(1<<7))&&TRACK_LOGIC[7]) PCIINT[7]();    //PB7 XTAL
}

then you just read it once at the start.

 

Also do you really want this as if/else? That means only one call back can be made - what if more than one is activated at the same time - even though only one triggered you in to the ISR() I would have thought you might want to handle any that are set ?

 

Oh and try to get into the habit of putting spaces on either side of a C operator - it makes code more readable..

ISR(PCINT0_vect)
{
    uint8_t readB = PINB;
    if ((readB & (1 << 0)) == TRACK_LOGIC[0]) PCIINT[0]();    //PB0
    else if ((readB & (1 << 1)) && TRACK_LOGIC[1]) PCIINT[1]();    //PB1
    else if ((readB & (1 << 2)) && TRACK_LOGIC[2]) PCIINT[2]();    //PB2
    else if ((readB & (1 << 3)) && TRACK_LOGIC[3]) PCIINT[3]();    //PB3
    else if ((readB & (1 << 4)) && TRACK_LOGIC[4]) PCIINT[4]();    //PB4
    else if ((readB & (1 << 5)) && TRACK_LOGIC[5]) PCIINT[5]();    //PB5
    else if ((readB & (1 << 6)) && TRACK_LOGIC[6]) PCIINT[6]();    //PB6 XTAL
    else if ((readB & (1 << 7)) && TRACK_LOGIC[7]) PCIINT[7]();    //PB7 XTAL
}

Oh and in main() why are you (confusingly) doing:

    DDRB &=~(1<<1); //PB1 Input
    PORTB |=(1<<1);    //Pull UP PB1

when the call to:

 ISR_PIN_ATTACH(&BlinkLed, 1, 1);

is going to do:

    PINB &=~(1<< bits);
    DDRB |= (1<< bits);

anyway ?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

I have taken note of all recommendations. 
 

The double statements

DDRB &=~(1<<bits);

PORTB |=(1<<bits);

 

was because i failed initially for get the needed interrupt work, manipulating to know 

where the trouble lie. 

 

I really agree with you on this

ISR(PCINT0_vect)
{
    uint8_t readB = PINB;
    if ((readB & (1 << 0)) == TRACK_LOGIC[0]) PCIINT[0]();    //PB0
    else if ((readB & (1 << 1)) && TRACK_LOGIC[1]) PCIINT[1]();    //PB1
    else if ((readB & (1 << 2)) && TRACK_LOGIC[2]) PCIINT[2]();    //PB2
    else if ((readB & (1 << 3)) && TRACK_LOGIC[3]) PCIINT[3]();    //PB3
    else if ((readB & (1 << 4)) && TRACK_LOGIC[4]) PCIINT[4]();    //PB4
    else if ((readB & (1 << 5)) && TRACK_LOGIC[5]) PCIINT[5]();    //PB5
    else if ((readB & (1 << 6)) && TRACK_LOGIC[6]) PCIINT[6]();    //PB6 XTAL
    else if ((readB & (1 << 7)) && TRACK_LOGIC[7]) PCIINT[7]();    //PB7 XTAL
}

 

Thanks for your time

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

What is connected to your port to generate the interrupts?

#1 Hardware Problem? https://www.avrfreaks.net/forum/...

#2 Hardware Problem? Read AVR042.

#3 All grounds are not created equal

#4 Have you proved your chip is running at xxMHz?

#5 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand."

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I connected CC1101 (GDO0). 

I need to know when payload arrived. the device signify arrival of payload by pulling GDO0 HIGH.