[Gearbox-devel] Review of gbxserialacfr

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

Same caveats as with gbxsickacfr (affiliated to Orca, only read through 
the code).

----------------------------------------------------------------------
-Relevance: high, there are many sensors w. serial interface
-Functionality: works reliably; used for NovatelSpan driver in
  Orca/Hydro, which I maintain.
         -Criticism: used/tested with a limited set of hardware,
                 doesn't seem to work in 500k mode (sick),
                 "laser-centric", i.e. current main purpose is
                 to run a sick LMS

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.


===========
= General:
===========
API:
. read() handles timeouts differently (throws immediately) from
   readFull() and readLine() (return -1 after timeout)
. setDebugLevel() implies multiple levels, comment implies on/off, code
   says: 0 -> off, 1 -> function calls, 2 -> data, 3 -> additional
   control flow (levels are additiv)
. several (debug) outputs with hardcoded file/function/linenr info (some
   wrong). I'd recommend replacement with __FILE__ __LINE__ __func__
   __pretty_func__ macros, yes, some of them are gnu only, but some are
   standard (__func__?) and this _is_ a Linux only lib at the moment.
? Why are you using void *buf in all read functions?

Timeout:
. wouldn't it be more elegant to handle blocking/non-blocking with
   select as well?

Exceptions:
. I'm fairly certain that I've seen string and or char* thrown from
   serial in the past;
   could these be caught internally and rethrown as SerialException?

Examples:
. should the test programs show up as examples somewhere in the docu?


============
= Specific:
============
serial.cpp:
- 407: cout can throw afaik, making the destructor throw, not sure
   whether that applies with a string constant.
- 501: it's standard procedure to get the current termios settings, and
   only change what one is interested in. However, I think it's also
   wrong.
   Consider: During testing you have enabled some arcane setting
        manually (setserial and friends), which happens to
        be pivotal to get your device to work. If termios attributes are
        set additively it will work perfectly,
        but only on the particular serial port with the manual setting.
        This could be frustrating to users (dev answer to bug report:
        works perfectly for me).
        It can also bite the developer, if something changes (new
        computer, reinstall, ...). If the manual setting has
        long been forgotten debugging becomes a lot of fun.
        The latter has happened to me once upon a time and cost me two
        very frustrating days.

serial.h
. 13-18 too many includes? termios.h, types.h, fcntl.h not needed at
   all, lockfile.h not needed with forward declaration?
. 101,124 these should be double quotes around \n and \0 in doxygen
   comments

doc.dox
? 35 What does the serial standard (rs232/422) have to do with the
   driver?
? License: refers to "LICENSE file included in this distribution",
   orca/hydro left over?
* Doesn't mention style

lockfile.cpp
. 44 wouldn't it be better if these guys were strings (call .c_str()
   where a char* is needed)?
. 113 shouldn't this be a positive test (errno == ESRCH), i.e. err on
   the side of safty (not unlocking)
. 179 dev_.c_str() couldn't this throw?
----------------------------------------------------------------------

Cheers,
Michael
-- 
Michael Moser                                        Technical Officer
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