use_sim_time error message is logged from multiple nodes

[jacobperron]

Bug report

Required Info:

• Operating System:
  • Ubuntu 18.04 and macOS
• Installation type:
  • source (Linux) and fat archives (macOS)
• Version or commit hash:
  • 0723a0a
• DDS implementation:
  • Fast-RTPS
• Client library (if applicable):
  • rclcpp

Steps to reproduce issue

Run the talker C++ demo:

ros2 run demo_nodes_cpp talker

Run the listener C++ demo:

ros2 run demo_nodes_cpp listener

Set the use_sim_time parameter for the talker to something other than a bool:

ros2 param set talker use_sim_time foo

Expected behavior

We see an error (or warning) logged from the talker node regarding the type mismatch for the use_sim_time parameter. E.g.

[ERROR] [talker]: use_sim_time parameter set to something besides a bool

Actual behavior

We see the above error message, not only for the talker node, but also for the listener node. I.e. both the talker and the listener log the following error message:

[ERROR] [talker]: use_sim_time parameter set to something besides a bool

Additional information

We can see that the parameter is set on the talker node, but not on the listener node, which is correct. The bug is that we see the listener log an error when it is not expected.

[hidmic]

So, it looks like rclcpp::TimeSource is listening to changes to use_sim_time system wide (see here). Judging by #609, this subscription serves no specific purpose and could/should be replaced by an on_set_parameters callback. Is that right @wjwwood ? If so, I'll do so.

[ivanpauno]

So, it looks like rclcpp::TimeSource is listening to changes to use_sim_time system wide (see here). Judging by #609, this subscription serves no specific purpose and could/should be replaced by an on_set_parameters callback. Is that right @wjwwood ? If so, I'll do so.

on_set_parameter is for rejecting/accepting a parameter.
It shouldn't be used for updating a "global" state.
See discussion here #772 (comment).
#609 is asking for a local "on_parameter_change" callback, but we currently don't have that feature.

At the moment, using on_parameter_event is the only correct way of doing this.
The fully qualified node name is part of the Parameter Event message.
It should be used for filter nodes out.

[hidmic]

It shouldn't be used for updating a "global" state.

Fair enough.

The fully qualified node name is part of the Parameter Event message.

Yes, I can work with that. Just checking if there's a better way to do so.

[hidmic]

@jacobperron #799 should've addressed your issue.

[jacobperron]

@jacobperron #799 should've addressed your issue.

Works for me 👍