RobotWebTools / rclnodejs

Node.js version of ROS 2.0 client
https://docs.ros.org/en/humble/Concepts/Basic/About-Client-Libraries.html?highlight=rclnodejs#community-maintained
Apache License 2.0
320 stars 70 forks source link

bool and bool_array should not be converted to strings in parameter msg #850

Closed imcelroy closed 2 years ago

imcelroy commented 2 years ago

Description

Parameter bool values are currently converted to strings (for example, true becomes 'true') when using the Parameter.toParameterMessage() method. These messages aren't parsed correctly when they are received by an rclcpp node and the as_bool() and as_bool_array() methods are used.

I have tested and it seems that the conversions here(parameter.js:L227-L234) aren't necessary, and the code works as expected if removed.

Steps To Reproduce

// Convert parameter to msg const setParametersRequest: rcl_interfaces.srv.SetParameters_Request = { parameters: parameters.map(function (e) { return e.toParameterMessage(); }), };

// Send request using the client Client<'rcl_interfaces/srv/SetParametersAtomically'> or Client<'rcl_interfaces/srv/SetParameters'> client.sendRequest(setParametersRequest, (resp) => { console.log('resp: ', resp) });


- With rclcpp:

rcl_interfaces::msg::SetParametersResult parameters_callback( const vector & parameters) { rcl_interfaces::msg::SetParametersResult result; result.successful = true; for (const rclcpp::Parameter & param : parameters) { if (param.get_name() == "foo") { vector foo = param.as_bool_array(); RCLCPP_INFO_STREAM( get_node()->get_logger(), "Received foo : " << foo[0] << " " << foo[1] << " " << foo[2]; } return result; }



**Expected Behavior**
`1 0 1` should be outputted in the terminal.

**Actual Behavior**
`1 1 1` is actually outputted in the terminal.
imcelroy commented 2 years ago

@minggangw @wayneparrott Thanks for accepting the PR. Any chance we could get this fix in a new npm release asap?

minggangw commented 2 years ago

I think we will have a new release targeting against the latest Humble Hawksbill soon.

imcelroy commented 2 years ago

@minggangw Ok great, will the release targeting Humble still be compatible with Galactic?

minggangw commented 2 years ago

@imcelroy Unlike the rclcpp/rclpy clients that will be released with the ROS2 packages, so you don't need to worry about the compatibility. rclnodejs is a stand-alone nodejs package, and as you know ROS2 has many releases in parallel and their patch releases, which makes it difficult to maintain the compatibility. We did try to leverage e.g., build flag, to cover different ROS2 releases, but usually, we only verify the binary with our tests on the latest stable ROS2 due to limited resources.

imcelroy commented 2 years ago

Thank you for the explanation, that makes sense.