Using interrupts for RPM, RPM being limited by number of interrupts counted?

Using Arduino Programming Questions
1

I'm having an issue with an RPM code, maybe you can help.
I'm using a hall effect sensor to pickup a keyway on a shaft to give me RPM.
Its triggering an interrupt once per revolution. The MAXIMUM hz for these interrupts is only 150hz.
The hall effect sensor is capable of 100x that at a minimum.

I'm using an Arduino Nano with a screw terminal shield.

The code is as follows:

volatile byte revolutions;
unsigned long rpm;
unsigned long timeold;
unsigned long rpmicros;
unsigned long revs;
void setup()
{
  Serial.begin(115200);
  attachInterrupt(0, rpms, FALLING);

  revolutions = 0;
  timeold = 0;
}

void loop()
{
  if (revolutions >= 5) {
    detachInterrupt(0);
    rpmicros = micros() - timeold;
    revs = (60000000 * revolutions);
    rpm = revs / rpmicros;
    timeold = micros();
    Serial.println(rpm);
    revolutions = 0;
    attachInterrupt(0, rpms, FALLING);
  }
}

void rpms()
{
  revolutions++;
}

The issue I am having is that the maximum RPM it will read seems to be proportional to the number of revolutions in the first line of the loop. If I set it revolutions >= 1, I will get an RPM but will only be able to read pretty much at idle. If I rev it up, the RPM seems to hit a wall and I just get what looks like a Sine wave on the serial plotter.

If I increase it to revolutions >= 3, I can get up to about (don't quote me on the exact number) 1600-1800 RPM before it hits this apparent wall.
At revolutions >= 5, I can get much higher.

At revolutions >= 1, I hit the wall somewhere around 60,000 microseconds per revolution.
60,000 microseconds is only ~17 hz. Shouldn't the Nano be able to operate faster than that???

Is there something in the math that I am missing, or maybe in the programming?

2

Any reason you're not showing us the 'rpms()' ISR?

You're also not protecting accesses to a volatile unsigned long with a critical section.

Edit:
My bad, I see it's a byte. And I now see the ISR. Maybe need more coffee this AM.

3

gfvalvo:
Any reason you're not showing us the 'rpms()' ISR?

You're also not protecting accesses to a volatile unsigned long with a critical section.

Edit:
My bad, I see it's a byte. Should it be bigger?

I edited the code to show the ISR. Apparently I didn't do a proper copy/paste. My apologies.

As for it being bigger, I am not sure. Would that have any effect on the issue I'm having?
The microseconds stored in rpmicros should never be longer than 60,000 microseconds under any circumstance.

As of now, I'm using the volatile byte - revolutions >= 5 in order to be able to read RPMS up to a reasonable range. I'd like to decrease it down to no more than 3 in order to cycle the code faster so I can receive more counts over a specific time frame.

4

There is a series of issues with your technique.

  • You are repetitively attaching/detaching the interrupt. The first line of the if statement should be to 'copy' revolutions and reset it. Don't worry about clearing.

-Once you read a variable that will be reset the reset should be next. rpmicros is computed with timeold, but it is three lines later before timeold is updated. For the most accuracy and less risk, it should be the next line.

  • I see division using long (non-decimal) data types. If you are not very careful you will get truncation and zero results very easily.

How do you figure that rpmicros will never be more than 60,000 microseconds?

5

Have a look at the code in this program that measures the speed and counts the revolutions

const byte fwdPin = 9;
const byte revPin = 10;
const byte potPin = A1;

int potVal;
int pwmVal;

unsigned long revMicros;
unsigned long prevRevMicros;
unsigned long revDuration;
unsigned long revCount;

unsigned long prevDisplayMillis;
unsigned long  displayInterval = 1000;

    // variables for the ISR
volatile unsigned long isrMicros;
volatile unsigned long isrCount;
volatile bool newIsrMicros = false;



void setup() {
    Serial.begin(115200);
    Serial.println("SimpleISRdemo.ino");
    pinMode (fwdPin, OUTPUT);
    pinMode (revPin, OUTPUT);

    isrCount = 0;
    attachInterrupt(0, revDetectorISR, RISING);
}

//==========

void loop() {
    getIsrData();
    if (millis() - prevDisplayMillis >= displayInterval) {
        prevDisplayMillis += displayInterval;
        showData();
        readPot();
        updateMotorSpeed();
    }
}

//===========

 void readPot() {
    potVal = analogRead(potPin);
}

//===========

void updateMotorSpeed() {
    pwmVal = potVal >> 2;

    digitalWrite(revPin,LOW);
    analogWrite(fwdPin, pwmVal);
 }

//===========

void getIsrData() {
    if (newIsrMicros == true) {
        prevRevMicros = revMicros; // save the previous value
        noInterrupts();
            revMicros = isrMicros;
            revCount = isrCount;
            newIsrMicros = false;
        interrupts();
        revDuration = revMicros - prevRevMicros;
    }
}

//===========

void showData() {
    Serial.println();
    Serial.println("===============");
    Serial.print("PWM Val "); Serial.println(pwmVal);
    Serial.print("  Rev Duration ");
    Serial.print(revDuration);
    Serial.print("  Rev Count ");
    Serial.print(revCount);
    Serial.println();
}

//===========

void revDetectorISR() {
    isrMicros = micros();
    isrCount ++;
    newIsrMicros = true;

}

Among other things note how I am using noInterrupts() rather than detachInterrupt() and that the value of micros() is saved within the ISR to capture it as soon as possible after the pulse is detected. Not also that I never bother to set isrCount back to zero.

...R

6

Hexen:
The microseconds stored in rpmicros should never be longer than 60,000 microseconds under any circumstance.

1rpm == 1 per 60 seconds == 1 per 60,000ms == 1 per 60,000,000us per revolution. You count until 5 revolutions, that's 300,000,000us.

7

DKWatson:
1rpm == 1 per 60 seconds == 1 per 60,000ms == 1 per 60,000,000us per revolution. You count until 5 revolutions, that's 300,000,000us.

adwsystems:
How do you figure that rpmicros will never be more than 60,000 microseconds?

Its not possible for the engine to run at less than 1000 RPM. In all honestly, it won't stay running below 1200 RPM in the situation I'm using it for.

1200 RPM = 50,000 microseconds between revolutions/interrupts.
More RPM = less microseconds, so the maximum microseconds stored in rpmicros should never exceed 50,000, and should never be less than 6,000 microseconds under ANY circumstance.

8

The correct way to access volatile variables outside the ISR is to use a critical section:

  noInterrupts (); 
  byte revs = revolutions ;
  revolutions = 0 ;
  interrupts() ;

Never use any other way of guarding such variables, certainly not detach/attachInterrupts,
or you will drop interrupts.

The action of noInterrupts/interrupts pair is to suspend execution of interrupts for a brief
period, not to lose or reset or retrigger them.
And it must be for a very brief period, just enough to access the protected variables.

9

adwsystems:

  • You are repetitively attaching/detaching the interrupt. The first line of the if statement should be to 'copy' revolutions and reset it. Don't worry about clearing.

-Once you read a variable that will be reset the reset should be next. rpmicros is computed with timeold, but it is three lines later before timeold is updated. For the most accuracy and less risk, it should be the next line.

  • I see division using long (non-decimal) data types. If you are not very careful you will get truncation and zero results very easily.

-When I don't detach the interrupt, the RPM I get is unreliable. There is quite a bit more code following the RPM calculations that was irrelevant here (with or without the additional code, the original problem still exists). The issue being that as the code gets longer, the RPM reading is affected unless I detach the interrupt. Also, what do you mean by copying the revolutions? Resetting it back to 0 seems essential to the math involved.

-Accuracy here isn't the issue. I will address that portion later.

-Zero results has never been an issue. It has never occurred.

10

MarkT:
The correct way to access volatile variables outside the ISR is to use a critical section:

  noInterrupts ();

byte revs = revolutions ;
 revolutions = 0 ;
 interrupts() ;




Never use any other way of guarding such variables, certainly not detach/attachInterrupts,
or you will drop interrupts.

The action of noInterrupts/interrupts pair is to suspend execution of interrupts for a brief
period, not to lose or reset or retrigger them. 
And it must be for a very brief period, just enough to access the protected variables.

Interesting. How brief of a period?
I'm also doing many other calculations after I get the RPM number. I'm taking readings from a load cell, a pair of 5v analog sensors, doing math on about a dozen variables, etc. All of these involving that RPM number calculated. Should I do these before or after interrupts() in the above code?

11

Hexen:
1200 RPM = 50,000 microseconds between revolutions/interrupts.
More RPM = less microseconds, so the maximum microseconds stored in rpmicros should never exceed 50,000, and should never be less than 6,000 microseconds under ANY circumstance.

Fair enough, but that's still only timing 1 revolution. If you don't perform the calculations in loop until revolutions >= n, then rpmicros will be n * whatever you think the maximum should never exceed. In this instance it probably will never matter, but your thinking that rpmicros should never exceed 50,000 is not valid and at slower speeds with the number of revolutions set too high, you're going to get overflow. If you're not testing at high rpm, the values being reported may not be reliable.

12

DKWatson:
Fair enough, but that's still only timing 1 revolution. If you don't perform the calculations in loop until revolutions >= n, then rpmicros will be n * whatever you think the maximum should never exceed. In this instance it probably will never matter, but your thinking that rpmicros should never exceed 50,000 is not valid and at slower speeds with the number of revolutions set too high, you're going to get overflow. If you're not testing at high rpm, the values being reported may not be reliable.

But the issue here is the opposite. I've tested it at something like 4200 RPM with the revolution count between 5 and 7. Even at 7 revolutions, that's 100,000 microseconds. It has NO problem doing this.

When I attempt to calculate RPM from a single revolution, the further I DECREASE the microseconds, the more of an issue I have. At 1 revolution, I cannot get the RPM variable to report anything over 1200 RPM, or less than 50,000 microseconds. I can rev the engine out to 4200, the rpm variable will not display over 1200.

13

Robin2:
Have a look at the code in this program that measures the speed and counts the revolutions

const byte fwdPin = 9;

const byte revPin = 10;
const byte potPin = A1;

int potVal;
int pwmVal;

unsigned long revMicros;
unsigned long prevRevMicros;
unsigned long revDuration;
unsigned long revCount;

unsigned long prevDisplayMillis;
unsigned long  displayInterval = 1000;

// variables for the ISR
volatile unsigned long isrMicros;
volatile unsigned long isrCount;
volatile bool newIsrMicros = false;

void setup() {
    Serial.begin(115200);
    Serial.println("SimpleISRdemo.ino");
    pinMode (fwdPin, OUTPUT);
    pinMode (revPin, OUTPUT);

isrCount = 0;
    attachInterrupt(0, revDetectorISR, RISING);
}

//==========

void loop() {
    getIsrData();
    if (millis() - prevDisplayMillis >= displayInterval) {
        prevDisplayMillis += displayInterval;
        showData();
        readPot();
        updateMotorSpeed();
    }
}

//===========

void readPot() {
    potVal = analogRead(potPin);
}

//===========

void updateMotorSpeed() {
    pwmVal = potVal >> 2;

digitalWrite(revPin,LOW);
    analogWrite(fwdPin, pwmVal);
}

//===========

void getIsrData() {
    if (newIsrMicros == true) {
        prevRevMicros = revMicros; // save the previous value
        noInterrupts();
            revMicros = isrMicros;
            revCount = isrCount;
            newIsrMicros = false;
        interrupts();
        revDuration = revMicros - prevRevMicros;
    }
}

//===========

void showData() {
    Serial.println();
    Serial.println("===============");
    Serial.print("PWM Val "); Serial.println(pwmVal);
    Serial.print("  Rev Duration ");
    Serial.print(revDuration);
    Serial.print("  Rev Count ");
    Serial.print(revCount);
    Serial.println();
}

//===========

void revDetectorISR() {
    isrMicros = micros();
    isrCount ++;
    newIsrMicros = true;

}




Among other things note how I am using noInterrupts() rather than detachInterrupt() and that the value of micros() is saved within the ISR to capture it as soon as possible after the pulse is detected. Not also that I never bother to set isrCount back to zero.

...R

Doesn't work here. You're providing a set interval by which to display the RPM. In my circumstance, I need this interval to be as quickly as possible, which is why I'm using every X revolutions.

I'll switch to noInterrupts() to see if that helps my situation.

14

Okay, so now we need to look at what's generating the interrupt. Have you confidence in the hall effect sensor picking up the signal?

15

Hexen:
Doesn't work here. You're providing a set interval by which to display the RPM. In my circumstance, I need this interval to be as quickly as possible.

Why? What can a human do with a numeric display that's updating several times per second?

16

Another thing to think about is to feed the sensor into T1 (pin5 on the Uno/Nano) and use timer/counter1 to count pulses. You can then set a match or overflow interrupt based on a preset number of revolutions and mark the time between for your calculations. This, I think, would be more accurate that timing the inter-pulse gaps.

17

DKWatson:
Okay, so now we need to look at what's generating the interrupt. Have you confidence in the hall effect sensor picking up the signal?

It works 100% flawlessly. I get a smooth and accurate RPM reading, until I hit the imaginary wall that I posted about. Increasing the revolution count before calculations are done raises the maximum possible RPM stored in the variable. Accuracy seems to be unaffected in any way.

gfvalvo:
Why? What can a human do with a numeric display that's updating several times per second?

Datalogging.

18

Hexen:
It works 100% flawlessly. I get a smooth and accurate RPM reading, until I hit the imaginary wall that I posted about. Increasing the revolution count before calculations are done raises the maximum possible RPM stored in the variable. Accuracy seems to be unaffected in any way.

Datalogging.

How about some serial print statements and posting what the numbers are?

19

Hexen:
Datalogging.

OK. I think I'd put that aside for the moment and just try the code provided by @Robin2 in Reply #4. It corrects the multiple flaws that have been pointed out in your implementation. Get the basic functionality working first, then add the bells and whistles.

As you build up the application you may find that something you just added breaks what's already working. But, at least you can address it because you know the last thing you added.

20

adwsystems:
How about some serial print statements and posting what the numbers are?

I've done this, and used the serial plotter.
The RPM reads perfectly. My issue is the "wall" I'm hitting by using less revolutions to determine RPM.