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.

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

[image_tools] Use ROS parameters instead of regular CLI arguments #398

Merged
merged 6 commits into from
Oct 17, 2019

Conversation

jacobperron
Copy link
Member

Using of ROS parameters make the code a little tidier, though a little more verbose on the command-line.
IMO, it seems like a good way to demonstrate best practices as well.

Example usage:

$ ros2 run image_tools cam2image --ros-args -p burger_mode:=true -p reliability:=reliable
$ ros2 run image_tools showimage --ros-args -p reliability:=reliable

Connects to #390

Using of ROS parameters make the code a little tidier, though a little more verbose on the command-line.
IMO, it seems like a good way to demonstrate best practices as well.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@wuffle-ros wuffle-ros bot added the in review Waiting for review (Kanban column) label Oct 15, 2019
@dirk-thomas
Copy link
Member

How does a user get a similar help than what was provided by --help before?

@jacobperron
Copy link
Member Author

jacobperron commented Oct 15, 2019

How does a user get a similar help than what was provided by --help before?

Good question, I guess there's no equivalent to --help.

Maybe this is a good example for a feature/tool that is lacking for ROS parameters.
At the moment, you can list parameters for the node, but only after it is running:

$ ros2 param list
/showimage:
  depth
  history
  reliability
  show_image
  use_sim_time

In addition it would be nice if we could see the types/constraints on these params (ros2/ros2cli#362).

And even better (equivalent to --help) would be to introspect the node before actually running it. I imagine some like

$ ros2 run image_tools showimage --ros-args --help
Node: name_of_node
  Parameters:
    <list of parameters and descriptors>
  Publishers:
    <list of publisher types and topics>
  Subscriptions:
    <list of subscription types and topics>
... etc for services and actions.

Node: another_node
   ... etc

I suppose this kind of introspection feature needs a lot more thought and discussion to make it happen.

@dirk-thomas
Copy link
Member

Not relevant in this case where the node is implemented in C++. But in the case of Python nodes using argparse another major advantage is getting free argcomplete functionality. So switching a Python node to parameters would loose that too which I think would be undesired.

Display a help message and exit if the non-ros flag '-h' or '--help' is given.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

I've added back an option for help (-h / --help): da736d0

Really, the 'execute' method is doing initialization.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron jacobperron added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Oct 17, 2019
Avoid blocking on construction by using a timer for the main loop.
This resolves an issue where we are unable to query parameters from the command line.
I've also moved initialization logic into it's own method, similar to showimage.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Use Node actions and parameters.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron
Copy link
Member Author

jacobperron commented Oct 17, 2019

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

@jacobperron jacobperron added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Oct 17, 2019
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

I was hoping the nodes would declare their parameters and --help would then call a generic function which get those descriptors and renders the usage.

Since the usage text was hard coded before it is certainly good enough for this PR to also hard code them.

The one inline comment doesn't need to be addressed - the same was already the case before this PR.

std::find(args.begin(), args.end(), "-h") != args.end())
{
std::stringstream ss;
ss << "Usage: cam2image [-h] [--ros-args [-p param:=value] ...]" << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: each std::endl also flushes the stream which doesn't seem necessary on each line.

@jacobperron
Copy link
Member Author

jacobperron commented Oct 17, 2019

I was hoping the nodes would declare their parameters and --help would then call a generic function which get those descriptors and renders the usage.

It seemed easier to do something like was done before to ensure formatting looked okay.
But, this is a nice idea. I think that a generic function, like get_node_usage(), should live in the client library or a utility somewhere that node developers can use. I can open up a feature request for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants