Support nested joints in JointPositionController #1851

srmainwaring · 2022-12-29T23:06:31Z

🦟 Bug fix

Partially Fixes #1844

Summary

The details of the bug and how to reproduce it are documented in gz-sim: #1844.

This PR enables the JointPositionController plugin to search for scoped joint names as well as unscoped ones. The approach is similar to that used in the Buoyancy plugin.

For the change to take effect the upstream change in gazebosim/gz-physics#464 is also required.

Context

The use case for this change is to be able to include standalone models for peripherals such as https://app.gazebosim.org/OpenRobotics/fuel/models/Gimbal%20Small%202D in a standard vehicle frame such as https://app.gazebosim.org/OpenRobotics/fuel/models/Iris%20with%20Standoffs and then configure the JointPositionController at the top level for joint control.

The only way the position controller will work at present is to copy the elements from gimbal model into the quadcopter model (renaming any duplicated joints and links). This is inconvenient but possible for PoC but does not scale well.

Checklist

Signed all commits for DCO
Added tests
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

src/systems/joint_position_controller/JointPositionController.cc

arjo129 · 2023-01-04T08:25:26Z

Any chance we could add a test (since we already have a SDF that can replicate this issue)?

srmainwaring · 2023-01-11T08:33:53Z

Any chance we could add a test (since we already have a SDF that can replicate this issue)?

@arjo129 do you mean use the example provided in gazebosim/gz-physics#464 in a c++ unit test? Should be able to do that. The physics PR has been rewritten following review suggestions, so would be good to have the updated approach reviewed and agreed before finalising tests for this PR.

arjo129 · 2023-01-12T20:42:37Z

@srmainwaring, that is correct. Itd be great if the example could be added to our integration tests.

srmainwaring · 2023-01-13T16:40:01Z

Itd be great if the example could be added to our integration tests.

@arjo129 - I'll put something together. The world file might also be worth adding to gz-sim/examples/worlds with a tutorial to accompany it.

Any thoughts on the dependency gazebosim/gz-physics#464?

arjo129 · 2023-01-18T09:17:13Z

I'm not really an expert on gz-physics side of things. While it looks good to me I'd rather @azeey or @scpeters makes the call on that front.

codecov · 2023-01-23T13:21:35Z

Codecov Report

Merging #1851 (0a4fb74) into gz-sim7 (5d35c5a) will decrease coverage by 0.01%.
The diff coverage is 57.14%.

@@             Coverage Diff             @@
##           gz-sim7    #1851      +/-   ##
===========================================
- Coverage    64.57%   64.57%   -0.01%     
===========================================
  Files          344      344              
  Lines        27578    27590      +12     
===========================================
+ Hits         17808    17815       +7     
- Misses        9770     9775       +5

Impacted Files	Coverage Δ
...int_position_controller/JointPositionController.cc	`74.13% <57.14%> (+0.68%)`	⬆️
src/ServerPrivate.cc	`82.53% <0.00%> (-0.44%)`	⬇️
src/SimulationRunner.cc	`91.99% <0.00%> (-0.16%)`	⬇️
src/systems/physics/Physics.cc	`66.27% <0.00%> (-0.07%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

srmainwaring · 2023-01-27T09:44:32Z

Rebased on gz-sim7 to include #1861.

srmainwaring · 2023-01-27T12:19:47Z

@ahcorde added test case for the joints in nested models. The upstream dependency in gz-physics has been approved - just waiting for it to be merged.

azeey

LGTM! I'll work on making a release of gz-physics for this.

src/systems/joint_position_controller/JointPositionController.cc

- Attempt to resolve by scoped name first. - Replace static variable by member in private data. Signed-off-by: Rhys Mainwaring <[email protected]> 2. Adopt review suggestions. Co-authored-by: Alejandro Hernández Cordero <[email protected]> 3. JointPosController: support nested joints - Add example world containing nested models with position controlled joints. Signed-off-by: Rhys Mainwaring <[email protected]> - Fix build warning - remove trailing whitespace. Signed-off-by: Rhys Mainwaring <[email protected]> 4. JointPosController: support nested joints - Add test for joint position controller with nested models. - Update PID gains in example worlds model. Signed-off-by: Rhys Mainwaring <[email protected]> 5. JointPosController: support nested joints - Review feedback - remove redundant check. Signed-off-by: Rhys Mainwaring <[email protected]>

mjcarroll · 2023-02-03T15:09:38Z

LGTM! I'll work on making a release of gz-physics for this.

Physics 6.3.0 is out.

mjcarroll · 2023-02-03T15:09:46Z

@osrf-jenkins retest this please

azeey · 2023-02-03T16:25:14Z

Homebrew bottles are still being built: osrf/homebrew-simulation#2194

azeey · 2023-02-03T19:39:47Z

Retrying homebrew:

azeey · 2023-02-03T22:56:28Z

Okay, the tests are passing on macOS: .

@ahcorde any remaining issues?

azeey · 2023-02-08T15:58:50Z

None of the CI failures appear to be caused by this PR. Merging.

srmainwaring requested a review from mjcarroll as a code owner December 29, 2022 23:06

github-actions bot added the 🌱 garden Ignition Garden label Dec 29, 2022

srmainwaring mentioned this pull request Dec 29, 2022

Gimbal: enable gimbal in iris model ArduPilot/ardupilot_gazebo#43

Merged

4 tasks

azeey self-requested a review January 2, 2023 17:13

ahcorde requested changes Jan 3, 2023

View reviewed changes

srmainwaring mentioned this pull request Jan 4, 2023

Resolve joints in nested models gazebosim/gz-physics#464

Merged

8 tasks

srmainwaring force-pushed the srmainwaring/7/joint-pos-controller branch 4 times, most recently from 1b66900 to 908e394 Compare January 23, 2023 12:17

srmainwaring force-pushed the srmainwaring/7/joint-pos-controller branch from 908e394 to 8029cfd Compare January 27, 2023 09:40

srmainwaring force-pushed the srmainwaring/7/joint-pos-controller branch from 294d43d to 85c9bae Compare January 27, 2023 09:49

srmainwaring requested review from ahcorde and removed request for azeey and mjcarroll January 27, 2023 12:18

srmainwaring mentioned this pull request Feb 1, 2023

JointPositionController not working for nested models #1844

Closed

azeey added the needs upstream release Blocked by a release of an upstream library label Feb 1, 2023

azeey approved these changes Feb 2, 2023

View reviewed changes

src/systems/joint_position_controller/JointPositionController.cc Outdated Show resolved Hide resolved

srmainwaring force-pushed the srmainwaring/7/joint-pos-controller branch from 36d8eb2 to 0a4fb74 Compare February 3, 2023 08:44

mjcarroll removed the needs upstream release Blocked by a release of an upstream library label Feb 3, 2023

ahcorde approved these changes Feb 8, 2023

View reviewed changes

azeey merged commit e078dba into gazebosim:gz-sim7 Feb 8, 2023

srmainwaring deleted the srmainwaring/7/joint-pos-controller branch February 8, 2023 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support nested joints in JointPositionController #1851

Support nested joints in JointPositionController #1851

srmainwaring commented Dec 29, 2022 •

edited

Loading

arjo129 commented Jan 4, 2023 •

edited

Loading

srmainwaring commented Jan 11, 2023

arjo129 commented Jan 12, 2023

srmainwaring commented Jan 13, 2023

arjo129 commented Jan 18, 2023

codecov bot commented Jan 23, 2023 •

edited

Loading

srmainwaring commented Jan 27, 2023

srmainwaring commented Jan 27, 2023

azeey left a comment

mjcarroll commented Feb 3, 2023

mjcarroll commented Feb 3, 2023

azeey commented Feb 3, 2023

azeey commented Feb 3, 2023

azeey commented Feb 3, 2023

azeey commented Feb 8, 2023

Support nested joints in JointPositionController #1851

Support nested joints in JointPositionController #1851

Conversation

srmainwaring commented Dec 29, 2022 • edited Loading

🦟 Bug fix

Summary

Context

Checklist

arjo129 commented Jan 4, 2023 • edited Loading

srmainwaring commented Jan 11, 2023

arjo129 commented Jan 12, 2023

srmainwaring commented Jan 13, 2023

arjo129 commented Jan 18, 2023

codecov bot commented Jan 23, 2023 • edited Loading

Codecov Report

srmainwaring commented Jan 27, 2023

srmainwaring commented Jan 27, 2023

azeey left a comment

Choose a reason for hiding this comment

mjcarroll commented Feb 3, 2023

mjcarroll commented Feb 3, 2023

azeey commented Feb 3, 2023

azeey commented Feb 3, 2023

azeey commented Feb 3, 2023

azeey commented Feb 8, 2023

srmainwaring commented Dec 29, 2022 •

edited

Loading

arjo129 commented Jan 4, 2023 •

edited

Loading

codecov bot commented Jan 23, 2023 •

edited

Loading