enable parameter update recursively only when QoS override parameters.

[fujitatomoya]

closes #2741

[wjwwood]

I'm still surprised that this is needed. I see that we explicitly tell people not to change parameters in the post change callback:

rclcpp/rclcpp/include/rclcpp/node.hpp

Line 1184 in 605251b

* This callback should not modify parameters.

But I don't remember why. Do you have a good idea why this was the case @fujitatomoya? I really think we should understand this before we workaround it.

[fujitatomoya]

@wjwwood

But I don't remember why. Do you have a good idea why this was the case @fujitatomoya?

no i do not have any clue on this. i guess this has been here for a long time before i know that...

[fujitatomoya]

Summary:

• add_pre_set_parameters_callback and add_on_set_parameters_callback should never change (set/declare/undeclare) the parameter recursively because this could generates the parameter data base inconsistency. (e.g following callbacks might deny the parameter operation after the recursive parameter changes.)
• add_post_set_parameters_callback can change the parameter without parameter data base inconsistency explained above. but it could generate the infinite loop in the post set callbacks if the parameters are cross-referenced. to avoid this problem, it should avoid recursive parameter changes in default.
• we could have another thread to get this parameter operation done from the parameter callbacks in general use. but introducing another thread in the system would generate complication and racy condition. besides, operation could not be synchronized with callback completion.
• we could have the internal special APIs to work-around this particular problem when creating publisher and subscription with QoS override parameters possibly around rclcpp::detail::declare_qos_parameters. but it could be expected that during post set parameter callback, the application needs to change the parameter recursively, these special methods are not available for these general use cases.
• we can provide formalized API to explicitly allow the recursive parameter operation during parameter callback. (current PR proposal) in this case either system or user can control the parameter operation recursively, but they need to understand what they are doing once it is enabled. at least, API documentation should be more described about possible problems and consequences.
• Note: https://github.com/fujitatomoya/ros2_test_prover/blob/master/prover_rclcpp/src/create_sub_via_param.cpp is actually anti-pattern with using add_on_set_parameters_callback, it should be explained with add_post_set_parameters_callback instead.

[fujitatomoya]

@jmachowinski @alsora @mjcarroll any opinions? i would like to have this fix, so that we can have #2378

[alsora]

Would it be possible to add some unit-tests to show that the changes work?

[fujitatomoya]

@alsora yeah, i will consider the test. thanks!

[fujitatomoya]

@alsora @jmachowinski test and docstring are added, can you take a look?

[fujitatomoya]

Pulls: #2742
Gist: https://gist.githubusercontent.com/fujitatomoya/a54329825b20833084b9dff16cd9badf/raw/45adffa43e80fc56bd41ee9dc43cc8ded5beef7f/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15628

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[fujitatomoya]

@jmachowinski just added docstring a bit more, could you check again? thanks for the review!

[fujitatomoya]

Pulls: #2742
Gist: https://gist.githubusercontent.com/fujitatomoya/cbe2527f600e83296e014f4ceae60b40/raw/45adffa43e80fc56bd41ee9dc43cc8ded5beef7f/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15634

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[pele1410]

Can this change be backported to Jazzy (or has it already been and just not released)?

I've got a PR in nav2 that needs this fix: ros-navigation/navigation2#4961

[jmachowinski]

@pele1410 No, it is ABI breaking and can therefore not be directly back ported.
You could perhaps create a non ABI breaking version of this by making enable_parameter_modification not part of the interface (and non virtual) and using an upcast in the declare_parameter_or_get function.