Skip to content
This repository has been archived by the owner on Nov 13, 2017. It is now read-only.

FakeController: publish all via points of trajectory in real time #20

Closed
wants to merge 1 commit into from

Conversation

rhaschke
Copy link
Contributor

Current implementation of FakeController only published the last point of a trajectory, that is jumping to the end in one step. At least for debugging of #713 and #716, I found it useful to see the trajectory being traversed by the scene robot also in demo mode, which is what this PR implements.

If the old behavior has some specific value to anybody, we could also go for two fake controllers side by side, e.g. a new ViaPointController or implementing the old behavior as WarpController. By adapting controllers.yaml, one would need to switch between them.

What is your opinion?

@rhaschke
Copy link
Contributor Author

Probably for benchmarking, we want to have the warping behavior.

@v4hn
Copy link
Contributor

v4hn commented Jul 15, 2016 via email

@davetcoleman
Copy link
Member

+1 to this version of a fake controller, see moveit/moveit_ros#708 (comment)

@rhaschke
Copy link
Contributor Author

Don't yet merge this. I'm thinking of several alternative fake controllers to choose from.
For benchmarking, the existing one (warping to the target) make sense.
For debugging, the via-point controller makes sense.
For nice visualization, the one from #708 makes sense.

I will re-open the PR if ready.

@rhaschke rhaschke closed this Jul 18, 2016
@v4hn
Copy link
Contributor

v4hn commented Jul 18, 2016

Don't confuse me like that by closing a request I'm currently reading. XD
If you want to support all three modes of operation, I would propose to have three different type entries aka FakeWarpController, FakeTrajectoryController, and FakeInterpolatedTrajectoryController.

One point I'm also very much interested in for the simulated controllers is a default configuration on startup.
We have one setup where the simulated robotic arm always starts up colliding with a wall..

So what do we do with #706 ? Merge it there as an alternative?
Close it and wait for your reimplementation here?
Merge a version of it into moveit_plugins to work with the fake trajectory controller you plan to write?

@rhaschke
Copy link
Contributor Author

Don't confuse me like that by closing a request I'm currently reading. XD
If you want to support all three modes of operation, I would propose
to have three different |type| entries aka |FakeWarpController|,
|FakeTrajectoryController|, and |FakeInterpolatedTrajectoryController|.

This is what I planned.

One point I'm also very much interested in for the simulated
controllers is a default configuration on startup.
We have one setup where the simulated robotic arm always starts up
colliding with a wall..

Good idea. But where do you want to specify this? I suggest to use an
appropriately named default pose, whose name is a private rosparam of
the controller node?

@davetcoleman
Copy link
Member

I suggest to use an appropriately named default pose, whose name is a private rosparam of the controller node?

That is how my moveit_sim_controller currently works

I would propose to have three different |type| entries aka |FakeWarpController|, |FakeTrajectoryController|, and |FakeInterpolatedTrajectoryController|.

Is there a good use case for all three of these? Seems like FakeWarpController is simply a bad/quick implementation @isucan whipped up that could benefit from interpolation. Not sure the difference between the other two controllers you are listing.

Off topic side note: I hate that the ControllerManager is not named ControllerInterface, since that is actually what it is and things like ros_control are the ControllerManager

@rhaschke
Copy link
Contributor Author

As I have written:

For benchmarking, the existing one (warping to the target) make sense - because its fast.
For debugging, the via-point controller makes sense - you can see which via points were used.
For nice visualization, the one from #708 makes sense - getting smooth interpolation.

Do you suggest to rename ControllerManager to ControllerInterface? I found the name clash with ros_control confusing too.

@v4hn
Copy link
Contributor

v4hn commented Jul 18, 2016

+1 to ControllerManager being confusing. I do not believe this alone justifies a renaming though.
I might be wrong here, but one could use the module as a "ControllerManager", right?
It only happens to be the case that most implementations only interface between move_group and external action servers (or do not provide a proper controller at all).
ros_control does its job quite well, so we could give up on that concept within MoveIt entirely
and discourage people from trying to build controllers in MoveIt.
This, together with the name conflict, might justify renaming the interface.

@davetcoleman
Copy link
Member

I've never seriously put the suggestion forward because it seems like a big breaking change, but with recent momentum I guess we could

I might be wrong here, but one could use the module as a "ControllerManager", right?

Yes, there is a switchControllers(), getActiveControllers() etc. - good point.

ros_control does its job quite well, so we could give up on that concept within MoveIt entirely

I never thought of it that way, but I really like that idea. @isucan himself has said that MoveIt! tries to tackle too many things, I think it has suffered from some scope creep.

@rhaschke rhaschke deleted the fake-controller-traverse-via-points branch July 19, 2016 23:59
@rhaschke rhaschke mentioned this pull request Jul 21, 2016
@v4hn
Copy link
Contributor

v4hn commented Jul 21, 2016

On Tue, Jul 19, 2016 at 04:13:23AM -0700, Dave Coleman wrote:

I've never seriously put the suggestion forward because it seems like a big breaking change, but with recent momentum I guess we could

I do believe we could do this in kinetic

@davetcoleman
Copy link
Member

I've turned this into an issue / proposal: moveit/moveit_ros#725

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants