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

Added DART feature for setting joint limits dynamically. #260

Merged
merged 8 commits into from
Jul 22, 2021

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Jun 1, 2021

🎉 New feature

Closes #96

Summary

Added features SetJointPositionLimitsCommandFeature, SetJointVelocityLimitsCommandFeature, SetJointEffortLimitsCommandFeature which allow runtime changes to joint limits.

The only thing that doesn't work as expected is setting a position limit and controlling the joint via velocity commands - the joint will slow down when violating the limit, but not stop. This is a reported DART issue: dartsim/dart#1583 . I at least tried to detect the situations when the bug affects the ign-physics behavior and issue an error that an effort limit should be set for the joint - this is a workaround for the bug (but there is no single thershold that would work for all cases, so I left the setting to the user).

Until gazebo-forks/dart#26 is out, some tests may be failing because limits are not respected. But I'd say this PR is kind of independent and can be merged even before the DART update (maybe temporarily disabling the failing tests). The tests known to fail with older DART and succeed with newer DART are JointSetVelocityLimitsWithForceControl and JointSetCombinedLimitsWithForceControl.

Test it

Automated tests were added. See the tests.

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

peci1 added 2 commits June 2, 2021 00:55
Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
…rted once newer DART is out).

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
@peci1 peci1 requested a review from mxgrey as a code owner June 1, 2021 23:16
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Jun 1, 2021
Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
@peci1
Copy link
Contributor Author

peci1 commented Jun 2, 2021

Followup ign-gazebo PR: gazebosim/gz-sim#847 .

@peci1
Copy link
Contributor Author

peci1 commented Jun 3, 2021

Example usage here: osrf/subt#948 .

@chapulina chapulina added the DART DART engine label Jun 22, 2021
@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Jul 6, 2021
@chapulina
Copy link
Contributor

Until gazebo-forks/dart#26 is out, some tests may be failing because limits are not respected.

I'm in favor of getring the release out before merging this so we can get it in with all the successful tests 👍

@codecov
Copy link

codecov bot commented Jul 7, 2021

Codecov Report

Merging #260 (f1d9269) into ign-physics3 (c788a0d) will decrease coverage by 0.19%.
The diff coverage is 63.52%.

❗ Current head f1d9269 differs from pull request most recent head 4984cec. Consider uploading reports for the commit 4984cec to get more accurate results
Impacted file tree graph

@@               Coverage Diff                @@
##           ign-physics3     #260      +/-   ##
================================================
- Coverage         74.63%   74.43%   -0.20%     
================================================
  Files               115      115              
  Lines              4703     4788      +85     
================================================
+ Hits               3510     3564      +54     
- Misses             1193     1224      +31     
Impacted Files Coverage Δ
dartsim/src/JointFeatures.cc 63.42% <49.18%> (-4.44%) ⬇️
include/ignition/physics/detail/Joint.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 c788a0d...4984cec. Read the comment docs.

Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

LGTM! Just a few minor comments.

dartsim/src/JointFeatures.hh Outdated Show resolved Hide resolved
dartsim/src/JointFeatures_TEST.cc Outdated Show resolved Hide resolved
dartsim/src/JointFeatures_TEST.cc Show resolved Hide resolved
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
include/ignition/physics/Joint.hh Outdated Show resolved Hide resolved
peci1 and others added 2 commits July 12, 2021 16:16
Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

Co-authored-by: Addisu Z. Taddese <addisu@gmail.com>
Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
@peci1
Copy link
Contributor Author

peci1 commented Jul 12, 2021

Thanks for the review. I addressed all issues.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey azeey merged commit 55a4901 into gazebosim:ign-physics3 Jul 22, 2021
azeey added a commit to azeey/gz-physics that referenced this pull request Oct 27, 2021
* Added DART feature for setting joint limits dynamically.

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

* Make code compatible with older DART release (this commit can be reverted once newer DART is out).

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

* Updated the number of models for test.

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

* Apply suggestions from code review

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

Co-authored-by: Addisu Z. Taddese <addisu@gmail.com>

* Fixed issues from review.

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

* Accomodate DART 6.9

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Co-authored-by: Addisu Z. Taddese <addisu@gmail.com>
Co-authored-by: Steve Peters <scpeters@openrobotics.org>
@azeey azeey mentioned this pull request Oct 27, 2021
azeey added a commit to azeey/gz-physics that referenced this pull request Oct 28, 2021
* Added DART feature for setting joint limits dynamically.

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

* Make code compatible with older DART release (this commit can be reverted once newer DART is out).

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

* Updated the number of models for test.

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

* Apply suggestions from code review

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

Co-authored-by: Addisu Z. Taddese <addisu@gmail.com>

* Fixed issues from review.

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

* Accomodate DART 6.9

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Co-authored-by: Addisu Z. Taddese <addisu@gmail.com>
Co-authored-by: Steve Peters <scpeters@openrobotics.org>
azeey added a commit that referenced this pull request Oct 28, 2021
* Added DART feature for setting joint limits dynamically.

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

* Make code compatible with older DART release (this commit can be reverted once newer DART is out).

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

* Updated the number of models for test.

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

* Apply suggestions from code review

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

Co-authored-by: Addisu Z. Taddese <addisu@gmail.com>

* Fixed issues from review.

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>

* Accomodate DART 6.9

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Co-authored-by: Addisu Z. Taddese <addisu@gmail.com>
Co-authored-by: Steve Peters <scpeters@openrobotics.org>
@chapulina chapulina mentioned this pull request Nov 11, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DART DART engine 🔮 dome Ignition Dome needs upstream release Blocked by a release of an upstream library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants