pr2_controller_interface/Reviews/2009-11-04_Doc_Review

Reviewer:

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

Vijay

If you're feeling ambitious, a flowchart/state-machine would be awesome. Something like:

init() → starting() → update() (loopback arrow) –stopping()

Specific Edits

2.2 Starting the controller
Typo? The starting method returns **true** if the startup was successful. If startup fails, the controller will not be started (update will not get called). It is possible to re-try starting a controller after it failed.
2.3 Updating the controller
Before: In the update loop, the real controller work is done.
After: In the update loop, the real control work is done.
2.4 Stopping the controller
I don't understand why the “every time” clause is so important. Maybe it needs another sentence of explanation?
2.5 Requesting a pointer to another controller
The arguments should be referred to by name, as opposed to type. Show the template info for the function signature. Is there a relevant tutorial? Might be useful to post a link to a relevant tutorial, if there is one.

Gunter

This is looking pretty good!

References to pr2_mechanism_control (e.g. in summary) presumably should now be pr2_controller_manager? --> DONE (fixed in manifest, will appear in wiki soon)
I like the flowchart, but it feels a little out of context at the beginning. Maybe after the first paragraph? Also, any way of highlighting that init() is non-realtime, rest are realtime? --> DONE (fixed in wiki)
Is there any issue of 'const NodeHandle' remaining? Does the tutorial need to be updated as not to copy the node handle any more? When running I had to insert a const - though I probably didn't have the absolutely latest code base. --> DONE (const NodeHandle is gone. I fixed the copy's in the tutorials)
Can you clarify: calling starting() again (retry) would be in a later cycle. --> What do you mean?? ==> I meant the re-try will have to be a separate service call and hence doesn't occur in the same cycle. There is no automatic re-try within the same servo cycle. How about something like:

The starting method is called once every time a controller is started BY THE PR2_CONTROLLER_MANAGER. Starting is executed in the same cycle as the first update call, right before this update call.

The starting method returns true if the startup was successful. If startup fails, the controller will not be started (update will not get called). THE PR2_CONTROLLER_MANAGER IS ALLOWED TO RE-TRY STARTING AGAIN AT A LATER TIME WITHOUT UNLOADING/LOADING THE CONTROLLER. --> DONE (fixed in wiki)

Can you help me understand (this is probably my C++ limitations): what is the difference in the controller and pr2_control_interface namespaces? In the intro and in getController you use the controller::Controller base class and name space. But in the include file you specify namespace pr2_controller_interface?? --> DONE (This was just wrong in the wiki. It should all have been pr2_controller_interface)
To clarify getController(), can you mention at the start that you implement four functions AND HAVE ACCESS TO ONE? Otherwise it seems (at first glance) that there is a fifth? --> DONE (corrected in wiki)