Add interface to allow systems to declare parameters #1431

ivanpauno · 2022-04-08T18:18:47Z

🎉 New feature

Based on gazebosim/gz-transport#305.
Replacement of #1280.

Summary

This PR adds:

An interface that can be implemented by systems that allows them to declare parameters (ISystemConfigureParameters):
- The gazebo simulation runner creates an instance of ignition::transport::parameters::ParametersRegistry (see Add parameters component gz-transport#305) with a namespace /world/<world_name> that provides the following services:
- /world/<world_name>/list_parameters service: List available parameters names and types.
- /world/<world_name>/get_parameter service: Get the type and value of a parameter.
- /world/<world_name>/set_parameter service: Set a parameter: parameter name, value and type need to be provided.

Summary of what's needed to declare and use a parameter can be seen in the wheel slip demo branch: diff.

Test it

I haven't written automated tests yet, but there's a demo using parameters in the wheel slip plugin:

Steps to run the demo:

Build from source ignition fortress checking out the two branches specified above (I can provide PRs for another version if needed).

Launch the trisphere cycle world:

ign gazebo trisphere_cycle_wheel_slip.sdf

In another terminal with the workspace sourced:
- List available parameters:
```
ign param -r /world/wheel_slip -l
```
  Expected output
  
  systems.wheel_slip.trisphere_cycle1.wheel_front [ignition.msgs.WheelSlipParameters]
  systems.wheel_slip.trisphere_cycle1.wheel_rear_right [ignition.msgs.WheelSlipParameters]
  systems.wheel_slip.trisphere_cycle0.wheel_rear_right [ignition.msgs.WheelSlipParameters]
  systems.wheel_slip.trisphere_cycle1.wheel_rear_left [ignition.msgs.WheelSlipParameters]
  systems.wheel_slip.trisphere_cycle0.wheel_rear_left [ignition.msgs.WheelSlipParameters]
  systems.wheel_slip.trisphere_cycle0.wheel_front [ignition.msgs.WheelSlipParameters]
- Get the systems.wheel_slip.trisphere_cycle1.wheel_front parameter:
```
ign param -r /world/wheel_slip -g -n systems.wheel_slip.trisphere_cycle1.wheel_front
```
  Expected output
  
  Getting parameter [systems.wheel_slip.trisphere_cycle1.wheel_front] for registry namespace [/world/wheel_slip]...
  Parameter type [ign_msgs.WheelSlipParameters]
  
  slip_compliance_lateral: 1
  slip_compliance_longitudinal: 1
- Set the systems.wheel_slip.trisphere_cycle1.wheel_front parameter to a different value:
```
ign param -r /world/wheel_slip -s -n systems.wheel_slip.trisphere_cycle1.wheel_front -t ign_msgs.WheelSlipParameters -m "
  slip_compliance_lateral: 1
  slip_compliance_longitudinal: 2"
```
  Expected output
  
  Setting parameter [systems.wheel_slip.trisphere_cycle1.wheel_front] for world [wheel_slip]...
  
  Parameter successfully set!
- Get the systems.wheel_slip.trisphere_cycle1.wheel_front parameter again, its value should have changed:
```
ign param -r /world/wheel_slip -g -n systems.wheel_slip.trisphere_cycle1.wheel_front
```
  Expected output
  
  Getting parameter [systems.wheel_slip.trisphere_cycle1.wheel_front] for registry namespace [/world/wheel_slip]...
  Parameter type [ign_msgs.WheelSlipParameters]
  
  slip_compliance_lateral: 1
  slip_compliance_longitudinal: 2

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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

scpeters · 2022-11-04T00:40:20Z

I've made a prerelease of gz-transport11, so macOS and windows CI is unstable, but Ubuntu will need some extra work for CI to pass. I think this is ready for review

Signed-off-by: Ivan Santiago Paunovic <[email protected]> Pass ecm as parameter to systems implementing the ConfigureParameters interface Signed-off-by: Ivan Santiago Paunovic <[email protected]> Fix parameter registry namespace Signed-off-by: Ivan Santiago Paunovic <[email protected]> Fixes after rebasing Signed-off-by: Ivan Santiago Paunovic <[email protected]>

ivanpauno · 2022-11-09T18:56:50Z

@scpeters I have rebased the PR.

I think this is ready for review

Yes!
Let me know if I should add any reviewer here

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

mjcarroll · 2022-11-09T21:23:35Z

I think there are still some linter issues here.

ivanpauno · 2022-11-10T13:32:26Z

I think there are still some linter issues here.

Could you share in which job you are seeing the failures?
It seems to me that all jobs are failing when building, because the gz-transport changes needed haven't been released yet.
I didn't find linters failures in the jobs' output.
Locally, I also don't see linter failures (though I'm not sure if I have cppcheck configured correctly, as I see a lot of unrelated linter errors).

For testing gazebosim/gz-sim#1431 Signed-off-by: Steve Peters <[email protected]>

scpeters · 2022-11-10T21:41:01Z

testing against prerelease of ignition-transport11 with #1789

ivanpauno · 2022-11-11T12:49:48Z

Thanks Steve!
Failures look unrelated to me, let me know if that's not the case.

I think this is ready for review.

adityapande-1995

Is there a way you could add any tests for these ? Otherwise looks good.

mjcarroll · 2022-11-28T15:41:26Z

This needs a (not-prerelease) transport11 release. Is there anything blocking that at this point?

ivanpauno · 2022-11-28T15:45:35Z

Is there a way you could add any tests for these ? Otherwise looks good.

I think I can add some test, I will try to do that in the following days.

scpeters · 2022-11-28T18:17:01Z

This needs a (not-prerelease) transport11 release. Is there anything blocking that at this point?

just the principle of testing with prereleases before we lock in new APIs. I'll open a ci_matching_branch/ PR to test with prereleases

scpeters · 2022-11-28T19:06:05Z

This needs a (not-prerelease) transport11 release. Is there anything blocking that at this point?

just the principle of testing with prereleases before we lock in new APIs. I'll open a ci_matching_branch/ PR to test with prereleases

I forgot I had already done that with #1789. The Ubuntu CI is building fine but failing with some test failures

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

ivanpauno · 2022-12-06T17:20:04Z

Is there a way you could add any tests for these ? Otherwise looks good.

Added one test in 0c8d710.
If you have more ideas let me know.

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

adityapande-1995

Test looks good, but CI is failing with :

 /github/workspace/include/ignition/gazebo/System.hh:28:10: fatal error: ignition/transport/parameters/Registry.hh: No such file or directory
   #include <ignition/transport/parameters/Registry.hh>

Changes here look good to me, waiting for green CI.

scpeters · 2022-12-07T22:18:06Z

Test looks good, but CI is failing with :
 /github/workspace/include/ignition/gazebo/System.hh:28:10: fatal error: ignition/transport/parameters/Registry.hh: No such file or directory
   #include <ignition/transport/parameters/Registry.hh>
Changes here look good to me, waiting for green CI.

I think this was working with the 11.3.0~pre1 prerelease, but it is not working with the 11.3.0 or 11.3.1 stable releases. Investigating...

scpeters · 2022-12-08T04:59:04Z

I think this was working with the 11.3.0~pre1 prerelease, but it is not working with the 11.3.0 or 11.3.1 stable releases. Investigating...

I think gazebosim/gz-transport#374 should fix it, though it will require another release

adityapande-1995

A question for @ivanpauno , is this meant to be used by sensors (or their systems) ? Right now for the wheel slip plugin I can understand that there will be only a single system, but in future if some sensor wants to declare parameters, how should the system separate parameters between sensors ?

For e.g., camera 1 and camera 2 want to have different parameters in the rendering sensors system ? Maybe this is not the intended use case.

scpeters · 2022-12-08T21:50:15Z

@osrf-jenkins run tests please

codecov · 2022-12-08T22:48:50Z

Codecov Report

Merging #1431 (154972f) into ign-gazebo6 (216d5a5) will decrease coverage by 0.01%.
The diff coverage is 85.00%.

@@               Coverage Diff               @@
##           ign-gazebo6    #1431      +/-   ##
===============================================
- Coverage        64.60%   64.59%   -0.02%     
===============================================
  Files              321      321              
  Lines            26297    26312      +15     
===============================================
+ Hits             16989    16995       +6     
- Misses            9308     9317       +9

Impacted Files	Coverage Δ
include/ignition/gazebo/System.hh	`100.00% <ø> (ø)`
src/SimulationRunner.hh	`100.00% <ø> (ø)`
src/SimulationRunner.cc	`91.77% <80.00%> (-0.12%)`	⬇️
src/SystemManager.cc	`96.35% <84.61%> (-1.29%)`	⬇️
src/SystemInternal.hh	`100.00% <100.00%> (ø)`
src/systems/thruster/Thruster.cc	`85.80% <0.00%> (-3.71%)`	⬇️

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

scpeters · 2022-12-09T00:19:26Z

there's a few jobs with failing tests, but there are some clean CI jobs as well

ivanpauno · 2022-12-14T14:50:12Z

A question for @ivanpauno , is this meant to be used by sensors (or their systems) ? Right now for the wheel slip plugin I can understand that there will be only a single system, but in future if some sensor wants to declare parameters, how should the system separate parameters between sensors ?

For e.g., camera 1 and camera 2 want to have different parameters in the rendering sensors system ? Maybe this is not the intended use case.

My idea is that the system is responsible of correct naming.
My branch with a wheel slip plugin demo is out of date, but it still gives an idea of how to avoid name colisions:

gz-sim/src/systems/wheel_slip/WheelSlip.cc

Lines 380 to 389 in c7ca5a3

    
           std::string scopedName = ignition::gazebo::scopedName( 
        
             linkParamsPair.first, _ecm, ".", false); 
        
           auto paramName = std::string("systems.wheel_slip.") + scopedName; 
        
           auto wsParams = std::make_unique<ignition::msgs::WheelSlipParameters>(); 
        
           wsParams->set_slip_compliance_lateral( 
        
             linkParamsPair.second.slipComplianceLateral); 
        
           wsParams->set_slip_compliance_longitudinal( 
        
             linkParamsPair.second.slipComplianceLongitudinal); 
        
           _registry.DeclareParameter(paramName, std::move(wsParams));

e.g. for cameras it could be:

/systems/sensors/cameras/camera1/<param_name>

ivanpauno · 2022-12-14T14:53:21Z

I think gazebosim/gz-transport#374 should fix it, though it will require another release

Thanks for the fix @scpeters !!!

there's a few jobs with failing tests, but there are some clean CI jobs as well

I have taken a look to the failing jobs and they seem to have unrelated failures.
Let me know if any change is needed here.

adityapande-1995

LGTM

azeey

LGTM!

ivanpauno requested a review from chapulina as a code owner April 8, 2022 18:18

github-actions bot added the 🏯 fortress Ignition Fortress label Apr 8, 2022

ivanpauno mentioned this pull request Apr 8, 2022

Add message definitions for the parameter component gazebosim/gz-msgs#241

Merged

8 tasks

chapulina added the needs upstream release Blocked by a release of an upstream library label Apr 11, 2022

ivanpauno mentioned this pull request Apr 26, 2022

Add parameters component gazebosim/gz-transport#305

Merged

10 tasks

ivanpauno force-pushed the ivanpauno/parameters-component6 branch from 317f448 to 7b38dc7 Compare May 24, 2022 19:57

chapulina added the enhancement New feature or request label Jul 23, 2022

scpeters requested a review from mjcarroll as a code owner November 3, 2022 05:14

ivanpauno force-pushed the ivanpauno/parameters-component6 branch from f5d91b7 to 817d842 Compare November 9, 2022 18:52

Fix linters

caf0580

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

scpeters added a commit to gazebo-tooling/gzdev that referenced this pull request Nov 10, 2022

Use prerelease for ignition-gazebo6

f83f5e4

For testing gazebosim/gz-sim#1431 Signed-off-by: Steve Peters <[email protected]>

scpeters mentioned this pull request Nov 10, 2022

Ci matching branch/parameters component6 #1789

Closed

adityapande-1995 approved these changes Nov 22, 2022

View reviewed changes

Add test case

0c8d710

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

remove whitespace

154972f

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

ivanpauno requested review from adityapande-1995 and removed request for chapulina December 6, 2022 18:21

adityapande-1995 requested changes Dec 6, 2022

View reviewed changes

scpeters mentioned this pull request Dec 8, 2022

Fix include/ignition/.../parameters header files gazebosim/gz-transport#374

Merged

8 tasks

adityapande-1995 reviewed Dec 8, 2022

View reviewed changes

adityapande-1995 approved these changes Dec 20, 2022

View reviewed changes

azeey approved these changes Dec 21, 2022

View reviewed changes

scpeters merged commit 10ed22b into ign-gazebo6 Dec 21, 2022

scpeters deleted the ivanpauno/parameters-component6 branch December 21, 2022 21:40

scpeters mentioned this pull request Dec 21, 2022

Write a tutorial about using transport parameters with systems #1845

Open

Gaurang-1402 mentioned this pull request Apr 1, 2024

Add Tutorial on Using Transport Parameters with Systems osrf/gazebo_tutorials#175

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add interface to allow systems to declare parameters #1431

Add interface to allow systems to declare parameters #1431

ivanpauno commented Apr 8, 2022

slip_compliance_lateral: 1
slip_compliance_longitudinal: 1

slip_compliance_lateral: 1
slip_compliance_longitudinal: 2

scpeters commented Nov 4, 2022

ivanpauno commented Nov 9, 2022

mjcarroll commented Nov 9, 2022

ivanpauno commented Nov 10, 2022

scpeters commented Nov 10, 2022

ivanpauno commented Nov 11, 2022

adityapande-1995 left a comment

mjcarroll commented Nov 28, 2022

ivanpauno commented Nov 28, 2022

scpeters commented Nov 28, 2022

scpeters commented Nov 28, 2022

ivanpauno commented Dec 6, 2022

adityapande-1995 left a comment •

edited

Loading

scpeters commented Dec 7, 2022

scpeters commented Dec 8, 2022

adityapande-1995 left a comment

scpeters commented Dec 8, 2022

codecov bot commented Dec 8, 2022

scpeters commented Dec 9, 2022

ivanpauno commented Dec 14, 2022

ivanpauno commented Dec 14, 2022

adityapande-1995 left a comment

azeey left a comment

Add interface to allow systems to declare parameters #1431

Add interface to allow systems to declare parameters #1431

Conversation

ivanpauno commented Apr 8, 2022

🎉 New feature

Summary

Test it

slip_compliance_lateral: 1 slip_compliance_longitudinal: 1

slip_compliance_lateral: 1 slip_compliance_longitudinal: 2

Checklist

scpeters commented Nov 4, 2022

ivanpauno commented Nov 9, 2022

mjcarroll commented Nov 9, 2022

ivanpauno commented Nov 10, 2022

scpeters commented Nov 10, 2022

ivanpauno commented Nov 11, 2022

adityapande-1995 left a comment

Choose a reason for hiding this comment

mjcarroll commented Nov 28, 2022

ivanpauno commented Nov 28, 2022

scpeters commented Nov 28, 2022

scpeters commented Nov 28, 2022

ivanpauno commented Dec 6, 2022

adityapande-1995 left a comment • edited Loading

Choose a reason for hiding this comment

scpeters commented Dec 7, 2022

scpeters commented Dec 8, 2022

adityapande-1995 left a comment

Choose a reason for hiding this comment

scpeters commented Dec 8, 2022

codecov bot commented Dec 8, 2022

Codecov Report

scpeters commented Dec 9, 2022

ivanpauno commented Dec 14, 2022

ivanpauno commented Dec 14, 2022

adityapande-1995 left a comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

slip_compliance_lateral: 1
slip_compliance_longitudinal: 1

slip_compliance_lateral: 1
slip_compliance_longitudinal: 2

adityapande-1995 left a comment •

edited

Loading