Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GetJointTransmittedWrench feature #283

Merged
merged 3 commits into from
Sep 21, 2021
Merged

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Aug 20, 2021

馃帀 New feature

Relates to #16, #124, and #143

Summary

This adds a new feature that exposes the transmitted forces on a joint, which are needed for creating a Force/Torque sensor. The computation of the wrench is similar to what's done in Gazebo-classic, but in Gazebo-classic, the wrench is expressed in the child body frame, but applied at the joint origin. Here, the wrench is expressed in the joint frame and applied at the joint origin.

This also adds a new quantity Wrench, which is a stacking of Angular and Linear vectors.

TODO

  • Create a RelativeQuantity that corresponds to Wrench
  • [ ] Ensure that applied force is not included in the computed wrench.

Test it

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

@codecov
Copy link

codecov bot commented Aug 20, 2021

Codecov Report

Merging #283 (42a5613) into main (45b52a4) will increase coverage by 0.02%.
The diff coverage is 65.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #283      +/-   ##
==========================================
+ Coverage   75.52%   75.54%   +0.02%     
==========================================
  Files         127      127              
  Lines        5597     5655      +58     
==========================================
+ Hits         4227     4272      +45     
- Misses       1370     1383      +13     
Impacted Files Coverage 螖
include/ignition/physics/Geometry.hh 100.00% <酶> (酶)
...nclude/ignition/physics/detail/RelativeQuantity.hh 91.86% <54.83%> (-8.14%) 猬囷笍
dartsim/src/JointFeatures.cc 64.83% <73.33%> (+1.26%) 猬嗭笍
include/ignition/physics/detail/Joint.hh 100.00% <100.00%> (酶)
include/ignition/physics/detail/GetEntities.hh 100.00% <0.00%> (酶)
dartsim/src/EntityManagementFeatures.cc 78.47% <0.00%> (+0.99%) 猬嗭笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 45b52a4...42a5613. Read the comment docs.

@@ -56,6 +56,10 @@ namespace ignition
using AngularVector = Vector<Scalar, (Dim*(Dim-1))/2>;
IGN_PHYSICS_MAKE_ALL_TYPE_COMBOS(AngularVector)

template <typename Scalar, std::size_t Dim>
using Wrench = Vector<Scalar, Dim + (Dim*(Dim-1))/2>;
IGN_PHYSICS_MAKE_ALL_TYPE_COMBOS(Wrench)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there an advantage to using a single vector over std::pair<LinearVector, AngularVector> or a struct? I would have trouble remembering if the linear or angular part of the wrench came first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to a struct in 1bd5c00

@chapulina chapulina moved this from Inbox to In progress in Core development Aug 23, 2021
@traversaro
Copy link
Contributor

This adds a new feature that exposes the constraint forces on a joint, which are needed for creating a Force/Torque sensor.

Just to understand, why is this needed to create a Force/Torque sensor? Most simulators already directly expose the joint force/torque that the child joint is applying to the parent joint, already including the constraint forces, the applied forces and anything else that could be necessary. I am probably missing something, but I don't see the advantage of computing/reading from the physics engine the "motor" joint force, the constraint force/torque and then adding them back at high level when most physics engines already provide directly this info.

@azeey
Copy link
Contributor Author

azeey commented Aug 27, 2021

The current API Joint::GetForce returns a force in the DOF of the joint (even though it's currently not working correctly for dartsim as mentioned in #143). It would not give you the force in the constraint directions. Aside from that, I think it's beneficial to have a separate Feature for this because it allows physics engines that don't expose their constraint forces to opt out of the feature.

When thinking of joint force/torque sensing, two applications come to mind: force/impedance control and 6 axis Force/Torque sensor. For the first one, for example we'd use revolute joints and would want to know the "measured" joint torque in the DOF of the joint. For this, I think the Joint::GetForce API makes sense. For the second application, we'd typically use a fixed joint and measure the constraint forces and torques in directions. This is where we'd use Joint::GetConstraintWrench.

I am probably missing something, but I don't see the advantage of computing/reading from the physics engine the "motor" joint force, the constraint force/torque and then adding them back at high level when most physics engines already provide directly this info.

I'm not sure I understand when you say "adding them back at high level". Are you saying users would want the two results added together?

@traversaro
Copy link
Contributor

I am probably missing something, but I don't see the advantage of computing/reading from the physics engine the "motor" joint force, the constraint force/torque and then adding them back at high level when most physics engines already provide directly this info.

I'm not sure I understand when you say "adding them back at high level". Are you saying users would want the two results added together?

Sorry, I definitely was not clear. If we want to implement the same semantics described in the last section of http://gazebosim.org/tutorials?tut=force_torque_sensor&cat=sensors, the FT sensor that is mounted so close to the revolute joint so that it is possible to ignore the flange and avoid to model it as a fixed joint, and can be modeled directly as a FT sensor on the revolute joint, is able to read the Force/Torque that is caused by two causes:

If you need to simulate a FT sensor attached to a revolute joint, you need to sum this two contributions. However, I think that alternativly we can just expose getBodyForce that is a quantity that we are looking for (and that for fixed joints should match getBodyForce), so that no higher level processing is necessary. Furthermore, that quantity is a byproduct of most forward dynamics algorithms, so most physics engine should expose it.

@azeey azeey changed the title Add GetJointConstraintWrench feature Add GetJointTransmittedWrench feature Sep 4, 2021
@azeey azeey marked this pull request as ready for review September 4, 2021 00:04
@azeey azeey requested a review from mxgrey as a code owner September 4, 2021 00:04
@azeey
Copy link
Contributor Author

azeey commented Sep 4, 2021

I've used the term TransmittedWrench in this PR pere @traversaro's suggestion, but I'm open to suggestions if that doesn't seem like a suitable name.

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still reviewing the test but have some suggestions for the API documentation in the meantime

include/ignition/physics/Joint.hh Outdated Show resolved Hide resolved
include/ignition/physics/Joint.hh Outdated Show resolved Hide resolved
include/ignition/physics/Joint.hh Outdated Show resolved Hide resolved
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 20, 2021
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey azeey changed the base branch from ign-physics2 to main September 20, 2021 22:39
@azeey
Copy link
Contributor Author

azeey commented Sep 20, 2021

Retargetted to main. @mjcarroll PTAL

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still reviewing the test, will get back to it after my next meeting

<diffuse>0.8 0.8 0.8 1</diffuse>
</material>
</visual>
<!-- Using 4 spheres as contact points to ensure stable constraint force readings -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect 3 spheres to be more stable than 4 with planar contact, but this is fine

dartsim/src/JointFeatures_TEST.cc Outdated Show resolved Hide resolved
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the sign convention and some test comments in 7bd1556

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Member

I updated the sign convention and some test comments in 7bd1556

one final set of fixes in 42a5613

Core development automation moved this from In progress to In review Sep 21, 2021
@azeey azeey added 馃彲 fortress Ignition Fortress and removed 馃彴 citadel Ignition Citadel labels Sep 21, 2021
@azeey
Copy link
Contributor Author

azeey commented Sep 21, 2021

Thanks for those fixes @scpeters!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 馃彲 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants