reading gps data -- one little hiccup

Go To Last Post
12 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I have a gps unit hooked up to USARTD0 on my ATXmega256A3. It came out of the box at 4800 baud, I set it to 9600... I've been able to turn off all but the RMC message and can still query the other ones when i need them. I've been working with the RMC message, but this happens with the others too.

percent signs have been replaced with #

First of all, here's the code:

void gps_parse()
{
   printf("\nparsing: #s ",gps_string);

   char gps_time [11];
   strcpy(gps_time,"");
   char gps_valid;
   char gps_lattitude [10];
   strcpy(gps_lattitude,"");
   char gps_ns;	  
   char gps_longitude [11];
   strcpy(gps_longitude,"");
   char gps_ew;	 
   char gps_speed [5];
   strcpy(gps_speed,"");
   char gps_heading [7];
   strcpy(gps_heading,"");
   char gps_date [7];
   strcpy(gps_date,"");

   char* token;
   char delim = ',';
   token = strtok(gps_string, &delim);
   if(strcmp(token,"$GPRMC") == 0)
      gps_message_type = RMC;
   else if (strcmp(token,"$GPGLL") == 0)
      gps_message_type = GLL;
   switch(gps_message_type)
   {
      case RMC:

	 strcpy(gps_time, strtok(NULL, &delim));
	 gps_valid = *strtok(NULL, &delim);
	 strcpy(gps_lattitude, strtok(NULL, &delim));
	 gps_ns = *strtok(NULL, &delim);
	 strcpy(gps_longitude, strtok(NULL, &delim));
	 gps_ew = *strtok(NULL, &delim);
	 strcpy(gps_speed, strtok(NULL, &delim));
	 strcpy(gps_heading, strtok(NULL, &delim));
	 strcpy(gps_date, strtok(NULL, &delim));
	 break;
      default:
	 printf("GPS PARSE ERROR\n");
	 break;
   }
	 printf("time: \n#s | ", gps_time);
	 printf("valid: #c | ", gps_valid);
	 printf("lat: #s",gps_lattitude);
	 printf("#c | ", gps_ns);
	 printf("long: #s", gps_longitude);
	 printf("#c | ",gps_ew);
	 printf("speed: #s | ", gps_speed);
	 printf("heading: #s | ", gps_heading);
	 printf("date: #s \n", gps_date);
   strcpy(gps_string,"");
}

gps_string is, well, a string that has a full NMEA message. As the code executes, every time the GPS module sends a new update (1 Hz), what I see in my terminal is something like:

parsing: $GPRMC,070923.000,A,3851.1384,N,07715.5597,W,0.07,264.53,251209,,*19
time: 
070923.000 | valid: A | lat: 3851.1384N | long: 07715.5597W | speed: 0.07 | heading: 264 | date: 53 

which tells me that it's entered the parsing function, with that $GPRMC string as the input so I can double-check the output.

It seems to be a problem with the strtok() function. You can see that once it gets to the value for the heading (265.53), it treats the period (".") as the delimiter, even though otherwise it's been using a comma

why does it do this? what can I do about it? thanks :)

by the way, gps_string (the full NMEA sentence) is being populated by an ISR:

ISR(USARTD0_RXC_vect)
{
   char c;
   c = USARTD0.DATA;
   strncat(&gps_string, &c, 1);
   if (c == '\n')
   {
      gps_parse();
   }
}
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I have several issues with your code from a stylistic and structure point of view. For starters, it is unwise to call functions from within an ISR to start. Instead your ISR, should simply load a buffer, then your main-loop code should read that into the string, and parse it. You are also mixing variable definitions with code, this makes things hard to read, it is far better to have your definitions at the top of the code block (the function in this case) and then perform any initializations. [that cannot be performed automatically for you in the definition stage]

But your problem is that you have defined delim to be of type char, but you are passing it as a string to strtok, by using a pointer to the char. Problem is that a string is not a simple pointer to a char, it as a pointer to an array of chars, with the last char element having a null value (the null terminator).

Here's a quick fixup/cleanup of your GPS parse code as it is.

void gps_parse()
{
   char gps_time [11] = "";
   char gps_valid;
   char gps_lattitude [10] = "";
   char gps_ns;    
   char gps_longitude [11] = "";
   char gps_ew;   
   char gps_speed [5] = "";
   char gps_heading [7] = "";
   char gps_date [7] = "";
   char* token;
   char delim[] = ",";

   printf("\nparsing: #s ",gps_string);

   token = strtok(gps_string, delim);
   if(strcmp(token,"$GPRMC") == 0)
      gps_message_type = RMC;
   else if (strcmp(token,"$GPGLL") == 0)
      gps_message_type = GLL;

   switch(gps_message_type)
   {
      case RMC:

    strcpy(gps_time, strtok(NULL, delim));
    gps_valid = *strtok(NULL, delim);
    strcpy(gps_lattitude, strtok(NULL, delim));
    gps_ns = *strtok(NULL, delim);
    strcpy(gps_longitude, strtok(NULL, delim));
    gps_ew = *strtok(NULL, delim);
    strcpy(gps_speed, strtok(NULL, delim));
    strcpy(gps_heading, strtok(NULL, delim));
    strcpy(gps_date, strtok(NULL, delim));
    break;
      default:
    printf("GPS PARSE ERROR\n");
    break;
   }
    printf("time: \n#s | ", gps_time);
    printf("valid: #c | ", gps_valid);
    printf("lat: #s",gps_lattitude);
    printf("#c | ", gps_ns);
    printf("long: #s", gps_longitude);
    printf("#c | ",gps_ew);
    printf("speed: #s | ", gps_speed);
    printf("heading: #s | ", gps_heading);
    printf("date: #s \n", gps_date);
    strcpy(gps_string,"");
} 

From your code it displays a lack of understanding of C, and structured programming to a certain extend. I suggest you get yourself a good book on C to help strengthen your knowledge in this area.

Writing code is like having sex.... make one little mistake, and you're supporting it for life.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks, that was indeed the problem. However, the only functional change you made to my code was:
char delim [] = ",";
and changing the strtok calls to use delim instead of &delim. (delim now is actually equivalent to &delim[0], or &delim for that matter. Though avr-gcc recognizes &delim as an incompatible pointer type, the code functions exactly the same) Unless you plan to make more drastic changes to my code, don't tell me I have a lack of understanding of C or structured programming.

Also, I know it's bad practice to call a function from an interrupt handler. What I had when trying to do it without calling from the interrupt handler was

ISR(USARTD0_RXC_vect)
{
   char c;
   c = USARTD0.DATA;
   strncat(&gps_string, &c, 1);
   if (c == '\n')
      gps_message_complete = 1;
}

and my main loop code was

   while (1)
   {
      if (gps_message_complete == 1)
            gps_parse();
   }

gps_parse() resets gps_message_complete to 0, and copies gps_string to a local string, then copies a blank string to gps_string.

gps_message_complete is a globally defined int, initialized to 0. Using some printf statements for debugging, i found that the if statement in the main look is never executed. I can't figure out why this would be either. Got any ideas?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

scubabuddha wrote:
Thanks, that was indeed the problem. However, the only functional change you made to my code was:
char delim [] = ",";

That was by design, I had no desire to re-write your code from scratch to have better structure, instead I cleaned up the initialization to show how, and addressed the fault I outlined only. Further refinements were left to you.

scubabuddha wrote:

Also, I know it's bad practice to call a function from an interrupt handler. What I had when trying to do it without calling from the interrupt handler was

ISR(USARTD0_RXC_vect)
{
   char c;
   c = USARTD0.DATA;
   strncat(&gps_string, &c, 1);
   if (c == '\n')
      gps_message_complete = 1;
}

and my main loop code was

   while (1)
   {
      if (gps_message_complete == 1)
            gps_parse();
   }


But that is STILL calling an external function, you are calling strncat(). You are better off manually stuffing the string yourself. The easiest way to do this is to maintain the character position in the string using a global variable, or a static local.

scubabuddha wrote:

gps_message_complete is a globally defined int, initialized to 0. Using some printf statements for debugging, i found that the if statement in the main look is never executed. I can't figure out why this would be either. Got any ideas?

Did you also declare it as volatile?

Also if it is just a flag, use a char instead of an int, as a char can be read atomically, while an int cannot.

volatile unsigned char gps_message_complete = 0;

…

ISR(USARTD0_RXC_vect) 
{ 
   char c; 
   static unsigned char gps_message_index = 0; 
   
   c = USARTD0.DATA; 

   if(gps_message_complete) return;

   gps_string[gps_message_index++]  = c; 
   
   if (c == '\n')
  {
      gps_string[gps_message_index]  = 0; 
      gps_message_index = 0; 
      gps_message_complete = 1; 
  }
}

And here is a cleaner version of your gps_parse function again. Note the lack of any strcpy calls.

void gps_parse() 
{ 
   char *gps_time  =  NULL; 
   char gps_valid = 0; 
   char *gps_lattitude  = NULL; 
   char gps_ns = 0;    
   char *gps_longitude  = NULL; 
   char gps_ew = 0;    
   char *gps_speed  = NULL; 
   char *gps_heading  = NULL; 
   char*gps_date  = NULL; 
   char *token; 
   char delim[] = ","; 

   printf("\nparsing: #s ",gps_string); 

   token = strtok(gps_string, delim); 
   if(strcmp(token,"$GPRMC") == 0) 
      gps_message_type = RMC; 
   else if (strcmp(token,"$GPGLL") == 0) 
      gps_message_type = GLL; 

   switch(gps_message_type) 
   { 
      case RMC: 

    gps_time = strtok(NULL, delim); 
    gps_valid = *strtok(NULL, delim); 
    gps_lattitude = strtok(NULL, delim); 
    gps_ns = *strtok(NULL, delim); 
    gps_longitude = strtok(NULL, delim); 
    gps_ew = *strtok(NULL, delim); 
    gps_speed = strtok(NULL, delim); 
    gps_heading = strtok(NULL, delim); 
    gps_date = strtok(NULL, delim); 
    break; 
      default: 
    printf("GPS PARSE ERROR\n");
    return; 
    break; 
   } 
    printf("time: \n#s | ", gps_time); 
    printf("valid: #c | ", gps_valid); 
    printf("lat: #s",gps_lattitude); 
    printf("#c | ", gps_ns); 
    printf("long: #s", gps_longitude); 
    printf("#c | ",gps_ew); 
    printf("speed: #s | ", gps_speed); 
    printf("heading: #s | ", gps_heading); 
    printf("date: #s \n", gps_date);
 
    gps_string[0] = 0;
    gps_message_complete = 0;
}

And note that I did add some structure to your code in my previous post, by separating the variable definitions from actual code. Functionally it was equivalent, but syntactically it was quite different. This latest version takes things one step further by removing some un-necessary memory usage, and several function calls.

Note that your function, along with my edited version, will break if called with data that is not RMS or GLL data, as those cases are not handled, in fact the code will take action on those strings based on the type from the last handled case.

Writing code is like having sex.... make one little mistake, and you're supporting it for life.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

glitch wrote:

But that is STILL calling an external function, you are calling strncat(). You are better off manually stuffing the string yourself. The easiest way to do this is to maintain the character position in the string using a global variable, or a static local.
oh, good call. I'll work on that. Your fix looks nice and clean

glitch wrote:

Did you also declare it as volatile?

Also if it is just a flag, use a char instead of an int, as a char can be read atomically, while an int cannot.


no, i did not declare it volatile. I will now. and thanks for the tip about chars vs ints

i see i have more to learn than i thought... :oops:

what's the reasoning behind not calling a function from an interrupt handler?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Function calling & return overhead is substantial. You want to minimize the amount of time spent in ISRs, thus the advice about not calling functions from an ISR.

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ka7ehk wrote:
Function calling & return overhead is substantial. You want to minimize the amount of time spent in ISRs, thus the advice about not calling functions from an ISR.

Jim

Just to expand... In the main loop, the compiler only needs to save used scratch registers.

In an ISR the compiler has no idea what scratch registers may be in use at any given moment, thus it needs to save all those registers, as it also has no easy way to determine what registers the called function may use. Now in reality, it is not all 32 registers, the compiler only needs to save the registers it knows the called function may not preserve (it will not save registers that a called function is guaranteed to preserve if it alters it). But still this is considerable additional overhead.

Writing code is like having sex.... make one little mistake, and you're supporting it for life.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hmmmm....

I fully agree with glitch. The code is:
1. Hard to read
2. Hard to debug
3. Hard to uderstand (this may be a fault in my brain)

In addition to this the code uses quite a lot of ram. All those string constants are copied into Your ram during the startup.

Your ISR will crash if the message exceeds the size of gps_string. This may happen if there is some garbage in the message on the vicinity of the line break.

If I had to design a NMEA parser for production use this would not be the way to do it.

That all said - please read the following as a advice from a person who has been designing all sorts of parsers for 30+ years. Some of them in places where updating is not an option.

First of all - reading the data from the GPS unit should be done using an ISR - as You are doing it. The ISR should be lightweight only storing the characters into some sort of a buffer. That buffer should not be overflowed (or very bad things happen). I would use a circular buffer of no more than 8 characters.

The parser. You have some options here. The most versatile is a state machine that will interpret the message character-by-character. Making it this way will allow Your MCU do other tasks while the message is being interpreted - You just call the parser every now and then between Your daily routines.

Another way of doing the parseing is Your way - storing everything into an extra large buffer and then figuring it out in one go. The problem here is that You have to fill that buffer in an ISR and that the buffer will be slow to interpret as there is quite a lot of data to interpret. State machine will do this with at least 100-fold speed as it does the parsing as the string is received. Another anomaly with this type of solution is that it is hard to implement a decent error handling into it. The code tends to become large and therefore hard to read / maintain.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

eskoilola wrote:
The parser. You have some options here. The most versatile is a state machine that will interpret the message character-by-character. Making it this way will allow Your MCU do other tasks while the message is being interpreted - You just call the parser every now and then between Your daily routines.

Another way of doing the parseing is Your way - storing everything into an extra large buffer and then figuring it out in one go. The problem here is that You have to fill that buffer in an ISR and that the buffer will be slow to interpret as there is quite a lot of data to interpret. State machine will do this with at least 100-fold speed as it does the parsing as the string is received. Another anomaly with this type of solution is that it is hard to implement a decent error handling into it. The code tends to become large and therefore hard to read / maintain.


I don't quite understand your (finite?) state machine approach to this problem. It seems like it would require a huge number of states.

I'm noticing as I try to make this code more robust and versatile that it's getting very large and unwieldy.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The number of states in this case will not become too huge. It should be in the vicinity of 30. I consider that as a low figure.

For example when reading the prefix of the message - this would be one state. The state leaves when the message:
- is something that is detected eg. "$GPRMC"
- has become too long (another error)
- has been sitting too long unchanged (another error)

Another example would be reading a number:
- exits the state when a delimiter is received
- exits with error if the number is too long
- exits with error if the number has been sitting too long unchanged

The state machine should bail out to the start state whenever there is an error. It is essential to set it up this way. When done this way the state machine synchronizes itself with the incoming data stream.

I have done a GSM modem thingy which uses this technique - it was fun to write it. it also works quite well despite the complex messaging with the modem.

I attach the project files here. My site will be shutting down shortly as I am going to travel tomorrow.

The file You should be looking for is Message.c and Modem.c.

Feel free to plagiate it in case You start feeling better with the "huge" number of states. ;)

Attachment(s): 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

This thread has been extremely informative. I need help you guys, you seem pretty into this. I'm familiar with basics of micro controllers and C programming and i wanted to interface a GPS receiver module to an AVR Atmel. I've heard including in this post about doing that through interrupts and USARTs I'm not familiar with either and want to get a start up on this could any of guys guide me through this? Your help will be greatly appreciated THANKS!!!

@scubabuddha You seem to be familiar with my situation please help!

Last Edited: Sat. Mar 6, 2010 - 01:31 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

I've heard including in this posts about doing that through interrupts and USARTs I'm not familiar with either and want to get a start up on this could any of guys guide me through this?

Go over to the Tutorials forum and locate the tutorial on interrupt driven USART communication.

As of January 15, 2018, Site fix-up work has begun! Now do your part and report any bugs or deficiencies here

No guarantees, but if we don't report problems they won't get much of  a chance to be fixed! Details/discussions at link given just above.

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]