Reading rpm program gets stucked

Using Arduino Programming Questions
1

Hello, this is my first time posting on the forum. So, i'm trying to make an SPI injection for a Trabant engine. I have never programmed before so i do not have extended programming skills. That's the code i have written till now

int injector = 13; //pin injector
int ledrosu = 3; // pin program
int hallinjectie = A0; //pin semnal hall injectie
int pompa = 4; // pompa relay signal
volatile int rpmcount = 0;
int rpm = 0;
unsigned long lastmillis = 0;

int hallState=0;

void setup() {
pinMode(injector,OUTPUT);
pinMode(ledrosu,OUTPUT);
pinMode(hallinjectie,INPUT);
pinMode(pompa,OUTPUT);
digitalWrite(pompa,HIGH);
Serial.begin(9600);
attachInterrupt(0,rpm_fan, FALLING);
}

void loop(){
hallState = analogRead(hallinjectie);

if (hallState == LOW) {
digitalWrite(injector, HIGH);
delay(4);
}
else {
digitalWrite(injector, LOW);}

if (millis() - lastmillis == 1000){ /Uptade every one second, this will be equal to reading frecuency (Hz)./

detachInterrupt(0);//Disable interrupt when calculating

rpm = rpmcount * 60; /* 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(rpm); // print the rpm value.
Serial.print("\t Hz=\t"); //print the word "Hz".
Serial.println(rpmcount); /print revolutions per second or Hz. And print new line or enter./

rpmcount = 0; // Restart the RPM counter
lastmillis = millis(); // Uptade lasmillis
attachInterrupt(0, rpm_fan, FALLING); //enable interrupt

}

}
void rpm_fan(){ /* this code will be executed every time the interrupt 0 (pin2) gets low.*/
rpmcount++;
}

It works fine till now but, when i'm triyng to spin the magnet around both hall effect sensors, the serial display gets blocked immediatly showing just one value. After that it won't work neither separatly. To make it work again i have to unplug the power source from the board.
P.s. i know i'm new into programming and building an ecu is a very hard project but this is what i wanna learn even for the future, so i will make almost any effort to learn.

poza arduino.jpg
poza arduino.jpg1080×1920 194 KB

2

if (millis() - lastmillis == 1000)Not a good idea in case you miss the millisecond where the difference is exactly 1000.
if (millis() - lastmillis >= 1000)would be safer.

Whilst it is good practice to prevent an interrupt occurring during a multi byte calculation it is not necessary, or advisable, to detach and attach the interrupt. Rather, use nointerrupts() and interrupts() to manage the interrupts but reenable them before you print anything.

3

Building from what @PaulS @UKHeliBob has said i suggest that instead of

detachInterrupt(0);//Disable interrupt when calculating

rpm = rpmcount * 60;

//etc

you do something like this

prevRPMcount = latestRPMcount;
noInterrupts();
   latestRPMcount = rpmCount;
interrupts();
RPMcountThisInterval = latestRPMcount - prevRPMcount;

and then use the value in RPMcountThisInterval for all your calculations

Note that there is no need to reset the value of rpmCount. However it would be necessary to define it as

volatile unsigned int rpmCount = 0;

This means there is very little time when interrupts are disabled and the only thing changing the value of rpmCount is the code in the ISR.

...R

4

Building from what @PaulS has said i suggest that instead of

Damn !
You have discovered my alter ego

5

Thank you vrey much guys, it helped me a lot. Now it is working fine, no more crashes. But i would like you to help me with one more thing. How can a get my measurements more accurate? I mean, now the program is reading the Hz and the rpm is going everytime up/down of 60, it will never show a result like 1234 RPM but just the result of the number of Hz multiplied by 60. ( example 1 Hz > 160=60rpm; 5Hz >560=300rpm)

Now with the appropiate modifications given by @Robin2 and @UKEHeliBob the code looks like this:

int injector = 13; //injecton pin trough mosfet
int ledrosu = 3; // will indicate that program start working.The engine can be started
int hallinjectie = A0; //injection hall sensor pin input signal
int pompa = 4; //  pump relay signal
volatile int rpmcount = 0;
int rpm = 0;
unsigned long lastmillis = 0;
volatile unsigned int rpmCount = 0;
unsigned int prevRPMcount=0;
unsigned int latestRPMcount=0;

int hallState=0;

void setup() {
  pinMode(injector,OUTPUT);
  pinMode(ledrosu,OUTPUT);
  pinMode(hallinjectie,INPUT);
  pinMode(pompa,OUTPUT);
  digitalWrite(pompa,HIGH);
  Serial.begin(9600);
attachInterrupt(0,rpm_fan, FALLING);
}

void loop(){
hallState = analogRead(hallinjectie); 

  if (hallState == LOW) {        
    digitalWrite(injector, HIGH); 
     delay(4); 
  }
    else {
    digitalWrite(injector, LOW);} 

    if (millis() - lastmillis >=1000){ /*Uptade every one second, this will be equal to reading frecuency (Hz).*/


prevRPMcount = latestRPMcount;
noInterrupts();
   latestRPMcount = rpmCount;
interrupts();
int RPMcountThisInterval = latestRPMcount - prevRPMcount;

rpm = rpmcount * 60; /* 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(rpm); // print the rpm value.
Serial.print("\t Hz=\t"); //print the word "Hz".
Serial.println(rpmcount); /*print revolutions per second or Hz. And print new line or enter.*/

rpmcount = 0; // Restart the RPM counter
lastmillis = millis(); // Uptade lasmillis
attachInterrupt(0, rpm_fan, FALLING); //enable interrupt

}
 
    }       
    void rpm_fan(){ /* this code will be executed every time the interrupt 0 (pin2) gets low.*/
rpmcount++;
}
6

You basically have two choices if you want single digit RPM - either count for a minute and display or measure the time between pulses and calculate RPM from that - the first is more accurate, the second can be faster, but is susceptible to jitter in the pulse generator. (there is a third way that comes to mind involving a phase locked loop, but that is more hardware). That said, you probably really do not want single rpm resolution because nothing runs at exactly the same speed, so the display will be constantly changing. There are some applications where I prefer an analog type meter for the display - RPM is one of them

7

Put on your math/design hat and ask yourself the following question: How long would it take to know that the RPM is exactly 1234 RPM? How many pulses would you have to count?

8

aarg:
Put on your math/design hat and ask yourself the following question: How long would it take to know that the RPM is exactly 1234 RPM? How many pulses would you have to count?

This is not hard it is gonna be 20.56 pulses but the now program is reading entire pulses... 1,2,3,...5....20,21....n pulses.

9

UKHeliBob:
Damn !
You have discovered my alter ego

I'm real sorry about that. It is something I am usually very careful about.

...R

10

moskyno93:
This is not hard it is gonna be 20.56 pulses but the now program is reading entire pulses... 1,2,3,...5....20,21....n pulses.

Rather than count the number of pulses in time T I suggest you measure the time for N pulses.

...R