Encoder data read with interrupt

[asdfsgada]

Hi,
I have a question about how to read encoder data using interrupt function in arduion.

I am using the encoder from below link.
Pololu - 250:1 Micro Metal Gearmotor HP 6V with 12 CPR Encoder, Back Connector.
So it has outputA and ouputB with 12 CPR.

I am using leonardo arduino board which I know it has 5 interrupt pin.
each pin is 0, 1,2,3,7 and I am using 0, 1,2,3 pin.
Also as I know each pin is match with interrupt pin 2,3,1,0 from Leonardo pinout.

So I setup the below arduino code to read the position data. Currently I am using two motor. So I need two position data. But I have a problem while reading the data.

As I know, 12 CPR means that I can discrimate 12 signals from one rotation. So with the below code 'm1_pos, m2_pos' values should be increase in 12/rotation. But actually it doesn't, it sometimes 180 or sometime 5000. (What a ridiculous number).

So I have a question the cause of my issues. Maybe my code is wrong or interrupt setting is wrong or else. I am directly wiring encoder each output pin of encoder to leonardo pinout.
(I skip some code from below code which is not related with encoder reading)

Thank You for any advice!

int m1_e1 = 0; // 
int m1_e2 = 1; // 

int m2_e1 = 2;  // 
int m2_e2 = 3; // 


void setup() {
  Serial.begin(115200);

  //motor 1 interrupt
  attachInterrupt(2, do_m1e1, CHANGE);
  attachInterrupt(3, do_m1e2, CHANGE);

  //motor 2interrupt
  attachInterrupt(1, do_m2e1, CHANGE);
  attachInterrupt(0, do_m2e2, CHANGE);


  pinMode(m1_en, OUTPUT);
  pinMode(m2_en, OUTPUT);
  pinMode(m1_in1, OUTPUT);
  pinMode(m1_in2, OUTPUT);
  pinMode(m2_in1, OUTPUT);
  pinMode(m2_in2, OUTPUT);
  
  pinMode(m1_e1, INPUT);
  pinMode(m1_e2, INPUT);
  pinMode(m2_e1, INPUT);
  pinMode(m2_e2, INPUT);


}

void do_m1e1(){
  // look for a low-to-high on channel A
  if (digitalRead(m1_e1) == HIGH) { 
    // check channel B to see which way encoder is turning
    if (digitalRead(m1_e2) == LOW) {  
      m1_pos = m1_pos + 1;
    } 
    else {
      m1_pos = m1_pos - 1;
    }
  }
  else   // must be a high-to-low edge on channel A                                       
  { 
    // check channel B to see which way encoder is turning  
    if (digitalRead(m1_e2) == HIGH) {   
      m1_pos = m1_pos + 1;
    } 
    else {
      m1_pos = m1_pos - 1;
    }
  }
  //Serial.println (m1_pos);
  m1_update = true;


}

void do_m1e2(){
  m1_update = true;
  // look for a low-to-high on channel B
  if (digitalRead(m1_e2) == HIGH) {   
   // check channel A to see which way encoder is turning
    if (digitalRead(m1_e1) == HIGH) {  
      m1_pos = m1_pos + 1;
    } 
    else {
      m1_pos = m1_pos - 1;
    }
  }
  // Look for a high-to-low on channel B
  else { 
    // check channel B to see which way encoder is turning  
    if (digitalRead(m1_e1) == LOW) {   
      m1_pos = m1_pos + 1;
    } 
    else {
      m1_pos = m1_pos - 1;
    }
  }
}


void do_m2e1(){
  m2_update = true;
  // look for a low-to-high on channel A
  if (digitalRead(m2_e1) == HIGH) { 
    // check channel B to see which way encoder is turning
    if (digitalRead(m2_e2) == LOW) {  
      m2_pos = m2_pos + 1;
    } 
    else {
      m2_pos = m2_pos - 1;
    }
  }
  else   // must be a high-to-low edge on channel A                                       
  { 
    // check channel B to see which way encoder is turning  
    if (digitalRead(m2_e2) == HIGH) {   
      m2_pos = m2_pos + 1;
    } 
    else {
      m2_pos = m2_pos - 1;
    }
  }

}


void do_m2e2(){
  m2_update = true;
  // look for a low-to-high on channel B
  if (digitalRead(m2_e2) == HIGH) {   
   // check channel A to see which way encoder is turning
    if (digitalRead(m2_e1) == HIGH) {  
      m2_pos = m2_pos + 1;
    } 
    else {
      m2_pos = m2_pos - 1;
    }
  }
  // Look for a high-to-low on channel B
  else { 
    // check channel B to see which way encoder is turning  
    if (digitalRead(m2_e1) == LOW) {   
      m2_pos = m2_pos + 1;
    } 
    else {
      m2_pos = m2_pos - 1;
    }
  }
}

[build_1971]

Please post your full code.
Now we do not know the type if mxPos...
Keep an interrupt short! Best is to set a flag only.
In loop handle the flag.
Interrupts need cli() in loop() if multi byte volatile variables are changed.
Variables that are modified by a ISR should be declared volatile.
On arduino R3 pin 0 and 1 are used for Serial...
Maybe a good idea to use an encoder reader library.

[horace]

I have used QEI decoder using interupt on change to detect changes in the A and B levels

works Ok at low speeds, e.g. for menu selection etc. For higher speeds I would use a microcontroller with QEI decoder hardware

what host microcontroller are you using?

[sterretje]

@horace

what host microcontroller are you using?

If I understand your question correctly, Leonardo as mentioned in the opening post

[horace]

@sterretje

Leonardo as mentioned in the opening post

I missed that! checking the ATmega32u4 does not appear to have a hardware QEI decoder

[PaulRB]

@asdfsgada

But actually it doesn't, it sometimes 180 or sometime 5000. (What a ridiculous number).

Do you have access to a oscilloscope? You could check the signals are similar those shown in the datasheet:

@build_1971

Keep an interrupt short! Best is to set a flag only.

I think they are fine. A couple of digitalRead() and a couple of if-statements will execute pretty quickly. No while-loops, delay() or Serial.print() are used, for example. If only a flag is set, there's a possibility that the main code in loop will miss transitions or miss the sequence in which the flags were set, and not update the counts correctly.

[sterretje]

@horace

I missed that!

Happens far too often to me

[horace]

have a look at Encoder Library

runs basic test OK with a simple rotary QEI encoder on a Leonardo using pins 2 and 3
serial monitor output as switch is rotated

1
2
3
4
5
6
7
8
7
8
7
6
5
4
3
2
1
0
-1
-2
-3
-4
-3
-4
-5
-6
-7
-8

[Deva_Rishi]

@build_1971

Please post your full code.

Do that first. With the variables that are updated in the ISR, not declared 'volatile' a reliable result can not be expected. That combined with them being 16-bit on a 8-bit architecture, chances are the reading of the variables is not done the way it should be.

@asdfsgada

  //motor 1 interrupt
  attachInterrupt(2, do_m1e1, CHANGE);
  attachInterrupt(3, do_m1e2, CHANGE);

  //motor 2interrupt
  attachInterrupt(1, do_m2e1, CHANGE);
  attachInterrupt(0, do_m2e2, CHANGE);

You should use the digitalPinToInterrupt(pin), to get the correct pin / interrupt.

Also if you are just counting revolutions and not to focussed on parts of it, you can reduce the count by simply only listening on 1 of the pins and compare to the other.
You should read both of the pin states the moment you enter the ISR using a direct port read rather than digitalRead() which is a lot slower.

anyway have a look at this thread on the matter. It is a bit of a long story though.

btw if you are using 'CHANGE" as a trigger, you can also use pin change interrupts which can be used on most pins.

[PaulRB]

@Deva_Rishi

You should use the digitalPinToInterrupt(pin), to get the correct pin / interrupt.

+1 for this suggestion

Pin numbers are not the same as interrupt numbers! Your code may not be working in the way you expect because the interrupt code may be called when you are not expecting it.

[asdfsgada]

Thanks for all your answers!
First below is my full code. (I add some description for each code.)
The purpose of this code is controlling two motor while getting encoders signals.
@build_1971 @Deva_Rishi

int m1_en = 9; // m1 pwm
int m2_en = 5; // m2 pwm
int m1_in1 = 12; // m1 dir1
int m1_in2 = 13; // m1 dir2
int m2_in1 = 8; //  m2 dir1
int m2_in2 = 6; //  m2 dir2


volatile int m1_pos = 0; // m1 pos count
volatile int m2_pos = 0; // m2 pos count
int m11_last = LOW;
int m11_now = LOW;

int m12_last = LOW;
int m12_now = LOW;

int m2_last = LOW;
int m2_now = LOW;

int m1_pwmValue = 0; // M1 PWM 
char m1_direction = 'a'; // M1 dir 
int m2_pwmValue = 0; // M2 PWM
char m2_direction = 'a'; // M2 dir

bool m1_active = false; // 
bool m2_active = false; // 
bool dual_active = true;

bool m1_update = false;
bool m2_update = false;

int m1_e1 = 0; // M1 encoderA Interrupt 2
int m1_e2 = 1; // M1 encoderB Interrupt 3

int m2_e1 = 2;  // M2 encoderB  interrupt 1
int m2_e2 = 3; // M2 encoderB  interrupt 0


void setup() {
  Serial.begin(115200);

  //motor 1 interrupt
  attachInterrupt(2, do_m1e1, CHANGE);
  attachInterrupt(3, do_m1e2, CHANGE);

  //motor 2interrupt
  attachInterrupt(1, do_m2e1, CHANGE);
  attachInterrupt(0, do_m2e2, CHANGE);


  pinMode(m1_en, OUTPUT);
  pinMode(m2_en, OUTPUT);
  pinMode(m1_in1, OUTPUT);
  pinMode(m1_in2, OUTPUT);
  pinMode(m2_in1, OUTPUT);
  pinMode(m2_in2, OUTPUT);


  pinMode(m1_e1, INPUT);
  pinMode(m1_e2, INPUT);
  pinMode(m2_e1, INPUT);
  pinMode(m2_e2, INPUT);


}


//Motor1 EncoderA interrupt
void do_m1e1(){
  // look for a low-to-high on channel A
  if (digitalRead(m1_e1) == HIGH) { 
    // check channel B to see which way encoder is turning
    if (digitalRead(m1_e2) == LOW) {  
      m1_pos = m1_pos + 1;
    } 
    else {
      m1_pos = m1_pos - 1;
    }
  }
  else   // must be a high-to-low edge on channel A                                       
  { 
    // check channel B to see which way encoder is turning  
    if (digitalRead(m1_e2) == HIGH) {   
      m1_pos = m1_pos + 1;
    } 
    else {
      m1_pos = m1_pos - 1;
    }
  }
  //Serial.println (m1_pos);
  m1_update = true;


}

//Motor1 encoder B interrupt
void do_m1e2(){
  m1_update = true;
  // look for a low-to-high on channel B
  if (digitalRead(m1_e2) == HIGH) {   
   // check channel A to see which way encoder is turning
    if (digitalRead(m1_e1) == HIGH) {  
      m1_pos = m1_pos + 1;
    } 
    else {
      m1_pos = m1_pos - 1;
    }
  }
  // Look for a high-to-low on channel B
  else { 
    // check channel B to see which way encoder is turning  
    if (digitalRead(m1_e1) == LOW) {   
      m1_pos = m1_pos + 1;
    } 
    else {
      m1_pos = m1_pos - 1;
    }
  }
}

//Motor2 EnccoderA interrupt
void do_m2e1(){
  m2_update = true;
  // look for a low-to-high on channel A
  if (digitalRead(m2_e1) == HIGH) { 
    // check channel B to see which way encoder is turning
    if (digitalRead(m2_e2) == LOW) {  
      m2_pos = m2_pos + 1;
    } 
    else {
      m2_pos = m2_pos - 1;
    }
  }
  else   // must be a high-to-low edge on channel A                                       
  { 
    // check channel B to see which way encoder is turning  
    if (digitalRead(m2_e2) == HIGH) {   
      m2_pos = m2_pos + 1;
    } 
    else {
      m2_pos = m2_pos - 1;
    }
  }

}

//Motor2 Encoder B interrupt
void do_m2e2(){
  m2_update = true;
  // look for a low-to-high on channel B
  if (digitalRead(m2_e2) == HIGH) {   
   // check channel A to see which way encoder is turning
    if (digitalRead(m2_e1) == HIGH) {  
      m2_pos = m2_pos + 1;
    } 
    else {
      m2_pos = m2_pos - 1;
    }
  }
  // Look for a high-to-low on channel B
  else { 
    // check channel B to see which way encoder is turning  
    if (digitalRead(m2_e1) == LOW) {   
      m2_pos = m2_pos + 1;
    } 
    else {
      m2_pos = m2_pos - 1;
    }
  }
}

int i =0;
//to control both motor together when the input is type 'q,_____'
void processDualMotorCommand(String input) {

  m1_pwmValue = getPWMValue(input.substring(2, input.indexOf(',', 2)));
  m1_direction = getDirection(input.substring(2, input.indexOf(',', 2)));
  
  m2_pwmValue = getPWMValue(input.substring(input.indexOf(',', 2) + 1));
  m2_direction = getDirection(input.substring(input.indexOf(',', 2) + 1));

  Serial.println("Parsed values:");
  Serial.print("Motor 1: PWM = ");
  Serial.print(m1_pwmValue);
  Serial.print(", Direction = ");
  Serial.println(m1_direction);
  Serial.print("Motor 2: PWM = ");
  Serial.print(m2_pwmValue);
  Serial.print(", Direction = ");
  Serial.println(m2_direction);

  m1_active = true;
  m2_active = true;
  controlMotor(1, m1_pwmValue, m1_direction);
  controlMotor(2, m2_pwmValue, m2_direction);
}


void loop() {
  if(m1_update && m2_update){
    Serial.print(m1_pos);
    Serial.print(" , ");
    Serial.println(m2_pos);
    
  }
  else if(m1_update) {
    Serial.print("motor1 : ");
    Serial.println(m1_pos);
    m1_update = false;
  }
  else if(m2_update){
    Serial.print("motor2 : ");
    Serial.println(m2_pos);
    m2_update = false;
  }

  
  m11_last = m11_now;
  m12_last = m12_now;
  
  //Get the serial input to control both motor
  //1,1,1 -> motor 1, speed type 1, direction 1
  //2,1,2 -> motor 2, speed type 1, direction 2
  //q,1,1,1,1 -> motor1 speed type 1, direction 1 / motor2 speed tpye 1, direction 1
  if (Serial.available() > 0) {
    String input = Serial.readStringUntil('\n');
    input.trim(); // 공백 제거

    if (input == "a") { // 초기화 명령
      m1_pos =0;
      m2_pos = 0;
      Serial.println(m1_pos);
      Serial.println(m2_pos);
      analogWrite(m1_en, 0);
      analogWrite(m2_en, 0);
      m1_active = false;
      m2_active = false;
      Serial.println("initialize");
    } else if (input.startsWith("1`")) { 
      Serial.println("m1 activate");

      m1_pwmValue = getPWMValue(input);
      m1_direction = getDirection(input);
      m1_active = true; // 모터 1 활성화
    } else if (input.startsWith("2`")) { 
      Serial.println("m2 activate");

      m2_pwmValue = getPWMValue(input);
      m2_direction = getDirection(input);
      m2_active = true; 
    }
    else if (input.startsWith("q`")){
      m1_active = true;
      m2_active = true;
      Serial.println("m1m2 activate");
      processDualMotorCommand(input);
      
    }
  }

  if (m1_active) {
    controlMotor(1, m1_pwmValue, m1_direction);
    //Serial.println(m1_pos);

  }

  if (m2_active) {
    controlMotor(2, m2_pwmValue, m2_direction);
  }
}

// move the motor based on pwm and dir info
void controlMotor(int motor, int pwmValue, char direction) {
  //Serial.println("motor running~");
  int enPin, in1Pin, in2Pin;
  if (motor == 1) {
    enPin = m1_en;
    in1Pin = m1_in1;
    in2Pin = m1_in2;
  } else if (motor == 2) {
    enPin = m2_en;
    in1Pin = m2_in1;
    in2Pin = m2_in2;
  } else {
    return;
  }

  if (direction == '1') { 
    digitalWrite(in1Pin, HIGH);
    digitalWrite(in2Pin, LOW);
  } else if (direction == '2') { 
    digitalWrite(in1Pin, LOW);
    digitalWrite(in2Pin, HIGH);
  }
  if(pwmValue == 1) pwmValue = 50;
  else if(pwmValue == 2) pwmValue = 200;
  analogWrite(enPin, pwmValue);

}

//parsing pwm value
int getPWMValue(String input) {
  int startIndex = input.indexOf('`') + 1;
  int endIndex = input.lastIndexOf('`');
  return input.substring(startIndex, endIndex).toInt();
}

//parsing direction value 
char getDirection(String input) {
  return input.charAt(input.length() - 1);
}

[jremington]

+2 for the Arduino Encoder library. It is very efficient, and works.

[asdfsgada]

Thanks for your answer. I will check your uploaded thread.

@Deva_Rishi

You should use the digitalPinToInterrupt(pin)

I checked the leonardo pinout carefully. I think it should have no issue like @PaulRB mentioned.
pin0 -> interrupt 2
pin1 -> interrupt 3
pin2 -> interrupt 1
pin3 -> interrupt 0

Pinout-Leonardo_latest.pdf

@Deva_Rishi

btw if you are using 'CHANGE" as a trigger, you can also use pin change interrupts which can be used on most pins.

This means that I don't need to use only pin0,1,2,3,7 for interrupt? I can use any other digital pin to use change trigger?

Thank You

[asdfsgada]

@horace
Okay I will check the encoder library

[build_1971]

@asdfsgada

m1_pos =0;
      m2_pos = 0;

These are volatile and should be protected from interrupts.

[asdfsgada]

Yes I declare it with violatile int. Do I have to do some more?

[Deva_Rishi]

@asdfsgada

bool m1_update = false;
bool m2_update = false;

Should also be declared 'volatile' Any variable that is used in the main program also also in the ISR should be declared volatile.

Another important issue that you have to deal with

@asdfsgada

Yes I declare it with violatile int. Do I have to do some more?

The m1_pos and m2_pos variables are of 16-bit type (int is signed 16-bit on a 32u4) and the processor is 8-bit.
so you should disable interrupts momentarily, copy the 16-bit values into other ones, and re-enable interrupts and continue processing with the copied values. There is a chance that the original value is changed while reading it, and since on an 8-bit MCU the process of reading a 16-bit value can be interrupted halfway, this means that the msB and lsB may be referring to different values.

[build_1971]

Read noInterrupts() in arduino docs.
https://docs.arduino.cc/language-reference/en/functions/interrupts/noInterrupts/