Program crashing during many simultaneous button presses

Setup
I am using a SparkFun ProMicro clone along with two WHDTS MCP23017-E/SS I2C Interface 16 Channel IO Expansion Modules to get 16 input/output channels for button presses.

Intended Function
Once a button is pressed in this circuit a keystroke is transmitted over USB and an LED on the button is lit for 1 second. One button press should correspond to one keystroke.

Problem
During single button presses, or even many button presses at nearly the same time my program works as expected (around 2-3 presses a second). However, if many button presses are going on at the same time (think spamming buttons during a video game speed) the program crashes and must be reset by pulling the USB power/data connection.

What I have tried
I have tried to implement delays in the code to account for single debounce, along with some hardware debouncing circuits to help (see the capacitor/resistor pair across the switch) since I had originally suspected this was the cause of my issue. While these fixes did help get a single button press working, and seemingly helped the number of button presses it could handle the problem still exists.

I can crash the program by spamming two different buttons (e.g. around 4-5 presses a second on each), it does not have to be more than that.

Another strange phenomena that is occurring is that occasionally when hitting the buttons fast an led on the wrong channel will momentarily light up, maybe something goofy going on in the register of the port expander?

Any ideas what could be causing this strange behavior?

Resources I've used
Admittedly the MCP23017 is weird how it handles interrupts. Here is the link to MCP23017 resource I used to make most of the interrupts work:

Switches used:

Link to specific MCP23017 board:

#include <Wire.h>
#include <Keyboard.h>
#include <Adafruit_MCP23017.h>
#include <Centipede.h>

int counter[] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};
long ledTime[] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};
unsigned long previousMillis[] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};
const long intervl = 1000;
int ledState[] = {LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW};
int pinNumber;
String array[] = {
  "Button A Pressed",
  "Button B Pressed",
  "Button C Pressed",
  "Button D Pressed",
  "Button E Pressed",
  "Button F Pressed",
  "Button G Pressed",
  "Button H Pressed",
  "Button I Pressed",
  "Button J Pressed",
  "Button K Pressed",
  "Button L Pressed",
  "Button M Pressed",
  "Button N Pressed",
  "Button O Pressed",
  "Button P Pressed"
};
String array2 = "ABCDEFGHIJKLMNOP";


// MCP23017 setup
// Register bits
#define MCP_INT_MIRROR true  // Mirror inta to intb.
#define MCP_INT_ODR    false // Open drain.

// Arduino pins
#define INTPIN 7   // Interrupt on this Arduino Uno pin.

#define controlArduioInt attachInterrupt(digitalPinToInterrupt(INTPIN),isr,FALLING)

Adafruit_MCP23017 mcp1;
Adafruit_MCP23017 mcp2;

/////////////////////////////////////////////
void setup(){
  Serial.begin(9600);
  Keyboard.begin();
  mcp1.begin();      // use default address 0
  mcp2.begin(1);      // use  address 1

  pinMode(INTPIN,INPUT);

  for (int i = 0; i <= 15; i++) {
    mcp2.pinMode(i, OUTPUT);
    mcp2.digitalWrite(i,ledState[i]);
    mcp1.pinMode(i, INPUT);
    mcp1.pullUp(i, HIGH);

    mcp1.setupInterruptPin(i,CHANGE);
    
  }
   mcp1.setupInterrupts(MCP_INT_MIRROR, MCP_INT_ODR, LOW); // The mcp output interrupt pin.

  // On interrupt, polariy is set HIGH/LOW (last parameter).
  
  //mcp.setupInterruptPin(MCP_INPUTPIN,CHANGE); // The mcp  action that causes an interrupt.

  mcp1.readGPIOAB(); // Initialize for interrupts.

  controlArduioInt; // Enable Arduino interrupt control.

}

/////////////////////////////////////////////
// The interrupt routine handles LED1
// This is the button press since this is the only active interrupt.
void isr(){
uint8_t p,v;

//static uint16_t ledState=0;

   noInterrupts();

   // Debounce. Slow I2C: extra debounce between interrupts anyway.
   // Can not use delay() in interrupt code.
   delayMicroseconds(2000);

   // Stop interrupts from external pin.
   detachInterrupt(digitalPinToInterrupt(INTPIN));
   interrupts(); // re-start interrupts for mcp

   p = mcp1.getLastInterruptPin();
   // This one resets the interrupt state as it reads from reg INTCAPA(B).
   v = mcp1.getLastInterruptPinValue();
   pinNumber = p;
   
   // Here either the button has been pushed or released.
   if ( p==p && v == 1) { //  Test for release - pin pulled high
    //if (v == 1) { //  Test for release - pin pulled high
      ledState[p] = HIGH;
      ledTime[p] = millis();
      counter[p]++;
   }
   mcp2.digitalWrite(p, ledState[p]);
   controlArduioInt;  // Reinstate interrupts from external pin.
}


//////////////////////////////////////////////PPPPPPPPPMIIIJJOO
void loop(){

  delay(250);
 // Serial.println(pinNumber);
  //Serial.println( counter[0]);
  
//
//  mcp.digitalWrite(MCP_LEDTOG1, HIGH);
//  mcp.digitalWrite(MCP_LEDTOG2, LOW);
//
//  delay(100);
//
//  mcp.digitalWrite(MCP_LEDTOG1, LOW);
//  mcp.digitalWrite(MCP_LEDTOG2, HIGH);
//Serial.println(counter);
for (int i = 0; i <= 15; i++) {
  if ((millis()- ledTime[i]) > intervl){
      ledState[i] = LOW;
      mcp2.digitalWrite(i, ledState[i]);
      }

  if (counter[i] >= 1){
  //Serial.println(array[i]);
  //Serial.println(array2.charAt(i));
  Keyboard.write(array2.charAt(i));
  counter[i]--;
  }
  else{
  counter[i] = 0;}
}
}

//

Circuit Diagram attached:

   noInterrupts();

Redundant inside an ISR, when an ISR is running, interrupts are automatically disabled. Detaching an interrupt, inside an ISR, just tells me you are flailing around without consulting any documentation. You are more lost than you even realize.

You have delays in your ISR. That is a no-no. You can debounce switches in an ISR using millis().

These problems wouldn't necessarily directly cause the problems that you have. But your completely non-standard framework would make it difficult to fix those, since you would be pussy-footing around and having to analyze all kinds of things that are taken for granted.

Go to Nick Gammon's tutorials on interrupts, you can Google it. Honestly you will have to completely re-write this piece of work.

“volatile” qualification for pinNumber isn’t redundant though.

for (int i = 0; i <= 15; I hate this.
Isn’t “< 16” more natural?

How do you debounce using millis() in an ISR when micros() and millis() depend on interrupts for their time ticks, and interrupts are disabled?

CrossRoads:
How do you debounce using millis() in an ISR when micros() and millis() depend on interrupts for their time ticks, and interrupts are disabled?

Because you only need the time stamp from the previous ISR call, to compare with the current millis() value, which does provide a valid time of entry to the ISR. So you can save it, and use it next time. Then you can implement a very simple "ignore" test at the beginning of the ISR. This code has been posted in a thread somewhere in the last 2 weeks or so, IIRC.

It is both necessary, and beneficial, that in this case the time delay does NOT occur during the execution of the ISR.

TheMemberFormerlyKnownAsAWOL:
“volatile” qualification for pinNumber isn’t redundant though.

for (int i = 0; i <= 15; I hate this.
Isn’t “< 16” more natural?

…especially when it’s a hard coded size constant, like

const int ARRAYSIZE = 16;
...
for (int i = 0; i < ARRAYSIZE...

aarg:

   noInterrupts();

Redundant inside an ISR, when an ISR is running, interrupts are automatically disabled. Detaching an interrupt, inside an ISR, just tells me you are flailing around without consulting any documentation. You are more lost than you even realize.

You have delays in your ISR. That is a no-no. You can debounce switches in an ISR using millis().

These problems wouldn't necessarily directly cause the problems that you have. But your completely non-standard framework would make it difficult to fix those, since you would be pussy-footing around and having to analyze all kinds of things that are taken for granted.

Go to Nick Gammon's tutorials on interrupts, you can Google it. Honestly you will have to completely re-write this piece of work.

I thought that was strange too, but it was in the example for the MCP23017 and thought it was needed to function.(example 3 specifically in this link: https://www.best-microcontroller-projects.com/mcp23017.html).
As for the tutorial, I assume you mean this? https://gammon.com.au/interrupts (posting for anyone who finds this thread in the future).

FALLING is the wrong interrupt type to use for an MCP23017. If a button is pressed while you are inside of the ISR already (when handling the previous button press), this won't trigger an interrupt on the Arduino, and the MCP keeps the interrupt pin low, since you've never handled the pin change. You need to attach an interrupt of type LOW, not FALLING.

I have no idea what happens when you re-enable interrupts in the ISR in order to be able to use I²C, though, I don't think it'll work like that ... IMHO, you shouldn't be reading or writing to the MCP in an ISR at all, move that code to the main loop.

Pieter

PieterP:
FALLING is the wrong interrupt type to use for an MCP23017. If a button is pressed while you are inside of the ISR already (when handling the previous button press), this won't trigger an interrupt on the Arduino, and the MCP keeps the interrupt pin low, since you've never handled the pin change. You need to attach an interrupt of type LOW, not FALLING.

I have no idea what happens when you re-enable interrupts in the ISR in order to be able to use I²C, though, I don't think it'll work like that ... IMHO, you shouldn't be reading or writing to the MCP in an ISR at all, move that code to the main loop.

Pieter

Whoa, changing FALLING to LOW worked like a charm! Do you have a better explanation why, can't quite wrap my head around your description.
As for reading/writing in the ISR, I assume the correct way would be to increment a counter or variable in the ISR then use it to write in the loop?
Thank you!

As for reading/writing in the ISR, I assume the correct way would be to increment a counter or variable in the ISR then use it to write in the loop?

Exactly. But why are you even using interrupts for a key scan?

jc4291:
Whoa, changing FALLING to LOW worked like a charm! Do you have a better explanation why, can't quite wrap my head around your description.

  1. Button is pressed
  2. MCP registers button pin change and brings its INT output low
  3. Arduino registers INT pin change and enters an ISR
  4. Interrupts are disabled, you disable the external interrupt, and re-enable interrupts
  5. You read the MCP input register to see which pin changed
  6. In reaction to the input register read, the MCP brings its INT output high again
  7. You do stuff with the pins you read, while still in the ISR
  8. While the Arduino is still in the ISR, another button pin change happens (because of contact bounce, for example)
  9. MCP registers button pin change and brings its INT output low
  10. This change does not trigger an interrupt on the Arduino because you disabled it in (4)
  11. You finish the “stuff” from (7), re-enable the external interrupt and exit the ISR
    As a result, the INT pin is now low because you've never handled the changes of steps (8-10). Since the INT pin is low already, it can no longer trigger a “falling” interrupt. The only way to get the INT pin high again is by reading the MCP input register, which only happens inside of your ISR, and this only happens when the INT pin falls.

PieterP:

  1. Button is pressed
  2. MCP registers button pin change and brings its INT output low
  3. Arduino registers INT pin change and enters an ISR
  4. Interrupts are disabled, you disable the external interrupt, and re-enable interrupts
  5. You read the MCP input register to see which pin changed
  6. In reaction to the input register read, the MCP brings its INT output high again
  7. You do stuff with the pins you read, while still in the ISR
  8. While the Arduino is still in the ISR, another button pin change happens (because of contact bounce, for example)
  9. MCP registers button pin change and brings its INT output low
  10. This change does not trigger an interrupt on the Arduino because you disabled it in (4)
  11. You finish the “stuff” from (7), re-enable the external interrupt and exit the ISR
    As a result, the INT pin is now low because you've never handled the changes of steps (8-10). Since the INT pin is low already, it can no longer trigger a “falling” interrupt. The only way to get the INT pin high again is by reading the MCP input register, which only happens inside of your ISR, and this only happens when the INT pin falls.

Got it, thanks so much for the explanation!

aarg:
Exactly. But why are you even using interrupts for a key scan?

A really good question. Ignorance is probably the answer.
Originally my code was scanning through the buttons and looking for a keypress, however this would cause a lot of dropped presses:
Bad code (don’t use because it takes to long to read each pin and drops presses):

//
#include "Adafruit_MCP23017.h"
#include <Keyboard.h>
//Note only works on promicro or leonardo

Adafruit_MCP23017 mcp1;    // Instantiate mcp object
Adafruit_MCP23017 mcp2;    // Instantiate mcp object

//Bounce bounce = Bounce();
int dly = 500;            // 0.5 second delay
int ledState[] = {LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW};
int buttonState[] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};
long ledTime[] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};
int lastButtonState[] = {LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW,LOW};
unsigned long previousMillis[] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};
long lastDebounceTime[] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};  // the last time the output pin was toggled
long debounceDelay = 20;    // the debounce time; increase if the output flickers


const long interval = 1000;

void setup() {
  Serial.begin(9600);
  Serial.println("Ready");
  Keyboard.begin();
  
   mcp1.begin();
   mcp2.begin(1);            // "Start" the mcp object
  for (int i = 0; i <= 15; i++) {
    mcp1.pinMode(i, OUTPUT);
    mcp1.digitalWrite(i,ledState[i]);
    mcp1.pinMode(i+8, INPUT);
    mcp1.pullUp(i+8, HIGH);
    
    mcp2.pinMode(i, OUTPUT);
    mcp2.digitalWrite(i,ledState[i]);
    mcp2.pinMode(i+8, INPUT);
    mcp2.pullUp(i+8, HIGH);
  }
}

void loop() {
   keyOne(8,0,'A');
   keyOne(9,1,'B');
   keyOne(10,2,'C');
   keyOne(11,3,'D');
   keyOne(12,4,'E');
   keyOne(13,5,'F');
   keyOne(14,6,'G');
   keyOne(15,7,'H');

   keyTwo(8,0,'I');
   keyTwo(9,1,'J');
   keyTwo(10,2,'K');
   keyTwo(11,3,'L');
   keyTwo(12,4,'M');
   keyTwo(13,5,'N');
   keyTwo(14,6,'O');
   keyTwo(15,7,'P');
   }

void keyOne(int ReadPin, int WritePin, char charOne){
     int reading = mcp1.digitalRead(ReadPin);
   
   if (reading != lastButtonState[WritePin]) {
    // reset the debouncing timer
    lastDebounceTime[WritePin] = millis();
  }

    if ((millis() - lastDebounceTime[WritePin]) > debounceDelay) {
    // whatever the reading is at, it's been there for longer
    // than the debounce delay, so take it as the actual current state:

    // if the button state has changed:
    if (reading != buttonState[WritePin]) {
      buttonState[WritePin] = reading;

      // only toggle the LED if the new button state is HIGH
      if (buttonState[WritePin] == HIGH) {
        //ledState = !ledState;
        Serial.println(charOne);
        Keyboard.write(charOne);
        ledState[WritePin] = HIGH;
        ledTime[WritePin] = millis(); 
      }
    }
      
      if ((millis()- ledTime[WritePin]) > interval){
      ledState[WritePin] = LOW;
      }
            mcp1.digitalWrite(WritePin, ledState[WritePin]);
      
    }
  
  // set the LED:
 

  // save the reading.  Next time through the loop,
  // it'll be the lastButtonState:
  lastButtonState[WritePin] = reading;
   
  }

  void keyTwo(int ReadPin2, int WritePin2, char charOne2){
     int reading2 = mcp2.digitalRead(ReadPin2);
   
   if (reading2 != lastButtonState[WritePin2+8]) {
    // reset the debouncing timer
    lastDebounceTime[WritePin2+8] = millis();
  }

    if ((millis() - lastDebounceTime[WritePin2+8]) > debounceDelay) {
    // whatever the reading is at, it's been there for longer
    // than the debounce delay, so take it as the actual current state:

    // if the button state has changed:
    if (reading2 != buttonState[WritePin2+8]) {
      buttonState[WritePin2+8] = reading2;

      // only toggle the LED if the new button state is HIGH
      if (buttonState[WritePin2+8] == HIGH) {
        //ledState = !ledState;
        Serial.println(charOne2);
        Keyboard.write(charOne2);
        ledState[WritePin2+8] = HIGH;
        ledTime[WritePin2+8] = millis(); 
      }
    }
      
      if ((millis()- ledTime[WritePin2+8]) > interval){
      ledState[WritePin2+8] = LOW;
      }
            mcp2.digitalWrite(WritePin2, ledState[WritePin2+8]);
      
    }
  
  // set the LED:
 

  // save the reading.  Next time through the loop,
  // it'll be the lastButtonState:
  lastButtonState[WritePin2+8] = reading2;
   
  }

After that failed I search around to try and find a better way of handling it, interrupts popped up as a potential solution since it would always record presses, so that is what I went with.
If there is a better/more concise way to do this rather than use interrupts I am happy to learn about it.

Okay. Your pin polling code is probably slow because of inefficiencies that you built into it. I'm not sure offhand what you're doing wrong, but it probably is just not optimized. Perhaps you are trying to inter-weave too many aspects. It appears that you attempt to handle every individual key press in a giant loop that includes triggering the actions it is to perform. That may mean that the 20ms debounce delay is multiplied by the number of keys, which is a bad design. You only need to debounce ONCE for ALL the keys. You should have one block that does nothing but read all the keys into a buffer. That should not take more than a few dozen microseconds. Does the mcp library not have a method to return all the input states at one time? Yes.

/**
 * Reads all 16 pins (port A and B) into a single 16 bits variable.
 * @return Returns the 16 bit variable representing all 16 pins
 */
uint16_t Adafruit_MCP23017::readGPIOAB() {

All the rest is standard debouncing, even less time for that. Does the mcp library not include debouncing capability? It's worth a look.

You can combine the debounce and actuation logic if you like, that is harmless.

Also this is wrong, you need unsigned type for a time stamp:

long lastDebounceTime[] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};  // the last time the output pin was toggled

Also, you are not alone in your need. Debouncing logic is such a basic, common requirement that a little bit of research will turn up reams of information about how to do it. So you don't have to depend on forum replies for that.