API review

Proposer: Sachin Chitta

Present at review:

• Tully
• Matei
• Gil

Question / concerns / comments

This review will be for the ROS API for the environment_server node in this package. This is the only API that will be officially released. The underlying C++ API will not be released now. The documentation for the API is on the main Wiki page. It uses messages and services from the planning_environment_msgs directory.

We are only reviewing the planning_environment API, planning_environment_msgs has already been through review.

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

Tully

• Is refresh_interval_collision_map the "expected duration" for collision_map?
• The howto in the parameters is way too long for the short API overview. It makes the topics seem like something completely different.
• ~show_collisions seems redundant, it could be done by using subscriber_callback and the built in condition. unless it's much more expensive.
• you can remap the robot_description parameter instead of parameterizing the parameter
• ~allow_valid_collisions when wouldn't you allow allowable contacts?
• ~refresh_interval_collision_map naming doesn't imply it's expected something like ~collision_map_expected_update_interval maybe?
• ditto for ~refresh_interval_kinematic_state and ~refresh_interval_pose_state
• What does ~compute_contacts do to any inputs or outputs?
• what are the primary data flows/use cases? I cannot understand from the Docs how to hook up this server to be useful? What is required, what is optional?

Matei

• SetConstraints

  • name is vague; maybe SetMonitoringConstraints

• GetExecutionSafety

  • broken link to service API page (maybe slow to update?)
  • the .srv file does not seem to exist in the repository

  • what is the difference between this and GetJointTrajectoryValidity?

• GetJointTrajectoryValidity

  • what does CHECK_FULL_TRAJECTORY mean?

• GetObjectsInCollisionMap

  • I thought the term CollisionMap refers *strictly* to points

  • here it seems to have a more general meaning (as it semantically should)

  • I still find it very confusing ( --> CollisionGrid, pretty pretty please?!?)

  • or at least change the name of this service to GetObjectsInCollisionXXX
• Parameters
  • collision_map
    • either the documentation for this is mismatched, or the name is terribly misleading
• Unrelated note

  • in the .srv autogenerated documentation, clicking on a message (that is part of a service) name fails because it looks for another service with that name

Meeting agenda

To be filled out by proposer based on comments gathered during API review period

Conclusion

Package status change mark change manifest)

• Action items that need to be taken.

• Major issues that need to be resolved

• refresh_interval_*_* change to collision_map_safety_timeout (DONE)
• Move yaml configuration to another page (DONE)
• Take out show_collisions - contacts are broadcast by default when computed - topic name should have "contacts" in it (DONE)
• compute_contacts - check the pipeline for this and how it is enforced currently (DONE)
• Inside the node, resolveName for robot_description (DONE)
• Documentation for remapping robot_description (DONE)
• Get rid of allow_valid_collisions (DONE)
• Update other refresh_interval_* to *_safety_timeout - (DONE - updated refresh_interval_pose to tf_safety_timeout)
• Get rid of refresh_interval_pose.. (DONE - updated refresh_interval_pose to tf_safety_timeout)
• Summary at the top of the documentation specifying what this node does and how it does it - read in a paragraph what this thing does - update (DONE)

• SetConstraints will go away because all constraints are now specified in service calls (DONE - SetConstraints is now specified as a service only for TESTING/DEBUGGING)

• Update move_arm page and take out GetExecutionSafety

• Note how these two services differ on the Wiki
• planning_environment_msgs API change - convert bits to bools (DONE)

• GetObjectsInCollisionMap is now GetCollisionObjects (CHECKED)

• "collision_map" parameter - typo ?? (DONE)
• check parameter documentation for "group_name" (DONE)
• all advertised services should be in the right namespace (CHECKED)