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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Populate JointTransmittedWrench from physics #989

Merged
merged 3 commits into from
Sep 22, 2021

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Aug 20, 2021

🎉 New feature

Summary

This populates joint constraint wrenches from ign-physics. This is needed to implement Force/Torque sensors.

Needs gazebosim/gz-physics#283

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

@azeey azeey requested a review from chawladevansh August 20, 2021 21:10
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Aug 20, 2021
@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Aug 23, 2021
@chapulina
Copy link
Contributor

@azeey , can you comment on the relation between this PR and:

@azeey
Copy link
Contributor Author

azeey commented Aug 23, 2021

#952 populates forces in the DOF of the joint whereas this PR populates the constraint (non-DOF direction) wrench on the joint. This wrench will not (hopefully) include the commanded force in the DOF of the joint. #952 need gazebosim/gz-physics#143, which we're still trying to resolve.

#977 I believe is the ign-gazebo counter part to the ign-sensors Force/Torque sensor.

@chapulina
Copy link
Contributor

Thanks, so do both this PR and #952 work together, or do we have to choose one or the other?

And I see some overlap between #977 and this one. Is that a superset of this?

@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 20, 2021
@azeey azeey changed the base branch from ign-gazebo3 to main September 20, 2021 22:47
@azeey azeey force-pushed the joint_constraint_wrench branch from 1eaf18c to 68dc519 Compare September 21, 2021 05:32
Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey azeey force-pushed the joint_constraint_wrench branch from 68dc519 to b367494 Compare September 21, 2021 05:33
@azeey azeey marked this pull request as ready for review September 21, 2021 05:34
@azeey azeey requested a review from chapulina as a code owner September 21, 2021 05:34
@azeey azeey requested a review from mjcarroll September 21, 2021 05:34
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I haven't tried running it but the code and test looks reasonable to me.

Can we add unit tests for the component in test/integration/components.cc? That would have caught the serialization typo

@azeey
Copy link
Contributor Author

azeey commented Sep 21, 2021

@osrf-jenkins run test

@azeey
Copy link
Contributor Author

azeey commented Sep 21, 2021

Looks like the prerelease repo is not enabled on CI, so it's using ignition-physics5 from nightlies. Do we need to enable prereleases?

@chapulina
Copy link
Contributor

Looks like the prerelease repo is not enabled on CI,

Ahhhhh see gazebo-tooling/gzdev#35

@chapulina
Copy link
Contributor

@osrf-jenkins run tests please

@codecov
Copy link

codecov bot commented Sep 22, 2021

Codecov Report

Merging #989 (4131093) into main (c992f31) will increase coverage by 0.03%.
The diff coverage is 82.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #989      +/-   ##
==========================================
+ Coverage   63.40%   63.43%   +0.03%     
==========================================
  Files         240      241       +1     
  Lines       19548    19577      +29     
==========================================
+ Hits        12394    12419      +25     
- Misses       7154     7158       +4     
Impacted Files Coverage Δ
src/systems/physics/Physics.cc 70.95% <82.14%> (+0.37%) ⬆️
...nition/gazebo/components/JointTransmittedWrench.hh 100.00% <100.00%> (ø)

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 c992f31...4131093. Read the comment docs.

@chapulina chapulina added 🏯 fortress Ignition Fortress and removed needs upstream release Blocked by a release of an upstream library 🏰 citadel Ignition Citadel labels Sep 22, 2021
@chapulina chapulina merged commit 5250b39 into gazebosim:main Sep 22, 2021
@azeey azeey changed the title Populate JointConstraintWrench from physics Populate JointTransmittedWrench from physics Sep 27, 2021
WilliamLewww pushed a commit to WilliamLewww/ign-gazebo that referenced this pull request Dec 7, 2021
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: William Lew <[email protected]>
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.

2 participants