So I started an electronic ignition project and got my lawnmower running (barely). I found some issues with some delays I was using in the interrupt loop, so instead I moved them to the void loop with a boolean trigger in the interrupt which improved the performance significantly.
My calculations seem to be strong and the code is testing very well, except to say that i have some slight variations (that may not be important) when monitoring the output signal between 10hz and 100hz (600 - 6000rpm's) on my DSO nano. Below is the code and a link to the running lawnmower.
Please let me know if you see anything that can be improved upon or let me know if you have any ?'s.
Please modify your post and put the code inside a pair of "code tags". You can add the code tags to your post by clicking the [ # ] button.
It's difficult to critique your code without a stated goal. From your post, it sounds as though everything is working to your satisfaction. Where I'm from, "works the way the user wants" and "is considered reliable for this kind of application" means that you're finished. What (more) are you hoping to accomplish?
Any variable you touch inside the interrupt routine and use outside, such as fire, should also be declared volatile
Serial communications are slow and serial.print blocks until it is done. The data you're sending out of the serial port at 9600 baud is going to have serious impacts on whether the fire event is called on time. Might well account for the fact that the lawnmower 'barely' runs.
Literals are interpreted as ints unless the compiler is told otherwise, by the use of the L or UL suffix. Neither 500000 or 60000000 fit into an int, so they will be truncated. The results of the computation will, then, not be what you expect.
As I said I was getting inconsistent readings from my Oscilloscope, what I was looking for was a critique of the code which might lead me to finding the issues with my inconsistencies. The other users did not seem to have trouble comprehending this concept which leads me to believe my goal was clear. Thank's for the critique of my post, however not what I was looking for.
Literals are interpreted as ints unless the compiler is told otherwise, by the use of the L or UL suffix. Neither 500000 or 60000000 fit into an int, so they will be truncated. The results of the computation will, then, not be what you expect.
Much appreciated!! I will get on changing those right away, but this raises a new question. If a literal is interpreted as an INT then 2.85 in the below code would be problematic as well? I don't see any options for a suffix for float in the documentation, does F work, or should I declare it as a variable?
Any variable you touch inside the interrupt routine and use outside, such as fire, should also be declared volatile
Serial communications are slow and serial.print blocks until it is done. The data you're sending out of the serial port at 9600 baud is going to have serious impacts on whether the fire event is called on time. Might well account for the fact that the lawnmower 'barely' runs.
Thanks, makes sense, Ill switch them over to volatile's.
Agreed, it is however just for testing purposes at this point. Later it will be hooked to an LCD with a much longer delay on the updates and running at a higher baud rate.
If a literal is interpreted as an INT then 2.85 in the below code would be problematic as well?
To be explicit, if a literal can be interpreted as an int, it will be, unless it is explicitly typed, with the L or UL suffix.
If a literal can not be interpreted as an int, as 2.85 can not be, it will be interpreted as a double. You can use the f or F suffix to force interpretation as a float, instead. On the Arduino, floats and doubles are the same size, so it doesn't matter if you use the f/F suffix, or not.
If a literal is interpreted as an INT then 2.85 in the below code would be problematic as well?
To be explicit, if a literal can be interpreted as an int, it will be, unless it is explicitly typed, with the L or UL suffix.
If a literal can not be interpreted as an int, as 2.85 can not be, it will be interpreted as a double. You can use the f or F suffix to force interpretation as a float, instead. On the Arduino, floats and doubles are the same size, so it doesn't matter if you use the f/F suffix, or not.
Thanks so much for your help Paul!! I added a servo as a tach, and made the suggested changes.
Still I have mild fluctuations on the DSO Nano and appears related to the dwell and lag waits I am seeing maybe as much as 50 microsecond variations. I don't think that 50 microseconds is going to make much of a difference @ 6k RPM (10millis per rev), but If I am doing something wrong and can get better resolution it would be nice.
Yes. You have an awful lot going on in that interrupt service routine. At 6000 RPM, assuming 1 pulse per revolution, you are getting 100 pulses per second. All that pulse() should be doing is counting pulses and recording when the last pulse occurred. The rest of those computations should be done in loop.
What is connected to analog pin 1? How does the value read from that sensor relate to timing?
All of your time variables (newmicros, oldmicros, newmillis, and oldmillis) are floats. The millis() and micros() functions, on the other hand, return unsigned long values. Does a RPM read of 6014.56 really mean much different from an RPM reading of 6014 or 6015? In other words, do you really need floats? At typical lawn mower engine rotational speeds, does the fractional value really matter?
Yes. You have an awful lot going on in that interrupt service routine. At 6000 RPM, assuming 1 pulse per revolution, you are getting 100 pulses per second. All that pulse() should be doing is counting pulses and recording when the last pulse occurred. The rest of those computations should be done in loop.
I agree 1,000% the rpm calculation can be done less often in the void loop area where the tach (servo) is adjusted. I am hesitant but see no reason why the "lag" and "oldmicros" can not be put in the "if (fire)" section of the void loop. Ill give it a shot!
PaulS:
What is connected to analog pin 1? How does the value read from that sensor relate to timing?
A potentiometer is currently plugged into analog pin 1 and is serving two functions.
as a placeholder for a MAP sensor which will later aid in a various host of other functions related to fuel mapping, but in the near future more or less just vacuum advance.
To advance and retard the timing. For example the sensor location currently reads at about 200degrees BTDC (Before Top Dead Center) So if I know by default I want to fire the coil at 6 degrees BTDC then I need to set the potentiometer to output a value of 194. The reading on the analog port is divided by 2.85 because a max reading of 1024 / 2.85 = 359.3 (360 degrees per revolution).
PaulS:
All of your time variables (newmicros, oldmicros, newmillis, and oldmillis) are floats. The millis() and micros() functions, on the other hand, return unsigned long values. Does a RPM read of 6014.56 really mean much different from an RPM reading of 6014 or 6015? In other words, do you really need floats? At typical lawn mower engine rotational speeds, does the fractional value really matter?
There is no good reason why the "newmillis" and "oldmillis" can not be long, they just serve towards the reporting function of the tach.
No I do not need fractions for the "rpm" which is why it is set to an int, but I do need it if for the "lag" value and both values share newmicros, and oldmicros. I seem to remember that in the early stages of the calculations my final value would not be a fraction unless the values they were being calculated from were floats. I will reconfirm this to be sure.
The reason I need it for lag is for precision timing, the overall goal here is not just to add electronic ignition to a lawnmower but rather to develop a full blown ECU I can use for much higher end applications. This is the early stages of my adventure.
I confirmed that my calculations bomb if newmciros, and oldmicros are set as long. I was able to clean up some of the others and make them int's and long's. see attached updates.
#include <Servo.h>
Servo RPMServo;
volatile boolean fire = false;
volatile int rpm;
volatile int dwell = 500;
volatile int timing;
volatile float newmicros;
volatile float oldmicros;
volatile long newmillis;
volatile long oldmillis;
volatile int lag;
int pos;
void setup()
{
pinMode(13,OUTPUT);
attachInterrupt(0, pulse, FALLING);
RPMServo.attach(9);
}
void loop()
{
newmillis = millis();
if ((newmillis - oldmillis) > 250) {
pos = map(rpm, 0, 6000, 14, 170);
RPMServo.write(pos);
oldmillis = newmillis;
}
if ((micros() - newmicros) > 500000L) {
newmicros = micros();
rpm = 0;
lag = 0;
timing = 0;
}
if (fire) {
rpm = (60000000L/(newmicros - oldmicros));
lag = ((((newmicros - oldmicros) / 360) * timing) - dwell);
oldmicros = newmicros;
delayMicroseconds(lag);
digitalWrite(13,HIGH);
delayMicroseconds(dwell);
digitalWrite(13,LOW);
fire=false;
}
}
void pulse()
{
newmicros = micros();
timing = analogRead(1) / 2.85F;
if ((newmicros - oldmicros) > 500000L)
{
oldmicros = newmicros;
}
else
{
fire = true;
}
}
