Arduino Tachometer Problem

I built a tachometer for my riding lawn mower but it’s not working correctly and I need some help. FYI I’m using an Atmega328 on a circuit board where pins 2 and 3(for ISR) are already in use. The problem is that my displayed rpm is very erratic and not accurate at all. I know the idle of my lawn mower is 1600 rpm but my tachometer displays between 800 and 2000 rpm. Honestly, I think the way I went about the code is wrong but I’m not sure how else to do it. Any help is much appreciated.

I’ve attached a pic of the schematic so you know how it’s hooked up.

Here’s the code:

#include <LiquidCrystal.h>
LiquidCrystal lcd (5,6,9,10,11,12);

const int sparkButton = 7;
const int Backlight = 4;														

int sparkState = digitalRead(sparkButton);
int lastSparkState = 0;
int time = millis();

long sparkCount;
long sparksPerSecond;
long SparksPerMinute;
long rpm = SparksPerMinute;

void setup()
{	
	lcd.begin(16,2);
	pinMode(Backlight,OUTPUT);
	pinMode(sparkButton,INPUT_PULLUP);
	digitalWrite(Backlight,HIGH);
	digitalWrite(sparkButton,HIGH);
}

void loop()
{		
sparkState = digitalRead(sparkButton);

if (millis() - time >0)			//Start time and counting pulses
{
	if (sparkState != lastSparkState)
		{
			if (sparkState==LOW)	sparkCount++;	
		}
}
lastSparkState = sparkState;

if (millis() - time >= 1000)		//End time and report total pulses
{
	time = millis();
	SparksPerMinute = sparkCount * 60;
	line1();
	lcd.print("Tachometer:");
	line2();
	lcd.print(SparksPerMinute);
	lcd.print(" RPM");
	sparkCount=0;
	
}	
}
				
///////////////////////////////////////////////////////////////////////////
void line1()
{
	lcd.setCursor(0,0);
}
///////////////////////////////////////////////////////////////////////////
void line2()
{
	lcd.setCursor(2,1);
}

Thanks!

Circuit.JPG

Hi, you are interfacing directly from the spark plug.. I would be afraid that this would kill the Atmega328P on the long run. I would have fed the output of the spark plug through an optoisolator to prevent any transient /spikes.

2nd, perhaps the output of the spark plug is as clean as you'd want it to be. Perhaps a little signal conditionning would be in order. Perhaps feeding the signal through a schmitd trigger which has an histerisys would be help. Or use a 555 time in one shot mode (monostable time) in order to ensure you generate one clean single pulse for each pulse from the spark plug ....

Those are only ideas ..... How did you validate that the real RPM is 1600rpm ?

dan

Also taking a small number such as revolutions per second and multiplying up to revolutions per minute is going to propagate the error by the same proportion. + or - 20 becomes + or - 1200.

You can take a moving average as a simple way of alleviating large deviations, since unless the frequency of the shaft is INCREDIBLY consistent, you are bound to get problems like this,

I'm not interfacing directly from the spark plug, there is a transistor that just uses that input to act as a switch...so I hope that wouldn't kill the Atmega. Plus, I am using a 4.7V Zener to regulate, as shown in the diagram (sorry it wasn't labeled). Maybe I need to use some conditioning but I'm hoping there's a way around that. I validated the rpm with a separate tachometer.

This is probably also worth mentioning: I programmed an Arduino Uno with blink to simulate a spark plug firing and I still get the same erratic and inaccurate behavior. I had the Uno blink with a delay(25).

Thanks

Hi,

the blink sketch should generate a very stable flashing of your LED. Would it be possible to post the code for your blink program ? Perhaps if find something on that blink sketch it will fix your other program?

Have you tried a different ATMEGA328 chip to see if it also behaves erratically ?

dan

Would it be possible to post the code for your blink program ?

void setup() {
   pinMode(13, OUTPUT);
}

void loop() {
  digitalWrite(13, HIGH);   
  delay(25);              
  digitalWrite(13, LOW);   
  delay(25);              
}

Have you tried a different ATMEGA328 chip to see if it also behaves erratically ?

I swapped the chip just to make sure

FYI I'm using an Atmega328 on a circuit board where pins 2 and 3(for ISR) are already in use.

Exactly what is attached to pins 2 and 3 and what is the ISR doing ?

Exactly what is attached to pins 2 and 3 and what is the ISR doing ?

Pins 2 and 3 are being used for buttons. I am not currently using an ISR, as I read that it can only be used on pins 2 and 3 (which are taken).

Thanks

Would you be able to change the test time from 1000 -> 30000 and change the multiplier from 60 to 2, and record a handful of values?

I would be better to to 60000 and a multiplier of 1, but you can get double the values with the first way, and should still be fine.

I have a thought and am curious.

Pins 2 and 3 are being used for buttons.

But not in the code you posted.

But not in the code you posted.

Correct! The code I posted is a sample of the overall code, which is 13K bytes. The tachometer response is identical in both the main code and sample that I posted. I was just trying to save everyone the headache of looking at 13K worth of code. Is there a better way to accomplish the tachometer goal than what I am doing? Thanks

//int time = millis();
unsigned long time = millis();

You need to change the declaration of time to unsigned long to match millis().

This is probably also worth mentioning: I programmed an Arduino Uno with blink to simulate a spark plug firing and I still get the same erratic and inaccurate behavior. I had the Uno blink with a delay(25).

First, I fixed the declaration of time, and then I set up one Arduino with your blink program and jumper pin 13 to pin 7 on another running the tachometer program I get stable output at 1200 rpm. Your code is not perfect, but it is counting 25ms pulses accurately. Every 7 or 8 counts it reports 1140, but that is an artifact of the timing. Grounds were connected. I did my test using output to the serial monitor but I checked it at 1200 baud so I don’t think the speed of your lcd printing is an issue.

Your first order of business is to get your program running with a known input.

As was mentioned above, your reading of the milli seconds should be an unsigned long but you could cast it into an unsigned int. Using it as a signed int is bound to cause it to hang. The other obvious error is that you expect meaningful results by counting the pulses for one second. At 800 RPM, that would be 13.333 counts. That means your counting would be 780 to 840, depending on several things. You should be counting how long between pulses. You might get a total of ten and then invert the result and multiply by 6. Also, where are you sensing the spark information. Most small resistors are not rated at 15Kv. A safer place would be on the points wire. These usually only spike to 300 to 400 V. There is usually a kill wire that you short to ground to stop the engine. This has the point rate on it. You might also put a resistor across the zener to help turn off the transistor. In your loop I don't see you waiting to see the transistor turn off. Dwight

You can to use pinchange interrups on all pins, not only on pins 2 and 3. These two pins only have a dedicated isr. For the other pins there is one isr for each of the three groups of 8 bits each.

Each pin can be individually enabled. Once interrupted, you have to figure out which pins of the group changed.

I have one of the cheap 12 pulse encoders driven by pinchange irqs on A0 and A1 for the encoder and A2 for the switch, it works like a charm.

http://playground.arduino.cc/Main/PinChangeInt

You need to change the declaration of time to unsigned long to match millis().

  • Thank you!

You should be counting how long between pulses

  • Ok that makes sense. Below is my code for the first attempt at doing this. Unfortunately it only prints a high number that I have to divide by ten million to get 429…which makes no sense. Do you have a small code sample of how I should go about counting time between pulses?

For the other pins there is one isr for each of the three groups of 8 bits each.

  • Awesome. Could you help me get this started, I’m not sure where to begin with the code as I’ve never done that before and the examples online are confusing me. Thanks!

Updated code for trying to count time between pulses:

#include <LiquidCrystal.h>
LiquidCrystal lcd (5,6,9,10,11,12);

const int sparkButton = 7;
const int Backlight = 4;
int sparkState = digitalRead(sparkButton);
unsigned long time = millis();
unsigned long start, finished, elapsed;

void setup(){
	lcd.begin(16,2);
	pinMode(Backlight,OUTPUT);
	pinMode(sparkButton,INPUT_PULLUP);
	digitalWrite(Backlight,HIGH);
	digitalWrite(sparkButton,HIGH);
}

void loop(){
	sparkState = digitalRead(sparkButton);

	if (sparkState==LOW)	start = millis();
	
	if (sparkState==HIGH)	finished = millis();
		
	elapsed = (finished - start) / 10000000;
	
	time = millis();
	line1();
	lcd.print("Tachometer:");
	line2();
	lcd.print(elapsed);
	
	start = 0;
	finished = 0;
	elapsed = 0;
}
///////////////////////////////////////////////////////////////////////////
void line1()
{
	lcd.setCursor(0,0);
}
///////////////////////////////////////////////////////////////////////////
void line2()
{
	lcd.setCursor(2,1);
}

I can offer you a small framework using pinchange interrupt on D7,
detecting the time between two falling edges in micros.

Access to the captured value is controlled via a newTime flag,
when its set, no new value will be written by the isr until it is cleared again.

I did no math on the captured micros, BOUNCE_TIME should be tuned,
the synchronization is a litte crude, but save.

It seems to run :wink: (on a Nano).

I hope you can use it as a starting point.

#include <LiquidCrystal.h>
LiquidCrystal lcd (5,6,9,10,11,12);

const int sparkButton = 7;
const int Backlight = 4;
             
#define BOUNCE_TIME   3000       

volatile bool newTime; 
volatile unsigned long lowLowTime;

void setup(){
	lcd.begin(16,2);
	pinMode(Backlight,OUTPUT);
	pinMode(sparkButton,INPUT_PULLUP);
	digitalWrite(Backlight,HIGH);
    pinChangeSetup();
}

void loop(){
  if (newTime) {
	line1();
	lcd.print("uS:");
	line2();
	lcd.print(lowLowTime);
  }	
}
 
 // handle pin change interrupt for D0 to D7 here
ISR (PCINT2_vect)
 {
 static byte prevState;
 static byte currState;
 static unsigned long prevTime;
 static unsigned long currTime;
 static unsigned long currInterval;
 static unsigned long highInterval;
 
    currState = digitalRead(sparkButton);
    if (prevState ^ currState) { // different?
        currTime = micros();
        currInterval = currTime - prevTime;
        if (currInterval >  BOUNCE_TIME) {
            prevState = currState;
            prevTime = currTime;
            if (!currState) {         // falling?
                if (!newTime) {
                  lowLowTime = currInterval + highInterval;
                  newTime = true;
                }
            } else {
              highInterval = currInterval;
            }
        }
    }
 }

void pinChangeSetup ()
  { 
  // pin change interrupt (example for D7)
  PCMSK2 |= bit (PCINT23); // want pin 7
  PCIFR  |= bit (PCIF2);   // clear any outstanding interrupts
  PCICR  |= bit (PCIE2);   // enable pin change interrupts for D8 to D13
  }

///////////////////////////////////////////////////////////////////////////
void line1()
{
	lcd.setCursor(0,0);
}
///////////////////////////////////////////////////////////////////////////
void line2()
{
	lcd.setCursor(2,1);
}

Hi Whandall,
Sorry man, I’ve been trying to get that code to work and I cannot seem to understand what it’s supposed to do. My screen says
uS:
7393240

Not sure what that means. Am I doing something wrong? Here’s the updated code:

#include <LiquidCrystal.h>
LiquidCrystal lcd (5,6,9,10,11,12);

const int sparkButton = 7;
const int Backlight = 4;

#define BOUNCE_TIME   3000

volatile bool newTime;
volatile unsigned long lowLowTime;

void setup(){
	lcd.begin(16,2);
	pinMode(Backlight,OUTPUT);
	pinMode(sparkButton,INPUT_PULLUP);
	digitalWrite(Backlight,HIGH);
	pinChangeSetup();
}

void loop(){
	if (newTime) {
		line1();
		lcd.print("uS:");
		line2();
		lcd.print(lowLowTime);
	}
}

// handle pin change interrupt for D0 to D7 here
ISR (PCINT2_vect)
{
	static byte prevState;
	static byte currState;
	static unsigned long prevTime;
	static unsigned long currTime;
	static unsigned long currInterval;
	static unsigned long highInterval;
	
	currState = digitalRead(sparkButton);
	if (prevState ^ currState) { // different?
		currTime = micros();
		currInterval = currTime - prevTime;
		if (currInterval >  BOUNCE_TIME) {
			prevState = currState;
			prevTime = currTime;
			if (!currState) {         // falling?
				if (!newTime) {
					lowLowTime = currInterval + highInterval;
					newTime = true;
				}
				} else {
				highInterval = currInterval;
			}
		}
	}
}

void pinChangeSetup ()
{
	// pin change interrupt (example for D7)
	PCMSK2 |= bit (PCINT7); // want pin 7
	PCIFR  |= bit (PCIF2);   // clear any outstanding interrupts
	PCICR  |= bit (PCIE2);   // enable pin change interrupts for D8 to D13
}

///////////////////////////////////////////////////////////////////////////
void line1()
{
	lcd.setCursor(0,0);
}
///////////////////////////////////////////////////////////////////////////
void line2()
{
	lcd.setCursor(2,1);
}

My snippet wanted to print the time one revolution takes, which obviously fails.

Your output correlates to one rotation all 7.4 seconds... It should output something near 50000 for 1200rpm.

I think there is only a very short low pulse, so the 3ms debounce will be way to long (longer than the pulse itself).

The first reading could be false (if you happen to see a falling edge first), but it should be too low, not too high.

The isr would update the measurement on each revolution, if newTime is set to low by the main loop.

I hope I did not mess up the logic.

I checked it here with an rotary encoder and the readings were plausible.

Anyway... the interrupt seems to function (as there is any printout), so you have a good starting point.

I’m still not able to get the code working properly for this. I had to change the code to what is below. Whats more, for some reason when I get the rpm to display correctly and smoothly, if I disconnect the 6 pin programming header, the rpm goes way up to between 12,000 and 60,000. What the heck could cause this? It seems to happen no matter what variation of the code I use. Only way to fix it is to have a delay of 50ms…which is funny because my blink sketch that’s simulating the spark plug stays on for 25ms then off for 25ms so something is not right here. Thanks

#include <LiquidCrystal.h>
LiquidCrystal lcd (5,6,9,10,11,12);

const int Backlight = 4;
int sparkButton = 7;
int sparkState;                    
int lastSparkState;               
int blinking;                       
unsigned long startTime ;     
unsigned long stopTime;               
unsigned long elapsedTime;              
unsigned long rpm;
unsigned long time4 = millis();

void setup(){
	lcd.begin(16,2);
	pinMode(Backlight,OUTPUT);
	pinMode(sparkButton,INPUT_PULLUP);       
	digitalWrite(sparkButton,HIGH);  
	digitalWrite(Backlight,HIGH);
}

void loop(){
	sparkState = digitalRead(sparkButton);      
	             
	if (sparkState == LOW && lastSparkState == HIGH  &&  blinking == false){    
		startTime = millis();                                   
		blinking = true;                                     
		//delay(50);		If I use this delay, the code works when programming header is disconnected...?!?!?                                             
		lastSparkState = sparkState;                          
	}

	else if (sparkState == LOW && lastSparkState == HIGH && blinking == true){        
		stopTime = millis();
		elapsedTime = stopTime - startTime;
		rpm = (1000 / elapsedTime * 60);          
		blinking = false;                                  
		lastSparkState = sparkState;               
		lcd.setCursor(0,0);
		lcd.print("Tachometer:");		
			
	if (millis() - time4 >= 1000){
		time4 = millis();
		lcd.setCursor(0,1);
		rpm = (1000 / elapsedTime * 60);
		lcd.print(rpm);
		lcd.print("   ");
	}
}	
lastSparkState = sparkState;
}

Most likely missing a ground return someplace. As a minimum, most things need two wires. A common ground and a signal. Dwight