demo for rclpy parameter_client #566
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
jacobperron
reviewed
Jun 20, 2022
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
jacobperron
reviewed
Jun 23, 2022
There was a problem hiding this comment.
Technically, the demo looks okay to me. However, there's a lot going on this demo and I want to make sure we present the content clearly (both in code and output). Running it produces a wall of text that is a bit overwhelming.
I think it would be nicer to clean up the output so it looks something like:
Listing parameters:
zero
one
etc...
Getting parameters:
zero: 0
one: 1
etc...
Note, this also makes it easier to automate testing the expected output if we end up doing that for the Python demos in the future. We do this for the C++ demos.
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
|
Example output: Format is valid yaml and should be consistent in case we end up wanting to automate testing these demos |
jacobperron
reviewed
Jul 21, 2022
There was a problem hiding this comment.
Thanks for cleaning up the output!
My remaining comments are mostly about trying to simplify the code. For reference, all of the other Python demos are less than 100 LOC. Though I don't think this has to be that short, I see a few ways we can reduce complexity.
Comment on lines
30
to
31
| context = rclpy.context.Context() | ||
| rclpy.init(context=context) |
There was a problem hiding this comment.
Correct me if I'm wrong, but I think we can just use the default context to keep things simple,
I.e. just elide passing an explicit context everywhere.
Suggested change
| context = rclpy.context.Context() | |
| rclpy.init(context=context) | |
| rclpy.init() |
Comment on lines
39
to
44
| target_node.declare_parameters('', [ | ||
| ('int_parameter', 1), | ||
| ('bool_parameter', False), | ||
| ('string_parameter', 'Hello World'), | ||
| ]) | ||
| target_executor.add_node(target_node) |
There was a problem hiding this comment.
In the spirit of keeping the demo simple, can we just use a single node (ie. use the same node to create the parameters client and as the target node)? Then we can avoid creating an executor and spinning in a separate thread.
There was a problem hiding this comment.
My original intent for having two nodes and two seperate contexts is to demonstrate the new functionality of first-class support for setting a remote nodes' parameters. I feel like having only a single node in the demo defeats that purpose since a node setting parameters for itself would presumably just use self.set_parameters.
That being said, I agree with keeping the demo simple. I think the best way to do that would be to not exhaustively run through every method offered by ParameterClient -- it is a little repetitive and overruns this demo with boilerplate code.
There was a problem hiding this comment.
I suppose if we really want to demo interacting with a remote node, then that node should be run independently (i.e. in a separate process). E.g. we could add a comment to this demo that the user should first run the parameter blackboard executable (or a new python equivalent) as part of this demo.
I'm also still okay with using a single node since this is what the cpp demos do, for example.
There was a problem hiding this comment.
Using the parameter blackboard sounds like a good idea, I'll work something out for that
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
jacobperron
reviewed
Jul 22, 2022
jacobperron
reviewed
Jul 22, 2022
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
jacobperron
reviewed
Aug 9, 2022
Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
jacobperron
approved these changes
Aug 29, 2022
cardboardcode
pushed a commit
to cardboardcode/demos
that referenced
this pull request
Jan 23, 2023
* WIP: python examples for async parameter client Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * reorganize Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * linting & use tempfile Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * linting Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * put async_param_server node in async_param_client Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * address review comments Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * delete param demo with static and non-static param Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * tabs aren't valid yaml Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * decrease demo complexity Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * use parameter blackboard Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * add warning if wait_for_services fails to demo Signed-off-by: Brian Chen <brian.chen@openrobotics.org>
cardboardcode
pushed a commit
to cardboardcode/demos
that referenced
this pull request
Jan 23, 2023
* WIP: python examples for async parameter client Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * reorganize Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * linting & use tempfile Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * linting Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * put async_param_server node in async_param_client Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * address review comments Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * delete param demo with static and non-static param Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * tabs aren't valid yaml Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * decrease demo complexity Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * use parameter blackboard Signed-off-by: Brian Chen <brian.chen@openrobotics.org> * add warning if wait_for_services fails to demo Signed-off-by: Brian Chen <brian.chen@openrobotics.org> Signed-off-by: Bey Hao Yun <beyhy94@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds demos for parameter_client functionality introduced in ros2/rclpy#959