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

implement parameter_client #959

Merged
merged 34 commits into from
Jun 29, 2022
Merged

implement parameter_client #959

merged 34 commits into from
Jun 29, 2022

Conversation

ihasdapie
Copy link
Member

New pull request due to previous CI failures. This PR reverts the revert and then applies fixes for windows CI as well as wildcard parameter precedence and default parameter listing depth

See:

@ihasdapie
Copy link
Member Author

ihasdapie commented Jun 15, 2022 •
edited
Loading

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ihasdapie ihasdapie mentioned this pull request Jun 15, 2022
Merged
@ihasdapie
Copy link
Member Author

ihasdapie commented Jun 15, 2022 •
edited
Loading

image
One test failure unrelated, other two are flaky tests in ros2param due to test timeout. (Reproduciable locally using stress and --retest-until-fail. I bumped timeout in ros2param and running CI again with more --retest-until-pass

@ihasdapie ihasdapie mentioned this pull request Jun 16, 2022
Merged
@ihasdapie ihasdapie changed the title implement parameter_client" implement parameter_client Jun 16, 2022
@ihasdapie
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
    CI passed (except for a flake8 issue I just fixed). Other failures unrelated

jacobperron
jacobperron reviewed Jun 17, 2022
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

I've done another pass with recent changes. Just some feedback related to documentation.

rclpy/rclpy/__init__.py Outdated Show resolved Hide resolved
rclpy/rclpy/parameter.py Outdated Show resolved Hide resolved
rclpy/rclpy/parameter.py Outdated Show resolved Hide resolved
rclpy/rclpy/parameter.py Outdated Show resolved Hide resolved
rclpy/rclpy/parameter.py Show resolved Hide resolved
rclpy/rclpy/parameter_client.py Outdated Show resolved Hide resolved
rclpy/rclpy/parameter_client.py Outdated Show resolved Hide resolved
@ihasdapie
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

ihasdapie and others added 22 commits June 17, 2022 12:15
@ihasdapie
Misc. typing fixes
abb82d7 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
Start AsyncParameterClient class
6e435e6 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
core parameter_client functionality ported
8b5d2d9 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
add service_is_ready method
4d0b44d 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
allow for listing all parameters
39dd54d 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
synchronus client set/get/list
ebc4481 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
use Parameter message type instead of rclpy.Parameter to align with r…
aec10c5 
…os2param

Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
move get_parameter_value from ros2param to rclpy
e79afbb 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
load parameters from file
852e3a8 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
add parameter_client to docs
f1731e3 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
address pr comments, docs, lint
5d56b1f 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
add tests for parameter client
591cffa 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
prepend underscore for private members
6fab6bf 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
squash linting complaints
79ac7fe 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
revert typing changes
be6e43d 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
address review comments
40acf32 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
add test to parameter_dict_from_yaml for nonexistent file
83be7d3 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
allow for set_parameter to take both rclpy Parameter and rcl_interfac…
4870ade 
…es parameter msg

Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
more consistent naming for functions
ff8a56e 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
nicer list comprehension
b7556f6 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@fujitatomoya @ihasdapie
name and type in descriptor(s) is ignored via declare_parameter(s). (#…
b90edf0 
…957)

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
Revert "implement parameter_client" (#958)
f815c6c 
See discussion @ #956

Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
fix linting
0c32ba1 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
address pr comments
fb21b66 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie ihasdapie force-pushed the revert-958-revert-945-brianc/parameter_client branch from 9d4ef96 to fb21b66 Compare June 17, 2022 19:15
@ihasdapie
don't reference ros2 param dump
214ab92 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@jacobperron
Copy link
Member

There are some flake8 issues to address: https://build.ros2.org/job/Rpr__rclpy__ubuntu_jammy_amd64/105/testReport/junit/rclpy/

ihasdapie and others added 2 commits June 21, 2022 15:26
@ihasdapie @jacobperron
Update rclpy/rclpy/parameter.py
d6dd191 
Co-authored-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
flake8
bbf43c4 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie ihasdapie force-pushed the revert-958-revert-945-brianc/parameter_client branch from d11c6ac to bbf43c4 Compare June 21, 2022 22:26
@jacobperron
Copy link
Member

@ros-pull-request-builder retest this please

@ihasdapie
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Windows failure:
image

@ivanpauno
Copy link
Member

The details of the windows error can be seen here https://ci.ros2.org/job/ci_windows/17278/consoleFull#console-section-786.
It's a Windows fatal exception: access violation (i.e. a segmentation fault).

It might be related to the PR or not, I'm not sure.

@ihasdapie
Copy link
Member Author

The same error occured in a nightly windows build yesterday https://ci.ros2.org/view/nightly/job/nightly_win_rel/2340/

@ivanpauno
Copy link
Member

The same error occured in a nightly windows build yesterday https://ci.ros2.org/view/nightly/job/nightly_win_rel/2340/

Ok, seems unrelated then

@ihasdapie ihasdapie mentioned this pull request Jun 23, 2022
Open
@ihasdapie
add load_parameter_file_atomically
fbf5959 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
jacobperron
jacobperron reviewed Jun 27, 2022
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

I found a few small things to complain about. After addressing them, please run CI again to ensure we're not breaking anything, thanks!

rclpy/rclpy/parameter_client.py Outdated Show resolved Hide resolved
rclpy/rclpy/parameter_client.py Show resolved Hide resolved
rclpy/rclpy/parameter_client.py Outdated Show resolved Hide resolved
rclpy/rclpy/parameter_client.py Outdated Show resolved Hide resolved
rclpy/test/test_parameter.py Outdated Show resolved Hide resolved
rclpy/test/test_parameter_client.py Outdated Show resolved Hide resolved
rclpy/test/test_parameter_client.py Outdated Show resolved Hide resolved
@ihasdapie
address PR comments
ddd40f3 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:21
@ihasdapie
Copy link
Member Author

Windows pass:
Build Status

Seems to be some flaky tests, just ran CI again with same configuration

@ihasdapie ihasdapie merged commit 3053a8a into rolling Jun 29, 2022
@ihasdapie ihasdapie deleted the revert-958-revert-945-brianc/parameter_client branch June 29, 2022 18:40
@ihasdapie ihasdapie mentioned this pull request Jul 14, 2022
Open
@Barry-Xu-2018 Barry-Xu-2018 mentioned this pull request Nov 11, 2022
Merged
@fujitatomoya fujitatomoya mentioned this pull request Mar 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobperron jacobperron jacobperron approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ihasdapie @jacobperron @ivanpauno @fujitatomoya