Hello, an auto LED dash display

Hello and straight to the point. :wink:

I want to put together a digital dash display with seven segement LED displaying speed, gear position, rpm and 15 LED shiftlight.

Instead of asking how to do it, I've googled and read and put together a program and was wondering if someone could critique it. I have no experience in programming in C and very limited programming experience in general.

I make a LOT of assupmtions about the input signals being perfect, but assuming I get that OK, does the code look like it would work?

Should I just post the program in here or attach it as a txt file? I can't find the attachment button.

int PPR = 4 // PPR- speed pulses per wheel revolution 
int TC = 1954 // TC- tire circumference
volatile int rpmcalc // rpmcalc rpm pulses in interrupt
volatile int speedcalc // speedcalc speed pulses in interrupt
volatile unsigned long timestop // timestop rpm stop times in microseconds
volatile unsigned long timestop2 // timestop2 speed stop times in microseconds
unsigned long timeold = 0L // timekeeping var for rpms in microseconds
unsigned long timeold2 = 0L // timekeeping var for speed in microseconds
int SL1 = 22 // shift lights SL1-15 specifies pins 22-36
int SL2 = 23
int SL3 = 24
int SL4 = 25
int SL5 = 26
int SL6 = 27
int SL7 = 28
int SL8 = 29
int SL9 = 30
int SL10 = 31
int SL11 = 32
int SL12 = 33
int SL13 = 34
int SL14 = 35
int SL15 = 36
unsigned int rpm = 0
unsigned int speed = 0


void setup ()
{ // set pins SL1-SL15 as output
pinMode(SL1, output);
pinMode(SL2, output);
pinMode(SL3, output);
pinMode(SL4, output);
pinMode(SL5, output);
pinMode(SL6, output);
pinMode(SL7, output);
pinMode(SL8, output);
pinMode(SL9, output);
pinMode(SL10, output);
pinMode(SL11, output);
pinMode(SL12, output);
pinMode(SL13, output);
pinMode(SL14, output);
pinMode(SL15, output); 
attachInterrupt (0, rpmcalc, rising);
// specifying the interrupt name- rpmcalc, the interrupt pin to watch- 0, and the state to watch for- rising
attachInterrupt (1, speedcalc, rising);
// specifying the interrupt name- speedcalc, the interrupt pin to watch- 1, and the state to watch for- rising
}


void loop ()
{ // First is the RPM calculations
if (rpmcalc >= 1)
{
rpm = 2 * rpmcalc * 60 * 1000000 / (timestop - timeold) 
// calculates rpm from the change in microseconds between pulses. micros return the number of microseconds the program has been running. timeold which is initially 0 marks the time of the last pulse
timeold = timestop;
rpmcalc = 0;
// resets the timer then waits for the next pulse
}

// Second is shift lights illumination. I have it set up to turn the first 5 LEDS sequentially from 5000, 6000, 7000, 8000, 9000. Then the next set of 5 LEDS on at 10000 (as a pre shift light). Finally, the final 5 LEDS on at 10900.

// First, turn all the shift lights to off
digitalWrite (SL1, LOW);
digitalWrite (SL2, LOW);
digitalWrite (SL3, LOW);
digitalWrite (SL4, LOW);
digitalWrite (SL5, LOW);
digitalWrite (SL6, LOW);
digitalWrite (SL7, LOW);
digitalWrite (SL8, LOW);
digitalWrite (SL9, LOW);
digitalWrite (SL10, LOW);
digitalWrite (SL11, LOW);
digitalWrite (SL12, LOW);
digitalWrite (SL13, LOW);
digitalWrite (SL14, LOW);
digitalWrite (SL15, LOW);

if (rpm > 10900)
{ // turn on all shift lights
digitalWrite (SL1, HIGH);
digitalWrite (SL2, HIGH);
digitalWrite (SL3, HIGH);
digitalWrite (SL4, HIGH);
digitalWrite (SL5, HIGH);
digitalWrite (SL6, HIGH);
digitalWrite (SL7, HIGH);
digitalWrite (SL8, HIGH);
digitalWrite (SL9, HIGH);
digitalWrite (SL10, HIGH);
digitalWrite (SL11, HIGH);
digitalWrite (SL12, HIGH);
digitalWrite (SL13, HIGH);
digitalWrite (SL14, HIGH);
digitalWrite (SL15, HIGH);
}
else if (rpm >= 10000)
{ // turn on first 10 shift lights
digitalWrite (SL1, HIGH);
digitalWrite (SL2, HIGH);
digitalWrite (SL3, HIGH);
digitalWrite (SL4, HIGH);
digitalWrite (SL5, HIGH);
digitalWrite (SL6, HIGH);
digitalWrite (SL7, HIGH);
digitalWrite (SL8, HIGH);
digitalWrite (SL9, HIGH);
digitalWrite (SL10, HIGH);
}
else if (rpm >= 9000)
{ // turn on first 5 shift lights
digitalWrite (SL1, HIGH);
digitalWrite (SL2, HIGH);
digitalWrite (SL3, HIGH);
digitalWrite (SL4, HIGH);
digitalWrite (SL5, HIGH);
}
else if (rpm >= 8000)
{ // turn on first 4 shift lights
digitalWrite (SL1, HIGH);
digitalWrite (SL2, HIGH);
digitalWrite (SL3, HIGH);
digitalWrite (SL4, HIGH);
}
else if (rpm >= 7000)
{ // turn on first 3 shift lights
digitalWrite (SL1, HIGH);
digitalWrite (SL2, HIGH);
digitalWrite (SL3, HIGH);
}
else if (rpm >= 6000)
{ // turn on first two shift lights
digitalWrite (SL1, HIGH);
digitalWrite (SL2, HIGH);
}
else if (rpm >= 5000)
{ // turn on first one shift light
digitalWrite (SL1, HIGH);
}


// Third- speed display calculations
if (speedcalc >= 10)
{ // waits for 10 speed pulses before updating the speed display. Can change this up or down to speed up refresh rates of speed display
speed = 10 * TC * 3600 / ((timestop2 - timeold2) * PPR * 1609)
// calculates speed from the change in microseconds between pulses. micros return the number of microseconds the program has been running. timeold which is initially 0 marks the time of the last pulse
timeold2 = timestop2;
speedcalc = 0;
// resets the timer then waits for the next pulse

// Last is gear indicator will refresh every 10 speed pulses. Calculating rpm/mph in each gear with a 190/55/17 tires results in: gear 1- 157, gear 2- 115, gear 3- 90, gear 4- 80, gear 5- 71, gear 6- 63
gearratio = rpm / speed
if (gearratio > 147 && gearratio < 167) gearpos = 1;
else if (gearratio > 105 && gearratio < 125) gearpos = 2;
else if (gearratio > 85 && gearratio < 95) gearpos = 3;
else if (gearratio > 75 && gearratio < 85) gearpos = 4;
else if (gearratio > 67 && gearratio < 75) gearpos = 5;
else if (gearratio > 60 && gearratio < 66) gearpos = 6;
else gearpos = 0 // using this to debug. the calculations did not work
}
}

rpmcalc ()
{ // this is the interrupt subprogram. when the pulse comes to interrupt 0, it stops the main program even stops the micros counter and runs this subprogram
rpmcalc ++;
// this counts the number of pulses by adding one to rpmcalc
timestop = micros();
// this captures the time the pulse was read
}

speedcalc ()
{ // this is the interrupt subprogram. when the pulse comes to interrupt pin 1, it stops the main program even stops the micros counter and runs this subprogram
speedcalc ++;
// this counts the number of pulses by adding one to rpmcalc
timestop2 = micros();
// this captures the time the pulse was read
}

i can't erase it but you should. enclose the code with code markers (press the hash mark thing above to see them).

looking at the code that's there, i think it's a solid start. I believe you will get better results if you do all your timing in the interrupt routine so you know you're not halfway to the next pulse when you get your stop time. Also, I think the RPM timing will be less erratic if you count(say) 10 pulses rather than just one. for example the interrupt routine maintains a static int counter, when the counter is 0 it stores the current micros() value, when the timer is 10 it subtracts the stored start time from current micros() and leaves the result in a global, resetting the counter to 0.

I would also recommend stepping thru your code on paper with some sample values. Look for precision and scaling issues.

Good luck, looks like fun.

Thanks for the suggestions.
I was worried that since micros stops counting in the interrupt, I should keep it as short as possible to avoid too much error.

10 samples on the rpm unfortunately at 1000 rpms (= 16.67 rev per second) at one pulse every two revs (my current input only 8 pulses per sec) means a refresh rate of a little over one second at 1000rpms, but at 10000rpms, the display is refreshed ecery 120ms. Maybe I could vary the samples taken depending on the previous rpm value?

Someone suggested I separate the program onto different processors. Maybe one for speed pulse pickup and one for rpm pulse pickup. Send data to a third processor for the data calculation and LED drivers. That way I wouldn't have to deal with two interrupts (one for speed, the other for rpm) in one program.

pinMode(SL1, output);

I spell it "OUTPUT".

attachInterrupt (0, rpmcalc, rising);

sp. "RISING"

rpm = 2 * rpmcalc * 60 * 1000000 / (timestop - timeold)

Needs a semicolon. (There are lots of missing semicolons)
"rpm" is declared as an "unsigned int" (range 0..65535) so you may get odd results with that 106. Try "unsigned long".

rpmcalc ()

If it's a "void", best tell the compiler.

if (speedcalc >= 10)

"speedcalc" is the address of a function, and will almost certainly be always > 10.

speedcalc = 0;

You can't set the address of a function to zero.

Your shift lights would probably be better implemented as an array.

Even though you may not have an Arduino board, you can still compile this stuff in the IDE.
The compiler would have caught all the things I spotted at a glance.

[edit]"speedcalc" and "rpmcalc" are both declared as "volatile int", but they're also declared as functions. This is a Bad Thingtm[/edit]

Thanks. Didn't do too much proofreading of the code.
I planned on putting it on an Arduino.

I'll look up LED arrays...