navigation/Reviews/2009-10-02_Doc_Review

This review will cover the following packages:

• base_local_planner

• carrot_planner

• costmap_2d

• move_base

• move_base_msgs

• nav_core

• navfn

• voxel_grid

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

Vijay

• Be consistent about how you specify units in your descriptions.
  

  • <Description>, <Units>
    <Subject>, <Units>, <more info>

• There are many dead links in the sidebar. These need to at least be populated with stubs like "There a no costmap_2d tutorials, refer to the navstack tutorials".

Ethan

• Use of the suffix ROS as in Costmap2DROS and NavfnROS seems weird. Are these nodes? Is there something else we can call them to be more specific?
  • The ROS suffix corresponds to a ROS Wrapper object. They're not nodes, but they are instantiated within nodes, read their configuration from the parameter server, publish over ROS, etc. I've added a descriptive section about this to the relevant pages.
• Is there a way of combating parameter overload? Perhaps examples of specific configurations for various robots, how the parameters to change were discovered, how the values were chosen, and how those changes gave rise to the desired robot behavior.
  • Adding example configurations to each package can help and I may do this in the near future. However, all the parameters that are available still need to be documented fully.

• Is ~name the private namespace for the node? Sometimes it is ~name, sometimes ~<name>. It seems like it should just be ~/.

  • This happened because someone came in and changed half of the pages to a new doc template and left the others. Apparently ~<name>/ is the proper convention.

• Tutorial packages show up in master list of navigation packages... should they? Probably not. Also organization at stack level is a little weird. Should "navigation" stack include amcl, fake_localization, map_server, robot_pose_ekf, voxel_grid? Maybe these belong in a "Mapping and Localization" stack.
  • The tutorials should be hidden from the master list and I've talked to Ken about this. As for the amcl, fake_localization, map_server, etc we talked about them being in a different stack, but decided against it for now. They may move out of the navigation stack in the future though.

base_local_planner

Vijay

• Overview Section

  • You say that this implements Trajectory Rollout and the Dynamic Window strategies. However, in the overview, you provide a different which looks like a home-brewed algorithm. Is your algorithm a hybrid of Trajectory Rollout and Dynamic Window, or is it one or the other? Be clear about how these 3 algorithms relate.

    • Modified the explananation to, I hope, be more clear on this
  • Provide a paper reference to Trajectory Rollout. If you already have, then add the words "Trajectory Rollout" somewhere in its description.
    • Added the words "Trajectory Rollout" to the relevant paper
• C++ ROS Wrappers Section
  • Why are they "C++ ROS Wrappers"? This section could probably just be called Nodes.
    • Ok, upon further inspection, these are actually not Nodes. Given that it has all the attributes of a Node (published topics, subscribed topics, params), I just assumed that it was a node.
      • Added better explanation of what a C++ ROS Wrapper is.

  • The TrajectoryPlannerROS overview isn't clear. It's also passive. And why is "name" in bold? Either explain what name is, or don't talk about it.

    Suggestion:
    TrajectoryPlannerROS
    The base_local_planner::TrajectoryPlannerROS sets up and maintains an underlying base_local_planner::TrajectoryPlanner object and also provides a ROS communication interface for it. It reads its configuration from the Parameter Server and operates within a ROS namespace (from hereon assumed to be name) specified at construction time. It also adheres to the nav_core::BaseLocalPlanner interface found in the nav_core package.

    Note that even though TrajectoryPlannerROS has ROS Params and communicates on ROS Topics, it is not a ROS Node. It interacts with other navigation via function calls, and thus cannot be run on the command line. Instead, it is instantiated inside of a ROS Node, along with other navigation stack classes.

    • Changed the text to be close to that suggested.
• Parameters section

  • ~name/max_vel_x - Is there no max_vel_y? Description for max_vel_x should be explicit about which direction:

    Suggestions:
    the maximum x velocity allowed for the base in meters/sec

    • Added a more clear description. There is the "y_vels" parameter which allows the user to specify what strafing velocities to explore for holonomic robots.

  • Be consistent between using words yaw and rotational. "the yaw acceleration limit". "the maximum rotational velocity".

    • Fixed things so that rotational is used across the board in explanations
• "Does the documentation define the Users of your Package, i.e. for the expected usages of your Stack, which APIs will users engage with?"
  • I have no idea which API I should be using. I'm guessing I only need to know the ROS API, unless I'm doing something more complicated. Do I even need to know the ROS API? Does the navstack hide much of the ROS API?
    • For different users, this means different things, the ROS API should be sufficient for most, but there are exceptions (ie: Sachin's door planner), I'm considering adding a section that addresses this, but the navigation tutorials make this clear... if I have time I'll do it
• "Are concepts introduced by the Package well illustrated?"

  • A screenshot showing local_plan would be nice.

  • A super short explanation of DWA and Traj Rollout would be nice.
    • Added a picture that I hope helps with this
• Is it clear to an outside user what the roadmap is for the Package?
  • Not present, but I'm also not sure what is needed
    • I don't think a roadmap is needed at the package level
• Is it clear to an outside user what the stability is for the Package?
  • Need a sentence about this
    • Added

• You have an uncommented script scripts/indefinite_nav.py. Either remove it, or document both the script and the wiki page explaining it.

  • Removed this script

Ethan

• base_local_planner package summary could use a more general intro sentence for those that don't understand the local/global destinction. Maybe modify the first sentence to be something like:
  • "This package provides implementations of the Trajectory Rollout and Dynamic Window approaches to local robot navigation on a plane." The overview does a good job of this, so it may be fine.
    • Updated the manifest
• Why is the local planner PUBLISHING name/global_plan, shouldn't it be subscribing to it?
  • Added comments that make this clear. The local planner receives the global plan via a function call, but it publishes the portion of the global plan that it is pursuing viz ROS for visualization purposes.
• What is DWA? This needs some explanation if it's the default. It seems to be picking a single curve based on some criteria, then computing the accelerations needed to achieve it rather than doing the trajectory roll out that is described in the rest of the package documentation.
  • Added text that, I hope, clears this up
• ~name/prune_plan (default: true) needs more specification for when a robot will eat up the path. As is I would have to look in the code to figure out if I wanted this option.
  • Added a better explananation
• ~name/simple_attractor What is an example where you might actually want to use this? If there isn't one it shouldn't be a param
  • This parameter is now deprecated. Really no use for it, was a bad first hack at the carrot planner.

carrot_planner

See carrot_planner/Reviews/2009-10-06 Doc Review

costmap_2d

See costmap_2d/Reviews/2009-10-06 Doc Review

move_base

Vijay

• In the manifest, I'm not sure that robot_action is still a valid term

  • Updated the manifest to have a more current description of move_base
• Action API. We need to figure out a reasonable style for talking about action APIs. I think this is all that you need:

  • MoveBase Action Specification

    • move_base/goal (move_base_msgs/MoveBaseActionGoal)

      • A pose to pursue in the world.

      move_base/feedback (move_base_msgs/MoveBaseActionFeedback)

      • The current position of the base in the world.

      move_base/result (move_base_msgs/MoveBaseActionResult)

      • Empty
        • I still think its important to differentiate between topics flowing in and out of the node for those who don't choose to use actionlib, but still want to talk with move_base
• Lots of parameters need units specified
  • Added units

• Some rephrasing: http://wiki/move_base?action=diff&rev1=50&rev2=51

  • Cool, thanks.
• The "Expected Robot Behavior" sections sounds a lot like an overview. Regardless, something like this needs to go near the top of the wiki page. This paragraph also sounds a lot like a state diagram. A diagram would definitely help
  • Moved it up to the top, added the requested diagram

move_base_msgs

Vijay

• This is empty. It should be populated with something.
• Maybe this package is where the move_base semantics should be described...
  • Added a brief description with links to move_base and actionlib

Ethan

• A brief summary just saying that these are the message types used to interact with the nav-stack using the Action library would probably help.
  • Added a brief description with links to move_base and actionlib

nav_core

Vijay

• Both the base BaseGlobalPlanner and BaseLocalPlanner need more explanation. Copying the descriptions from doxygen might be sufficient, but a few more sentences would be nice.

  • From doxygen: Provides an interface for global planners used in navigation. All global planners written as plugins for the navigation stack must adhere to this interface.
  • The doxygen: Provides an interface for local planners used in navigation. All local planners written as plugins for the navigation stack must adhere to this interface.
  • Added an explanation
• I'd like to see a list of existing local planners and global planners
  • Added these lists
• I'd like to see a picture showing where these two pieces fit in the nav stack
  • Added this picture
• Something about plugins should probably be mentioned
  • Mentioned plugins
• There are dead links in the sidebar. This is not ok.
  • Same comments as for other packages

Ethan

• I second the list of local and global planners, I think that is the most important addition to this page.
  • Added this list

navfn

Vijay

• "Does the documentation define the Users of your Package, i.e. for the expected usages of your Stack, which APIs will users engage with?"
  • Should be defined.
    • A section that I'll add when I have time to go back... not so urgent given the tutorials I think
• 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?
  • It sounds like there's a lot of math going on in here, yet there's no description or links to anything that might explain what's happening inside.
    • I need to ask Kurt if he has a paper that I can link to for this. I'll add this in when he gets back in town.
• ~name/plan - Specify how often this is published. Is it at a given rate, or is it triggered by something?
  • Added this to the text

Ethan

• Navfn can't actually use A* right now I believe, but this is not reflected in the package documentation.
  • Updated to docs to refer to A* as a future feature
• NO DOCUMENTATION FOR navfn.

  • Added Doxygen for NavFn, might take a little bit to show up on the wiki but its checked in. Also, the NavFn API is not stable... only the NavfnROS API is stable. I've updated the docs to talk about this. I just never got around to cleaning up Kurt's C-style code and tracking down the bugy with A*.

• The code API is sufficiently confusing that instructions on this page could be very helpful, particularly for navfn itself. When I was trying to use it I was particularly confused by the fact that the cost-map in the Dijkstras computation was relative to the goal not the start location.
  • Added notes to the relevant function calls in doxygen reflecting this
• What does "fn" stand for? Function?
  • function is right, ask Kurt. I think the docs make this as clear as its going to be

voxel_grid

Vijay

• No released API, so no docs. Sounds good to me.

Ethan

• N/A

Conclusion

Ethan

• There is an impressive amount of documentation written, but the system is sufficiently complicated that this makes sense. However, this makes getting the documentation up to the level of other packages will involve a lot more work. After some preliminary cleanup I would like to take another pass, perhaps through fewer packages. I think spreading the load somewhat would allow reviewers to go through more carefully, and also potentially make more documentation fixes themselves.