Go Down

Topic: In search of a better RPM readout... (Read 11187 times) previous topic - next topic

AdeV

One of the things I want my Arduino to do is be an ECU/data logger for a slow-speed diesel engine. One of the key metrics will be RPM - and, in particular, I need a pretty accurate RPM (to within +/-1rpm ideally), something the RPM reader in Playground isn't. I wanted it faster as well, so I get an updated RPM every second, instead of every 20s @ 60rpm for example.

After much faffing & trying things, I settled on risking the millis() [but later updated to use the micros()] function call in an interrupt. I'm using a Hall Effect switch to indicate a completed RPM (the final version may use several magnets to improve resolution).

Here's the code:

Code: [Select]
//-----------------------------------------------
// RPM deducer version 2
// Warning: uses millis() in interrupt, which is
// widely regarded as A Bad Thing...
//-----------------------------------------------
byte trigger;          // Set to TRUE if the interrupt has triggered at least once since the last loop
unsigned long t0, t1;  // The time in milliseconds that the interrupt fired q
float rpm;

void setup()
{
 Serial.begin(9600);
 attachInterrupt(0, rpm_fun, RISING);  // to pin2 of the Arduino

 t0 = 0;  // Previous time = 0
 t1 = 0;  // This time = 0
 trigger = 0;
}

void loop()
{
 float g = 0;

 if (trigger != 0) {
   // If the interrupt has been fired, work out the new RPM
   g = t1 - t0;
   //g = g/1000; // Convert to milliseconds
   rpm = (1/g) * 60000000;
   trigger = 0;  // Reset the interrupt
 }
   
 // Print RPM every second, or thereabouts, provided there's at least one average
 Serial.print("Last gap: ");
 Serial.print(g/1000);
 Serial.print("mS, RPM: ");
 Serial.println(rpm);
 
 // Wait around 1s
 delay(1000);
 
}

void rpm_fun()
{
 // Each rotation, this interrupt function is run twice
 // (is it?? Why? A: It's not (necessarily), it depends on the magnet.
 if (t0 == 0)
   t0 = micros();
 else {
   t0 = t1;
   t1 = micros();
   trigger = 1;
 }
}


This function seems  to give good results, when I tested it on an ancient record player it recorded 32.Xrpm and 44.Xrpm for the two speeds it'll do (no 16 or 78 option...), which is reasonably close to tolerance. The X varied a touch, and I can't remember what it was now. The record player is at least 20 years old, unused for the last 10, and may not be running at full speed...

However, the timings could be slightly low because, presumably, I'm losing microseconds while the interrupt code is running.

Can anyone think of a better way to keep the accuracy & update frequency I get from using the dreaded micros() function inside an interrupt?

borref

Calculating a slow RPM should be pretty easy, but as with many projects there's a bit more to it than meets the eye at first if you need a very accurate reading.

As of version 0018 it should be safe to call millis() and micros() from within an interrupt handler, so I would not worry too much about that warning in your source. The millis() function however is somewhat "jumpy" as it may increment twice on occasion (the reason is that it gets incremented once every 1024 microseconds rather than every 1000 microseconds). As such it may not be the best timebase for your RPM calculation. Micros on the other hand is accurate, but a bit of an overkill for slow RPM.

An altenative is to use the timer0 overflow count directly (one tick every 1024 microseconds). Your interrupt handler could then look something like this:

Code: [Select]

extern volatile unsigned long timer0_overflow_count;
volatile boolean ticks_valid;
volatile word ticks_per_rev;

void rpm_fun()
{
 static word pre_count;
 word count = (word)timer0_overflow_count;

 ticks_per_rev = count - pre_count;
 pre_count = count;
 ticks_valid = true;
}

void loop()
{
 if (ticks_valid) {
   float rpm = ticks_per_rev * (60.0 / 1024.0);
   ticks_valid = false;
   Serial.print("RPM: ");
   Serial.println(rpm);
 }
}


The above should give you a "fresh" rpm for every full rev accurate to within one decimal (+/- 1 timer tick).

Note that the volatile prefix must be used for any variables modified in an interrupt handler. If you didn't use the volatile prefix for your record player test - you should not rely on those calculations being accurate.

A couple warnings:

- The above code will only be valid for boards with a 16MHz clock.
- The loop must run faster than one engine rev. If not (e.g. if you keep the 1000ms delay) you need to guard your rpm calculations against interrupts.

For a slow RPM engine you're not likely to improve on accuracy with additional magnets.

AdeV

Hi Ben,

Thanks for the reply; I can see how it's doing something similar to my calls to micros(), just in fewer clock ticks & guess.

The only thing I can call you out on is your RPM calculation; I had to modify it to this:

Code: [Select]
   float rpm = (60000000.0 / 1024.0) / ticks_per_rev;

to make it work: the number of ticks rises with slower RPMs, so it's an inverse relationship rather than a linear one. Also, it still seems to be in microseconds, hence the 60 million numerator.

On the bright side (for me, at least), the speeds your calculations give are exactly the same as the calculations I was getting, so I can deduce that either the Arduino is running fast, or the turntable is running slow...


I want to keep the 1 second refresh for all variables (I have temperature sensors to add yet, and maybe other stuff too), but I will implement that using the millis() function instead of a delay(), like this:

Code: [Select]
void loop()
{
   // record millis()

   // RPM calculations

   // Read other sensors

   // if this millis - last millis > 1000
      // output data via serial

}


So I'm not overwhelmed by data, and when I do get an update, it's as up-to-date as it can be.

PaulS

Code: [Select]
float rpm = (60000000.0 / 1024.0) / ticks_per_rev;
Floating point arithmetic on an Arduino is done in software, so it is pretty slow. Every time that you compute 60000000.0 / 1024.0 the result should be the same, so, get out your handy calculator and determine the result, and use it instead of calculating it each time.
The art of getting good answers lies in asking good questions.

AdeV

Paul - you're right of course. I just re-jigged the equation as supplied, I've done no optimisation yet. In reality, I can probably get away with using integer maths, to save time.

borref

Quote
to make it work: the number of ticks rises with slower RPMs, so it's an inverse relationship rather than a linear one. Also, it still seems to be in microseconds, hence the 60 million numerator.

It is indeed - my mistake. I made a digital tach last year for a 6 cyclinder V6 engine and used the same base as above and it came out pretty accurate.

If you introduce delays in your loop (millis or otherwise) you should add enable/disable interrupts when reading the tick count. Otherwise you run the risk of reading while its being updated.

Quote
Floating point arithmetic on an Arduino is done in software ...

As is integer arithmetic - so speedwise they're pretty much the same for an equal number of bits precision.

Constant arithmetic in parentheses such as "(60000000.0 / 1024.0)" will always be calculated compile time - so there's nothing to gain (speedwise) from doing this yourself.

AdeV

Well, I hooked it up to the engine today, and it all works :) Click either of the pics below for a bigger version:

Setup:

In action:

There is an issue, however. The difference between ~644rpm & ~651rpm is one timer tick (91 vs 90); the next step (89 ticks) is a similar distance away. Unfortunately, that's just not granular enough :(

I'll investigate micros() again tomorrow, to see if it's any different.

borref

640 RPM is an order of magnitude faster than the 60 you suggested in your first post so you will need a finer resolution tick count then to meet your requirements.

Timer0 (which is the base for millis and micros) runs at 250 kHz and ticks will increment once every 256 cycles. If my calculations are right - the error (+/- 1 ticks) at 60 RPM is only +/- 0.12, but at 600 RPM it is as much as +/- 12.29.

My actual implementation accounts for timer0 fractions - so effectively I have ticks with 250 kHz resolution. Error is then down to +/- 0.048 RPM at 600 RPM. Accounting for fractions adds some additional complexity:

Code: [Select]
extern volatile unsigned long timer0_overflow_count;

volatile boolean ticks_valid;
volatile uint32_t ticks_per_rev;

void setup()
{
 // to be added
}

void rpm_fun()
{
 static uint32_t ticks_pre;
 uint32_t ticks_now;
 byte t;

 ticks_now=timer0_overflow_count;
 t=TCNT0;
 if ((TIFR0 & _BV(TOV0)) && (t<255)) ticks_now++;
 ticks_now=(ticks_now << 8) + t;

 ticks_per_rev = ticks_now - ticks_pre;
 ticks_pre = ticks_now;
 ticks_valid = true;
}

void loop()
{
 if (ticks_valid) {
   float rpm;
   uint32_t tpr;
   
   noInterrupts();
   tpr = ticks_per_rev;
   ticks_valid = false;
   interrupts();
   
   rpm = 15e6 / tpr;
   Serial.print("RPM: ");
   Serial.println(rpm);
 }
}

pluggy

#8
May 28, 2010, 02:07 pm Last Edit: May 28, 2010, 02:14 pm by stephen_t Reason: 1
The lump of wood with the bit of stripboard stuck on the end reminds me of my handiwork :)

Personally I'd stick with millis and count off a number of revs to work out the RPM, if it was updated every second thats 10 revs at 600 rpm. Just count off the revs and every 10th one, call millis() and get a time.  It would give you better than 1 RPM granularity.  
http://pluggy.is-a-geek.com/index.html

AdeV

Hi Ben,

Quote
640 RPM is an order of magnitude faster than the 60 you suggested in your first post


Indeed, apologies, I didn't mean to mislead...

I will try the revised code tonight & let you know. One thing, my first language isn't C, so can I ask what these two lines are doing pls?

Code: [Select]
 if ((TIFR0 & _BV(TOV0)) && (t<255)) ticks_now++;
 ticks_now=(ticks_now << 8) + t;



Pluggy - stick around, you'll see lots more bodgery as time goes on  ;D I usually "finish" a project before it gets into a box...

borref

Quote
my first language isn't C, so can I ask what these two lines are doing pls?

Well - it is a "virus" that eventually will erase your Arduino and stop your engine! ;)

Not so. It is a test to see if a timer overflow is pending. This can happen if the timer overflows while processing your hall switch interrupt - and then we need to increment the overflow counter to get an accurate tick count. This is similar to what you will find if looking at the source for the micros function.

I was down this route last year and what seemed like an innocent project at the surface turned out to be more of a challenge than I expected. My engine revs at 5000 RPM max and I get three pulses per rev. As you did, I started out with the millis difference and a simple rev count, but could not get the accuracy I was after. The jumpy nature was particularly noticeable for low revs (mine idles around 650). The problem being that millis is jumpy by design and when you divide by a few revs only it shows.

There is still more to what I had to do, but you should try this out first to see if it meets your expectations.

AdeV

That works much better, thank you  8-)

Another somewhat interesting glitch has appeared...

on Pin3, I'm mapping the received RPM to a PWM value, which I feed into a 100uA meter via a large resistor. This gives me an approximate RPM in analogue form.

If I leave it unplugged, the RPMs work exactly as advertised. If I plug it in, the RPM goes crazy, typically reading 30,000+, maxing at 75000. Nothing is changing in the software...

Hardware wise, I'm using Pin2 for the Hall sensor, which is active low. The RPM_Sense triggers on a rising value, i.e. just after the magnet has passed under the sensor.

I'm using Pin3 for my PWM output, this is wired through a ~50K resistor into a moving-coil 100uA meter and back to gnd.

My code, currently, is as follows (some of the comments are out of date):

Code: [Select]
//-------------------------------------------------------------------------------------
// ArduECU version 0.01 alpha
// Compiled with Arduino v0018
//
// Thanks to the members of the arduino.cc forum for their assistance

// Many comments removed to fit sketch into forum posting box...

extern volatile unsigned long timer0_overflow_count;  // Record the most recent timer ticks
volatile boolean ticks_valid;                         // Indicates a valid reading
volatile unsigned long ticks_per_rev;                 // Number of ticks counted in the current revolution
unsigned long msSinceRPMReading;                      // Number of mSec since the last rpm_sense (used to spot zero RPM)
float lastRPM, peakRPM;                               // Most recent RPM reading, and highest RPM recorded since last reset

const float __TICKS_TO_RPMS = 15e6;                   // Convert clock ticks to RPM by dividng ticks into this number
                                                     // The number will change if there are more magnets in an rpm
                                                     //   (e.g. 2 magnets = 29296.875)
const unsigned int __WAIT_BEFORE_ZERO_RPM = 2000;     // Number of mS to wait for an rpm_sense before assunming RPM = 0.
const int __REV_GAUGE_PIN = 3;                        // 100uA meter on this pin, with 50K current-limiting resistor


unsigned long msSinceSend;                            // mSec when the last data was sent to the serial port
const unsigned int __WAIT_BETWEEN_SENDS = 1000;       // Number of mS to wait before sending a batch of data.


void setup()
{

 Serial.begin(9600);                                 // Initialise serial comms.
 msSinceSend = millis();                             // Data sent counter

 attachInterrupt(0, rpm_sense, RISING);              // RPM sense will cause an interrupt on pin2
 msSinceRPMReading = 0;                              // If more than 2000 (i.e. 2 seconds),
                                                     // then RPMs can be assumed to be zero (< 15rpm
                                                     // at most, with a single magnet, no small IC
                                                     // can run that slowly).
 lastRPM = 0;                                        // Current RPM to zero
 peakRPM = 0;                                        // Max recorded RPM to zero
 pinMode(__REV_GAUGE_PIN,OUTPUT);                    // Set pin 3 to be output

}

// ------------------------------------------------------------------------------------------------
// FUNCTION: RPM-SENSE
//
// Called whenever the RPM sense signal rises. In my setup, the hall effect switch is normally
// high, goes low when a south pole is introduced to the sensor, and rises back to high as the
// magnet goes away. Thus, the RPM sense is called on the trailing edge of the magnet.
//
// Version Date        By  Comment
// -------|-----------|---|------------------------------------------------------------
//   0.01a 26-May-2010 JAV Created (with an assist from BenF from the arduino.cc forum
//
// ------------------------------------------------------------------------------------------------
void rpm_sense()
{
 static unsigned long pre_count;               // Last timer0_overflow_count value
 unsigned long ticks_now;                      //

 ticks_now = timer0_overflow_count;            // Read the timer

 byte t = TCNT0;
 if ((TIFR0 & _BV(TOV0)) && (t<255))
   ticks_now++;
 ticks_now = (ticks_now << 8) + t;

 if (pre_count == 0) {                         // First time around the loop?
   pre_count = ticks_now;                      // Yes - set the precount, don't use this number.
 } else {
   ticks_per_rev = ticks_now - pre_count;      // No - calculate the number of ticks...
   pre_count = ticks_now;                      // ...reset the counter...
   ticks_valid = true;                         // ...and flag the change up.
 }
}

void loop()
{
 unsigned long thisMillis = millis();          // Read the time once

 // Calculate RPM
 if (ticks_valid) {                            // Only come here if we have a valid RPM reading
   unsigned long thisTicks;
   
   noInterrupts();
   thisTicks = ticks_per_rev;
   ticks_valid = false;
   interrupts();
   
   lastRPM = __TICKS_TO_RPMS / thisTicks;      // Convert ticks to RPMs
   ticks_valid = false;                        // Reset the flag.
   msSinceRPMReading = thisMillis;             // Set the time we read the RPM.
   if (lastRPM > peakRPM)
     peakRPM = lastRPM;                        // New peak RPM
 } else {
   // No tick this loop - is it more than X seconds since the last one?
   if (thisMillis - msSinceRPMReading > __WAIT_BEFORE_ZERO_RPM) {
     lastRPM = 0;                              // At least 2s since last RPM-sense, so assume zero RPMs
     msSinceRPMReading = thisMillis;           // Reset counter
   }
 }
 
 // Calculate temperatures
 // not implemented yet
 
 
 
 // Is it time to send the data?
 if (thisMillis < msSinceSend)                  // If thisMillis has recycled, reset it
   msSinceSend = millis();
   
 if (thisMillis - msSinceSend > __WAIT_BETWEEN_SENDS) {
   // Yes: Build the serial output...
   
   // For now, send everything plaintext. Maybe compression would be a good thing, later down the line...
   Serial.print("uECUv0.01|");                  // Send ID - ditch this for something smaller
   Serial.print(lastRPM);                       // Current RPMs
   Serial.print("|");                           // Field Separator
   Serial.print(peakRPM);                       // Peak RPMs
   Serial.print("|");                           // Field Separator
   Serial.print(0);                             // Temperature #1 (water in)
   Serial.print("|");                           // Field Separator
   Serial.print(0);                             // Temperature #2 (water out)
   Serial.print("|");                           // Field Separator
   Serial.print(0);                             // Temperature #3 (exhaust gas)

   // Debugging
   // Serial.print("|");                           // Field Separator
   // Serial.print(ticks_per_rev);                 // clock ticks in the last rpm

   // That'll do for bnow
   Serial.println();                            // EOL.

   msSinceSend = millis();                      // Reset the timer
   
   if (Serial.available()) {
     char cmd = Serial.read();
     if (char('R') == cmd) {
       peakRPM=0;
     }
     Serial.flush();
   }
   
 }

 // Set the RPM gauge value in real-time...
 if (lastRPM < 1000) {
   // Sub-1000 RPMs, map to between 0-255
   analogWrite(__REV_GAUGE_PIN,map(lastRPM,0,1000,0,255));
 } else {
   // If RPMs > 1000, write 255 (=meter FSD); but we've got bigger problems if the RPMs ever get this high...
   analogWrite(__REV_GAUGE_PIN,255);  
 }

}


I've probably done something silly, but I don't understand why it only goes bonkers when the meter is actually plugged in.

borref

One possibility could be that your hall sensor is picking up the PWM frequency from the meter coil. Have you tried to relocate the meter away from the sensor?

Also you may want to look at adding a low-pass filter on pin 3 to eliminate the PWM frequency from reaching your meter.

AdeV

The sensor is about 8ft away from the meter, although obviously the wires come pretty close at the Arduino end (adjacent pins). It didn't do it with the old version of the code, which used the 60e6/1024 calculation but, I have changed the wire to the meter since then; I used some twisted pair, maybe that's causing an issue.

AdeV

OK, that was weird...

I added a 3k3 resistor on the breadboard, and the problem went away. Adjusted the main pot down to around 46K so the scale remained correct, and all is well.

The signal wire (to the potentiometer) is around a foot long, the lead out to the meter another 3ft or thereabouts. The ground wire, therefore, is about 4ft & straight back to the board. The wire is recovered from some structural network cable, so it's solid core, twisted pair (the green pair, in case that makes a difference  ;)). There's about an inch of untwisted wire at the potentiometer.

The same circuit didn't need the on-breadboard resistor when I ran it with jumper wires & croc clips to the potentiometer & the meter.

I can only assume that, with the twisted wire, some kind of signal got onto the gnd line & fritzed the hall switch readings, somehow.


Go Up