Interrupt Function being called infinitely

I’m trying to take in multiple inputs using a shift register and a Interrupt pin on a photon running firmware version 2.0.1
The circuit uses a interrupt to detect any button press and uses a shift register to parse through the buttons and find the button pressed.
Here is the wiring diagram

The code

#define latch_pin A1
#define clock_pin A3
#define data_pin A5
#define input_pin A0
bool menu1 , menu2, menu3, menu4, menu5,menu6, menu7, menu8;
byte Input, Output, Check=1;
int j, count;
void setup(){ //setup
  count = 0;
  pinMode(clock_pin, OUTPUT);//clock
  pinMode(data_pin, OUTPUT);//data
  pinMode(latch_pin, OUTPUT);//latch
  pinMode(input_pin, INPUT);//Input from buttons
  pinMode(D7,OUTPUT);
  SPI.setBitOrder(MSBFIRST);
  SPI.setClockDivider(SPI_CLOCK_DIV2);
  SPI.begin();
  SPI.transfer(255);
  
  digitalWrite(latch_pin, HIGH);
  digitalWrite(latch_pin, LOW);
  Serial.begin(9600);

  attachInterrupt(A0, pin_read, RISING); //Interrupt for button press
  Serial.println("All Set");
}
void loop(){ //loop
    //Blinking LED to check if program is stuck in an infinite loop
    digitalWrite(D7,HIGH);
    delay(1000);
    digitalWrite(D7,LOW);
    delay(1000);
    
}
void pin_read(){ //Interrupt function called on button press
    //Identifies which button is pressed
    for(j=0; j<50; j++)
    delayMicroseconds(1000);

    Check=1;
    for(j=0; j<8; j++){
        SPI.transfer(Check);
        digitalWrite(latch_pin, HIGH);
        digitalWrite(latch_pin, LOW);
        delayMicroseconds(500);
        if(digitalRead(input_pin)==HIGH){
            takeInput(j);//print the button pressed                   
        }
        Check = Check<<1;
    }
 
    SPI.transfer(255);
    digitalWrite(latch_pin, HIGH);
    digitalWrite(latch_pin, LOW);
    while(digitalRead(input_pin)==HIGH){}

}

void takeInput(int input){ //Outputs which button is pressed
    switch(input){
        case 0:
            menu1 = true;
            Serial.println("Button 1 pressed");
        break;
        case 1:
            menu2 = true;
            Serial.println("Button 2 pressed");
        break;
        case 2:
            menu3 = true;
            Serial.println("Button 3 pressed");
        break;
        case 3:
            menu4 = true;
            Serial.println("Button 4 pressed");
        break;
        case 4:
            menu5 = true;
            Serial.println("Button 5 pressed");
        break;
        case 5:
            menu6 = true;
            Serial.println("Button 6 pressed");
        break;
        case 6:
            menu7 = true;
            Serial.println("Button 7 pressed");
        break;
        case 7:
            menu8 = true;
            Serial.println("Button 8 pressed");
        break;
        
    }
}

The interrupt function is called infinitely and also prevents OTA updates. The infinite calling starts only after the first button press.
I have tried debugging by using INPUT_PULLUP , changing the resistor values. I have also tried analogRead of the input_pin but it does not have any spikes.

Hi,
welcome to the community :wink:
just one thing sparks in my mind as from reference:

A0 share the same interrupt handler with A3 and D2.
and you are using A3 for a SPI clock.
IMHO your interrupt comes from A3 not A0.
May be try to use A2 instead as this pin doesn’t share the same interrupt handler whit any other pins

Yeah, I forgot to mention that too. I have tried it with D6 too which has an independent interrupt handler. But the results are the same. The interrupt function is called infinitely. I have also checked using another arduino to check if there are any stray pulses which trigger the interrupt, but that too doesnt happen
I also observed that the infinite looping only starts after I press a button and it continues until I disconnect the interrupt pin. Upon disconnecting the pin, the looping stops and starts again after pressing the button.

Shared EXTI lines only pose a problem when you try to attach an interrupt to multiple pins on that same line - if only one of the respective GPIOs “subscribes” to an interrupt the other pins won’t influence that.

However, having a 58*1000µs delays plus a blocking while() inside an ISR is a big no-no!

The SPI lib. has nice functionality which I believe can be used in your case, namely bitSet(), bitClear() and shiftOut()

So I’m not sure if this will work also not sure if break statement can be used inside ISR at all and this is just my suggestion how I will try to “bite” this. So don’t be mad :see_no_evil: if my approach doesn’t make any sence but may be it’s worth to try:

#include "SPI.h"

const int latchPin = A1;
const int clockPin = A3;	
const int dataPin = A5;	
const int inputPin = A2;
byte reg = 0;
bool menu1 , menu2, menu3, menu4, menu5,menu6, menu7, menu8;


unsigned long interval = 0;

void setup(){ //setup

  pinMode(clockPin, OUTPUT);//clock
  pinMode(dataPin, OUTPUT);//data
  pinMode(latchPin, OUTPUT);//latch
  pinMode(inputPin, INPUT);//Input from buttons
  pinMode(D7,OUTPUT);
  Serial.begin(9600);

  attachInterrupt(inputPin, pin_read, RISING); //Interrupt for button press
  Serial.println("All Set");
}

void loop()
{ 

if (millis() - interval > 1000) {
     interval = millis();
     digitalWriteFast(D7, !pinReadFast(D7));
}

takeInput(reg);
}

void pin_read(){ //Interrupt function called on button press
    
          
  reg = 0;	
  for (int i = 0; i < 8; i++)	
  {
    bitSet(reg, i);		
    digitalWriteFast(latchPin, LOW);
    shiftOut(dataPin, clockPin, LSBFIRST, reg);
    digitalWriteFast(latchPin, HIGH);
    
    if(pinReadFast(inputPin)==HIGH){
            break;  //not sure if allowed here !!!              
        }
    delayMicroseconds(1000); //not sure if needed ???
    bitClear( reg, i);
    digitalWriteFast(latchPin, LOW);
    shiftOut(dataPin, clockPin, LSBFIRST,  reg);
    digitalWriteFast(latchPin, HIGH);
   
  }

}

void takeInput(byte input){ //Outputs which button is pressed
    switch(input){
        case 1:
            menu1 = true;
            Serial.println("Button 1 pressed");
            clear_reg();
            break;
        case 2:
            menu2 = true;
            Serial.println("Button 2 pressed");
            clear_reg();
        break;
        case 4:
            menu3 = true;
            Serial.println("Button 3 pressed");
            clear_reg();
        break;
        case 8:
            menu4 = true;
            Serial.println("Button 4 pressed");
            clear_reg();
        break;
        case 16:
            menu5 = true;
            Serial.println("Button 5 pressed");
            clear_reg();
        break;
        case 32:
            menu6 = true;
            Serial.println("Button 6 pressed");
            clear_reg();
        break;
        case 64:
            menu7 = true;
            Serial.println("Button 7 pressed");
            clear_reg();
        break;
        case 128:
            menu8 = true;
            Serial.println("Button 8 pressed");
            clear_reg();
        break;
        
    }
}

void clear_reg(){
reg = 0;
for (int i = 0; i < 8; i++)	
    {
        bitClear(reg, i);
        digitalWriteFast(latchPin, LOW);
        shiftOut(dataPin, clockPin, LSBFIRST,  reg);
        digitalWriteFast(latchPin, HIGH);
    }    
}

from my point of view the ISR is as fast as couple ms when ISR detect HIGH on inputPin it’s break and takeInput(reg), called from main loop with the last stage of reg then takeInput() clear the reg

I hope so my way of thinking is correct :stuck_out_tongue_winking_eye:
also I used A2 instead of A0 for inputPin just in case

I have to get rid of ugly break statement as is ugly as s… I was thinking for couple of hours how to fix this and make more useful as eg. finish ISR routine to the end and some how to get a status of couple or even more SW pressed at “the same time” (in the same ISR) and I think i got it:

there is no sence to brake as we losing the info of pressed buttons it is bether to latch all HIGH status copy each of them to an array and then call from main loop foor each array elements.

add a gobal array of 8 elements

#include "SPI.h"

const int latchPin = A1;
const int clockPin = A3;	
const int dataPin = A5;	
const int inputPin = A0;
byte reg = 0;
byte button_pressed[8]; // added to catch pressed SW's
bool menu1 , menu2, menu3, menu4, menu5,menu6, menu7, menu8;

read pin() call:

void pin_read(){ //Interrupt function called on button press    
  reg = 0;	
  for (int i = 0; i < 8; i++)	
  {
    bitSet(reg, i);		
    digitalWriteFast(latchPin, LOW);
    shiftOut(dataPin, clockPin, LSBFIRST, reg);
    digitalWriteFast(latchPin, HIGH);
    
    if(!pinReadFast(inputPin)==HIGH){
        delayMicroseconds(2000); //not sure if needed ???
        bitClear( reg, i);
        digitalWriteFast(latchPin, LOW);
        shiftOut(dataPin, clockPin, LSBFIRST,  reg);
        digitalWriteFast(latchPin, HIGH);
        }
      button_pressed[i] = reg; //latch each pin pressed status
  }

}

main loop:

void loop()
{ 

if (millis() - interval > 1000) {
     interval = millis();
     digitalWriteFast(D7, !pinReadFast(D7));
}

  for (int i =0; i<8; i++)
    {
       takeInput(button_pressed[i]); 
        button_pressed[i] = 0;
    }
}

and also get rid of clear_reg() on each cases of takeInput()

void takeInput(byte input){ //Outputs which button is pressed
    switch(input){
        case 1:
            menu1 = true;
            Serial.println("Button 1 pressed");
            break;
        case 2:
            menu2 = true;
            Serial.println("Button 2 pressed");
        break;
        case 4:
            menu3 = true;
            Serial.println("Button 3 pressed");
        break;
        case 8:
            menu4 = true;
             Serial.println("Button 4 pressed");
        break;
        case 16:
            menu5 = true;
            Serial.println("Button 5 pressed");
        break;
        case 32:
            menu6 = true;
            Serial.println("Button 6 pressed");
        case 64:
            menu7 = true;
            Serial.println("Button 7 pressed");
        break;
        case 128:
            menu8 = true;
            Serial.println("Button 8 pressed");
        break;  
    }
}

Hope that help and I’m really curious if will really work as supposed to :+1:

The datasheet suggests that the 74HC595 can be clocked at least at 5MHz.
Hence I’m not sure where the 1000µs respectively 2000µs in the ISR come from and what they are for.
As said earlier, having that amount of delays (8 times) in an ISR is definitely something that should be avoided.

The interrupt latency on the Photon should be in the ballpark of 10µs not milliseconds.

Instead of clocking the data manually you could try asynchronous SPI.transfer() to keep the ISR as short as possible and the interpretation of the result can be handled by the callback function.
Alternatively you could use the shiftIn() function

In order to know which button(s) were pressed I’d not use multiple menuX variables neither do I see the need for an array.
Once you get used to bit operations all you need is the one byte you get from the shift register and some simple functions to extract the desired info on the fly.

Consider it my lack of knowledge on this issue. I could not find a better way of debouncing the input so that a single button press is only considered. Hence I used the delayMicroseconds function and the while() to ensure that a single press is only considered.
The problem arises after a single button press. The ISR is called infinitely even though there is no input. I confirmed this using another arduino to tap into the input_pin but there were no spikes in the signal.
I do not understand how the ISR is triggered infinitely.

In order to debounce an ISR you’d normally prevent it from executing again rather than trapping the flow inside the ISR.
e.g.

uint32_t usDebounce = 0;

void ISR() {
  if (micros() - usDebounce < 5000) return;
  uint32_t usDebounce = micros();
 
  // ISR workload
}

You may want to backtrack a bit and remove all “business logic” from the ISR and only do a bare minimum to check whether the interrupt fires correctly.

BTW, wouldn’t a PISO shift register (e.g. 74HC165 or 74HC895) or even better a I/O expander (e.g. MCP23008) not be better suited for your use-case?

With the SIPO shift register I’d only use the interrupt to initiate the scanning process to be handled in a separate thread (e.g. in a one-shot software timer) while and further ISR triggers should be prevented (e.g. detaching the interrupt).
Once the scanning is done re-attach/enable the interrupt.

I too would use an MCP23008 I2C 8-port GPIO expander for an application like this, as ScruffR mentioned above.

It connects via I2C (D0/D1) and you can connect up to 8 of them to a single I2C port. Another nice feature is that you can enable built-in pull-ups so you only need to connect your switch to GND; you don’t need 8 more pull resistors, one for each button.

Best of all, you can use the MCP23008-RK library along with the DebounceSwitchRK library to not only decode which switch was pressed and debounce, but also handle long press, multiple-press, etc…

The MCP23008 is also available in at least 3 different packages depending on what you need: PDIP for breadboards, SOIC when you’re getting started making your own SMD circuit boards, and 20-QFN when you’re getting fancy and want to minimize board space used.

3 Likes