search

pal-robotics / play_motion

A tool to play pre-recorded motions on a robot using ros_control
8 stars 23 forks source link

MoveIt! integration #36

Closed adolfo-rt closed 10 years ago

adolfo-rt commented 10 years ago

Unit tests build but are currently not passing.

po1 commented 10 years ago

This means that the devel job will start to fail, which is not a problem as long as we fix these unit tests promptly. How long do you think it will take to adapt the unit tests to the new interface?

adolfo-rt commented 10 years ago

One or two days of work. I need to create a rrbot moveit configuration and launch it. Then adapt existing tests a bit, i.e., review the checked conditions in some of them, and possibly force no planning.

The alternative for now would be to not build the tests altogether.

po1 commented 10 years ago

Could we not just force no planning for the time being? Is the behavior so different?

adolfo-rt commented 10 years ago

Could we not just force no planning for the time being? Is the behavior so different?

Even if a particular motion is requested without planning, play_motion must be ready to take requests with planning. To do this, when constructing a play_motion server, we instantiate some moveit-specific members, which is slow (few seconds), hence can't be done on demand, for each request. As things are now, play_motion won't even initialize if the moveit configuration is absent.

po1 commented 10 years ago

@adolfo-rt please update this branch with the possibility to work without a planner (only with skip_planning: True) The required modifications to the existing unit tests should be fairly small. It would be great if we could not break them in this PR.

adolfo-rt commented 10 years ago

Should be fine now. Units tests build cleanly and pass.

po1 commented 10 years ago

Also, a sample configuration file with comments, units and default values for approach_planner would be cool

adolfo-rt commented 10 years ago

Also, a sample configuration file with comments, units and default values for approach_planner would be cool

I could add it in the ros wiki doc. Sounds good?.

po1 commented 10 years ago

I could add it in the ros wiki doc. Sounds good?.

Since there already is a sample.yaml, I'd rather have it updated. That doesn't mean I don't want to put all of it in the wiki. It is just way simpler to copy a config file than clumsily copy and pasting from a wiki page.

adolfo-rt commented 10 years ago

How will this work if planning_disabled is not explicitly set and no MoveIt! configuration exists for the robot?

If the required play_motion part of the configuration is missing (e.g., which planning groups to use, and in which order), an exception is raised, and the node will die and issue a ROS_FATAL statement.

If the play_motion config is fine, but the MoveIt! side of things is absent, play_motion will not initialize. Currently it'll keep on waiting indefinitely for the move_group server to come up.

po1 commented 10 years ago

This means that configurations that were working before will stop working after merging this. It is fine as long as we provide some documentation. I would add a line about this in the README.md

po1 commented 10 years ago

@adolfo-rt I will merge this PR since the unit tests pass. For the little details, let's open an issue.

adolfo-rt commented 10 years ago

Please review again

po1 commented 10 years ago

Looks good, thank you very much for the effort. Merging.