Servo: Ensure yaml param types are correct #427

AndyZe · 2021-04-17T18:31:10Z

Avoid warnings like this:

InvalidParameterTypeException(moveit_servo.collision_check_rate): parameter 'moveit_servo.collision_check_rate' has invalid type: expected [double] got [integer]

tylerjw

Thankyou for doing this! Simple change that will clean warnings out of the logs.

AndyZe · 2021-04-18T18:06:10Z

@tylerjw is the CI failure here the typical Servo "flaky test" that you've been seeing recently?

I think so, because BuildAndTest / foxy-ci-testing + clang-tidy passed but BuildAndTest / foxy-ci + ccov did not. It also passes locally.

v4hn

In MoveIt I introduced parseDouble two years ago to solve this exact use-case with ROS parameters.
Apparently this was discarded in the transition (because ROS2 does not use xmlrpc for parameter handling?).

The problem is still the same though and I would propose to add an appropriate method back and use it throughout the code base (apparently servo could use it).
If you fix it in our configurations, you will just mask the problem and users will still run into it for no reason.

codecov · 2021-04-18T22:02:47Z

Codecov Report

Merging #427 (e7c5e5e) into main (2bd2261) will increase coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #427      +/-   ##
==========================================
+ Coverage   51.98%   52.00%   +0.02%     
==========================================
  Files         223      223              
  Lines       23342    23340       -2     
==========================================
+ Hits        12132    12135       +3     
+ Misses      11210    11205       -5

Impacted Files	Coverage Δ
moveit_ros/moveit_servo/src/servo_parameters.cpp	`64.92% <0.00%> (-2.32%)`	⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp	`56.11% <0.00%> (-0.12%)`	⬇️
moveit_ros/moveit_servo/src/pose_tracking.cpp	`76.48% <0.00%> (+0.59%)`	⬆️
...meterization/work_space/pose_model_state_space.cpp	`81.14% <0.00%> (+0.63%)`	⬆️
.../ompl_interface/src/detail/constrained_sampler.cpp	`59.46% <0.00%> (+16.22%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bd2261...e7c5e5e. Read the comment docs.

AndyZe · 2021-04-19T00:01:31Z

The other option is to put the cast functionality into rosparam_shortcuts.

v4hn · 2021-04-19T07:52:32Z

The other option is to put the `cast` functionality into `rosparam_shortcuts`.

Sounds like a good idea. I think even rclcpp should implement that fallback, but the next best option would probably be rosparam_shortcuts.

henningkayser · 2021-04-19T09:26:26Z

In MoveIt I introduced parseDouble two years ago to solve this exact use-case with ROS parameters.
Apparently this was discarded in the transition (because ROS2 does not use xmlrpc for parameter handling?).

The problem is still the same though and I would propose to add an appropriate method back and use it throughout the code base (apparently servo could use it).
If you fix it in our configurations, you will just mask the problem and users will still run into it for no reason.

I understand your frustration but actually this problem is actually an improvement imo. There has been a push to add variant types and implicit casts to rclcpp which have been rejected for a reason. A double is declared as double and an int is declared as int, otherwise you end up having arbitrary number types in configs which make parameters even more confusing (unless each parameter has has the type commented inside the yaml file). The exception is also very clear and trivial to fix. I'm all for adding an implicit cast function to rosparam_shortcuts but would not support using it as a convention throughout the code base.

v4hn · 2021-04-19T13:51:50Z

I understand your frustration but actually this _problem_ is actually an improvement imo. There has been a push to add variant types and implicit casts to rclcpp which have been rejected for a reason

Sure it's not a bug, it's a feature of the superior new framework. ;) The feature, no implicit cast from int to double of user-provided values, is the same in ROS though and we added a workaround for it. The reasons you cite were "we don't want it" and "it requires some significant refactoring to do correctly throughout the API". I don't expect this ever to happen in rclcpp and it sounds like a good idea to provide the helper with rosparam_shortcuts.

A double is declared as double and an int is declared as int

Agreed. But the parameter should be *declared* on the code side and the documentation, not in the user-provided configuration. This also seems to be part of the issue in supporting it in the auto-supporting rclcpp API.

The exception is also very clear and trivial to fix.

Sure, you just have to fix it every time you write `0` when the system expects `0.0`. It's just another useless pitfall and I guess it's only my robotics systems that are full enough of those.

I'm all for adding an implicit cast function to `rosparam_shortcuts` but would not support using it as a convention throughout the code base.

I'm all for cleaning the example configurations to use the correct types (because they are actually part of the documentation), once it's actually supported to pass number literals without a ".0" suffix in.

AndyZe · 2021-04-19T14:35:55Z

I'll make a PR to ensure the types are handled well in rosparam_shortcuts 👍

tylerjw · 2021-04-19T19:11:02Z

I'm all for cleaning the example configurations to use the correct types (because they are actually part of the documentation), once it's actually supported to pass number literals without a ".0" suffix in.

Right now there is a warning when it does the implicit conversion. What more do we want? I don't think it is a good idea to have implicit conversions without a warning. If the type should be an integer, it should be declared that way.

I think this PR is a good change and we should fix our parameters to have explicit types and where that type is implied based on a literal we should include the .0 so it is obvious to anyone reading the config that this is a floating point value and not an integer.

@AndyZe the failure you are seeing here is the "flaky test" issue. Re-run this a few times and you'll see that. I also have not reproduced this failure locally, although I haven't put much effort into it. There is a chance it could be reproduced locally.

v4hn · 2021-04-19T20:15:06Z

I think this PR is a good change

Yes.

Right now there is a warning when it does the implicit conversion. What more do we want?

For starters it would be nice to have a warning that does not look like the value is ignored. ;) Bust mostly, to have [this logic](https://github.com/ros-planning/moveit2/blob/main/moveit_ros/moveit_servo/src/servo_parameters.cpp#L69) in a reusable place and not reinvent the wheel wherever we run into it. Also to prevent errors. Looking at the code in detail, does this actually behave correctly? To me it looks like the parameter will actually be *declared* int in the node if it is specified like that in the configuration. Doesn't that mean that users can never set a dynamic parameter declared like that to double values at runtime?

AndyZe · 2021-04-19T20:21:23Z

I agree with @v4hn -- Servo shouldn't be using unique parsing code that was put together in a rush for GSoC. I also have my doubts about that parsing function.

tylerjw · 2021-04-19T21:00:17Z

I've been working with Boston to create a standard way of doing ros2 parameters in the rosparam_shortcuts package. If we could move whatever we want for this there and servo could just use it, that would be great.

AndyZe · 2021-04-20T14:14:30Z

I tested that the Servo parsing code casts the int's to doubles correctly. It does seem to work! So I'm going to remove the warning for int->double only. I think this is a good fix for now and I'll make an issue to standardize Servo parsing code in the future.

This is my debug code:

  catch (const rclcpp::exceptions::InvalidParameterTypeException& e)
  {
    RCLCPP_WARN_STREAM(logger, "InvalidParameterTypeException(" << param_name << "): " << e.what());

    // Catch a <double> parameter written in the yaml as "1" being considered an <int>
    if (std::is_same<T, double>::value)
    {
      node->undeclare_parameter(param_name);
      output_value = node->declare_parameter<int>(param_name, 0);
      RCLCPP_ERROR_STREAM(logger, "Param name: " << param_name);
      RCLCPP_ERROR_STREAM(logger, "Param value: " << output_value);
      RCLCPP_ERROR_STREAM(logger, "Param type: " << typeid(output_value).name());
      RCLCPP_ERROR_STREAM(logger, "===");
    }

…oveit2 into andyz/servo_param_types

tylerjw

I looked more carefully at the up-conversion code and thought of something that could go wrong as a result of it. I now think it is maybe a bad idea at all because we are not just accepting an int we are changing the type that it is declared as.

moveit_ros/moveit_servo/src/servo_parameters.cpp

tylerjw · 2021-04-20T17:03:58Z

moveit_ros/moveit_servo/src/servo_parameters.cpp

+      {
+        node->undeclare_parameter(param_name);
+        // Implicitly cast to double
+        output_value = node->declare_parameter<int>(param_name, 0);


The problem I see with doing this is that if we use the dynamic parameter system and someone initially hits this because their yaml has an int in it then later they use the parameter service to update with a double it will fail to set the second time.

I think we should maybe only catch the exception to print a useful error message and exit.

OK, I'm fine with that. That will force people to quickly ensure their yaml files are correct ;)

Done. May this be the last push I make on this silly PR ;)

That's exactly what I pointed out above @tylerjw , thanks for confirming.

My question is whether it's possible, with the current rclcpp interfaces, to get the specified integer parameter, but still declare the parameter as double. If that's not possible, it's a shortcoming of rclcpp because user-configurations should not define/restrict a node's parameter types.
If it's possible, it would be a good idea to provide that functionality with rosparam_shortcuts (and use it here among other places).

I don't understand how to accept integers but still declare the parameter as a double with the current system in ROS2. I think I now understand what you were saying up above. I'm sorry it took me a while to get here.

I hope I'm just missing something and that it is possible to still declare the parameter as a double and up-convert any integer that comes in. If that's not possible maybe we could find a way to contribute it upstream (a new "number" type maybe).

Sorry I missed your comment about dynamic parameters at first, too

Everyone's ignoring me anyway.

Ensure yaml param types are correct

1eac1c5

tylerjw approved these changes Apr 17, 2021

View reviewed changes

AndyZe changed the title ~~Ensure yaml param types are correct~~ Servo: Ensure yaml param types are correct Apr 18, 2021

AndyZe mentioned this pull request Apr 18, 2021

Ensure panda joint limits have the proper type moveit/moveit_resources#63

Merged

v4hn previously requested changes Apr 18, 2021

View reviewed changes

henningkayser approved these changes Apr 19, 2021

View reviewed changes

Merge branch 'main' into andyz/servo_param_types

69f0901

AndyZe added 3 commits April 20, 2021 09:45

Ignore the casting of int's to double's

21d622a

Merge branch 'andyz/servo_param_types' of https://github.com/AndyZe/m…

675957f

…oveit2 into andyz/servo_param_types

Update includes

52732fe

tylerjw reviewed Apr 20, 2021

View reviewed changes

Throw error if parameter type doesn't match

e7c5e5e

tylerjw approved these changes Apr 20, 2021

View reviewed changes

AndyZe merged commit 2be91d2 into moveit:main Apr 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Servo: Ensure yaml param types are correct #427

Servo: Ensure yaml param types are correct #427

AndyZe commented Apr 17, 2021

tylerjw left a comment

AndyZe commented Apr 18, 2021 •

edited

Loading

v4hn left a comment

codecov bot commented Apr 18, 2021 •

edited

Loading

AndyZe commented Apr 19, 2021

v4hn commented Apr 19, 2021 via email

henningkayser commented Apr 19, 2021

v4hn commented Apr 19, 2021 via email

AndyZe commented Apr 19, 2021

tylerjw commented Apr 19, 2021

v4hn commented Apr 19, 2021 via email

AndyZe commented Apr 19, 2021

tylerjw commented Apr 19, 2021

AndyZe commented Apr 20, 2021 •

edited

Loading

tylerjw left a comment

tylerjw Apr 20, 2021

AndyZe Apr 20, 2021

AndyZe Apr 20, 2021

v4hn Apr 20, 2021

tylerjw Apr 20, 2021

AndyZe Apr 20, 2021

Servo: Ensure yaml param types are correct #427

Servo: Ensure yaml param types are correct #427

Conversation

AndyZe commented Apr 17, 2021

tylerjw left a comment

Choose a reason for hiding this comment

AndyZe commented Apr 18, 2021 • edited Loading

v4hn left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 18, 2021 • edited Loading

Codecov Report

AndyZe commented Apr 19, 2021

v4hn commented Apr 19, 2021 via email

henningkayser commented Apr 19, 2021

v4hn commented Apr 19, 2021 via email

AndyZe commented Apr 19, 2021

tylerjw commented Apr 19, 2021

v4hn commented Apr 19, 2021 via email

AndyZe commented Apr 19, 2021

tylerjw commented Apr 19, 2021

AndyZe commented Apr 20, 2021 • edited Loading

tylerjw left a comment

Choose a reason for hiding this comment

tylerjw Apr 20, 2021

Choose a reason for hiding this comment

AndyZe Apr 20, 2021

Choose a reason for hiding this comment

AndyZe Apr 20, 2021

Choose a reason for hiding this comment

v4hn Apr 20, 2021

Choose a reason for hiding this comment

tylerjw Apr 20, 2021

Choose a reason for hiding this comment

AndyZe Apr 20, 2021

Choose a reason for hiding this comment

AndyZe commented Apr 18, 2021 •

edited

Loading

codecov bot commented Apr 18, 2021 •

edited

Loading

AndyZe commented Apr 20, 2021 •

edited

Loading