Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throws wrong type error while parsing ompl_planning.yaml #184

Closed
jrgnicho opened this issue Apr 29, 2020 · 8 comments
Closed

Throws wrong type error while parsing ompl_planning.yaml #184

jrgnicho opened this issue Apr 29, 2020 · 8 comments
Labels
bug Something isn't working

Comments

@jrgnicho
Copy link
Contributor

Description

In loading the config yaml files for MoveItCPP in my application,
an incorrect yaml type error is thrown when parsing the ompl_planning.yaml file

Overview of your issue here.
I don't have a screen capture of the actual error message but it seemed to suggest that it expected a string but got a double instead. I was able to get it to accept the yaml file by placing quotes around the value assigned to longest_valid_segment_fraction field in order to make it a string, see here

Your environment

  • ROS Distro: [Eloquent]
  • OS Version: e.g. Ubuntu 18.04
  • Source
  • master branch

Steps to reproduce

Load ompl_planning.yaml file which includes the longest_valid_segment_fraction set to a double

Expected behaviour

MoveItCPP should load the yaml file and configure itself from it

Actual behaviour

Throws exception saying that it expected a string but got a double instead

Backtrace or Console output

Did't capture :(

@jrgnicho jrgnicho added the bug Something isn't working label Apr 29, 2020
@JafarAbdi
Copy link
Member

You're right, it's a bug in ompl_interface.cpp#L181 it loop over the parameters' name and expect string for all of them which isn't the case here

In ROS1 calling a getParam with a different type return false, in ROS2 it throws an exception

@jrgnicho
Copy link
Contributor Author

I see, that'll be tricky to get to work right if it throws an exception

@JafarAbdi
Copy link
Member

@jrgnicho How about adding a map of KNOWN_GROUP_PARAMS and ParameterType and getting the parameter that matches the parameter type .?

@ddengster
Copy link
Contributor

ddengster commented Apr 30, 2020

rclcpp::Parameter param = node_->get_parameter(group_name_param + "." + k);
        if (param.get_type() == rclcpp::ParameterType::PARAMETER_STRING)
        {
          std::string value = param.get_value<std::string>();
          if (!value.empty())
            specific_group_params[k] = value;
          continue;
        }
        else if (param.get_type() == rclcpp::ParameterType::PARAMETER_DOUBLE)
        {
          double value_d = param.get_value<double>();
          // convert to string using no locale
          specific_group_params[k] = moveit::core::toString(value_d);
          continue;
        }
        else if (param.get_type() == rclcpp::ParameterType::PARAMETER_INTEGER)
        {
          int value_i = param.get_value<int>();
          specific_group_params[k] = std::to_string(value_i);
          continue;
        }
        else if (param.get_type() == rclcpp::ParameterType::PARAMETER_BOOL)
        {
          bool value_b = param.get_value<bool>();
          specific_group_params[k] = std::to_string(value_b);
          continue;
        }

Something like this should work.

sidetrack: I have a working move_group node, i'm a bug away from a working version (and a draft PR). If you happen to be fixing a bug that occurs with the srdf injesting the joint values of a group_state with the wrong joint order let me know if you've found a solution. turns out the solution was to port joint reordering functionality over in joint_trajectory_controller. I've integrated it into the pr.

@henningkayser
Copy link
Member

Something like this should work.

Probably, but in general I would prefer either a template function (instead of explicit type checks) or introducing a convention that forbids variable types. I also thought that there are already some implicit conversions available, at least for double and int.

sidetrack: I have a working move_group node, i'm a bug away from a working version (and a draft PR). If you happen to be fixing a bug that occurs with the srdf injesting the joint values of a group_state with the wrong joint order let me know if you've found a solution.

That's interesting, could you start an issue on that?

@jrgnicho
Copy link
Contributor Author

rclcpp::Parameter param = node_->get_parameter(group_name_param + "." + k);
        if (param.get_type() == rclcpp::ParameterType::PARAMETER_STRING)
        {
          std::string value = param.get_value<std::string>();
          if (!value.empty())
            specific_group_params[k] = value;
          continue;
        }
        else if (param.get_type() == rclcpp::ParameterType::PARAMETER_DOUBLE)
        {
          double value_d = param.get_value<double>();
          // convert to string using no locale
          specific_group_params[k] = moveit::core::toString(value_d);
          continue;
        }
        else if (param.get_type() == rclcpp::ParameterType::PARAMETER_INTEGER)
        {
          int value_i = param.get_value<int>();
          specific_group_params[k] = std::to_string(value_i);
          continue;
        }
        else if (param.get_type() == rclcpp::ParameterType::PARAMETER_BOOL)
        {
          bool value_b = param.get_value<bool>();
          specific_group_params[k] = std::to_string(value_b);
          continue;
        }

Something like this should work.

sidetrack: I have a working move_group node, i'm a bug away from a working version (and a draft PR). If you happen to be fixing a bug that occurs with the srdf injesting the joint values of a group_state with the wrong joint order let me know if you've found a solution.

That should work but I'm with @henningkayser on preferring template functions

@ddengster
Copy link
Contributor

ddengster commented May 14, 2020

That's interesting, could you start an issue on that?

turns out the solution was to port joint reordering functionality over in joint_trajectory_controller. I've integrated it into the jtc pr.

@henningkayser
Copy link
Member

That's interesting, could you start an issue on that?

turns out the solution was to port joint reordering functionality over in joint_trajectory_controller. I've integrated it into the jtc pr.

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants