wge100_camera/Reviews/2010_01_21_Doc_Review

Reviewer:

Instructions for doing a doc review

See DocReviewProcess for more instructions

1. Introduction scopes users well and points to other good starting points
2. All APIs seems to be documented, except as noted below
3. Tutorials exist. Tutorials on external triggering are light on explanation, but since these come pre-configured, this may be enough.
4. Hardware dependencies are documented.
5. Is it clear to an outside user what the roadmap is for the Package?
6. Is it clear to an outside user what the stability is for the Package?
7. Are concepts introduced by the Package well illustrated? The timing could be confusing to a new user. Other than that, it seems good.
8. Is the research related to the Package referenced properly? i.e. can users easily get to relevant papers? N/A
9. Are any mathematical formulas in the Package not covered by papers properly documented? N/A

For each launch file in a Package

1. Is it clear how to run that launch file?
2. Does the launch file start up with no errors when run correctly?
3. Do the Nodes in that launch file correctly use ROS_ERROR/ROS_WARN/ROS_INFO logging levels?

Concerns / issues

Eric

• "GPL only because of list.h" is unclear - adding "all other files licensed under BSD" or other similar statement would clarify

  • Blaise: Changed to "GPL only because of list.h; other files released under BSD. Brian actually pushed for not mentioning BSD in the manifest, but I tend to side with Eric on this one.

• "the driver expects timestamps to be provided on a roslib/Header topic" - this topic should be named, and should be included in the ROS api. I didn't see this topic on a running robot, so I couldn't fill in this information.

  • Blaise: I have added it to the subscriptions section of the ROS API and in the timestamping section. This topic is interesting because the name of the topic the node subscribes to is set dynamically by a parameter. Back when I implemented this, I didn't know about remapping, and it was the natural thing to do. If I had done it more recently, I would probably have used remapping, and then had huge problems getting the synchronizer to work. Indeed both the MultiTriggerController and the wge100_camera_node need to change around the topic name on which they communicate timestamps when changing camera triggering modes on the head. Makes me realize that in a dynamically reconfigurable world, static remapping can be quite a liability. Of course you can make things work by having a short lived node that copies from one topic to the other, but that involves a performance hit (small for sending around headers). Some kind of symlinking would also get around this, but enough rambling for now...

• firmware_images/wge100_camera_demo.launch doesn't seem to be in the right place, unless it's meant to be hidden, in which case "demo" seems like the wrong name.

  • Blaise: Fixed that a couple of days ago in trunk. It was definitely an accidental move.

Tutorials

• "Make sure that your WGE100 camera is powered at 12V, connected to your computer, and has a green flashing light like below." Doesn't provide and guidance on how to connect to the computer or to 12V. Is there documentation of the pinout or something else we can point people to? If not, we should tell them not to use the camera unless they have a 12V and ethernet adapter cable from WG (which may be OK, since we're not giving these out standalone).

Rob

Main page

• In section 2.1, both monocular and stereo point to image_proc. Is this correct, or should stereo point to stereo_image_proc?

  • Blaise: Good catch, you're thorough.

• In section 2.2, should access_register and discover be links?

  • Blaise: Wasn't my plan, but it is a good idea. Done.

• In section 5.2, should the camera name be new_name the second time through?

  • Blaise: Yup. Now you know I faked it all. (There was a good reason, I think I went and changed the messages in the code, and then couldn't run the camera for some reason or other.)

Advanced commands

• You might want to boldly mention that check_flash is destructive (besides the comment in the code block)

  • Blaise: Done, in bold.

Tutorials

• Section 1.8, Viewing the WGE100 Camera Images, still contains a FIXME

  • Blaise: It is gone.

• One tutorial is ConfiguringAndUsing the other is ConfiguringandUsing

  • Blaise: Fixed.

• The external triggering tutorial is incomplete

  • Blaise: I nixed that one. It was obsolete in addition to being incomplete. And currently there is no use case for using it.

Conclusion

Eric

• All changes done in place except for concerns above.

  • Blaise: Done

• All changes to tutorials done in line except for :
  • "Make sure that your WGE100 camera is powered at 12V, connected to your computer, and has a green flashing light like below." Doesn't provide and guidance on how to connect to the computer or to 12V. Is there documentation of the pinout or something else we can point people to? If not, we should tell them not to use the camera unless they have a 12V and ethernet adapter cable from WG (which may be OK, since we're not giving these out standalone).

Rob

• Minor changes made in place except for concerns above.
• Overall, really nice documentation Blaise!