Re: [gpsd-dev] [PATCH] Support UBX NAV-PVT

[Fred Wright]

On Tue, 1 Aug 2017, Fred Wright wrote:

> I want to look at this some more before pushing it, but at least I have a
> valid commit now.

Ok, I've looked at it now, and have two substantive corrections plus a
bunch of whitespace corrections.

1) The change to the big case statement accidentally deleted the "break"
line at line 615, casuing UBX_NAV_PVT to fall through to UBX_NAV_POSUTM.
Since the latter does nothing but emit a log message, it didn't affect the
test results, but would have been nticeable with logging turned on.

2) I don't agree with the Pythagorean scale factor in mapping hacc to epx
and epy (line 239).  The correct mapping depends upon circumstances, and
in particular, for a circular error zone, one would have epx = epy = hacc.
Although epx and/or epy may *sometimes* be smaller than hacc, there's not
enough information to determine this, so the conservative thing to do is
assume the circular case.  Also, some forum posts suggest that hacc tends
to be overoptimistic, so scaling it down is underconservative.  In
addition, if I *were* using a scale factor, I'd define it as a parameter
rather than burying a magic constant in the body of the code, and wouldn't
be so stingy with the precision.

Whitespace-wise, the general issue is that this file uses tabs as much as
possible, while your additions used all spaces.  The overarching rule on
style is consistency.  There were also two preexisting lines gratuitously
changed to spaces without any real edits.

I've pushed a set of three commits to a "clarkli" branch on my GitHub,
whose diffs you can see here:

        https://github.com/fhgwright/gpsd/compare/master...clarkli

The first commit is yours as is.  The second makes two substantive changes
as noted above (including updating the test results to reflect the epx/epy
change).  The third is whitespace-only changes, including breaking up one
long line.

If you agree, I'll squash this into one corrected commit and push it.

Fred Wright