Skip to content

Fail bad sentences that pass the CRC check #57

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
Mar 8, 2021
Merged

Fail bad sentences that pass the CRC check #57

merged 18 commits into from
Mar 8, 2021

Conversation

lesamouraipourpre
Copy link
Contributor

As the CRC is only 8 bit and bad sentences will get past it, this PR attempts to catch bad sentences by comparing them to an expected parameter type list after the CRC check. Fixes #55

The _update() method does the following:

  1. When available read a complete sentence from UART/I2C. Return False if not available. (No change)
  2. Check the CRC. Return False if it fails. (No change)
  3. If the sentence is NOT from a GNSS talker, assume it is valid and return True.
  4. Call a _parse_XXX method for the sentence type, eg. _parse_rmc() for RMC.
  5. If the data length is not an expected size, return False.
  6. Within these methods, call _parse_data() with an appropriate sentence type and the data.
  7. Match the data against the expected type and convert it. Return False if it fails.
  8. Use the parsed data to interpret the message and update instance variables.
  9. Return True

The time/data parsing code has also been refactored into a method so that it is not repeated through the code.

Hardware tested with:
Pyportal Pynt to Adafruit Mini GPS over I2C
I've had it connected for over 18 hours with no errors raised.

This is Draft as it would be appreciated if others can test on different hardware.

@lesamouraipourpre
Enable the GSV parsing with _parse_gpgsv
15015b5
@lesamouraipourpre
Spelling.
002f850
@lesamouraipourpre
Enable the GSA parsing with _parse_gpgsa
c4c999a
@lesamouraipourpre
Merge branch 'parse-gsa-and-gsv' into bad-sentences
e5dcc01
@lesamouraipourpre
Rename the _parse_[sentence] methods so that they don't have gp as pa…
6dcfca0 
…rt of their name.

eg. _parse_gpgll -> _parse_gll
@lesamouraipourpre
Refactor the sentence parsing (Fixes #55)
7a59234 
Refactor the date and time parsing into a method
Add an instance variable self.mode_indicator
Add an instance variable self.magnetic_variation

Sentence Parsing:
1) Read the sentence from I2C/UART (No change)
2) Validate the CRC, else fail (No change)
3) Call _parse_[SENTENCE_TYPE](), if the sentence was from a GNSS, else return True
4) The data length is compared against it's expected length, else fail
5) These then call _parse_data(PSEUDO_SENTENCE_TYPE,DATA)
6) The data is converted into the expected parameter types, else fail

PSEUDO_SENTENCE_TYPE is usually SENTENCE_TYPE, but can be modified for
a SENTENCE_TYPE with variable data length.

The update() method returns True if:
  The sentence passes CRC AND
    Not from A GNSS (No parsing happens) OR
    From a GNSS, with a handled sentence type which passes parsing OR
    From a GNSS, with an unhandled sentence type (No parsing happens)
Else False
@lesamouraipourpre
Use longitude for longitude, not latitude. D'Oh!
632dc4a
@lesamouraipourpre
Correct some of the RMC parameter types.
91f5846
@lesamouraipourpre
Pass the debug value into _parse_data
2cc4473
@lesamouraipourpre
Typo. Thank you, PyLint
2f79c44
@lesamouraipourpre
Missing return statement. Thank you, PyLint.
92efe69
@lesamouraipourpre
Black/PyLint pass
037be6e
@lesamouraipourpre
Modify the time_source example so that it doesn't trigger a PyLint du…
23b2df8 
…plicate warning.
@lesamouraipourpre
Add PyLint disable=duplicate-code to time_source.
07d4838
@lesamouraipourpre
Add PyLint disable=duplicate-code to all examples.
ad6930a
@tannewt
Copy link
Member

tannewt commented Feb 25, 2021

Here is how I think we'll want to fix the duplicate-code check: adafruit/cookiecutter-adafruit-circuitpython#111

tannewt
tannewt requested changes Feb 25, 2021
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these further improvements. Please check the RAM impact too.

@lesamouraipourpre lesamouraipourpre marked this pull request as ready for review March 8, 2021 19:06
tannewt
tannewt approved these changes Mar 8, 2021
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you!

@tannewt tannewt merged commit fd9d92a into adafruit:master Mar 8, 2021
@lesamouraipourpre lesamouraipourpre deleted the bad-sentences branch March 9, 2021 17:01
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Mar 10, 2021
@adafruit-adabot
Automated update by Adabot (adafruit/adabot@6f44843)
d4ff12f 
Updating https://github.com/adafruit/Adafruit_CircuitPython_Fingerprint to 2.2.0 from 2.1.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_Fingerprint#28 from admiralmaggie/master

Updating https://github.com/adafruit/Adafruit_CircuitPython_GPS to 3.8.0 from 3.7.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_GPS#57 from lesamouraipourpre/bad-sentences

Updating https://github.com/adafruit/Adafruit_CircuitPython_HT16K33 to 4.1.5 from 4.1.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_HT16K33#86 from SAK917/master

Updating https://github.com/adafruit/Adafruit_CircuitPython_IS31FL3731 to 3.0.1 from 3.0.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_IS31FL3731#39 from dglaude/patch-1

Updating https://github.com/adafruit/Adafruit_CircuitPython_MONSTERM4SK to 0.1.2 from 0.1.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_MONSTERM4SK#6 from FoamyGuy/pylintrc_update
  > Removed pylint process from github workflow

Updating https://github.com/adafruit/Adafruit_CircuitPython_AirLift to 1.0.1 from 1.0.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_AirLift#2 from FoamyGuy/pylintrc
  > Set correct black and reuse versions
  > Removed pylint process from github workflow
  > Hardcoded black version

Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Text to 2.15.4 from 2.15.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Text#132 from jposada202020/tab_replacement
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Text#130 from kmatch98/master
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Text#126 from FoamyGuy/label_base_class

Updating https://github.com/adafruit/Adafruit_CircuitPython_PIOASM to 0.2.2 from 0.2.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_PIOASM#9 from tannewt/audiobusio_examples
  > Merge pull request adafruit/Adafruit_CircuitPython_PIOASM#13 from FoamyGuy/pylintrc

Updating https://github.com/adafruit/Adafruit_CircuitPython_Pixel_Framebuf to 1.1.2 from 1.1.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_Pixel_Framebuf#4 from FoamyGuy/main
  > Hardcoded black version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tannewt tannewt tannewt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The CRC check doesn't catch all bad messages
2 participants
@lesamouraipourpre @tannewt