using an interrupt to get rpm

I've seem to have run into a wall when trying to get the RPM of a DC Motor to use for PID Control.
I've butchered my own code so I'm going to start fresh with examples of just the rpm code so it's easier to understand.

when using this code to get the rpm, Because of the 1 second delay the PID code is only getting a rpm update every second which is making my motor pulse up and down and it never finds the setpoint, I change this to a very low number and it doesn't matter, the delay just gets shorter but it doesn't help. If I remove the delay, then the rpm doesn't update at all. I assume it should, just with no delay? but I guess not.

 void loop()
 {
   //Update RPM every second
   delay(1000);
   //Don't process interrupts during calculations
   detachInterrupt(0);
   rpm = 30*1000/(millis() - timeold)*rpmcount;
   timeold = millis();
   rpmcount = 0;
   attachInterrupt(0, rpm_fun, FALLING);
  }

This code which is basically the same as above but instead of a delay it has a if statement. It updates the RPM every 20 pulses and does not go back to 0 if the rpm input stops, which is a issue. Because if the PID overshoots and the motor turns off, the rpm will never come back down and it will never start again.

 void loop()
 {
//Update RPM every 20 counts
if (rpm >= 20) { 
   //Don't process interrupts during calculations
   detachInterrupt(0);
     rpm = 30*1000/(millis() - timeold)*rpmcount;
     timeold = millis();
     rpmcount = 0;
attachInterrupt(0, rpm_fun, FALLING);
}

I can't figure this out. I basically need the rpm to always be updated with no delay (so the first block of code is out) but the second block of code won't go back to 0 if the input stops, it just stays at the last rpm it was at.
I might be going at this the wrong way, I don't know. Someone please put me in the right direction. If it helps I'm using the PID library and the processing front end to monitor what's going on with this.

Large latency in the feedback is always bad news. I think you'll have to tune the PID.

It would help if you showed all the code the problem might be lurking in the bits you don't post. Like you could have a print statement in the ISR.

with no delay? but I guess not.

It depends on what you meant by "with no delay".

Since you are counting periods / pulses, there are necessarily delays - you are measuring two points in time. and you cannot do so without any elapse of time (aka delays).

You can, however, measure it quickly and transparently so that there appears to be no delay.

For example, in your 2nd approach (counting pulses), you can implement a time-out; you can use an intermediate variable to keep pulse cont and dump it to an output variable only when certain conditions are met (enough pulses have arrived or enough time has elapsed).

compubob:
I've seem to have run into a wall when trying to get the RPM of a DC Motor to use for PID Control.
I've butchered my own code so I'm going to start fresh with examples of just the rpm code so it's easier to understand.

It would help if you posted all of the relevant code. For example, what is in your ISR? I can assume that it just increments a counter, but I don't know. And I don't know what type rpmcount is, if it was volatile or static, if you handle roll-overs, or if it gets reset in more than 1 place in the code. It is helpful to post the smallest code set that shows the problem, but post all of it.

when using this code to get the rpm, Because of the 1 second delay the PID code is only getting a rpm update every second which is making my motor pulse up and down and it never finds the setpoint, I change this to a very low number and it doesn't matter, the delay just gets shorter but it doesn't help. If I remove the delay, then the rpm doesn't update at all. I assume it should, just with no delay? but I guess not.

 void loop()

{
  //Update RPM every second
  delay(1000);
  //Don't process interrupts during calculations
  detachInterrupt(0);
  rpm = 30*1000/(millis() - timeold)*rpmcount;
  timeold = millis();
  rpmcount = 0;
  attachInterrupt(0, rpm_fun, FALLING);
 }

I can see why removing the delay is causing you grief. Without it, it is possible that the delta t (millis() - timeold) is 1 or even 0. Also, without the delay, most of your loop is spent with the ISR off, and you miss a bunch of interrupts. You need to design the code so that it doesn't need the delay.

This code which is basically the same as above but instead of a delay it has a if statement. It updates the RPM every 20 pulses and does not go back to 0 if the rpm input stops, which is a issue. Because if the PID overshoots and the motor turns off, the rpm will never come back down and it will never start again.

 void loop()

{
//Update RPM every 20 counts
if (rpm >= 20) {
  //Don't process interrupts during calculations
  detachInterrupt(0);
    rpm = 30*1000/(millis() - timeold)*rpmcount;
    timeold = millis();
    rpmcount = 0;
attachInterrupt(0, rpm_fun, FALLING);
}

Once again, delta t could be 0 or 1, which would make the rpm calculation bad. Also
instead of if (rpm >= 20) { shouldn't it be if (rpmcount >= 20) {?
If rpm ever goes below 20, it will never change again. That's not what you wanted, is it?

I can't figure this out. I basically need the rpm to always be updated with no delay (so the first block of code is out) but the second block of code won't go back to 0 if the input stops, it just stays at the last rpm it was at.
I might be going at this the wrong way, I don't know. Someone please put me in the right direction. If it helps I'm using the PID library and the processing front end to monitor what's going on with this.

Posting the whole actual code would help more.

OK, pointers. First of all, don't detach and re-attach the ISR. You miss all the pulses while the ISR is detached. If you are careful about how to read the rpmcount, you can just leave the interrupts on all the time. Second, what is the type of rpmcount? It should be volatile unsigned long.
I'm not sure about your calculation of rpm. If delta t is 0 the sketch will crash. If it is 1 then you get back 30,000 * rpmcount. So if you read 20 pulses in about 1 millisecond, your calculated rpm is 600,000. Does that sound right to you? OTOH, if delta t is large, then 30K/delta t may be 1 or 0. This calculation also loses a lot of precision by the order of operations. I'd go back and re-think the rpm calculation. You also don't mention how fast the motor is or the resolution of your encoder. Too many interrupts coming too fast can overwhelm the arduino, and your rpmcount gets inaccurate.

Back to basics. To calculate the speed, you need to know the number of pulses and the elapsed time, and the resolution of the encoder. P/RT = speed. There are limits though. if T is too small then your calculation is not accurate, or will hit /0 errors. If you don't have any pulses, then speed will be 0 no matter what T is (if you get 0/0 then the actual speed could be anything). You need to take this into account and watch out for the corner cases. Generally, there are two approaches. 1) measure the amount of time it takes to get a fixed number of pulses, or 2) count the pulses during a fixed interval of time. There's actually a third approach where you measure both arbitrary time and pulses and divide, but as I mentioned, there are lots of corner cases. You seem to have tried all 3, but not quite correctly ;).

Just off the top of my head (not tested or even compiled), just to give an idea.

#define INTERVAL 200   // a fifth of a second. Picked arbitrarily.
#define RESOLUTION 20  // 20 pulses per revolution.
volatile unsigned long rpmcount = 0;
unsigned long oldtime;

void loop()
{
    unsigned long deltaT ;
    unsigned long pulses;

    deltaT = millis() - oldtime;
    pulses = rpmcount;

    if (deltaT > INTERVAL) {
       rpm = (pulses * 60 * 1000) / (RESOLUTION * deltaT);  // order of ops is important.
       oldtime = millis();
       rpmcount = 0;
    }
}

Some things to think about. Is millisecond resolution fine enough? There are arduino functions for microseconds, too. Is RPM the measurement you want to use? RPS or PPS or even PPmS could be used. If you are still not getting smooth movement from your PID algorithm, you might need to tune it. Tuning PID is tricky.

HTH,

Thank you for taking the time to explain all of this. It's really helpful and now I understand it a bit more.

currently my motor is 1 pulse per rotation for testing, it's just a thick band of tape on the shaft and 50% of it is painted silver and the other 50% of it is black so the optical ir reflective sensor can read it I hope that the resolution is not too low. I've been playing with the PID tuning settings for a while and it seems that most of my issues are leading back to the way it's getting the rpm.

the reason I didn't post all of my code it because it's messed up and I'm embarrassed.. but here it is, it might be broken in it's current state.

/******************************************************************
 * PID Simple Example (Augmented with Processing.org Communication)
 * Version 0.3
 * by Brett Beauregard
 * License: Creative-Commons Attribution Share-Alike
 * April 2011
 ******************************************************************/

#include <PID_v1.h>

//Define Variables we'll be connecting to
double Setpoint, Input, Output;
int setpointPin=0, inputPin=1, outputPin=13;

//Specify the links and initial tuning parameters
PID myPID(&Input, &Output, &Setpoint,10,40,0, DIRECT);
 
 
 volatile byte rpmcount;

 unsigned int rpm;

 unsigned long timeold;


unsigned long serialTime; //this will help us know when to talk with processing

void setup()
{
  //initialize the serial link with processing
  Serial.begin(9600);
  attachInterrupt(0, rpm_fun, RISING);
     rpmcount = 0;
   rpm = 0;
   timeold = 0;
  //initialize the variables we're linked to
  Input = (rpm);
  Setpoint = analogRead(setpointPin);

  //turn the PID on
  myPID.SetMode(AUTOMATIC);
}

void loop()
{

  

  
  //send-receive with processing if it's time
  if(millis()>serialTime)
  {
    SerialReceive();
    SerialSend();
    serialTime+=500;
  }
     if (rpmcount >= 20) { 
     //Update RPM every 20 counts, increase this for better RPM resolution,
     //decrease for faster update
     rpm = 30*1000/(millis() - timeold)*rpmcount;
     timeold = millis();
     rpmcount = 0;
  //pid-related code
  Setpoint = analogRead(setpointPin);
  myPID.Compute();
  analogWrite(outputPin,Output);
  
   }
 }

 void rpm_fun()
 {
   rpmcount++;
   //Each rotation, this interrupt function is run twice
 }



/********************************************
 * Serial Communication functions / helpers
 ********************************************/


union {                // This Data structure lets
  byte asBytes[24];    // us take the byte array
  float asFloat[6];    // sent from processing and
}                      // easily convert it to a
foo;                   // float array



// getting float values from processing into the arduino
// was no small task.  the way this program does it is
// as follows:
//  * a float takes up 4 bytes.  in processing, convert
//    the array of floats we want to send, into an array
//    of bytes.
//  * send the bytes to the arduino
//  * use a data structure known as a union to convert
//    the array of bytes back into an array of floats

//  the bytes coming from the arduino follow the following
//  format:
//  0: 0=Manual, 1=Auto, else = ? error ?
//  1: 0=Direct, 1=Reverse, else = ? error ?
//  2-5: float setpoint
//  6-9: float input
//  10-13: float output  
//  14-17: float P_Param
//  18-21: float I_Param
//  22-245: float D_Param
void SerialReceive()
{

  // read the bytes sent from Processing
  int index=0;
  byte Auto_Man = -1;
  byte Direct_Reverse = -1;
  while(Serial.available()&&index<26)
  {
    if(index==0) Auto_Man = Serial.read();
    else if(index==1) Direct_Reverse = Serial.read();
    else foo.asBytes[index-2] = Serial.read();
    index++;
  } 
  
  // if the information we got was in the correct format, 
  // read it into the system
  if(index==26  && (Auto_Man==0 || Auto_Man==1)&& (Direct_Reverse==0 || Direct_Reverse==1))
  {
    Setpoint=double(foo.asFloat[0]);
    //Input=double(foo.asFloat[1]);       // * the user has the ability to send the 
                                          //   value of "Input"  in most cases (as 
                                          //   in this one) this is not needed.
    if(Auto_Man==0)                       // * only change the output if we are in 
    {                                     //   manual mode.  otherwise we'll get an
      Output=double(foo.asFloat[2]);      //   output blip, then the controller will 
    }                                     //   overwrite.
    
    double p, i, d;                       // * read in and set the controller tunings
    p = double(foo.asFloat[3]);           //
    i = double(foo.asFloat[4]);           //
    d = double(foo.asFloat[5]);           //
    myPID.SetTunings(p, i, d);            //
    
    if(Auto_Man==0) myPID.SetMode(MANUAL);// * set the controller mode
    else myPID.SetMode(AUTOMATIC);             //
    
    if(Direct_Reverse==0) myPID.SetControllerDirection(DIRECT);// * set the controller Direction
    else myPID.SetControllerDirection(REVERSE);          //
  }
  Serial.flush();                         // * clear any random data from the serial buffer
}

// unlike our tiny microprocessor, the processing ap
// has no problem converting strings into floats, so
// we can just send strings.  much easier than getting
// floats from processing to here no?
void SerialSend()
{
  Serial.print("PID ");
  Serial.print(Setpoint);   
  Serial.print(" ");
  Serial.print(Input);   
  Serial.print(" ");
  Serial.print(Output);   
  Serial.print(" ");
  Serial.print(myPID.GetKp());   
  Serial.print(" ");
  Serial.print(myPID.GetKi());   
  Serial.print(" ");
  Serial.print(myPID.GetKd());   
  Serial.print(" ");
  if(myPID.GetMode()==AUTOMATIC) Serial.print("Automatic");
  else Serial.print("Manual");  
  Serial.print(" ");
  if(myPID.GetDirection()==DIRECT) Serial.println("Direct");
  else Serial.println("Reverse");
}

Well you're right about it being broken ;). For starters, you're never updating Input within loop().

You shouldn't be updating your RPM measurement every 20 counts. You're trying to smooth out the readings for the current RPM but you should leave that work to the PID library. If you're having problems with your optical sensing then you need to fix that or you will never get reliable control.

I would suggest that you simplify by setting Input to the time (milliseconds) between interrupts. Also adjust the value read from analogRead(setpointPin) to units that are equivalent to the milliseconds you want per revolution. Remember that the PID library tries to match the value of Input to Setpoint and the units for each variable must be equivalent.

It would also be helpful to know what range of speeds (what RPM will be typical) you're trying to achieve.

Chagrin:
Well you're right about it being broken ;). For starters, you're never updating Input within loop().

You shouldn't be updating your RPM measurement every 20 counts. You're trying to smooth out the readings for the current RPM but you should leave that work to the PID library. If you're having problems with your optical sensing then you need to fix that or you will never get reliable control.

I would suggest that you simplify by setting Input to the time (milliseconds) between interrupts. Also adjust the value read from analogRead(setpointPin) to units that are equivalent to the milliseconds you want per revolution. Remember that the PID library tries to match the value of Input to Setpoint and the units for each variable must be equivalent.

It would also be helpful to know what range of speeds (what RPM will be typical) you're trying to achieve.

between 0-4000rpm. Thanks, when I get time I will redo and post the new code. I was just jumbling things around for testing and it ended up like this.. lol

I redid everything and I'm now using the TimedAction library to check for the RPM every 100ms and apply the correct math to it in the TimedAction loop. As far as I can see it's working perfectly and the PID follows nearly perfect. I've even added another pulse per revolution on my wheel and it smoothed things out, I will add a few more when I get down to making the final product. I still have to add another pulse counter for the setpoint so I hope it will be able to handle two interrupts without much slowdown.
Also I did find that I was getting a lot of interference from the motor and it was causing false rpm readings.. I found that using the same GND for the power supply that's powering the motor that I was also using for the IR sensor causes this and it has to be moved to the other gnd pin.
Thank you everyone for your advice, even though I did end up using another method, But it is appreciated :slight_smile: