Attachinterrupt too slow

Hi,

I am currently working on a project where I will need to use an encoder. (HN3806-AB-600N) I think there is a problem with the attachinterrupts. This is the code that I am using. If I rotate the shaft REALLY slowly it works fine. However, when I try to rotate it faster (still very slowly) it does not count the pulses at all. I belive that the encoder is in perfect condition.

Do you have any suggestions?

volatile long temp, counter = 0; //This variable will increase or decrease depending on the rotation of encoder
    
void setup() {
  Serial.begin (9600);
  pinMode(2, INPUT_PULLUP); // internal pullup input pin 2 
  
  pinMode(3, INPUT_PULLUP); // internalเป็น pullup input pin 3
   //Setting up interrupt
  //A rising pulse from encodenren activated ai0(). AttachInterrupt 0 is DigitalPin nr 2 on moust Arduino.
  attachInterrupt(0, ai0, RISING);
   
  //B rising pulse from encodenren activated ai1(). AttachInterrupt 1 is DigitalPin nr 3 on moust Arduino.
  attachInterrupt(1, ai1, RISING);
  }
   
  void loop() {
  // Send the value of counter
  if( counter != temp ){
  Serial.println (counter);
  temp = counter;
  }
  }
   
  void ai0() {
  // ai0 is activated if DigitalPin nr 2 is going from LOW to HIGH
  // Check pin 3 to determine the direction
  if(digitalRead(3)==LOW) {
  counter++;
  }else{
  counter--;
  }
  }
   
  void ai1() {
  // ai0 is activated if DigitalPin nr 3 is going from LOW to HIGH
  // Check with pin 2 to determine the direction
  if(digitalRead(2)==LOW) {
  counter--;
  }else{
  counter++;
  }
  }

Thanks,
Anton

I think your topic title is misleading.

“counter” should be volatile, but not “temp”

A shared long should only be read with interrupts disabled.

Hi,

Thank you for your prompt reply!
I have changed temp so thats it not volatile anymore, however I do not understand what you mean by reading a shared long.
Sorry if my questions are trivial. I am very new to this.

Also, if you want it to be as fast as possible, use Direct Port Reads instead of digitialRead().

This is my updated code. It still functions the same way. :frowning:

#define pinOfPin(P)\
 (((P)>=0&&(P)<8)?&PIND:(((P)>7&&(P)<14)?&PINB:&PINC))
#define pinIndex(P)((uint8_t)(P>13?P-14:P&7))
#define pinMask(P)((uint8_t)(1<<pinIndex(P)))
#define isLow(P)((*(pinOfPin(P))& pinMask(P))==0)

volatile unsigned int counter = 0; //This variable will increase or decrease depending on the rotation of encoder
unsigned int temp;
   
void setup() {
 Serial.begin (9600);
 pinMode(2, INPUT_PULLUP); // internal pullup input pin 2 
 
 pinMode(3, INPUT_PULLUP); // internalเป็น pullup input pin 3
  //Setting up interrupt
 //A rising pulse from encodenren activated ai0(). AttachInterrupt 0 is DigitalPin nr 2 on moust Arduino.
 attachInterrupt(0, ai0, RISING);
  
 //B rising pulse from encodenren activated ai1(). AttachInterrupt 1 is DigitalPin nr 3 on moust Arduino.
 attachInterrupt(1, ai1, RISING);
 }
  
 void loop() {
 // Send the value of counter
 if( counter != temp ){
 Serial.println (counter);
 temp = counter;
 }

 }
  
 void ai0() {
 // ai0 is activated if DigitalPin nr 2 is going from LOW to HIGH
 // Check pin 3 to determine the direction
 if(isLow(3)) {
 counter++;
 }else{
 counter--;
 }
 }
  
 void ai1() {
 // ai0 is activated if DigitalPin nr 3 is going from LOW to HIGH
 // Check with pin 2 to determine the direction
 if(isLow(2)) {
 counter--;
 }else{
 counter++;
 }
 }
 void loop() 
{
  noInterrupts ();
  long copyCounter = counter;
  interrupts ();
  if( copyCounter != temp ){
    Serial.println (counter);
    temp = copyCounter;
  }
}

After implementing it it is still the same. :frowning:

long temp;
volatile long counter;

ISR (INT0_vect) {
  (PIND3) ? counter-- : counter++;
}

ISR (INT1_vect) {
  (PIND2) ? counter-- : counter++;
}

void setup() {
  Serial.begin (9600);
  pinMode(2, INPUT_PULLUP);
  pinMode(3, INPUT_PULLUP);
  cli();//prohibit interrupts
  EICRA |= B00001111;// set int0 and int1 to trigger on rising edge
  EIMSK |= B00000011;// set pin 2 and pin 2 to int0 and in1 function
  sei();//allow interrupts
}

void loop() {
  long currentCount = readCounter();
  if ( currentCount != temp ) {
    Serial.println (currentCount);
    temp = currentCount;
  }
}

long readCounter() {
  cli();//prohibit interrupts while we read counter. Interrupts still cue up to 64.
  long c = counter;
  sei();//allow interrupts
  return c;
}

A few problems with your code.

  1. function calls are too slow
  2. volatile longs are not atomic and must be read with interrupts disabled into a non-volatile variable.
  3. spinning it fast will prevent the serial port from keeping up at 9600 baud... or any baud rate. It would be better to print with some interval of time instead of when !=temp...
void loop() {
  long currentCount = readCounter();
  Serial.println (currentCount);
  delay(100);
}

Thank you for the code! After testing it I found that only works under 10rpm.

haasanton:
Thank you for the code! After testing it I found that only works under 10rpm.

This won't print as often as every rpm, but it will count revolutions.

volatile long counter;
unsigned long printTimestamp;

ISR (INT0_vect) {
  (PIND3) ? counter-- : counter++;
}

ISR (INT1_vect) {
  (PIND2) ? counter-- : counter++;
}

void setup() {
  Serial.begin (9600);
  pinMode(2, INPUT_PULLUP);
  pinMode(3, INPUT_PULLUP);
  cli();//prohibit interrupts
  EICRA |= B00001111;// set int0 and int1 to trigger on rising edge
  EIMSK |= B00000011;// set pin 2 and pin 2 to int0 and in1 function
  sei();//allow interrupts
  printTimestamp = millis();
}

void loop() {
  if (millis() - printTimestamp >= 100UL) {
    printTimestamp += 100UL;
    long currentCount = readCounter();
    Serial.println (currentCount);
  }
}

long readCounter() {
  cli();//prohibit interrupts while we read counter. Interrupts still cue up to 64.
  long c = counter;
  sei();//allow interrupts
  return c;
}

Still only works when turned slowly. Does this imply that somethings wrong with the encoder?

haasanton:
Thank you for the code! After testing it I found that only works under 10rpm.

Please post the complete program for your latest version so we can see exactly what you have done.

I have successfully used an interrupt to monitor the speed of a small DC motor running at up to 16,000 RPM (about 260 pulses per second) (and the code would probably have been fine for higher speeds). As well as detecting the speed the Atmega 328 was operating a PID calculation to manage the speed and receiving wireless data to inform it about the required speed.

…R

This is the latest version.

volatile long counter;
unsigned long printTimestamp;

ISR (INT0_vect) {
  (PIND3) ? counter-- : counter++;
}

ISR (INT1_vect) {
  (PIND2) ? counter++ : counter--;
}

void setup() {
  Serial.begin (9600);
  pinMode(2, INPUT_PULLUP);
  pinMode(3, INPUT_PULLUP);
  cli();//prohibit interrupts
  EICRA |= B00001111;// set int0 and int1 to trigger on rising edge
  EIMSK |= B00000011;// set pin 2 and pin 2 to int0 and in1 function
  sei();//allow interrupts
  printTimestamp = millis();
}

void loop() {
  long currentCount = readCounter();
  if ( currentCount != temp ) {
    Serial.println (currentCount);
    temp = currentCount;
  }
}

long readCounter() {
  cli();//prohibit interrupts while we read counter. Interrupts still cue up to 64.
  long c = counter;
  sei();//allow interrupts
  return c;
}

You are still trying to print out something every pulse. And you are still trying to use a very slow baud rate. This is where all the time is going.

Even with using this code it still is not fast enough. Shall I change baud rate even higher?

volatile long counter;
unsigned long printTimestamp;

ISR (INT0_vect) {
  (PIND3) ? counter-- : counter++;
}

ISR (INT1_vect) {
  (PIND2) ? counter-- : counter++;
}

void setup() {
  Serial.begin (115200);
  pinMode(2, INPUT_PULLUP);
  pinMode(3, INPUT_PULLUP);
  cli();//prohibit interrupts
  EICRA |= B00001111;// set int0 and int1 to trigger on rising edge
  EIMSK |= B00000011;// set pin 2 and pin 2 to int0 and in1 function
  sei();//allow interrupts
  printTimestamp = millis();
}

void loop() {
  if (millis() - printTimestamp >= 100UL) {
    printTimestamp += 100UL;
    long currentCount = readCounter();
    Serial.println (currentCount);
  }
}

long readCounter() {
  cli();//prohibit interrupts while we read counter. Interrupts still cue up to 64.
  long c = counter;
  sei();//allow interrupts
  return c;
}

You can’t do this safely:

long readCounter() {
  cli();//prohibit interrupts while we read counter. Interrupts still cue up to 64.
  long c = counter;
  sei();//allow interrupts
  return c;
}

Variable ‘c’ is dynamic, the value is undefined after returning from the function. If it returns a valid value, it’s just good luck. At least, you have to make it static.

haasanton:
Still only works when turned slowly. Does this imply that somethings wrong with the encoder?

You need to explain what you mean by only works when.... There could be a lot of things wrong. I have no clue why you are reading the other interrupt pin inside your interrupt. Upload some pictures and give us the serial output. Explain your expectation and your logic behind your code. If could easily identify some mistakes when coding for the first time, especially with interrupts. Do you NEED to work with interrupts? would polling work just as well? The code I posted speeds the response, by eliminating the function calls and working with direct ports. It also addresses the atomic value of counter, And, it puts less tax on the UART. If you are Serial.println()-ing a long variable at 9600 baud, it can take up to 14.583 milliseconds to complete a single print. So, being interval based, instead of !=temp based, it will only print 10 times a second(at 100UL as the interval). But, if your interrupt code is working, the number should reflect an accurate count. if the count isn't accurate, maybe the code inside your interrupt service functions isn't doing what you expect it should do.

aarg:
You can't do this safely:

long readCounter() {

cli();//prohibit interrupts while we read counter. Interrupts still cue up to 64.
 long c = counter;
 sei();//allow interrupts
 return c;
}




Variable 'c' is dynamic, the value is undefined after returning from the function. If it returns a valid value, it's just good luck. At least, you have to make it static.

wrong, have you taken a look at the millis() function in wiring.c

aarg:
You can’t do this safely:

long readCounter() {

cli();//prohibit interrupts while we read counter. Interrupts still cue up to 64.
long c = counter;
sei();//allow interrupts
return c;
}




Variable 'c' is dynamic, the value is undefined after returning from the function. If it returns a valid value, it's just good luck. At least, you have to make it static.

Nope.

aarg:
You can't do this safely:

long readCounter() {

cli();//prohibit interrupts while we read counter. Interrupts still cue up to 64.
 long c = counter;
 sei();//allow interrupts
 return c;
}




Variable 'c' is dynamic, the value is undefined after returning from the function. If it returns a valid value, it's just good luck. At least, you have to make it static.

It's fine. C / C++ returns by value (i.e. a copy). It would be a problem if OP was trying to return a pointer to 'c'.