Re: [Gearbox-devel] Review of gbxsickacfr

[Alex B. <a.b...@ac...>]

> And here's a review of the sick driver.

Thanks again.  Comments inline again.

>          - How flexible is this driver with regard to the various other
> SICK models? Would it be hard to adapt, should someone feel motivated to
> do so?

I don't think there was anything about the implementation that would preclude 
the outdoor version, but I haven't tested.  Not sure what other models there 
might be.  I know SICK recently brought out a bunch of much newer versions, 
which I expect would have different protocols but I'm just guessing.

> - More comments in some of the various utility headers briefly
> mentioning what each file is for would allow others to use them more
> easily and thus potentially lead to them being promoted up to the top
> level.

Can you be more specific?  I thought these were fairly well-documented.  I saw 
that gbxutilacfr was a bit sparse, so I added some comments there.

> - The style of doxygen triggers throughout the various files appear 
> to be inconsistent.

You mean like:
//!
//! comment
//!

versus:
/*!
 *  comment
 */
?? Personally I don't mind having both co-exist.

> * No internal units specified in documentation.

Specified now.

> * API documentation lacks description of function arguments.

Which API/functions do you mean?

> messages.h:
> . 156: Bad day? :)

I did a lot of cut-n-paste that day...

> - 168,173,184: What do the comments mean?

The byte-positions in the message.  Doesn't hurt to leave them there, anyone 
reading the SICK manual will guess what they are pretty quick.

> driver.cpp:
> - 75,76: Clarify comments, maybe add TODO?
done.
> * 230: Error string gives wrong function name.
done.
> * 313: Add a TODO for this. Fix file name.
What do you mean?  There's a 'TODO' there as-is.



Cheers,

Alex