adafruit/Adafruit_GPS

Malformed NMEA sentence causes dataloss

orrmany opened this issue · 13 comments

orrmany commented

I am developing a fitness-datalogger using nrF52840 Feather and UltimateGPS Featherwing. The GPS module and/or the serial erratically sends malformed NMEA sentence, which causes data loss.

An example of such a sentence:
$GPRMC,181536.000,A,5936.79K,D*3A

  • Arduino board: INSERT ARDUINO BOARD NAME/TYPE HERE
    nrf52840 Express Feather

  • Arduino IDE version (found in Arduino -> About Arduino menu): INSERT ARDUINO
    VERSION HERE
    n/a

  • List the steps to reproduce the problem below (if possible attach a sketch or
    copy the sketch code in too): LIST REPRO STEPS BELOW

Try to parse "$GPRMC,181536.000,A,5936.79K,D*3A" with the library.

This is a malformed NMEA sentence (field count is invalid), and as such it should have been discarded entirely. However, the parse() method partially parses it, causing (the earlier established) GPS date info to became invalid (setting the GPS date to the bogus 2000-00-00 date).

This requirement is true for any decent GPS parser, IMHO: the effect of the unseccesful parsing attempt of any invalid NMEA sentence shall leave all stateful data members of the GPS instance unchanged
#bug

sellensr commented

The parser performs a rudimentary check for valid checksum sentence source and sentence name, then proceeds to parse and assign values as it moves through the sentences, simply assuming the data is valid. You are correct it will fail if the GPS sends bogus data.

The current library is close to the memory limits for use in an UNO, so any further checks should probably be made with those limits in mind.

drak7 commented

It's malformed but has a correct checksum, that's really strange. Any idea why the module is doing that?

orrmany commented

I do not know why the module produce such sentences, but the incorrect parsing causes a lot of headaches. I produce GPX file from various fitness sensors and from the GPS data. The library parse() method indicates a successful fix after such a sentence, hence my code makes a new element with the now invalid date for up to 3 seconds (after that incident only GGA sentences were produced, i.e., no valid date for 3 seconds.
All the GPX validating parsers barf on this file then claiming invalid date-stamp (quiet rightfully). :(
The most bothersome is that there is no indication of error.

I still think that checking for sentence correctness is a necessary requirement for any decent parser.
If the UNO has memory limits, then maybe some compile-guard around the correctness checking code and a BIG RED warnings in the documentation for UNO users about the lack of checking could be the solution.

  • 👍1
svdrummer commented
orrmany commented

@svdrummer, I also wrote software for living, protocol testers, in fact. As such, I am quite curious, so pls. tell me how to validate an incoming character string for correctness _without _ parsing it???

Parsing a character string for pre-validation purposes prior to sending it to parse it second time for retrieving parsed content -- that does not sound as a good and efficient programming practice for me....

drak7 commented

We could at least count the fields and make sure the sentence has the proper amount, that shouldn't add too much overhead.

  • 👍1
orrmany commented

I also receive sentences like this:
$GPRMC,181532.000,A,5936.7948,N,01741.89$GPGGA,181533.000,5936.7948,N,01741.8986,E,2,09,0.85,5.5,M,23.8,M,0000,0000*68
That is, where GPRMC and GPGGA partially overlaps. Such cases always contain an incomplete first sentence then a "$" sign in the middle of the sentence delineating the start of the second sentence. Often times there are even two "*" characters, but one, or both digits of the checksum of the first sentence is overwritten by the opening "$" character of the second sentence....
This one also fails to parse, albeit the second sentence is usually full and correct

orrmany commented

Of course I myself will look into a bugfix, or a more robust parser code eventually, but currently I am overwhelmed with my daytime job :/

orrmany commented

I hope employing a proper transaction semantic (i.e., parse data into temporary variables and only committing to the members of the GPS class instance when the parse is complete and scucessful) would help w/o significant memory increase. I have an UNO clone so I will try to validate on an Uno when I will have a fix.
I reported this bug in the hope that the community would provide a fix sooner than I can, as I am busy like hell with my daytime job now.

drak7 commented

What speed are you using for the GPS serial and what is your GPS update rate? The overlapping sentences can be caused by a mismatch with those.

sellensr commented

parse() calls check() before proceeding, fails if check not passed.

check() does only rudimentary name, source, checksum right now.

An option for check could be parsing in a dummy instance, then testing (some of) the results of the parsing for validity.

orrmany commented

9600, 1HZ, EGNOS enabled

What speed are you using for the GPS serial and what is your GPS update rate? The overlapping sentences can be caused by a mismatch with those.

orrmany commented

GPS.begin(9600);
GPS.sendCommand(PMTK_SET_NMEA_UPDATE_1HZ); // 1 Hz update rate
GPS.sendCommand(PMTK_API_SET_FIX_CTL_1HZ); //1 Hz sampling
GPS.sendCommand(PMTK_ENABLE_SBAS); ///< Enable search for SBAS satellite (only works with 1Hz
///< output rate)
GPS.sendCommand(PMTK_ENABLE_WAAS); ///< Use WAAS for DGPS correction data