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

[Clark Li]

>Previously, I hadn't bothered to try to apply your patch after seeing the 
>issue I reported, but I've now tried it and it (either version) fails to 
>apply.  There seems to be some inconsistency between the state of driver_ubx.c 
>and what the patch expects.  Ordinarily this might be due to the patch's being 
>based on a different revision of the file, but in this case the blob ID in the 
>patch matches the state of the file, so I'm puzzled.  Did you have uncommitted 
>changes to the file at the time you ran git format-patch?  I don't know what 
>the effect of that is, but it's my best guess as to why the patch is bad.
That last patch was not based on the very latest in master branch. But I guess 
forwarding the output of git-send-email from gmail to outlook should also be 
blamed... I am attaching the original git-format-patch output this time, 
hopefully this one can be applied without conflicts.

>Except that if you look closely at the diff, you'll see that the only actual 
>change to that file is the introduction of some spurious line breaks.  It 
>makes sense that the output is effectively unchanged, but it doesn't make 
>sense that some spurious newlines appeared.  Did you perhaps obtain the 
>"updated" version via copy-n-paste rather than from "regress-driver -b"?
The chk file was updated using "regress-driver -b" but again this might be due 
to the email forwarding...

>It's also odd that only that one case was affected by whatever you did. 
>Did you only run the one test case, rather than all of them (typically via 
>"scons check")?
scons check was run to check all the regression tests. ublox-aek-4t.log might 
be the only regression that contains NAV-NAV-SOL.

>With regard to the new test data, did you do some form of sanity check on it?  
>All the regression tests can do is verify that the code continues to produce 
>the same results as before; they have no built-in concept of "correct" results.
Yes sanity check has been done on for positon/speed/time when the log was 
recorded.

>I don't know the specific details, but since it looks like you're adding 
>support for a new message type that duplicates the functionality of an older 
>message type, a possible approach would be to configure the receiver to output 
>both and then verify that both the old and new versions of the code come up 
>with the same (or possibly better) TPV decodes.  Though if supporting both 
>results in duplicate TPV outputs, some tweaking would be needed to be able to 
>compare them.
TPV for NAV-PVT provides extra 'epx', 'epy' and 'epv' fields while NAV-SOL does 
not. I assume you meant 'more fields' by 'better TPV decodes', as positional 
accuracy depends on the actual GPS configuration and its internal calculation.

>Incidentally, when updating regression-test data, a fairly convenient approach 
>(if one is working in a git repo) is just to run "scons gps-makeregress" and 
>then let git tell you what actually changed (and see if it's plausible).  That 
>lets you view the diffs much more flexibly than looking at the test output 
>from gps-regress.
Thanks for the extra tips!

-----Original Message-----
From: Fred Wright [mailto:address@hidden 
Sent: Saturday, July 22, 2017 8:41 AM
To: GPSD Developers List
Cc: Clark Li
Subject: RE: [gpsd-dev] [PATCH] Support UBX NAV-PVT


(Back on-list since some issues might be of wider interest)

On Thu, 20 Jul 2017, Clark Li wrote:

> Thanks Fred. I've sent out another one with log/check for NAV-PVT from 
> ublox-neo-m8n.

Previously, I hadn't bothered to try to apply your patch after seeing the issue 
I reported, but I've now tried it and it (either version) fails to apply.  
There seems to be some inconsistency between the state of driver_ubx.c and what 
the patch expects.  Ordinarily this might be due to the patch's being based on 
a different revision of the file, but in this case the blob ID in the patch 
matches the state of the file, so I'm puzzled.  Did you have uncommitted 
changes to the file at the time you ran git format-patch?  I don't know what 
the effect of that is, but it's my best guess as to why the patch is bad.

> Changes for ublox-aek-4t.log.chk is actually for the processing fTOW 
> in driver_ubx.c

Except that if you look closely at the diff, you'll see that the only actual 
change to that file is the introduction of some spurious line breaks.  It makes 
sense that the output is effectively unchanged, but it doesn't make sense that 
some spurious newlines appeared.  Did you perhaps obtain the "updated" version 
via copy-n-paste rather than from "regress-driver -b"?

It's also odd that only that one case was affected by whatever you did.
Did you only run the one test case, rather than all of them (typically via 
"scons check")?

With regard to the new test data, did you do some form of sanity check on it?  
All the regression tests can do is verify that the code continues to produce 
the same results as before; they have no built-in concept of "correct" results. 
 I don't know the specific details, but since it looks like you're adding 
support for a new message type that duplicates the functionality of an older 
message type, a possible approach would be to configure the receiver to output 
both and then verify that both the old and new versions of the code come up 
with the same (or possibly better) TPV decodes.  Though if supporting both 
results in duplicate TPV outputs, some tweaking would be needed to be able to 
compare them.

Incidentally, when updating regression-test data, a fairly convenient approach 
(if one is working in a git repo) is just to run "scons gps-makeregress" and 
then let git tell you what actually changed (and see if it's plausible).  That 
lets you view the diffs much more flexibly than looking at the test output from 
gps-regress.

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.

0001-Support-UBX-NAV-PVT.patch
Description: 0001-Support-UBX-NAV-PVT.patch