Re: [Gearbox-devel] Review: Garmin GPS driver

[Alex M. <al...@ca...>]

Thanks Geoff,
alex 


> ======
> - gbxutilacfr needs to be moved out of of the gbxsickacfr 
> directory if other libraries are going to start depending on 
> it. This should be done as soon as possible.

Done. It's now in src/gbxutilacfr

> - Regarding driver.cpp:93, what sort of feedback does the 
> user get that the GPS is functioning? Is it even possible to 
> tell, with NMEA sentences, that the GPS is functioning beyond 
> it getting a fix?
> 
> doc.dox:
> ========
> * You sent a revision saying you had removed the dependency 
> on IceUtil, but the docs still mention it. If it doesn't 
> depend on IceUtil anymore, this should be removed.

Done.

> . Briefly mention the exception types that may be thrown by 
> this library.

I say it specifically now in the documentation of the Driver::read()
function with a an example of how to catch it how to print out the message.

 
> driver.h:
> =========
> - The Novatel stuff is confusing. I think this would be 
> vastly improved by doing as the enum's comment says and 
> categorising them into more generic names.

All gone. Only three NMEA fix types are listed.

> . Line 135: "monitorining"? Perhaps it's just a problem with 
> my monitorin, but that looks like a typo. :)

Gone.

> 
> driver.cpp:
> ===========
> . Lines 275-278: Could unknown messages be a visible warning 
> instead of an error? This may allow more flexibility with 
> other GPS devices that output NMEA sentences.

In enableDevice() we tell the device explicitely which message to send. So
we don't expect to get anything other than a (sub)set of the 3 messages we
can parse. Therefore the list of messages which we handle here is
exhaustive. If we add another message it will show up here as well.

> . Lines 341-342, 378-379, 399-400: This would be good 
> information to have in the logging output (in a verbose mode 
> or something?).

In the spirit of minimum useful functionality, I'd leave this to the user of
the driver. It's easy to dump this to whatever once you have the message.
What do you think?

> * Line 387: The contents of this file should be available to 
> library users so they understand the data they're getting.

Added it to the documentation.
 
> test.cpp:
> =========
> * Line 50: Copy-and-pasted comments should be changed.
> * The example should be improved by actually printing out 
> some of the received data.

Done.

> 
> nmea.h:
> =======
> - If you feel enthused enough, the indentation in this file 
> could do with cleaning up.

Did some. :)

> 
> latlon2mga.*
> ============
> ? Unused. I assume these are providing a facility of the 
> gbxgpsutilacfr library that the Garmin driver doesn't use? 
> Possibly should be removed, and added later if they are 
> needed elsewhere.

Your guess was right. Removed completely.