LCD displaying weird Symbols after Interrupt

I'm trying to use an LCD to print values which an interrupt calculates, but after the first interrupt, the LCD displays weird symbols and goes hay wire. It does work for the first interrupt though.

Its very odd. Any suggestions?

Here's some example code:

//------------------------------------------------------------------------
#include <LiquidCrystal.h>
unsigned int revPeriod; // Rotation Time
volatile unsigned long prevTDCtime; // Previous time Hall Sensor Picked up
volatile unsigned long TDCtime; // Current TDC Time
volatile unsigned long nextTDC; // Predicted next TDC Time
unsigned int rpm;
unsigned int crankAngle; // Crankshaft Angle out of 360 degrees
boolean TDCinterrupt1 = false; //
const int V17Pin = 8; // Valves 1 and 7 assigned to Pin 3 Intake
const int V35Pin = 9; // Valves 3 and 5 assigned to Pin 4 Intake
const int V28Pin = 10; // Valves 2 and 8 assigned to Pin 5 Exhaust
const int V46Pin = 11; // Valves 4 and 6 assigned to Pin 6 Exhaust
LiquidCrystal lcd (12,11,6,5,4,3);

void setup()
{
// Communicate to Serial Port
Serial.begin (9600);
lcd.begin(16,2);
pinMode (V17Pin, OUTPUT); // Valve 1&7 as Output
pinMode (V35Pin, OUTPUT); // Valve 3&5 as Output
pinMode (V28Pin, OUTPUT); // Valve 2&8 as Output
pinMode (V46Pin, OUTPUT); // Valve 4&6 as Output
nextTDC = 1;
lcd.setCursor(4,0);
lcd.print("READY");

attachInterrupt(0,TDCinterrupt, RISING); // Interrupt 0 is Pin 2 Hall Effect Sensor

}

void TDCinterrupt()
{
TDCinterrupt1 = true;
prevTDCtime = TDCtime; // save last TDC
TDCtime = micros(); // save THIS TDC
revPeriod = TDCtime - prevTDCtime; // calculate revolution period
if (prevTDCtime > TDCtime) {
revPeriod = TDCtime - prevTDCtime + 2147483648; // correct for micros() overflow every 71 minutes
}
else {
revPeriod = TDCtime - prevTDCtime; // no correction needed
}
nextTDC = TDCtime + revPeriod;
rpm = 60000000/revPeriod;
}

void loop ()
{
if ( TDCinterrupt1){

cli();
crankAngle = map(micros(), TDCtime, nextTDC, 0, 360); // Change time into degrees
sei();
crankAngle = constrain(crankAngle, 0, 360); // Limit crankshaft vales from 0 to 360

if ( TDCtime == 0){
Serial.print ("** Device Immobile **");
lcd.setCursor(0,0);
lcd.print ("ON");

}
lcd.setCursor(1,1);
lcd.print(rpm);

}

The rpm displays clearly for the first ISR, but after that it goes crazy.

Another example code would be adding this into the previous code:

void loop ()
{ 
  if ( TDCinterrupt1){
    lcd.print(TDCtime);
  cli();

First thing I noticed was a var and the LCD sharing pin 11

 const int V46Pin = [glow]11[/glow]; // Valves 4 and 6 assigned to Pin 6  Exhaust
 LiquidCrystal lcd (12,[glow]11[/glow],6,5,4,3);

Furthermore:

  [glow]revPeriod = TDCtime - prevTDCtime;[/glow]  // calculate revolution period
  if (prevTDCtime > TDCtime)  {
     revPeriod = TDCtime - prevTDCtime + 2147483648;  // correct for micros() overflow every 71 minutes
  }
  else  {
     [glow]revPeriod = TDCtime - prevTDCtime;[/glow]  // no correction needed
    }
  • The else part is done double, can be removed

rpm = 60000000[glow]L[/glow]/revPeriod;
You need a L to indicate 60M is a long.
Furthermore if revperiod is zero ==> divide by zero.


Does all this code need to be calculated in the ISR or can it be moved to loop after the if ( TDCinterrupt1){ to keep the ISR as short as possible.

 void TDCinterrupt()
{
  TDCinterrupt1 = true;
  prevTDCtime = TDCtime;  // save last TDC
  TDCtime = micros();  // save THIS TDC
  // rest can be done outside the ISR???
 }

the map() function expects a signed long as parameters. micros() is unsigned long so every 35 minutes it might be interpreted as a negative number?


TDCinterrupt1 is set to true but never to false again? Or is this not the complete code?

First thing I noticed was a var and the LCD sharing pin 11

Good Eye. With nothing but the LCD plugged into the arduino, I didnt even notice :o

The else part is done double, can be removed

agree

You need a L to indicate 60M is a long.
Furthermore if revperiod is zero ==> divide by zero.

Your right, I need to use a multiplier like the playground ReadingRPM
I will also add the L

the map() function expects a signed long as parameters. micros() is unsigned long so every 35 minutes it might be interpreted as a negative number?

Is there an unsigned map function?

Does all this code need to be calculated in the ISR or can it be moved to loop after the if ( TDCinterrupt1){ to keep the ISR as short as possible.

I did not post the whole code, but if you need me too, I will. Basically this code is controlling solenoids to run an engine. The timing for the intake and exhaust has to be as good as possible, so I think we concluded in the other thread that putting it in the loop, you had the potential of missing an interrupt? I don't know, give me your opinion.

TDCinterrupt1 is set to true but never to false again? Or is this not the complete code?

As mentioned its not the complete code. I would like to have tdcinterrupt1 go to false again when the engine is at a standstill. I was thinking I could say something about when the crank angle is at the same value for so many seconds, set TDCinterrupt1 = FALSE.

I'm not the most proficient programmer, so maybe you can help me with that.

Thanks for your time/help.

Change those other variables to volatile as well.

Is there an unsigned map function?

something like this?

 unsigned long map(unsigned long x, unsigned long in_min, unsigned long in_max, unsigned long out_min, unsigned long out_max)
{
  return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}

I did not post the whole code, but if you need me too, I will.

Could be easier to debug than mindreading :wink:

missing an interrupt? I don't know, give me your opinion

It is best practice too keep ISR()'s as short as possible - but not shorter - to prevent reentry. Reentry is that the ISR is called again before the last call is finished. You can imagine there might be quite some interference :slight_smile: Also important is to keep your data consistent, if your main loop is interrupted anywhere and the ISR changes some var's calculations in the main loop must be correct. Use cli() and sei() to disable/enable interrupts around the critical calculations in loop() however that may cause missing interrupts, so these should also be used with care.

Could be easier to debug than mindreading Wink

I'll spare you, at the moment, and give you the stripped down version of the code (no timing control). I made a few changes based upon your previous suggestions...

//------------------------------------------------------------------------
#include <LiquidCrystal.h>
unsigned int revPeriod; // Rotation Time
volatile unsigned long prevTDCtime; // Previous time Hall Sensor Picked up
volatile unsigned long TDCtime; // Current TDC Time
volatile unsigned long nextTDC; // Predicted next TDC Time
volatile byte rpmcount;
unsigned int rpm;
unsigned int crankAngle; // Crankshaft Angle out of 360 degrees
boolean TDCinterrupt1 = false; //
const int V17Pin = 9; // Valves 1 and 7 assigned to Pin 3 Intake
const int V35Pin = 11; // Valves 3 and 5 assigned to Pin 4 Intake
const int V28Pin = 10; // Valves 2 and 8 assigned to Pin 5 Exhaust
const int V46Pin = 12; // Valves 4 and 6 assigned to Pin 6 Exhaust
LiquidCrystal lcd (8,7,6,5,4,3);

void setup()
{
// Communicate to Serial Port
Serial.begin (9600);
lcd.begin(16,2);
pinMode (V17Pin, OUTPUT); // Valve 1&7 as Output
pinMode (V35Pin, OUTPUT); // Valve 3&5 as Output
pinMode (V28Pin, OUTPUT); // Valve 2&8 as Output
pinMode (V46Pin, OUTPUT); // Valve 4&6 as Output
nextTDC = 1;
lcd.setCursor(4,0);
lcd.print("READY");
attachInterrupt(0,TDCinterrupt, RISING); // Interrupt 0 is Pin 2 Hall Effect Sensor
}

void TDCinterrupt()
{
TDCinterrupt1 = true;
prevTDCtime = TDCtime; // save last TDC
TDCtime = micros(); // save THIS TDC
revPeriod = TDCtime - prevTDCtime; // calculate revolution period
if (prevTDCtime > TDCtime) {
revPeriod = TDCtime - prevTDCtime + 2147483648; // correct for micros() overflow every 71 minutes
}

nextTDC = TDCtime + revPeriod;
rpm = 60000000L/revPeriod;

}

void loop ()
{
if ( TDCinterrupt1){

cli();
crankAngle = map(micros(), TDCtime, nextTDC, 0, 360); // Change time into degrees
sei();
crankAngle = constrain(crankAngle, 0, 360); // Limit crankshaft vales from 0 to 360
}

if ( crankAngle >= 1 && crankAngle <= 179) // If the crankshaft is inbetween 1 to 179 degrees of revolution then...
{
digitalWrite(V17Pin, HIGH); // Fire Valve 1 and 7 (Intake)
digitalWrite(V46Pin, HIGH); // Fire Valve 4 and 6 (Exhaust)
Serial.println("Intake (1,7) Exhaust (4,6) Fired 100%"); // Print Status and Percent of Valve Open
lcd.setCursor(12,1);
lcd.print ("+");
lcd.setCursor(13,1);
lcd.print ("+");
}
else {
digitalWrite(V17Pin, LOW); // Don't Fire Valve 1 and 7 (Intake)
digitalWrite(V46Pin, LOW); // Don't Fire Valve 4 and 6 (Exhaust)
lcd.setCursor(12,1);
lcd.print ("-");
lcd.setCursor(13,1);
lcd.print ("-");
}
if ( crankAngle >= 181 && crankAngle <= 359) // If the crankshaft is inbetween 181 to 359 degrees of revolution then...
{
digitalWrite(V35Pin, HIGH); // Fire Valve 3 and 5 (Intake)
digitalWrite(V28Pin, HIGH); // Fire Valve 2 and 8 (Exhaust)
lcd.setCursor(14,1);
lcd.print ("+");
lcd.setCursor(15,1);
lcd.print ("+");
Serial.println("Intake (3,5) Exhaust (2,8) Fired 100%"); // Print Status and Percent of Valve Open
}
else {
digitalWrite(V35Pin, LOW); // Don't Fire Valve 3 and 5 (Intake)
digitalWrite(V28Pin, LOW); // Don't Fire Valve 2 and 8 (Exhaust)
lcd.setCursor(14,1);
lcd.print ("-");
lcd.setCursor(15,1);
lcd.print ("-");
}

lcd.setCursor(1,1);
lcd.print(rpm,DEC);
}

First it is good practice to initialize your variables to a known value. The compiler will often make them zero / 0 but not al the times.

**rpm = 60000000L/revPeriod; **
rpm is an int while this calculation is long so there is possible loss of precission.

crankAngle = map(micros(), TDCtime, nextTDC, 0, 360);
Not patched the map function to unsigned long as I proposed...

** lcd.setCursor(15,1);**
** lcd.print ("+");**
Your lcd is 16 x 2, by setting the cursor at 15, 1 it is at the end of the line and you are writing outside the LCD's screen, maybe into memory or registers or something. Count of the positions goes from 0..15 not from 0..16.

This latter could be the cause of the weird chars.

rpm = 60000000L/revPeriod;
rpm is an int while this calculation is long so there is possible loss of precission.

This is not the most important calculation in the code, but what could I do to prevent this loss of precision?

Not patched the map function to unsigned long as I proposed...

I forgot to add it. Would it look like this, since they are all unsigned longs ? Can you explain what the return is doing?

crankAngle = unsigned long map (micros(), TDCtime, nextTDC, 0, 360);
{
  return (micros - TDCtime) * (360 - 0) / (nextTDC - TDCtime) + 0;
}

lcd.setCursor(15,1);
lcd.print ("+");
Your lcd is 16 x 2, by setting the cursor at 15, 1 it is at the end of the line and you are writing outside the LCD's screen, maybe into memory or registers or something. Count of the positions goes from 0..15 not from 0..16.

This latter could be the cause of the weird chars.

I already caught this and fixed it, after I posted the code. ;D
But the weird characters were caused by the double pin assignment earlier, and is fixed now.

Thanks for the help thus far rob. When we go over these few crinkles, I'll give you the "complete" code to review, if you don't mind :wink:

Good to hear things work, if you have finalized your code please post it, also for people in the future who have similar issues.

This is not the most important calculation in the code, but what could I do to prevent this loss of precision?

make rpm a long too.


Add this function definition to your code (at the end is ok)

unsigned long ULmap(unsigned long x, unsigned long in_min, unsigned long in_max, unsigned long out_min, unsigned long out_max)
{
  return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}

And change the map call in your exisiting code

crankAngle = ULmap (micros(), TDCtime, nextTDC, 0, 360);

Good to hear things work, if you have finalized your code please post it, also for people in the future who have similar issues.

Great! It will probably take two posts. I have two questions regarding the final code. I have recently learned about calling to functions outside of the loop. I set up my timing and start control outside of the loop to try to minimize clutter, and allow the loop to run faster. Is this in fact the better way?

Also, with start mode, I use delays between each valve, but I don't think that will work right. The plan is that once the engine starts spinning, the interrupt kicks in and invalidates the start code. If the interrupt happens during a delay, I'm doomed. I was thinking of using a "blink without delay" but I'm not sure if I can call to micros/millis if the interrupt is as well. Your opinion?

Thanks

FYI. The ULmap as displayed below does not compile correctly. It thinks the UL is part of the name, and states that ULmap has not been declared.

//--- NEWMAN180 1/4/11
//------------------------------------------------------------------------
#include <LiquidCrystal.h>
volatile unsigned int revPeriod; // Rotation Time
volatile unsigned long prevTDCtime; // Previous time Hall Sensor Picked up
volatile unsigned long TDCtime; // Current TDC Time
volatile unsigned long nextTDC; // Predicted next TDC Time
volatile unsigned long rpm;
volatile unsigned int crankAngle; // Crankshaft Angle out of 360 degrees
unsigned int a; // first crank angle of 180
unsigned int a1; // second crank angle of 180
unsigned int b; // first crank angle of 360
unsigned int b1; // second crank angle of 360
boolean TDCinterrupt1 = false; //
const int timingPin = 14;
const int maintbuttonPin = 15;
const int pushstartPin = 16;
int pushstartState = 0;
int maintbuttonState = 0;
int buttonstate = 0;
int buttonpushcounter = 0;
int lastbuttonstate = 0;
int start;
int timing;
int maint;
const int V17Pin = 9; // Valves 1 and 7 assigned to Pin 3 Intake
const int V35Pin = 11; // Valves 3 and 5 assigned to Pin 4 Intake
const int V28Pin = 10; // Valves 2 and 8 assigned to Pin 5 Exhaust
const int V46Pin = 12; // Valves 4 and 6 assigned to Pin 6 Exhaust
LiquidCrystal lcd (8,7,6,5,4,3);

void setup()
{
// Communicate to Serial Port
Serial.begin (9600);
lcd.begin(16,2);
pinMode (V17Pin, OUTPUT); // Valve 1&7 as Output
pinMode (V35Pin, OUTPUT); // Valve 3&5 as Output
pinMode (V28Pin, OUTPUT); // Valve 2&8 as Output
pinMode (V46Pin, OUTPUT); // Valve 4&6 as Output
pinMode (pushstartPin, INPUT);
pinMode (timingPin, INPUT);
pinMode (maintbuttonPin, INPUT);
rpm = 0;
nextTDC = 1;
lcd.setCursor(0,0);
lcd.print("ON");
attachInterrupt(0,TDCinterrupt, RISING); // Interrupt 0 is Pin 2 Hall Effect Sensor
digitalWrite(2,HIGH);
delay(3500);
}

void TDCinterrupt()
{
TDCinterrupt1 = true;
prevTDCtime = TDCtime; // save last TDC
TDCtime = micros(); // save THIS TDC
revPeriod = TDCtime - prevTDCtime; // calculate revolution period
if (prevTDCtime > TDCtime) {
revPeriod = TDCtime - prevTDCtime + 2147483648; // correct for micros() overflow every 71 minutes
}
nextTDC = TDCtime + revPeriod;

rpm = 60000000L/revPeriod;
}

void loop ()
{
if ( TDCinterrupt1){
cli();
crankAngle = ULmap(micros(), TDCtime, nextTDC, 0, 360); // Change time into degrees
sei();
crankAngle = constrain(crankAngle, 0, 360); // Limit crankshaft vales from 0 to 360
}
//---------------------------------- Button Counter Control
buttonstate = digitalRead(timingPin);

if (buttonstate != lastbuttonstate)
{
if (buttonstate == HIGH) {
buttonpushcounter++; // Update Button Counter
}

}

lastbuttonstate = buttonstate;
if( buttonpushcounter > 9){
buttonpushcounter = 0;}
//--------------------------------------- Button Timing Control
timing = timingcontrol();
timing;

//-------------------------------- Timing Center

if ( crankAngle >= a && crankAngle <= a1) // If the crankshaft is inbetween 1 to 179 degrees of revolution then...
{
digitalWrite(V17Pin, HIGH); // Fire Valve 1 and 7 (Intake)
digitalWrite(V46Pin, HIGH); // Fire Valve 4 and 6 (Exhaust)
Serial.println("Intake (1,7) Exhaust (4,6) Fired 100%"); // Print Status and Percent of Valve Open
lcd.setCursor(12,1);
lcd.print ("+");
lcd.setCursor(13,1);
lcd.print ("+");
}

else {
digitalWrite(V17Pin, LOW); // Don't Fire Valve 1 and 7 (Intake)
digitalWrite(V46Pin, LOW); // Don't Fire Valve 4 and 6 (Exhaust)
lcd.setCursor(12,1);
lcd.print (" ");
lcd.setCursor(13,1);
lcd.print (" ");
}

if ( crankAngle >= b && crankAngle <= b1) // If the crankshaft is inbetween 181 to 359 degrees of revolution then...
{
digitalWrite(V35Pin, HIGH); // Fire Valve 3 and 5 (Intake)
digitalWrite(V28Pin, HIGH); // Fire Valve 2 and 8 (Exhaust)
lcd.setCursor(14,1);
lcd.print ("+");
lcd.setCursor(15,1);
lcd.print ("+");
Serial.println("Intake (3,5) Exhaust (2,8) Fired 100%"); // Print Status and Percent of Valve Open
}

else {
digitalWrite(V35Pin, LOW); // Don't Fire Valve 3 and 5 (Intake)
digitalWrite(V28Pin, LOW); // Don't Fire Valve 2 and 8 (Exhaust)
lcd.setCursor(14,1);
lcd.print (" ");
lcd.setCursor(15,1);
lcd.print (" ");
}

unsigned long ULmap(unsigned long x, unsigned long in_min, unsigned long in_max, unsigned long out_min, unsigned long out_max)
{
return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}
//------------------------------- RPM Display

lcd.setCursor(1,1);
lcd.print(rpm,DEC);

//------------------------------- START MODE

start = startmode();
start;

//--------------------------------Maitenance Mode

maint = maintmode();
maint;

}
int startmode (){
if (TDCtime == 0){
pushstartState = digitalRead(pushstartPin);
if (pushstartState == HIGH){

delay(3000);
Serial.print ("** Start MODE **"); // Start Mode, Open valves 2.5 Seconds, Close 15, Open all intake and exhaust. Open and close exhaust .1 second
lcd.clear();
lcd.setCursor(3,0);
lcd.print ("Start Mode");
digitalWrite( V17Pin, HIGH);
digitalWrite( V35Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
lcd.setCursor(0,1);
delay(3500);
digitalWrite( V17Pin, LOW);
digitalWrite( V35Pin, LOW);
digitalWrite( V28Pin, LOW);
digitalWrite( V46Pin, LOW);
delay(15000);
lcd.clear();
lcd.print("Starting");
digitalWrite( V17Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
delay(100);
digitalWrite( V28Pin, LOW);
delay(100);
digitalWrite( V17Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
delay(100);
digitalWrite( V28Pin, LOW);
delay(100);
digitalWrite( V17Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
delay(100);
digitalWrite( V28Pin, LOW);
delay(100);
digitalWrite( V17Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
delay(100);
digitalWrite( V35Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
delay(100);
digitalWrite( V46Pin, LOW);
delay(100);
digitalWrite( V35Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
delay(100);
digitalWrite( V46Pin, LOW);
delay(100);
digitalWrite( V35Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
delay(100);
digitalWrite( V46Pin, LOW);
delay(100);
digitalWrite( V35Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
delay(100);
digitalWrite( V46Pin, LOW);
delay(100);
digitalWrite( V35Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
delay(100);
digitalWrite( V46Pin, LOW);
delay(100);
digitalWrite( V35Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
delay(100);
digitalWrite( V46Pin, LOW);
digitalWrite( V17Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
delay(100);
digitalWrite( V28Pin, LOW);
delay(100);
digitalWrite( V17Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
delay(100);
digitalWrite( V28Pin, LOW);
delay(100);
digitalWrite( V17Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
delay(100);
digitalWrite( V28Pin, LOW);
delay(100);
digitalWrite( V17Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
delay(100);
digitalWrite( V35Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
delay(100);
digitalWrite( V46Pin, LOW);
delay(100);
digitalWrite( V35Pin, HIGH);
digitalWrite( V28Pin, HIGH);
digitalWrite( V46Pin, HIGH);
delay(100);
digitalWrite( V46Pin, LOW);

else{

lcd.setCursor(0,0);
lcd.print("START KEY");

}
}
}

int timingcontrol(){
if (buttonpushcounter == 0){
a = 1;
a1 = 179;
b = 181;
b1 = 359;
lcd.setCursor(12,0);
lcd.print("100%");}

if (buttonpushcounter == 1){
a = 1;
a1 = 162;
b = 181;
b1 = 342;
lcd.setCursor(12,0);
lcd.print(" 90%");
}
if (buttonpushcounter == 2){
a = 1;
a1 = 144;
b = 181;
b1 = 324;
lcd.setCursor(12,0);
lcd.print(" 80%");
}
if (buttonpushcounter == 3){
a = 1;
a1 = 126;
b = 181;
b1 = 306;
lcd.setCursor(12,0);
lcd.print(" 70%");
}
if (buttonpushcounter == 4){
a = 1;
a1 = 108;
b = 181;
b1 = 288;
lcd.setCursor(12,0);
lcd.print(" 60%");
}
if (buttonpushcounter == 5){
a = 1;
a1 = 90;
b = 181;
b1 = 270;
lcd.setCursor(12,0);
lcd.print(" 50%");
}
if (buttonpushcounter == 6){
a = 1;
a1 = 72;
b = 181;
b1 = 252;
lcd.setCursor(12,0);
lcd.print(" 40%");
}
if (buttonpushcounter == 7){
a = 1;
a1 = 54;
b = 181;
b1 = 234;
lcd.setCursor(12,0);
lcd.print(" 30%");
}
if (buttonpushcounter == 8){
a = 1;
a1 = 36;
b = 181;
b1 = 216;
lcd.setCursor(12,0);
lcd.print(" 20%");
}
if (buttonpushcounter == 9){
a = 1;
a1 = 18;
b = 181;
b1 = 198;
lcd.setCursor(12,0);
lcd.print(" 10%");
}

}

int maintmode() {
maintbuttonState = digitalRead(maintbuttonPin);
if (maintbuttonState == HIGH){
cli();
TDCtime = 0;
nextTDC = 0;
prevTDCtime = 0;
lcd.clear();
lcd.print("Maintenance MODE");
digitalWrite(V28Pin,HIGH);
digitalWrite(V46Pin,HIGH);
delay(60000);
}

}

wow

ULmap will not work as it is defined inside the loop I guess (I don't have the complete overview of your code yet)

Furthermore you must spend some time learning about array's - http://www.arduino.cc/en/Reference/Array - and functions - http://www.arduino.cc/en/Reference/FunctionDeclaration.

If you have mastered these two basic techniques you can cleanup your code quite a lot.

Also important to learn is to use proper identation as that makes code better readable.

Snippet by snippet

int maintmode() 
{
  maintbuttonState = digitalRead(maintbuttonPin);
  if (maintbuttonState == HIGH)
  {
     cli();     [glow]// where is the sei()[/glow]
     TDCtime = 0;
     nextTDC = 0;
     prevTDCtime = 0;
     lcd.clear();
     lcd.print("Maintenance MODE");
     digitalWrite(V28Pin,HIGH);
     digitalWrite(V46Pin,HIGH);
     delay(60000);
  }
}

This is the code of timingcontrol with the use of array's

int timingcontrol()
{
  a = 1;
  b = 181;
  lcd.setCursor(12,0); 
   
  int a1array[10] = { 179, 162, 144, 126, 108, 90, 72, 54, 36, 18 };
  int b1array[10] = { 359, 342, 324, 306, 288, 270, 252, 234, 216, 198 };
   
  a1 = a1array[buttonpushcounter];
  b1 = b1array[buttonpushcounter];
   
  lcd.print(100-10*buttonpushcounter);
  lcd.print("%");
}

In fact as all values are linear (almost) this can be optimized to

int timingcontrol()
{
  a = 1;
  b = 181;

  a1 = 180 - 18 * buttonpushcounter;
  if (buttonpushcounter ==0) a1 = a1 -1;  // makes 179
  b1 = a1 + 180;
   
  lcd.setCursor(12,0); 
  lcd.print(100-10*buttonpushcounter);
  lcd.print("%");
}

loop with some proper indentation, and a bit compacting

void loop ()
{ 
  if ( TDCinterrupt1)
  {
    cli();
    crankAngle = ULmap(micros(), TDCtime, nextTDC, 0, 360);  // Change time into degrees
    sei(); 
    crankAngle = constrain(crankAngle,  0, 360); // Limit crankshaft vales from 0 to 360
  }
  
//----------------------------------  Button Counter Control   
buttonstate = digitalRead(timingPin);
 
  // Update Button Counter
  if ((buttonstate != lastbuttonstate) && (buttonstate == HIGH)) buttonpushcounter++; 
  lastbuttonstate = buttonstate;
  if( buttonpushcounter > 9) buttonpushcounter = 0;
  
 
//--------------------------------------- Button Timing Control 
  timing = timingcontrol();
  // timing; ?????

//--------------------------------  Timing Center

// If the crankshaft is inbetween 1 to 179 degrees of revolution then...
  if ( crankAngle >= a && crankAngle <= a1) 
  { 
    digitalWrite(V17Pin, HIGH); // Fire Valve 1 and 7 (Intake)
    digitalWrite(V46Pin, HIGH); // Fire Valve 4 and 6 (Exhaust)
    Serial.println("Intake (1,7) Exhaust (4,6) Fired 100%"); // Print Status and Percent of Valve Open
    lcd.setCursor(12,1);
    lcd.print ("++");
  } 
  else 
  {
    digitalWrite(V17Pin, LOW); // Don't Fire Valve 1 and 7 (Intake)
    digitalWrite(V46Pin, LOW); // Don't Fire Valve 4 and 6 (Exhaust)
     lcd.setCursor(12,1);
     lcd.print ("  ");
  }
  
  // If the crankshaft is inbetween 181 to 359 degrees of revolution then...
  if ( crankAngle >= b && crankAngle <= b1) 
  {
     digitalWrite(V35Pin, HIGH); // Fire Valve 3 and 5 (Intake)
     digitalWrite(V28Pin, HIGH); // Fire Valve 2 and 8 (Exhaust)
     lcd.setCursor(14,1);
     lcd.print ("++");
     Serial.println("Intake (3,5) Exhaust (2,8) Fired 100%"); // Print Status and Percent of Valve Open
  }
  else 
  {
    digitalWrite(V35Pin, LOW); // Don't  Fire Valve 3 and 5 (Intake)
    digitalWrite(V28Pin, LOW); // Don't Fire Valve 2 and 8 (Exhaust)
    lcd.setCursor(14,1);
    lcd.print ("  ");
  }      

//------------------------------- RPM Display
      
  lcd.setCursor(1,1);
  lcd.print(rpm,DEC);

//------------------------------- START MODE

  start = startmode();
  // start;   

//--------------------------------Maitenance Mode

  maint = maintmode();
  // maint;
}
unsigned long ULmap(unsigned long x, unsigned long in_min, unsigned long in_max, unsigned long out_min, unsigned long out_max)
{
  return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}

Furthermore you must spend some time learning about array's - http://www.arduino.cc/en/Reference/Array - and functions - http://www.arduino.cc/en/Reference/FunctionDeclaration.

If you have mastered these two basic techniques you can cleanup your code quite a lot.

Also important to learn is to use proper identation as that makes code better readable.

I've been trying. I've learned all of this with no background or class's in programming, so my brain isn't exactly wired to think in "code". I am a mechanical engineer major, who assumed this was easier then it actually is ::slight_smile:

I was searching for a function like array, but didn't know how to find that actual definition.

cli(); // where is the sei()

With "maitenance mode", I won't be needing to activate the interrupt. It's mainly something to allow me to turn the engine over by hand. If no valves we open, it be impossible. I also bought a switch, so that delay will no longer be there.

In fact as all values are linear (almost) this can be optimized to
Code:

int timingcontrol()
{
a = 1;
b = 181;

a1 = 180 - 18 * buttonpushcounter;
if (buttonpushcounter ==0) a1 = a1 -1; // makes 179
b1 = a1 + 180;

lcd.setCursor(12,0);
lcd.print(100-10*buttonpushcounter);
lcd.print("%");
}

This is awesome. Looks like it would work perfectly. If only I knew about setting up these functions.

loop with some proper indentation, and a bit compacting

Great, thank you. I will post an updated version of mine with this included after this post.

// timing; ?????

Why the question marks?

// start;

why do you have // in front of all the functions I'm calling? The // makes it so arduino can't see that.

I've attempted to clean the code further as well as label everything more clearly.

I got rid of the ULmap at the moment as it still won't compile.
I also attempted to use a fucntion in start mode to get rid of all the clutter. I don't know if I implemented it correctly. Please let me know. Thanks

//--- NEWMAN180  1/4/11
 //------------------------------------------------------------------------
 #include <LiquidCrystal.h>
 volatile unsigned int revPeriod; // Rotation Time
 volatile unsigned long prevTDCtime; // Previous time Hall Sensor Picked up
 volatile unsigned long TDCtime; // Current TDC Time
 volatile unsigned long nextTDC; // Predicted next TDC Time
 volatile unsigned long rpm; // Used to calculate RPM
 volatile unsigned int crankAngle; // Crankshaft Angle out of 360 degrees
 unsigned int a; // first crank angle of 180
 unsigned int a1; // second crank angle of 180
 unsigned int b; // first crank angle of 360
 unsigned int b1; // second crank angle of 360
 boolean TDCinterrupt1 = false; // States very first interrupt is ignored
 const int timingPin = 14; // Timing Button Assignment
 const int maintbuttonPin = 15; // Maint Button Assignment
 const int pushstartPin = 16; // Push Start Button Assignment
 int pushstartState = 0; // Push Start Button State
 int maintbuttonState = 0; // Maint Button State
 int buttonstate = 0; // Used to count button pushes for Timing
 int buttonpushcounter = 0;
 int lastbuttonstate = 0; 
 int start; // Used when declaring Start Mode Function  
 int timing; // Used when declaring Timing Mode Function
 int maint; // Used when declaring Maint Mode Function
 const int V17Pin = 9; // Valves 1 and 7 assigned to Pin 3  Intake
 const int V35Pin = 11; // Valves 3 and 5 assigned to Pin 4  Intake
 const int V28Pin = 10; // Valves 2 and 8 assigned to Pin 5  Exhaust
 const int V46Pin = 12; // Valves 4 and 6 assigned to Pin 6  Exhaust
 LiquidCrystal lcd (8,7,6,5,4,3);
  
  void setup()
 {
    // Communicate to Serial Port
   Serial.begin (9600);
   lcd.begin(16,2);
   pinMode (V17Pin, OUTPUT); // Valve 1&7 as Output
   pinMode (V35Pin, OUTPUT); // Valve 3&5 as Output
   pinMode (V28Pin, OUTPUT); // Valve 2&8 as Output
   pinMode (V46Pin, OUTPUT); // Valve 4&6 as Output
   pinMode (pushstartPin, INPUT); // Push Start Button Input
   pinMode (timingPin, INPUT); // Timing Button Input
   pinMode (maintbuttonPin, INPUT); // Maint Button Input
   rpm = 0;
   nextTDC = 1;
   lcd.setCursor(0,0);
   lcd.print("ON");
   attachInterrupt(0,TDCinterrupt, RISING); // Interrupt 0 is Pin 2 Hall Effect Sensor  
   digitalWrite(2,HIGH);
   delay(3500);
 }
  
 void TDCinterrupt()
{
  TDCinterrupt1 = true;  // First interrupt has passed
  prevTDCtime = TDCtime;  // save last TDC
  TDCtime = micros();  // save THIS TDC
  revPeriod = TDCtime - prevTDCtime;  // calculate revolution period
  if (prevTDCtime > TDCtime)  {
  revPeriod = TDCtime - prevTDCtime + 2147483648;}  // correct for micros() overflow every 71 minutes
  nextTDC = TDCtime + revPeriod;  
  rpm = 60000000L/revPeriod; // Calculate RPM divide rotation period by the difference of micros to minute
 }

void loop ()
{
  if ( TDCinterrupt1)
  {
    cli // Disable Interrupt
    crankAngle = map(micros(), TDCtime, nextTDC, 0, 360);  // Change time into degrees
    sei(); // Enable Interrupt
    crankAngle = constrain(crankAngle,  0, 360); // Limit crankshaft vales from 0 to 360
  }

//----------------------------------  Button Counter Control
buttonstate = digitalRead(timingPin);
  // Update Button Counter
  if ((buttonstate != lastbuttonstate) && (buttonstate == HIGH)){ buttonpushcounter++;} // If the buttonstate is greater than last, and it is high, increase counter
  lastbuttonstate = buttonstate;
  if( buttonpushcounter > 9) {buttonpushcounter = 0;} // If buttonstate has been pushed more than 9 times, reset to 0

//--------------------------------------- Button Timing Control 
 timing = timingcontrol(); // 
 timing; // Calling Timing Control Function

//--------------------------------  Timing Center
  if ( crankAngle >= a && crankAngle <= a1)// If the crankshaft is inbetween 1 to 179 degrees of revolution then a and a1 from timing function
  {
    digitalWrite(V17Pin, HIGH); // Fire Valve 1 and 7 (Intake)
    digitalWrite(V46Pin, HIGH); // Fire Valve 4 and 6 (Exhaust)
    Serial.println("Intake (1,7) Exhaust (4,6) Fired 100%"); // Print Status and Percent of Valve Open
    lcd.setCursor(12,1);
    lcd.print ("++");
  }
  else
  {
    digitalWrite(V17Pin, LOW); // Don't Fire Valve 1 and 7 (Intake)
    digitalWrite(V46Pin, LOW); // Don't Fire Valve 4 and 6 (Exhaust)
     lcd.setCursor(12,1);
     lcd.print ("  ");
  }
 
  if ( crankAngle >= b && crankAngle <= b1) // If the crankshaft is inbetween 181 to 359 degrees of revolution then use b and b1 from timing function
  {
     digitalWrite(V35Pin, HIGH); // Fire Valve 3 and 5 (Intake)
     digitalWrite(V28Pin, HIGH); // Fire Valve 2 and 8 (Exhaust)
     lcd.setCursor(14,1);
     lcd.print ("++");
     Serial.println("Intake (3,5) Exhaust (2,8) Fired 100%"); // Print Status and Percent of Valve Open
  }
  else
  {
    digitalWrite(V35Pin, LOW); // Don't  Fire Valve 3 and 5 (Intake)
    digitalWrite(V28Pin, LOW); // Don't Fire Valve 2 and 8 (Exhaust)
    lcd.setCursor(14,1);
    lcd.print ("  ");
  }

//------------------------------- RPM Display    
  lcd.setCursor(1,1);
  lcd.print(rpm,DEC);
//------------------------------- START MODE
  start = startmode();
  start; // Calling Start Mode function
//--------------------------------Maitenance Mode
  maint = maintmode();
  maint; // Calling Maint Mode Function
}

//-------------------------Start Mode Function
int startmode (){
  if (TDCtime == 0){
  pushstartState = digitalRead(pushstartPin);
  if (pushstartState == HIGH){  
  delay(3000);
  Serial.print ("** Start MODE **"); 
           lcd.clear();
           lcd.setCursor(3,0);
           lcd.print ("Start Mode"); 
 // Open All Valves for 3.5 Seconds          
           digitalWrite( V17Pin, HIGH);
           digitalWrite( V35Pin, HIGH);
           digitalWrite( V28Pin, HIGH);
           digitalWrite( V46Pin, HIGH);
           lcd.setCursor(0,1);
           delay(3500);
// Close All Valves and wait for 15 seconds
           digitalWrite( V17Pin, LOW);
           digitalWrite( V35Pin, LOW);
           digitalWrite( V28Pin, LOW);
           digitalWrite( V46Pin, LOW);
           delay(15000);
           lcd.clear();
           lcd.print("Starting");
    int i;
    int n;
    n = 5;
    for (i = 0; i < n ; i++) { // Do the following function  5 times in a row. Open one intake and all exhaust
           digitalWrite( V17Pin, HIGH);
           digitalWrite( V28Pin, HIGH);
           digitalWrite( V46Pin, HIGH);
           delay(100);
           digitalWrite( V28Pin, LOW);// Open and close exhaust every 10th of a second
           delay(100);}
    int j;
    int m;
    m = 5;
    for (j = 0; j < m ; j++) { // Do the following function  5 times in a row. Open other intake and all exhaust                    
           digitalWrite( V35Pin, HIGH);
           digitalWrite( V28Pin, HIGH);
           digitalWrite( V46Pin, HIGH);
           delay(100);
           digitalWrite( V46Pin, LOW);// Open and close exhaust every 10th of a second
           delay(100);}
else{    
    lcd.setCursor(0,0);
    lcd.print("START *KEY*");// If tdc time still equals 0, Please hit start button again    
}
  }
    }
 //-------------------------------- Timing Control Function
 int timingcontrol()
{
  a = 1;
  b = 181;

  a1 = 180 - 18 * buttonpushcounter; // a1 is upper end of first half of crank angle
  if (buttonpushcounter ==0) a1 = a1 -1;  // When buttonpushcounter equals 0, it demands a seperate function to equal 179 rather than 0
  b1 = a1 + 180; // b1 is the upper end of the second half of crank angle
  
  lcd.setCursor(12,0);
  lcd.print(100-10*buttonpushcounter); // print value of timing
  lcd.print("%");
} 
//-------------------------------- Maint Mode Function
 int maintmode() {
    maintbuttonState = digitalRead(maintbuttonPin);
   if (maintbuttonState == HIGH){
     cli(); // disable interrupt
     TDCtime = 0; // set all values to 0 to enforce no "automatic" rotation
     nextTDC = 0;
     prevTDCtime = 0;
     lcd.clear();
     lcd.print("Maintenance MODE");
     digitalWrite(V28Pin,HIGH); // Open all Exhaust and stay open as long as maintbutton is high
     digitalWrite(V46Pin,HIGH);
  }
     
   }

I don't know if I implemented it correctly

If it does compile you can check it.

If it does compile you can check it.

it compiles, I just didn't know if I applied the it to the "formula" correctly. I guess since you didn't make a comment, I did :wink:
I'm still not sure about using the delays in start mode though. I feel like I'll miss an important interrupt.