-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixup JointStiffnessController to output actuation instead of generalized_force #22329
base: master
Are you sure you want to change the base?
Fixup JointStiffnessController to output actuation instead of generalized_force #22329
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of order platform (still need feature review)
Q: in #22315 you retained a generalized_force port in addition to the actuation port. Here you are replacing the generalized_force port with actuation. Should these be treated identically? Also with this change I believe the release notes should say "breaking change" since the deprecated port has changed behavior.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform)
systems/controllers/joint_stiffness_controller.h
line 27 at r1 (raw file):
* and `τ_app` contains applied forces from force elements added to the * multibody model (this can include damping, springs, etc. See * MultibodyPlant::CalcForceElementsContribution()). B⁻¹ is the inverse of the
nit: inverse (or pseudoinverse if underactuated)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argument for also exposing generalized_force in the inversedynamicscontroller was because it can be useful in different workflows (like actuating a floating hand, which is a pain to add actuators for). i couldn't imagine any similar motivation here... it's joint stiffness control, afterall. So I intentionally made a different decision here.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform)
systems/controllers/joint_stiffness_controller.h
line 27 at r1 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
nit: inverse (or pseudoinverse if underactuated)
the same block of texts says this class assumes the plant is fully-actuated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: LGTM missing from assignee sammy-tri(platform)
systems/controllers/joint_stiffness_controller.h
line 27 at r1 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
the same block of texts says this class assumes the plant is fully-actuated.
Oh, I didn't read the part that hadn't changed -- sorry! All good.
I agree with the reasoning for deprecating the However, @sherm1 with your platform hat on, please help us defend our promise to uses:
-- https://drake.mit.edu/stable.html Breaking changes should be "very rare". Any PR which is a breaking change needs stiff scrutiny from the platform reviewer. For any breaking change, the platform reviewer should ask the author to explain their rationale for not doing a more gentle deprecation. If there is a good reason, that's fine -- we just want it written down as part of the review. In this case, it seems to me like we can copy-paste the old calc method for the deprecated output port to serve as the deprecated calc's implementation, and so it seems to me like introducing a non-breaking deprecation would be practical and sound. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point, Jeremy. WDYT Russ?
Reviewable status: LGTM missing from assignee sammy-tri(platform)
…ized_force Follows RobotLocomotion#22315, which implemented the same fix for InverseDynamicsController. Previously there was a tacit assumption that the actuators were declared in the same order as the joints. This PR resolves that, and (to make this clear) renames the output port from `generalized_force` to `actuation`.
ce41bfe
to
a215b19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I agree that's better (and had considered doing it in the first place).
Reviewable status: LGTM missing from assignee sammy-tri(platform)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee sammy-tri(platform)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 7 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion
systems/controllers/joint_stiffness_controller.h
line 159 at r2 (raw file):
// Calculator for the deprecated output port. void CalcOutputForce(const Context<T>& context, BasicVector<T>* force) const;
nit: odd indentation
Follows #22315, which implemented the same fix for InverseDynamicsController.
Previously there was a tacit assumption that the actuators were declared in the same order as the joints. This PR resolves that, and (to make this clear) renames the output port from
generalized_force
toactuation
.+@sammy-tri for feature review, please?
+@sherm1 for platform review, please?
This change is