[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!
Cheers,
Clark
-----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
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
______________________________________________________________________
This email has been scanned by the Symantec Email Security.cloud service.
For more information please visit http://www.symanteccloud.com
______________________________________________________________________
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.