continuing my DAQ/ feed back system found at the link.
After double checking my logic for an engine rpm, i cant quite seem to find my error.
engine is a briggs and stratton 10hp, min rpm is set at 1800, and max is 3800. arduino should be able to keep up with this frequency when pulling off the coil.
int E_rpm = 0;
unsigned long lastmillis = 0;
unsigned long currmillis = 0;
int Update = 1000; //change to update faster or slower
void setup(){
Serial.begin(9600);
attachInterrupt(0, e_rpm, FALLING); //pin 2
}
void loop(){
currmillis = millis();
if (currmillis - lastmillis >= Update){ //Uptade every one second, this will be equal to reading frecuency (Hz).
detachInterrupt(0);//Disable interrupt when calculating
E_rpm = E_rpm * (currmillis - lastmillis); // Convert frecuency to RPM, note: this works for one interruption per full rotation. For two interrups per full rotation use rpmcount * 30.
Serial.print("RPM =\t"); //print the word "RPM" and tab.
Serial.print(E_rpm); // print the rpm value.
Serial.print(",");
Serial.print(millis());
Serial.print("\n");
E_rpm = 0; // Reset the engine RPM counter
lastmillis = millis(); // Uptade lasmillis
attachInterrupt(0, e_rpm, FALLING); //enable interrupt (interupt #, code to run, when to run it)
}
}
// this code will be executed every time the interrupt 0 (pin2) gets low.
void e_rpm(){
E_rpm++;
}
A function named the same as a variable (even with different case) is going to cause confusion. Re-using that variable that was storing counts to now contain something else is even more confusing.
Attaching and detaching an interrupt is considered to be a slow and expensive operation. For code where timing is so critical that you have to use interrupts, this expense is usually prohibitive. Get used to interrupts() and noInterrupts(). Grab a copy of the volatile variable (oh dear, you didn't use volatile did you?) and re enable the interrupts as quickly as possible. You can usually do this so no events are lost or missed.
Hi,
If you go and look where E_rpm is updated, by the interrupt, its not RPM, its the number of interrupts since E_rpm was last zeroed.
E_rpm is not the term that I would be using, ShaftCount or something like that would be more appropriate as RPM hasn't been calculated yet.
void e_rpm() is probably not the right term either, ShaftCountUpdate probably more explanitary.
The sketch makes more sense if you rename E_rpm and void e_rpm().
Although that truncates down to the nearest integer, but you can live with that.
You must lose the detachInterrupt nonsense - the interrupt must be there all the time
or it will not count correctly. Do the calculation in the ISR and write the result to
a (volatile) variable.
In loop you can read that variable whenever you need it, but guard the read against
interrupts:
noInterrupts () ;
my_rpm = rpm ; // grab copy of volatile while ISR is held-off
interrupts () ;
Although that truncates down to the nearest integer, but you can live with that.
Don't you think that somewhere in the calculation of RPM should be the number of revolutions?
Do the calculation in the ISR and write the result to
a (volatile) variable.
I disagree. The detach/attach bit is nonsense, but the calculation of RPM should NOT be done in the ISR. The ISR should only count pulses (revolutions).
So, after some tweaking and investigation this is the code that i came up with:
#include <math.h>
volatile double E_counts = 0;
double e_counts;
double e_rpm;
unsigned long lastmillis = 0;
unsigned long currmillis = 0;
int Update = 1000; //change to update faster or slower
void setup(){
Serial.begin(9600);
attachInterrupt(0, e_count, FALLING); //pin 2
}
void loop(){
currmillis = millis();
if (currmillis - lastmillis >= Update){ //Uptade every one second, this will be equal to reading frecuency (Hz).
noInterrupts () ;//Disable interrupt when calculating
e_counts = E_counts;
E_counts = 0; // Reset the engine revolution counter
interrupts () ; //enable interrupt (interupt #, code to run, when to run it)
e_rpm = (e_counts / (currmillis - lastmillis)); // Convert counts to rev per ms
lastmillis = millis(); // Uptade lasmillis
e_rpm = e_rpm / .0000166; //convert rev per ms to min
//Serial.print(currmillis - lastmillis);
//Serial.print("RPM =\t"); //print the word "RPM" and tab.
Serial.print(e_rpm); // print the rpm value.
Serial.print(",");
Serial.print(millis());
Serial.print("\n");
}
}
// this code will be executed every time the interrupt 0 (pin2) gets low.
void e_count(){
E_counts++;
Serial.print("*");
Serial.print(E_counts);
Serial.print("\n");
}
from the sound of the engine and a store bought tiny tack, I believe these readings are close, but I would like to add in a running average. IE it builds up an array of 10, and then does the average. then 0=1, 1=2,and so forth then does another average.
PaulS:
Don't you think that somewhere in the calculation of RPM should be the number of revolutions?
Yes, its 1 since that's how many revolutions happen between one interrupt and the next. I did
foul up though, it should be currentmicros and prevmicros and 60000000L: microsecond granularity is needed here.
the calculation of RPM should NOT be done in the ISR. The ISR should only count pulses (revolutions).
Probably wrong - you want to disable interrupts for as short as possible to avoid adding jitter to the readings,
so only passing back one int instead of two longs will help. Doing a long divide in the ISR isn't very onerous,
and it cannot affect the timing of the next interrupt unless the rpm is extremely large (millions).
After some digging around, and some pointers from previous project managers, this is what I have come up with for a final engine rpm code, and wanted some other eyes to look it over:
#include <math.h>
volatile double E_rev = 0;
unsigned long engine_time;
unsigned long last_engine_time;
double e_counts;
double e_rpm;
unsigned long lastmillis = 0;
unsigned long currmillis = 0;
int Update = 1000; //change to update faster or slower
void setup(){
Serial.begin(9600);
attachInterrupt(0, e_count, FALLING); //pin 2
}
void loop(){
currmillis = millis();
if (currmillis - lastmillis >= Update){ //Uptade every one second, this will be equal to reading frecuency (Hz).
noInterrupts () ;//Disable interrupt when calculating
e_counts = E_rev;
E_rev = 0; // Reset the engine revolution counter
interrupts () ; //enable interrupt (interupt #, code to run, when to run it)
e_rpm = (e_counts / (currmillis - lastmillis)); // Convert counts to rev per ms
lastmillis = millis(); // Uptade lasmillis
e_rpm = e_rpm / .0000166; //convert rev per ms to min
//e_rpm = e_rpm / 4;
//Serial.print(currmillis - lastmillis);
//Serial.print("RPM =\t"); //print the word "RPM" and tab.
Serial.print(e_rpm); // print the rpm value.
Serial.print(",");
Serial.print(millis());
Serial.print("\n");
}
}
// this code will be executed every time the interrupt 0 (pin2) gets low.
void e_count(){
engine_time = micros();
//debounce engine rpm reader
if (engine_time - last_engine_time > 1400) //15873 microseconds @ 3800 rpm
{
E_rev++;
last_engine_time = engine_time;
}
}
Please keep in mind that this is my first time working with interrupts and really any arduino project altogether.
The next step is to migrate this into an acceleration timer, monitoring both engine and rear wheel speeds and logging them to an sd card along side the time. for clutch tuning purposes.
Here's an interesting excercise, what is 1,000,000,000,000.00 + 1? In the computer it's equal to 1,000,000,000,000.00 The "+1" is lost in the precision.
Double also requires a lot of bytes. Since the AVR Arduinos can only work on 8 bits at a time, adding 1 to a double takes a lot of processor cycles. This is not appropriate for an interrupt.
Integers are for counting. Add one to an integer and you will always get a result (it may not be the result you were expecting however.) Integers come in different sizes depending on what you expect to be counting. If you expect that you will always be able to read out that value at least every 32767 counts, then a "normal" int is the best type to use.
from my little bit of a C class, im just used to using double all the time because it didnt matter much with what we were processing, nor did i have to care about memory usage. These arduinos have very little memory in that respect. In regards to the overflow and loosing the +1, in the counter, that "wouldn't" happen as the most it should ever be counting to is 63.
But saving space is critical knowing what is to come next. This should be correct then?
volatile int E_rev = 0;
unsigned long engine_time;
unsigned long last_engine_time;
int e_counts;
double e_rpm;
