@agyoungs this sounds reasonable. thanks for creating PR 👍 is this good to review?

@fujitatomoya Yeah, it's ready for review. I noticed one of the linting checks failed so I was going to refactor the test cases into a separate function, but I thought that might make it less apparent what changed for the review.

I'm happy to resolve the linting failure in whatever way is preferred

I have one minor remark, the '__check_parameter_value_in_range' function is now too long in my opinion. I would split it up, and have one function per parameter type.

This looks reasonable to me. I agree with working to reduce the length/complexity of the calls with some modularity/reuse of the single checks within the array.

For more complicated checks or application specific checks with recovery logic we do have validation callbacks that can be registered on the parameter provider. I see one limitation that this would provide is that you couldn't provide application specific logic such as choosing to normalize the change instead of rejecting the change that you would be able to do in the custom validation callback. It might make sense to apply this default constraint only if there's not a custom validation callback?

@jmachowinski i will go ahead to start CI. please have an another look.

Pulls: #2828
Gist: https://gist.githubusercontent.com/fujitatomoya/d3cff3b725362f235b171e04478654b0/raw/0f5c48eb7b82f1ac7f88ca7d2b6ba3b334cf9330/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/15868

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

For more complicated checks or application specific checks with recovery logic we do have validation callbacks that can be registered on the parameter provider. I see one limitation that this would provide is that you couldn't provide application specific logic such as choosing to normalize the change instead of rejecting the change that you would be able to do in the custom validation callback. It might make sense to apply this default constraint only if there's not a custom validation callback?

@tfoote if I'm understanding correctly, the existing snippet below is relevant.

  // Check if the value being set complies with the descriptor.
  rcl_interfaces::msg::SetParametersResult result = __check_parameters(
    parameter_infos, parameters, allow_undeclared);
  if (!result.successful) {
    return result;
  }
  // Call the user callbacks to see if the new value(s) are allowed.
  result =
    __call_on_set_parameters_callbacks(parameters, on_set_callback_container);
  if (!result.successful) {
    return result;
  }

I'm not 100% sure if you're referring to a specific situation, but I see a few cases here:

• Parameter does not set the range constraint
  • Non-issue, this code won't affect anything regardless if a parm check callback is registered
• Parameter sets range constraints and registers param check callback
  • Callback doesn't use the range check for anything
    • This is probably just a backwards compatibility concern. Potentially there's some code out there that accidentally sets the range constraints and updates will now fail.
    • Is there any concern here?
• Callback was using the range check to do its own custom check that is different than a per element range check
  • I'm assuming this is the case you're most interested in
  • I would guess that it's pretty non-standard for the callback to modify the parameters within the nodespace. The params are a const reference so any modifications it makes would have to be a copy and local only to the node. Unless a custom callback was editing the list and then calling another param update with the modified values immediately after. Or maybe I'm missing some additional feedback the custom param check callback can provide?
  • And if it's not modifying the parameters locally, then maybe it's simply performing a range check that's different than my interpretation

So the last major bullet above has a couple cases that would definitely interfere with a custom callback only if that callback was expecting to use the range constraints for something else. You mentioned we could potentially disable the internal check if the parameter has a param check callback. I'm assuming that this would only apply to the array parameters. The limitation here is that one would have to implement their own range check (assuming this becomes a standard internal check) and then whatever custom check they wanted to perform on the array if they want both the standard check and their own custom check.

In my opinion this feels more like an override, and if all the parameters aren't subjected to a user defined override then probably none of them should be. You currently can't do a custom range check (using the range constraint) on a PARAMETER_DOUBLE or a PARAMETER_INTEGER, right? I think that's because those checks are so obvious.

So I think the question is, is this considered an obvious check? If not, it probably doesn't make sense to make it a standard internal check.

I have one minor remark, the '__check_parameter_value_in_range' function is now too long in my opinion. I would split it up, and have one function per parameter type.

I didn't read this closely enough. I didn't really break up the __check_parameter_value_in_range into a check for each type. It still calls the check for each of the 4 types (integer, integer_array, double, double_array), however the code doing the checks has been moved into a separate function (one for an integer range check and one for a double range check). I think this eliminates your core concern of the function being too long.

CI looks like got cut off, i will restart the CI.

CI:

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

@agyoungs cpplint is not happy, can you fix that?

@agyoungs cpplint is not happy, can you fix that?

I went to refactor the test since it complained it was too long. But then I found a few superfluous test cases that brought the line count back within the required range. I think that fixed it, although I still see a failing check although I'm not sure what's failing

Pulls: #2828
Gist: https://gist.githubusercontent.com/fujitatomoya/415fd0bdcd45e7afd345d3b4597f7988/raw/0f5c48eb7b82f1ac7f88ca7d2b6ba3b334cf9330/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/15897

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

Do we need to backport this to other ROS versions?

we can backport this but this changes the behavior, maybe it should stay in rolling?