pr2_camera_synchronizer/Reviews/2010_01_19_Doc_Review

Reviewer:

Instructions for doing a doc review

See DocReviewProcess for more instructions

1. Does the documentation define the Users of your Package, i.e. for the expected usages of your Stack, which APIs will users engage with?
2. Are all of these APIs documented?
3. 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?
4. If there are hardware dependencies of the Package, are these 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?
8. Is the research related to the Package referenced properly? i.e. can users easily get to relevant papers?
9. Are any mathematical formulas in the Package not covered by papers properly documented?

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

Wim

I made a number of small changes that were obvious. These are remaining issues.

• The introduction misses a general explanation of what you can do with the camera's and projector: you can time the camera exposure and the projector light, so that multiple camera's can run at the same time, and some will see projected images and some will see unprojected images.

  • Blaise: Fixed, will be slow to update from the manifest.

• It could also be helpful to put section 5 more in the beginning, as this gives an overview of the system, and the sections 1-4 rely on the reader already knowing the hardware components.

  • Blaise: Done.

• The last line before section 3, says it is for the right forearm camera, but I don't see any 'r' in that line

  • Blaise: Fixed

• In section 5.3 you say that pr2 ethercat runs controllers. Should this be pr2_controller_manager?

  • Blaise: Fixed

• Section 6.1 is hard to understand Actually, after reading it I have absolutely no idea how to configure the timing

  • Blaise: Give it another try. There was a mismatch between the figure and the text that probably helped confuse you.

  • Could you number the pulses in the image?

    • Blaise: They were, they still are.

  • "Pulses 2 and 4 are spaced by about one pulse-length more than '2T'" I do not understand. I read that 2 and 4 are spaced about T more than 2T, so they are spaced about 3T?

    • Blaise: The following comment should clarify this.

  • I do not understand the difference between a pulse length and a pulse period T.

    • Blaise: The pulse length is how long the projector is on for. The pulse period is the average time between the start of one pulse and the start of the next.

• Section 6.2 "Whether the projector should be on at any given time can't be decided by any one user.". What does this mean? What if I put the projector in 'ProjectorOff' mode, don't I decide what the mode if for for all users?

  • Blaise: I have made the explanation more detailed. Tell me how you like it.

• It looks like I can still get untextured images in 'ProjectorOn' mode? So in the on mode the projector is sometimes off. Then how is this mode different with the auto mode? My best understanding is that on means "on by default, but automatically off when needed" and that auto mode means "off by default, but automatically on when needed". Is this correct?

  • Blaise: I have tweaked the definition of the mode to say that the projector is "pulsing" when it is on. Indeed, the projector is never on continuously (it would die). On means that the pulses are being produced whether a camera is using them or now, whereas auto means that the pulses are produced only when a camera is configured to use them.

• It would be good to mention that the 'AlternatingProjector' mode puts the images with and without texture on a different topic.

  • Blaise: It was documented elsewhere. I added it here too.

• Section 6.5: "In particular rates that are a multiple of 120 Hz cannot currently be achieved exactly". Is 120 Hz the only problematic rate, or is this just an example?

  • Blaise: If your AC power is 60Hz, then fluorescent lights flicker at 120Hz. I added a mention of 60Hz AC.

Brian

• I made a few small changes: http://wiki/pr2_camera_synchronizer?action=diff&rev2=44&rev1=40

• Section 3: "To facilitate using the narrow stereo camera in AlternatingProjector mode..." What's AlternatingProjector mode? Ah, it gets defined later, in Section 6.3; perhaps make a forward-pointing link in Section 3.

  • Blaise: Fixed.

• Section 4: "Note: This will only work if the trigger controllers are working." Are the trigger controllers sometimes broken? Perhaps should that be "running" instead of "working"?

  • Blaise: Fixed.

• Suggest putting at the very beginning a link to Section 7 (ROS API), because that's where you find out in detail how to use the synchronizer.

  • Blaise: Added a link near the top.

Conclusion