Parsing bug introduced in 1.5.0?
ladyada opened this issue · 20 comments
@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
Will do!
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.
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
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
16:18:08.605 -> Location: 41396.9726N, 116.3685E
Same code on a Mega. Maybe a memory issue?
That's what I'm thinking -- the newer version pushes hard against the limits of the old processors.
Maybe we need a warning to stay with 1.4 or earlier for 8 bit processors?
Yeah, we might need something like that.
I'll keep looking to see if I can find where it's going wrong.
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.
what was added that makes it no longer work on the UNO?
did all the sprintf()s get added in 1.5.0?
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.
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?
@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?
Yep, I'm looking through it.
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};
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!