Electronic Ignition Control

Morning,
Having picked up my Arduino Uno a few days ago, I have tried to program it to be an EDIS controller, but as I am new to programming, I am not sure if I have done this the most effiecient way.

To give a bit of context:
EDIS was a system used by Ford in the 1990's to generate sparks without the use of a distributor. The EDIS module does the control of the sparks itself and has a default timing advance of 10 degrees, detected using a crankshaft position sensor.
The Arduino will recieve a 'PIP' signal (1 pulse per half revolution) from the EDIS module, and needs to return a SAW signal (variable length pulse between 64us and 1792us) approximately 10us after the PIP falling edge - It doesnt have to be exact.

Specifics:
The RPM of the engine, and manifold pressure (as measured by a 0 - 260kpa, 0-5VDC absolute pressure sensor) correspond to a value in the ignition 'map'
I have tried to implement a 2D array for this map, but I would like to be able to modify this without having to reload the program onto the arduino for tuning (updating values) whilst connected.

The final thing to mention is that for testing, I am not using the EDIS module (don't have one yet), so have simulated a pulsed output on pin 7 and linked this on the board to interupt pin 2.

The code seems to give the desired result, but probably isn't very efficient code

Any comments or suggestions greatly appreciated.
Oliver

//Global variables
volatile byte rpmcount;
unsigned long timeold;
int engine_rpm = 0;


// Global array for the ignition map, with values as microsecond duration of EDSIS SAW signal 
// RPM values 0 to 1200 have 2048us added to the SAW duration to instigate MultiSpark from the EDIS unit.

// RPM values seperated into columns, Manifold Pressure (kpa) seperated into rows.

int ignition_map [17] [16] = {
//0  ,5   ,6    ,9  ,12  ,15 ,18 ,21 ,24 ,27 ,30 ,33 ,36 ,39 ,42,45     x100 RPM 

{3123,3123,3021,2816,2714,618,570,512,435,369,307,246,184,123,64,64},   //20kpa
{3123,3123,3021,2816,2714,618,570,512,435,369,307,246,184,123,64,64},   //25kpa
{3123,3123,3021,2816,2714,618,570,512,435,369,307,246,184,123,64,64},   //30kpa
{3123,3123,3021,2816,2714,618,570,512,435,369,307,246,184,123,64,64},   //35kpa
{3123,3123,3021,2816,2714,618,570,512,435,369,307,246,184,123,64,64},   //40kpa
{3130,3130,3027,2822,2720,624,576,518,442,375,314,252,191,129,68,64},   //45kpa
{3136,3136,3034,2829,2726,630,582,525,448,381,320,259,197,136,74,64},   //50kpa
{3142,3142,3040,2835,2733,637,589,531,454,388,326,265,204,142,81,64},   //55kpa
{3149,3149,3046,2842,2739,643,595,538,461,394,333,271,210,148,87,64},   //60kpa
{3200,3200,3098,2893,2790,694,646,589,512,445,384,323,261,200,138,77},  //65kpa 
{3251,3251,3149,2944,2842,746,698,640,563,497,435,374,312,251,189,128}, //70kpa 
{3302,3302,3200,2995,2893,797,749,691,614,548,486,425,364,302,241,179}, //75kpa 
{3354,3354,3251,3046,2944,848,800,742,666,599,538,476,415,353,292,230}, //80kpa 
{3405,3405,3302,3098,2995,899,851,794,717,650,589,527,466,404,343,282}, //85kpa 
{3430,3430,3328,3123,3021,925,877,819,742,676,614,553,492,430,369,307}, //90kpa
{3430,3430,3328,3123,3021,925,877,819,742,676,614,553,492,430,369,307}, //95kpa
{3430,3430,3328,3123,3021,925,877,819,742,676,614,553,492,430,369,307}, //100kpa

};


// Initialise Parameters
void setup(){
  attachInterrupt(0, pip_interupt, FALLING); // initialise interupt to dectect the falling edge of the PIP signal coming in on digital pin 2
   rpmcount = 0;
   timeold = 0;
   Serial.begin(115200);
  pinMode(7, OUTPUT);
  pinMode(13, OUTPUT);
}
 

// The loop routine runs over and over again forever: whilst device is powered  
void loop(){ 

 // Section of code to simulate the signal from EDIS. For this simulation, the pulse is output on digital pin 7
  int delay_value = 500;    

  digitalWrite(7, HIGH);   
  delay(1);               
  digitalWrite(7, LOW);    
  delay(delay_value);      

 }

// PIP signal interupt - There are two PIP pulses per revolution, so the interrupt function is run twice per revolution
void pip_interupt()  {
   rpmcount++;  
   if (rpmcount >= 2) {      // Update RPM every 2 pulses, increase this for better RPM resolution, decrease for a faster update
     engine_rpm = 30000/(millis() - timeold)*rpmcount;
     timeold = millis();
     rpmcount = 0;
     Serial.println("engine rpm");
     Serial.println(engine_rpm,DEC);
   }
   
    int manifold_pressure = analogRead(A0)  // Read value from pressure sensor. Returns values from 0 to 1023
  
  int map_value = (rpm_pressure_to_spark(engine_rpm, manifold_pressure));    // Retrieve ignition map value 
  generate_SAW(map_value);                                                   // Call generate SAW function
  }
  
// Function to map the engine rpm to an interger value for the array element int decode_rpm(int rpm) {
  int max_rpm = 4500;               // Maximum Engine RPM
  int idle_rpm = 550;               // Idle Speed 
  int increment = max_rpm / 15;     // initialize increment to be the difference in rpm between each array element and the next
  int map_rpm = 0;
   if(rpm<idle_rpm){                // if rpm less than idle_rpm, use values from ignition map column [1]
     map_rpm = 1;
   } else if(rpm>max_rpm) {         // if rpm more than the maximum engine RPM, use values from ignition map column [15]
     map_rpm = 15;       
   }else{
   while(map_rpm * increment < rpm) // otherwise find which array element corresponds the the rpm value   
      map_rpm++;
   }
   return map_rpm;
}

 
// Function to map the engine manifold absolute pressure to an interger value for the array element int decode_pressure(int pressure) {
   int map_pressure_kpa = map(pressure,0,1023,0,260);
   int map_pressure;
   if(map_pressure_kpa < 20){
     map_pressure = 0;
   }else if (map_pressure_kpa > 100){
     map_pressure = 16;
   }else{
     map_pressure = (((52 * map_pressure_kpa)/260)-4);  //52 = 260/5 (max pressure / increment)
   }
   return map_pressure;
}
 
 
// Function to determine Ignition table value based on manifold pressure and engine rpm int rpm_pressure_to_spark(int rpm, float pressure){
  int rpm_index = decode_rpm(rpm);
  int pressure_index = decode_pressure(pressure);
  return ignition_map [pressure_index] [rpm_index];
                                
}

 
// Function to generate SAW signal to return to EDIS int generate_SAW(int SAW_length_us){
  int map_value_us = SAW_length_us;
   
  digitalWrite(13,HIGH);
  delayMicroseconds(map_value_us);
  digitalWrite(13,LOW);
  Serial.println("SAW Length in microseconds");
  Serial.println(map_value_us,DEC);
 }

There seems to be a typo in the definition of decode_rpm() - the function signature has got sucked into the preceding comment line. Similarly for rpm_pressure_to_spark() and generate_SAW().

You have serial output in your interrupt handler. Don't do that. If the amount of serial output exceeds the amount that can be buffered, the Arduino will lock up.

Your large ignition map could be moved to PROGMEM to save RAM, but if you have enough RAM then it's fine where it is.

I suggest that you should average the MAP reading over a suitable timescale because the instantaneous MAP value can fluctuate wildly. Your RPM sensing is also based on individual pulse lengths and it would probably be better to average them over several pulses. An exponential decaying average would be a good algorithm for both jobs.

Having worked on this a bit over a few months, I am now at the stage of testing it on the vehicle.
As said before, the EDIS module outputs a square wave signal with the falling edge signifying 90 degrees before TDC.
To start with, I am purely using my code to determine the RPM and print it to the serial port, so I can check that it is working correctly before moving onto having it control the EDIS module.

The problem is that I get erratic RPM values - the below is an extract from a short journey. Note 38000RPM is way too fast.

RPM, MAP, micros()

1786,96,927022560
1793,96,927043904
1793,96,927043904
1793,96,927043904
1793,96,927043904
1546,96,927065328
1546,96,927065328
1546,96,927065328
1546,96,927065328
1546,96,927086752
1686,96,927086752
38580,96,927094428
38580,96,927094428
38742,96,927109404
39217,96,927114484
438,96,927120084
438,96,927120084
438,96,927120084
544,96,927132268
544,96,927132268
544,96,927132268
544,96,927150652
567,96,927150652
567,96,927150652

The code used is below (split over 2 posts due to max character limit), and I welcome any suggestions on what is causing the problem.

//-------------------------------------------- Include Files --------------------------------------------//
#include <Streaming.h>

//-------------------------------------------- Global variables --------------------------------------------//
volatile unsigned long timeold;
volatile int engine_rpm;            // current engine rpm
float map_value_us;                 // map value in microseconds
int rev_limit = 4550;               // rev limitter
float map_value = 0.0;              // initialise map_value to be 0 degrees advance
boolean output = false;
boolean fixed = false;              // declare whether to use fixed advance values
int fixed_advance = 15;             // fixed advance value
boolean multispark = false;         // declare whether to use multispark
int interrupt_X = 3;                // declare interrupt pin
int SAW_pin = 13;                   // declare Spark advance word output 

// manifold pressure averaging array
const int numReadings = 20;         // size of pressure averaging array
int readings[numReadings];          // Array of pressure readings
int current_index = 0;              // the index of the current pressure reading
unsigned long total = 0;            // the running pressure total
unsigned int manifold_pressure = 0;        // Initial average manifold pressure
int map_pressure_kpa;

//rpm averaging array
const int num_rpm_readings = 10;               //size of rpm averaging array
volatile int rpm_readings[num_rpm_readings];   // array of rpm readings
volatile int current_index_rpm = 0;            // the index of the current rpm reading
volatile unsigned long rpm_total = 0;          // the running rpm total
volatile unsigned int engine_rpm_average = 0;  // Initial average engine rpm

//Serial input initailisation
String inputString = "";            // a string to hold incoming data
boolean stringComplete = false;     // whether the string is complete

// Global array for the ignition map, with values as degrees of advance
// RPM values seperated into columns, Manifold Pressure (kpa) seperated into rows.

const int pressure_axis[17] = { 20, 25, 30, 35, 40, 45, 50, 55, 60, 65, 70, 75, 80, 85, 90, 95, 100};  // 17
const int rpm_axis[17]  = { 0, 500, 600, 900, 1200, 1500, 1800, 2100, 2400, 2700, 3000, 3300, 3600, 3900, 4200, 4500, 4550};  // 17
const int ignition_map [17] [17] = {
//RPM 0, 500, 600, 900 etc.
{18,18,18,30,34,36,38,40,43,46,48,50,53,55,58,58,0},      //20kpa
{18,18,18,30,34,36,38,40,43,46,48,50,53,55,58,58,0},      //25kpa
{18,18,18,30,34,36,38,40,43,46,48,50,53,55,58,58,0},      //30kpa
{18,18,18,30,34,36,38,40,43,46,48,50,53,55,58,58,0},      //35kpa
{18,18,18,30,34,36,38,40,43,46,48,50,53,55,58,58,0},      //40kpa
{18,18,22,30,34,36,38,40,43,45,48,50,53,55,57,58,0},      //45kpa
{18,18,21,29,34,35,37,39,43,45,48,50,52,55,57,58,0},      //50kpa
{17,17,21,29,33,35,37,39,42,45,47,50,52,54,57,58,0},      //55kpa
{17,17,21,29,33,35,37,39,42,45,47,49,52,54,57,58,0},      //60kpa
{15,15,19,27,31,33,35,37,40,43,45,47,50,52,55,57,0},      //65kpa
{13,13,17,25,29,31,33,35,38,41,43,45,48,50,53,55,0},      //70kpa
{11,11,15,23,27,29,31,33,36,39,41,43,46,48,51,53,0},      //75kpa
{9,9,13,21,25,27,29,31,34,37,39,41,44,46,49,51,0},        //80kpa
{7,7,11,19,23,25,27,29,32,35,37,39,42,44,47,49,0},        //85kpa
{6,6,10,18,22,24,26,28,31,34,36,38,41,43,46,48,0},        //90kpa
{6,6,10,18,22,24,26,28,31,34,36,38,41,43,46,48,0},        //95kpa
{6,6,10,18,22,24,26,28,31,34,36,38,41,43,46,48,0},        //100kpa
};
 
 
//-------------------------------------------- Initialise Parameters --------------------------------------------//
void setup(){
  timeold = 0;
    
  attachInterrupt(1, pip_interupt, FALLING);                                // initialise interupt to dectect the falling edge of the PIP signal coming in on digital pin 2 (interupt 0)
  pinMode(SAW_pin, OUTPUT);                                                 // Assign SAW_pin as a digital output
  pinMode(interrupt_X, INPUT);
  digitalWrite (interrupt_X, HIGH);
  
  for (int thisReading = 0; thisReading < numReadings; thisReading++)       //populate manifold pressure averaging array with zeros
    readings[thisReading] = 0;  
    
  for (int thisReading = 0; thisReading < num_rpm_readings; thisReading++)   //populate rpm averaging array with zeros
    rpm_readings[thisReading] = 0;
   
  Serial.begin(38400);                                                       // Initialise serial communication at 38400 Baud  
  inputString.reserve(200);                                                  //reserve 200 bytes for serial input - equates to 25 characters
  
}

 
//-------------------------------------------- he loop routine runs over and over again forever: whilst device is powered --------------------------------------------// 
void loop(){
 
 if (stringComplete){   
   if(inputString == "fixed") {
         Serial.println("Fixed Advance Selected");
         fixed = true;
         inputString = "";
         stringComplete = false;
    }
    else if (inputString == "map") {
         Serial.println("Ignition Advance Selected");
         fixed = false;
         inputString = "";
         stringComplete = false;
    }
    else if (inputString == "ms on") {
          Serial.println("Multispark Enabled");
          multispark = true;
          inputString = "";
          stringComplete = false;
    }
    else if (inputString == "ms off") {
          Serial.println("Multispark Disabled");
          multispark = false;
          inputString = "";
          stringComplete = false;
    }
    else if (inputString == "output on"){
       output = true; 
       inputString = "";
       stringComplete = false;
     }
     else if (inputString == "output off"){
       output = false;
       inputString = "";
       stringComplete = false;
     }
  } 
  if (output == true){
    Serial << engine_rpm_average << ","  << map_pressure_kpa << "," << timeold << endl;
  }
 // Averaging the Manifold Pressure Reading 
  total = total - readings[current_index];        //subtract the previous reading in current array element from the total reading:
  readings[current_index] = 400;//analogRead(A0);       //take a read from the sensor and place in current array element 
  total = total + readings[current_index];        //add the reading to the total
  current_index++;              //advance to the next element in the array      
  if (current_index >= numReadings - 1){              //at end of the array...
    current_index = 0;                            // ...wrap around to the beginning
  }  
  manifold_pressure = total / numReadings;   //calculate the average - change from constant value for testing 
  map_pressure_kpa = map(manifold_pressure,0,1023,0,260);  
  delay(1);                                  //delay in between reads for stability  
} 


//-------------------------------------------- Triggered when serial input detected --------------------------------------------//
void serialEvent() {
  while (Serial.available()) {                    //whilst the serial port is available...
    inputString = Serial.readStringUntil('\n');   //... read in the string until a new line is recieved
    stringComplete = true;                      
  }
}
// ------------------------  Continued below---------------------------//
//-------------------------------------------- PIP signal interupt --------------------------------------------// 
void pip_interupt()  {
//  detachInterrupt(1);                                       // Used for testing
 if ( digitalRead(interrupt_X) == LOW ){                      // continue only if interrupt pin is LOW - Eliminates false rising edge triggering.
      generate_SAW(map_value);                                // Generate SAW using previous average engine rpm and pressure - move to bottom of this function to use current rpm and pressure
      
      engine_rpm = 30000000/(micros() - timeold);             //take the time of a reading and subtract the time of previous reading to determine rpm. Note: 30000ms as two PIP per revolution
      timeold = micros();
      rpm_total= rpm_total - rpm_readings[current_index_rpm]; //subtract the previous reading in current array element from the total reading      //set time of current reading as the new time of the previous reading
      rpm_readings[current_index_rpm] = engine_rpm;           //place current rpm value into current array element
      rpm_total= rpm_total + rpm_readings[current_index_rpm]; //add the reading to the total     
      current_index_rpm++;                                    //advance to the next element in the array
      if (current_index_rpm >= (num_rpm_readings - 1))   {    //at end of the array...
         current_index_rpm = 0;                               // ...wrap around to the beginning
      }
      engine_rpm_average = rpm_total / num_rpm_readings;      //calculate the average
      map_value = rpm_pressure_to_spark(engine_rpm_average, map_pressure_kpa);    // Retrieve ignition map value as degrees of advance.
 }                                          
// attachInterrupt(1, pip_interupt, FALLING);
}


//-------------------------------------------- Function to map the engine rpm to value for the array element --------------------------------------------// 
int decode_rpm(int rpm_) {
  int idle_rpm = 500;                 // Idle Speed
  int map_rpm = 0;
   if(rpm_ <idle_rpm){                // if rpm less than idle_rpm, use values from ignition map column [1]
     map_rpm = 1;
   } else if(rpm_ >=rev_limit) {      // if rpm more than the rev limitter, use values from ignition map column [16] - rev limitter
     map_rpm = 16;      
   }else{
     while(rpm_ > rpm_axis[map_rpm]){ // otherwise find which array element corresponds the the rpm value  
        map_rpm++;
     }
   }
    //Serial.println(map_rpm);
   return map_rpm;
}
 
//-------------------------------------------- Function to map the engine manifold absolute pressure to value for the array element --------------------------------------------//
int decode_pressure(int pressure_) {
   
   int map_pressure = 0;
   if(pressure_ < 20){
     map_pressure = 0;
   }else if (pressure_ > 100){
     map_pressure = 16;
   }else{
     while(pressure_ > pressure_axis[map_pressure]){
     map_pressure++;
     }
   }
   return map_pressure;
}
 
 
//-------------------------------------------- Function to determine Ignition table value based on manifold pressure and engine rpm --------------------------------------------//
int rpm_pressure_to_spark(int rpm, int pressure){
  float table_value;
  int map_rpm_index = decode_rpm(rpm);                      // call function decode_rpm
  int map_pressure_index = decode_pressure(pressure);       // call function decode_pressure
  float pressure_index_high;
  float pressure_index_low;
  
      pressure_index_high = mapfloat(pressure, pressure_axis[map_pressure_index-1], pressure_axis[map_pressure_index], ignition_map[map_pressure_index][map_rpm_index], ignition_map[map_pressure_index-1][map_rpm_index]);
      pressure_index_low  = mapfloat(pressure, pressure_axis[map_pressure_index-1], pressure_axis[map_pressure_index], ignition_map[map_pressure_index][map_rpm_index-1], ignition_map[map_pressure_index-1][map_rpm_index-1]);
   

   table_value = mapfloat(rpm, rpm_axis[map_rpm_index-1], rpm_axis[map_rpm_index], pressure_index_low, pressure_index_high);

  return table_value;
                               
}

 
//-------------------------------------------- Function to generate SAW signal to return to EDIS --------------------------------------------//
int generate_SAW(float advance_degrees){
  if (fixed == true){                                    // If serial input has been set to fixed, set map_value_us to the fixed value
   map_value_us = 1536 - (25.6 * fixed_advance);
  }
  else if (multispark && engine_rpm_average <= 1000){    // If engine rpm below 1000 and multispark has been set, add 2048us to map_value_us
   map_value_us = (1536 - (25.6 * advance_degrees))+2048;
  } else{                                                // Otherwise read from map
     map_value_us = 1536 - (25.6 * advance_degrees);
  }
  
  if (map_value_us < 64){                                // If map_value_us is less than 64 (smallest EDIS will accept), set map_value_us to 64us
    map_value_us = 64;
  }
  
  int code_delay = 50;
  long ten_ATDC_delay = (((60000000/engine_rpm_average)/36)-code_delay);
  delayMicroseconds(ten_ATDC_delay);
  digitalWrite(SAW_pin,HIGH);                                 // send output to logic level HIGH (5V)
  delayMicroseconds(map_value_us);                            // hold HIGH for duration of map_value_us
  digitalWrite(SAW_pin,LOW);                                  // send output to logic level LOW (0V)

}


//-------------------------------------------- Function to relicate map() function with floating point values --------------------------------------------//
float mapfloat(float x, float in_min, float in_max, float out_min, float out_max)
{
  return ((x - in_min) * ((out_max - out_min) / (in_max - in_min))) + out_min;
}

It would be better to add the code as an attachment rather than split it up, if it's too big to be posted inline.

The data extract you posted suggests the engine suffered a substantial rpm drop over a very short timescale and dropped down to 400 rpm or so, which seems extremely low to me. This makes me question whether the underlying data is reliable. If the data is generally not reliable then of course that needs to be sorted out before it's worth look at specific points where the data looks wrong. Having said that, and without looking at the code yet, the general characteristics of that glitch is the sort of thing you might see if there was a numeric overflow somewhere in a calculation.

Thanks for looking.
I guess the next step is to strip the code down to the simple measurement of the RPM and see if I get suitable results that way. If so, it points towards a code problem rather than an input problem.

Regards,
Oliver

Sorry if you already explained this and I missed it, but where is the signal coming from that you're using for RPM timing? If you're using a standard MVR sensor then the output voltage is speed dependent and it might be that at the lower end of the speed range the amplitude isn't enough to be detected reliably. That might result in either artificial 'bounce' (spurious signals) or missed signals. The code seems to be averaging the timing over ten pulses so it might be that a period of erratic signals showed up as a calculated rpm that was credible (consistent from cycle to cycle and not fluctuating wildly) but wrong.

PeterH:
Sorry if you already explained this and I missed it, but where is the signal coming from that you're using for RPM timing?

There is a VR sensor reading a 36-1 trigger wheel on the crankshaft, that connects to Ford's EDIS module. This in turn outputs a signal held at 12V, but temporarily dropping to 0V (falling edge), twice per rotation.
This signal goes through a potential divider with 5.1V zener to limit the voltage into the arduino interrupt pin.

My code is attempting to detect the time between falling edges and work out the RPM based on this period.

That sounds like a solid design and I wouldn't expect any signal level issues with that. If you haven't already done it I'd encourage you to test the timing input extensively though - you could still be suffering from EM noise issues on the connection between Arduino and EDIS. Is it practical to connect to a signal with a known actual rpm for a known duration and confirm that the received interrupt count is always close to what you'd expect? A few tests at different frequency ranges would let you rule out a whole load of potential issues in the interface circuit.

I've looked into the code a bit further and a bit surprised how much is going on inside that ISR, include some calls to delayMicroseconds(). It is probably OK but I'd keep an eye out for potential performance issues.

The ISR (and functions it calls) uses lots of global data, which isn't a disaster but opens up potential for bugs that would be best avoided. There are quite a few globals that seem to be used only inside the ISR and could be redefined as statics inside the ISR itself. The ones used only in the interrupt context don't need to be declared volatile - removing the volatile would improve ISR performance a bit. (I don't know whether that's an issue.)

I noticed that map_value is used in the main context and in an interrupt context and is not declared volatile. I don't know that this would account for the odd timing you're getting, but it's a source of potential bugs. Also, if you have any variables being updated during the ISR and also used within the main context then you must disable interrupts around accesses in the main context to ensure that updates are always atomic. The recommended way to do that is disable interrupts, copy the value to a local temporary variable and re-enable interrupts so that interrupts are disabled for as short a time as possible. It's quite possible that the problem you're seeing is actually just a variable being updated while you're in the middle of printing it since your main code is not currently protected.

As a point of style, your code to maintain current_index_rpm would be simpler and cleaner if you wrote it as:

current_index_rpm = (current_index_rpm+1) % num_rpm_readings;

If you wanted, you could get rid of that FIFO completely by using a decaying average instead of a rolling average, but what you have should work anyway.

Thanks again for the wealth of information you are giving.

This evening I tried the code stripped right back (code used below):

//-------------------------------------------- Include Files --------------------------------------------//
#include <Streaming.h>

//-------------------------------------------- Global variables --------------------------------------------//
volatile unsigned long timeold;
volatile unsigned long timenew;
volatile int engine_rpm;            // current engine rpm
int rev_limit = 4550;               // rev limitter
boolean output = false;
boolean fixed = false;              // declare whether to use fixed advance values
int fixed_advance = 15;             // fixed advance value
boolean multispark = false;         // declare whether to use multispark
int interrupt_X = 3;                // declare interrupt pin

//rpm averaging array
const int num_rpm_readings = 10;               //size of rpm averaging array
volatile int rpm_readings[num_rpm_readings];   // array of rpm readings
volatile int current_index_rpm = 0;            // the index of the current rpm reading
volatile unsigned long rpm_total = 0;          // the running rpm total
volatile unsigned int engine_rpm_average = 0;  // Initial average engine rpm

//Serial input initailisation
String inputString = "";            // a string to hold incoming data
boolean stringComplete = false;     // whether the string is complete

 
//-------------------------------------------- Initialise Parameters --------------------------------------------//
void setup(){
  timenew = 0;
  timeold = 0;
    
  attachInterrupt(1, pip_interupt, FALLING);                                // initialise interupt to dectect the falling edge of the PIP signal coming in on digital pin 2 (interupt 0)
  pinMode(interrupt_X, INPUT);
  digitalWrite (interrupt_X, HIGH);

  Serial.begin(38400);                                                       // Initialise serial communication at 38400 Baud  
  inputString.reserve(200);                                                  //reserve 200 bytes for serial input - equates to 25 characters
  
}

 
//-------------------------------------------- he loop routine runs over and over again forever: whilst device is powered --------------------------------------------// 
void loop(){
 
 if (stringComplete){   
   if(inputString == "fixed") {
         Serial.println("Fixed Advance Selected");
         fixed = true;
         inputString = "";
         stringComplete = false;
    }
    else if (inputString == "map") {
         Serial.println("Ignition Advance Selected");
         fixed = false;
         inputString = "";
         stringComplete = false;
    }
    else if (inputString == "ms on") {
          Serial.println("Multispark Enabled");
          multispark = true;
          inputString = "";
          stringComplete = false;
    }
    else if (inputString == "ms off") {
          Serial.println("Multispark Disabled");
          multispark = false;
          inputString = "";
          stringComplete = false;
    }
    else if (inputString == "output on"){
       output = true; 
       inputString = "";
       stringComplete = false;
     }
     else if (inputString == "output off"){
       output = false;
       inputString = "";
       stringComplete = false;
     }
  } 
  if (output == true){
    Serial << engine_rpm_average << ","  << timeold << endl;
  }
  delay(1000);                                  //delay in between reads for stability  
} 


//-------------------------------------------- Triggered when serial input detected --------------------------------------------//
void serialEvent() {
  while (Serial.available()) {                    //whilst the serial port is available...
    inputString = Serial.readStringUntil('\n');   //... read in the string until a new line is recieved
    stringComplete = true;                      
  }
}

 
//-------------------------------------------- PIP signal interupt --------------------------------------------// 
void pip_interupt()  {
      engine_rpm = 30000000/(micros() - timeold);             //take the time of a reading and subtract the time of previous reading to determine rpm. Note: 30000ms as two PIP per revolution
      timeold = micros();
      
      rpm_total= rpm_total - rpm_readings[current_index_rpm]; //subtract the previous reading in current array element from the total reading      //set time of current reading as the new time of the previous reading
      rpm_readings[current_index_rpm] = engine_rpm;           //place current rpm value into current array element
      rpm_total= rpm_total + rpm_readings[current_index_rpm]; //add the reading to the total     
      current_index_rpm++;                                    //advance to the next element in the array
     
      if (current_index_rpm >= (num_rpm_readings - 1))   {    //at end of the array...
         current_index_rpm = 0;                               // ...wrap around to the beginning
      }
      engine_rpm_average = rpm_total / num_rpm_readings;      //calculate the average 
      
}

The results were simillar to before:
431,14914156
243,15725400
243,15725400
243,15725400
1700,18977136
1022,19991708
3002,20964060
3986,22000052
588,23005412
38355,23970324
38290,24630776
38290,24630776
2734,27013392
2787,28015168
2510,28509712

I then used the 'RPM Fun' code from the arduino website

 volatile byte rpmcount;

 unsigned int rpm;

 unsigned long timeold;

 void setup()
 {
   Serial.begin(38400);
   attachInterrupt(1, rpm_fun, FALLING);

   rpmcount = 0;
   rpm = 0;
   timeold = 0;
 }

 void loop()
 {
   if (rpmcount >= 20) { 
     //Update RPM every 20 counts, increase this for better RPM resolution,
     //decrease for faster update
     rpm = 30*1000/(millis() - timeold)*rpmcount;
     timeold = millis();
     rpmcount = 0;
     Serial.println(rpm,DEC);
   }
 }

 void rpm_fun()
 {
   rpmcount++;
   //Each rotation, this interrupt function is run twice
 }

This resulted in much better results, albeit a little jumpy, but for a 40 year old engine started from cold, I wouldnt expect it to be completely smooth:
940
940
920
920
880
840
900
900
840
860
800
840
840
840
860
800
820
820
780
760
820
800
720
720
640
660
780
660
780
680
680
800
820

Indicating that my code is the problem.

Good to be able to rule out hardware/electrical issues.

Have you considered just adopting the simple known working code?

If you want to fix your own version, you still need to address the use of shared data between the interrupt and main contexts.

The problem I have in using the simple code is that I need to send a signal back to the EDIS module to tell it how much advance to use, at a particular time in the cycle.
In my ignorance of coding, I assume that this means that my RPM calculations need to be done during the interrupt so I can then use these to calculate the signal to send back to the EDIS.
The simple code is calculating the RPM at an undetermined time during the main loop, so I wouldn't be able to use this for sending a response at the correct time in the cycle.

If I am missing some clever way of doing what I need to do, then please point it out.

Finally, I'm not sure I understand what current_index_rpm = (current_index_rpm+1) % num_rpm_readings; is doing, or which part of my code it is replacing.

Regards,
Oliver

I see what you mean. That does seem like a valid reason to do the heavy lifting in the interrupt handler - I would still aim to keep that logic as light as possible though. I suggest you focus your bug-hunting attention on the global data shared between the interrupt and main contexts, to ensure that it is all declared volatile and that any access to multi-byte values is protected as I outlined earlier.

The code you quoted is a way to increment a value and make it wrap back to zero when it reaches a limit. It does the same thing as the code you use below but is simpler and more compact:

      current_index_rpm++;                                    //advance to the next element in the array
     
      if (current_index_rpm >= (num_rpm_readings - 1))   {    //at end of the array...
         current_index_rpm = 0;                               // ...wrap around to the beginning
      }

Using addition and modulus operators together like this might look strange the first time you encounter it but is a very common coding pattern.

I think I've sorted it.
I removed the serial input and output sections and replaced it with a simple Serial.Println one line output and all the problems went away - It would appear that the more complex serial stuff was taking too long or something.
I've no idea how the megajolt / megasquirt can run multiple data outputs for computer displays etc. but clearly they manage somehow - perhaps their serial output is more efficient than that which the arduino uses.

Thanks for all your help.
Oliver

Helo

I am working on ford escors mk1 racing version modification.
I also like the idea to use arduino to control edis4.
Its possible to use arduino without map sensor?
Only to control edis4 by RPM...

Does your software work well on your car? Or thera are some problems?

I need some help

best regards

Hi Oliver.
Was reading your post on edis control and i'm also looking to do the same thing .

I've built a test rig with an angle grinder + 36-1 wheel and light dimmer (controls rpm effectively) which is running the standard edis to give 10 degrees advance. But now the more difficult control/map for it is needed.

Did you ever get the bugs ironed out , and could I be cheeky enough to ask for a copy of the code.

Kind regards

Paul.

Okay, so now that you have gotten the bugs out of your code, would you kindy post your code for others to use, rather than having to debug code that has problems as that is the last stuff you posted please.

I've done quite a bit of work on the EDIS stuff, including megasquirt, megajolt, and working on the arduino route.

Sorry for reviving an old thread, but I just did this:

Would welcome constructive critics