Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jwfmcclain opened this issue Jan 29, 2019 · 7 comments
Closed

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

jwfmcclain opened this issue Jan 29, 2019 · 7 comments

Comments

@jwfmcclain
Copy link

jwfmcclain commented Jan 29, 2019

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
Copy link
Contributor

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
Copy link
Author

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 dhalbert reopened this Jan 29, 2019
@dhalbert dhalbert changed the title Missing call to ord in send_command()? Document that commands must be bytes; check on uses of ord() Jan 29, 2019
@dhalbert
Copy link
Contributor

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
Copy link
Author

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
Copy link

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
Copy link
Author

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

@deshipu
Copy link

deshipu commented May 9, 2019

Fixed by #21

@deshipu deshipu closed this as completed May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants