Rate my ISR. How fast is it?

Hi,
I’ve been trying to write a fast ISR to increment a counter with an encoder. I just wanted to know whether my code is actually an improvement on just using digitalRead() etc. This is the first time I have used direct port manipulation and implemented bit math, so please let me know if I’m doing anything very wrong. It does work, but if there are any improvements that can be made please let me know.

This is the code I wrote with port manipulation:

#include <Arduino.h>

#define encoderA 2
#define encoderB 3

volatile int counter = 0;

volatile int encoderAState;
volatile bool prevEncoderAState;
volatile int encoderBState;

void isr()
{
  cli();

  //pushing pin state to position 0 then clearing all other bits. (HIGH state = dec 1, LOW = dec 0)
  encoderAState = PIND >> 2 & 0b00000001;
  encoderBState = PIND >> 3 & 0b00000001;

  if (encoderAState != prevEncoderAState)
  {
    if (encoderAState == encoderBState)
      counter++;
    else
      counter--;

    Serial.println(counter);
  }
  prevEncoderAState = encoderAState;

  sei();
}

void setup()
{
  pinMode(encoderA, INPUT);
  pinMode(encoderB, INPUT);

  Serial.begin(9600);

  prevEncoderAState = digitalRead(encoderA);

  attachInterrupt(digitalPinToInterrupt(2), isr, CHANGE);
  attachInterrupt(digitalPinToInterrupt(3), isr, CHANGE);
}

void loop()
{
  
}

You do not need to disable interrupts when in an ISR as they are disabled automatically on entry and enabled on exit

There is no need to use an int for this. Use a char or byte instead.

volatile int encoderAState;

I also don't see a need for the bit shift. The bit mask is all you need to isolate the encoder state (the resulting variable is either zero or nonzero).

  encoderAState = PIND&4;
  encoderBState = PIND&8;

Interrupts are automatically disabled before the ISR is entered, and automatically enabled upon exit from an ISR, so no need for cli() and sei().

Serial.print?
In an ISR?

Yep, just noticed that. NEVER attempt do serial I/O in an interrupt!

Isn't it a little weird that a pin change interrupt for encoderB will do nothing unless encoderA value has changed? Maybe something I don't understand about your encoder? If it's a quadrature encoder, the two phases have equal priority...

Only declare a variable as volatile if it is used both inside and outside of the ISR. volatile tells the compiler that the value of the variable can change at any time, and will lead to additional memory accesses to keep the value updated properly.

Since both inputs are on a single port, read the port once, store in a variable, and split out the separate pin values from there.

Thanks for all the helpful replies.
As I said this is the first time I have tried to write an efficient ISR, all of your comments has helped me to understand it a lot better.

jremington:
I also don't see a need for the bit shift. The bit mask is all you need to isolate the encoder state (the resulting variable is either zero or nonzero).

If I did this what would the best way be to evaluate encoderAState and encoderBState. Would you just set the types to booleans or is their a different way?

david_2018:
Since both inputs are on a single port, read the port once, store in a variable, and split out the separate pin values from there.

Does this act to speed it up?
Regarding the serial.print in the isr, that was more for troubleshooting, I will remove that when I implement it into the main code.

jremington:
Interrupts are automatically disabled before the ISR is entered, and automatically enabled upon exit from an ISR, so no need for cli() and sei().

Is this an arduino library thing or does this occur in the CPP standard library as well?

Does this act to speed it up?

You’d have to examine the generated code, or add some code to time it (e.g pin toggle, and scope or logic analyser)

aarg:
Isn't it a little weird that a pin change interrupt for encoderB will do nothing unless encoderA value has changed? Maybe something I don't understand about your encoder? If it's a quadrature encoder, the two phases have equal priority...

Would you just remove the pin change interrupt for encoderB?

TheMemberFormerlyKnownAsAWOL:
You'd have to examine the generated code, or add some code to time it (e.g pin toggle, and scope or logic analyser)

Ah ok. I don't have a scope or logic analyzer. Guess I'll just attach an led to the pin toggle and look if it stays on for less time! haha.

You can toggle a single pin in a single instruction.
You could use a second Arduino’s capture hardware to measure pulse times, right down to single cycle resolution.

tomdixo:
Would you just remove the pin change interrupt for encoderB?

Yes, one interrupt per rotational "click" is enough. In the ISR just get the value of B, but only when A is LOW. B can be HIGH or LOW, that gives you the direction of rotation.

NB: For better understanding: don't name them A and B, but Clock and Data.

1 - Never call println in an isr

2 - you have declared the encoder states as volatile. this means that the C compiler cannot optimise them inside your ISR. In this fragment:

  if (encoderAState != prevEncoderAState)
  {
    if (encoderAState == encoderBState)

The compiler must generate code to re-fetch the values of these variables from memory and not use values that might be in registers.

3 - I'm not convinced that your quadrature code is correct. It does nothing on the change of encoder B.

4 - you do a bit shift and then a mask to pull out a bit and make it boolean. A mask and a "equal to zero?" is cheaper.

#define encoderA 2
#define encoderB 3

volatile boolean isr_update = false; 
volatile int isr_counter = 0;

int prevPIND;
boolean prevEncoderAState;

void isr()
{
  int pind = PIND & 0b1100;
  if(pind == prevPIND) 
    return;

  boolean encoderAState = (pind & 0b0100) != 0;
  boolean encoderBState = (pind & 0b1000) != 0;

  if ((prevEncoderAState == prevEncoderAState) == (encoderAState==encoderBState))
    isr_counter++;
  else
    isr_counter--;

  prevPIND = pind;
  prevEncoderAState = encoderAState;
  isr_update = true;
}

void setup()
{
  pinMode(encoderA, INPUT);
  pinMode(encoderB, INPUT);

  Serial.begin(9600);

  prevPIND = PIND & 0b1100;

  attachInterrupt(digitalPinToInterrupt(2), isr, CHANGE);
  attachInterrupt(digitalPinToInterrupt(3), isr, CHANGE);
}

void loop()
{
    noInterrupts();
    boolean update = isr_update;
    int counter = isr_counter;
    isr_update = false;
    interrupts();

    if(update) {
      Serial.println(counter);
    }
}

This topic was automatically closed 120 days after the last reply. New replies are no longer allowed.