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

[Gary E. Miller]

Yo teyrana!

On Wed, 13 Jun 2018 10:52:58 -0400
teyrana <address@hidden> wrote:

> > I see nothing about GPVTG (http://aprs.gids.nl/nmea/#vtg) that tells
> > us the GPVTG tells us the GPS fix mode is at least 2D.

> I see what you're saying, it's certainly not *explicitly* set in the
> NMEA message.

Then how about I give you a citation back:

u-blox8-M8_ReceiverDescrProtSpec_\(UBX-13003221\).pdf 

Sections: "7.5 Freezing the Course Over Ground", and
"30.1.3 Protocol Configuration".

Under 30.1.3, look at table "NMEA filtering flags" for the flag 
"Track filtering":

"Enable to permit course over ground (COG) to be reported even when it would
otherwise be frozen".

So, at least some u-blox 8 can be configured to report GPVTG even
when the last known good dat is stale.

BUT, the bigger problem, is your patch takes some of the test cased that
were reporting valid 3D fixes, and now they report 2D fixes.  That is
not good.  We can quible over whether 2D is a valid minimum, but
downgrading 3D to 2D is a show stopper.

> However, this chunk is behavior consistent with your
> codebase, at the following locations in 'driver_nmea0183.c':
> 
> https://git.savannah.gnu.org/cgit/gpsd.git/tree/driver_nmea0183.c#n210
> https://git.savannah.gnu.org/cgit/gpsd.git/tree/driver_nmea0183.c#n311
> https://git.savannah.gnu.org/cgit/gpsd.git/tree/driver_nmea0183.c#n398
> 
> (for processing RMC, GLL, and GGA, respectively.)

I very briefly checked those against the u-blox doc.  Yes, those could
be better, but on the surface those look consistent at least with older
u-blox doc.  New patches need to be consistent with the latest doc.

The issue that has been coming up recently is the new GPS with built in
IMUs and DR.  It will take a while before the old code accounts for these
new developments, but that is no excuse for new code to repeat the
ommisions of the past.

I'd rather only have one can of worms open at one time.  If you want we
can go back and fix those other sentences to conform to the newer
NMEA standards and u-blox doc after your patch is live.

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

Yeah ugly compromises everywhere.  Does not mean we need to repeat old
mistakes.

> 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.

So?  And do you really want to go off your track now, and fix more
stuff?  One bug at a time.

> 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.
> >  */  

Yes, but as 'scons check' shows, at least for real data, they manage
to finesse things and get good results.  Your code, by downgrading 3D to
2D, and losing 'eps', fails that test.

> 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

FYI: u-blox deprecates GPGGA.

> 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?

Dunno, but gpsd has a ton of test cases.  Your answers are in there.
That is the data used to judge all patches

All you need to do is make a small change, run 'scons check', and see
what you broke.  Your patch 1 broke a lot.

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

Yup!  Which is why it is very important to get it just right.

> And while we're at it-- do any of the test/daemon/*log files have
> timing info?

Sure, they have whatever time stamps the GPS put in their output.
Not as good aas you might want, but good enough to show the need
for a bit more in the patch.

gpsd does have some code, used by some modes of gpsprof, to do finer
grain timing.  Might be something useful in there, if you want to
work for it.

> If we're getting into this discussion, let's base it
> off some hard data.

And it was that hard data, run over your patch #1, that showed me you
are close, but not quite there.

> > 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.

Yup!  But, with a bit of effort, a lot of interesting things fall
in your lap.  Like your patch downgrading 3D fixes to 2D, and losing
'eps'.  Those are pretty important to some people.  I could kept
looking at the 25 failed tests, but that was enough for me to toss it
back to you.

> 1. From the savannah / mainline branch ('tail -3
> test/daemon/bn-9015.log.chk'):
> 
> $GPRMC,171007.972,A,5200.8514,N,00421.7851,E,000.0,000.0,120610,,,A*65
> >
> > {"class":"TPV","mode":3,"time":"2010-06-12T17:10:07.972Z","ept":0.005,"lat":52.014190000,"lon":4.363085000,"alt":7.700,"epx":8.483,"epy":10.737,"epv":34.500,"track":0.0000,"speed":0.000,"climb":0.100,"eps":21.47,"epc":69.00}
> > $GPVTG,000.0,T,,M,000.0,N,000.0,K,A*0D  

Useless to me.  You need to compare the old and new results to see if
it got better, or worse.  Better is good, worse is not.

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

I think that is a false choice.  Easy choices, but not the only choices.

IFF that was the ONLY choice, I'd go with output twice.  You'll see
that choice often made in gpsd, and it messes up people all the time.

> Note: The "eps" field is not even touched by this patch

And yet, your patch clearly broke it, and more.  Not good.  The
devil is in the details, and the detail here is which mask bits to set
when.

gpsd is a very finely balanced set of compromises.  Maybe even a house
of cards.  But at least it is a stable house of cards, don't degrade it.

> -- if you
> would like gpsd to only output 'eps' once for each update, then
> someone needs to implement a different type of "clear" flag.

I'm gonna take a WAG and say that CLEAR_IS, REPORT_IS and MODE_SET
belong in the GPVTG handler only under very specific circumstances.

And, now you begin to see why NMEA is a PITA, and we recommend the
native GPS output if possible.

> And then implement that. And test it.  And asking for that change to
> this patch is feature creep, to put it mildly.

Feature creep is better than obvious regressions, but I suspect there
are better solutions.

> 3. Patch, v6
> https://pastebin.com/uGG9jH30

I'm pretty sure GPVTG is rarely, if ever, the last message in a cycle.
So REPORT_IS not good here.  In your case you have ZDA after VTG, so
REPORT_IS is best not in VTG.

I applied your patch, locally, minuse the REPORT_IS, CLEAR_IS and
MODE_SET.

If did change the regressions a bit, but mostly in a good way.  4 tests
got better, one got worse.  Very close to acceptable.

The worse:

test/daemon/navika-100-fix.log

-{"class":"TPV","mode":3,"time":"2016-04-21T22:32:51.000Z","ept":0.005,"lat":37.965442167,"lon":-122.532423833,"alt":9.200,"epv":60.260,"track":338.5100,"speed":0.035,"climb":0.000}
+{"class":"TPV","mode":3,"time":"2016-04-21T22:32:51.000Z","ept":0.005,"lat":37.965442167,"lon":-122.532423833,"alt":9.200,"epv":60.260,"track":338.5100,"magtrack":0.0000,"speed":0.035,"climb":0.000}

Clearly magtrack=0 is wrong.  But that is what the GPVTG said:

$GPVTG,338.51,T,0.00,M,0.068,N,0.126,K,A*24

I'm almost inclined to let that one go, and push that modified
patch, but I'd like one whack at finding a fix.  Maybe special case
magtrack=0.0 if it is wildly inconsistent with track?  If so, what would
'wildly inconsistent' mean?

Given that gpsd is used in LEO, we may just need to pass on the
incorrect data from the GPS.

And, note the last field, field 9: A.  That means at least a 2D fix.
Does your GPS not emit 9 fields?  That would be part of a solution to
your 2D/3D problem.


RGDS
GARY
---------------------------------------------------------------------------
Gary E. Miller Rellim 109 NW Wilmington Ave., Suite E, Bend, OR 97703
        address@hidden  Tel:+1 541 382 8588

            Veritas liberabit vos. -- Quid est veritas?
    "If you can’t measure it, you can’t improve it." - Lord Kelvin

pgpIL1rvUgbI2.pgp
Description: OpenPGP digital signature