Help with this RPM code

Hi i have this rpm code that is working... i can display the values in my oled screen... but the values are not right

it counts the rpm of car engine through brake points of distributor
every one revolution the distributor has 4 brake points (4 stroke - 4 cylinder car )

thats why i have place '' rpm = (15 * breakNum); '' in my code above (60/4 = 15)

the problem is that when i have the code to update rpm every 1 sec

 //Update RPM every second
   delay(1000);

i get a number almost twice the real rpms

when i make the delay to delay(300); i got rpm lower than the real ones

(i measure the rpms with a normal digital car tachometer as an reference to my project)

is that normal?? any idea about what is wrong with this code and how to make it more accurate ?

/* Gia dokimi me othoni mono strofometro */

#include "U8glib.h"

U8GLIB_SSD1306_128X64 u8g(U8G_I2C_OPT_NO_ACK);  // Display which does not send AC

////////////////////////////////////////////////////// RPM /////////////////////////////////////////////////////////////////////
volatile byte breakNum;        // "volatile" is used with interrupts
unsigned int rpm;

// Counts the number of interrupts
void break_count()
 {
      breakNum++;
 }
/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////



void setup(void) {
 
// assign default color value
  if (u8g.getMode() == U8G_MODE_R3G3B2)
  {
    u8g.setColorIndex(255);     // white
  }
  else if (u8g.getMode() == U8G_MODE_GRAY2BIT)
  {
    u8g.setColorIndex(3);         // max intensity
  }
  else if (u8g.getMode() == U8G_MODE_BW)
  {
    u8g.setColorIndex(1);         // pixel on
  }
  else if (u8g.getMode() == U8G_MODE_HICOLOR)
  {
    u8g.setHiColorByRGB(255, 255, 255);
  }
  
//////////////////////////////////////////////////////////////////////////// RPM //////////////////////////////////////////////


   attachInterrupt(0, break_count, FALLING);
  // attachInterrupt(0, break_count, RISING);

      breakNum = 0;
      rpm = 0;
  
}

//////////////////////////////////////////////////////////////////////////// RPM //////////////////////////////////////////////

void Rpm(void) { 
  u8g.setFont(u8g_font_fub11);
  u8g.setFontRefHeightExtendedText();
  u8g.setDefaultForegroundColor();
  u8g.setFontPosTop();
  u8g.drawStr(4, 40, "RPM");
  u8g.setPrintPos(70, 40); 
  u8g.print(rpm);  
}


void loop(void) {
  

  
////////////////////////////////// RPM SERIAL MONITOR////////////////////////////  
// Update RPM every second
   delay(1000);
   // Don't process interrupts during calculations
   detachInterrupt(0);
  // Depending on what you are testing you might need to change the formula for the rpm
  // For instance testing a prop would give 2 breaks per rotation so (60 * rpmcount) / 2
  // rpm = (60 * breakNum / 4 );
   rpm = (15 * breakNum);
   //rpm = (60 * breakNum);
   //rpm = (240 * breakNum);  // test for big numbers in display
   
 
  
   breakNum = 0;

     
   //Restart the interrupt processing
   attachInterrupt(0, break_count, FALLING);
  // attachInterrupt(0, break_count, RISING);

/////////////////////////////////////////////////////////////////////////

  
  
  
  // picture loop
  u8g.firstPage();  
  do {
    Rpm();   //mpainei to onoma tou void me to display
  } while( u8g.nextPage() );
  
  // rebuild the picture after some delay
  //delay(50);
 

 
}

thanks in advance

Remember, a distributor shaft turns at half the speed of the crankshaft for a four stroke engine.

Thank you very much remind me this.. i had totaly forgot it when i was calculating the type of my code..

so i have to rise the number

thanks

i will post the results when i try it with the new update to my sketch

A few things ...
You should use noInterrupts() and interrupts() to pause the interrupts rather than detaching and attaching.

If it was my project I would not bother setting breakNum back to zero. Just subtract the previous value from the latest value to get the number in the interval.

I think you would get more accurate data if you use the ISR to record the value of micros() when each pulse is received and calculate the speed from the number of microseconds between pulses.

The use of delay() seems like it will work in your example but ...
It may not be as accurate as you think because you also need to take account of the time taken by the rest of your code. The difference may not be great with an interval of 1000 msecs.

More importantly, if you start adding extra features to your program the use of delay() is likely to get in the way. Have a look at how millis() is used to manage timing without blocking in several things at a time. Using millis() will automatically take account of the time required for other parts of your code.

...R

``Thank you both for the help (sorry for the delay)

i tried some of your suggestions about the noInterrupts()
the problem is that with the code i have posted doesnt work fine with what i want to do.. reads different values from normal rpm... and if i fix the numbers to be accurate in low rpm than get wrong readings in higher rpm and the opposite

i find an already rpm code with mills
the code is from seanauff (thanks) and the his project can be find in GitHub

i cant find out how to display the values of getrpm() in the below code to my oled screen (i want it that in order to have visual display of rpm to test it if it works well with the car... without having to bring with me my laptop)

the code with oled display :

#include "U8glib.h"
U8GLIB_SSD1306_128X64 u8g(U8G_I2C_OPT_NO_ACK);  // Display which does not send AC

////////////////////////////////////////////////////
const byte tachInterrupt = 0; // tach signal on interrupt 0 (digital pin 1)
byte engineCylinders = 4; // for tach calculation (pulses per revolution = 2 * cylinders / cycles)
byte engineCycles = 4; // for tach calculation
int refreshInterval = 750; // milliseconds between sensor updates
unsigned long previousMillis = 0;
const byte tachPin = 0; // tach signal on digital pin 1 (interrupt 0)
volatile int RPMpulses = 0;





void setup()
{
/////////////////////////////////// OLED/////////////////////////////////////

u8g.setRot180();
   if (u8g.getMode() == U8G_MODE_R3G3B2)
  {
    u8g.setColorIndex(255);    
  }
  else if (u8g.getMode() == U8G_MODE_GRAY2BIT)
  {
    u8g.setColorIndex(3);        
  }
  else if (u8g.getMode() == U8G_MODE_BW)
  {
    u8g.setColorIndex(1);        
  }
  else if (u8g.getMode() == U8G_MODE_HICOLOR)
  {
    u8g.setHiColorByRGB(255, 255, 255);
  }  

 ///////////////////////////////////////////////////////////////////////////////// 
  
  pinMode(tachPin, INPUT_PULLUP); // enable internal pullup for tach pin
  attachInterrupt(tachInterrupt, countRPM, FALLING);
  Serial.begin(9600);
}

//////////////////////////////////////////////RPM//////////////////////////////
void Rpm(void) { 
  u8g.setFont(u8g_font_fub11);
  u8g.setFontRefHeightExtendedText();
  u8g.setDefaultForegroundColor();
  u8g.setFontPosTop();
  u8g.drawStr(4, 20, "RPM");
  u8g.setPrintPos(70, 20); 
  u8g.print(getRPM());  
}

////////////////////////////////////////////////////////////////////////////////////////////

void loop()
{
  if(millis() - previousMillis > refreshInterval)
  {
    previousMillis = millis();
    Serial.println(getRPM());
    
  }
}

// counts tach pulses
void countRPM()
{
  RPMpulses++;
}

// checks accumulated tach signal pulses and calculates engine speed
// returns engine speed in RPM
// Resolution: 30000 * engineCycles / refreshInterval / engineCylinders RPM (for default values = 20 RPM)
int getRPM()
{
  int RPM = int(RPMpulses * (60000.0 / float(refreshInterval)) * engineCycles / engineCylinders / 2.0 ); // calculate RPM
  RPMpulses = 0; // reset pulse count to 0
  RPM = min(9999, RPM); // don't return value larger than 9999
  return RPM;

 //////////////////////////////////////////oled_rpm///////////////////////////////////
 u8g.firstPage();  
  do {
    Rpm();   //mpainei to onoma tou void me to display
  } while( u8g.nextPage() ); 
 ///////////////////////////////////////////////////////////////////////////////////  
}

and here is the original code of seanauff that displays the rpm in serial monitor of arduino ide

const byte tachInterrupt = 1; // tach signal on interrupt 1 (digital pin 3)
byte engineCylinders = 8; // for tach calculation (pulses per revolution = 2 * cylinders / cycles)
byte engineCycles = 4; // for tach calculation
const byte tachPin = 0; // tach signal on digital pin 3 (interrupt 1)
int refreshInterval = 750; // milliseconds between sensor updates
unsigned long previousMillis = 0;

volatile int RPMpulses = 0;

void setup()
{
  pinMode(tachPin, INPUT_PULLUP); // enable internal pullup for tach pin
  attachInterrupt(tachInterrupt, countRPM, FALLING);
  Serial.begin(9600);
}

void loop()
{
  if(millis() - previousMillis > refreshInterval)
  {
    previousMillis = millis();
    Serial.println(getRPM());
  }
}

// counts tach pulses
void countRPM()
{
  RPMpulses++;
}

// checks accumulated tach signal pulses and calculates engine speed
// returns engine speed in RPM
// Resolution: 30000 * engineCycles / refreshInterval / engineCylinders RPM (for default values = 20 RPM)
int getRPM()
{
  int RPM = int(RPMpulses * (60000.0 / float(refreshInterval)) * engineCycles / engineCylinders / 2.0 ); // calculate RPM
  RPMpulses = 0; // reset pulse count to 0
  RPM = min(9999, RPM); // don't return value larger than 9999
  return RPM;
}

thanks in advance for the help