costmap_2d/Reviews/2009-10-06 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
Be consistent between "<name>" and "name" across packages
- Should be consistent now
- static_map
- What's a static map? You need to describe it somewhere
- Added description
- I think "/static_map" should be "static_map"
- Changed
You say "If the costmap is initialized from a static map". It would be nice to reference the "~<name>/static_map" param here (maybe even an anchor?), just so I know how it is being initialized.
- Added the reference
- What's a static map? You need to describe it somewhere
- robot_base_frame
I think this frame has to be aligned with "~<name>/footprint". If so, this peculiarity needs to be specified.
- Added an explanation for this
- transform_tolerance - Reading the description, I have no idea what this means. Why do you have even it? I removed it from the laser_scan_assembler.
- Added a better description of this parameter. It is important.
- footprint
- Need to specify units in footprint parameter.
- Added units
- A picture explaining this param would also be useful
- May add one later, but I think the explanation is sufficient for now
- Maybe even provide an explicit example of what this param looks like for a robot like the PR2
- Need to specify units in footprint parameter.
- Sensor Management
- There are a lot of parameters here that have assume a lot of intuition about how these sensors will work with the nav stack. As a user of the costmap, I'm probably going to run down this list of param, and put in a best guess at my first cut at a launch file. To do that, I need to know how to choose better values for these params. You need to answer questions like:
- How do you choose a good max_obstacle_height? I'm guessing this is based on the height of your robot
- Added a section on this
- How do you choose a good min_obstacle height? This is based on sensor noise, I think.
- Added a section on this
- Maybe they don't have to be explained in the parameter section, since that could clutter the section. They could very as easily be their own "choosing suitable costmap parameter values for a robot" section.
- How do you choose a good max_obstacle_height? I'm guessing this is based on the height of your robot
- I have no idea what a "rolling window" is. You need to link to something that explains it, or actually explain it.
- Added a section on this
sensor_frame: For what data source types can the costmap read the frame from the message? I'm guessing it is PointCloud and LaserScan.
- Added a section on this
- data_type: What do you mean by "supported natively"? Can the costmap take plugins that can support it? Maybe "supported natively" should simply be "supported"
- Fair point... done.
~<name>/<source_name>/obstacle_range - Sounds like all obstacles are inserted at 2.5 meters away.
- Changed the explanation of this
- There are a lot of parameters here that have assume a lot of intuition about how these sensors will work with the nav stack. As a user of the costmap, I'm probably going to run down this list of param, and put in a best guess at my first cut at a launch file. To do that, I need to know how to choose better values for these params. You need to answer questions like:
There are many concepts involving the overall function of the costmap that are not explained at all. Right now, I'm trying to piece together a holistic understanding of costmap 2d by merging all the parameter descriptions into one cohesive thought in my head. I'm going to hold off reviewing this until there is more context with which to review this package.
Things to describe: Rolling Window, Static Map, voxel vs costmap, unknown space, ray tracing, map updates
- Added sections at the top of the page that attempt to give a concise but adequate description of the costmap... feel free to tear it up, and let me know what's still missing/unclear
Ethan
- Confusing mention of 3D in summary. Specify that the 3D voxel grid gets projected down onto the plane.
- Added a section that talks about this a bit at the top
- A few images would be immensely helpful.
- Added some images, could probably use some more
- What is doing the free-space stuff? Did those messages go away?
- Um... not sure what you're referring to here. Do you mean clearing freespace? What messages are you talking about?
Vijay (Take 2)
- Overview
- Picture needs some description. Not necessarily clear what all the colors and polygon mean.
- Added a description
- Marking and Clearing
I would rewrite the first sentence:
Before: The costmap is capable of automatically subscribing to sensor topics over ROS and updating itself accordingly.
After: The costmap automatically subscribes to sensors topics over ROS and updates itself accordingly.- Done and done
- Occupied, Free, and Unknown Space
I didn't know that a costmap could have a wide variety of values. As written, it sounds like this was already explained to me somewhere. Do I even need to know that it can have a variety of values? Either explain this more, or explain it less.
- Tried to make thinks clear by changing the sentence
What's the difference between costmap & costmap_2d? Are these the same thing. Why do both have verbatim `` tags around them? Somewhere near the beginning you need something like "The costmap_2d::Costmap2DROS object, from now on, called costmap..."
Now added a sentence in the intro refering to the costmap contained within the costmap_2d::Costmap2DROS object and removed the vebatim `` tags around costmap.
- Inflation
In reference to:
- "We use the term "possibly" because it might be that it is not really an obstacle cell, but some user-preference, that put that particular cost value into the map. "
- Added a one sentence example saying that users can add their own cost values to the costmap
- Costmap2DROS
- Intro
Before: “It is responsible for setup and maintenance of an underlying Costmap2D object”
After: “It is responsible for setup and maintenance of an underlying costmap object”- Changed... a bit different that what you suggested, but I think it works.
- transform_tolerance
- Add something like:
“ If the transform between the global frame and robot_base_frame is transform_tolerance older than ros::time::now(), then the navigation stack will stop the robot.”
- Added something like this. Actually, added exactly this.
- Add something like:
- observation_sources
- Add something like:
“This defines each of the <source_name> namespaces defined below”
- Added
- Add something like:
- expected_update_rate
- What is this param used for? Is it a failsafe check? If I expect data every .1 seconds, should I actually specify .11, just to give a little more buffer?
- Added a much more detailed explanation of this parameter explaining that it is a failsafe. Also, added an example of how to set it.
- What is this param used for? Is it a failsafe check? If I expect data every .1 seconds, should I actually specify .11, just to give a little more buffer?
<source_name>/max_obstacle_height
- Explain how this interacts with max_obstacle_height
- Added an explanation
- Explain how this interacts with max_obstacle_height
- rolling_window
- Can static_map and rolling_window both be set to true. Looking at the docs, it seems like these are mutually exclusive. Make this more clear
- Added this in the parameters section
- Can static_map and rolling_window both be set to true. Looking at the docs, it seems like these are mutually exclusive. Make this more clear
Need units for origin_z & z_resolution
- Added
- mark_threshold
I'm guessing this should read “The <<maximum>> number of marked cells allowed in a column considered to be "free".”
- Oops just missed that, thanks
- Really weird type in Map type parameters section:
“The following pThe threshold value at which to consider a cost lethal when reading in a map.parameters are only used if map_type is set to "costmap"”
- A cut-and-paste gone bad on my part.. fixed
- Intro
- Required Transforms
- Probably should link to amcl here
- Added a link
- Probably should link to amcl here
- Costmap2DPublisher
nflated_obstacles should probably be inflated_obstacles
- fixed
- Costmap2D and VoxelCostmap2D
Need to specify who will need to use this API. This is generally not the suggested way to use costmap_2d (Most people should be using Costmap2DROS, I think). For ObservationBuffer, you already mention this, but you need it for the other libraries as well.
- Added similar sentences for the other objects
- Picture needs some description. Not necessarily clear what all the colors and polygon mean.
Seems like a section missing that should be talking about how the costmap interacts with TF. I didn't even realize how dependent this was on TF, until I asked about exactly what the transform_tolerance parameter did. Talk about why the "required tf transform" is even needed in the first place.
- Added a section about tf
Vijay (Take 3 - Oct 7, 2009)
- Awesome. Looks great!
