Add interface to allow systems to declare parameters

[ivanpauno]

🎉 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:

• https://github.com/ignitionrobotics/ign-gazebo/tree/ivanpauno/parameters-component-wheel-slip-demo6
• https://github.com/ignitionrobotics/ign-msgs/tree/ivanpauno/parameters-component-wheel-slip-demo8

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]

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

[ivanpauno]

@scpeters I have rebased the PR.

I think this is ready for review

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

[mjcarroll]

I think there are still some linter issues here.

[ivanpauno]

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).

[scpeters]

testing against prerelease of ignition-transport11 with #1789

[ivanpauno]

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]

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

[ivanpauno]

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]

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]

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

[ivanpauno]

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.

[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]

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]

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]

@osrf-jenkins run tests please

[codecov]

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]

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

[ivanpauno]

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]

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!