Measuring car RPM is giving weird answers

This one is leaving me stumped… a little precursory info:

My car has a tach pin in the ECM which can be enabled via software to put out a square wave that triggers off the crank pulley teeth in the car. The pulley is a 58x tooth so there are 58 pulses per revolution and I have set the ECM to be HIGH for 29 and LOW for 29 so there should be 1 rising edge or falling edge per revolution and I have confirmed the signal is relatively clean via scope (see attached). My arduino code is basically set up to utilize the interrupt pin to trigger off the edge and time the period between edges in order to calculate the engine RPM. Simple in principle. In practice it’s been a nightmare. Somehow my math isn’t working because the values are much bigger than I would have expected (off by a factor of 20 or so) and I’m getting negative numbers also which, despite my efforts to eliminate with if statements, still keep showing up. Very odd. I think there may be some sort of overflow problem or my understanding of how to time interrupt events is incorrect or the noise on the square wave is triggering the interrupt prematurely. I really don’t know what the next step should be as I’ve been randomly trying things to no avail. Anybody see what’s F’d with my code?

#include "SPI.h"
#include "Adafruit_GFX.h"
#include "Adafruit_ILI9341.h"

// For the Adafruit shield, these are the default.
#define TFT_DC 9
#define TFT_CS 10

// Use hardware SPI (on Uno, #13, #12, #11) and the above for CS/DC
Adafruit_ILI9341 tft = Adafruit_ILI9341(TFT_CS, TFT_DC);
// If using the breakout, change pins as desired
//Adafruit_ILI9341 tft = Adafruit_ILI9341(TFT_CS, TFT_DC, TFT_MOSI, TFT_CLK, TFT_RST, TFT_MISO);

const unsigned char picture[] PROGMEM = {

//blah blah blah massive bitmap

  };


byte RPMp = 3;

int MAP = 0;
int TPS = 0;
int KR = 0;
int AFR = 0;
double RPM = 0; 
byte a = 0;

unsigned long currenttime = 0;
unsigned long interval = 0;


void setup() {
  
  pinMode(RPMp, INPUT_PULLUP);
  
  tft.begin();

}

void loop() {

  tft.setRotation(3);

  tft.drawPixel(16, 16, ILI9341_WHITE);
  delay(3000);  
  tft.fillScreen(ILI9341_BLACK);

//blah blah blah this is all unrelated graphics stuff

  sensorTest();

}


void sensorTest() {
  
  double avgrpm[10];
  double finalrpm = 0;
  tft.setTextColor(ILI9341_WHITE);
  tft.setTextSize(2);

  currenttime = micros();
  attachInterrupt(1, interruptRoutine, FALLING);
  while(1){

    tft.setCursor(0, 0);    
    tft.println((float)analogRead(A7)*5/1023); 
    tft.println((float)analogRead(A6)*5/1023); 
    tft.println((float)analogRead(A5)*5/1023); 
    tft.println((float)analogRead(A4)*5*2.375/1023+6.0625); 
    
    RPM = (double)interval*0.000001;
    RPM = 3/RPM;
    finalrpm = 0;
    for(int b = 0; b<10; b++) {
  
      finalrpm += (double)avgrpm[b];

    }
    finalrpm *= 0.1;
    tft.println((int)finalrpm); 
    delay(100);
    tft.fillRect(0, 0, 60, 80, ILI9341_BLACK);
    if(a>9)
      a=0;   
  }

}


void interruptRoutine() {
  interval = micros() - currenttime;
  currenttime += interval;
  a++;
}

Sorry, found a couple more obvious bugs and the latest code is now giving wrong answers but a little less broken? in the sense they are consistently positive now and only off by an order of magnitude but still bouncing around between a few hundred and few thousand RPMs when the real engine speed is pretty stable at around 700.

… I feel like the arduino is triggering incorrectly off the square wave. Otherwise why would the numbers be so erratic?

void sensorTest() {
  
  unsigned int avgrpm[] = {0,0,0,0,0,0,0,0,0,0};
  unsigned int finalrpm = 0;
  tft.setTextColor(ILI9341_WHITE);
  tft.setTextSize(2);

  currenttime = micros();
  attachInterrupt(1, interruptRoutine, FALLING);
  while(1){

    tft.setCursor(0, 0);    
    tft.println((float)analogRead(A7)*5/1023); 
    tft.println((float)analogRead(A6)*5/1023); 
    tft.println((float)analogRead(A5)*5/1023); 
    tft.println((float)analogRead(A4)*5*2.375/1023+6.3625); 
    
    RPM = (double)interval*0.000001;
    RPM = 60/RPM;
    avgrpm[a] = (unsigned int)RPM;
    finalrpm = 0;
    for(int b = 0; b<10; b++) {
  
      finalrpm += avgrpm[b];

    }
    finalrpm *= 0.1;
    tft.println(finalrpm); 
    delay(100);
    tft.fillRect(0, 0, 80, 80, ILI9341_BLACK);
    if(a>9)
      a=0;   
  }

}


void interruptRoutine() {
  interval = micros() - currenttime;
  currenttime += interval;
  a++;
}

You are breaking some basic rules. http://www.gammon.com.au/interrupts

are you sure you are capturing every interrupt?

delay(100);

here is another approach:

// CLASS DEFINITION

class FanSpeed {
  
  public:
    void 
      setup(uint8_t irq_pin, void (*ISR_callback)(void), int value),
      handleInterrupt(void);
    double
      getSpeed(),
      getHertz();

  private:
    double
      _timeConstant = 60000000.0;
    uint32_t
      _lastMicros = 0UL,
      _interval = 60000000UL;
    void(*ISR_callback)();
};

void FanSpeed::setup(uint8_t irq_pin, void (*ISR_callback)(void), int value)
{
  attachInterrupt(digitalPinToInterrupt(irq_pin), ISR_callback, value);
}

inline void FanSpeed::handleInterrupt(void)
{
  uint32_t nowMicros = micros();
  _interval = nowMicros - _lastMicros;
  _lastMicros = nowMicros;
}

double FanSpeed::getSpeed()
{
  if (micros() - _lastMicros < 1000000UL) // has rotated in the last second
  {
    return _timeConstant / _interval;
  }
  else
  {
    return 0;
  }   
}

double FanSpeed::getHertz()
{
  if (micros() - _lastMicros < 1000000UL) // has rotated in the last second
  {
    return getSpeed() * 60.0;
  }
  else
  {
    return 0;
  }   
}

// PROGRAM START

FanSpeed* fan1;
uint8_t fan1pin = 2;

FanSpeed* fan2;
uint8_t fan2pin = 3;

void setup()
{
  Serial.begin(9600);
  pinMode(fan1pin, INPUT_PULLUP);
  fan1 = new FanSpeed();
  fan1->setup(fan1pin, []{fan1->handleInterrupt();}, FALLING);
  fan2 = new FanSpeed();
  fan2->setup(fan2pin, []{fan2->handleInterrupt();}, FALLING);
}

void loop()
{
  static uint32_t lastMillis = 0;
  if (millis() - lastMillis > 1000UL)
  {
    char message[64] = "";
    sprintf(message, "Fan1 Speed:%4d RPM\t%4d Hz", uint32_t(floor(fan1->getSpeed() + 0.5)), uint32_t(floor(fan1->getHertz() + 0.5)));
    Serial.println(message);
    sprintf(message, "Fan2 Speed:%4d RPM\t%4d Hz", uint32_t(floor(fan2->getSpeed() + 0.5)), uint32_t(floor(fan2->getHertz() + 0.5)));
    Serial.println(message);
    lastMillis = millis();
  }
}

of course you only need one FanSpeed object…

Aarg: Are you suggesting that the rule I'm breaking is that I didn't declare the variables in the interrupt "volatile" ?

Bulldog: The delay was because if I didn't use it, it was really hard to read the screen because the refresh was so fast the numbers would appear to be a blur and I could never see a number long enough to read it. I'm assuming the problem with delay is that is uses the same timer that micros() does, thus screwing up the counting?

Gahhhrrrlic: Aarg: Are you suggesting that the rule I'm breaking is that I didn't declare the variables in the interrupt "volatile" ?

Bulldog: The delay was because if I didn't use it, it was really hard to read the screen because the refresh was so fast the numbers would appear to be a blur and I could never see a number long enough to read it. I'm assuming the problem with delay is that is uses the same timer that micros() does, thus screwing up the counting?

my guess is that a may be incremented twice in your while loop... which begs the question of why you are using that structure, but that probably isn't your problem...

my example leaves the interrupt to do the work, and you just get the last RPM value each time you update your display... kind-of-thing.

Gahhhrrrlic: Aarg: Are you suggesting that the rule I'm breaking is that I didn't declare the variables in the interrupt "volatile" ?

if (and only if) the compiler believes that in the flow of your program you are not using a variable, the compiler may optimize it away...

since the main() program calls setup() and loop() but not your ISR, that could be a problem as the optimization occurs. The compiler may believe that this variable is never used. because it cannot "see" that this function is used (as in your case, it is triggered by a hardware event).

to prevent this kind of optimization, you declare a variable volatile; the optimization will no longer affect this variable.

HOWEVER, if you are in fact using those variables in the context of your program flow, such variables may not actually be optimized out. So, your use of the variables in the context of the ISR and within functions guaranteed to be called, may have eliminated the need to protect the variables from the compiler's appetite for optimization.

You should however keep in convention and assert that those variables are protected by simply using the volatile keyword. This helps you indicate that someone else may come along and change this value (at any time).

I hope that makes sense!

Ok that makes perfect sense to me.

I fixed the delay thing and the volatile now so my code looks like this. I’m going to go and try it in the car:

byte RPMp = 3;

int MAP = 0;
int TPS = 0;
int KR = 0;
int AFR = 0;
double RPM = 0.0; 
byte a = 0;

volatile unsigned long currenttime = 0;
volatile unsigned int interval = 0;
unsigned long delaycounter = 0;
void sensorTest() {
 
 unsigned int avgrpm[] = {0,0,0,0,0,0,0,0,0,0};
 unsigned int finalrpm = 0;
 tft.setTextColor(ILI9341_WHITE);
 tft.setTextSize(2);

 currenttime = micros();
 attachInterrupt(1, interruptRoutine, FALLING);
 while(1){

   tft.setCursor(0, 0);    
   tft.println((float)analogRead(A7)*5/1023); 
   tft.println((float)analogRead(A6)*5/1023); 
   tft.println((float)analogRead(A5)*5/1023); 
   tft.println((float)analogRead(A4)*5*2.375/1023+6.3625); 
   
   RPM = (double)interval*0.000001;
   RPM = 60/RPM;
   avgrpm[a] = (unsigned int)RPM;
   finalrpm = 0;
   for(int b = 0; b<10; b++) {
 
     finalrpm += avgrpm[b];

   }
   finalrpm *= 0.1;
   tft.println(finalrpm); 
   delaycounter = micros();
   while((micros() - delaycounter) < 100000) {
   }

   delaycounter = 0;
   tft.fillRect(0, 0, 80, 80, ILI9341_BLACK);
   if(a>9)
     a=0;   
 }

}


void interruptRoutine() {
 interval = micros() - currenttime;
 currenttime += interval;
 a++;
}

isn’t this:

while((micros() - delaycounter) < 100000) {
    }

just a long-winded this:

delay(100);

they both block…

my point was that if a is incremented in your ISR, (and that WILL happen if your ISR is called during a blocking delay like the above two, you could miss an increment of the a when you get back to your array management…

Oh I get it. Ok so basically I'm going to have to find another way to update my screen that doesn't involve tying up the rest of the loop. Maybe a counter that allows the screen to refresh every 1000 loops or something.

As expected, the results were no better in-car. Values still vary randomly between a few hundred and a few thousand. I'll try restructuring the code for the screen refresh.

look at the loop() function in the example I provided…

void loop()
{
  static uint32_t lastMillis = 0;
  if (millis() - lastMillis > 1000UL)  //<<<<<<<< updates once per second...
  {
    char message[64] = "";
    sprintf(message, "Fan1 Speed:%4d RPM\t%4d Hz", uint32_t(floor(fan1->getSpeed() + 0.5)), uint32_t(floor(fan1->getHertz() + 0.5)));
    Serial.println(message);
    sprintf(message, "Fan2 Speed:%4d RPM\t%4d Hz", uint32_t(floor(fan2->getSpeed() + 0.5)), uint32_t(floor(fan2->getHertz() + 0.5)));
    Serial.println(message);
    lastMillis = millis();
  }
}

Got it and implemented such a technique.

… so I accidentally nuked my nano by shorting a couple pins and setting fire to the regulator. This accident was not in vane though because it forced me to use another nano, which I have verified exhibits the exact same behaviour (randomly fluctuating RPM readings). At this point I really think the interrupts are getting triggered by edges that it is “seeing”, despite the source signal being pretty decent. I’m not sure what I can do about this. I take it there’s no way to use hardware interrupts that will trigger off some custom voltage level so I’m stuck with whatever it thinks is “HIGH” and “LOW”. I could put a capacitor on the interrupt pin to ground but I’m not sure that will reliably fix the problem either. Any recommendations at this point?

Code:

void sensorTest() {
 
 unsigned int avgrpm[] = {0,0,0,0,0,0,0,0,0,0};
 unsigned int finalrpm = 0;
 tft.setTextColor(ILI9341_WHITE);
 tft.setTextSize(2);

 currenttime = micros();
 attachInterrupt(1, interruptRoutine, FALLING);
 while(1){
   
   RPM = (double)interval*0.000001;
   RPM = 60/RPM;
   avgrpm[a] = (unsigned int)RPM;
   finalrpm = 0;
   for(int b = 0; b<10; b++) {
 
     finalrpm += avgrpm[b];

   }
   finalrpm *= 0.1;

   if((micros() - delaycounter) > 100000) {

     tft.fillRect(0, 0, 64, 80, ILI9341_BLACK);
     tft.setCursor(0, 0);    
     tft.println((float)analogRead(A7)*5/1023); 
     tft.println((float)analogRead(A6)*5/1023); 
     tft.println((float)analogRead(A5)*5/1023); 
     tft.println((float)analogRead(A4)*5*2.375/1023+6.3625);       
     tft.println(finalrpm);

     delaycounter = micros();      
     
   }

   if(a>9)
     a=0;   
 }

}


void interruptRoutine() {
 interval = micros() - currenttime;
 currenttime += interval;
 a++;
}

oscilloscope give you a nice square wave?

A picture of it is attached in my first post. I don’t know if I’d call it “good” but I’ve seen worse. The average returned value on the display is 4000 something (although it bounces around and rarely stays on that number). 4000 something RPM would be 75’ish RPS when it should really be about 10 so it’s triggering about 7 times more often than it should… and everything in between. I’m wondering if there could be a DC offset, shifting the wave too low or too high.

Oh shit, just had a thought… what if I feed the input into the comparator pins and then output a square wave from the comparator to the interrupt pin? Then I could set my own threshold voltage?

Code:

That is NOT all of your code. Quit wasting our time posting snippets.

Hi,

time the period between edges in order to calculate the engine RPM

Why dont you count pulses over a fixed time period, then calculate the RPM.
As you RPM goes up, the period goes down, you need to process faster and faster between pulses.

If you count over say 1Second, calculate( x 60), display, then go back and count again.’

Have you used a signal generator as an RPM source to check your code algorithm with a clean square-wave signal?

Tom… :slight_smile:

PaulS: That is NOT all of your code. Quit wasting our time posting snippets.

On the contrary. I figured it would be wasteful of people's time to read all the graphical garbage I have filling the sketch prior to the sensor function being called. Also, I suggest you not allow yourself to be my slave, which is what you're doing here, if you think that I am somehow governing the usage of your time.

TomGeorge: I like your suggestion about the square wave test. I will have to see if the hardware I have can be set up to do this. I'm sure I can rig my laptop spk port with some effort. As for timing pulses, I'm not sure what the advantage would be, as I'd still need to keep track of the interrupt events and count them. If the interrupt hardware is twitchy or the signal is not clean, I suspect the count or the period will be wrong until I fix it. Am I understanding your suggestion correctly?

Hi, If you count pulses, with interrupts, and you calculate every second, you only have to multiply the count by 60 to get RPM, and you can do it ALL in integer.

Have to written code that just outputs to the serial port, forget your display stuff?

Just write some code that JUST inputs the pulses and converts them to RPM and displays on the serial port.

If you wrote the code in stages you should have been through this stage. OR did you try and write it ALL at once?

Tom... :)

I understand the algorithm you're describing but what I'm seeing is that the interrupts are happening more often than they should so irrespective of HOW you calculate the pulse frequency (whether by the period method I'm using or the counting method you're describing), both strategies depend on the interrupts occurring at the right times. If the car's ECM is putting out a square wave at 10Hz and the interrupt routine is firing at 70 Hz, changing the code won't affect the results. I have a suspicion that there is either a DC offset or sufficient noise to hit the interrupt threshold value prematurely so I think your earlier suggestion to test a clean square wave is the most prudent call. If my hunch is right, I may have to abandon basic interrupts altogether and use the comparator's interrupt vector instead because at least that way, the arduino is looking for a single voltage threshold rather than a range from HIGH to LOW. Then I can use a pot or something to figure out what the perfect voltage ref ought to be, so that the peaks and valleys of my square wave are equidistant from it.

Hi,
bf12b83cb8c3fb70e2ccba5ce640e4f5d99e2482.png
The OPs Trace.

Have you got gnd of the arduino and gnd of the ECM connected?
Are you using a x10 probe?
Have you got the channel in AC or DC mode?
300mV per division, that pulse is not very big.

How have you got the Arduino and the ECM connected?

Thanks… Tom… :slight_smile: