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 components to dynamically set joint limits #847

Merged
merged 8 commits into from
Nov 10, 2021

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Jun 2, 2021

🎉 New feature

Summary

This PR adds support for dynamic setting of joint limits (position, velocity and effort).

Depends on gazebosim/gz-physics#260 (and also gazebo-forks/dart#26, though it is not needed if it's no problem that some limits are not enforced).

Test it

Integration tests were added, see the usage in them.

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

Codecheck fails with src/systems/physics/Physics.cc:1943: Small and focused functions are preferred: PhysicsPrivate::UpdatePhysics() has 534 non-comment lines (error triggered by exceeding 500 lines). [readability/fn_size] [1], I don't really know what to do about that.

This is aimed at running in DARPA SubT finals.

@github-actions github-actions bot added the 🔮 dome Ignition Dome label Jun 2, 2021
@azeey azeey added the needs upstream release Blocked by a release of an upstream library label Jun 2, 2021
@peci1
Copy link
Contributor Author

peci1 commented Jun 3, 2021

Example usage here: osrf/subt#948 .

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.

Looks pretty good. I have a few comments that should be easy to address.

src/systems/physics/Physics.cc Outdated Show resolved Hide resolved
test/integration/physics_system.cc Show resolved Hide resolved
test/integration/physics_system.cc Show resolved Hide resolved
test/integration/physics_system.cc Outdated Show resolved Hide resolved
@peci1
Copy link
Contributor Author

peci1 commented Jul 13, 2021

I addressed all review comments and added a new (de)serialization test for the newly added components.

I was thinking about removing the Cmd suffix from the components (as was requested in the ign-physics part), but here it seems to me the suffix makes sense. I can imagine that in the future, somebody will want to create JointPositionLimits read-only component (although the information is already accessible via the axis component).

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.

Approving with one minor comment.

test/integration/components.cc Show resolved Hide resolved
@chapulina
Copy link
Contributor

Waiting until the end of September to release ign-physics

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
@azeey
Copy link
Contributor

azeey commented Oct 28, 2021

I'm going to retarget this to ign-gazebo3.

@azeey azeey changed the base branch from ign-gazebo4 to ign-gazebo3 October 29, 2021 03:18
@azeey azeey requested a review from iche033 as a code owner October 29, 2021 03:18
@azeey azeey added 🏰 citadel Ignition Citadel and removed 🔮 dome Ignition Dome labels Oct 29, 2021
@scpeters
Copy link
Member

I recommend requiring version 2.5 of ignition-physics2

@scpeters
Copy link
Member

I think the docker cache is preventing new gzdev values in the Ubuntu builds:

Step 15/51 : RUN rm -fr /home/jenkins/workspace/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/gzdev
 ---> Using cache
 ---> 73265823fecb
Step 16/51 : RUN git clone --depth 1 https://github.com/ignition-tooling/gzdev -b master /home/jenkins/workspace/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/gzdev
 ---> Using cache
 ---> 5646bc68480b
Step 17/51 : RUN /home/jenkins/workspace/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/gzdev/gzdev.py repository enable --project=ignition-gazebo3 --force-linux-distro=bionic || ( git -C /home/jenkins/workspace/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/gzdev pull origin master &&     /home/jenkins/workspace/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/gzdev/gzdev.py repository enable --project=ignition-gazebo3 --force-linux-distro=bionic )
 ---> Using cache
 ---> a85cc93d1942

Signed-off-by: Martin Pecka <peckama2@fel.cvut.cz>
@peci1
Copy link
Contributor Author

peci1 commented Nov 1, 2021

I increased the minimum required version.

@scpeters
Copy link
Member

scpeters commented Nov 1, 2021

looks like the docker cache has been refreshed, so the Ubuntu Jenkins jobs are passing, though Actions have some issues

@azeey
Copy link
Contributor

azeey commented Nov 1, 2021

The failure in the Focal Github Action is because a different version of DART (6.9) is used there. I see we've dealt with this before, but I'm not sure how it works:
https://github.com/ignitionrobotics/ign-gazebo/blob/632245103cb6e6b7849be5b34364f34a4e1218fd/test/integration/diff_drive_system.cc#L324-L328

@peci1
Copy link
Contributor Author

peci1 commented Nov 1, 2021

maybe similar to gazebosim/gz-physics@65397fb ?

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@azeey
Copy link
Contributor

azeey commented Nov 9, 2021

I added a check for DART 6.10.0 in 6b89ccb. I also added the NOLINT I asked you to remove from #869 because codecheck was failing 😅

@codecov
Copy link

codecov bot commented Nov 9, 2021

Codecov Report

Merging #847 (5a73fea) into ign-gazebo3 (df06bb2) will decrease coverage by 0.07%.
The diff coverage is 74.07%.

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

@@               Coverage Diff               @@
##           ign-gazebo3     #847      +/-   ##
===============================================
- Coverage        77.64%   77.57%   -0.08%     
===============================================
  Files              222      225       +3     
  Lines            12805    12885      +80     
===============================================
+ Hits              9942     9995      +53     
- Misses            2863     2890      +27     
Impacted Files Coverage Δ
src/systems/physics/Physics.cc 69.98% <68.65%> (-0.14%) ⬇️
...ignition/gazebo/components/JointEffortLimitsCmd.hh 100.00% <100.00%> (ø)
...nition/gazebo/components/JointPositionLimitsCmd.hh 100.00% <100.00%> (ø)
...nition/gazebo/components/JointVelocityLimitsCmd.hh 100.00% <100.00%> (ø)
...nclude/ignition/gazebo/components/Serialization.hh 92.85% <100.00%> (+2.53%) ⬆️
src/SimulationRunner.cc 93.42% <0.00%> (-1.07%) ⬇️

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 df06bb2...c84d2dc. Read the comment docs.

@azeey azeey self-assigned this Nov 10, 2021
@azeey azeey removed the needs upstream release Blocked by a release of an upstream library label Nov 10, 2021
@azeey
Copy link
Contributor

azeey commented Nov 10, 2021

@osrf-jenkins run tests please

@azeey azeey merged commit fa3d4fd into gazebosim:ign-gazebo3 Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants