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

Do not use "off" for client_configure_introspection param in service introspection client #625

Closed
Yadunund opened this issue May 11, 2023 · 3 comments · Fixed by #634
Closed
Assignees
Labels
enhancement New feature or request

Comments

@Yadunund
Copy link
Member

As detailed here osrf/ros2_test_cases#885 (comment)

@Yadunund Yadunund added the enhancement New feature or request label May 11, 2023
@clalancette
Copy link
Contributor

I have to disagree with this one. The underlying data structures use the terminology off, metadata, content, so I think this demo should use the same thing.

This issue isn't specific to this demo; basically any value that YAML considers to be "false" will be considered a boolean.

As @jacobperron pointed out, there are actually a couple of different workarounds for this. One is to quote it:

ros2 param set /introspection_client client_configure_introspection \"off\"

Another option is to use the explicit string configuration from YAML:

ros2 param set /introspection_client client_configure_introspection '!!str off'

Maybe we should just make this explicit in the test case instead?

@jacobperron
Copy link
Member

Yeah, it would be nice to use the same terminology. I'm torn, since it would be nice to avoid this quirk of the demo. I suppose it's not a big deal since it's just a demo.

If someone wanted to make this feature configurable during runtime (like the demo) in their own application (or as part of the core ROS implementation), what could we suggest? It seems like "off" might be a poor name choice given how integrated YAML is in ROS.

@clalancette
Copy link
Contributor

After talking about this in Waffle, we decided that we should probably change this to disabled instead of off to avoid the problem. I'll submit a PR in the future to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants