Need help with measuring RPM code.

Hello, I have the following code in which when a NPN transistor turns on from the pulses of a magneto, digital pin 4 gets pulled low through the transistor and I count the milliseconds in between each time the pin is falling/pulled low.

The problem is, in the serial output I'm getting many values that are totally wrong along with very few that are totally correct. I need help to try and figure out the code to ignore these values. I am not looking for a hardware based solution. I can confirm that the values are NOT garbage or random:

What I think is happening, there are 3 magnets on the flywheel, so this causes the transistor to pull pin 4 low very quickly in between each magnet which gives a very high RPM reading in the serial output but is wrong because this wouldn't be a full revolution; the 3 magnets are close together.

I will include the serial output, showing the correct RPM readings, which there are very few of compared to of the ones I need to figure out how to ignore.

Thank you for the help!

Program code:

#include <PinChangeInt.h>

#define ENGINE_KILL_FLAG 1

#define RPM_INPUT_PIN 4

uint16_t rpmTimeoutInterval = 1000;
volatile unsigned long previousRPMFallTimeMillis = 0;
volatile uint16_t engineRPM = 0;

volatile uint8_t bUpdateFlagsShared;

volatile uint16_t unEngineKillInShared;

uint16_t unEngineKillStartPeriod;

void setup()
{
  Serial.begin(9600); 
  
  PCintPort::attachInterrupt(RPM_INPUT_PIN, calcRPMInput, FALLING);
}

void loop()
{
  static uint16_t unEngineKillIn;

  static uint8_t bUpdateFlags;

  if (bUpdateFlagsShared)
  {
    noInterrupts();
    
    bUpdateFlags = bUpdateFlagsShared;
   
    if (bUpdateFlags & ENGINE_KILL_FLAG)
    {
      unEngineKillIn = unEngineKillInShared;
    }

    bUpdateFlagsShared = 0;
   
    interrupts(); 
  }

  unsigned long currentMillis = millis();
  
  if (currentMillis - previousRPMFallTimeMillis > rpmTimeoutInterval)
  {
    engineRPM = 0;
  }

  Serial.println(engineRPM);
  
  bUpdateFlags = 0;
}

void calcRPMInput()
{
  unsigned long currentMillis = millis();

  uint16_t pulseDuration = currentMillis - previousRPMFallTimeMillis;
  
  if (pulseDuration <= rpmTimeoutInterval)
  {
    // 60,000 milliseconds in one minute
    
    uint16_t rpm = 60000 / pulseDuration;
   
    engineRPM = rpm;
  }    

  previousRPMFallTimeMillis = currentMillis;
}

Serial output:

0 <-- Correct RPM, aka engine not started
0 <-- Correct RPM
0 <-- Correct RPM
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
2068
329 <-- Correct RPM
329 <-- Correct RPM
329 <-- Correct RPM
329 <-- Correct RPM
2727
2727
2727
2727
2727
2727
2727
2727
2727
2727
2727
2727
2727
2727
2727
2727
2727
2727
2727
2727
2727
2727
2727
434 <-- Correct RPM
434 <-- Correct RPM
434 <-- Correct RPM
3157
3157
3157
3157
3157
3157
3157
3157

Should probably test your system for slower speeds to begin with. To make sure it at least works nicely at low speeds..... and then focus on high speed after that.

Could also try micros() instead of millis()

And, could set the serial speed to something higher... Serial.begin(115200);

Southpark:
Should probably test your system for slower speeds to begin with. To make sure it at least works nicely at low speeds..... and then focus on high speed after that.

Could also try micros() instead of millis()

And, could set the serial speed to something higher... Serial.begin(115200);

I do not follow you, how are speeds the problem? The RPM readings works fine, the issue is the other magnets cause the transistor to pull pin 4 low before a full revolution happens (I think at least). Appreciate any help :slight_smile:

aviatorken:
the 3 magnets are close together

So that implies there's a lot of magneto not covered with magnets. Why not use some debounce type code and look for a 'quiet' time and take your reading off that?

dougp:
So that implies there's a lot of magneto not covered with magnets. Why not use some debounce type code and look for a 'quiet' time and take your reading off that?

It's a lawnmower flywheel so the magneto isn't covered with magnets, the flywheel has 3 magnets spaced apart about 1/4" with North South North polarity I'm pretty sure.

aviatorken:
It's a lawnmower flywheel so the magneto isn't covered with magnets, the flywheel has 3 magnets spaced apart about 1/4" with North South North polarity I'm pretty sure.

That sounds like there will be a long gap followed by 3 pulses.

Just count the pulses and when the 3rd pulse is detected record the time and set the count back to 0.

...R

Robin2:
That sounds like there will be a long gap followed by 3 pulses.

Just count the pulses and when the 3rd pulse is detected record the time and set the count back to 0.

...R

It doesn't work out to be that simple though, as you can see from the serial output, you can sometimes get 1 to 6 valid rpm readings so there wouldn't be an accurate way of counting since all of the data isn't getting back. Also, what if the flywheel position is in the middle of the 3 magnets upon startup, then the count would be completely wrong.

aviatorken:
It doesn't work out to be that simple though, as you can see from the serial output, you can sometimes get 1 to 6 valid rpm readings so there wouldn't be an accurate way of counting since all of the data isn't getting back. Also, what if the flywheel position is in the middle of the 3 magnets upon startup, then the count would be completely wrong.

Any chance of removing or demagnetising 2 of those magnets?

Southpark:
Any chance of removing or demagnetising 2 of those magnets?

Hah, not a chance lol. The engine is governed to 3600 as per ANSI law, so I could probably safely ignore bogus values over 3750 rpm or so.

aviatorken:
Hah, not a chance lol.

hahaha! Ok.... could maybe experiment with the timing then..... like.... get the arduino to detect 1 pulse to begin with...and set the time of the occurrence of that pulse as a reference time. Then... if any more pulses are detected with X microseconds ..... then ignore those pulses But if another pulse comes along AFTER X (threshold) microseconds, then take that as the valid pulse reading..... and record the time...(eg. stop time).

Then take the difference...stop time minus reference time.... to get a time difference. And estimate the angular speed from that. Then start all over again.

It's basically a debounce sort of thing. Keep twiddling with the value of X until you get good readings all the time.

Southpark:
hahaha! Ok.... could maybe experiment with the timing then..... like.... get the arduino to detect 1 pulse to begin with...and set the time of the occurrence of that pulse as a reference time. Then... if any more pulses are detected with X microseconds ..... then ignore those pulses But if another pulse comes along AFTER X (threshold) microseconds, then take that as the valid pulse reading..... and record the time...(eg. stop time).

Then take the difference...stop time minus reference time.... to get a time difference. And estimate the angular speed from that. Then start all over again.

It's basically a debounce sort of thing. Keep twiddling with the value of X until you get good readings all the time.

Gotcha! Your suggestion on upping the baud rate and switching to micros() seems to have helped! I've only tested it on the breadboard and computer with me manually touching a wire to the transistor's base to create the pulses for now. So far so good though! :slight_smile:

aviatorken:
It doesn't work out to be that simple though, as you can see from the serial output, you can sometimes get 1 to 6 valid rpm readings so there wouldn't be an accurate way of counting since all of the data isn't getting back.

Then the solution seems to be to measure your timing based on the first pulse after the long gap. I think that is the solution suggested by @Southpark. In a similar bouncing situation I used the pulseISR to turn itself off and start a hardware timer to generate another ISR at about the half-revolution stage. The timerISR was used to turn the pulseISR back on ready for when the next pulse comes. This worked well on a very small DC motor running at 12,000 or 15,000 RPM.

Also, what if the flywheel position is in the middle of the 3 magnets upon startup, then the count would be completely wrong.

That can also be sorted by looking for the long gap.

...R

PS Another option might be to use an optical detector that reacts to a spot of white paint on the flywheel or shaft.

I'm still trying to wrap my head around figuring out how to determine the right duration of time to record. Using a hall effect sensor to help me figure out the magnet on the flywheel, I determined that the pattern is: FALLING (1st magnet leading edge), RISING (1st magnet trailing edge), FALLING (3rd magnet leading edge), RISING (3rd magnet trailing edge).

The middle/2nd magnet does nothing, I believe it's only there to strengthen/couple the field to the ignition coil.

So really I need to figure out how to find the time window between the RISING of the 3rd magnet to the FALLING of the 1st magnet, but I do not know how due to to the same pattern is created between the 1st and 3rd magnets which is what I need to avoid for false readings.

The only thing I can think of quickly is to record 3 High to Low periods and pick the one with the greatest duration, but this might be flawed? Just an idea that came to my head that caused me to edit this post. Or record 2 Low to Low periods and pick the one with the longest duration?

Edit 2: added current code, basically waiting for 3 pulses to occur and do subtraction to find the longest duration between the two groups: pulse 1 and 2, and pulse 2 and 3. This may be flawed logic though? Do I need to wait for 4 pulses to occur and add a 3rd group, pulses 3 and 4?

The below code still gives me false readings, I cannot seem to grasp a solution.

Help is appreciated :slight_smile:

Code:

#include <PinChangeInt.h>

#define ENGINE_KILL_FLAG 1

#define RPM_INPUT_PIN 4

unsigned long rpmTimeoutInterval = 1000000;

volatile unsigned long rpmPulse1Micros = 0;
volatile unsigned long rpmPulse2Micros = 0;
volatile unsigned long rpmPulse3Micros = 0;

volatile uint16_t volatileEngineRPM = 0;

volatile uint8_t bUpdateFlagsShared;
volatile uint8_t rpmPulseCounter = 0;

volatile uint16_t unEngineKillInShared;

uint16_t unEngineKillStartPeriod;

void setup()
{
  Serial.begin(115200); 

  PCintPort::attachInterrupt(RPM_INPUT_PIN, calcRPMInput, FALLING);
}

void loop()
{
  static uint16_t unEngineKillIn;

  static uint8_t bUpdateFlags;

  if (bUpdateFlagsShared)
  {
    noInterrupts();
    
    bUpdateFlags = bUpdateFlagsShared;
   
    if (bUpdateFlags & ENGINE_KILL_FLAG)
    {
      unEngineKillIn = unEngineKillInShared;
    }

    bUpdateFlagsShared = 0;
   
    interrupts(); 
  }

  uint16_t engineRPM = volatileEngineRPM;

  unsigned long rpmPulse3MicrosCopy = rpmPulse3Micros;

  if (micros() - rpmPulse3MicrosCopy > rpmTimeoutInterval)
  {
    engineRPM = 0;
  }

  Serial.println(engineRPM);

  bUpdateFlags = 0;
}

void calcRPMInput()
{
  rpmPulseCounter++;

  if (rpmPulseCounter == 1)
  {
     rpmPulse1Micros = micros();
  }
  else if (rpmPulseCounter == 2)
  {
     rpmPulse2Micros = micros();
  }

  if (rpmPulseCounter == 3)
  {
    rpmPulseCounter = 0;

    rpmPulse3Micros = micros();

    unsigned long pulse1and2Duration = rpmPulse2Micros - rpmPulse1Micros;
    unsigned long pulse2and3Duration = rpmPulse3Micros - rpmPulse2Micros;
    unsigned long pulseDuration = pulse2and3Duration;
    
    if (pulse1and2Duration > pulse2and3Duration)
    {
      pulseDuration = pulse1and2Duration;
    } 

    if (pulseDuration >= 16000 && pulseDuration <= rpmTimeoutInterval)
    {
      // 60,000,000 microseconds in one minute

      uint16_t rpm = 60000000 / pulseDuration;
   
      volatileEngineRPM = rpm;
    }
  }
}

aviatorken:
I'm still trying to wrap my head around figuring out how to determine the right duration of time to record. Using a hall effect sensor to help me figure out the magnet on the flywheel, I determined that the pattern is: FALLING (1st magnet leading edge), RISING (1st magnet trailing edge), FALLING (3rd magnet leading edge), RISING (3rd magnet trailing edge).

The middle/2nd magnet does nothing, I believe it's only there to strengthen/couple the field to the ignition coil.

So really I need to figure out how to find the time window between the RISING of the 3rd magnet to the FALLING of the 1st magnet, but I do not know how due to to the same pattern is created between the 1st and 3rd magnets which is what I need to avoid for false readings.

The only thing I can think of quickly is to record 3 High to Low periods and pick the one with the greatest duration, but this might be flawed? Just an idea that came to my head that caused me to edit this post. Or record 2 Low to Low periods and pick the one with the longest duration?

Edit 2: added current code, basically waiting for 3 pulses to occur and do subtraction to find the longest duration between the two groups: pulse 1 and 2, and pulse 2 and 3. This may be flawed logic though? Do I need to wait for 4 pulses to occur and add a 3rd group, pulses 3 and 4?

The below code still gives me false readings, I cannot seem to grasp a solution.

Help is appreciated :slight_smile:

Code:

#include <PinChangeInt.h>

#define ENGINE_KILL_FLAG 1

#define RPM_INPUT_PIN 4

unsigned long rpmTimeoutInterval = 1000000;

volatile unsigned long rpmPulse1Micros = 0;
volatile unsigned long rpmPulse2Micros = 0;
volatile unsigned long rpmPulse3Micros = 0;

volatile uint16_t volatileEngineRPM = 0;

volatile uint8_t bUpdateFlagsShared;
volatile uint8_t rpmPulseCounter = 0;

volatile uint16_t unEngineKillInShared;

uint16_t unEngineKillStartPeriod;

void setup()
{
 Serial.begin(115200);

PCintPort::attachInterrupt(RPM_INPUT_PIN, calcRPMInput, FALLING);
}

void loop()
{
 static uint16_t unEngineKillIn;

static uint8_t bUpdateFlags;

if (bUpdateFlagsShared)
 {
   noInterrupts();
   
   bUpdateFlags = bUpdateFlagsShared;
 
   if (bUpdateFlags & ENGINE_KILL_FLAG)
   {
     unEngineKillIn = unEngineKillInShared;
   }

bUpdateFlagsShared = 0;
 
   interrupts();
 }

uint16_t engineRPM = volatileEngineRPM;

unsigned long rpmPulse3MicrosCopy = rpmPulse3Micros;

if (micros() - rpmPulse3MicrosCopy > rpmTimeoutInterval)
 {
   engineRPM = 0;
 }

Serial.println(engineRPM);

bUpdateFlags = 0;
}

void calcRPMInput()
{
 rpmPulseCounter++;

if (rpmPulseCounter == 1)
 {
    rpmPulse1Micros = micros();
 }
 else if (rpmPulseCounter == 2)
 {
    rpmPulse2Micros = micros();
 }

if (rpmPulseCounter == 3)
 {
   rpmPulseCounter = 0;

rpmPulse3Micros = micros();

unsigned long pulse1and2Duration = rpmPulse2Micros - rpmPulse1Micros;
   unsigned long pulse2and3Duration = rpmPulse3Micros - rpmPulse2Micros;
   unsigned long pulseDuration = pulse2and3Duration;
   
   if (pulse1and2Duration > pulse2and3Duration)
   {
     pulseDuration = pulse1and2Duration;
   }

if (pulseDuration >= 16000 && pulseDuration <= rpmTimeoutInterval)
   {
     // 60,000,000 microseconds in one minute

uint16_t rpm = 60000000 / pulseDuration;
 
     volatileEngineRPM = rpm;
   }
 }
}

Why not count the number of pulses in a full revolution before storing time?
Example Code:

#include <PinChangeInt.h>

// My Encoder has 1 Clock pulses per revolution
// note that 60000000.0 = (60 seonds * 1000000 microseconds)microseconds in a minute / 1 pulses in 1 revolution)
#define Multiplier 60000000.0 // don't forget a decimal place to make this number a floating point number
#define #define RPM_INPUT_PIN 4 
//#define RPM_INPUT_PIN 2 // My Uno only has pins 2 and 3 as interrupt pins with My compile testing 


#define PulsesPerRevolution 3 // how many pulses does your magnets trigger per revolution

volatile unsigned long dTime; // Delt in time
volatile byte Readings;
volatile byte Pos;
volatile long SpeedInRPM[10];
void setup() {
  // put your setup code here, to run once:
  Serial.begin(9600);

  PCintPort::attachInterrupt(RPM_INPUT_PIN, calcRPMInput, FALLING);
//  attachInterrupt(digitalPinToInterrupt(RPM_INPUT_PIN), calcRPMInput, FALLING); // this is what I used for the compile test
}

void loop() {
  // put your main code here, to run repeatedly:
  static unsigned long _ATimer;
  if ((millis() - _ATimer) >= (100)) { // see what we have 10 times a second
    _ATimer = millis();
    Serial.print("RPM:");
    Serial.print(SpeedInRPM[(Pos-1) %10]);

    int Val;
    long sum;
    int MinRPM;
    int MaxRPM;
    for (char i = 0; i < Readings; i++) {
      Val = SpeedInRPM[(i) % 10];
      sum += Val;
      if (i < 10) {
        MinRPM = min(Val, MinRPM);
        MaxRPM = max(Val, MaxRPM);
      }
    }

    Serial.print(" Min:");
    Serial.print(MinRPM);
    Serial.print(" Max:");
    Serial.print(MaxRPM);
    int Average = sum / Readings;
    Serial.print(" Average:");
    Serial.print(Average);
    Pos = 0;
    Readings = 0;

  }
}

void calcRPMInput(uint32_t Time, uint32_t PinsChanged, uint32_t Pins) {
  static byte ctr;
  static uint32_t lTime; // Saved Last Time of Last Pulse
  ctr++;
  if (ctr >= PulsesPerRevolution) { // We have counted enough times to make a full revolution of the flywheele with wird magnet positions
    uint32_t cTime; // Current Time
    cTime = micros();// Lets get the time first.
    ctr = 0;
    dTime= cTime - lTime;; // Delt in time Store 10 readings for averaging
    if (Readings < 10) Readings++; // So we average correctly
    lTime = cTime;
    SpeedInRPM[Pos++ %10] = Multiplier / dTime; // Calculate the RPM after we have reached 1 revolution worth of pulses
    /*what does this do : 
     * Pos++ %10 
     * first it gets the current value of Pos and returns it.
     * afterwords it increments it by 1 
     * then to prevent overflowing our array we test it 
     * by getting the remainder of the value returned 
     * by Pos which is a number between 0 and 9 (10 Spots)
     * there is an issue with rollover but we reset the Pos counter 
     * it long before we get there.
     */
  }
}

Threw in min max and average for testing this
I compiled it but not tested.
Z

aviatorken:
I'm still trying to wrap my head around figuring out how to determine the right duration of time to record. Using a hall effect sensor to help me figure out the magnet on the flywheel, I determined that the pattern is: FALLING (1st magnet leading edge), RISING (1st magnet trailing edge), FALLING (3rd magnet leading edge), RISING (3rd magnet trailing edge).

The middle/2nd magnet does nothing, I believe it's only there to strengthen/couple the field to the ignition coil.

So really I need to figure out how to find the time window between the RISING of the 3rd magnet to the FALLING of the 1st magnet, but I do not know how due to to the same pattern is created between the 1st and 3rd magnets which is what I need to avoid for false readings.

If this was my project I would save the time at the first FALLING and then I would wait the amount of time for about a half revolution before testing for the next FALLING which ought to be the first one of the next group no matter what triggered the initial one.

...R

Is it possible to just detect the first edge, and record the time... t_ref = micros()..... and then .... if you detect any other edges, where those OTHER edges fall within a certain amount of time of t_ref..... then ignore those other edges. Eg.... if you detect a pulse, current time MINUS t_ref <= 100 microseconds .... then don't do anything.

Eventually, another edge will come where the time gap will be quite large (relative to t_ref).... ie. occurs at time current time MINUS t_ref > 100 microsecond.... then record the stop time ... ie stop_time = micros().

===

UPDATE:

Robin2:
If this was my project I would save the time at the first FALLING and then I would wait the amount of time for about a half revolution before testing for the next FALLING which ought to be the first one of the next group no matter what triggered the initial one.

...R

I agree with Robin2.

I think what's also happening, is that at slower speeds, when the magnets are moving between the fixed coil armature (letter U shaped), there are many HIGH and LOW periods which causes the transistor connected to pin 4 to turn on and off giving the false readings. I suppose a capacitor would fix alot of this, but was looking for a pure software approach if possible.

Southpark:
Is it possible to just detect the first edge, and record the time... t_ref = micros()..... and then .... if you detect any other edges, where those OTHER edges fall within a certain amount of time of t_ref..... then ignore those other edges. Eg.... if you detect a pulse, current time MINUS t_ref <= 100 microseconds .... then don't do anything.

Eventually, another edge will come where the time gap will be quite large (relative to t_ref).... ie. occurs at time current time MINUS t_ref > 100 microsecond.... then record the stop time ... ie stop_time = micros().

===

UPDATE:

I agree with Robin2.

I just tried this using what you suggest of ignoring readings of pulses < X microseconds; the problem is, the engine can go to 3600 rpm when running normally, so I can still get false readings even at a slowish cranking/electric starting speed of 600rpm.

aviatorken:
I just tried this using what you suggest of ignoring readings of pulses < X microseconds; the problem is, the engine can go to 3600 rpm when ignited, so I can still get false readings even at a slowish cranking/starting speed of 600rpm.

You need to explain exactly how you did that (i.e. post your code). I know that what I recommended in Reply #14 works because I have used it myself.

...R

Robin2:
You need to explain exactly how you did that (i.e. post your code). I know that what I recommended in Reply #14 works because I have used it myself.

...R

Sure, no problem. Sorry I forgot to include it :slight_smile:

Code:

#include <PinChangeInt.h>

#define ENGINE_KILL_FLAG 1

#define RPM_INPUT_PIN 4

unsigned long rpmTimeoutInterval = 1000000;

volatile unsigned long rpmFirstEdgeMicros = 0;
volatile unsigned long lastRPMReadingMicros = 0;

volatile uint16_t volatileEngineRPM = 0;

volatile uint8_t bUpdateFlagsShared;
volatile uint8_t rpmPulseCounter = 0;

volatile uint16_t unEngineKillInShared;

void setup()
{
  Serial.begin(115200); 

  PCintPort::attachInterrupt(RPM_INPUT_PIN, calcRPMInput, FALLING);
}

void loop()
{
  static uint16_t unEngineKillIn;

  static uint8_t bUpdateFlags;

  if (bUpdateFlagsShared)
  {
    noInterrupts();
    
    bUpdateFlags = bUpdateFlagsShared;
   
    if (bUpdateFlags & ENGINE_KILL_FLAG)
    {
      unEngineKillIn = unEngineKillInShared;
    }

    bUpdateFlagsShared = 0;
   
    interrupts(); 
  }

  uint16_t engineRPM = volatileEngineRPM;

  unsigned long rpmMicrosCopy = lastRPMReadingMicros;

  if (micros() - rpmMicrosCopy > rpmTimeoutInterval)
  {
    engineRPM = 0;
  }

  bUpdateFlags = 0;
}

void calcRPMInput()
{
  rpmPulseCounter++;

  if (rpmPulseCounter == 1)
  {
    rpmFirstEdgeMicros = micros();
  }

  if (rpmPulseCounter > 1)
  {
    unsigned long duration = micros() - rpmFirstEdgeMicros;

    if (duration > 16000)
    {
      if (duration <= rpmTimeoutInterval)
      {
        // 60,000,000 microseconds in one minute

        uint16_t rpm = 60000000 / duration;

        volatileEngineRPM = rpm;

        lastRPMReadingMicros = micros();
      }

      rpmPulseCounter = 0;
    }
  }
}