Variable losing value, possible overflow ?

Hello,

I'm currently developing a GPS Tracker based on ATMega328, a GPS Module u-blox NEO-6M-V2 and a LCD screen.
My project has a button to select the information I show on the display. The GPS send the data every 30sec using SoftwareSerial.

I use interrupt to manage button push (using variable "selectedMode" that i increment on each push).
The interrupt is also triggered when the GPS sends information.

The problem : sometimes the variable "selectedMode" goes to 0 so my display goes to the 1st page event if I don't push the button.
May it be the serial communication from the GPS that overflow and erase my variable ?
I don't see any reason ?

Here is my code (I removed some parts for brevity) :

#define F_CPU 16000000

#include <Ports.cpp>
#include <TinyGPS++.cpp>
#include <SoftwareSerial.cpp>
#include <LiquidCrystal.cpp>

#define MODE_OFF 4
// Set pins.
#define BUTTON_PIN 2    // The number of the push-button pin.
#define LCD_LIGHT_PIN A4 // The number of the pin where anode of the display backlight is.

#define LCD_LIGHT_ON_TIME 10000 // How long (in milliseconds) should lcd light stay on?
volatile int val;                        // variable for reading the pin status
volatile int val2;                       // variable for reading the delayed status

static const int RXPin = 12, TXPin = 0;
// The TinyGPS++ object
TinyGPSPlus gps;

// The serial connection to the GPS device
SoftwareSerial gpsSerial(RXPin, TXPin);

byte interruptPin = 0;
unsigned int currentLcdLightOnTime = 0;

volatile byte selectedMode = 0;
volatile byte oldMode = -1;

const int sentenceSize = 64;

char gpsSentence[sentenceSize];

// Initialize the LiquidCrystal library with the numbers of the interface pins.
// LiquidCrystal(rs, e, d4, d5, d6, d7)
LiquidCrystal lcd(8, 9, 5, 6, 7, 3);

ISR(WDT_vect) {
	Sleepy::watchdogEvent();
}

void modeSelectorInterrupt() {
	val = digitalRead(BUTTON_PIN);			// read input value and store it in val
	delay(10);								// 10 milliseconds is a good amount of time for debouncing
	val2 = digitalRead(BUTTON_PIN);			// read the input again to check for bounces
	if (val == val2)
	{
		if (val == HIGH) {
			++selectedMode;
			if (selectedMode == MODE_OFF + 1)
			{
				selectedMode = 0;
			}
		}
	}
}

// code removed

void readGps()
{
	static int i = 0;
	for (unsigned long start = millis(); millis() - start < 1000;)
	{
		while (gpsSerial.available())
		{
			char ch = gpsSerial.read();
			
			gps.encode(ch);
			if (ch != '\n' && ch != '\r' && i < sentenceSize)
			{
				gpsSentence[i] = ch;
				++i;
			}
			else
			{
				gpsSentence[i] = '\0';
				i = 0;
				//Serial.println(gpsSentence);
			}
		}
	}
	
}

void getInfos()
{
	readGps();
	
	lcd.clear();
	lcd.print(selectedMode);
	lcd.print("-");
	lcd.print(gpsSerial.overflow());
	lcd.print("-");
	switch (selectedMode) {
		case 0:
		LcdOn();
		displayPosition();
		break;
		case 1:
		LcdOn();
		displayDateTime();
		break;
		case 2:
		LcdOn();
		displaySats();
		break;
		case 3:
		LcdOn();
		displaySpeed();
		break;
		case MODE_OFF:
		LcdOff();
		break;
	}
}

void setup() {
	gpsSerial.begin(9600);
	// Set the lcd number of columns and rows.
	lcd.begin(16, 2);

	attachInterrupt(interruptPin, modeSelectorInterrupt, RISING);

	// Set the push-button pin as an input.
	pinMode(BUTTON_PIN, INPUT);
	// Set the lcd display backlight anode pin as an output.
	pinMode(LCD_LIGHT_PIN, OUTPUT);
	// Set the lcd display backlight anode pin to high - lcd light on.
	digitalWrite(LCD_LIGHT_PIN, HIGH);

	getInfos();
}

void loop() {
	getInfos();
	//delay(1000);
	//Sleepy::loseSomeTime(60000);
	Sleepy::powerDown();
}

Please help

You should not have any delay() in an ISR. A 10ms delay is about 500 times longer than your entire ISR should take to execute.

As far as I know delay() doesn't work in an ISR anyway. That may mean that you are getting more switch increments than you think.

Why bother with an ISR just to read a switch? Just read the switch in loop().

...R

Thank you for your answer.

In fact the code I pasted was just a test, and the bug is the same even if I put nothing in the ISR routine.
I tried with the following code and it's the same problem.

If i add debud Serial.println.... before the selectedMode = 0, it's never written on the output unless if I push the button. But the value of selectedMode goes to 0 and my display shows what in the function "displayPosition".

I use an ISR to wake up the atmega when I push the button (I powerdown the cpu to save battery ). Am I wrong ?

#define F_CPU 16000000

#include <Ports.cpp>
#include <TinyGPS++.cpp>
#include <SoftwareSerial.cpp>
#include <LiquidCrystal.cpp>

#define MODE_OFF 4
// Set pins.
#define BUTTON_PIN 2    // The number of the push-button pin.
#define LCD_LIGHT_PIN A4 // The number of the pin where anode of the display backlight is.

#define LCD_LIGHT_ON_TIME 10000 // How long (in milliseconds) should lcd light stay on?
volatile int val;                        // variable for reading the pin status
volatile int val2;                       // variable for reading the delayed status
int buttonState;                // variable to hold the button state


static const int RXPin = 12, TXPin = 0;
static const uint32_t GPSBaud = 9600;
// The TinyGPS++ object
TinyGPSPlus gps;

// The serial connection to the GPS device
SoftwareSerial gpsSerial(RXPin, TXPin);

byte interruptPin = 0;
unsigned int currentLcdLightOnTime = 0;

volatile byte selectedMode = 0;
volatile byte oldMode = -1;

const int sentenceSize = 64;

char gpsSentence[sentenceSize];

// Initialize the LiquidCrystal library with the numbers of the interface pins.
// LiquidCrystal(rs, e, d4, d5, d6, d7)
LiquidCrystal lcd(8, 9, 5, 6, 7, 3);

ISR(WDT_vect) {
	Sleepy::watchdogEvent();
}

void modeSelectorInterrupt() {
	
}


void displayPosition()
{
	if(gps.location.isValid())
	{
		lcd.print("Lat : ");
		lcd.print(gps.location.lat(), 6);
		lcd.setCursor(0,1);
		lcd.print("Lng : ");
		lcd.print(gps.location.lng(), 6);
	}
	else
	{
		lcd.print("No position");
	}
}


void displayDateTime()
{
	TinyGPSDate d = gps.date;
	if(d.isValid())
	{
		char sz[12];
		sprintf(sz, "%02d/%02d/%04d ", d.day(), d.month(), d.year());
		lcd.print(sz);
	}
	else
	{
		lcd.print("No date");
	}
	lcd.setCursor(0,1);
	TinyGPSTime t = gps.time;
	if(t.isValid())
	{
		char sz[12];
		sprintf(sz, "%02d:%02d:%02d ",t.hour(), t.minute(), t.second());
		lcd.print(sz);
	}
	else
	{
		lcd.print("No time");
	}
}


void displaySats() // and altitude
{
	if(gps.satellites.isValid())
	{
		lcd.print("Satellites : ");
		lcd.print(gps.satellites.value());
	}
	else
	{
		lcd.print("No satellites");
	}
	lcd.setCursor(0,1);
	if(gps.altitude.isValid())
	{
		lcd.print("Altitude : ");
		lcd.print(gps.altitude.value());
	}
	else
	{
		lcd.print("No altitude");
	}
}


void displaySpeed()
{
	lcd.print(gps.speed.knots());
	lcd.print(" kts");
	lcd.setCursor(0,1);
	lcd.print(gps.speed.kmph());
	lcd.print(" km/h");
}

void LcdOn()
{
	if (digitalRead(LCD_LIGHT_PIN) == LOW)
	{
		lcd.begin(16, 2);
		digitalWrite(LCD_LIGHT_PIN, HIGH);
	}
}
void LcdOff()
{
	digitalWrite(LCD_LIGHT_PIN, LOW);
}

void readGps()
{
	static int i = 0;
	for (unsigned long start = millis(); millis() - start < 1000;)
	{
		while (gpsSerial.available())
		{
			char ch = gpsSerial.read();
			
			gps.encode(ch);
			if (ch != '\n' && ch != '\r' && i < sentenceSize)
			{
				gpsSentence[i] = ch;
				++i;
			}
			else
			{
				gpsSentence[i] = '\0';
				i = 0;
				//Serial.println(gpsSentence);
			}
		}
	}
	
}

void getInfos()
{
	readGps();
	
	lcd.clear();
	lcd.print(selectedMode);
	lcd.print("-");
	lcd.print(gpsSerial.overflow());
	lcd.print("-");
	switch (selectedMode) {
		case 0:
		LcdOn();
		displayPosition();
		break;
		case 1:
		LcdOn();
		displayDateTime();
		break;
		case 2:
		LcdOn();
		displaySats();
		break;
		case 3:
		LcdOn();
		displaySpeed();
		break;
		case MODE_OFF:
		LcdOff();
		break;
	}
}

void modeSelector() {
	val = digitalRead(BUTTON_PIN);			// read input value and store it in val
	delay(10);								// 10 milliseconds is a good amount of time for debouncing
	val2 = digitalRead(BUTTON_PIN);			// read the input again to check for bounces
	if (val == val2)
	{
		if (val == HIGH) {
			++selectedMode;
			if (selectedMode == MODE_OFF + 1)
			{
				selectedMode = 0;
			}
		}
	}
}

void setup() {
	//Serial.begin(115200);

	gpsSerial.begin(9600);
	// Set the lcd number of columns and rows.
	lcd.begin(16, 2);

	// Print the message to the lcd.
	lcd.setCursor(0, 0); // First row.
	lcd.print("GPS");
	lcd.setCursor(0, 1); // Second row.
	lcd.print("Initialisation");
	attachInterrupt(interruptPin, modeSelectorInterrupt, RISING);

	// Set the push-button pin as an input.
	pinMode(BUTTON_PIN, INPUT);
	// Set the lcd display backlight anode pin as an output.
	pinMode(LCD_LIGHT_PIN, OUTPUT);
	// Set the lcd display backlight anode pin to high - lcd light on.
	digitalWrite(LCD_LIGHT_PIN, HIGH);

	getInfos();
}

void loop() {
	modeSelector();
	getInfos();
	//delay(1000);
	//Sleepy::loseSomeTime(60000);
	Sleepy::powerDown();
}

I don't understand why ?

			gps.encode(ch);

gps.encode() returns true (the sentence is complete) or false (the sentence is not complete). Why are you ignoring the return value?

	for (unsigned long start = millis(); millis() - start < 1000;)
	{

Reading GPS data for one second is silly. The GPS does not output a continuous stream of data. It outputs bursts of data periodically. You should read and store all available data, then parse the data when you get a complete sentence.

	readGps();
	
	lcd.clear();
	lcd.print(selectedMode);
	lcd.print("-");
	lcd.print(gpsSerial.overflow());
	lcd.print("-");
	switch (selectedMode) {
		case 0:
		LcdOn();
		displayPosition();

The readGps() function should return true (got data) or false (no complete sentence). The displayPosition() function should NOT be called until there IS good data to parse and display.

Thank you for your answer.
I use this code :

PaulS:

	for (unsigned long start = millis(); millis() - start < 1000;)
{


Reading GPS data for one second is silly. The GPS does not output a continuous stream of data. It outputs bursts of data periodically. You should read and store all available data, then parse the data when you get a complete sentence.

because if I don't (like in the code below), the GPS object doesn't get enought NMEA sentence to be "fed" correctly and I don't have any values.
With this code :

void readGps()
{
	static int i = 0;
	char gpsSentence[sentenceSize];

	Serial.print("readGPS start : ");
	Serial.println(millis());
	while (gpsSerial.available())
	{
		char ch = gpsSerial.read();
		if (ch != '\n' && i < sentenceSize)
		{
			gpsSentence[i] = ch;
			++i;
		}
		else
		{
			gpsSentence[i] = '\0';
			i = 0;
		}

		if(gps.encode(ch))
		{
			Serial.print("sentence complete : ");
			Serial.println(gpsSentence);
		}

	}
	Serial.print("readGPS end : ");
	Serial.println(millis());
	
}

I get this output :

readGPS end : 44974
start : 45012
readGPS end : 46606
readGPS start : 46650
readGPS end : 47132
readGPS start : 47176
readGPS end : 47659
readGPS start : 47703
readGPS end : 48185
readGPS start : 48229
readGPS end : 48711
readGPS start : 48755
readGPS end : 48974
: 49019
sentence complete : $GPVTG,,,,,,,,,N*30
,,,,010914,,,N*75

readGPS end : 49772
readGPS start : 49816
readGPS end : 50301
rt : 50340
readGPS end : 50342
readGPS start : 50386
readGPS end : 50867
readGPS start : 50911
readGPS end : 51401
readGPS start : 51445
readGPS end : 51664
 : 51709
sentence complete : $GPVTG,,,,,,,,,N*30
,,,,010914,,,N*77

readGPS end : 53518
readGPS start : 53563
readGPS end : 54053
readGPS start : 54097
readGPS end : 54590
readGPS start : 54634
readGPS end : 54853
: 54898
sentence complete : $GPVTG,,,,,,,,,N*30
,,,,010914,,,N*72

readGPS end : 56708
readGPS start : 56752

even when I let the module on for 10min.
I guess that the cpu goes to sleep and miss some communication sent by the GPS ?

because if I don't (like in the code below),

which doesn't use gps.encode(). Why not?

PaulS:

because if I don't (like in the code below),

which doesn't use gps.encode(). Why not?

it does :

if(gps.encode(ch))
{
	Serial.print("sentence complete : ");
	Serial.println(gpsSentence);
}

maybe I don't understand what you mean (sorry if it's the case, it's my 1st arduino project)
I could use the return value to display or not but it's already done in my displayPosition funtion :

void displayPosition()
{
	if(gps.location.isValid())
	{
		lcd.print("Lat : ");
		lcd.print(gps.location.lat(), 6);
		lcd.setCursor(0,1);
		lcd.print("Lng : ");
		lcd.print(gps.location.lng(), 6);
	}
	else
	{
		lcd.print("No position");
	}
}

it does :

Sorry. I missed that. But, I don't see the purpose of this:

		if(gps.encode(ch))
		{
			Serial.print("sentence complete : ");
			Serial.println(gpsSentence);
		}

The calling function has NO idea whether the Arduino received a complete sentence. It can NOT read the Serial Monitor.