personalrobotics / prpy

Python utilities used by the Personal Robotics Laboratory.
BSD 3-Clause "New" or "Revised" License
62 stars 19 forks source link

Bugfix/ada trajdescription error #257

Closed sjavdani closed 8 years ago

sjavdani commented 8 years ago

When executing trajectories for ada, the execution occurs, but the following error is thrown: "AttributeError: 'JointTrajectory' object has no attribute 'GetDescription'"

Seems to be a problem with exec_traj (line 414) not having a description. this error is thrown on the following line in util.py: 294 description = traj.GetDescription() Returning the other trajectory traj doesn't produce the same error

Note that traj is of class 'openravepy.openravepy.openravepy_int.Trajectory', while exec_traj is class 'trajectory_msgs.msg._JointTrajectory.JointTrajectory'

psigen commented 8 years ago

sigh Please see the related thread: https://github.com/personalrobotics/ada/issues/43#issuecomment-150868573

This has come up several times, but no one seems to have resolved it. The underlying issue is in this line: https://github.com/personalrobotics/ada/blob/master/adapy/src/adapy/adarobot.py#L205

The result of ExecuteTrajectory is supposed to be an OpenRAVE trajectory, but this code is returning a ROS JointTrajectory, which is totally different. Thus, the subsequent code gets messed up, because it can't do any of the usual postprocessing like setting Tags.

Someone needs to go in and convert the return of ExecuteTrajectory from ROS trajectories back to OpenRAVE trajectories. I'm not really sure how this code was originally intended to work in the first place.

psigen commented 8 years ago

@sjavdani @mkoval: I think it's preferable to fix the underlying issue of adapy returning a completely incorrect type instead of removing the working trajectory tags functions from prpy, right?

mkoval commented 8 years ago

@psigen is right. The correct place to change this is inside the ExecuteTrajectory function in AdaPy. We convert the OpenRAVE trajectory to a ROS trajectory, execute it, and (optionally) use that return value to wait for the trajectory to finish execution. You should modify that function to return the original OpenRAVE trajectory, instead of the ROS trajectory.

sjavdani commented 8 years ago

Alright, I see the "quickfix" mike suggested in the other thread. I'll try that next time I'm in lab and submit if it works.

Is there a better fix you might suggest, or is the suggested quickfix the right fix anyway?

psigen commented 8 years ago

Thanks @sjavdani!

@mkoval's quickfix is alright, but the correct thing to do would be to actually convert the returned ROS JointTrajectory into an OpenRAVE trajectory.

Each JointTrajectoryPoint has a list of positions / velocities / accelerations per joint that become the waypoints in an OpenRAVE trajectory. By diff-ing the time_from_start fields, you can get the delta_time that OpenRAVE uses.

This isn't very hard to do, but it's little bit fiddly mostly due to OpenRAVE's ConfigurationSpecification. @mkoval or @stefanos might have code to do this in a utility module somewhere: if not, there probably should be since OpenRAVE Trajectory <-> ROS Joint Trajectory is a very useful conversion.

It should ideally be possible to make a pair of utility functions with the signatures: openrave_from_ros_trajectory(traj_ros, robot) ros_from_openrave_trajectory(traj_or, robot)

mkoval commented 8 years ago

The ros_control joint trajectory controller does not currently send feedback via actionlib, which is how we reconstruct the executed trajectory in Python (see https://github.com/ros-controls/ros_controllers/issues/173). It currently only publishes this data on a controller status topic.

I don't think it is worth adding a workaround for this - we'd be much better off modifying the controller to publish the data in the right place. This is non-trivial because it requires adding a realtime-safe method of publishing feedback to realtime_tools.

I suggest you go with the quick fix for now. We can revisit it once we have the controller publishing feedback via actionlib.

sjavdani commented 8 years ago

The suggested quick fix works. Therefore, I'll close this pull request since it is wrong/unnecessary.

psigen commented 8 years ago

Thanks for spending the time to look into the underlying issue!