Pin change interrupt debounce/remove interrupt flag

albinjalbinj · July 2, 2020, 6:09pm

Hi,
I'm making a music keyboard and decided to learn about using interrupts. I'm using an Arduino Mega and this is my code right now:

/*

*/
#include <avr/interrupt.h>

byte portB;

volatile byte columns[] = {
    22, 23,
    24, 25,
    26, 27,
    28, 29,
    37, 36,
};

volatile byte previous[] = {
    255, 255,
    255, 255,
    255, 255,
    255, 255,
    255, 255,
};

volatile byte Cpin;

void setup() {
    Serial.begin(9600);
    {
    DDRB = B00000000;
    PORTB = B11111111;
    }
}

void loop() {
    for(volatile byte pin = 0; pin < sizeof(columns); pin++)
    {
      pinMode(columns[pin], OUTPUT);
    {
    cli();
    PCICR |= 0b00000001;
    PCMSK0 |= 0b11111111;
    Cpin = pin;
    sei();
    }
      pinMode(columns[pin], INPUT);
    }
}

ISR(PCINT0_vect) {
    portB = PINB;
    if (PINB != previous[Cpin])
  {
    Serial.print ("portB = ");
    Serial.print (portB, BIN);
    Serial.print (" ");
    Serial.print (previous[Cpin], BIN);
    Serial.print (" ");
    Serial.println (Cpin);
    previous[Cpin] = PINB;
  }
}

When using the monitor, I get a readout that corresponds to the right byte and column, but it's constantly reading and in turn switching between values. I also get 2 reads on each column. The goal is that the code will only read once when the pin change interrupt fires, and then stick to that reading until another comes along or the button is released.

The keyboard I'm using is velocity sensitive so there are 2 switches for every key, but I haven't gotten to the velocity part of the code yet.

portB = 11111110 11111111 1
portB = 11111111 11111110 1
portB = 11111110 11111111 0
portB = 11111111 11111110 0
portB = 11111110 11111111 1
portB = 11111111 11111110 1
portB = 11111110 11111111 0
portB = 11111111 11111110 0

This is a snippet of the monitor when pressing and holding one key.

Why does my code read each column twice and why is it reading continuously and not only once?

I guess I need some debounce which might solve my problem, so I would love some input on good debounce solutions inside ISRs.

Another thought I had was that disabling the interrupt flag could be a solution, so a second ISR isn't called instantly after the first ISR is done. Would this be an option?

Thankful for any help

blh64 · July 2, 2020, 11:45pm

For starters, you can not use Serial.print() inside your ISR. Only do the minimal amount and finish. Typically, you set a flag and react to that flag inside loop().

You also do not to have every variable in you program as 'volatile' - only the ones that are used in the ISR and the main code. I'm not sure why you are repeatedly changing your pins from OUTPUT to INPUT.

You also have a lot of extra brackets throughout your code '{' and '}'

albinjalbinj · July 5, 2020, 8:09pm

Thanks for your answer.

Okay, I'll remove the serial print part. I've used it to try to see if my program does what it should.

I'll try to reduce the ISR to only get the "scan" of the keyboard and then return to the loop to use the scan gathered in the ISR.

The reason for repeatedly changing the pins from OUTPUT to INPUT was that I thought that I could scan the columns that way, but I guess that that's not the way to do it. Please recommend me a good way to scan the columns.

Yeah, I will clean up my code and organize the brackets more, right now it's a mess.

Thanks again

anon57585045 · July 5, 2020, 8:14pm

Low level code like this demands inline documentation (comments).

albinjalbinj · July 12, 2020, 5:54pm

Now I've made some revisions to my code and cleaned it up. I also added comments.

I tried using a flag instead of sending serial.print in the ISR, but I can't get it to work like I want to.

The main problem is that the isr_flag never goes "false" which makes the loop print continuously, and I'm not sure why it doesn't go false.

The current serial print in the loop is only for debugging purposes, the end result I'm looking for is to send 3 serial bytes as quickly as possible when a button is pressed but I haven't made that code yet, going to do it when I get the debug print to work as I want it to.

/*

*/
#include <avr/interrupt.h>

volatile byte portB;

volatile byte columns[] = {
    22, 23,
    24, 25,
    26, 27,
    28, 29,
    37, 36,
};

volatile byte previous[] = {
    255, 255,
    255, 255,
    255, 255,
    255, 255,
    255, 255,
};

volatile byte Cpin;

volatile boolean isr_flag;

void setup() {
    Serial.begin(9600);
    cli();
    DDRB = B00000000;     //sets port B as inputs
    PORTB = B11111111;    //sets port B as HIGH
    PCICR |= 0b00000001;    //turn on interrupts on port B
    PCMSK0 |= 0b11111111;   //turn on all pins in port B
    sei();
}

void loop() {
    for(volatile byte pin = 0; pin < sizeof(columns); pin++)
    {
      pinMode(columns[pin], OUTPUT);
      Cpin = pin;                         //grab the current column pin
      if(isr_flag = true)                 //check if ISR has been triggered
      {
      Serial.print (isr_flag);
      Serial.print (" ");
      Serial.print ("portB = ");
      Serial.print (portB, BIN);          //current port scan of rows in current column
      Serial.print (" ");
      Serial.print (previous[Cpin], BIN); //last port scan of the current column
      Serial.print (" ");
      Serial.println (Cpin);              //current column
      }
      pinMode(columns[pin], INPUT);
      isr_flag = false;
    }
}

ISR(PCINT0_vect) {
    portB = PINB;
    if (PINB != previous[Cpin])           //compare current scan with previous scan
  {
    previous[Cpin] = PINB;                //make the current scan the previous
    isr_flag = true;                      //set isr flag to true to notify the loop that an interrupt has happened
  }
}

cattledog · July 12, 2020, 6:18pm

//if(isr_flag = true)   
if(isr_flag == true)

RayLivingston · July 12, 2020, 6:26pm

if(isr_flag = true)

How can isr_flag go false, when you're constantly setting it to true in loop?

isr_flag = false;

And what happens if the isr is called JUST before this line is executed?

Regards,
Ray L.

albinjalbinj · July 14, 2020, 3:46pm

Thanks, I fixed the == and now it works a bit more like I want it to.

RayLivingston:
isr_flag = false;
And what happens if the isr is called JUST before this line is executed?

I think I solved this with the current revision by disabling interrupts in the printing section of the code.

/*

*/
#include <avr/interrupt.h>

volatile byte portB;

volatile byte columns[] = {
    22, 23,
    24, 25,
    26, 27,
    28, 29,
    37, 36,
};

volatile byte previous[] = {
    255, 255,
    255, 255,
    255, 255,
    255, 255,
    255, 255,
};

volatile byte Cpin;

volatile boolean isr_flag;

void setup() {
    Serial.begin(9600);
    cli();
    DDRB = B00000000;     //sets port B as inputs
    PORTB = B11111111;    //sets port B as HIGH
    PCICR |= 0b00000001;    //turn on interrupts on port B
    PCMSK0 |= 0b11111111;   //turn on all pins in port B
    sei();
}

void loop() {
    for(volatile byte pin = 0; pin < sizeof(columns); pin++)
    {
      pinMode(columns[pin], OUTPUT);
      Cpin = pin;                         //grab the current column pin
      if(isr_flag == true)                 //check if ISR has been triggered
      {
      cli();
      Serial.print (isr_flag);
      Serial.print (" ");
      Serial.print ("portB = ");
      Serial.print (portB, BIN);          //current port scan of rows in current column
      Serial.print (" ");
      Serial.print (previous[Cpin], BIN); //last port scan of the current column
      Serial.print (" ");
      Serial.println (Cpin);              //current column
      isr_flag = false;
      sei();
      }
      pinMode(columns[pin], INPUT);
    }
}

ISR(PCINT0_vect) {
    portB = PINB;
    if (PINB != previous[Cpin])           //compare current scan with previous scan
  {
    previous[Cpin] = PINB;                //make the current scan the previous
    isr_flag = true;                      //set isr flag to true to notify the loop that an interrupt has happened
  }
}

This is the current state of the sketch. What I still don't get is why it loops continuously, like when I push the key connected to columns 0 and 1 (since every key has 2 buttons), the monitor shows a readout like this:

1 portB = 11111110 11111110 0
1 portB = 11111110 11111110 1
1 portB = 11111111 11111111 2
1 portB = 11111110 11111110 0
1 portB = 11111110 11111110 1
1 portB = 11111111 11111111 2
1 portB = 11111110 11111110 0
1 portB = 11111110 11111110 1
1 portB = 11111111 11111111 2

Ideally, I'd like it to only read column 0 and 1 once for every keypress and not continuously.

Thanks for all the great help everyone

blh64 · July 14, 2020, 5:10pm

You have a volatile variable Cpin that you set inside a for() loop inside loop(). You set it to a new value and then check the isr flag which, if set could have easily been done so by the previous value of Cpin. Imagine that the interrupt happens near the end of the for() loop during your pinMode() call. The next iteration of the for() loop will increment Cpin but then you process the isr_flag as if the current iteration caused it.

You also do not need to declare your colum[] array as volatile - they are constants. You are making the compiler do more work. The same is true for your for() variable pin - it does not need to be volatile.

cattledog · July 14, 2020, 6:30pm

if(isr_flag == true)                 //check if ISR has been triggered
      {
      cli();
      Serial.print (isr_flag);
      Serial.print (" ");
      Serial.print ("portB = ");
      Serial.print (portB, BIN);          //current port scan of rows in current column
      Serial.print (" ");
      Serial.print (previous[Cpin], BIN); //last port scan of the current column
      Serial.print (" ");
      Serial.println (Cpin);              //current column
      isr_flag = false;
      sei();
      }

Serial printing requires interrupts, so you should not use cli() sei() to surround the serial output section.

Instead, use cli() sei() to surround a block of code which transfers values from the interrupt to copies of those values. Then use the copies of the values for the output or other processing.

See Nick Gammon's tuturial for details on enabling and disabling interrupts
https://gammon.com.au/interrupts

Topic		Replies	Views
Pin Change Interrupt/Port Interrupt LEDs and Multiplexing	8	1119	May 6, 2021
Pin Change Interrupts - Difference in PORT Read & DigitalRead Programming	12	4748	May 5, 2021
Problem interrupt pin change Programming	28	7972	May 5, 2021
Pin change interrupt occur when enabling it back Sensors	5	1363	May 6, 2021
External interrupt (INT0) and digital Pin2 (INPUT) simultaneous usage Programming	10	2973	May 5, 2021

Pin change interrupt debounce/remove interrupt flag

Related topics