Saisnmart 2004 LCD display and microswitch, Display not updating.

[system]

Hi all. Having some problems with a sainsmart LCD (as i have seen so are quite a few people), a little different to the usual though.

I'm familiar with the Arduino environment but can't figure this one out.
For a small project i'm undertaking i'm using an interrupt to to count the number of revolutions of a wheel and then calculating the speed and wanting to then output this to an lcd display on a small go kart.

i've managed to get this to read into the serial monitor correctly, and set up the display so that it will output some text using I2C and help from http://arduino-info.wikispaces.com/LCD-Blue-I2C .

I am using arduino 1.0.5 and have tried both arduino unoR3 and Mega2560

The Problem:
I have so far been unable to get the display to then output the calculated value, it appears to output everything initially (so that it says the speed is 0) and then no longer update the display.

I have tried multiple methods to this including creating a function for the updating display, no longer writing the result to serial (to view in serial monitor), as well as creating a function to calculate the speed instead of lines of code in a loop. I've rearranged the order in which the functions are called.

Help on this matter would be greatly appreciated (i'm sure its just something simply i've missed). Hopefully i've explained it well enough. :S

The code:
#include <LCD.h>
#include <LiquidCrystal.h>
#include <LiquidCrystal_I2C.h>
#include <Wire.h>

////////initialise pins//////////////

#define BACKLIGHT_PIN 3
#define En_pin 2
#define Rw_pin 1
#define Rs_pin 0
#define D4_pin 4
#define D5_pin 5
#define D6_pin 6
#define D7_pin 7
const int ButtonPin = 3; // microswitch speedo pin

///////////////////////////////////////////////////////////////////////

////// LCD Setup //////////

#define I2C_ADDR 0x3F // Define I2C Address where the PCF8574A is
LiquidCrystal_I2C lcd(I2C_ADDR,En_pin,Rw_pin,Rs_pin,D4_pin,D5_pin,D6_pin,D7_pin);
/////////////////////////////////////////////////////////////////

////////Variable Defining///////
int Speed=0;
int KartSpeed;
double MaxSpeed=0;
int Calc;
int FuelRate=0;
volatile int NbTopsSwitch = LOW;
/////////////////////////////////////////////////////////////////

void setup()
{
//////Initialise Serial////
Serial.begin(9600);
//////////////////////////

//////PinSetup/////////
pinMode(ButtonPin, INPUT);
/////////////////////////

////////LCD Setup////////
lcd.begin (20,4);
lcd.setBacklightPin(BACKLIGHT_PIN,POSITIVE);
lcd.setBacklight(HIGH);

////////////////////////////////////////

///////Attach Interrupts//////////////
attachInterrupt (1, buttonhigh, RISING); //and the interrupt is attached to trigger on the rising edge of the switch press

}

void loop()
{
/////// Call the display function///////
updatedisplay();
/////////////////////////////////

////////// Call the Speed Calc Function
SpeedCalc();
}

////// Update display function///////
void updatedisplay()
{
lcd.clear(); // clear everything from the LCD
lcd.home (); // go home
lcd.print("Speed =");
lcd.setCursor(11,0); // set cursor to 11th column on line 1
lcd.print(Speed);
lcd.setCursor(0,1); // set cursor to 1st column on line 2
lcd.print("MaxSpeed = MPH");
lcd.setCursor(11,1); // set cursor to 11th column on line 2
lcd.print(MaxSpeed);
lcd.setCursor(0,2); // set cursor to 1st column on line 3
lcd.print("EngineTemp = oC");
lcd.setCursor(0,3); // set cursor to 1st column on line 4
lcd.print("FuelFlow = LPH");
lcd.setCursor(11,3); // set cursor to 11h column on line 4
lcd.print(FuelRate);
}

///////////////////////////

//////Speed calc Function
void SpeedCalc()
{
NbTopsSwitch = 0; //Set NbTops to 0 ready for calculations
sei(); //Enables interrupts
delay (1000); //Wait 1 second
cli(); //Disable interrupts
Speed = ((NbTopsSwitch/19588.3)*3600); // speed is the number of revolutions in one second (determined by nbTopsSwitch)
// Divided by the number of revolutions in a mile
// multiplied by the number of seconds in an hour to give MPH
Serial.print (Speed, DEC); //Prints the number calculated above
Serial.print (" MPH\r\n"); //Prints "MPH" and returns a new line

}

///////////////////////////////////////////

//////Called Interrupt///////
void buttonhigh()
{
NbTopsSwitch++; ////count the rising and falling edge of the switch pulse
}

///////////////////

..... Code Adapted from http://arduino-info.wikispaces.com/LCD-Blue-I2C, and http://www.seeedstudio.com/wiki/G3/4_Water_Flow_sensor.

I have attached images of the back of the LCD, what i get as an output, the sensor shield i'm using and reading in a speed to the serial monitor compared to the display.

[bperrybap]

First, some unreleated items.
When using fm's library you only include the header file for the interface class.
i.e. you should not include LCD.h and LiquidCrystal.h

The code is misuing the API function setBacklight().

setBacklight(dimvalue);

is for setting a backlight intensity not for turning on/off the backlight.
Since the value of HIGH is defined to be 1 the code is asking the library to set
the backlight intensity to 1 or 1/255 of its brightness which is very dim.
In this case it happens to still work because the library gives you the lowest
dim value it can which for this hardware, which is full on.
My recommendation for this is to use the newer constructor to setup the backlight
parameters. That way the begin() call will turn on the backlight as well.
example:

LiquidCrystal_I2C   lcd(I2C_ADDR,En_pin,Rw_pin,Rs_pin,D4_pin,D5_pin,D6_pin,D7_pin, BACKLIGHT_PIN, POSITIVE);

Then remove these two lines:

  lcd.setBacklightPin(BACKLIGHT_PIN,POSITIVE);
  lcd.setBacklight(HIGH);

If you want to turn the backlight on/off later use

backlight(); // turn on backlight
noBacklight(); turn off backlight

---

Then there is the sei() an cli() calls.
Those should use the arduino calls

interrupts(); // enable interrupts
noInterrupts(); // disable interrupts

While they currently are 0 and 1 don't assume that LOW is 0 or HIGH is 1
and use them in place of a 0 or 1.
i.e. use 0 for this line:

volatile int NbTopsSwitch = 0;

---

But you could leave off the assignem altogether as 0 is assigned
to all globals by default.

Doing those types things will ensure that the code will be more portable and
will work on different LCD types as well as other platforms like chipkit, DUE, Maple, etc

---

---

As far as the actual issue in the code goes, it looks to me like
the code in SpeedCalc() is messing with the interrupt mask
and leaving interrupts disabled.
"as is" it will end up disabling all interrupts before it leaves
and then never re-enable them.
When interrupts are disabled, lots of things won't work,
serial, i2c, millis() timers, any attached interrupt functions.

Maybe the enable interrupts was meant to be after the Speed calculation
to ensure the atomicity of the nBTopsSwitch variable during the calculation?
Maybe the code should be:

//////Speed calc Function
 void SpeedCalc()
 {
   NbTopsSwitch = 0;   //Set NbTops to 0 ready for calculations
  delay (1000);   //Wait 1 second
  noInterrupts();      //Disable interrupts
  Speed = ((NbTopsSwitch/19588.3)*3600); // speed is the number of revolutions in one second (determined by nbTopsSwitch)
                                         // Divided by the number of revolutions in a mile
                                         // multiplied by the number of seconds in an hour to give MPH
  interrupts(); // enable interrupts
 Serial.print (Speed, DEC); //Prints the number calculated above
  Serial.print (" MPH\r\n"); //Prints "MPH" and returns a  new line
 
 }

If you wanted to further reduce the time interrupts are blocked you could
declare a temporary local and only mask the interrupts while it is grabbed from NbTopsSwitch.

--- bill

[system]

Hi Bill, Thanks!!! worked first time. Thanks for the help