[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [gpsd-dev] [Patch Submission] $GPVTG w/magnetic course

From: teyrana
Subject: Re: [gpsd-dev] [Patch Submission] $GPVTG w/magnetic course
Date: Wed, 13 Jun 2018 10:52:58 -0400

1. Promoting mode to 2D without explicit flag

This worries me:

+  if (session->newdata.mode < MODE_2D) {
+      session->newdata.mode = MODE_2D;
+      mask |= MODE_SET;
+  } 
I see nothing about GPVTG ( that tells us
the GPVTG tells us the GPS fix mode is at least 2D.  Got a citation?
Maybe make it a comment in the code?

I see what you're saying, it's certainly not *explicitly* set in the NMEA message. However, this chunk is behavior consistent with your codebase,
at the following locations in 'driver_nmea0183.c':

(for processing RMC, GLL, and GGA, respectively.)

Granted, each of those has more context, and more documentation - but each sets the newdata.mode based on implicit conclusions.

Most interestingly,  GGA, *WHICH HAS A MODE FIELD* ignores that field, and instead sets gpsd-internal mode based on what altitude fixes the message reports.

And I'll just finish up with a direct quote from stable repo, describing why we set the mode the way we do: 

 * This is a bit dodgy.  Technically we shouldn't set the mode
 * bit until we see GSA.  But it may be later in the cycle,
 * some devices like the FV-18 don't send it by default, and
 * elsewhere in the code we want to be able to test for the
 * presence of a valid fix with mode > MODE_NO_FIX.


I'm not sue this is the right place for :

+  // request to report an output message
+  mask |= CLEAR_IS;
+  mask |= REPORT_IS;

 Okay, I've submitted this patch both with and without these lines.   Just tell me which y'all prefer.  

Running 'scons check' shows why:


Notice "eps" is getting lost.  Most likely because of a premature CLEAR_IS
so the entire data for the cycle could ne get accumulated.

Getting CLEAR_IS and REPORT_IS right is hard (maybe impossible).  If
possible, you only want them on the last message of a cycle.

The observed sequence for our GPS is: 
2018-05-17 10:01:37: $GPGGA,140137.00,4221.8535787,N,07101.8901519,W,2,07,0.8,12.204,M,-33.056,M,6.0,0138*47
2018-05-17 10:01:37: $GPVTG,55.57,T,70.20,M,0.12,N,0.23,K,D*23
2018-05-17 10:01:37: $GPZDA,140137.00,17,05,2018,00,00*6E
2018-05-17 10:01:37: $GPHDT,185.38,T*02

So, if this is an important point, let's get into it: 
And are those sequences stable? (1) Across reboots? (2) Across a manufacturer's product line? (3) Across different manufacturers? 

If we're really going to make a big discussion of this, depending on an implementation quirk to determine timing, seems fragile, at best.

And while we're at it-- do any of the test/daemon/*log files have timing info?  If we're getting into this discussion, let's base it off some hard data.

Spend some time looking at 'scons check' to ensure your patch has only
positive effects.

By nature of the patch, it's going to change the output of those .chk files.  But not in a simple way.   In a non-zero sum, subjective, complicated, ugly trade-off sort of way.  For example, here are three options:

1. From the savannah / mainline branch ('tail -3 test/daemon/bn-9015.log.chk'):


2. With CLEAR_IS & REPORT_IS the output becomes:

3. With just REPORT_IS, the output becomes:

I don't see any easy, perfect solution here: You either clobber the 'eps' field, or output it twice.

Note: The "eps" field is not even touched by this patch  -- if you would like gpsd to only output 'eps' once for each update, then someone needs to implement a different type of "clear" flag.  
And then implement that. And test it.  And asking for that change to this patch is feature creep, to put it mildly.

3. Patch, v6

reply via email to

[Prev in Thread] Current Thread [Next in Thread]