Car tachometer, optocoupler/interrupt questions

I plan to use the code below to make a tachometer (24 led, each led is 250RPM)

The idea is that a square wave triggers an interrupt, the time between two of these is logged and subtracted to get the time between pulses to determine the RPM. The result gets scaled down to 250RPM increments and shifted out to 24 leds.

I’m wondering if it would be better to reattach the interrupt at the end of the main loop to reduce the chances of the loop being interrupted before the three shiftouts (Worried about the value of RPM changing between shiftouts, might display some funky garbage).

Also curious if optocouplers need to be debounced in any way, seeing how bad bouncing can be if I trigger the interrupt with a push button (PulseStartTime and PulseEndTime were often the same number!) started to make me wonder about it.

I’m thinking I can trigger the interrupt with the optocoupler, to isolate the arduino from the 12v square wave the car produces.

Does it sound like I’m going down the right path for this to work?

//NOTE: No clue if the RPMcalc math is correct
//NOTE: use micros insead of millis (mills might be too slow)

volatile boolean Cycle = true;                  // set to TRUE for PulseStartTime and set to FALSE for each PulseEndTime
volatile unsigned long PulseStartTime;   // Saves Start of pulse in ms
volatile unsigned long PulseEndTime;     // Saves End of pulse in ms
unsigned long PulseTime;        // Stores dif between start and stop of pulse
unsigned long RPM = 0;          // RPM to ouptut 30*1000/PulseTime)
unsigned long leddata;
int latchPin = 8; //Shift register latch pin
int clockPin = 12; //Shift register clock pin
int dataPin = 11; //Shift register data pin
uint32_t ledData; //Value to shift out
int ScaledRPM;

void setup()
{
 pinMode(latchPin, OUTPUT);
 pinMode(clockPin, OUTPUT);
 pinMode(dataPin, OUTPUT);
 attachInterrupt(0, RPMPulse, RISING); // Attaches interrupt to Digi Pin 2
}

void RPMPulse()
{
  if (Cycle)                    // Check to see if start pulse
  {
    PulseStartTime = micros();  // stores start time
    Cycle = false;              // sets counter for start of pulse
  }
  else
  {
    detachInterrupt(0);         // Turns off inturrupt for calculations
    PulseEndTime = micros();    // stores end time
    Cycle = true;               // resets counter for pulse cycle
    calcRPM();                  // call to calculate pulse time
  }
}

void calcRPM()
{
  PulseTime = PulseEndTime - PulseStartTime; // Gets pulse duration
  RPM = 30000000/PulseTime*2;                 // Calculates RPM (milisecond version, original: RPM = 30*1000/PulseTime*2;     
  ScaledRPM = RPM/250; //Should divide RPM by 250 (On the gauge, one led is 250 RPM). Keep in mind any float values get tossed (499rpm would end up as 250)
  attachInterrupt(0, RPMPulse, RISING);      // re-attaches interrupt to Digi Pin 2
}

void loop()
{

ledData = (1 << ScaledRPM) - 1;
shiftOut(dataPin, clockPin, LSBFIRST, ledData); 
shiftOut(dataPin, clockPin, LSBFIRST, ledData >> 8); //Bitshifting because shiftout only sends 1 byte at a time
shiftOut(dataPin, clockPin, LSBFIRST, ledData >> 16); 
delay(1000);                  // 1 sec delay for debugging output
}

I do not think interrupts are at all suitable for this purpose.

What is the source for your tachometer? If it is an ignition system, then of course it will need debouncing, that is one of the worst possible sources of "bounce", and bounces are very difficult to deal with using interrupts.

I can’t understand why you are treating alternate pulses as either the start or the end of a timing sequence. Every pulse should be dealt with identically. This also makes it more difficult to implement a debouncing strategy.

To reduce the complexity of your ISR I’d suggest something like this. It also makes your separate calculation function redundant.

void RPMPulse()
{
unsigned long t=micros();
//250 x 24 = 6,000 RPM so we can safely assume:
//Anything over 20,000 RPM is bounce

if ( ( t-PulseStartTime)< 50 )
  return;

unsigned long PulseTime = t - PulseStartTime; // Gets pulse duration
RPM = 60000000/PulseTime;       // Calculates RPM 
    
ScaledRPM = RPM/250; 
PulseStartTime=t;
}

Tachometer source is from an ignition amplifier, which takes input from a magnetic pickup and controls the grounding of the coil. It has a dedicated pin for a tachometer.

I’m hoping since it’s a slightly more modern system it isn’t too noisy, but I can deal with the filtering of noise in hardware.

I’m more wondering if, in general, a optocoupler needs to be debounced or has any “bounce” in the way a physical switch bounces. Or does the LED light up consistently and fast enough that there is no bouncing?

I was hoping to directly measure the time between the rising edge of two square waves to have it as responsive as possible.

What do you think would be more suitable? Most all of the AVR-based car tachometers I’ve read about use interrupts, but if there’s a better way I’d like to try it.

KenF:
I can’t understand why you are treating alternate pulses as either the start or the end of a timing sequence. Every pulse should be dealt with identically. This also makes it more difficult to implement a debouncing strategy.

To reduce the complexity of your ISR I’d suggest something like this. It also makes your separate calculation function redundant.

void RPMPulse()

{
unsigned long t=micros();
//250 x 24 = 6,000 RPM so we can safely assume:
//Anything over 20,000 RPM is bounce

if ( ( t-PulseStartTime)< 50 )
  return;

unsigned long PulseTime = t - PulseStartTime; // Gets pulse duration
RPM = 60000000/PulseTime;      // Calculates RPM
   
ScaledRPM = RPM/250;
PulseStartTime=t;
}

This is interesting, though at the end after PulseStartTime=t; won’t the function set t back to the current number of microseconds once it is called again?

Well like I said, the rising edge of EVERY square wave is both the beginning of a cycle and the end of the previous.

Didn't see your reply, edited my post.

Aryl: This is interesting, though at the end after PulseStartTime=t; won't the function set t back to the current number of microseconds once it is called again?

That's the whole idea. t is holding the time of this interrupt call and pulseStart will hold the time it was last called.

Ah, that's right, ()millis doesn't advance when an interrupt is running. Thank you.

Aryl: Ah, that's right, ()millis doesn't advance when an interrupt is running. Thank you.

Actually millis DOES advance while an interrupt is running but it is only read at the start of the routine, so the variable t will not.

If I'm understanding correctly, the value pulseStart will be 0 until the first cycle, when it will be set to the start of the last interrupt (t), then t gets overwritten again and the old value of t, now stored as pulseStart, is subtracted from the "new" value of t, this measures the time between pulses, then it continues until reset or power is lost, correct?

Edit: Just re-read what you posted, edited out a stupid question.

Another question, do you think it would be better (Update faster) to use a frequency to voltage IC and a voltage divider to turn the AVR into an analog voltage meter, map the voltage to RPM and shiftout the results?

Or would that be too inaccurate... I need to look through the datasheets.

Aryl: Another question, do you think it would be better (Update faster) to use a frequency to voltage IC and a voltage divider to turn the AVR into an analog voltage meter, map the voltage to RPM and shiftout the results?

Or would that be too inaccurate... I need to look through the datasheets.

No. The biggest bottleneck you have is in the time it takes to shift out the 24 bits for your LEDs. Otherwise you could even update on EVERY pulse. Even this hardly qualifies as a "bottleneck" It should work fine as it is.

The LM2917 is a chip designed for just this so it shouldn't be inaccurate.

KenF: No. The biggest bottleneck you have is in the time it takes to shift out the 24 bits for your LEDs. Otherwise you could even update on EVERY pulse. Even this hardly qualifies as a "bottleneck" It should work fine as it is.

Understood. Sorry if it seems like most of these are stupid questions, reading the references for commands and actually trying to do something with them is a wold of difference...

Edited my post you quoted to make sure I understand what is going on.

I guess I know how it feels to be the person calling tech support now, and not the person answering :P

Aryl:
Understood. Sorry if it seems like most of these are stupid questions, reading the references for commands and actually trying to do something with them is a wold of difference…

Edited my post you quoted to make sure I understand what is going on.

I guess I know how it feels to be the person calling tech support now, and not the person answering :stuck_out_tongue:

No worries :slight_smile:

Ah, one more thing: I don’t need to declare anything as volatile, do I?

The only variable used in both the ISR and the main loop is ScaledRPM, but since it’s only modified in the ISR, and only read in the main loop, it doesn’t need to be volatile, correct?

And although PulseStartTime is a global variable, it isn’t touched outside the ISR so it doesn’t need to be volatile either.

I could also make RPM a local variable, since it’s only used in RPMPulse().

This is where I am assuming the above is true:

int ScaledRPM; //Only modified inside ISR, only read outside ISR
unsigned long ledData; //Variable to be shifted out
unsigned long PulseStartTime; //Only read/modified inside ISR
int latchPin = 8; //Shift register latch pin
int clockPin = 12; //Shift register clock pin
int dataPin = 11; //Shift register data pin


void setup()
{
 pinMode(latchPin, OUTPUT);
 pinMode(clockPin, OUTPUT);
 pinMode(dataPin, OUTPUT);
 attachInterrupt(0, RPMPulse, RISING); // Attaches interrupt to Digi Pin 2
 //Serial.begin(9600);
 
}

void RPMPulse()
{
unsigned long t=micros();
//250 x 24 = 6,000 RPM so we can safely assume:
//Anything over 20,000 RPM is bounce

if ( ( t-PulseStartTime)< 50 )
  return;

unsigned long PulseTime = t - PulseStartTime; // Gets pulse duration
unsigned long RPM = 60000000/PulseTime;       // Calculates RPM 
    
ScaledRPM = RPM/250; 
PulseStartTime=t;
} 

void loop()
{
  //Serial.println(ScaledRPM);
ledData = (1 << ScaledRPM) - 1;
shiftOut(dataPin, clockPin, LSBFIRST, ledData); 
shiftOut(dataPin, clockPin, LSBFIRST, ledData >> 8); //Bitshifting because shiftout only sends 1 byte at a time
shiftOut(dataPin, clockPin, LSBFIRST, ledData >> 16); 
delay(200);                  // 1 sec delay for debugging output
}

Aryl: Ah, one more thing: I don't need to declare anything as volatile, do I?

The only variable used in both the ISR and the main loop is ScaledRPM, but since it's only modified in the ISR, and only read in the main loop, it doesn't need to be volatile, correct?

No that's not correct. Since ScaledRPM is read in the loop and modified in the ISR, it will need to be declared as volatile :)

...and protected with a critical section.

Just an alternative idea I implemented on a computer controlled drag racing transmission project.

My input came from an MSD box and rather than an opto I went with an RC filter, followed by a Zener to clamp spikes, and then into three Schmitt-trigger inverters and the result was crisp and reliable. I also want to say we used a divide-by-4 counter for some sprint cars.

My decision loop was 100 microseconds and I wanted to get the rpm values on the fly so the ISR populated a ring buffer with the clock interval between the last pulse. The ring buffer meant that I was guaranteed to never collide with the reads in the main loop.

This was done on an 8051 where I could optimize with assembly and I am too new to Arduino to know if this matters but with a buffer size of 8 the head pointer could be managed with bit math because the location of the buffer in RAM was aligned to allow that. Also the ring buffer made it easy to use either decaying average or normal average of the previous 5 reads to smooth out the value. Both methods worked well but I forget which the customer went with in the end.

Anyway, sounds like a fun project. Good luck.

AndyCO

AndyCO: Just an alternative idea I implemented on a computer controlled drag racing transmission project.

My input came from an MSD box and rather than an opto I went with an RC filter, followed by a Zener to clamp spikes, and then into three Schmitt-trigger inverters and the result was crisp and reliable. I also want to say we used a divide-by-4 counter for some sprint cars.

My decision loop was 100 microseconds and I wanted to get the rpm values on the fly so the ISR populated a ring buffer with the clock interval between the last pulse. The ring buffer meant that I was guaranteed to never collide with the reads in the main loop.

This was done on an 8051 where I could optimize with assembly and I am too new to Arduino to know if this matters but with a buffer size of 8 the head pointer could be managed with bit math because the location of the buffer in RAM was aligned to allow that. Also the ring buffer made it easy to use either decaying average or normal average of the previous 5 reads to smooth out the value. Both methods worked well but I forget which the customer went with in the end.

Anyway, sounds like a fun project. Good luck.

AndyCO

Those are great ideas, I didn't know what a ring buffer was until now. I found a few similar projects and I think I'm going to spend a few days reading through them. Thanks!