Tachometer fixer - Guidance needed

I am making a arduino Rbbb, that should fix my tachometer in my car.
I have put a 5 cyl engine in a car that normally had a 4 cyl engine.

The tacho gets its signal from the input of the ignition driver, a square-wave 5v/0v signal.

In the testing i use a UNO for making the signal.

The problem:
It preforms nicely until about 7000 Rpm., from there to about 8000 Rpm. it gets fuzzy, and when i simulate 10.000 Rpm. it just outputs 29.000 Rpm.

I am no skilled programmer, but i believe i get the basics.
But i have kinda hit a wall here, any pointers would be great. Also if i have made any "don't"s in the code.

the code:

unsigned long input_millis = 0; // time at interrupt 
unsigned long diff_millis = 0; // input cycle time
unsigned long new_millis = 0; // output cycle time (divided by 2) (pin 13)
unsigned long last_cycle = 0; // time last cycle pin change
// unsigned long lastmillis = 0; // time last updated (serial display) to be commented out 

const int OutPin =  13; // the output pin for the new signal

int OutState = LOW; // the current status of the output pin


void setup(){

 pinMode(OutPin, OUTPUT); 
// Serial.begin(9600); 
 attachInterrupt(0, ign_sig, FALLING); // the input from ECU - pin 2
}

void loop(){
  
  if(millis() - last_cycle > new_millis) {
    last_cycle = millis();   
    if (OutState == LOW)
      OutState = HIGH;
    else
      OutState = LOW;
    digitalWrite(OutPin, OutState);
  }

// if (millis() - lastmillis == 1000){
// detachInterrupt(0); //Disable interrupt when calculating
// Serial.print("input millis =\t"); 
// Serial.print(input_millis) ;
// Serial.print("\t new millis =\t"); 
// Serial.print(new_millis) ;
// Serial.print("\t diff millis =\t");  
// Serial.println(diff_millis); 
// lastmillis = millis(); // Uptade lasmillis
// attachInterrupt(0, ign_sig, FALLING); //enable interrupt
// }

}

 // this code will be executed every time the interrupt 0 (pin2) gets low.
void ign_sig(){

  diff_millis = (millis() - input_millis);
  new_millis = (diff_millis / 2 / 4 * 5); 
  input_millis = millis();
}

Thanks in advance, Jesper

First thing I see is that if you change any variables inside the ISR, they must be declared volatile. So diff_millis, new_millis, and input_millis must be declared volatile.

The tacho gets its signal from the input of the ignition driver, a square-wave 5v/0v signalThe tacho gets its signal from the input of the ignition driver, a square-wave 5v/0v signal

Would this be one pulse per revolution, or one pulse per spark? 5 cylinder engines are not something I see every day. I’m trying to figure out how the relationship between spark and engine revolution is handled. If it’s a 4 cylinder engine, I would assume 4 sparks per revolution, with the starting cylinder in any revolution being the cylinder after the last spark in the previous revolution. Bends my brain! You post has made me thankful my tractor’s 3 cylinder engine is a diesel. :~

Just for information, what is an Rbbb?

Thanks, i will try.
Is the a guide for looking these things up, i am often just taking a guess when declaring :roll_eyes:

A 4 cyl 4-stroke have 4 ignitions pr 2 revs, this just have 5 pr 2 revs.

A Rbbb is this http://moderndevice.com/product/rbbb-kit/ , The engine is running a MegaSquirt ecu, so when finished, it will be placed inside the ecu.

First thing I see is that if you change any variables inside the ISR, they must be declared volatile. So diff_millis, new_millis, and input_millis must be declared volatile.

No a variable modified in the ISR only needs to be volatile IF IT IS USED OUTSIDE THE ISR.

@OP Is the signal one pulse per rev or is it a pwm where the duty cycle changes with the number of rev.

If it is a pulse per second then the ISR is just

void ign_sig(){
recCounter++;
  
}

You just count the recs with the ISr and then in loop work out the revs per second.

Mark

Those RPMs are a bit eye-watering!

It looks as if you are measuring the elapsed time between successive interrupts and then scaling that to determine the interval on the output side. That's a reasonable strategy, but the input interval at max RPM is going to be a very small number of ms and I think you aren't going to have the timing resolution you need. You will probably need to use micros() instead.

The variables shared between the interrupt context and the main context need to be declared volatile.

I haven't quite followed what this line of code is intended to do:

  new_millis = (diff_millis / 2 / 4 * 5);

Can you explain? In any case it would be better to call millis() once and save the result, rather than call it multiple times. Similarly, in loop it would be better to increment last_cycle by new_millis rather than call millis() again. The code might be easier to follow if the variable names were a bit more meaningful, btw.

No a variable modified in the ISR only needs to be volatile IF IT IS USED OUTSIDE THE ISR.

Of course! .

void ign_sig(){

recCounter++;
  }

I agree with this, and with the need to use micros(). At 10,000 RPM, that’s 166.666… RPS, or 6 ms per revolution. Since we get 5 sparks (and, I assume, 5 input pulses) in 2 revolutions, that’s 5 pulses in 12 ms, or 2.4 ms (2400 microseconds) between pulses.

So, if we just count pulses in an ISR, and count for a fixed duration (using millis() or micros() ) in the main loop, we should be able to calculate an RPM quite easily, right?

I just had a thought (I know, I know!)... Anyway, the pulse repetition rate/time, is a function of RPM. I think all that's required is to find the time between two pulses, and you have the period, from which to calculate the RPM. Gathering timing over a longer period is not necessary.

void loop() {
   unsigned long temp = pulseIn(ingitionPin, LOW, timeout);
   unsigned long start = micros();
   temp = pulseIn(ingitionPin, LOW, timeout); 
   unsigned long end = micros;
   unsigned long duration = end - start;
   int RPM = calcRPM(duration);
   showRPM(RPM);
}

int  calcRPM(unsigned long period) {
   // calculate RPM from duration
}

void showRPM(int RPM) {
    // display RPM in whatever form desired
   // perhaps gather a number of samples, and keep a running average
}

Notes:
temp is probably not needed, as all we are interested in is when the pulse goes low.
start records this time. Same for end
We have found the period between falling edges, so assign to duration
Should give very fast results without gathering data over a long period.
The calculation of RPM may or may not include some sort of averaging/smoothing
I've left the calculation and display of the RPM as an excercise for the OP.