Optical Encoder issue

Hello,
I have been working on finding the RPM of a motor using an optical encoder. The code has been written, but I have found one main issue. The RPM of the motor is not constant enough to be a proper number. It can be at times anywhere from the hundreds, to one, all while moving at a constant speed. The output does appear to decrease over time also. My code is posted below.

Code:

int tick = 0;
int oldIn = 0;
int currentIn = 0;
float mins = 0;
float timeMillis = 0;
float oldTimeMillis = 0;
float rpmReturn = 0;
float rpmPrint;
void setup() {
  // put your setup code here, to run once:
  // attachInterrupt(digitalPinToInterrupt(2), readRPM, CHANGE);
  pinMode(2, INPUT);
   Serial.begin(9600);
}

void loop() {
  currentIn = digitalRead(2);
  if(currentIn != oldIn){
    tick++;
  }
  
  timeMillis = millis();
  if (tick >= 360){
    tick = 0;
    rpmPrint = rpm();
    Serial.println(rpmPrint);
    oldTimeMillis = timeMillis;
  }
  oldIn = currentIn;
}
void readRPM(){
  tick++;
}
float rpm() {
  timeMillis = timeMillis - oldTimeMillis;
  mins = (timeMillis / 1000) / 60;
  rpmReturn = 1/mins;
  return rpmReturn;
}

We tried changing from an interrupt to just putting the If Change into the code, but there was no change. Does anyone know of a way to stabilize this output?

As you haven't told us the number of steps per revolution there are in your optical encoder and the usual speed of the motor, it's difficult to tell what's happening. It looks like your interrupts are coming in faster than the program can handle them. Instead of calculating the RPM for every tick change, count (say) 100 of them and calculate and print an average speed over 100 ticks.

Here is a debugging modification of your code.
It emulates relatively constant speed if your code between timemillis() AND oldTimeMillis() is constant, without unpredictable, but increasing as you output more characters, Serial output time.
It run around 7.4 rpmPrint on Due
Not sure if that is enough precision on your processor.
So in an essence - do the calculations first than the Serial prints after.

[code]
int tick = 0;
int oldIn = 0;
int currentIn = 0;
float mins = 0;
float timeMillis = 0;
float oldTimeMillis = 0;
float rpmReturn = 0;
float rpmPrint;
void setup() {
  // put your setup code here, to run once:
  // attachInterrupt(digitalPinToInterrupt(2), readRPM, CHANGE);
  pinMode(2, INPUT);
  Serial.begin(115200);
}

void loop() {
  //currentIn = digitalRead(2);
  currentIn = ~currentIn; 
  //delay(100);                 just to slow things down for observation 
  Serial.println(currentIn); 
  if (currentIn != oldIn) {
    tick++;
  }
  //timeMillis = millis(); you do not need current time  here 
  if (tick >= 360) {
     timeMillis = millis(); but here 
    tick = 0;
    rpmPrint = rpm();
  
    oldTimeMillis = timeMillis;

  Serial.println(rpmPrint);

    //delay(2000);         if test delay here the rmp Serial needs to be adjusted 
    }
  oldIn = currentIn;
}
void readRPM() {
  tick++;
}
float rpm() {
  timeMillis = timeMillis - oldTimeMillis;
   mins = (timeMillis / 1000) / 60;
  rpmReturn = 1 / mins;

Serial.println( timeMillis); I forgot check if this needs to be removed / moved also  
  return rpmReturn;
}

[/code]

Do you have the specifications on your optical encoder and recommended wiring diagrams? Often theses encoders require external pull up resistors.

Can you verify that you get a clean 0/1 digital read from the encoder if you turn the motor slowly by hand?.

I have looked closer at your code, and find a few problems. I still question whether or not you are getting a clean signal, but here are some things to fix in the code.

  1. When you set oldTimeMillis = timeMillis the value of timeMillis has been modified in the float rpm() function.
    It really needs to be set to the current time.

  2. While your program will work with timeMillis and oldTimeMillis as floats, it is better practice to make them unsigned long for consistency with millis(). You then need to modify your constants in the rpm function to force the math for mins to a float.

I have tested your code by incrementing tick ever 10 ms and it reads a constant 16.65 rpm as it should with the changes I have made.

int tick = 0;
int oldIn = 0;
int currentIn = 0;
float mins = 0;
//float timeMillis = 0;
//float oldTimeMillis = 0;
unsigned long timeMillis = 0;
unsigned long oldTimeMillis = 0;
float rpmReturn = 0;
float rpmPrint;
void setup() {
  // put your setup code here, to run once:
  // attachInterrupt(digitalPinToInterrupt(2), readRPM, CHANGE);
  pinMode(2, INPUT);
   Serial.begin(9600);
}

void loop() {
  //currentIn = digitalRead(2);
  //if(currentIn != oldIn){
    tick++;
  //}
  delay(10);//approx 100 ticks/second = 3.6 sec/rev = 16.67 rpm
  
  timeMillis = millis();
  if (tick >= 360){
    tick = 0;
    rpmPrint = rpm();
    Serial.println(rpmPrint);
    //oldTimeMillis = timeMillis;//this value was modified in function
    oldTimeMillis = millis();
  }
  oldIn = currentIn;
}
void readRPM(){
  tick++;
}
float rpm() {
  timeMillis = timeMillis - oldTimeMillis;
  mins = (timeMillis / 1000.0) / 60.0; //force constants to float
  rpmReturn = 1/mins;
  return rpmReturn;
}

We found are issue. We are using motors that can turn at 6800 RPM, and at those speeds, it easily outpaces the Arduino's analog read. We are now trying new encoders that lower it from 360 ticks per rotation to a possible 3 ticks. Thanks for all of the replies!

Ezrai:
We found are issue. We are using motors that can turn at 6800 RPM, and at those speeds, it easily outpaces the Arduino's analog read. We are now trying new encoders that lower it from 360 ticks per rotation to a possible 3 ticks. Thanks for all of the replies!

See reply #1. :angry: