meshtastic / firmware

Meshtastic device firmware
https://meshtastic.org
GNU General Public License v3.0
3.52k stars 869 forks source link

Clarification of GPS hasLock() function #842

Closed a-f-G-U-C closed 3 years ago

a-f-G-U-C commented 3 years ago

This is not the easiest issue to describe, so please bear with me. Device: TBEAM T22_V1.1 20191212 but the issue is not specific to this device. Version: multiple, including latest master.

It all started from the observation that my t-beams on-screen reporting of GPS lock was, in some cases, inconsistent with the indication from the GPS's own LED. A quick search led to function hasLock() in GPS.h, which is referenced at several points in the code, as the authoritative indicator of the current GPS lock status.

So far, this seems to follow the Principle of Least Astonishment: gps->hasLock() should indeed mean that the GPS has a lock (right now). However, the actual implementation appears to follow an unnecessarily complex and somewhat loose chain of assumptions, which results in hasLock() no longer reflecting the GPS lock status accurately, causing the discrepancy above.

It starts here: https://github.com/meshtastic/Meshtastic-device/blob/5fe3ec09de674e241ff7e4a2180225ef99af0fbc/src/gps/GPS.h#L60-L61 ... and is gets progressively worse, but that's another story for a separate issue. :)

For the scope of this issue, I am only seeking clarification whether hasLock() is indeed intended to serve as a best-effort indicator of the GPS's current lock status - which unfortunately is not the case currently.

Thank you.

geeksville commented 3 years ago

I think my original intent was that hasLock() mean't "has had a lock so far this boot (i.e. the location is at least kinda valid)". But yeah kinda loosy goosie ;-)

a-f-G-U-C commented 3 years ago

Thanks, thought that'd probably be the case :) In this case, it makes sense to keep hasValidLocation on the "sticky lock" behavior that you're describing above (and myself in issue #843), and make hasLock() a strictly real-time indicator (per PR #851 etc). Both are useful in their own way.

a-f-G-U-C commented 3 years ago

Closing with a final comment. Post PRs #851, #861, the meaning of hasLock() and hasValidLocation is as follows: