adafruit / Adafruit_CircuitPython_GPS

GPS parsing module for CircuitPython. Meant to parse NMEA data from serial GPS modules.
MIT License
75 stars 58 forks source link

Document that commands must be bytes; check on uses of ord() #15

Closed jwfmcclain closed 5 years ago

jwfmcclain commented 5 years ago

Looks like the GPS.send_command() is missing a call to ord() in the checksum calculation loop. This means if you pass in a sting for command you get an exception (sorry don’t have it handy). Worked around by converting the arg to bytes before passing in, but the example code on learn.adafruit.com passes the commands in as a string.

Ran into this running a 3.x version on an M4 feather.

dhalbert commented 5 years ago

The strings sent via GPS.send_command() should all be bytes. Apologies -- I thought we had fixed all the examples and sample code but I found one example that was still using strings. busio.UART now deals strictly with bytes and bytearray, and is not meant to be used with strings.

jwfmcclain commented 5 years ago

Thanks, is the requirement that command has to be bytes (or similar) worth adding to the doc string?

I suppose you need the ord() call on line 155 (when reading responses) for backward compatibility with older implementations of UART?

dhalbert commented 5 years ago

Fixing the doc string is a good idea, and we'll check on the ord(). Thanks for bringing this up. I'm reopening so we can keep track of this.

jwfmcclain commented 5 years ago

FWIW, this morning after a soft reboot of the M4, I got this when calling update():

Traceback (most recent call last):
  File "code.py", line 152, in <module>
  File "adafruit_gps.py", line 102, in update
  File "adafruit_gps.py", line 148, in _parse_sentence
UnicodeError: 

No idea if this was the very first call to update() after the soft reset or not, but I am wondering if the UART line got some noise as a result of the reset and that caused some of the bytes to fall outside of the ASCII range. If so it might be worthwhile in _parse_sentence() to check the checksum before converting the sentence to a string.

jerryneedell commented 5 years ago

I see this frequently on start up -- I always assumed it was just starting in the middle of receiving a packet from the GPS and the decoding failed. I just restart... probably should handle it better.

jwfmcclain commented 5 years ago

Of course it might easier and more robust just to catch the UnicodeError and drop the sentence.

deshipu commented 5 years ago

Fixed by #21