OP seams to be busy. Last time responding was more than a week ago. In the mean time helpers can add more arguments drowning OP in more or less useful possibilities.
and your point is?
Wait for a reponse from OP. Nobody wants lots of different suggestions. I have asked for help myself, got plenty of different suggestions and there was just no time, no energy, to follow up all those different ideas.
Unfortunately the car doesn't run at the moment. Whet I get it fixed (may take a few months), the first thing I'm going to try is to use a capacitor to try and clean/smoothen the signal that's coming from the voltage divider and see if that works. I've ordered an assortment of ceramic capacitors to experiment with. Next suggestion i'm going to try is to use a zener diode (and perhaps a octo coupler).
Consider a circuit like this:
to isolate the logic from the mess off the coil; a diode, couple of resistors, a zener and an opto-coupler (and maybe a small 0.1uF cap on the logic-side VCC pin of the opto...)
@Blackfin
That looks prity much like the curcuitry I once made, tapping the same signal, maybe the posituve side, and that project ran for 10 years without a hickup.
@OP
Adopt this idea. It's really good.
In the interest of giving credit where it's due, I cribbed the image from here:
Looked sound to me.
I'm going to order the parts to build that circuit. I'm assuming the H11L1MS also accepts 5V instead of 3.3V? Can I use ordinary 0.25W resistors?
Check that up! Don't push 5 volts into any 3.3 volt device is my advice. It might work sometimes but fail other times or fail after some time of running.
I've built the suggested circuit using this diagram (found here: Ignition Coil Interface · Issue #2 · seanauff/classic-car-sensor-interface · GitHub):
My H11L1MS indeed accepts 5V and it outputs a nice clean signal for the Arduino:
I'm not sure if I'm happy with the accuracy though:
Hz rpm rpm Arduino difference % difference
42,756 641 741 15,6 100
43,36 650 769 18,3 119
43,234 649 710 9,4 61
43,323 650 656 0,9 6
85,513 1283 1396 8,8 113
86,29 1294 1510 16,7 216
86,2 1293 1453 12,4 160
85,685 1285 1453 13,1 168
85,513 1283 1425 11,1 142
85 1275 1396 9,5 121
85,255 1279 1368 7 89
123,239 1849 1966 6,3 117
124,626 1869 1881 0,6 12
123,639 1855 1938 4,5 83
120,12 1802 1897 5,3 95
121,047 1816 1955 7,7 139
121,167 1818 1824 0,3 6
120,927 1814 1966 8,4 152
121,407 1821 1909 4,8 88
134,596 2019 2166 7,3 147
134,596 2019 2052 1,6 33
136,003 2040 2070 1,5 30
143,564 2153 2337 8,5 184
152,347 2285 2394 4,8 109
158,572 2379 2308 3 71
158,887 2383 2365 0,8 18
103,3 1550 1738 12,1 188
The first column is measurement from the oscilloscope, the 2nd calculated rpm from oscilloscope, 3rd rpm calculated by adruino.
I'm now calculating the rpm using rev*(60000/time)/(C/2).
rev = number of pulses counted
time = measuring time (ms)
c = number of cilinders
Is there a way to improve the accuracy? I am aware that this method of calculation makes that the value the Arduino displays always lags slightly behind compared to the actual rpm.
Perhaps it would be better to measure the time between pulses?
Well done! That is a success. There is some error but surely we can trim that down.
What circuitry are You using to transform the signal to Arduino levels?
Is there a way to improve the accuracy?...
Perhaps it would be better to measure the time between pulses?
There are several approaches to increase accuracy and reduce lag, but ultimately it will be a trade-off between the two. Right now we don't know what's causing the inaccuracy. It could be C coding errors, who knows? Post your current code.
Sounds from the formula you mentioned that you are counting pulses over a fixed period. Assuming no coding errors, the accuracy of that method is limited by the number of pulses counted. The smaller the count, the lower the accuracy. Other methods include timing a single pulse cycle, timing a fixed number of pulse cycles, measuring width of single pulse, total width of a fixed number of pulses...
All verry interesting and helpfull. As an engineer the output from the distributor to trigger the coil reies on the collapse of the stored current in the capacitor which bridges the points. That voltave can get quite high depending on the condition of the points, the condenser, and also the quality of the earth in the distributor baseplate. From experience the last one has caused more problems than anything else. So. There were a companies many years ago which proudced devices which got rid of the points/Capacitor set up andd replaced them with an opto coupler or hall sender; which was the system eventually adopted by most manufacturers.
Trying to clean up the output is always a problem and many tachometers adopted a different system. Mine has a coil which is wound round the No1 H.T lead, the induced voltage is then read, amplified and then used to drive the tacho.
I have a similar problem which I am working on and I am just about to post a circuit for some advice and consideration.
I will be watching for further input.
Railroader:
Well done! That is a success. There is some error but surely we can trim that down.
What circuitry are You using to transform the signal to Arduino levels?
bitterend:
All verry interesting and helpfull. As an engineer the output from the distributor to trigger the coil reies on the collapse of the stored current in the capacitor which bridges the points. That voltave can get quite high depending on the condition of the points, the condenser, and also the quality of the earth in the distributor baseplate. From experience the last one has caused more problems than anything else. So. There were a companies many years ago which proudced devices which got rid of the points/Capacitor set up andd replaced them with an opto coupler or hall sender; which was the system eventually adopted by most manufacturers.
Trying to clean up the output is always a problem and many tachometers adopted a different system. Mine has a coil which is wound round the No1 H.T lead, the induced voltage is then read, amplified and then used to drive the tacho.
I have a similar problem which I am working on and I am just about to post a circuit for some advice and consideration.
I will be watching for further input.
The circuitry I'm using is shown here:
The only difference is that instead of using 3.3v I'm using 5V so I can use the same power source (a usb-powerbank for now) for the display, the arduino and the optocoupler. This produces a clean square wave as can be seen on the oscilloscope (see photo or video in my previous post). For now I stuck the components on a breadboard. I plan on soldering it when I'm happy with the accuracy.
PaulRB:
There are several approaches to increase accuracy and reduce lag, but ultimately it will be a trade-off between the two. Right now we don't know what's causing the inaccuracy. It could be C coding errors, who knows? Post your current code.Sounds from the formula you mentioned that you are counting pulses over a fixed period. Assuming no coding errors, the accuracy of that method is limited by the number of pulses counted. The smaller the count, the lower the accuracy. Other methods include timing a single pulse cycle, timing a fixed number of pulse cycles, measuring width of single pulse, total width of a fixed number of pulses...
Yes the arduino counts pulses during the "display update interval" (currently set to 500ms). The arduino also calculates the actual measuring time.
This is my current code
#include <Arduino.h>
#include <TM1637Display.h>
// Module connection pins (Digital Pins)
#define CLK 3
#define DIO 4
TM1637Display display(CLK, DIO);
// A
// ---
// F | | B
// -G-
// E | | C
// ---
// D
const uint8_t SEG_ERR[] = {
0x0, // uit
SEG_A | SEG_D | SEG_E | SEG_F | SEG_G, // E
SEG_E | SEG_G, // r
SEG_E | SEG_G // r
};
const uint8_t SEG_HOI[] = {
SEG_G, // -
SEG_B | SEG_C | SEG_E | SEG_F | SEG_G, // H
SEG_E | SEG_F, //I
SEG_G // -
};
const uint8_t SEG_OFF[] = {
0x0, // uit
SEG_A | SEG_B | SEG_C | SEG_D | SEG_E | SEG_F, // O
SEG_A | SEG_E | SEG_F | SEG_G, //F
SEG_A | SEG_E | SEG_F | SEG_G, //F
};
float rev=0; //
int c=8; //number of cylinders
int rpm; //
int oldtime=0; //
int time; //
int updateInterval = 500; //display update interval (ms), the higher this value, the slower the screen updates, but RPM is calculated more precise
void isr(){ //interrupt service routine
rev++;
}
void setup() {
display.setBrightness(4); //0-4
display.clear();
display.setSegments(SEG_HOI); //display start message
delay(2000);
attachInterrupt(0,isr,FALLING); //attaching the interrupt
}
void loop() {
delay(updateInterval); //wait display update interval
detachInterrupt(0); //detaches the interrupt
time=millis()-oldtime; //finds the time
rpm=rev*(60000/time)/(c/2); //calculate rpm (number of pulses * (amount of measuring periods in a minute) / (number of cylinders/2)
oldtime=millis(); //saves the current time
rev=0; //resets number of pulses counted
if (rpm<0){
display.setSegments(SEG_ERR);
}
if (rpm==0){
display.setSegments(SEG_OFF);
}
if (rpm>9999){
display.setSegments(SEG_ERR);
}
if (rpm>0){
display.showNumberDec(rpm, false);
}
attachInterrupt(0,isr,FALLING);
}
Read #31 again. The resolution is poor counting pulses. Do some math. RPM stands for revolutions per minute. Your measureing time is only half a second. That is like RPM divided by 60*2, 120.Your readings ought to jump in steps of 120/4 (8 cylinders, i pulse per 2 revolution per cylinder == 4 pulses per one revolution).
float rev=0;
This variable should not be a float, it only ever holds positive, integer numbers, so I suggest "unsigned int" or "unsigned long". Floats are slow and should be avoided where possible. I don't see any need for them in your code. Also, because it is used by the interrupt routine and other functions, it should be declared "volatile".
int oldtime=0;
int time;
Any variable used to hold or calculate values using millis() should be "unsigned long", otherwise overflows can occurr with strange results.
rpm=rev*(60000/time)/(c/2);
When dividing two integers, C will perform integer division, which can cause loss of precision. To minimise that, perform multiplications first and divisions after:
rpm=rev*60000*2/time/c;
time=millis()-oldtime;
Your idea was to count the pulses over "updatePeriod", and you enable/disable interrupts to avoid counting any pulses while your code is calculating the rpm and updating the display. But the line above will include the time taken to update the display, which could be several tens of milliseconds, so this will introduce some inaccuracy into your rpm calculation.
At 42.7Hz, you would expect to detect either 21 or 22 pulses in your 500ms measurement period. That would give an error of 30rpm. But your results are 100rpm too high, indicating 3 or 4 extra detections. so I wonder if your circuit is still detecting some pulses multiple times. Try printing the value of "revs".
I don't think these suggestions will fix your problems, but they are important principles you should be aware of.
Quick question, does using "RISING" instead of "FALLING" for the interrupt make any difference in my case?
Railroader:
Read #31 again. The resolution is poor counting pulses. Do some math. RPM stands for revolutions per minute. Your measureing time is only half a second. That is like RPM divided by 60*2, 120.Your readings ought to jump in steps of 120/4 (8 cylinders, i pulse per 2 revolution per cylinder == 4 pulses per one revolution).
I'm not sure if I understand what you're saying. At idle the engine will run at 500-750 rpm. Let's say that minimum rpm is 450. At 450 rpm the pulse frequency is 30 Hz. With a display update interval of 500ms the arduino should count 15 pulses. Or am I mistaken?
If the arduino counts 14 pulses instead of 15 pulses it should calculate 420 rpm (30 rpm or 6.7% off), if 16 pulses were counted it should calculate 480 rpm (30 rpm or 6.7% off). So it should calculate rpm with at least 93% accuracy at low rpm (and 99.8% at 7500rpm) if I'm not mistaken.
PaulRB:
float rev=0;This variable should not be a float, it only ever holds positive, integer numbers, so I suggest "unsigned int" or "unsigned long". Floats are slow and should be avoided where possible. I don't see any need for them in your code. Also, because it is used by the interrupt routine and other functions, it should be declared "volatile".
The float variable was a leftover from a previous version which I forgot to change. Thanks for pointing it out, but what do you mean by declaring it volatile?
int oldtime=0;
int time;
Any variable used to hold or calculate values using millis() should be "unsigned long", otherwise overflows can occurr with strange results.
I've changed the datatypes into unsigned long. btw Is there a downside to using unsigned long instead of int? I'm asking because I'm not sure if it will ever overflow as millis() would overflow after around 50 days (micros() after 70 minutes) but the engine wil not run for more than 6 hours straight (after which the arduino will also be turned off, resetting millis()).
rpm=rev*(60000/time)/(c/2);When dividing two integers, C will perform integer division, which can cause loss of precision. To minimise that, perform multiplications first and divisions after:
rpm=rev*60000*2/time/c;
Thanks, I was not aware of this.
time=millis()-oldtime;Your idea was to count the pulses over "updatePeriod", and you enable/disable interrupts to avoid counting any pulses while your code is calculating the rpm and updating the display. But the line above will include the time taken to update the display, which could be several tens of milliseconds, so this will introduce some inaccuracy into your rpm calculation.
Would it help to move this line
oldtime=millis();
to the end of the loop, right before the interrupt gets reattached?
At 42.7Hz, you would expect to detect either 21 or 22 pulses in your 500ms measurement period. That would give an error of 30rpm. But your results are 100rpm too high, indicating 3 or 4 extra detections. so I wonder if your circuit is still detecting some pulses multiple times. Try printing the value of "revs".
I've previously disabled the calculation and let it display time(=millis()-oldtime)This way I noticed it takes about 21 ms to complete the loop. I then set the update interval to 979ms and let it display rev (= Hz. ) And it was almost spot on with the oscilloscope. So I don't think there is a problem with the counting of the pulses.
I don't think these suggestions will fix your problems, but they are important principles you should be aware of.
The rpm calculated by the arduino cannot be compared directly to the value displayed by the oscilloscope as the measurement of the arduino is an average of the previous 500ms where the oscilloscope calculates the frequency by measuring the length of the last pulse. I will test the suggested changes first, perhaps the improvement already makes the accuracy acceptable.
sjirafje:
Quick question, does using "RISING" instead of "FALLING" for the interrupt make any difference in my case?
An interrupt should be used for the non-normal situation. What is the normal, longest time, for the signal, 5 volts or zero volts? The answer will tell you what should be the interrupt.
Paul
Would it help to move this line
Code: [Select]
oldtime=millis();
to the end of the loop, right before the interrupt gets reattached?
Yes, but why not forget time and oldtime and simply write
rpm=rev*60000*2/updateInterval/c;