Parsing bug introduced in 1.5.0?

[ladyada]

@drak7 please check out this report of a parsing error introduced in 1.5.0 (not seen in 1.4.0) https://forums.adafruit.com/viewtopic.php?f=48&t=165596&p=814432#p814432

[drak7]

Will do!

[sellensr]

GPS.parse("$GPGGA,113600.000,4151.1969,N,00213.9092,E,1,05,1.46,521.1,M,51.1,M,,*69");

// GPS.parse("$GPRMC,113600.000,A,4151.1969,N,00213.9092,E,0.60,245.89,300520,,,A*6F"); Serial.print("Location: "); Serial.print(GPS.latitude, 4); Serial.print(GPS.lat); Serial.print(", "); Serial.print(GPS.longitude, 4); Serial.println(GPS.lon);

yields

Location: 4151.1969N, 213.9092E

So I don't think it's a parsing problem. Same result with the RMC sentence parsed. I wonder if the extra character in the latitude was present in the string sent for parsing.

[drak7]

If I parse the sentences on their own it's fine, but if I parse one after the other I get the same error. Still digging into it.

GPS.parse("$PGTOP,11,2*6E");
GPS.parse("$GPGGA,174140.000,4139.9716,N,00116.3685,E,1,08,1.24,516.2,M,51.5,M,,*6D");
GPS.parse("$GPRMC,174140.000,A,4139.9716,N,00116.3685,E,0.16,240.69,280520,,,A*62");

16:00:02.646 -> Time: 17:41:40.10848
16:00:02.646 -> Date: 28/5/2020
16:00:02.646 -> Fix: 1 quality: 1
16:00:02.646 -> Location: 41396.9726N, 116.3685E
16:00:02.646 -> Speed (knots): 0.16
16:00:02.646 -> Angle: 240.69
16:00:02.646 -> Altitude: 516.20
16:00:02.646 -> Satellites: 8

[sellensr]

      GPS.parse("$PGTOP,11,2*6E");
      GPS.parse("$GPGGA,174140.000,4139.9716,N,00116.3685,E,1,08,1.24,516.2,M,51.5,M,,*6D");
      GPS.parse("$GPRMC,174140.000,A,4139.9716,N,00116.3685,E,0.16,240.69,280520,,,A*62");
      Serial.print("Location: ");
      Serial.print(GPS.latitude, 4); Serial.print(GPS.lat);
      Serial.print(", ");
      Serial.print(GPS.longitude, 4); Serial.println(GPS.lon);

works OK when I run it on my ItsyBitsy M0 under IDE 1.8.10

[drak7]

16:18:08.605 -> Location: 41396.9726N, 116.3685E

Same code on a Mega. Maybe a memory issue?

[sellensr]

That's what I'm thinking -- the newer version pushes hard against the limits of the old processors.

[sellensr]

Maybe we need a warning to stay with 1.4 or earlier for 8 bit processors?

[drak7]

Yeah, we might need something like that. I'll keep looking to see if I can find where it's going wrong.

[sellensr]

I think it's just the stack colliding with the heap. I played a little with different configurations when I was building it and got similar weirdness on an UNO clone. We could spend a lifetime trying to shrink off those last few bytes and never have the safety we would want.

[ladyada]

what was added that makes it no longer work on the UNO?

[ladyada]

did all the sprintf()s get added in 1.5.0?

[ladyada]

@gearmesh don't take over issues

[sellensr]

The sprintf()s are new, however they are (all, I hope) confined inside conditional compiles for the NMEA extensions that don't get compiled for the UNO and other small processors. I did test for basic functionality with an UNO clone, but you run out of memory pretty quickly as you add sketch variables.

[drak7]

I'm still not sure it's running out of memory, if so it should have problems with everything, not just the latitude. I'm running it on an Uno and I have 603 bytes of RAM available. That ought to be enough. While debugging and putting Serial.println in various places it suddenly works. Any idea why that would have any effect?

[ladyada]

@drak7 there just isnt much here that uses RAM if printf()'s are not in the object file - it could be some memory being overwritten. this library worked really reliably till 1.5.0 - i think we may need to bisect to find where this was introduced?

[drak7]

Yep, I'm looking through it.

[drak7]

Introduced in 3876f29

[drak7]

NMEA_parse.cpp line 685:

char degreebuff[10];
char *e = strchr(p, '.');
if (e == NULL || e - p > 6)
  return false;                // no decimal point in range
strncpy(degreebuff, p, e - p); // get DDDMM
long dddmm = atol(degreebuff);

degreebuff is not initialized. On the first time through the contents happen to be 0 so all is well. Next time it gets allocated there's some junk there already, so when strncpy finishes there may or may not (likely not) be a 0 terminating the string. atol then gets a crazy result.

I'll submit a fix and do a lot of testing, it just needs to be initialized to 0:

char degreebuff[10] = {0};

[sellensr]

Sorry about that! I guess I assumed strncpy() was good for a terminating 0 like strcpy(). It would be good to search for the same error elsewhere. Might have been late at night...

looks like same issue at line 765

Cheers,

Rick

On Jun 4, 2020, at 12:58, Matt Goodrich notifications@github.com wrote:

NMEA_parse.cpp line 685:

char degreebuff[10]; char *e = strchr(p, '.'); if (e == NULL || e - p > 6) return false; // no decimal point in range strncpy(degreebuff, p, e - p); // get DDDMM long dddmm = atol(degreebuff); degreebuff is not initialized. On the first time through the contents happen to be 0 so all is well. Next time it gets allocated there's some junk there already, so when strncpy finishes there may or may not (likely not) be a 0 terminating the string. atol then gets a crazy result.

I'll submit a fix and do a lot of testing, it just needs to be initialized to 0:

char degreebuff[10] = {0}; — You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/adafruit/Adafruit_GPS/issues/120#issuecomment-638981475, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAU44SLZXF2L4XVICIQBCZ3RU7HFDANCNFSM4NR3Y5EA.

[drak7]

No worries, I missed it when I tested it before. I submitted a PR to fix this issue. If you'd like to check the other strncpy instances please do!