Add range constraints for applicable array parameters

[agyoungs]

I noticed that array numeric parameters (integer and double) didn't have any range checking. To me it seemed that if you had an array and bounds you would simply just want each element of the array to be within those bounds. So instead of requiring a new constraint class for array types I thought it made sense to apply the constraint to each element.

Additional thoughts: Would this be limiting for potential future constraints for arrays? The only additional thing I could think of for an array would be min/max length, although I don't have an immediate use-case for that.

If there are array specific constraints to be implemented in the future, they can exist as a separate class and validated in a similar manner. I don't think this limits the ability to add array specific constraints and allows re-use of the already implemented constraint checks of the base type.

[fujitatomoya]

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

[agyoungs]

@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

[jmachowinski]

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.

[tfoote]

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?

[fujitatomoya]

@agyoungs thanks for the PR.

test covers the most cases as aligned with integer and double type 🚀

[fujitatomoya]

lgtm with green CI.

[fujitatomoya]

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

[fujitatomoya]

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

[agyoungs]

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.

[agyoungs]

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.

[fujitatomoya]

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

[fujitatomoya]

CI:

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

[tfoote]

Yeah, the prevalidation and ability to prefilter and adjust out of range values is probably too much of a corner case. Keeping it simple that it will pre-validate the ranges for a declared range is simpler to explain and makes sense and is consistent with what we were already doing for other types.

This looks a lot better with the refactored checking for the unitary types reused inside the for loops.

[fujitatomoya]

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