Reading an encoder using interrupts

Everyone,

I am trying to read from an encoder using interrupts.

For some reason it is only working in one direction, not the other direction. It never seems to negative decrement and for the life of me I cannot figure it out.

Would anyone mind taking a look at my code and seeing if you see something obvious?

The VNH5019MC is just a motor controller that causes it to move.

The way the MC works is a positive value between 1 and 400 goes in one direction, and -400 to -1 goes the opposite direction.

Either positive or negative values are working as my motor is going in the right direction, but my encoder counter is only going positive.

As an FYI I am resetting at 0 because this application will only have a 360 degree rotation limit when I get the code working properly.

Any help would be much appreciated.

#include "VNH5019MC.h"
VNH5019MC md;


boolean AisHigh,BisHigh;

#define encoderPinA 2
#define encoderPinB 3

short CPR=32; //counts per revolution base
short GR=131; //Gear Ratio

volatile int encoderCount = 0; //Will count my encoder ticks

short DD=2; //debounce delay

boolean rotating; //debouncing boolean




void setup() {
  
 // digitalWrite(2, HIGH);  // turn on pullup resistor
  //digitalWrite(3, HIGH);  // turn on pullup resistor
  
  AisHigh=false; //set boolean for AisHigh = false as starting point
  BisHigh=false; // ""   "" for BisHigh
  
  attachInterrupt(0,riseA,RISING);
  attachInterrupt(0,fallA,FALLING);
  attachInterrupt(1,riseB,RISING);
  attachInterrupt(1,fallB,FALLING);
  
  Serial.begin(9600);
  md.init();
}



void loop() {
  
  rotating=true; //reset debounce boolean
  
  
  goToPos(500,-50); //make motor move CCW at neg 50
  
  Serial.println(encoderCount);
  
}


void goToPos(int encPos,int speed) {
  
    md.setM1Speed(speed);

  
}



void riseA() {
  
  // debounce
  if ( rotating ) delay (DD);  // wait a little until the bouncing is done
  
  AisHigh=true; //found rise set true
  
  iterate();
  /*
  if(BisHigh) {
    encoderCount+=1;
  }
  else {
    encoderCount-=1;
  }
  */
  
  rotating=false;
  
  //Serial.print("RiseA ");
  //Serial.println(AisHigh);
  
}

void fallA() {
  // debounce
  if ( rotating ) delay (DD);  // wait a little until the bouncing is done
  
  AisHigh=false;
  
  iterate();
  
  /*
  if(BisHigh) {
    encoderCount-=1;
  }
  else {
    encoderCount+=1;
  }
    */
    
  rotating=false;
  
  //Serial.print("FallA ");
  //Serial.println(AisHigh);
  
}


void riseB() {
  // debounce
  if ( rotating ) delay (DD);  // wait a little until the bouncing is done
  
  BisHigh=true;
  
  iterate();
  /*
   if(AisHigh) {
    encoderCount+=1;
  }
  else {
    encoderCount-=1;
  }
  */
  rotating=false;
  
  //Serial.print("RiseB ");
  //Serial.println(BisHigh);
  
}


void fallB() {
  // debounce
  if ( rotating ) delay (DD);  // wait a little until the bouncing is done
  
  BisHigh=false;
  
  iterate();
  
  /*
  
  if(AisHigh) {
    encoderCount-=1;
  }
  else {
    encoderCount+=1;
  }
  
  */
  
  rotating=false;
  
  // Serial.print("FallB ");
  //Serial.println(BisHigh);
}

void iterate() {
  
  if(AisHigh == BisHigh) {
    encoderCount+=1;
  }
  else {
    encoderCount-=1;
  }
  
  if(encoderCount>=CPR*GR || encoderCount<=CPR*GR*-1) {
    //implies we have gone one full rotation 
    //Reset the encoderCount
    encoderCount=0;
  }
  
 // Serial.println("I");
  
}

Cañ you really have two ISRs attached to one interrupt source?

I did not study your code - is your direction detector signed variable?
I am using Gray code and direction detection works ( as it should) when encoder A are B are attached to Int0 /int1.

AWOL:
Cañ you really have two ISRs attached to one interrupt source?

No. Only one defined trigger action allowed per attachment declaration.

volatile int encoderCount = 0; //Will count my encoder ticks

This needs to be signed int to detect negative rotation

No. Only one defined trigger action allowed per attachment declaration.

(I knew that, it was just a hint to the OP to check for themself)

Vaclav:
I did not study your code

We have enough problems just getting people to post code, so when they do, I think it is only courteous (there's that word again) to read it, don't you?

Vaclav:

volatile int encoderCount = 0; //Will count my encoder ticks

This needs to be signed int to detect negative rotation

{Facepalm}

Vaclav:

volatile int encoderCount = 0; //Will count my encoder ticks

This needs to be signed int to detect negative rotation

I thought an int was always signed unless declared unsigned.

Everyone, thanks.

I did not realize you could only attach one ISPR per interrupt, that does make sense though.

I will try to tweak this using a change.

If you know which direction the motor is rotating, because you told it to rotate that way, do you really need to get direction from the encoder?

Some links to the motor controller and encoder, if it's a separate device, would be good.

PaulS:
If you know which direction the motor is rotating, because you told it to rotate that way, do you really need to get direction from the encoder?

Some links to the motor controller and encoder, if it's a separate device, would be good.

Paul, the reason I want to know direction is I am making an application with a home built linear actuator that I am hoping will NOT be backdriveable without very high forces applied. So, my hope is that I can turn off my motors at times, and if a force is very high it will actually back drive my motors, rather than destroy my motors. In this case I want to be able to note exact positioning and the only way I can see doing that would be by reading the encoder pos either positive or negative. Hopefully I am explaining this properly, and perhaps I am overcomplicating this and do not need both directions, but I cannot see how at this point.

Regarding the MC and encoder, I am using this Pololu MC:

And this motor/encoder combo also from Pololu:

I have re-written the code as follows, and I am now getting directional position, but I am quite sure that there is a problem somewhere. This encoder is supposed to be 64 CPR, and with a 131:1 gear box, I should be seeing about 8384 counts per output shaft rotation, and I am seeing about 1000 or so.

#include "VNH5019MC.h"
VNH5019MC md;


boolean AisHigh,BisHigh;

#define encoderPinA 2
#define encoderPinB 3

short CPR=64; //counts per revolution base
short GR=131; //Gear Ratio

volatile  int encoderCount = 0;

short DD=2; //debounce delay

boolean rotating; //debouncing boolean




void setup() {
  pinMode(encoderPinA, INPUT); 
  pinMode(encoderPinB, INPUT); 


  digitalWrite(encoderPinA, HIGH);  // turn on pullup resistor
  digitalWrite(encoderPinA, HIGH);  // turn on pullup resistor
  
  AisHigh=false;  //set AisHigh bool to false to set it to low as a start
  BisHigh=false;  //""  BisHigh ""
  
  attachInterrupt(0,changeA,CHANGE);
  
  attachInterrupt(1,changeB,CHANGE);
  
  
  Serial.begin(9600);
  md.init();
}



void loop() {
  
  rotating=true; // reset debunker boolean
  
  
  goToPos(500,50); //set speed and direction
  
  Serial.println(encoderCount);
  
}


void goToPos(int encPos,int speed) {
  
    md.setM1Speed(speed);

  
}

void changeA() {
  
  // debounce
  if ( rotating ) delay (DD);  // wait a little until the bouncing is done
  
  //first toggle the AisHigh boolean
  
  if(digitalRead(encoderPinA)==LOW) AisHigh=false;
  else AisHigh=true;
  
  
  iterate();
  
  rotating=false;
  
}

void changeB() {
  
  // debounce
  if ( rotating ) delay (DD);  // wait a little until the bouncing is done
  
  //first toggle the BisHigh boolean
  if(digitalRead(encoderPinB)==LOW) BisHigh=false;
  else BisHigh=true;
  
  
  iterate();
  
  rotating=false;

  
}



void iterate() {
  
  if(AisHigh == BisHigh) {
    encoderCount+=1;
  }
  else {
    encoderCount-=1;
  }
  
  if(encoderCount>=CPR*GR || encoderCount<=CPR*GR*-1) {
    //implies we have gone one full rotation 
    //Reset the encoderCount
    encoderCount=0;
  }
  
 // Serial.println("I");
  
}
  attachInterrupt(0,changeA,CHANGE);
  attachInterrupt(1,changeB,CHANGE);

So, changeA() and changeB() are interrupt service routines.

void changeA() {
  
  // debounce
  if ( rotating ) delay (DD);  // wait a little until the bouncing is done

You can't use delay() in an ISR. ISRs are supposed to be fast. delay() is the exact opposite of fast.

void iterate() 
{
 // Serial.println("I");
}

I know that Serial.print() is commented out, but Serial.print() relies on interrupts, which are disabled during an ISR. You should never call Serial.print() from an ISR. Not even for debugging purposes.

Interestingly enough I just added encoders to my project.

The counts per revolution are very high. They will generate an interrupt around once every 120 microseconds. You do not want to be, and should not need to debounce the signals. If your counts seem low it's because you are not servicing the interrupts fast enough.

This is the code I used to read my encoder pololu encoder: Pololu - Encoder for Pololu Wheel 42x19mm

It uses a timer interrupt to sample the encoder output but in your case you would use the pin interrupts. The code is also setup to service two encoders, one for each of the bot's two motors.

//----------------------------------------
//
//----------------------------------------

volatile int16_t _lft_ticks = 0;
volatile int16_t _rht_ticks = 0;
volatile uint16_t _counter = 0;
volatile bool _error = false;
char _lApin;
char _lBpin;
char _rApin;
char _rBpin;

//----------------------------------------
//
//----------------------------------------

void setup_encoder( char lftPinA, char lftPinB, char rhtPinA, char rhtPinB )
{
  _init_pin( &_lApin, lftPinA);
  _init_pin( &_lBpin, lftPinB);
  _init_pin( &_rApin, rhtPinA);
  _init_pin( &_rBpin, rhtPinB);

  cli();
  // set timer2 interrupt at 1000 Hz
	// we are assuming a clk speed of 16MHz
  TCCR2A = 0;
  TCCR2B = 0;
  TCNT2  = 0;
  OCR2A = 249;
  TCCR2A |= (1 << WGM21);
  TCCR2B |= (1 << CS22);   
  TIMSK2 |= (1 << OCIE2A);
  sei();
}

//----------------------------------------
//
//----------------------------------------

void _init_pin( char *pin, char value)
{
  *pin = value;
  pinMode(value, INPUT);
  digitalWrite( value, HIGH);
}

//----------------------------------------
//
//----------------------------------------

bool get_ticks_since_last( int16_t *lft, int16_t *rht, uint16_t *ms )
{
  cli();
  *lft = _lft_ticks;
  *rht = _rht_ticks;
  *ms = _counter;
  _lft_ticks = _rht_ticks = _counter = 0;
  char error = _error;
  _error = false;
  sei();

  return !error;
}

//----------------------------------------
//
//----------------------------------------

void clear_ticks()
{
  cli();
  _lft_ticks = _rht_ticks = _counter = 0;
  _error = false;
  sei();
}

//----------------------------------------
//
//----------------------------------------

ISR(TIMER2_COMPA_vect)
{
  // this routine gets called once every 1 millisecond
  _counter++;

  static char lastLA = 0;
  static char lastLB = 0;
  static char lastRA = 0;
  static char lastRB = 0;

  _process(_lApin, _lBpin, &lastLA, &lastLB, &_lft_ticks);
  _process(_rApin, _rBpin, &lastRA, &lastRB, &_rht_ticks);
}

//----------------------------------------
//
//----------------------------------------

void _process( char Apin, char Bpin, char *lastA, char *lastB, volatile int16_t *ticks )
{
  char A = (digitalRead( Apin) == HIGH) ? 1 : 0;
  char B = (digitalRead( Bpin) == HIGH) ? 1 : 0;
  char lA = *lastA;
  char lB = *lastB;
  char dA = A!=lA;
  char dB = B!=lB;

  if( dA && dB )
  {
    // both should not change at the same time
    _error = true;
  }
  else if ( dA || dB )
  {
    if (A^lB) 
    {
      *ticks += 1;
    }
    else if(B^lA)
    {
      *ticks -= 1;
    }
  }
  *lastA = A; 
  *lastB = B;
}

Hope that is somewhat helpful. I explain a lot of it here: http://wp.me/p493sy-7L

  _lft_ticks = _rht_ticks = _counter = 0;
  _error = false;

What's with the leading underscores? Wouldn't it be easier, and less work, and more readable, to omit them?

char _lApin;
char _lBpin;
char _rApin;
char _rBpin;

According to the C++ standard, variables in the global namespace, starting with an underscore, are reserved.

17.4.3.2.1 Global names

Certain sets of names and function signatures are always reserved to the implementation:

Each name that contains a double underscore (_ _) or begins with an underscore followed by an uppercase letter is reserved to the implementation for any use.

Each name that begins with an underscore is reserved to the implementation for use as a name in the global namespace.

Sorry, I should have cleaned up the code a bit. I had the encoder in it's own .ino file and wanted to not worry about name clashes with the main code.

Ultimately I would move the code to a library and make it a class.

I found an encoder library from Adafruit that works well:
http://code.google.com/p/adaencoder/

It's nice in that it returns a signed value 'clicks' that is the number of clicks/detents you've turned with the knob.