Handle dynamic reconfiguration of parameters

[jeaninevbrr]

Hey,

We wanted to be able to tweak filter parameters dynamically. So we added the functionality to handle parameter updates in the FilterChain. I did have to make parameters readable for that. I don't know if there is a specific reason to make them read_only?

[jonbinney]

Very cool! I remember that last time there were some complexities to which parameters can be read only, but that might have been just for the name and type. I'll take a deeper look at this tonight.

[jeaninevbrr]

Thanks, just FYI: This PR on laser_filters is kind of related

[jonbinney]

OK life got in the way today.... will check on this tomorrow.

[jonbinney]

I remember there was some subtlety about which params could/should be readable last time I was working on this code, so I need to dive into this a bit more and make sure I understand it. Hopefully will have time this weekend.

[jonbinney]

I'll do a more detailed review tomorrow, but I have a couple of initial thoughts.

1. Is "OnSetParameter" the right kind of callback (not saying it isn't, but it's something to think through)? In newer ROS2 versions there are Pre-set and post-set callbacks which allow the node to modify the changes that are going to happen, or to respond aftewards. In theory on-set-parameter callbacks shouldn't have any side effects: https://docs.ros.org/en/rolling//Concepts/Basic/About-Parameters.html#parameter-callbacks
2. I think that modify-able parameters should be opt-in, with a is_writeable option to getParam that defaults to false. Not all filters will know how to respond to changing parameters.

[jeaninevbrr]

Hey Jonathan,

Thanks for your feedback.
About point 1: I think you are right and the "add_post_set_parameters_callback" would be more suitable in this case, because we never reject a parameter change. Unfortunately, this function is not available in Humble. There probably "on_parameter_event" should be used. Choosing one or the other would break your ros2 branch for some users anyways. I recommend keeping the current on_set_parameter_callback, do you agree?

About point 2: I added this functionality, however as read_only, to comply to naming in the parameter description.

[jonbinney]

I think it's worth using the right callbacks for each distro, although it would be a bit more work. We ran into a similar issue in the laser_filters package with matched_callbacks (ros-perception/laser_filters#195), which weren't supported in humble. The solution was to define a compile flag based on the version of rclcpp and use that to enable or disable the feature.

How does that approach sound? We would use "post set parameter callback" on versions where rclcpp has them, and "on set parameter callback" on versions where it doesn't (humble).

[jeaninevbrr]

I looked a bit into this approach. It is possible off course, but it is going to result in quite a lot of #ifdefs in this code and causes a breaking change for all the implementations of the filter_base class. That counts for laser_filters, but possibly people's own implementations. You already use the

• If we use "on_parameter_event" in humble, we need an extra interface as input of the class, because it is a function of the parameter client api and not the node api. This would mean a breaking the signature of the configure function. We could consider using on_set_parameter_callback for humble still, so we can use the parameter interface from the node api. But that would break the whole sense of this change right. Because it is officially not the right function for it.
• Because "on_parameter_event/on_set_parameter_callback" and "add_post_set_parameters_callback" have different signatures the definition of the callback_handle_ will be different.
• Binding the reconfigure callback to the callback handle will be different of course.
• Because of the different signatures we also need two different definitions of the bound reconfigureCB. This is the most annoying change, because all the implementations of the virtual reconfigureCB will break. For example, you use the on_set_parameter_callback signature already in laser_filters

Knowing this impact, do you still want to implement it this way? If you do, i can add it to this PR, and make one extra for laser_filters. But for code readability and for the sake of backwards compatibility i still rather keep on_set_parameter_callback for all ros2 versions.

[jonbinney]

Yes, I think you're right. I'll plan to update the callback type next time we have a new ros2 distro; at that point we'll probably want to branch for humble anyway.

[jonbinney]

Was this the last PR you had for filters? If so I'll do a release and we can start on the laser_filters PRs :-)

[jeaninevbrr]

Yes, I think this it it :)