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.

Sign up for GitHub

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

Jump to bottom

Add parameters component #305

Merged
merged 60 commits into from
Oct 26, 2022
Merged

Add parameters component #305

merged 60 commits into from
Oct 26, 2022

Conversation

ivanpauno
Copy link
Contributor

@ivanpauno ivanpauno commented Apr 8, 2022 •
edited
Loading

🎉 New feature

Replaces gazebosim/gz-sim#1280.
Same as #304 but rebased to target ignition-transport11.

Connected to:

Summary

Adds an implementation of a parameter registry, which allows through services to:

  • Declare a parameter.
  • Get the value of a parameter.
  • Set the value of a parameter.
  • List all parameters.

The same operations can be done directly locally, using the parameters registry API.
There's also a parameters client implementation, which is just a wrapper on top of sending requests to the services.

Parameter types are defined as protobuf messages.
This PR also adds a ign param cli command, which allows to easily interact with parameters.

TODO:

  • Add tests!!!
  • Open PR in ignition gazebo using this!!!

Test it

TODO

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

@ivanpauno ivanpauno requested a review from caguero as a code owner April 8, 2022 18:01
@ivanpauno ivanpauno changed the title Ivanpauno/parameters component11 Add parameters component Apr 8, 2022
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Apr 8, 2022
This was referenced Apr 8, 2022
Merged
Merged
@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Apr 11, 2022
caguero
caguero requested changes Apr 18, 2022
View reviewed changes
parameters/include/ignition/transport/parameters/Registry.hh Outdated Show resolved Hide resolved
parameters/include/ignition/transport/parameters/Registry.hh Outdated Show resolved Hide resolved
parameters/include/ignition/transport/parameters/Registry.hh Outdated Show resolved Hide resolved
parameters/include/ignition/transport/parameters/Registry.hh Outdated Show resolved Hide resolved
parameters/include/ignition/transport/parameters/Registry.hh Outdated Show resolved Hide resolved
parameters/include/ignition/transport/parameters/Client.hh Outdated Show resolved Hide resolved
parameters/include/ignition/transport/parameters/Client.hh Outdated Show resolved Hide resolved
parameters/src/Client.cc Show resolved Hide resolved
parameters/include/ignition/transport/parameters/Client.hh Outdated Show resolved Hide resolved
parameters/src/cmd/ParamCommandAPI.hh Outdated Show resolved Hide resolved
caguero
caguero requested changes Apr 18, 2022
View reviewed changes
Copy link
Collaborator

@caguero caguero left a comment

Choose a reason for hiding this comment

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

See also the style errors reported by CI.

@ivanpauno ivanpauno requested a review from caguero April 26, 2022 17:56
ivanpauno added 16 commits May 24, 2022 15:58
@ivanpauno
wip
3d691a7 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
wip
493af65 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
wip
2573250 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Complete implementation of parameters::Registry class
c0b587f 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Use const std::string & when appropriate
c7e67ac 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
First ParameterClient implementation
d3d0a09 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Add ListParameters() to both registry and client
1878dbb 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Fix
5b8cf84 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Fix declare parameter
8490314 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Add command line API for param
e47860c 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Document ParameterRegistry class
c201493 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
fix code style
a96f102 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Make methods const when possible. Fix logs
832a8ab 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Make mehtods const if possible, alphabetize includes
e45478a 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Remove copy-pasted line in docs
d0e64ce 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
* Use pimpl in ParametersClient.
8ef1fce 
* Document ParametersClient.

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Remove commented out code, renaming
34ee98e 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Update docs
cd6053f 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Add extra info to logs
3a0b953 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Copy link
Contributor Author

It looks good to me! My only concern is about the use of exceptions. I know that they have their advantages but the convention that we follow is to avoid them as much as possible and not to terminate the execution. Is there any chance we could avoid exceptions? Maybe return an error structure with extra information as opposed to a boolean for not loosing information?

Done, please take a look to the last few commits.

@scpeters
Copy link
Member

it looks like there are several test failures

@ivanpauno
Copy link
Contributor Author

it looks like there are several test failures

Yes sorry, I'm figuring those out locally.

@ivanpauno
fix tests + linters
f3a36c7 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Remove unnecessary change
0d784e1 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Maybe fix windows issue
be51729 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Second try
16f2420 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Renaming
58b3608 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
More renaming, docs
15a72a9 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
test cases for ParameterResult
6c5df2a 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
another try to fix windows
be44635 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
fix
76a4c30 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Fix linter issues
81c1a0c 
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno ivanpauno requested a review from scpeters October 14, 2022 19:32
@scpeters
Copy link
Member

I think the ABI checker is a false positive, and the windows CI problems are from pre-existing protobuf compiler warnings

@caguero have all your concerns been addressed?

@scpeters scpeters requested a review from caguero October 25, 2022 18:16
caguero
caguero approved these changes Oct 25, 2022
View reviewed changes
Copy link
Collaborator

@caguero caguero left a comment

Choose a reason for hiding this comment

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

Looking good!

@scpeters scpeters merged commit 2d0d1d0 into ign-transport11 Oct 26, 2022
@scpeters scpeters deleted the ivanpauno/parameters-component11 branch October 26, 2022 17:21
@scpeters scpeters mentioned this pull request Oct 27, 2022
Merged
scpeters pushed a commit to gazebosim/gz-sim that referenced this pull request Dec 21, 2022
@ivanpauno
Add interface to allow systems to declare parameters (#1431)
10ed22b 
This adds the ISystemConfigureParameters interface, which
allows systems to declare parameters. This uses a
transport::parameters::ParametersRegistry and creates
services for listing, getting, and setting parameters.

Based on gazebosim/gz-transport#305.

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@scpeters scpeters mentioned this pull request Dec 21, 2022
Open
@Gaurang-1402 Gaurang-1402 mentioned this pull request Apr 1, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caguero caguero caguero approved these changes

@scpeters scpeters Awaiting requested review from scpeters

Assignees

@ivanpauno ivanpauno

Labels
enhancement New feature or request 🏯 fortress Ignition Fortress needs upstream release Blocked by a release of an upstream library
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ivanpauno @caguero @scpeters @chapulina