Just the interface for a combined gripper+manipulator. #11320
Conversation
There was a problem hiding this comment.
+@RussTedrake (in case you don't still have my email hanging around).
Reviewable status: LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake)
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @edrumwri and @RussTedrake)
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 26 at r1 (raw file):
* An abstract class for adding a combined manipulator/gripper model and its * controller to a Diagram and querying aspects of the model (e.g., the number * of manipulator generalized positions).
it would help to see how you propose to use this in concert with the current manipulation station. it's hard for me to see her if it actually makes things better.
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 29 at r1 (raw file):
*/ template <typename T> class CombinedManipulatorAndGripperModel {
Manipulator is the wrong word here. (for me, manipulator implies arm + gripped by itself). I think ArmAndGripper might be better?
The implication here is that you've generalized this beyond IIWA, but you don't take the urdf/sdf anywhere here? (the current manipulation station interface already supports multiple urdfs... e.g. iiwa7 and iiwa14.)
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake)
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 26 at r1 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
it would help to see how you propose to use this in concert with the current manipulation station. it's hard for me to see her if it actually makes things better.
I'm sorry to hear that- I was hoping this small PR would be enough for you to get the idea without you needing to digest the entire branch (here: https://github.com/edrumwri/drake/tree/manipulation_station_refactor). The branch is unpolished but shows how it all fits together.
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 29 at r1 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Manipulator is the wrong word here. (for me, manipulator implies arm + gripped by itself). I think ArmAndGripper might be better?
The implication here is that you've generalized this beyond IIWA, but you don't take the urdf/sdf anywhere here? (the current manipulation station interface already supports multiple urdfs... e.g. iiwa7 and iiwa14.)
Hah! Arm and manipulator have the opposite meanings to me. But that's fine- no problem changing that.
The idea is generally to have a separate derived class for each (arm)manipulator/gripper combination, which is responsible for its own urdf/sdf loading. The constructor for the Iiwa/Schunk combo would permit passing in a flag indicating whether the Iiwa7 or 14 were to be used.
There was a problem hiding this comment.
took a look, and I think it looks fine. @EricCousineau-TRI @kmuhlrad -- please weigh in if you have any comments/concerns?
Reviewable status: 2 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @RussTedrake)
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @edrumwri and @RussTedrake)
a discussion (no related file):
First pass done; PTAL.
Basically, would like to see as few virtual methods as possible if this already requires MultibodyPlant as its interface.
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 29 at r1 (raw file):
Previously, edrumwri (Evan Drumwright) wrote…
Hah! Arm and manipulator have the opposite meanings to me. But that's fine- no problem changing that.
The idea is generally to have a separate derived class for each (arm)manipulator/gripper combination, which is responsible for its own urdf/sdf loading. The constructor for the Iiwa/Schunk combo would permit passing in a flag indicating whether the Iiwa7 or 14 were to be used.
FTR, I'd agree with Russ's term of art; had incorporated in this little sdformat PR ;)
https://bitbucket.org/osrf/sdf_tutorials/src/f1c91186b5a38bc2092cde20513d0f4a73fd8315/nesting/proposal.md?at=default
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 31 at r1 (raw file):
class CombinedManipulatorAndGripperModel { public: explicit CombinedManipulatorAndGripperModel(
Rather than virtualize the instances, why not pass them here?
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 42 at r1 (raw file):
/// Gets the manipulator generalized positions. virtual VectorX<T> GetManipulatorPositions(
nit Can I ask why this (and other instance-fixed methods) have to be virtual?
If they're all going to use the constituent instance ids, then this doesn't have to be virtual?
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 96 at r1 (raw file):
/// This method adds the manipulator and gripper models to the internal plant. virtual void AddRobotModelToMultibodyPlant() = 0;
BTW Seeing the need for this makes me a bit sad... Kinda makes me wish we had subtree clones, something abstractly like this:
https://www.mathworks.com/help/robotics/ref/robotics.rigidbodytree.subtree.html
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 96 at r1 (raw file):
/// This method adds the manipulator and gripper models to the internal plant. virtual void AddRobotModelToMultibodyPlant() = 0;
nit What about fixturing semantics?
Should this return the arm model instance, or something to intelligently inform welding?
There was a problem hiding this comment.
Besides the language the interface looks good to me.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @edrumwri and @RussTedrake)
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 26 at r1 (raw file):
Previously, edrumwri (Evan Drumwright) wrote…
I'm sorry to hear that- I was hoping this small PR would be enough for you to get the idea without you needing to digest the entire branch (here: https://github.com/edrumwri/drake/tree/manipulation_station_refactor). The branch is unpolished but shows how it all fits together.
Thanks, this branch was helpful
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 29 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
FTR, I'd agree with Russ's term of art; had incorporated in this little sdformat PR ;)
https://bitbucket.org/osrf/sdf_tutorials/src/f1c91186b5a38bc2092cde20513d0f4a73fd8315/nesting/proposal.md?at=default
+1 for language change, I was confused by the use of Manipulator at first.
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @edrumwri, @EricCousineau-TRI, and @RussTedrake)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
First pass done; PTAL.
Basically, would like to see as few virtual methods as possible if this already requiresMultibodyPlantas its interface.
@EricCousineau-TRI this is an excellent question. Please look at my response below and see what you think.
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 26 at r1 (raw file):
Previously, kmuhlrad (Katy Muhlrad) wrote…
Thanks, this branch was helpful
Done.
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 29 at r1 (raw file):
Previously, kmuhlrad (Katy Muhlrad) wrote…
+1 for language change, I was confused by the use of Manipulator at first.
Done.
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 42 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Can I ask why this (and other instance-fixed methods) have to be virtual?
If they're all going to use the constituent instance ids, then this doesn't have to be virtual?
This decision emanated from how the gripper position is (currently) modeled with two joints but a single DoF. The concrete class currently must have some knowledge about how to interpret and use the single DoF for the gripper that is transmitted by the ManipStation GUI. I could imagine an arm having similar problems.
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 96 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit What about fixturing semantics?
Should this return the arm model instance, or something to intelligently inform welding?
Arm and gripper instances could be returned but I don't think the welding can happen in the station itself.
There was a problem hiding this comment.
Posted a very strong opinion against coupling and all that jazz; PTAL.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @amcastro-tri, @edrumwri, @EricCousineau-TRI, @hongkai-dai, @rcory, and @RussTedrake)
examples/manipulation_station/combined_arm_and_gripper_model.h, line 53 at r2 (raw file):
/// Sets the arm generalized positions. virtual void SetArmPositions( const systems::Context<T>& diagram_context,
Why diagram_context instead of plant_context? This means this could only ever interface with this diagram's stuff; also, does this set the commanded positions, or the physical positions? (Through this API, it has access to everything, so it could scribble over any part.)
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 42 at r1 (raw file):
Previously, edrumwri (Evan Drumwright) wrote…
This decision emanated from how the gripper position is (currently) modeled with two joints but a single DoF. The concrete class currently must have some knowledge about how to interpret and use the single DoF for the gripper that is transmitted by the ManipStation GUI. I could imagine an arm having similar problems.
I'm a bit wary about this, as it feels like scope creep for the base class.
That means that this interface wants to take responsibility for closed-loop kinematics / dynamics, even though it openly exposes MultibodyPlant, which should accommodate that at some point in the future. A major concern I have
is that this interface will explode the moment these components are used for
more non-trivial tasks, like optimization. (Mayhaps for @rcory and
@hongkai-dai's analyses?)
I'd strongly suggest that the following interface, in the aims of being minimal
and orthogonal to existing classes:
- Since this API heavily relies on
MultibodyPlantpublicly, get rid of all
sugary forwarding methods. Please only exposearm_instanceand
gripper_instance; do not forward{Get|Set}{Positions|Velocities}, etc. - The only virtual methods this exposes are
CommandVirtualGripper(qd, vd),
GetVirtualGripperState(q, v), andSetVirtualGripperState(q, v); the only
concrete use case I know of, where this is useful, is for the Schunk gripper.qandvcould be non-scalar, but there should be additional methods
which indicate how this mapping is made, esp. when computing Jacobians
w.r.t. the realq, vand "virtual"q, v, dynamics, etc.- Any other formulations of grippers should really be handled by
closed-loop joints / holonomic constraints inMultibodyPlant, rather than
through this interface.- To be clear, I'd advocate first for simple holonomic constraints:
phi(q) = q_gripper_left + q_gripper_right == 0- not the old
incantation that we had, which I believe was physically realistic
but very complex (and slow) closed-loop structure.
- To be clear, I'd advocate first for simple holonomic constraints:
I know @amcastro-tri has a lot on his plate, so holonomic constraints /
closed-loop joints may not come in soon, but I'd strongly recommend against
over-abusing this interface as a band-aid to that problem.
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 96 at r1 (raw file):
Previously, edrumwri (Evan Drumwright) wrote…
Arm and gripper instances could be returned but I don't think the welding can happen in the station itself.
What state would this leave the arm's base link in?
Would this leave it floating, or would it weld it to "remove" the floating-body coordinates?
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @RussTedrake)
examples/manipulation_station/combined_arm_and_gripper_model.h, line 53 at r2 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Why
diagram_contextinstead ofplant_context? This means this could only ever interface with this diagram's stuff; also, does this set the commanded positions, or the physical positions? (Through this API, it has access to everything, so it could scribble over any part.)
Done.
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 31 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Rather than virtualize the instances, why not pass them here?
You mean that the manipulation station itself loads in the models and passes them to this class?
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 42 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I'm a bit wary about this, as it feels like scope creep for the base class.
That means that this interface wants to take responsibility for closed-loop kinematics / dynamics, even though it openly exposes
MultibodyPlant, which should accommodate that at some point in the future. A major concern I have
is that this interface will explode the moment these components are used for
more non-trivial tasks, like optimization. (Mayhaps for @rcory and
@hongkai-dai's analyses?)I'd strongly suggest that the following interface, in the aims of being minimal
and orthogonal to existing classes:
- Since this API heavily relies on
MultibodyPlantpublicly, get rid of all
sugary forwarding methods. Please only exposearm_instanceand
gripper_instance; do not forward{Get|Set}{Positions|Velocities}, etc.- The only virtual methods this exposes are
CommandVirtualGripper(qd, vd),
GetVirtualGripperState(q, v), andSetVirtualGripperState(q, v); the only
concrete use case I know of, where this is useful, is for the Schunk gripper.
qandvcould be non-scalar, but there should be additional methods
which indicate how this mapping is made, esp. when computing Jacobians
w.r.t. the realq, vand "virtual"q, v, dynamics, etc.- Any other formulations of grippers should really be handled by
closed-loop joints / holonomic constraints inMultibodyPlant, rather than
through this interface.
- To be clear, I'd advocate first for simple holonomic constraints:
phi(q) = q_gripper_left + q_gripper_right == 0- not the old
incantation that we had, which I believe was physically realistic
but very complex (and slow) closed-loop structure.I know @amcastro-tri has a lot on his plate, so holonomic constraints /
closed-loop joints may not come in soon, but I'd strongly recommend against
over-abusing this interface as a band-aid to that problem.
Done, mostly. I think we need a "default open" configuration for the gripper, it is useful (ATM) to separate the set/get positions/velocities methods for the gripper, and all commands- for gripper and arm- will be sent through exposed controller ports.
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 96 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
What state would this leave the arm's base link in?
Would this leave it floating, or would it weld it to "remove" the floating-body coordinates?
I've affixed it to the environment in my derived class and it would make sense to do so if this class remains specific to the manipulation station.
If there is a dream to extend this class beyond the manipulation station, it might make sense to let the station do the welding (though the station would then need some info about the manipulator that does not seem straightforward to provide, namely how should this particular manipulator be welded to a particular environment). I'd argue for deferring that discussion and just clarifying in the docs that this method welds the arm to the world using some pose that works well for the robot + the manipulation station.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @edrumwri, @EricCousineau-TRI, and @RussTedrake)
a discussion (no related file):
Previously, edrumwri (Evan Drumwright) wrote…
@EricCousineau-TRI this is an excellent question. Please look at my response below and see what you think.
OK I like the down-scoping! Posted some more questions, but redacting this design objection since it's satisfied.
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 31 at r1 (raw file):
Previously, edrumwri (Evan Drumwright) wrote…
You mean that the manipulation station itself loads in the models and passes them to this class?
Yes. Or rather, I have no idea what state the incoming `plant is in
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 42 at r1 (raw file):
Previously, edrumwri (Evan Drumwright) wrote…
Done, mostly. I think we need a "default open" configuration for the gripper, it is useful (ATM) to separate the set/get positions/velocities methods for the gripper, and all commands- for gripper and arm- will be sent through exposed controller ports.
Thanks! And that sounds good on ports and stuff, but what about the controller's initial conditions? Who is responsible for that? That should be documented.
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 96 at r1 (raw file):
Previously, edrumwri (Evan Drumwright) wrote…
I've affixed it to the environment in my derived class and it would make sense to do so if this class remains specific to the manipulation station.
If there is a dream to extend this class beyond the manipulation station, it might make sense to let the station do the welding (though the station would then need some info about the manipulator that does not seem straightforward to provide, namely how should this particular manipulator be welded to a particular environment). I'd argue for deferring that discussion and just clarifying in the docs that this method welds the arm to the world using some pose that works well for the robot + the manipulation station.
I am totes a fan of deferring that API complexity, and putting this clarification in the docs.
That being said, can you update the docs to state that the override must affix the plant somewhere?
Also, upon second reading, why is this method even public?
What workflow needs this? Why can't this be done in the downstream constructor, and why can't the appropriate model instances be passed to this base constructor?
(At present, having all these virtual methods still feels very messy.)
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @edrumwri and @RussTedrake)
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 31 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Yes. Or rather, I have no idea what state the incoming `plant is in
To clarify: By "state", I have no idea if this plant is empty, contains the environment + arm + gripper, or just the environment, with or without the manipulands.
Will this finalize the plant? Must the plant be registered with a SceneGraph? Should it be connected to the SceneGraph in a DiagramBuilder?
Can you elaborate on this in this constructor, using @pre statements or what not?
examples/manipulation_station/combined_manipulator_and_gripper_model.h, line 96 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I am totes a fan of deferring that API complexity, and putting this clarification in the docs.
That being said, can you update the docs to state that the override must affix the plant somewhere?Also, upon second reading, why is this method even public?
What workflow needs this? Why can't this be done in the downstream constructor, and why can't the appropriate model instances be passed to this base constructor?
(At present, having all these virtual methods still feels very messy.)
To clarify, this would be better illustrated by addressing the above comment about the contract on the incoming plant and its incoming state.
|
Happy New Year, and thanks for your interest in contributing to Drake! Since this PR has not received any updates over the past 6+ months, we are closing it automatically. To resume the PR again, you may reopen it with updates, or open a new one. |
Combinations of grippers and manipulators would implement this abstract class.
This change is