[Gearbox-devel] Review: Garmin GPS driver

[Michael M. <m....@ca...>]

Hi,

usual caveats (affiliated to Orca, only read through the code).
------------------------------------------------------------------------
==========
= General
==========
-Relevance: relevant, fairly common (low-end) gps receiver
-Functionality: Not tested
-Criticism: -
-Nice to haves: -

===========
= Specific
===========
I've prefixed all my comments with {*|-|.}, meaning:
* I think this needs to be fixed before acceptance.
- This could be better and I'd strongly recommend fixing it, but I'd
    vote for acceptance regardless.
. This is merely a suggestion which the author is free to ignore.
? Not sure what's going on.

API:
=====
*enum PositionType (driver.h:40-74): All the Novatel gear needs to go.
IMO the information here should be exactly what the device is providing,
no more, no less.
*struct Data (driver.h:77-122): some of the fields are not used (e.g. 
l.116-117 observationCountOnL1/2), these should go.
.The assembly of different NmeaMessages into one monolithic struct Data
might be better of in the glue-layer
.horizontal/verticalPositionError (l.102-106): these are probably just
dop (dilution of precision). If that's the case they are only a lower 
bound (based on satellite geometry) for the true error, which might be 
much larger.
Potentially worth documenting.

Driver:
========
driver.cpp:
. 14-15 should be deleted
? 17 is this portable?
. 63 should be deleted
*170-186 replaced ICE dependent timing with POSIX. However, timeOfRead_
and friends are _not_ set, so timestamps are always the same 
(uninitialised) value.
.timestamps (once the above is corrected) are set on receiving of a GGA
message; this should be documented.
?Are all messages in a frame certain to refer to the same time?
.290-291 some tokens (DiffAge/Id) seem to be ignored.
-338 Imho geoidal Separation refers to the local height difference
between geoid and ellipsoid; Here it is assigned a token of name 
GeoidHgt, which sounds more like height over geoid. Should be checked 
against docs.
.341-344 delete?
.356 HeadingMag ignored
*356 climbRate made up (0.0; should be dropped in API)
?387 where is "garminErrorPositionEstimate.txt"; might answer my comment
about dop; should potentially move to header/doxygen
?394 token ignored (EPE)?

gbxgpsutilacfr:
================
latlon2mga.cpp/h doesn't seem to be used. Might be re-named latlon2utm.
nmea.cpp/h I like it, no comments.

test:
======
I like it, no comments.

Cheers,
Michael

-- 
Michael Moser                                         Research Student
Australian Centre for Field Robotics   University of Sydney, Australia
http://www.acfr.usyd.edu.au                      http://www.cas.edu.au
Phone: +61 2 9036 9691    Mobile: 0415 917 607    Fax: +61 2 9351 7474