Delay function doesn't work

Hello, I am testing a DC gearmotor with an incremental optical encoder and want to calibrate it for the encoder clicks.

I am using an Arduino Mega 2560 and using pins 2 and 3 for A and B outputs of the encoder. I am using a Cytron MD 13S motor driver. The PWM output is connected to Mega's pin 4 and Direction outputput is connected to pin 7.

When I run the code without the encoders with interrupts it works fine but with the interrupts, the motor just keeps going and going as though the delay (1000) in line 28 doesn't exist. The debugging line after that doesn't appear on the serial monitor. What am I doing wrong? There are no compiler errors.

#include "CytronMotorDriver.h"
#define encoder0PinA  2
#define encoder0PinB  3

// Configure the motor driver.
CytronMD motor(PWM_DIR, 4, 7);  // PWM = Pin 4, DIR = Pin 7.


volatile unsigned int encoder0Pos = 0;

void setup() {
  pinMode(encoder0PinA, INPUT);
  pinMode(encoder0PinB, INPUT);

  // encoder pin on interrupt 0 (pin 2)
  attachInterrupt(0, doEncoderA, CHANGE);

  // encoder pin on interrupt 1 (pin 3)
  //attachInterrupt(1, doEncoderB, CHANGE);

  Serial.begin (9600);
}

void loop() {


  motor.setSpeed(128);  // Run forward at 50% speed.
  delay(1000);

  Serial.println ("First part Done");

  //motor.setSpeed(255);  // Run forward at full speed.
  //delay(1000);



  motor.setSpeed(0);    // Stop.
  delay(1000);

  motor.setSpeed(-128);  // Run backward at 50% speed.
  delay(1000);

  Serial.println ("Second Part Done");

  //motor.setSpeed(-255);  // Run backward at full speed.
  //delay(1000);

  motor.setSpeed(0);    // Stop.
  delay(1000);



}

void doEncoderA() {
  // look for a low-to-high on channel A
  if (digitalRead(encoder0PinA) == HIGH) {

    // check channel B to see which way encoder is turning
    if (digitalRead(encoder0PinB) == LOW) {
      encoder0Pos = encoder0Pos + 1;         // CW
    }
    else {
      encoder0Pos = encoder0Pos - 1;         // CCW
    }
  }

  else   // must be a high-to-low edge on channel A
  {
    // check channel B to see which way encoder is turning
    if (digitalRead(encoder0PinB) == HIGH) {
      encoder0Pos = encoder0Pos + 1;          // CW
    }
    else {
      encoder0Pos = encoder0Pos - 1;          // CCW
    }
  }
  Serial.println (encoder0Pos, DEC);
  // use for debugging - remember to comment out
}

void doEncoderB() {
  // look for a low-to-high on channel B
  if (digitalRead(encoder0PinB) == HIGH) {

    // check channel A to see which way encoder is turning
    if (digitalRead(encoder0PinA) == HIGH) {
      encoder0Pos = encoder0Pos + 1;         // CW
    }
    else {
      encoder0Pos = encoder0Pos - 1;         // CCW
    }
  }

  // Look for a high-to-low on channel B

  else {
    // check channel B to see which way encoder is turning
    if (digitalRead(encoder0PinA) == LOW) {
      encoder0Pos = encoder0Pos + 1;          // CW
    }
    else {
      encoder0Pos = encoder0Pos - 1;          // CCW
    }
  }
}

Get rid of the Serial.print in the ISR. May not be the only problem. But, it is a problem.

How many ticks/rotation on that encoder? I assume it's on the motor shaft, not the gearbox output, correct?

Ok .. that worked, thanks! But now I don't get the readout of the encoder - how do I get it?

Blackfin:
How many ticks/rotation on that encoder? I assume it's on the motor shaft, not the gearbox output, correct?

The specs say it is 200 cpr and the encoder is attached to the main shaft and not the output shaft. I want to get the exact count so that I can use this motor for precise positioning.

An external interrupt has a higher priority here than the overflow ISR of timer0 which controls delay() and other timing functions. A heavy weight external interrupt ISR will block these timing functions. In principle, ISRs must be short and fast. As pointed out, the Serial function is definitely bad.

cosmicone:
The specs say it is 200 cpr and the encoder is attached to the main shaft and not the output shaft. I want to get the exact count so that I can use this motor for precise positioning.

OK. My thought was that it might be a 1024tick/rev encoder and was flooding the MCU with so many interrupts it couldn't service delay() in a timely way.

Glad to see it was just the Serial.print() in the ISR.

Because the encoder value is not a single byte, reading its value will require a few bus cycles and so could be interrupted by the encoder ISR.

If you want to read the value, you could do something like

    noInterrupts();
    uint16_t nowEnc = encoder0Pos;
    interrupts();

Blackfin:
OK. My thought was that it might be a 1024tick/rev encoder and was flooding the MCU with so many interrupts it couldn't service delay() in a timely way.

Glad to see it was just the Serial.print() in the ISR.

Because the encoder value is not a single byte, reading its value will require a few bus cycles and so could be interrupted by the encoder ISR.

If you want to read the value, you could do something like

    noInterrupts();

uint16_t nowEnc = encoder0Pos;
   interrupts();

Where do I insert that code and how do I see the encoder value on the serial monitor?

As an experiment try making your loop something like:

void loop() 
{
    motor.setSpeed(128);  // Run forward at 50% speed.
    delay(1000);
    motor.setSpeed(0);    // Stop.
    delay(1000);

    noInterrupts();
    uint16_t nowEnc = encoder0Pos;
    interrupts;    
    Serial.print( "Encoder value after part 1: " ); Serial.println( nowEnc );

    motor.setSpeed(-128);  // Run backward at 50% speed.
    delay(1000);
    motor.setSpeed(0);    // Stop.
    delay(1000);

    noInterrupts();
    uint16_t nowEnc = encoder0Pos;
    encoder0Pos = 0;
    interrupts;    
    Serial.print( "Encoder value after part 2: " ); Serial.println( nowEnc );

}

Blackfin:
As an experiment try making your loop something like:

void loop() 

{
   motor.setSpeed(128);  // Run forward at 50% speed.
   delay(1000);
   motor.setSpeed(0);    // Stop.
   delay(1000);

noInterrupts();
   uint16_t nowEnc = encoder0Pos;
   interrupts;    
   Serial.print( "Encoder value after part 1: " ); Serial.println( nowEnc );

motor.setSpeed(-128);  // Run backward at 50% speed.
   delay(1000);
   motor.setSpeed(0);    // Stop.
   delay(1000);

noInterrupts();
   uint16_t nowEnc = encoder0Pos;
   encoder0Pos = 0;
   interrupts;    
   Serial.print( "Encoder value after part 2: " ); Serial.println( nowEnc );

}

Won't compile.

Arduino: 1.8.13 (Windows Store 1.8.42.0) (Windows 10), Board: "Arduino Mega or Mega 2560, ATmega2560 (Mega 2560)"

C:\Users\IT Support\Documents\Arduino\cytron_with_encoder\cytron_with_encoder.ino: In function 'void loop()':

cytron_with_encoder:42:5: error: 'interrupts' was not declared in this scope

interrupts;

^~~~~~~~~~

C:\Users\IT Support\Documents\Arduino\cytron_with_encoder\cytron_with_encoder.ino:42:5: note: suggested alternative: 'intptr_t'

interrupts;

^~~~~~~~~~

intptr_t

cytron_with_encoder:57:14: error: redeclaration of 'uint16_t nowEnc'

uint16_t nowEnc = encoder0Pos;

^~~~~~

C:\Users\IT Support\Documents\Arduino\cytron_with_encoder\cytron_with_encoder.ino:41:14: note: 'uint16_t nowEnc' previously declared here

uint16_t nowEnc = encoder0Pos;

^~~~~~

exit status 1

'interrupts' was not declared in this scope

Oops. Put a pair of brackets at the end of interrupts:

interrupts();

Blackfin:
Oops. Put a pair of brackets at the end of interrupts:

interrupts();

Still won't compile :frowning:
Arduino: 1.8.13 (Windows Store 1.8.42.0) (Windows 10), Board: "Arduino Mega or Mega 2560, ATmega2560 (Mega 2560)"

C:\Users\IT Support\Documents\Arduino\cytron_with_encoder\cytron_with_encoder.ino: In function 'void loop()':

cytron_with_encoder:57:14: error: redeclaration of 'uint16_t nowEnc'

uint16_t nowEnc = encoder0Pos;

^~~~~~

exit status 1

redeclaration of 'uint16_t nowEnc'

Copy paste error on my part. Remove the uint16_t in front of the 2nd nowEnc:

    noInterrupts();
    nowEnc = encoder0Pos;
    encoder0Pos = 0;
    interrupts();    
    Serial.print( "Encoder value after part 2: " ); Serial.println( nowEnc );

Blackfin:
Copy paste error on my part. Remove the uint16_t in front of the 2nd nowEnc:

    noInterrupts();

nowEnc = encoder0Pos;
   encoder0Pos = 0;
   interrupts();    
   Serial.print( "Encoder value after part 2: " ); Serial.println( nowEnc );

Still getting compiling errors :frowning:

Arduino: 1.8.13 (Windows Store 1.8.42.0) (Windows 10), Board: "Arduino Mega or Mega 2560, ATmega2560 (Mega 2560)"

C:\Users\IT Support\Documents\Arduino\cytron_with_encoder\cytron_with_encoder.ino: In function 'void loop()':

cytron_with_encoder:41:5: error: 'nowEnc' was not declared in this scope

nowEnc = encoder0Pos;

^~~~~~

C:\Users\IT Support\Documents\Arduino\cytron_with_encoder\cytron_with_encoder.ino:41:5: note: suggested alternative: 'noTone'

nowEnc = encoder0Pos;

^~~~~~

noTone

exit status 1

'nowEnc' was not declared in this scope

Please post the code as it sits now.

Blackfin:
Please post the code as it sits now.

#include "CytronMotorDriver.h"

#define encoder0PinA  2
#define encoder0PinB  3

// Configure the motor driver.
CytronMD motor(PWM_DIR, 4, 7);  // PWM = Pin 4, DIR = Pin 7.

volatile unsigned int encoder0Pos = 0;

void setup() {
  pinMode(encoder0PinA, INPUT);
  pinMode(encoder0PinB, INPUT);

// encoder pin on interrupt 0 (pin 2)
  attachInterrupt(0, doEncoderA, CHANGE);

// encoder pin on interrupt 1 (pin 3)
  //attachInterrupt(1, doEncoderB, CHANGE);

Serial.begin (9600);
}

void loop() {

motor.setSpeed(128);  // Run forward at 50% speed.
  delay(1000);

Serial.println (“First part Done”);

//motor.setSpeed(255);  // Run forward at full speed.
  //delay(1000);

motor.setSpeed(0);    // Stop.
  delay(1000);

noInterrupts();
    nowEnc = encoder0Pos;
    encoder0Pos = 0;
    interrupts(); 
    Serial.print( "Encoder value after part 2: " ); Serial.println( nowEnc );

motor.setSpeed(-128);  // Run backward at 50% speed.
  delay(1000);

Serial.println (“Second Part Done”);

//motor.setSpeed(-255);  // Run backward at full speed.
  //delay(1000);

motor.setSpeed(0);    // Stop.
  delay(1000);

noInterrupts();
    nowEnc = encoder0Pos;
    encoder0Pos = 0;
    interrupts(); 
    Serial.print( "Encoder value after part 2: " ); Serial.println( nowEnc );

}

void doEncoderA() {
  // look for a low-to-high on channel A
  if (digitalRead(encoder0PinA) == HIGH) {

// check channel B to see which way encoder is turning
    if (digitalRead(encoder0PinB) == LOW) {
      encoder0Pos = encoder0Pos + 1;        // CW
    }
    else {
      encoder0Pos = encoder0Pos - 1;        // CCW
    }
  }

else  // must be a high-to-low edge on channel A
  {
    // check channel B to see which way encoder is turning
    if (digitalRead(encoder0PinB) == HIGH) {
      encoder0Pos = encoder0Pos + 1;          // CW
    }
    else {
      encoder0Pos = encoder0Pos - 1;          // CCW
    }
  }
  //Serial.println (encoder0Pos, DEC);
  // use for debugging - remember to comment out
}

void doEncoderB() {
  // look for a low-to-high on channel B
  if (digitalRead(encoder0PinB) == HIGH) {

// check channel A to see which way encoder is turning
    if (digitalRead(encoder0PinA) == HIGH) {
      encoder0Pos = encoder0Pos + 1;        // CW
    }
    else {
      encoder0Pos = encoder0Pos - 1;        // CCW
    }
  }

// Look for a high-to-low on channel B

else {
    // check channel B to see which way encoder is turning
    if (digitalRead(encoder0PinA) == LOW) {
      encoder0Pos = encoder0Pos + 1;          // CW
    }
    else {
      encoder0Pos = encoder0Pos - 1;          // CCW
    }
  }
}

OK. I think you erased both instances of the "uint16_t"; I think this works now (it compiles here...)

#include "CytronMotorDriver.h"
#define encoder0PinA  2
#define encoder0PinB  3

// Configure the motor driver.
CytronMD motor(PWM_DIR, 4, 7);  // PWM = Pin 4, DIR = Pin 7.


volatile unsigned int encoder0Pos = 0;

void setup() {
  pinMode(encoder0PinA, INPUT);
  pinMode(encoder0PinB, INPUT);

  // encoder pin on interrupt 0 (pin 2)
  attachInterrupt(0, doEncoderA, CHANGE);

  // encoder pin on interrupt 1 (pin 3)
  //attachInterrupt(1, doEncoderB, CHANGE);

  Serial.begin (9600);
}

void loop() {


  motor.setSpeed(128);  // Run forward at 50% speed.
  delay(1000);

  Serial.println ("First part Done");

  //motor.setSpeed(255);  // Run forward at full speed.
  //delay(1000);



  motor.setSpeed(0);    // Stop.
  delay(1000);

   noInterrupts();
    uint16_t nowEnc = encoder0Pos;
    interrupts();   
    Serial.print( "Encoder value after part 2: " ); Serial.println( nowEnc );

  motor.setSpeed(-128);  // Run backward at 50% speed.
  delay(1000);

  Serial.println ("Second Part Done");

  //motor.setSpeed(-255);  // Run backward at full speed.
  //delay(1000);

  motor.setSpeed(0);    // Stop.
  delay(1000);

noInterrupts();
    nowEnc = encoder0Pos;
    encoder0Pos = 0;
    interrupts();   
    Serial.print( "Encoder value after part 2: " ); Serial.println( nowEnc );

}

void doEncoderA() {
  // look for a low-to-high on channel A
  if (digitalRead(encoder0PinA) == HIGH) {

    // check channel B to see which way encoder is turning
    if (digitalRead(encoder0PinB) == LOW) {
      encoder0Pos = encoder0Pos + 1;         // CW
    }
    else {
      encoder0Pos = encoder0Pos - 1;         // CCW
    }
  }

  else   // must be a high-to-low edge on channel A
  {
    // check channel B to see which way encoder is turning
    if (digitalRead(encoder0PinB) == HIGH) {
      encoder0Pos = encoder0Pos + 1;          // CW
    }
    else {
      encoder0Pos = encoder0Pos - 1;          // CCW
    }
  }
  //Serial.println (encoder0Pos, DEC);
  // use for debugging - remember to comment out
}

void doEncoderB() {
  // look for a low-to-high on channel B
  if (digitalRead(encoder0PinB) == HIGH) {

    // check channel A to see which way encoder is turning
    if (digitalRead(encoder0PinA) == HIGH) {
      encoder0Pos = encoder0Pos + 1;         // CW
    }
    else {
      encoder0Pos = encoder0Pos - 1;         // CCW
    }
  }

  // Look for a high-to-low on channel B

  else {
    // check channel B to see which way encoder is turning
    if (digitalRead(encoder0PinA) == LOW) {
      encoder0Pos = encoder0Pos + 1;          // CW
    }
    else {
      encoder0Pos = encoder0Pos - 1;          // CCW
    }
  }
}

Blackfin:
OK. I think you erased both instances of the "uint16_t"; I think this works now (it compiles here...)

#include "CytronMotorDriver.h"

#define encoder0PinA  2
#define encoder0PinB  3

// Configure the motor driver.
CytronMD motor(PWM_DIR, 4, 7);  // PWM = Pin 4, DIR = Pin 7.

volatile unsigned int encoder0Pos = 0;

void setup() {
 pinMode(encoder0PinA, INPUT);
 pinMode(encoder0PinB, INPUT);

// encoder pin on interrupt 0 (pin 2)
 attachInterrupt(0, doEncoderA, CHANGE);

// encoder pin on interrupt 1 (pin 3)
 //attachInterrupt(1, doEncoderB, CHANGE);

Serial.begin (9600);
}

void loop() {

motor.setSpeed(128);  // Run forward at 50% speed.
 delay(1000);

Serial.println ("First part Done");

//motor.setSpeed(255);  // Run forward at full speed.
 //delay(1000);

motor.setSpeed(0);    // Stop.
 delay(1000);

noInterrupts();
   uint16_t nowEnc = encoder0Pos;
   interrupts();  
   Serial.print( "Encoder value after part 2: " ); Serial.println( nowEnc );

motor.setSpeed(-128);  // Run backward at 50% speed.
 delay(1000);

Serial.println ("Second Part Done");

//motor.setSpeed(-255);  // Run backward at full speed.
 //delay(1000);

motor.setSpeed(0);    // Stop.
 delay(1000);

noInterrupts();
   nowEnc = encoder0Pos;
   encoder0Pos = 0;
   interrupts();  
   Serial.print( "Encoder value after part 2: " ); Serial.println( nowEnc );

}

void doEncoderA() {
 // look for a low-to-high on channel A
 if (digitalRead(encoder0PinA) == HIGH) {

// check channel B to see which way encoder is turning
   if (digitalRead(encoder0PinB) == LOW) {
     encoder0Pos = encoder0Pos + 1;         // CW
   }
   else {
     encoder0Pos = encoder0Pos - 1;         // CCW
   }
 }

else   // must be a high-to-low edge on channel A
 {
   // check channel B to see which way encoder is turning
   if (digitalRead(encoder0PinB) == HIGH) {
     encoder0Pos = encoder0Pos + 1;          // CW
   }
   else {
     encoder0Pos = encoder0Pos - 1;          // CCW
   }
 }
 //Serial.println (encoder0Pos, DEC);
 // use for debugging - remember to comment out
}

void doEncoderB() {
 // look for a low-to-high on channel B
 if (digitalRead(encoder0PinB) == HIGH) {

// check channel A to see which way encoder is turning
   if (digitalRead(encoder0PinA) == HIGH) {
     encoder0Pos = encoder0Pos + 1;         // CW
   }
   else {
     encoder0Pos = encoder0Pos - 1;         // CCW
   }
 }

// Look for a high-to-low on channel B

else {
   // check channel B to see which way encoder is turning
   if (digitalRead(encoder0PinA) == LOW) {
     encoder0Pos = encoder0Pos + 1;          // CW
   }
   else {
     encoder0Pos = encoder0Pos - 1;          // CCW
   }
 }
}

Now the motor won't move. The serial output is like this:

First part Done
Encoder value after part 2: 0
Second Part Done
Encoder value after part 2: 0
First part Done
Encoder value after part 2: 0
Second Part Done
Encoder value after part 2: 0
First part Done
Encoder value after part 2: 0
Second Part Done
Encoder value after part 2: 0
First part Done
Encoder value after part 2: 0
Second Part Done

Does it spin with your original code?

You can see all I added was a few lines after each stop to grab the encoder count and serial-print it.

The motor PWM stuff hasn't changed.

If the encoder uses Hall effect sensors with NPN open collector outputs, you may need:

  pinMode(encoder0PinA, INPUT_PULLUP);
  pinMode(encoder0PinB, INPUT_PULLUP);

Can you turn the motor shaft by hand?