ros/ros_comm

rosparam: C++ / Python inconsistency

Closed this issue · 4 comments

From answers.ros.org:

Python will work with rosparam set use_sim_time 1 but C++ needs rosparam set use_sim_time true. Maybe there should be a warning about this on the wiki or at run time.

Can we fix this somewhere?

Any code which reads a parameter must convert it according to some logic. E.g. for boolean consider "true", "1", (what ever else) as true. Please consider providing a pull request to make all locations where use_sim_time gets read uses that kind of logic.

I would assume that C++ should be the easier part since its type will already ensure that it reads a boolean value. May be the Python part is not setting a boolean value in the first place but a string / integer instead? Then it should convert the value to be bool before setting it on the parameter server (else C++ will not interpret it like that:

if (v.getType() != XmlRpc::XmlRpcValue::TypeBoolean)
).

Since this seems to be a usage problem of the API and there is no follow up on this I will close the ticket for now. Please consider to comment with further information or provide pull request to the code which uses the API incorrectly (referencing this here would help for trackability).

I do not have time or interest in fixing this, I just wanted to point out the problem.

peci1 commented

This behavior really is misleading if you code in Python, and then jump in C++ for a while. From Python, the user might have learned that setting bool parameters to 1 works. Then he reads the same parameter as a bool in C++, and boom, he gets nothing, as if the parameter weren't set at all (well, hasParam returns true, but getParam returns false and param always uses the default value).

Consider this:

rosparam set /param true  # results in "true value" in Python and C++
rosparam set /param True  # results in "true value" in Python and C++
rosparam set /param 1  # results in "true value" in Python and false in C++

This is IMO caused by the type autoconversions Python does and by the Python API, which can't know in advance that it searches for a boolean (which the C++ API knows). So Python just finds the integer value, and then casts it to bool, which is fine. In C++, there is this typecheck added, which doesn't even try to cast the value to boolean.

My suggestion would be to introduce a special check in the C++ code, that would cast integer 0 and 1 to the appropriate bools (consistently with other parts of ROS, like roslaunch or xacro). And it'd also solve all these issues like different behavior of /use_sim_time in C++ and Python when set to 1.

Roslaunch example: roslaunch treat 1 as true in arguments. So if you do e.g.:

<launch>
    <arg name="use_sim_time" default="true" />
    <param name="/use_sim_time" value="$(arg use_sim_time)" />
</launch>

your code won't work in C++ if the user launches with roslaunch ... use_sim_time:=1. The code can be protected by writing:

<launch>
    <arg name="use_sim_time" default="true" />
    <param name="/imitate_real_robot" value="true" if="$(arg imitate_real_robot)" />
    <param name="/imitate_real_robot" value="false" unless="$(arg imitate_real_robot)" />
</launch>

which is IMO non-intuitive.

I see that there could be a few cases where the suggested change would break things, but then I'd say the broken code is anyway wrongly written (so that it e.g. sets a param to 1 in Python and reads it as boolean in C++ and they don't care it doesn't work).