[Top][All Lists]

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

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

From: Clark Li
Subject: Re: [gpsd-dev] [PATCH] Support UBX NAV-PVT
Date: Sun, 6 Aug 2017 23:53:25 +0000

Hi Fred,

I agree with the changes. Thanks for looking into this!

-----Original Message-----
From: gpsd-dev [mailto:address@hidden On Behalf Of Fred Wright
Sent: Friday, August 04, 2017 6:15 AM
To: GPSD Developers List
Subject: Re: [gpsd-dev] [PATCH] Support UBX NAV-PVT

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 

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:

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 

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

Fred Wright

This email has been scanned by the Symantec Email service.
For more information please visit 

This communication may contain confidential or copyright information. If you 
are not an intended recipient, you must not keep, forward, copy, use, save or 
rely on this communication, and any such action is unauthorized and prohibited. 
If you have received this communication in error, please reply to this email to 
notify the sender of its incorrect delivery and then delete both the original 
message and your reply.

reply via email to

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