Rclcpp: Node parameters callback called five times for changes to use_sim_time

[jacobperron]

Bug report

Required Info:

• Operating System:
  
  
  
  • Ubuntu 20.04
  
  
• Installation type:
  
  
  
  • from source
  
  
• Version or commit hash:
  
  
  
  • dc7e95aaa524f758ff8ccf903698065420f9cc48
  
  
• DDS implementation:
  
  
  
  • N/A
  
  
• Client library (if applicable):
  
  
  
  • rclcpp
  
  

Steps to reproduce issue

Build the following node:

#include <iostream>
#include <memory>

#include "rclcpp/rclcpp.hpp"

int main(int argc, char ** argv)
{
  rclcpp::init(argc, argv);
  auto node = std::make_shared<rclcpp::Node>("foo_node");

  auto param_change_callback =
    [](std::vector<rclcpp::Parameter> parameters)
    {
      std::cerr << "Parameter callback" << std::endl;
      auto result = rcl_interfaces::msg::SetParametersResult();
      result.successful = true;
      return result;
    };
  auto param_callback_handle = node->add_on_set_parameters_callback(param_change_callback);

  rclcpp::spin(node);

  rclcpp::shutdown();
  return 0;
}

Run it and then set the use_sim_time parameter on it:

ros2 param set /foo_node use_sim_time true

Expected behavior

The parameter is set successfully and the registered callback is called once.

Actual behavior

The parameter is set successfully and the registered callback is called five times.

Additional information

I've experimented with a user-defined parameter, and do not experience this issue (i.e. the parameter callback is called exactly once).

[fujitatomoya]

This is because that after enabling use_sim_time, it will create subscription on topic /clock with qos_override enabled. so the following parameters will be declared after use_sim_time.

/foo_node:
  qos_overrides./clock.subscription.depth
  qos_overrides./clock.subscription.durability
  qos_overrides./clock.subscription.history
  qos_overrides./clock.subscription.reliability

i think that these are meant to be exposed to control and override the QoS setting. i am not sure if this is bug or not.

I've experimented with a user-defined parameter, and do not experience this issue (i.e. the parameter callback is called exactly once).

i guess that this is because qos_override is not enabled. we can have the same situation if user enables qos_override for publisher or subscription.

@ivanpauno friendly ping. what do you think?

[ivanpauno]

@ivanpauno friendly ping. what do you think?

I think this is not a bug, seems to be ok behavior to me

[ivanpauno]

This also will happen the first time use_sim_time is set to True, but not afterwards (because the qos overrides parameters are already declared the second time).

[fujitatomoya]

@jacobperron how do you think about this? on this, i am with @ivanpauno 's opinion.

[jacobperron]

I see, it is due to the qos overrides parameters being declared.

While I agree it's not really a bug, it might be confusing to users (like it was to me). We already have a way for users to provide a validation callback for QoS overrides, so it doesn't seem necessary to additionally trigger parameter callbacks. I think this is briefly discussed in the design doc. I'm not sure how we settled the implementation though.

[fujitatomoya]

it might be confusing to users (like it was to me).

as a matter of fact, so was I. probably these parameter events (happens only start-up time once, and read-only) should be concealed for user application, validation_callback is responsible to validate the overriding parameters at the start-up. it seems unnecessary event for user callback.

[jacobperron]

probably these parameter events (happens only start-up time once, and read-only) should be concealed for user application

I think so. This is noted in the design doc, that we might just want to avoid user callbacks for read-only parameters.

[clalancette]

I think so. This is noted in the design doc, that we might just want to avoid user callbacks for read-only parameters.

I'm mildly opposed to this. One thing you can do today is install a parameter set callback, then declare all of your parameters, and you'll get a callback for all of them. That way all of your parameter-handling code is in one place, and it is easy to add things there. If we make it so that read-only parameters don't trigger a callback, then you have to split your parameter handling code over the constructor and the callback. This is doable, but I think it is a worse experience for the user.

[jacobperron]

I'm mildly opposed to this. One thing you can do today is install a parameter set callback, then declare all of your parameters, and you'll get a callback for all of them. That way all of your parameter-handling code is in one place, and it is easy to add things there. If we make it so that read-only parameters don't trigger a callback, then you have to split your parameter handling code over the constructor and the callback. This is doable, but I think it is a worse experience for the user.

That's fair. Though in the case of qos_overrides parameters, we should probably avoid triggering user callbacks. If someone declines one of those parameter changes, the node blows up (core dump). I think this could easily happen by accident if a user isn't aware of this implementation detail.

[ivanpauno]

That's fair. Though in the case of qos_overrides parameters, we should probably avoid triggering user callbacks. If someone declines one of those parameter changes, the node blows up (core dump). I think this could easily happen by accident if a user isn't aware of this implementation detail.

It shouldn't blow up, if the parameter is rejected it's just not applied and the original qos settings will be used.
I would expect anybody setting a parameter callback to be filtering by parameter names and only check for the ones they want, and accept the others.

[ivanpauno]

I would keep triggering the on parameter set callback in all read-only parameters including parameter overrides.
You can also use the qos validation callback which is more convenient for this case, but I would still trigger the others.

[jacobperron]

It shouldn't blow up, if the parameter is rejected it's just not applied and the original qos settings will be used.

In this case, what is the original QoS? I didn't override any QoS settings, but I can reject the declared value and the node crashes.

I would expect anybody setting a parameter callback to be filtering by parameter names and only check for the ones they want, and accept the others.

I agree. I can close this particular ticket, but I'll open up a new issue related to the node crashing when I reject a qos override.

[ivanpauno]

In this case, what is the original QoS? I didn't override any QoS settings, but I can reject the declared value and the node crashes.

ah yeah, that's true.
But well, you can also declare a parameter with a default value that doesn't pass the validation callback with the same consequence.

[jacobperron]

I've opened a separate ticket describing the issue related to qos_overrides: https://github.com/ros2/rclcpp/issues/1587