Help with tachometer code

Just wondering if someone could look at this code for me, I am trying to measure the rpm on my car and am hooking the rpm pulse to the arduino using this circuit : http://public.blu.livefilestore.com/y1pmGjO7cRHKUBXl1QhjtANHoubwSIzvEu1ZB_nYRFQK3vfpEGhjx9nD73zb14C97U8ZGGqhPnpyOFT5uGk4072hA/TachFilter.JPG

The code works and is accurate at 0 to 1500 rpm but then for some reason goes way out about 1500 reading follow:

2000 is reading as 2200
2500 is reading as 2800
3000 is reading as 3000

Any one have any ideas why this might be its really got me stumped. I will post the code below and any help would be much appriciated .

#include <LiquidCrystal.h>
volatile byte rpmcount;
unsigned int rpm;
unsigned long timeold;
LiquidCrystal lcd(12, 13, 11, 10,9,8,7);

 void rpm_fun()
 {
    //Update count
      rpmcount++;
 }

void setup()
 {
   attachInterrupt(0, rpm_fun, RISING);
   rpmcount = 0;
   rpm = 0;
   timeold = 0;
 
 }

 void loop()
 {
   delay(200);   // Sample Time
   detachInterrupt(0);

   rpm = (millis() - timeold) / rpmcount;
   rpm = 1000/rpm;
   rpm = (rpm / 3) * 60;  // Six Cyl - 3 PPR  
   timeold = millis();
   rpmcount = 0;
   
  lcd.clear();
  lcd.setCursor(0, 0);
  lcd.print(int(rpm));  // print rpm
   
   //Restart the interrupt processing
   attachInterrupt(0, rpm_fun, RISING);

  }

Is the reading at 3000 rpm dead on per your post or does it show 3300? That's what I get if I do integer math per your code below:

   rpm = (millis() - timeold) / rpmcount;        *****200/30=6
   rpm = 1000/rpm;                             *****1000/6=166
   rpm = (rpm / 3) * 60;  // Six Cyl - 3 PPR   *****55*60=3300

I think if you rearrange the math you could get within a few percent (e.g. move the 1000 from the 2nd calc to the 1st - you'd have to use a long integer of course).

I would recommend though that you switch to measuring the time difference for some fixed number of pulse (50 or so). Your interrupt routine just has to count pulses til the 50 then calculate the millis difference and store it somewhere. Your Loop routine can just wake up every 200 ms, calculate rop, and update the display. You'll still have to be careful with integer math but you won't be exposed to just missing or just catching a pulse.

I wouldn't bother with repeatedly attaching/detaching the interrupt either - that's just a recipe for missing pulses.

I've been poking at tach's off and on - what are you doing for power?

Thanks for the reply I understand what your saying but I don't get exactly how I should do it. To clarify my first post I have a tacho which is accurate and when that reads 3000 rpm this program reads 3800 ish rpm the weird thing is under 1500 rpm it is 100% accurate. I tried it with float math below to get rid of the integer problems:

  rpm = (millis() - timeold) / rpmcount;
  rpmfloat = (float) 1000.0/rpm;
  rpmfloat = (float)(rpmfloat / 3.0) * 60.0;  // Six Cyl - 3 PPR  
  timeold = millis();
  rpmcount = 0;
   
  lcd.clear();
  lcd.setCursor(0, 0);
  lcd.print(int(rpmfloat));  // print rpm

This still gives the same error as with integer math but im probably writing it wrong.

I hate to be a pest but could you give me a bit more of an idea of how I would rearrange the equation I got to me honest maths arnt my strong suite :stuck_out_tongue:

Thanks Again

In your code:

rpm = (millis() - timeold) / rpmcount;

you are dividing a time quantity by an event quantity. That will give you a time quantity (a period). What you want to do is to divide an event quantity by a time quantity; that will give you a frequency. Scale the frequency to Hz and then multiply that by 60 to get RPM.

const float freqHz = ((float) rpmcount / (float) (millis() - timeold)) * 1000.0;
const float RPM = freqHz * 60.0;

Be careful to avoid any zero division.

Thanks for that chess player, I implemented the info you gave me now it is accurate to about 2000 rpm but past that it's still reading a higher rpm than it actually is, its really got me stumped.

I am going to try out what bill suggested and have it log 50 pulses then calculate, The maths looks right now so I can only guess its not reading the pulses right or the time is not being logged correctly.

Looking at your code:

volatile byte rpmcount;

I would suggest that an event counter should be not a byte but rather (at least) an unsigned int to avoid overflow problems.

How about:

volatile unsigned int eventcount;

which has better naming. HOWEVER, because of the possibility of an interrupt hitting at exactly the wrong time, the routine that reads the eventcount must first disable interrupts, then read the value, and then re-enable interrupts.

Even after that still doding the same thing, Im thinking the arduino might not be the best way to read this. Might be better to go with a freq to voltage converter.

  rpm = (millis() - timeold) / rpmcount;
  rpmfloat = (float) 1000.0/rpm;
  rpmfloat = (float)(rpmfloat / 3.0) * 60.0;  // Six Cyl - 3 PPR

might be ok except the main damage happens in the 1st line before you get to floats. I'm not confident on casting and floats but try:

countsPerSecondfloat =1000.0*(millis()-timeold)/(float)rpmcount;
rpmfloat=...

I used the extra float variable countsPerSecondfloat mostly just so I could get straight what the numbers meant. You would be fine with long integers, you just need to get the multiply by 1000 done before you divide the 200'ish ms by the 30'ish count.

I'll see if I have the math from a tach routine handy that uses ints and just arranges the sequence not to lose precision.

The frequency to voltage conversion (like an LM2917 I think) is an easy way to go if you don't need much precision.

ok, I checked and my routine did use floats. My interrupt routine tracks the time for 10 pulses - you would have to do 30, then calculates rpm as follows

rpm=60000/(float)tachtime*10; //rpm based on time for 10 revs

tachtime would be volatile unsigned long updated by an interrupt routine as follows:

void tachmonitor(){
static int tachcount; static unsigned long previousmillis;
if (tachcount <9) { //bulking up tach interrupts by a factor of 10
    tachcount++;
 }
 else {  //every 10th tach pulse we calculate elapsed time
     tachtime=millis()-previousmillis;
     previousmillis=millis();
  }

I can't promise this is exact because my code has a bunch of other stuff going on which I cut out. Given that I've still got floats in there yours will be better if you understand it better.

Thanks for all the help the code above gives the same error but its not an issue with the code, I setup a 555 timer and the program reads spot on so its clearly an issue with the signal, I am using the circuit in my first post to smooth the tach signal out but above 1500 rpm the noise gets to great and that is where it gets wrong. Any idea on a better way to filter the signal ??

The classic hookup point for a tach is to the negative terminal on the coil (J1 in your circuit below).

The ignition signal coming off the coil neg. terminal is a decaying sinus wave that initially peeks at +/- 200V as the coil charges. When the coil discharge through the sparkplug, the amplitude will drop to about +/- 40V, but with a different frequency compared to the charging phase. When the didtributor points close, the signal drops to ground and the cycle repeats.

If this classic ignition and pickup applies to your setup I would look for a different interface circuit as what you have is likely to damage your Arduino.

Well the car does have a pickup point under the dash but I checked the schematic and it just links to the - side of the coil, But what your saying could be the issue I am having with the figures being out at high rpm, Could you possibly point me towards the kind of circuit I should be looking at for this ?

Any help would be great

The iginition waveform (coming off coil neg.) is supposed to look like this:

One approach is to trigger on the first pulse after t1 and a simple circuit to do this is as follows (see the bottom part):

Tach-In connects to the pickup from your coil and tach-out goes to an Arduino digital pin. D2 will supress the negative going pulses (this is missing from your circuit and is why it may eventually destroy your AtMega). If R2 is too high you will pick up more than the initial pulse and if too low you will not see any pulses. So if you plan on building this you may want to replace R2 with a pot and tune it until you get a single pulse.

R1/R2 acts as a voltage divider, D3 (4V7 zener) is to limit voltage on the Arduino input pin to a safe level. Arduino Gnd should be connected to car battery minus.

Thanks for the great info there ben

Looking at the circuit it really makes sense, Just one thing with the values, im not really good with the math but with that divider at the peak of 200v I would see about 1.5v output is this ok for using the arduino interupt ?. Also assuming the second part of the wave peaks at 150v that will be around 1v output and is that low enough to not be picked up ?

I've used this circuit on a V6 4.3L GM engine and arrived at these values through trial and error (had no scope available at the time). It works perfect (+/- 1 RPM accuracy) from idle (600 RPM) to wide open throttle (4500 RPM on a good day).