Controlling DC motor using encoder

Question

I'm trying to control the speed of two DC motors using an Arduino Uno and encoders that are connected to the motors.

I've written a code to check whether there's a change in the position of the encoder and according to that calculate the velocity of the motors.

Ive used this website for the code:

I'm having problems when calculating the difference between the new position of the encoder and the old position of the encoder. For some reason that difference keeps going up even though the speed stays the same.

This is my code so far:

#define pwmLeft 10
#define pwmRight 5
#define in1 9
#define in2 8
#define in3 7
#define in4 6

//MOTOR A
int motorSpeedA = 100;
static int pinA = 2; // Our first hardware interrupt pin is digital pin 2
static int pinB = 3; // Our second hardware interrupt pin is digital pin 3
volatile byte aFlag = 0; // let's us know when we're expecting a rising edge on pinA to signal that the encoder has arrived at a detent
volatile byte bFlag = 0; // let's us know when we're expecting a rising edge on pinB to signal that the encoder has arrived at a detent (opposite direction to when aFlag is set)
volatile long encoderPos = 0; //this variable stores our current value of encoder position. Change to int or uin16_t instead of byte if you want to record a larger range than 0-255
volatile long oldEncPos = 0; //stores the last encoder position value so we can compare to the current reading and see if it has changed (so we know when to print to the serial monitor)
volatile long reading = 0; //somewhere to store the direct values we read from our interrupt pins before checking to see if we have moved a whole detent

//MOTOR B
static int pinC = 12; // Our first hardware interrupt pin is digital pin 2
static int pinD = 33; // Our second hardware interrupt pin is digital pin 3
volatile byte cFlag = 0; // let's us know when we're expecting a rising edge on pinA to signal that the encoder has arrived at a detent
volatile byte dFlag = 0; // let's us know when we're expecting a rising edge on pinB to signal that the encoder has arrived at a detent (opposite direction to when aFlag is set)
volatile long encoderPosB = 0; //this variable stores our current value of encoder position. Change to int or uin16_t instead of byte if you want to record a larger range than 0-255
volatile long oldEncPosB = 0; //stores the last encoder position value so we can compare to the current reading and see if it has changed (so we know when to print to the serial monitor)
volatile long readingB = 0;

int tempPos;
long vel;
unsigned long newtime;
unsigned long oldtime = 0;

void setup() {
  //MOTOR A
  pinMode(pinA, INPUT_PULLUP); // set pinA as an input, pulled HIGH to the logic voltage (5V or 3.3V for most cases)
  pinMode(pinB, INPUT_PULLUP); // set pinB as an input, pulled HIGH to the logic voltage (5V or 3.3V for most cases)
  attachInterrupt(0, PinA, RISING); // set an interrupt on PinA, looking for a rising edge signal and executing the "PinA" Interrupt Service Routine (below)
  attachInterrupt(1, PinB, RISING); // set an interrupt on PinB, looking for a rising edge signal and executing the "PinB" Interrupt Service Routine (below)
  //MOTOR B
  pinMode(pinC, INPUT_PULLUP); // set pinA as an input, pulled HIGH to the logic voltage (5V or 3.3V for most cases)
  pinMode(pinD, INPUT_PULLUP); // set pinB as an input, pulled HIGH to the logic voltage (5V or 3.3V for most cases)
  attachInterrupt(0, PinC, RISING); // set an interrupt on PinA, looking for a rising edge signal and executing the "PinA" Interrupt Service Routine (below)
  attachInterrupt(1, PinD, RISING);
  Serial.begin(9600); // start the serial monitor link
  pinMode (in1, OUTPUT);
  pinMode (in2, OUTPUT);
  pinMode (in3, OUTPUT);
  pinMode (in4, OUTPUT);
  digitalWrite (8, HIGH);
  digitalWrite (9, LOW); //LOW
  digitalWrite (7, LOW); //LOW
  digitalWrite (6, HIGH);
  pinMode (pwmLeft, OUTPUT);
  pinMode (pwmRight, OUTPUT);
}

void PinA(){
  cli(); //stop interrupts happening before we read pin values
  reading = PIND & 0xC; // read all eight pin values then strip away all but pinA and pinB's values
  if(reading == B00001100 && aFlag) { //check that we have both pins at detent (HIGH) and that we are expecting detent on this pin's rising edge
    encoderPos --; //decrement the encoder's position count
    bFlag = 0; //reset flags for the next turn
    aFlag = 0; //reset flags for the next turn
  } else if (reading == B00000100) bFlag = 1; //signal that we're expecting pinB to signal the transition to detent from free rotation
  sei(); //restart interrupts
}

void PinB(){
  cli(); //stop interrupts happening before we read pin values
  reading = PIND & 0xC; //read all eight pin values then strip away all but pinA and pinB's values
  if (reading == B00001100 && bFlag) { //check that we have both pins at detent (HIGH) and that we are expecting detent on this pin's rising edge
    encoderPos ++; //increment the encoder's position count
    bFlag = 0; //reset flags for the next turn
    aFlag = 0; //reset flags for the next turn
  } else if (reading == B00001000) aFlag = 1; //signal that we're expecting pinA to signal the transition to detent from free rotation
  sei(); //restart interrupts
}

void PinC(){
  cli(); //stop interrupts happening before we read pin values
  readingB = PIND & 0xC; // read all eight pin values then strip away all but pinA and pinB's values
  if(readingB == B00001100 && cFlag) { //check that we have both pins at detent (HIGH) and that we are expecting detent on this pin's rising edge
    encoderPosB --; //decrement the encoder's position count
    dFlag = 0; //reset flags for the next turn
    cFlag = 0; //reset flags for the next turn
  } else if (readingB == B00000100) dFlag = 1; //signal that we're expecting pinB to signal the transition to detent from free rotation
  sei(); //restart interrupts
}

void PinD(){
  cli(); //stop interrupts happening before we read pin values
  readingB = PIND & 0xC; //read all eight pin values then strip away all but pinA and pinB's values
  if (readingB == B00001100 && dFlag) { //check that we have both pins at detent (HIGH) and that we are expecting detent on this pin's rising edge
    encoderPosB ++; //increment the encoder's position count
    dFlag = 0; //reset flags for the next turn
    cFlag = 0; //reset flags for the next turn
  } else if (readingB == B00001000) cFlag = 1; //signal that we're expecting pinA to signal the transition to detent from free rotation
  sei(); //restart interrupts
}

void loop(){
  analogWrite(pwmLeft, motorSpeedA);
  analogWrite(pwmRight, motorSpeedA);
  if(oldEncPos != encoderPos) {
    newtime = millis();
    tempPos = encoderPos - oldEncPos;
    vel = tempPos / (newtime - oldtime);
    Serial.println(tempPos);
    oldEncPos = encoderPos;
    oldtime = newtime;
    delay(250);
  } 
  if(oldEncPosB != encoderPosB) {
    Serial.println(encoderPosB);
    oldEncPosB = encoderPosB;
  }    
}

The two if statements are just made to check that the encoders are working properly. In the first if statement I'm trying to do the calculations of the velocity.

I would appreciate any feedback.

Thank you.

EDIT:

I found out theres an encoder library which makes everything a lot easier.

so now my code looks like this:

#include <Encoder.h>
#define pwmLeft 10
#define pwmRight 5
Encoder myEncA(3, 2);
Encoder myEncB(13, 12);
unsigned long oldtimeA = 0;
unsigned long oldtimeB = 0;
int speedA = 100;
int speedB = 130;

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

  digitalWrite (8, HIGH);
  digitalWrite (9, LOW); //LOW
  digitalWrite (7, LOW); //LOW
  digitalWrite (6, HIGH);

  pinMode (pwmLeft, OUTPUT);
  pinMode (pwmRight, OUTPUT);
}

long oldPositionA  = -999;
long oldPositionB  = -999;

void loop() {

  analogWrite(pwmLeft, speedA);
  analogWrite(pwmRight, speedB);

  long newPositionA = myEncA.read();
  long newPositionB = myEncB.read();
  if ((newPositionA != oldPositionA) || (newPositionB != oldPositionB)) {
    unsigned long newtimeA = millis ();
    long positionA = newPositionA - oldPositionA;
    long positionB = newPositionB - oldPositionB;
    long velB = (positionB) / (newtimeA - oldtimeA);
    long velA = (positionA) / (newtimeA - oldtimeA);
    oldtimeA = newtimeA;
    oldPositionA = newPositionA;
    oldPositionB = newPositionB;
    Serial.println(velB);
  }
}

I am still having problems with my "B" motor, the calculation is still way off for some reason.

Motor "A" works fine

Answer 1

A couple of issues, including a divide by zero error in loop(). This scan cause a reset of your controller. Always check the value of the divisor when doing a division!

Using only positive transitions unnecessarily reduces the resolution of your readings by 2.

The Arduino is an 8bit controller... Reading an int requires multiple instruction, which means you should disable interrupts before reading an int that's modified by an interrupt routine. Failure to do so will cause odd jumps in the vakue read. This is usually done like this:

//...
NoInterrupts();
int copyOfValue = value;   // use the copy to work with.
interrupts();
//...

In your case, a single byte value is likely enough to store movement, with a reset every 30 ms, this should give you a top speed of 255 pulses/30ms = 8500 pulses/second or 1275000 rpm for a 24 ticks/turn encoder. :) in that case, no need to disable interrupts for a reading.

with one reading per 30ms, 1 tick /30ms = 33 tick/seconds, or 85 RPM. It's a bit high for motion. You may need to average readings, depending on your application.

Also, the algorithm you are using will definitely not work. The main reason is that the delay between reads and adjustments is too small. Most readings will be of zero. You will run into the problem when removing the println() calls. I suggest a pacing of at least 30 ms between readings. 100 ms may work a bit better, depending on your application. Using a float variable for speed average will definitely help.

void loop()
{
  //...
  if(oldEncPos != encoderPos) {
    newtime = millis();
    tempPos = encoderPos - oldEncPos;
    vel = tempPos / (newtime - oldtime);   // <--  if newtime == oltime => divide by zero.
    //...
  } 
  //...
}

The encoder reading code seems awfully complex...

#define PIN_A  2  // encoder bit 0
#define PIN_B  3  // encoder bit 1

volatile char encVal1;
volatile unsigned char encPos1;  // using char 

void OnEncoder1Change()
{
   char c = (digitalRead(pinA) ? 0b01 : 0)
          + (digitalRead(pinB) ? 0b10 : 0);  // read
   char delta = (c - encVal1) & 0b11;        // get difference, mask

   if (delta == 1)                           // delta is either 1 or 3
       ++encPos1;
   else
       --encPos1;
   encVal1 = c;                              // keep reading for next time.
   encPos1 += delta;                         // get position.
   // no need to call sei()
}

setup()
{
  pinMode(pinA, INPUT_PULLUP);
  pinMode(pinB, INPUT_PULLUP);

  // get an initial value
  encValA  = digitalRead(pinA) ? 0b01 : 0;
  encValA += digitalRead(pinB) ? 0b10 : 0;

  // use digitalPinToInterrupt() to map interrupts to a pin #
  // ask for interrupt on change, this doubles .
  attachInterrupt(digitalPinToInterrupt(PIN_A), OnEncoder1Change, CHANGE);  
  attachInterrupt(digitalPinToInterrupt(PIN_B), OnEncoder1Change, CHANGE);


  //...
}

unsigned char oldTime;
unsigned char oldPos;
int speed;
void loop()
{
    unsigned char time = millis();

    if (time - oldTime > 30)        // pace readings so you have a reasonable value.
    {
        unsigned char pos = encPos1;
        signed char delta = pos - oldPos;

        speed = 1000 * delta) / (time - oldTime);  // signed ticks/s

        encPos1 -= pos;  // reset using subtraction, do you don't miss out 
                         // on any encoder pulses.
        oldTime = time;
    }
}