Help accurately measuring motor speed

Hello,
I recently posted on these forums looking for help finding a motors speed using a optical interrupter. I did receive some amazing help and learned a lot in the process! However I now have 2 versions of code that I have written up to find the speed and rpm, and I am unsatisfied with the output of both!

the first code here was the one I initially wrote. It essentially counts how many times the spokes on a slotted disk pass by in a stated amount of time. Using that data it then easily calculates speed and rpm. My dilemma is the rpm output of this code seems to rise by around 4-6 so it is not very accurate. I fear this may be because of my low resolution disk (only 20 slots), but was wondering if possibly there is something wrong in the code.

#include <LiquidCrystal.h>



//................................................all consants used.........................
const int spokes = 20.0;    // number of spokes on reading sproket
float wheelDiameter = 10.0; //wheel diameter in cm
const int interval = 500;            // time, in millisec between readings
//.................................................................................
float wheelSpeed = 0;          // speed wheel is rotating
volatile int rotations = 0;             // number of sprokets read
long previousMillis = 0;        // will store last time rotaions was updated
//LiquidCrystal lcd(RS, E, D4, D5, D6, D7);
LiquidCrystal lcd(12, 11, 10, 9, 8, 7);

void setup() {
  Serial.begin(9600); 
  attachInterrupt(0, sproket, FALLING); //sensor input tied to pin 2
  lcd.begin(20, 4);
  lcd.display();
}

void loop()
{
//......................Calculate rpm/speed and send over serial/lcd...............
  unsigned long currentMillis = millis();
 
  if(currentMillis - previousMillis > interval) 
  {
    previousMillis = currentMillis;   
    wheelSpeed =  ((  (float)rotations / spokes) * 3.14 * wheelDiameter) * (1000.0/interval);
   Serial.print(wheelSpeed);
   Serial.println(" cm/s");
   Serial.print(((float)rotations / spokes)*60.0*(1000.0/interval));
   Serial.println(" rpm");
   lcd.setCursor(0, 3);
   lcd.print(wheelSpeed);
   lcd.setCursor(4, 3);
   lcd.print("cm/s");
   lcd.setCursor(11, 3);
   lcd.print(((float)rotations / spokes)*60.0*(1000.0/interval));
   lcd.setCursor(15, 3);
   lcd.print("rpm");
   rotations = 0;
  }
}
//........................ Monitor number of spokes passing by....................
void sproket()
{
  rotations += 1;
}]

This second code I wrote is very similar to the first, but instead it measures the time in-between spokes on the wheel and calculates the speed and rpm based of that. Although I think this code is much more accurate, with a constant spinning motor the output will rise from 50rpm to 120rpm in a matter of 500millisec. Making the output unusable for any practical purpose.

//.....................................libraries....................................
#include <LiquidCrystal.h>



//................................................all consants used.........................
const int spokes = 20.0;    // number of spokes on reading sproket
float wheelDiameter = 10.0; //wheel diameter in cm
const int interval = 1000;            // time, in millisec between readings
//.................................................................................
float wheelRpm = 0;          // speed wheel is rotating
volatile int time = 0;             // number of sprokets read
long previousMillis = 0;        // will store last time rotaions was updated
long previousTime = 0;          
long currentTime = 0;           // all 3 of these are used for calculating speed
long clockedTime = 0;
//.....................................................................................


//LiquidCrystal lcd(RS, E, D4, D5, D6, D7);
LiquidCrystal lcd(12, 11, 10, 9, 8, 7);

void setup() {
  Serial.begin(9600); 
  attachInterrupt(0, sproket, FALLING); //sensor input tied to pin 2
  lcd.begin(20, 4);                     //setup lcd
  lcd.display();
}

void loop()
{
//......................Calculate rpm/speed and send over serial/lcd...............
  unsigned long currentMillis = millis();
 currentTime = millis();
 
  if(currentMillis - previousMillis > interval)                        // only updateing output every so often
  {
    previousMillis = currentMillis;
    if (clockedTime != previousTime)                                  // testing to see if wheel is moving
    {
   Serial.print(((float)wheelRpm / 60) * 3.14 * wheelDiameter);            // outputs
   Serial.println(" cm/s");
   Serial.print(wheelRpm);
   Serial.println(" rpm");
   lcd.setCursor(0, 3);
   lcd.print(((float)wheelRpm / 60) * 3.14 * wheelDiameter);
   lcd.setCursor(4, 3);
   lcd.print("cm/s");
   lcd.setCursor(11, 3);
   lcd.print(wheelRpm);
   lcd.setCursor(15, 3);
   lcd.print("rpm");
   clockedTime = previousTime;
    }
    else
    {
   Serial.print("0.00");                                                   // outputs
   Serial.println(" cm/s");
   Serial.print("0.00");
   Serial.println(" rpm");
   lcd.setCursor(0, 3);
   lcd.print("0.00");
   lcd.setCursor(4, 3);
   lcd.print("cm/s");
   lcd.setCursor(11, 3);
   lcd.print("0.00");
   lcd.setCursor(15, 3);
   lcd.print("rpm");
    }
  }
}
//........................ Monitor number of spokes passing by....................

void sproket()
{
  time = currentTime - previousTime;                      // finding time between spokes
  previousTime = currentTime;
  wheelRpm = ((float)60.0 / (time * spokes * .001));      //  calculating rpm's
}

Any help would be greatly appreciated! If the mistake(s) are something simple I do apologize, this is my first time learning to program in any language and I started teaching myself about a week ago. Thanks

start with

volatile int rotations = 0; ==> volatile unsigned long rotations = 0;

the int will overflow after only 32K pulses ~1600 rotations.

robtillaart: start with

volatile int rotations = 0; ==> volatile unsigned long rotations = 0;

the int will overflow after only 32K pulses ~1600 rotations.

correct me if i'm wrong, but the only way that would become a problem is if the motor were spinning at 3200 rpm (it should be around 60rpm max).... considering 'rotations' is reset to 0 every 500millisec.

void sproket()
{
  time = currentTime - previousTime;                      // finding time between spokes
  previousTime = currentTime;
  wheelRpm = ((float)60.0 / (time * spokes * .001));      //  calculating rpm's
}

Ah, aren't you better off finding the currentTime right now? When it happens? And I would make time unsigned long - either the compiler will do it anyway internally, or it will get it wrong.

You don't need to cast 60.0 to float - what else could it be?

Also make wheelRpm volatile as it is calculated in an ISR.

[quote author=Nick Gammon link=topic=94648.msg710691#msg710691 date=1330669535]

Ah, aren't you better off finding the currentTime right now? When it happens? And I would make time unsigned long - either the compiler will do it anyway internally, or it will get it wrong.

You don't need to cast 60.0 to float - what else could it be?

Also make wheelRpm volatile as it is calculated in an ISR. [/quote]

Made the changes you suggested and the difference is incredible! the rpm only fluctuate by about .5, which I am more then happy with! I believe you also helped me with this originally and gave me the idea of finding frequency. Want to thank you a ton for all your help!

One last question though. When reading up on 'volatile' variables I come across this quote:

A variable should be declared volatile whenever its value can be changed by something beyond the control of the code section in which it appears, such as a concurrently executing thread. In the Arduino, the only place that this is likely to occur is in sections of code associated with interrupts, called an interrupt service routine.]

So does this mean that wheelRpm, previousTime, and currentTime should all be made volatile? Is making a bunch of variables volatile going to have any ill effect? Thanks again!

  rotations += 1;t Meaningful names help, too. You are NOT counting rotations in this interrupt, unless there is only one interrupt per rotation. If that was the case, though, spokes would be 1, not 20.

Have you ever noticed that the language used to program the Arduino is called C++, rather than C+=1? Did you ever wonder why?

I've often wondered why it isn't called ++C.

I've often wondered why it doesn't have a more OO type name. C->add(1)

I've often wondered why it isn't called ++C.

I think there are two reasons for that. The simplest is that C++ sounds better to me than ++C.

The second is in the definition of pre-fix vs. post-fix notation. On post-fix form, the value of the variable is used first (C), then added to. So, this gives emphasis to the fact that the language is based on C, with additions but without changing anything in C. The pre-fix notation imples that C is incremented then used, which gives the impression that C is fundamentally changed, which is not the case.

  1. You have clearly thought about that far too much.

  2. Does

rotations += 1;

compile differently to

rotations++;

?

You forgot to ask about:

 rotations = rotations + 1;

Well let’s find out shall we?

Test sketch:

volatile int rotations;

void setup ()
{
 rotations++;
 rotations += 1;
 rotations = rotations + 1;
}
void loop () {}

Compiler generated code:

void loop ();
volatile int rotations;

void setup ()
{
 rotations++;
  a6:	80 91 00 01 	lds	r24, 0x0100
  aa:	90 91 01 01 	lds	r25, 0x0101
  ae:	01 96       	adiw	r24, 0x01	; 1
  b0:	90 93 01 01 	sts	0x0101, r25
  b4:	80 93 00 01 	sts	0x0100, r24

 rotations += 1;
  b8:	80 91 00 01 	lds	r24, 0x0100
  bc:	90 91 01 01 	lds	r25, 0x0101
  c0:	01 96       	adiw	r24, 0x01	; 1
  c2:	90 93 01 01 	sts	0x0101, r25
  c6:	80 93 00 01 	sts	0x0100, r24

 rotations = rotations + 1;
  ca:	80 91 00 01 	lds	r24, 0x0100
  ce:	90 91 01 01 	lds	r25, 0x0101
  d2:	01 96       	adiw	r24, 0x01	; 1
  d4:	90 93 01 01 	sts	0x0101, r25
  d8:	80 93 00 01 	sts	0x0100, r24
}
  dc:	08 95       	ret

Absolutely identical. So choose the form that is most informative to you and future maintainers.


I prefer:

 rotations++;

… on the basis that we are clearly adding 1 and not some other number.


However:

 rotations += 1;

… suggests that maybe some day we might add 2.


And:

 rotations = rotations + 1;

… suggests that maybe we meant to add 1 to old_rotations or some other variable. And maybe we didn’t mean to add 1.

As I thought. I preferrotations++;too, but I don't see any reason to pick up people that use one of the alternatives.

I prefer Inc(rotations) ; - ooops - wrong language ;)

(hint - code has to finish with a full stop as the last character) .

I wouldn't "pick up" people who use other forms, because they are all valid. That's all personal style after all. And in case you are wondering:

 ++rotations;
 rotations -= -1;
 rotations += (6 - 5);

All generate the same code as above. It's almost as if the compiler knows what you really mean! Creeepy.

Even this:

 rotations += (6 - 5) * 1 + 0;

Msquare: I prefer Inc(rotations) ; - ooops - wrong language ;)

Sounds a bit like Pascal.

but I don't see any reason to pick up people that use one of the alternatives.

I was not picking on OP for the use of rotations += 1 , or rotations = rotations + 1 or inc(rotation) or any other variation of incrementing rotation. I was simply pointing out that rotations was a lousy name, in that what is being stored in the variable is NOT the number of rotations that have occurred,