Default example for Asset Tracker GPS prints erratic NMEA sentences to serial

If you can also modify the library’s updateGPS() method to include something like:

	while (Serial1.available()) {
		gps.read();
	}

I think that’ll also greatly improve the data you’re reading in.

[quote=“Hypnopompia, post:22, topic:21356, full:true”]
If you can also modify the library’s updateGPS() method to include something like:
[ … ]
I think that’ll also greatly improve the data you’re reading in.[/quote]

I have two concerns:

1] I’m not seeing any garbage lines anymore. So I’m starting to wonder if they were just a string handling problem?

2] A tight serial read could actually work against us? If there are multiple types of NMEA strings with different sets of data (similar to the PMTK_SET_NMEA_OUTPUT_ commands), when we get several different lines in a row, wouldn’t the code end up just parsing the last one that made it through and skip the other strings which represent different formats?

That’s a really good point. I think you’re right in that there is a risk of missing a message if that loop reads in multiple messages. The assettracker library should probably check gps.newNMEAreceived() inside that loop inside updateGPS() and call gps.parse() if necessary. Basically everything already in the updateGPS() method needs to be wrapped inside a Serial1.available().

The parse() method will be responsible for parsing different messages and updating the properties of the gps object with the latest information. You might miss historical information of two of the same type of message however. (such as calculating speed) It probably comes down to what you’re trying to do.

I’m just noticing that void loop() {} just isn’t fast enough to handle the data coming from Serial1.

I’m a great debugger but I’m no programmer. I’m over my head with the specifics of Particle / GitHub / etc. Those suggestions seem good, but you’ve gone beyond my ability to hack in minor adjustments into a language that I don’t understand! :sweat_smile:

Say… you seem like a responsible kind of guy. I’ve been using your cool map project. How do you feel about taking what I’ve done so far and running with it?

PS: I have seen three very short malformed sentences in the past 20 minutes or so. Perhaps it wasn’t just a parsing problem and there still is that underlying serial read problem there after all.

EDIT: I left this running. Out of 314 sentences processed, 310 had the checksum in the right place and were good, 0 were detected with an invalid checksum, and 4 were mangled and did not have the checksum character in the right place.

[quote=“futurebird, post:7, topic:21356, full:true”]
did you update the github? I’ve been having issues with this code I don’t know if this is the issue, but I’d like to rule it out.[/quote]
I’ve only updated my personal copy. I don’t know your level of experience (and I have almost none), so forgive the explanation below if it is too simple.

In the Particle Build IDE, click on the libraries icon (left side bar, towards the bottom) then click on the blue ‘Contribute Library’ button towards the top. Put in the name jmccorm/TrackerDebug and click on the blue button below it. Click on the import button on the left hand side. You can try or modify its 1_GPS_Features example to see if it fixes your problem.

Please hurry and grab it. I think having it there prevents me from using the official library. At the very least, it confuses me. Ha!

Wait a second… you never said! What was the problem you were having?!

I’ve let the both the official AssetTracker library and my own version run for a while, and I have some rough statistics as well as some observations to share.

Statistics for the official AssetTracker library------------------------------
NMEA sentences observed (sample size) : 217
Messages that are actually corrupt: 2 (~1%)
Messages falsely detected as corrupt: 119 (~55%)
Messages parsed for location data: 96 (~44%)

Statistics for the modified AssetTracker library--------------------------------
NMEA sentences observed (sample size): 622
Messages that are actually corrupt: 6 (~1%)
Messages falsely detected as corrupt: 0 (0%)
Messages parsed for location data: 616 (~99%)

DISCUSSION: In the original library, the parser was being fed sentences which may or may not have a carriage return character at the end of the string. If the carriage return was not there, it believed that the checksum was missing from the string. Due to a programming error, if the checksum was missing, the string was actually sent on to be parsed for positional data.

Sentences which had a carriage return on the end were detected as having a checksum. That hexadecimal checksum value was read and then compared against the actual checksum of the string. Unfortunately, the calculation of the actual checksum began at the wrong offset, so in the observed cases, the checksum would not match, and those otherwise valid positional updates were not processed.

After looking at a large set of data, I actually did find a pattern of when carriage returns were and weren’t in the strings that were sent to the parser. The longest strings (for me, 65 characters) would not have a carriage return on the end, so they skipped the checksum routine and were parsed for their data. All other strings would have a carriage return.

My GPS was stationary and sitting on my desk. The longest strings happened when the GPS unit computed the direction of travel to be greater than 99.99 degrees. (So that’s 100-359.99 degrees.) It usually ended up creating streaks where GPS positional updates were processed for a number of updates, then not processed for a number of updates, then back again. If my GPS unit was actually in motion, the behavior of when I was or wasn’t getting updates would have been very different.

I did not subject either library to a faster flow of messages (by either increasing the update rate or enabling more types of messages to be sent). Based on my earlier experimentation, I believe that doing so create a buffer overflow in either library. I will leave that issue for someone who is more capable in addressing it.

SUMMARY: The Adafruit GPS library is significantly bugged but manages to operate in a degraded mode. A very large number of updates are discarded, and the discard rate seems to be influenced by speed or direction of travel (which creates longer or shorter strings). The checksum is never correctly evaluated, so any results are vulnerable to corruption, but this appears to be a rare event in the default configuration. The read routine from the GPS still needs to be enhanced to avoid dropping data when a higher throughput is enabled. Otherwise, the 1% read failure rate is probably acceptable in most cases.

EDIT: Thinking about this some more, we’re probably seeing the LF get cut off because of a UART overflow, right? Also the code for the “!” character (which clears the string and starts a brand new line) probably masks some overflow that is happening behind the scenes. If so, this library change is only going to reliably solve the problem of tracking a stationary object. If you start heading the “wrong” direction and go too fast, the GPS messages are going to be too long. We have to be able to read in larger messages for this library to work reliably.

2 Likes

Another library problem? This time in the intialization…

if I insert the following 10 second delay into the AssetTracker::gpsOn() function, the behavior massively changes. All of the sudden, I’m getting what looks like a ton of overflow…

gps.begin(9600);
delay(10000);
gps.sendCommand(PMTK_SET_NMEA_OUTPUT_RMCGGA);
delay(500);

Why? I think we are sending all of these initialization commands too soon, and they’re not actually taking effect. (How long of a delay is needed? Do we need to wait for the GPS to fully initialize? I don’t know.) So when we put in that 10 second delay which allows this command to actually work, we enabled two message types to be sent to us: $GPRMC and $GPGGA. That’s way too much data all at once.

However, if I make the following change…

gps.begin(9600);
delay(10000);
gps.sendCommand(PMTK_SET_NMEA_OUTPUT_RMCONLY);
delay(500);

…then I am regularly getting only valid $GPRMC strings. (I suspect that RMCONLY mode is the default behavior of the GPS, and so that’s what we end up with when our premature initialization commands are ignored.)

It might also be smart (but probably not necessary) to send a dummy command just before the real commands, just to make sure a garbage character didn’t work its way into the connection. Of course, looking for responses rather than just blindly sending commands it even smarter, but we’re on a microcontroller, so I don’t know if people want to the space on those routines.

EDIT: It is worth noting that the intended functionality of the library appears to be to have both the $GPGGA strings and $GPRMC enabled and streamed to us from the GPS module. Up until this point, we’ve been concentrating on just the $GPRMC message because that is all that was coming through (due to a bad initialization routine).

The $GPGGA strings are going to be longer than $GPRMC strings and, at least for me, they are always being truncated past 65 characters. The broken checksum code in the original library will pass them through. But once that is fixed, there is no way these $GPGGA lines can be read with this library’s existing read routines and our limited serial RX buffer size.

I think this additional problem (with premature initialization commands) reinforces my view that this library really needs a full review from top to bottom. I’m sorry that I can’t be the person to make all of these corrections, but hopefully I’m providing some value by digging into these problems, documenting what I find, fixing what I can, and calling for action.

1 Like

Hi @jmccorm,

I suspect the problem is that reading from Serial1 isn’t happening in a tight loop while characters are available. I have an example here that demonstrates a better read loop that should handle the extra data better:

Thanks,
David

1 Like

Guys,
Thanks so much for the work you’re putting in to debugging this. I have the same problem… which manifests as bad checksum errors. I have a sneaking suspicion that this is leading to my electron crashing after a few hours of running due to a buffer overrun or something similar . The symptom is the code seems to stop running after several hours … … e.g. no more data transmissions, no more serial debug.

_ Is there an updated version of Fancy Asset Tracker that I should be trying… or is the dust still swirling. Thanks again for your sleuth work here
-jc

Guys,

The reading on this debugging is fascinating. Just in case anyone reading this is interested in an alternative:

I’ve had a Photon reading NMEA sentences from an Adafruit ultimate gps board for a while as a soak test. It’s using the Tinygps++ library. As of this morning it’s processed 1.55 million NMEA sentences since the last reboot. Of those, 1.39 million passed the checksum test, the remainder didn’t. The photon is also reading temps, updating a display, and posting data to the Particle cloud as well as Blynk. So I figure that most of the missed checksums are because occasionally it doesn’t get back to the uart before the buffer overflows.

The library was pretty easy to use. Like I say, just fyi. I was late to the game & don’t have an official asset tracker so just by happenstance picked that library.

Y’all carry on!

Best,
Mike

Ps - a “while” is about four days. It’s spitting out all the default sentences as fast as it can.

1 Like

I have also gone for the TinyGPS++ and really do not regret it.

Only some smaller adjustments had to be done in the basic exemple like changing gps baudrate to 9600, changing ss( softserial) to Serial1 (hardware) and some string handling to get the Publish message according spec .

/Klas

1 Like

It is unfortunate that the expected length of a standard message from a GPS receiver is longer the the receive buffer on the Particle Photon/Electron.

What you propose is a good workaround for a number of situations..My concern would be that any code would have to center around constantly being ready to pull data from the serial connection. Such a program couldn't go off and do anything else for any period of time. Or it would have to be really smart about when it goes off to do something else.

If we could rewind the clock, back before the GPS module was released, I might have wanted some slightly custom firmware on the chip to generate shorter messages. (We could go with fixed length messages without field delimiters. Or we could do a binary data transfer instead of numeric ASCII.)

Today, what makes the most sense to me would be exposing the (currently private) variables for the RX and TX buffer sizes. During initialization, I'd like the see the users to be able to choose how big they need their buffers to be. That would solve this and any number of other situations. But then again, I don't have my hands in the firmware.

Let me know what you all decide to do with all of this (serial overflow issue as well as the others).

1 Like

I too would like a way to support a custom buffer for any interface that currently houses its own buffer - just to be prepared :wink:
And maybe also an event driven aproach too, where you could actually define a threshold for the buffer above which an interrupt would be generated or callback function gets called to pull the data from the buffer before it overflows.
With such an even you could even get away with a short(er) buffer.

Having said this, if the serialEvent() routines actually were events and not mere “extensions” of loop() these would be a perfect place to transfer the internal buffer data into a parsing queue.

How about this as a proposal @Dave & @mdma? Worth a GitHub “issue”?

@jmccorm, you’re right - a custom firmware with a reduced data output would be handy. Looking through the Adafruit support forums reveals that they aren’t warm to the idea, even though GlobalTop modules can be flashed in the field.

However, some control is possible through command sent to the GPS module at runtime, at least in theory. Thus any unwanted sentences could be turned off and desired sentences emitted at a lower rate. This won’t make the UART buffers any larger but it could reduce the amount of stuff flowing into them and allow more breathing room.

Command packet 314 referenced in the Adafruit manual for the part allows control over which NMEA sentences are emitted and how frequently.

And this code provides some examples of how to use it.

I say “in theory” above as I’ve not tried these yet myself, though they are on my list of things to explore.

Hope this helps,
Mike

It helps, but we're already there.

The initialization code in the library already uses the code ($PMTK_SET_NMEA_OUTPUT_RMCGGA) to enable only the $GPRMC and $GPGGA sentences. That translates into "$PMTK314,0,1,0,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0*28".

Because that command string is being sent too soon (see post #28 above), the command appears to be ignored. The GPS module appears to operate using the default setting, which only has the $GPRMC sentence enabled. Unfortunately, just a single $GPRMC line by itself and nothing else still ends up being too big for the buffer from time to time. :frowning:

1 Like

Ah, my apologies - I’d missed that. I’ve been following with interest as figuring out how often / when to pay attention to the GPS in order to obtain a fix is of interest to me as well.

1 Like

Just reading this thread for the first time. I had similar problems described above and made a few adjustments in the loop to compensate. And I also changed to the tinygps library as it looked a lot cleaner.

The problem I observed was the serial buffer would fill and then not accept any more characters until it was emptied. Depending on timing it would contain only part of a sentence, or an incomplete sentence. These would be rejected of course and by chance I would also begin reading a new sentence as the buffer was being emptied and continue to read as the sentence completed. If I output what I was reading to the console character by character it would often look like garbage.

To make a long story short what I did to make the loop reliable was to throw away all the old data in the buffer at the start of every cycle, then continue waiting and reading to synchronize on the start of a new sentence. This could take up to a second or so. Then keep reading and process the data normally. No more errors ever after adopting this process.

void loop()
{
    bool isValidGPS = false, validStart = false;
    unsigned long d = 0, x = 0;
    int sentences = 0;
    
    int avail = Serial1.available();
    if (avail >= 63) {
        for (int x = 0; x < avail; x++) {
            Serial1.read();
        }
    }
    if (Serial1.available() > 0 && Serial1.peek() != '$') {
        while (Serial1.peek() != '$' && Serial1.peek() != -1)
            Serial1.read(); // empty the buffer up to a new $ or end of buffer
    }
    
    for (unsigned long start = millis(); millis() - start < 1000 || (validStart && sentences <= 5);)
    {
        while (Serial1.available())
        {
            char c = Serial1.read();
            if (!validStart && c == '$') {
                validStart = true; // mark the beginning of the sentence
                d = millis() - start;
            }
            
            // incrementally handle and parse gps data. returns true after a valid sentence received
            if (validStart) {
                if (gps.encode(c)) {
                    isValidGPS = true;
                    validStart = false;
                    sentences++;
                }
            }
        }
        x = millis() - start;
    }

    if (isValidGPS){
        float lat, lon;
        unsigned long age;
    
        gps.f_get_position(&lat, &lon, &age);
        
        sprintf(szInfo, "%.4f,%.4f alt: %.1f course: %.1f sats: %d", 
            (lat == TinyGPS::GPS_INVALID_F_ANGLE ? 0.0 : lat), (lon == TinyGPS::GPS_INVALID_F_ANGLE ? 0.0 : lon), 
            gps.f_altitude(), gps.f_course(), gps.satellites());
    }
    else{
        // Not a vlid GPS location, jsut pass 0.0,0.0
        // This is not correct because 0.0,0.0 is a valid GPS location, we have to pass a invalid GPS location
        // and check it at the client side
        sprintf(szInfo, "0.0,0.0");
    }
    
    Particle.publish("gpsloc", szInfo);
    
    unsigned long chars;
    unsigned short good_sentences, failed_cs;
    
    gps.stats(&chars, &good_sentences, &failed_cs);
    
    sprintf(szInfo, "D: %ld, C: %ld, S: %d, B: %d, X: %ld", d, chars, good_sentences, failed_cs, x);
    delay(20);
    Particle.publish("stats", szInfo);
    
    Serial.write(String::format("\r\nend cserr:%u dur:%ul\r\n", failed_cs, x));
    
    delay(sleep);
}
1 Like

Hi dklinkman- your post has been the most helpful on this thread. The problem isn’t specific to the Asset Tracker, it’s probably true for most serial GPS interfaces on the Electron. The Electron requires the inner loop. Here’s what I ended up with after using your code as inspiration:

void loop() {
    if (Serial1.available() > 0 && Serial1.peek() != '$') {
        while (Serial1.peek() != '$' && Serial1.peek() != -1)
            Serial1.read(); // empty the buffer up to a new $ or end of buffer
    } else if (Serial1.available() > 0 && Serial1.peek() == '$') {
        while (Serial1.peek() != '\r' && Serial1.peek() != '\r') {
            char c = Serial1.read();
            if (gps.encode(c)) {
                onValidGpsSentence();
            }
        }
    }   
}

The problem isn't specific to the Asset Tracker, it's probably true for
most serial GPS interfaces on the Electron. The Electron requires the
inner loop.

Seems like a good way to entrap a valid sentence.

I don't know the language, but offhand, either this line is extraordinarily clever, or it isn't doing what you intended:

while (Serial1.peek() != '\r' && Serial1.peek() != '\r') {

Ugh. I'll have to pick this up later. I'm being commanded to go to the grocery store. :weary:

either this line is extraordinarily clever, or it isn't doing what you intended:

Occam's Razor says the second explanation is most valid.

I intended to check for \r or \n. Oops.

1 Like