camera1394/Reviews/2010-06-19_Doc_Review

Proposer: Jack O'Quin

Reviewers:

• Eric Perko
• Blaise Gassend

Documentation

• camera1394

• camera1394/Tutorials

• camera1394/Troubleshooting

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?

Concerns and Issues

Enter your thoughts on the documentation and any questions or concerns you have here. Please sign your name. Anything you want to address in the review should be documented here before the start of the meeting.

• (Jack) Would the camera1394 page be clearer if documentation for the Box Turtle version moved to a different, subordinate page?

  • (Eric) I think moving the docs for the BoxTurtle version's parameters to a separate page would be a good move. Right now, it might be confusing to a new user if they missed that sentence stating that a whole section of the docs are for the old version, not the latest C-Turtle version. Are there other ROS packages that have gone through a major change like this driver did? How did they handle documentation of old and new interfaces?

  • (Jack) I plan to move that to a subordinate wiki page. Not sure how others handle that. Strictly speaking, it was an "experimental", "unstable" interface and does not require continued support. I do intend to provide it for Box Turtle users for as long as makes sense. We'll have to decide (with user input) whether to back-port the stable interface for that release.

• (Eric) Would it be worthwhile to start some sort of section listing cameras that are known to be compatible with this driver, the level of features they implement, etc? At the very least, it could be nice to have a list of cameras ROS users have used successfully. I wouldn't want to duplicate the list of supported hardware on the libdc1394 pages, but it might be nice to have some idea of whether a camera has some good or bad points related to use in robotics.

  • (Jack) Good idea.

• (Eric) All the tutorials looked pretty good and should be more than enough for an initial release.

• (Eric) I like the different things addressed by the Troubleshooting pages, but I feel like the organization is a bit... clunky. For example, when I looked at the first error and it's steps, I was confused by the things to check after there was a solution listed. It was unclear that there were steps and solutions to the things addressed by each step. Perhaps different subsections for each possible cause. So for each error, we could have some sort of table or something that would include Possible Cause, Troubleshooting Steps to determine the Cause, Possible Solution. Thoughts?

  • (Jack) I agree that the current format is clunky. My first attempt was just to collect information on most of the important failures. We can add more as questions arise on ros-users. I'd like some kind of "flowchart" for diagnosing problems. I'll check how other ROS package handle similar issues.

• (Blaise) First, I would like to say congratulations. This documentation looks really easy to use, and really covers all the bases. Good work! Now all the nitty details that I was able to scrape (with difficulty) from the bottom of the barrel.

• (Blaise) The "change the permissions" in ConnectingToIEEE1394Cameras link didn't jump to the right location in the troubleshooting file.

• (Blaise) In the dynamic reconfiguration tutorial, you say that setting incorrect values for the first four parameters is likely to cause the camera to stop responding. Do you mean that the camera will crash, and setting correct values at a later time will not work? Is there a software way of resetting the camera when this happens?

  • (Jack) Some cameras behave better than others. I mention some resetting techniques in the TroubleShooting page, will add a link to that in the Tutorials.

• (Blaise) In the bus bandwidth section, you say "if you have a newer bus, change the iso_speed to 800". I think that "you can increase the iso_speed to 800" would be better. Currently you get the impression that you should be increasing to 800 Mbps, but you should only be doing that if the data rate from your camera actually needs it and you aren't sharing the bandwidth with other devices. (Extra comment, can you really not set the iso_speed lower than 100 Mbps? Aren't there low resolution/low frame rate modes that require less than that?)

  • (Jack) Ken Tossell confirms that setting iso_speed to 800 should only be a suggestion.

  • (Jack) the standard only goes down to 100 Mb/s for the bus. That is a "legacy" mode. I doubt you can find many systems with less than 400 Mb/s for 1394a. The newer 1394b is 800 Mb/s. Higher bandwidths are defined by libdc1394, and there are emerging standards so I allow setting them, but I doubt they will get much use any time soon. Any individual camera can use as much or as little bandwidth as it needs (up to the bus limit).

• (Blaise) In the Bayer tutorial: I think that the overview sentence might be more accurate as: "Many IIDC cameras provide color images in a Bayer coded form."

• (Blaise) In the camera features tutorial, you say: "Select Query periodically, while the value and the picture keep changing." It would make more sense to me to say "Wait until the image has stabilized, and select the Query state." Are there cases where the more tedious process you describe will actually give better results?

  • (Jack) I have found that the values continue to change slightly even though the picture appears the same (to my eyes). I'll try to clarify that in the text.

• (Blaise) In the "Using the parameter server" section, you say "For IIDC features, don't forget to set the control state to Manual (3), otherwise the corresponding value will probably be ignored by the device.". The following would make more sense to me: "For IIDC features, don't forget to set the control state to Manual (3), otherwise the value will be ignored if the camera defaults to a control state other than manual."

  • (Jack) It's not necessarily the default, it depends on what state the camera was in when the driver starts.

• (Blaise) In the multiple cameras tutorial, stereo camera section, you can actually simplify things. Normally, you simply remap image to camera_name/left/image. You don't have to remap each topic independently. (Can you confirm that remapping the image namespace does work with your driver? I believe that you would have to make an effort for it not to work, but remapping has caught me off guard enough times that I mistrust my predictive abilities.)

  • (Jack) should work, will test.

• (Blaise) I think that I would have appreciated a justification for the bizarre ranges for the parameters, especially why some start at 0 and others at -10. A little paragraph somewhere would be helpful.

  • (Jack) There's not much justification. That's why I was originally hoping to eliminate the GUI ranges altogether (but didn't get it done in time).

• (Blaise) The doxygen page should say that this driver has no code API. (Let me know if you need help on how to do this. You can probably take the .dox file out of wge100 as an example.)

• (Blaise) The wiki is automatically making some of the parameters (e.g. OnePush) into wiki links. Adding an exclamation mark in front should solve the problem. I recognize that this is a limitation of the autogenerated dynamic reconfigure documentation.

  • (Jack) I was sticking with the auto-generated parameter documentation. Now that things are stable, I'll hand-edit the output to fix some of those problems.

• (Blaise) The documentation for ~bayer_method has each parameter repeated twice, rather than having the numerical value. E.g., HQ (HQ). Likewise image_proc in the same parameter has empty parentheses after it. Same comment for bayer_pattern.

  • (Jack) There is no numerical value, this is an enum of str_t. That is what dynamic_reconfigure generates: first the variable name, then the string value. They just happen to be the same. I'll hand-edit the output to fix some of those problems. We might want to consider improving enum of str_t handling in the future.

• (Blaise) I found it a bit strange how you say that the default value for the camera_info_url is uncalibrated. It gave me the impression that the string was set to "uncalibrated". I think it would be preferable to say that the default is the empty string, and separately indicate the meaning of the empty string. I also noticed that you are sometimes indicating the default value in the parameter description instead of letting the wiki format the default for you. With more digging I found that you were probably doing that because the wiki is failing to report the empty string as a default value. I opened ticket rot#2830 to see if there is a better way.

Meeting agenda

As there is not much controversy, we decided to have the Conclusion discussion via e-mail. This page will be updated to reflect the results.

This meeting is scheduled for 2010-06-22 at 3:00PM Central Time (1:00PM Pacific Time).

Conclusion

Completed tasks:

• Move the Box Turtle version documentation to a subordinate page.

• Make a subordinate page listing cameras that are known to work.

• Clarify wording in numerous places.

• Make setting iso_speed to 800 a suggestion, when bus, camera and adapter all support 1394b.

• Clean up auto-generated parameter documentation by hand.

Deferred until later:

• Figure out how to organize the TroubleShooting page so the control flow is clearer. Other packages don't seem to do it any better.

This review is now closed.