[SOLVED] Interrupt is still running the ISR after CHANGE already occured

Hi everyone!

I am using a rotary encoder, 20 positions, 2 bit gray code. I am trying to Serial.print("Hallelujah") when the encoder turns X positions. I.e. when I turn it clockwise 3 times, Serial.prints("Hallelujah").

I am using interrupts(CHANGE) and the main loop. The problem is that the interrupt runs the ISR program more than once after CHANGE is detected. When I wrote the program, I was hoping that CHANGE would only trigger the program to run once because I have chronological outputs for every different time the program runs.

Here's the output from the encoder pins:
pinA 0 0 0 1 1 1 1 1 1 0 0 0 0 0 0
pinB 0 0 0 0 0 0 1 1 1 1 1 1 0 0 0

I copied my code below; here is my logic outline though.

  1. Interrupt detects when pinA CHANGES
  2. Triggers doPinA program
  3. doPinA program changes one value in an array called Records[20] from 0 to 1
  4. doPinA sums up all the values in Records[20] to index the calculated position of the encoder
    Ideal Example: If the doPinA program is triggered 3 times, Records should look like:
    Records[20]: 1 1 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
    Therefore, the sum is 3. The index indicates that 3 leaves the rotary encoder at 54 degrees offset.

I copied the printout as well. It's a bit messy, but you can see that doPinA ran multiple times for A1B0. I thought the interrupt would only trigger doPinA ONCE when A1B0 changed from A0B0.

PinA1PinB018 degreesPinA1PinB036 degreesPinA1PinB054 degreesPinA1PinB072 degreesPinA1PinB090 degreesPinA090 degrees90 degreesPinA1PinB0108 degreesPinA1PinB0126 degreesPinA0126 degrees126 degreesPinA0PinB0144 degrees144 degrees144 degreesPinA0PinB0162 degrees162 degreesPinA1PinB0144 degrees144 degrees144 degreesPinA1PinB0PinA1PinB0PinA1PinB0PinA1PinB0PinA1PinA1PinA1PinA1PinA1PinA1PinA1PinA1PinA1PinB0PinA1PinB0PinA1PinB0PinA0PinA1PinB0PinA1PinB0PinA1PinA1PinB0PinA1PinB0PinA0PinA1PinA1PinB0PinA1PinB0PinA0PinA0PinA1PinB0PinA1PinB0PinA1PinB0PinA1PinB0PinA1PinA1PinA1PinA1PinA1PinA1PinA1PinA1PinA1PinA1PinB0PinA1PinB0PinA0

Main Code:

#define PinA 2
#define PinB 3 
#define FanucDI1 10
int pinStatus;
volatile int term=-1; 
volatile int records[20];
volatile int calDegree;
//INSERT STOPPING DEGREE (18,36,54,72,90,etc)
volatile int StopDeg=72;
void setup() {
  pinMode(PinA, INPUT);
  pinMode(PinB, INPUT);
  pinMode(FanucDI1, OUTPUT);
  for (int i=0; i<20; i++){
    records[i]=0;}
  attachInterrupt(0,doPinA, CHANGE);
  Serial.begin(9600);
}
void loop() {
  noInterrupts();
 
 /*if (calDegree==StopDeg){
  digitalWrite(FanucDI1, HIGH);
  Serial.print("FANUC DI1 WORKS!!!");
 }*/
 interrupts();
 }
void doPinA(){
 if (digitalRead(PinA)==1){
  Serial.print("PinA");
  Serial.print(digitalRead(PinA));
  if (digitalRead(PinB)==0)
  { 
    Serial.print("PinB");
    Serial.print(digitalRead(PinB));
    pinStatus=1;
    if (term<20){
      term++; }
    else {term=-1;}
    records[term]=pinStatus;}}
    calPos();}
void calPos(){
  int x=0;
  for (int i=0; i<20;i++){
    x=x+records[i];
  }
  switch (x){
    case 1:
      calDegree=18;
      Serial.print("18 degrees");
      break;
    case 2:
      calDegree=36;
      Serial.print("36 degrees");
      break;
    case 3:
      calDegree=54;
      Serial.print("54 degrees");
      break;
    case 4:
      calDegree=72;
      Serial.print("72 degrees");
      break;
    case 5:
      calDegree=90;
      Serial.print("90 degrees");
      break;
    case 6:
      calDegree=108;
      Serial.print("108 degrees");
      break;
    case 7:
      calDegree=126;
      Serial.print("126 degrees");
      break;
    case 8:
      calDegree=144;
      Serial.print("144 degrees");
      break;
    case 9:
      calDegree=162;
      Serial.print("162 degrees");
      break;
    case 10:
      calDegree=144;
      Serial.print("144 degrees");
      break;
    default:
      calDegree=0;
      break;
  }
}

Is there any bounce in your encoder? You can get rid of the bounce using a time delay, but I've had more luck soldering two 0.1uF caps between the clock and data lines and ground. Try looking at the Rotary library and its examples.

  Serial.print("PinA");
  Serial.print(digitalRead(PinA));

In an ISR? Dream on.

I gave up on trying to read your code. It is an abomination.

Every { and every } should be on its own line. All code between { and } should be properly indented.

ISRs should be as fast as possible. ALL that it should do is note that the encoder moved. It is up to loop() to deal with that fact.

Speaking of loop():

void loop() {
  noInterrupts();
 interrupts();
 }

That's just unbelievable.

Dear PaulS.

No, the noInterrupts() and interrupts() are pretty believable. What's unbelievable is your unhelpfulness. I put in the Serial.prints to help debug. Anyone else besides PaulS have anything helpful to add?

-Mebble

Serial is interrupt driven, and disabling interrupts there accomplishes absolutely nothing in any event. 1.0.x versions of Arduino would I think actually hang running that sketch (tx buffer fills, so print blocks, but the buffer will never empty since the interrupts that empty it are disabled) - but now it polls the register. It's ending up spending very long periods of time with interrupts disabled waiting to print to serial (9600 baud = 960 bytes per second max - so your loop is spending most of it's time waiting on full serial buffers) - so millis() is going to be way off (you can only have interrupts disabled for 1ms without risking millis missing counts)

I have no clue why you thought disabling interrupts there made any sense, anyway. There's nothing there that makes any sense to block interrupts from. Particularly as the other thing there is a digitalWrite() setting a pin to 1 which never gets set to anything else, so it could go in setup and do the same thing!

Are you aware that interrupts will be disabled for the vast majority of time? That, combined with the fact that every serial.print() is probably blocking for ~1ms per byte (since there's nothing else that might slow it down), could very well be what's causing your problem... The other thing that comes to mind is if the interrupt pin is a button (which we always recommend against), and when the switch is open, the pin is allowed to float (not electrically connected to anything, with pin set as input - that'll produce random readings - there are many threads explaining how to wire up buttons)

In general, ISR's should be as short as possible. You want to do as little as possible inside the ISR, and then set a flag that you check in loop, and handle it from within loop, not the ISR. You certainly shouldn't be writing to serial from within an ISR (though it's not nearly as bad as writing to serial with interrupts disabled - writing to serial from ISR works, but it can break things and is poor practice, so you shouldn't do it)

Mebble:
I put in the Serial.prints to help debug.

Serial.prints will cause the ISR to lock up. So they cause problems, they don't solve them.

I suggest you use the auto-format tool on your code.

I have to agree with PaulS here. Your code needs a total rewrite, sorry about that chief.

You just can't do all that stuff in an ISR. It won't work.

Thank you econJack, DrAzzy, and Nick for explaining the problems to me.

@Nick, your link was incredibly helpful.

Update: The program works now. Counting only ONCE when the rotary encoder spins ONCE. The problem was all the extra steps in the ISR. I cleaned it up and put most of the steps in loop().

Now onto my next project!
Thanks again!

Care to show us the cleaned up version?

Yes! I apologize for the delay.

Yes! I apologize for the delay. For people having trouble with ISRs, just remember that ISR should be kept clean and simple. Doing that and moving everything to void loop() made my code work.

#define PinA 2
#define PinB 3 
#define FanucDI1 10
volatile int pinStatus=0;
volatile int term=-1; 
volatile int records[20];
volatile int calDegree;
//INSERT STOPPING DEGREE (18,36,54,72,90,etc)
volatile int StopDeg=90;
 
void setup() {
  pinMode(PinA, INPUT);
  pinMode(PinB, INPUT);
  pinMode(FanucDI1, OUTPUT);
  for (int i=0; i<20; i++){
    records[i]=0;
    }
  attachInterrupt(0,doPinA, CHANGE);
  Serial.begin(9600);
}

void loop() {
  Serial.print("PinA");
  Serial.print(digitalRead(PinA));
  Serial.print("PinB");
  Serial.print(digitalRead(PinB));
  Serial.print(calDegree);
  Serial.print("\n");

if (pinStatus==1){
  if (term<20){
      term++; 
      records[term]=pinStatus;
      pinStatus=0;
      }
 else {
  term=-1;
  for (int i=0; i<20; i++){
    records[i]=0;
    }
 }
}
 calPos(); 
 if (calDegree==StopDeg){
  digitalWrite(FanucDI1, HIGH);
  Serial.print("FANUC DI1 WORKS!!!");
 }
 else digitalWrite(FanucDI1, LOW);
 }
 
void doPinA(){
  delay(250);
 if (digitalRead(PinA)==1){
  if (digitalRead(PinB)==0){ 
    pinStatus=1;
    }
  } 
}

void calPos(){
  int x=0;
  for (int i=0; i<20;i++){
    x=x+records[i];
  }
  calDegree=(x*18);
}

Clean, simple, and FAST.

void doPinA(){
  delay(250);
 if (digitalRead(PinA)==1){
  if (digitalRead(PinB)==0){ 
    pinStatus=1;
    }
  } 
}

By what definition of fast is delay(250)? You can NOT use delay() in an ISR.