Urgent need help with PIM604 encoder switch bouncing

Im making a micromouse according to UKMARS spec - see schematics: Main Board - UKMARS

and im using a Pololu N20 20:1 motor with a Pimeroni PIM604 magnetic encoder with 6 pole magnetic disk.

when using attach interrupt to count the encoder ticks it doesnt count properly and I get a weird output of zeroes and ones and negative ones when I turn the wheel and the direction does not seem to matter

Here is my code:

#if defined(__AVR_ATmega328__) || defined(__AVR_ATmega328P__)
#define __digitalPinToPortReg(P) (((P) <= 7) ? &PORTD : (((P) >= 8 && (P) <= 13) ? &PORTB : &PORTC))
#define __digitalPinToDDRReg(P) (((P) <= 7) ? &DDRD : (((P) >= 8 && (P) <= 13) ? &DDRB : &DDRC))
#define __digitalPinToPINReg(P) (((P) <= 7) ? &PIND : (((P) >= 8 && (P) <= 13) ? &PINB : &PINC))
#define __digitalPinToBit(P) (((P) <= 7) ? (P) : (((P) >= 8 && (P) <= 13) ? (P)-8 : (P)-14))

// general macros/defines
#if !defined(BIT_READ)
#define BIT_READ(value, bit) ((value) & (1UL << (bit)))
#endif
#if !defined(BIT_SET)
#define BIT_SET(value, bit) ((value) |= (1UL << (bit)))
#endif
#if !defined(BIT_CLEAR)
#define BIT_CLEAR(value, bit) ((value) &= ~(1UL << (bit)))
#endif
#if !defined(BIT_WRITE)
#define BIT_WRITE(value, bit, bitvalue) (bitvalue ? BIT_SET(value, bit) : BIT_CLEAR(value, bit))
#endif

#define fast_write_pin(P, V) BIT_WRITE(*__digitalPinToPortReg(P), __digitalPinToBit(P), (V));
#define fast_read_pin(P) ((int)(((BIT_READ(*__digitalPinToPINReg(P), __digitalPinToBit(P))) ? HIGH : LOW)))

#else
#define fast_write_pin(P, V) digitalWrite(P, V)
#define fast_read_pin(P) digitalRead(P)
#endif

const int RED_LEDS = 12; // RED LEDS
const int RED_LED_H_BRIDGE = 13; // RED LED H BRIDGE

const int INDICATOR_LED_R = 6; // INDICATOR LED RIGHT 
const int INDICATOR_LED_L = 11; // INDICATOR LED LEFT

const int ENCODER_R_A = 3; // ENCODER RIGHT A (ticks first when motor forward)
const int ENCODER_R_B = 5; // ENCODER RIGHT B (ticks first when motor backward)

const int ENCODER_L_A = 4; // ENCODER LEFT A (ticks first when motor forward)
const int ENCODER_L_B = 2; // ENCODER LEFT B (ticks first when motor backward)

const int SPEED_MOTOR_L = 9; // PWM MOTOR LEFT 
const int SPEED_MOTOR_R = 10; // PWM MOTOR RIGHT 

const int DIR_MOTOR_L = 7; // DIRECTION MOTOR LEFT 
const int DIR_MOTOR_R = 8; // DIRECTION MOTOR RIGHT 

const int RIGHT_SENSOR = A0;
const int LEFT_SENSOR = A2;
const int MIDDLE_SENSOR = A1;

int rightEncoderPos = 0; // Counts for right encoder position
int leftEncoderPos = 0; // Counts for left encoder position


void setup() {
  Serial.begin(9600);
  
  pinMode(RED_LEDS, OUTPUT);
  pinMode(RED_LED_H_BRIDGE, OUTPUT);
  pinMode(INDICATOR_LED_R, OUTPUT);
  pinMode(INDICATOR_LED_L, OUTPUT);

  pinMode(ENCODER_R_A, INPUT);
  pinMode(ENCODER_R_B, INPUT);
  pinMode(ENCODER_L_A, INPUT);
  pinMode(ENCODER_L_B, INPUT);

  pinMode(SPEED_MOTOR_L, OUTPUT);
  pinMode(SPEED_MOTOR_R, OUTPUT);
  pinMode(DIR_MOTOR_L, OUTPUT);
  pinMode(DIR_MOTOR_R, OUTPUT);

  attachInterrupt(digitalPinToInterrupt(ENCODER_L_B), readEncoderLeft, RISING);

}

void loop() {

  int read_right_sensor = analogRead(RIGHT_SENSOR);
  int read_left_sensor = analogRead(LEFT_SENSOR);
  int read_middle_sensor = analogRead(MIDDLE_SENSOR);


  Serial.println(leftEncoderPos);

}

void readEncoderLeft(){
  int readEncoderA;
  readEncoderA = fast_read_pin(ENCODER_L_A);
  if(readEncoderA > 0){
    leftEncoderPos++; // If the interrupt triggered and A is already high then we are moving forward
  } else {
    leftEncoderPos--; // Otherwise we are moving backwards 
  }
}

fast read pin method just reads the pin quicker but i get the same issues with normal digitalread()
What can I do to solve this issue

Try setting a flag in the ISR, then processing the flag outside the ISR.

void loop() {
  if (readEL)
    processEL();
}

void readEncoderLeft(){
  readEL = 1;
}

void processEL() {
  readEL = 0;
  int readEncoderA;
  readEncoderA = fast_read_pin(ENCODER_L_A);
  if(readEncoderA > 0){
    leftEncoderPos++; // If the interrupt triggered and A is already high then we are moving forward
  } else {
    leftEncoderPos--; // Otherwise we are moving backwards 
  }
}

Variables that are used inside and outside a interrupt service routine must be declared volatile.

I recommend that you use the NewEncoder-library instead of writing the encoder-code from scratch

The encoder outputs are NPN open collector, input pins should be:

pinMode(ENCODER_R_A, INPUT_PULLUP);

The attach interrupt line should be:

attachInterrupt(digitalPinToInterrupt(ENCODER_L_B), readEncoderLeft, FALLING);

Here's a simple test sketch I use with a Pololu gearmotor and DRV8871 driver, works fine at over 6000 RPM without missing counts.

unsigned long start;
long target;
              
const byte ain1Pin = 9,
           ain2Pin = 10,
           encoderPinA = 2,
           encoderPinB = 4;
const uint16_t end = 500;
           
int speed = 48;
volatile long pos;           
int16_t pps, oldpps;
long err; 

void setup()
{
  Serial.begin(9600);
  attachInterrupt(0, readEncoder, FALLING);
  pinMode(ain1Pin,OUTPUT);
  pinMode(ain2Pin,OUTPUT);
  pinMode(encoderPinA,INPUT_PULLUP);
  pinMode(encoderPinB,INPUT_PULLUP);    
}

void loop()
{
  if(Serial.available() > 0)
  {
     if(Serial.peek() == 's')
       speed = Serial.parseInt();
     target = Serial.parseInt();
     //target += pos;
     Serial.println(target);
     if(Serial.read() == '\n')
         while(Serial.available() > 0)
         {
          Serial.read();  
         }
  }
  err = target - pos;
  if(err == 0)
    {   
      digitalWrite(ain1Pin,HIGH);
      digitalWrite(ain2Pin,HIGH); 
    }
    else if(err < 0)
    {
      analogWrite(ain2Pin,0);  
      analogWrite(ain1Pin,speed);
    }
    else
    {
      analogWrite(ain1Pin,0);  
      analogWrite(ain2Pin,speed);
  }
    
  if(millis() - start > end)
  {
     noInterrupts();
     pps = pos - oldpps;
     oldpps = pos;
     interrupts();
     if(pps != 0)
     {
       Serial.print(pos);
       Serial.print("\t" ); Serial.print(pps * 2);
       Serial.print("\t" ); Serial.println(err);
     } 
     start += end;     
  }
}
void readEncoder()
{
  bitRead(PIND,4) ? --pos : ++pos;
}  


Thanks for the reply
I followed some advice in the replies and changed pin modes to input pullup and made the counter variable volatile

What I found on more inspection is while running at highest baud rate and using a stable power source - encoder pulse B has some bounce where it looks like its 2x freq and is negating every increment from the comparison in the readEncoder function

please see Imgur: The magic of the Internet for the screenshot of serial plotter

The State Table Technique for rotary encoders provides automatic debouncing.

Carefully take the back cap off the motor / encoder and inspect. I fixed one once by carefully bending the Hall sensor toward the magnet wheel, reducing the air gap.

Thank you so much - I didn't understand this at all when first looking at it
then I ended up making an ISR that basically did the same thing but very inefficiently:


volatile bool interruptOccurred = false;
volatile unsigned long startTime;
volatile unsigned long endTime;
volatile int uptick = 1;
volatile bool isActive;

...

attachInterrupt(digitalPinToInterrupt(ENCODER_L_B), interruptHandlerLeft, CHANGE);

...

void interruptHandlerLeft() {
 if (interruptOccurred) {
   if(uptick == 4){
     endTime = micros();
     uptick = 1;
   }
   if(uptick == 3 && fast_read_pin(ENCODER_L_A) == LOW && isActive == true){ // If A active when B trigger and on 3rd pulse is low then forward
         leftEncoderPos++;
         isActive = false;
     } else if(uptick == 2 && fast_read_pin(ENCODER_L_A) == HIGH && isActive == false){ // If A not active when B trigger and also active on second tick we are in reverse
         leftEncoderPos--;
     }
 } else {
   if(uptick == 1){
     startTime = micros();
     if(fast_read_pin(ENCODER_L_A) == HIGH){
       isActive = true;  
     } else{
       isActive = false;
     }
   }
   uptick++;
 }
 interruptOccurred = !interruptOccurred;
}

As it turns out - this is like checking the state transitions:

void interruptHandlerLeft() {
  static uint8_t prevState = 0; 
  static uint8_t currState = 0;
  static unsigned long lastInterrupt = 0; 
  
  currState = (fast_read_pin(ENCODER_L_B) << 1) | fast_read_pin(ENCODER_L_A);
  
  unsigned long currentTime = micros();
  unsigned long deltaTime = currentTime - lastTime;
  lastInterrupt = currentTime;
  
  // direction based on prev direction
  uint8_t direction = (prevState << 2) | currState;
  switch(direction) {
    case 0b0001:
    case 0b0111:
    case 0b1110:
    case 0b1000:
      leftEncoderPos++;
      break;
    case 0b0010:
    case 0b1100:
    case 0b0101:
    case 0b1011:
      leftEncoderPos--;
      break;

    default:
      break;
  }

  prevState = currState;
}

Thanks man now my encoders can actually count ticks of rotations

What Arduino board are you compiling for? I wrote a very efficient implementation of that algorithm. Take a look, it might give you some ideas: https://github.com/gfvalvo/NewEncoder