fix bug where `has_fix` remained `True` even though the fix was lost

[jkittner]

I am experiencing the following behavior with this sample code:

import time
import serial

import adafruit_gps

uart = serial.Serial('/dev/ttyS0', baudrate=9600, timeout=10)

gps = adafruit_gps.GPS(uart, debug=False)
gps.send_command(b"PMTK314,0,1,0,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0")
gps.send_command(b"PMTK220,1000")

while True:
    gps.update()
    print(f'gps.has_fix={gps.has_fix}')
    print(f'gps.has_3d_fix={gps.has_3d_fix}')
    print(f'gps.fix_quality={gps.fix_quality}')
    print(f'gps.fix_quality_3d={gps.fix_quality_3d}')
    print('=' * 79)
    time.sleep(.1)

1. start the system inside, no fix, red led blinks quicky, everything as expected

   gps.has_fix=False
   gps.has_3d_fix=False
   gps.fix_quality=0
   gps.fix_quality_3d=0

2. bring the GPS-unit outside, as expected it gets a fix, red led blinks slowly, property has_fix is set correctly

   gps.has_fix=True
   gps.has_3d_fix=False
   gps.fix_quality=1
   gps.fix_quality_3d=0

3. bring the GPS unit back inside, the red LED starts to blink quickly, however it still returns has_fix=True

   gps.has_fix=True
   gps.has_3d_fix=False
   gps.fix_quality=1
   gps.fix_quality_3d=0

I had a look at what _read_sentence returns, and it seems like the GPS continues to return some data for quite some time e.g b'$GPGGA,150745.000,1234.1234,N,12345.1234,E,1,06,2.89,33.2,M,47.4,M,,*5A\r\n' . after a while it actually stops, to return proper data, but has_fix remains True even though the red led blinks quickly. The number of satellites goes down to 1 and the RMC sends V for Warning, but has_fix remains true...

b'$GPRMC,154518.000,V,,,,,0.00,0.00,230921,,,N*4A\r\n'
gps.has_fix=True
gps.has_3d_fix=False
gps.fix_quality=6
gps.fix_quality_3d=0

Solution/Fix

The issue is, that the sentence b'$GPRMC,154518.000,V,,,,,0.00,0.00,230921,,,N*4A\r\n' cannot be parsed here and the method returns early and self.fix_quality is not set and the property has_fix remains at the point of the last successfull parse: https://github.com/adafruit/Adafruit_CircuitPython_GPS/blob/36554486700593bada0e9de7abcb4d83116b04e3/adafruit_gps.py#L447-L449 so this way the code below is never reached: https://github.com/adafruit/Adafruit_CircuitPython_GPS/blob/36554486700593bada0e9de7abcb4d83116b04e3/adafruit_gps.py#L456-L460

A different way than the one I chose here would be to add another parsing step to check if at least the A or V parameter is parsable and set has_fix according to this. My though for this solution was, that if there was no parsable data, we also don't have a fix so has_fix must be false.

This is also the case for gga and gsa, however they also send a fix parameter in their data, so we could try to parse them separately to differentiate between a lost fix and some other non parsable data problem. I am not sure if this even makes sense to do...

[jepler]

Thanks! I approved the workflow to run so that we can make sure it passes our automated checks. I don't know enough about GPS to comment on the correctness of this fix.

[jepler]

The failure of the automated checks appears to be unrelated to your change. I'm consulting with a colleague about the problem.

[kattni]

I'm going to merge this because we are merging a fix soon for the failures.

[jkittner]

Thanks for merging!

The CI failure can be fixed with this addition to the .pylintrc or by passing a --py-version=3.5 to the call.

diff --git a/.pylintrc b/.pylintrc
index 845d2b0..fc11b6d 100644
--- a/.pylintrc
+++ b/.pylintrc
@@ -55,7 +55,7 @@ confidence=
 # no Warning level messages displayed, use"--disable=all --enable=classes
 # --disable=W"
 # disable=import-error,print-statement,parameter-unpacking,unpacking-in-except,old-raise-syntax,backtick,long-suffix,old-ne-operator,old-octal-literal,import-star-module-level,raw-checker-failed,bad-inline-option,locally-disabled,locally-enabled,file-ignored,suppressed-message,useless-suppression,deprecated-pragma,apply-builtin,basestring-builtin,buffer-builtin,cmp-builtin,coerce-builtin,execfile-builtin,file-builtin,long-builtin,raw_input-builtin,reduce-builtin,standarderror-builtin,unicode-builtin,xrange-builtin,coerce-method,delslice-method,getslice-method,setslice-method,no-absolute-import,old-division,dict-iter-method,dict-view-method,next-method-called,metaclass-assignment,indexing-exception,raising-string,reload-builtin,oct-method,hex-method,nonzero-method,cmp-method,input-builtin,round-builtin,intern-builtin,unichr-builtin,map-builtin-not-iterating,zip-builtin-not-iterating,range-builtin-not-iterating,filter-builtin-not-iterating,using-cmp-argument,eq-without-hash,div-method,idiv-method,rdiv-method,exception-message-attribute,invalid-str-codec,sys-max-int,bad-python3-import,deprecated-string-function,deprecated-str-translate-call
-disable=print-statement,parameter-unpacking,unpacking-in-except,old-raise-syntax,backtick,long-suffix,old-ne-operator,old-octal-literal,import-star-module-level,raw-checker-failed,bad-inline-option,locally-disabled,locally-enabled,file-ignored,suppressed-message,useless-suppression,deprecated-pragma,apply-builtin,basestring-builtin,buffer-builtin,cmp-builtin,coerce-builtin,execfile-builtin,file-builtin,long-builtin,raw_input-builtin,reduce-builtin,standarderror-builtin,unicode-builtin,xrange-builtin,coerce-method,delslice-method,getslice-method,setslice-method,no-absolute-import,old-division,dict-iter-method,dict-view-method,next-method-called,metaclass-assignment,indexing-exception,raising-string,reload-builtin,oct-method,hex-method,nonzero-method,cmp-method,input-builtin,round-builtin,intern-builtin,unichr-builtin,map-builtin-not-iterating,zip-builtin-not-iterating,range-builtin-not-iterating,filter-builtin-not-iterating,using-cmp-argument,eq-without-hash,div-method,idiv-method,rdiv-method,exception-message-attribute,invalid-str-codec,sys-max-int,bad-python3-import,deprecated-string-function,deprecated-str-translate-call,import-error,bad-continuation,pointless-string-statement
+disable=print-statement,parameter-unpacking,unpacking-in-except,old-raise-syntax,backtick,long-suffix,old-ne-operator,old-octal-literal,import-star-module-level,raw-checker-failed,bad-inline-option,locally-disabled,locally-enabled,file-ignored,suppressed-message,useless-suppression,deprecated-pragma,apply-builtin,basestring-builtin,buffer-builtin,cmp-builtin,coerce-builtin,execfile-builtin,file-builtin,long-builtin,raw_input-builtin,reduce-builtin,standarderror-builtin,unicode-builtin,xrange-builtin,coerce-method,delslice-method,getslice-method,setslice-method,no-absolute-import,old-division,dict-iter-method,dict-view-method,next-method-called,metaclass-assignment,indexing-exception,raising-string,reload-builtin,oct-method,hex-method,nonzero-method,cmp-method,input-builtin,round-builtin,intern-builtin,unichr-builtin,map-builtin-not-iterating,zip-builtin-not-iterating,range-builtin-not-iterating,filter-builtin-not-iterating,using-cmp-argument,eq-without-hash,div-method,idiv-method,rdiv-method,exception-message-attribute,invalid-str-codec,sys-max-int,bad-python3-import,deprecated-string-function,deprecated-str-translate-call,import-error,bad-continuation,pointless-string-statement,consider-using-f-string

 # Enable the message, report, category or checker with the given id(s). You can
 # either give multiple identifier separated by comma (,) or put this option

I was wondering why the second pylint pre-commit hook uses language system which is kidna pre-commit's escape hatch? This is not very portable...

Or have you considered making this >= 3.6 only? and maybe use pyupgrade to reformat e.g. the .format calls to f-strings? I'd be happy to work on this.

[jepler]

f-strings are not supported in all circuitpython boards, so it's not appropriate to start using them in libraries, unlesss those libraries are known to only work with the higher capacity boards in the first place.