search

ros2 / demos

Apache License 2.0
493 stars 329 forks source link

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

Closed Yadunund closed 1 year ago

Yadunund commented 1 year ago

As detailed here https://github.com/osrf/ros2_test_cases/issues/885#issuecomment-1539212847

clalancette commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.