wiimote/Reviews/Jan 11 2010_Doc_Review
Reviewer: Blaise
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?
- Are all of these APIs documented?
- 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?
- If there are hardware dependencies of the Package, are these documented?
- Is it clear to an outside user what the roadmap is for the Package?
- Is it clear to an outside user what the stability is for the Package?
- Are concepts introduced by the Package well illustrated?
- Is the research related to the Package referenced properly? i.e. can users easily get to relevant papers?
- 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?
- Does the launch file start up with no errors when run correctly?
- Do the Nodes in that launch file correctly use ROS_ERROR/ROS_WARN/ROS_INFO logging levels?
Concerns / issues
Blaise
- The instructions on how to get the wiimote node going (currently sections 5-7) should be split out into a tutorial for consistency. Some reformatting is also needed to be consistent with other tutorials.
- The node still seems to be using the Point message despite concerns that were raised during the API review.
Gil
The package description is a little confusing. I think there's too much information with too much specificity for a package description. Information about configuration, message types, and such makes the header more confusing. How about: "The wiimote package provides the functionality of allowing a wiimote to communicate with other nodes through the ROS framework. The node uses Bluetooth to communicate with the WiiMote Motion Plus and to make available the data from the controller - accelerometer and gyro data, the state of LEDs, the IR camera, rumble (vibrator), buttons, and battery state. The node additionally allows for feedback to the controller operator using the LEDs and vibration, either directly manipulating the LEDs or vibration or setting up a timed pattern for feedback."
- Given that the API stability is at the top of the page, I think its clear that the package is unstable. Unstable could be put in bold face to make it even more clear.
- I agree with Blaise that a good deal of this material needs to be broken out into tutorials. A reasonable page layout could be to keep sections 1,2,3,7, and 8 on the current page, and make two tutorials, one out of sections 4-6 and another out of 9.
- A picture of the button fields, like the one on ps3joy, would be useful.
Romain
- I agree that a picture with annotations would help understand terms such as "Motion+", "Wiimtoe", "Rocker", and the LED numbering ("from left to right": viewed from which direction?), and I agree about needing a shorter / simpler package description.
- What is the "message send date"? I thought that was the definition of Header stamp. Can you clarify or point to an explanation somewhere?
- switch_array: should be timed_switch_array from the message definition?
- I can't find SWITCH_PULSE_PATTERN (last example) in the message definition.
- I don't understand the pairing process: do you press and then put it down and calibration starts in the next 6 seconds (what are the 6 seconds about in the prompt?), or do you have to put it down and carefully press without making it move?
Conclusion
Blaise
Tuturoials split up
need some formatting still to be consistent ( will do by 01/25/2011)
no longer using Point message
Gil
description fixed
package API is now stable
broken out done
picture fixed
Romain
picture fixed
all other comments addressed