stereo_image_proc/Reviews/01-04-2010_Doc_Review

Reviewer:

• Radu
• kwc

Instructions for doing a doc review

See DocReviewProcess for more instructions

• Does the documentation define the Users of your Package, i.e. for the expected usages of your Stack, which APIs will users engage with?

  Not sure if this question is appropriate for stereo_image_proc.

• Are all of these APIs documented?

  The package's API is not entirely documented. Most functions do not have a clear description or usage example.

• Do relevant usages have associated tutorials? (you can ignore this if a Stack-level tutorial covers the relevant usage), and are the indexed in the right places?

  Yes.

• If there are hardware dependencies of the Package, are these documented?

  There are no hardware dependencies.

• Is it clear to an outside user what the roadmap is for the Package?

  If by roadmap we refer to the usage/dependency graph, then yes, it is clear.

• Is it clear to an outside user what the stability is for the Package?

  Yes, the package status is marked as unreviewed.

• Are concepts introduced by the Package well illustrated?

  Yes.

• Is the research related to the Package referenced properly? i.e. can users easily get to relevant papers?

  No, but stereo processing has become a known technique, so maybe it's not needed? However, we could reference some of Kurt's work here.

• Are any mathematical formulas in the Package not covered by papers properly documented?

For each launch file in a Package

• Is it clear how to run that launch file?

There are no lunch files, however, it is clear how to start the nodes.

• Does the launch file start up with no errors when run correctly?

Yes.

• Do the Nodes in that launch file correctly use ROS_ERROR/ROS_WARN/ROS_INFO logging levels?

There are no messages at all, which might be an issue.

Concerns / issues

• Not all stereo/color processing parameters are enabled via dynamic_reconfigure. Looking at StereoImageProc.cfg, some seem commented out.

• Doxygen-like API comments are missing. This shouldn't be hard to do: just reformat the already existing comments.

• Maybe it's just me, but the code indentation is driving me crazy. I wonder how many different editors were used to edit the files in this package.

• The code contains a lot of values embedded that should be either set globally via #define, or sent as parameters.
• The header files include definitions of methods that do not exist. See do_stereo_sparse_fast () for example.

• Why is FindPlanes part of stereo_image_proc ?

• Lots of @todos all over the place - do they represent things that still need to be done before release?
• Does stereoimage.cpp have lots of things in common with image_proc/image.cpp ? If so, shouldn't they be grouped together for code reusage?
• The wiki docs are pointing to pages which do not exist. Example:

  • To view point clouds, you can use rviz. See instructions _here_.

• Some of the parameters under "3.1.3 Parameters" on the wiki, do not contain enough information. Either the user should be referenced to a different source (e.g., scientific paper, other wiki page) or the parameter should be better explained. Example:

~num_disp (int, default: 64)

• Number of disparities (pixels).

kwc:

• Documentation should not assume any knowledge of the PR2 launch files or expectation that the user is using those. If need be, page can link to a PR2-specific page elsewhere, but definitely not here.

Conclusion

• More of an all-round review than a DOC review, the package looks good and it is easy to use. However, there are a few outstanding issues (see above) that need to be resolved before an official release in my opinion.