Skip to content

implement parameter_client#945

Merged
ihasdapie merged 20 commits intomasterfrom
brianc/parameter_client
Jun 14, 2022
Merged

implement parameter_client#945
ihasdapie merged 20 commits intomasterfrom
brianc/parameter_client

Conversation

@ihasdapie
Copy link
Copy Markdown
Member

@ihasdapie ihasdapie commented May 25, 2022 •
edited
Loading

This PR ports the rclcpp parameter_client to rclpy and also moves some functionality out of ros2param into rclpy. Related changes in ros2param can be found in ros2/ros2cli#716

I'd like some review on the proposed architecture & implementation before I clean things up and write tests etc.

@ihasdapie ihasdapie mentioned this pull request May 25, 2022
Merged
@ihasdapie ihasdapie requested a review from jacobperron May 25, 2022 00:24
@ihasdapie
Copy link
Copy Markdown
Member Author

ihasdapie commented May 25, 2022

Just a sanity check:

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

jacobperron
jacobperron reviewed May 25, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Looks reasonable to me! I have mostly minor comments

jacobperron
jacobperron reviewed May 27, 2022
View reviewed changes
jacobperron
jacobperron reviewed May 27, 2022
View reviewed changes
@ihasdapie ihasdapie force-pushed the brianc/parameter_client branch 2 times, most recently from d58af19 to 1ba1a56 Compare May 27, 2022 21:27
ihasdapie
ihasdapie commented Jun 1, 2022
View reviewed changes
ihasdapie added 13 commits June 1, 2022 15:22
@ihasdapie
Misc. typing fixes
097e997 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
Start AsyncParameterClient class
b851df6 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
core parameter_client functionality ported
86d9d20 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
add service_is_ready method
3bc9513 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
allow for listing all parameters
a56b1ae 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
synchronus client set/get/list
64e074f 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
use Parameter message type instead of rclpy.Parameter to align with r…
a3b4fa3 
…os2param

Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
move get_parameter_value from ros2param to rclpy
e2d26b7 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
load parameters from file
5d27400 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
add parameter_client to docs
3de00bd 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
address pr comments, docs, lint
7be64ba 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
add tests for parameter client
ff9b0e4 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
prepend underscore for private members
2b3f677 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie ihasdapie force-pushed the brianc/parameter_client branch from 581def1 to 2b3f677 Compare June 1, 2022 22:22
@ihasdapie ihasdapie marked this pull request as ready for review June 1, 2022 22:22
@ihasdapie
squash linting complaints
7894e96 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
Copy link
Copy Markdown
Member Author

ihasdapie commented Jun 1, 2022 •
edited
Loading

ci jobs for this PR and related PR ros2/ros2cli#716

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

jacobperron
jacobperron reviewed Jun 3, 2022
View reviewed changes
ihasdapie added 2 commits June 3, 2022 10:12
@ihasdapie
revert typing changes
95ad20a 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
address review comments
75a9b1f 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
jacobperron
jacobperron reviewed Jun 3, 2022
View reviewed changes
jacobperron
jacobperron reviewed Jun 3, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Another round of review!

thanks for iterating :)

ihasdapie added 3 commits June 3, 2022 14:46
@ihasdapie
add test to parameter_dict_from_yaml for nonexistent file
efc8b4d 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
allow for set_parameter to take both rclpy Parameter and rcl_interfac…
8c27cf5 
…es parameter msg

Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie
more consistent naming for functions
56a2c21 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
jacobperron
jacobperron reviewed Jun 3, 2022
View reviewed changes
jacobperron
jacobperron approved these changes Jun 3, 2022
View reviewed changes
Copy link
Copy Markdown
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.

LGTM

Did you have an example to add to the demos package? We could create a parameters directory there for demo_nodes_py.

@ihasdapie
Copy link
Copy Markdown
Member Author

ihasdapie commented Jun 3, 2022

Yup I'll submit something to the demos package as well

@ihasdapie
Copy link
Copy Markdown
Member Author

ihasdapie commented Jun 3, 2022

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

@ihasdapie
nicer list comprehension
7a2c47d 
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
@ihasdapie ihasdapie force-pushed the brianc/parameter_client branch from 9816834 to 7a2c47d Compare June 3, 2022 23:33
@ihasdapie
Copy link
Copy Markdown
Member Author

ihasdapie commented Jun 9, 2022

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

@ihasdapie ihasdapie merged commit 5c5393d into master Jun 14, 2022
@delete-merged-branch delete-merged-branch bot deleted the brianc/parameter_client branch June 14, 2022 19:56
@ihasdapie ihasdapie restored the brianc/parameter_client branch June 14, 2022 19:58
@Blast545
Copy link
Copy Markdown

Blast545 commented Jun 15, 2022

👨‍🌾 This PR introduced a test regression in the windows jobs of the buildfarm.
See: nightly_win_rel#2334 and nightly_win_deb#2389

I see that CI triggered for this PR already showed this error, why was it ignored?

Can you take a look @ihasdapie ?

@ihasdapie
Copy link
Copy Markdown
Member Author

ihasdapie commented Jun 15, 2022 •
edited
Loading

Ah shoot, my bad. I'll push something to resolve it shortly. Just a test error, looks something specific to python tempfile and the permissions on the buildfarm machine

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Jun 15, 2022

@ihasdapie i think we need to revert this.

This was referenced Jun 15, 2022
Closed
Merged
Merged
This was referenced Oct 1, 2025
Merged
Merged
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 @Blast545 @fujitatomoya @jacobperron