Inconsistent handling of type attribute in <param command=""> tags

[jarvisschultz]

When using roslaunch, if you use a command= attribute inside of a param tag, the value of stdout is set to the parameter server as a string, regardless of the value of the type attribute. In rosmon, the value of type is read and respected (in other words, the correct behavior even though it's inconsistent with roslaunch).

example.launch

<launch>
  <param name="command_param" type="double" command="echo -n 1.23" />
</launch>

Running that with roslaunch:

jarvis@jschultz:~⟫ rosparam get /command_param
'1.23'

Running with rosmon:

jarvis@jschultz:~⟫ rosparam get /command_param
1.23

FWIW, even though the roslaunch behavior seems wrong, it is inline with the documentation. For the command attribute they say:

The output of the command will be read and stored as a string.

[xqms]

Hey, good catch - incompatibility with roslaunch is definitely a bug. I started to look into this, but don't have time to finish, so I'll keep my notes here:

I think we also need to look at <param textfile="file"> - does it have the same behavior? If yes, we can just scrap the auto-conversion code at

rosmon/rosmon_core/src/launch/launch_config.cpp

Line 783 in 273debb

return paramToXmlRpc(ctx, computeString->get(), fullType);

See also the comment further up:

rosmon/rosmon_core/src/launch/launch_config.cpp

Lines 637 to 641 in 273debb

	// Dynamic parameters are more complicated. We split the computation in
	// two parts:
	// 1) Compute the value as std::string
	// 2) Convert to desired type (or dissect into multiple parameters in
	// case of YAML-typed parameters).

Seems that type="yaml" is respected - or at least I thought so at some point ;)

I need to write a full unit test on this - probably I'll have time over the weekend.

[jarvisschultz]

Great questions. I just tried with <param textfile="file" /> and in roslaunch, just like the command attribute, it doesn't matter what the value of type is, the param is always loaded as a string. This seems to be true even if the contents of the text file are valid YAML and type="yaml". Note, the docs for textfile say the same thing as for command:

The contents of the file will be read and stored as a string

It's also worth noting that this is probably ROS version specific. The section about type="yaml" at the bottom currently says new in Lunar. I haven't dug into PRs, relevant changes, etc. but it is worth noting the computer I'm currently on is running Kinetic. Might be worth seeing if there is different behavior for some of these conclusions across ROS versions

[xqms]

It seems that this inconsistency in roslaunch has been fixed in the meantime. Starting from ROS Lunar, the type attribute is properly respected, at least in my tests:

$ docker run -it ros:lunar-ros-core
root@14354222f2d4:/# cat > test.launch
<launch>
  <param name="command_param" type="double" command="echo -n 1.23" />
</launch>
root@14354222f2d4:/# roscore &
[1] 34
[...]

root@14354222f2d4:/# roslaunch test.launch
... logging to /root/.ros/log/41ace022-328b-11eb-8895-0242ac110002/roslaunch-14354222f2d4-74.log
Checking log directory for disk usage. This may take awhile.
Press Ctrl-C to interrupt
Done checking log file disk usage. Usage is <1GB.

started roslaunch server http://14354222f2d4:37539/

SUMMARY
========

PARAMETERS
 * /command_param: 1.23
 * /rosdistro: lunar
 * /rosversion: 1.13.7

NODES

ROS_MASTER_URI=http://localhost:11311

No processes to monitor
shutting down processing monitor...
... shutting down processing monitor complete
root@14354222f2d4:/# rosparam get /command_param
1.23
root@14354222f2d4:/#

What do you think about just printing a warning when $ROS_VERSION == "kinetic" and the type attribute is used together with command or textfile? In the end, users will want to fix their launch files / param clients anyway since they will break when they update to Lunar/Melodic/Noetic.

[xqms]

... and this is the PR that changed the behavior: ros/ros_comm#1045
Commit: ros/ros_comm@df86805#diff-23538634097530a39f2541d19a84384c977a11aeead5de5956c2552cb0757c22

[jarvisschultz]

Excellent summary! Thanks for digging into the roslaunch history to find the change. I think the proposed fix of just printing a warning is sufficient. Only possible suggestion I can think of would be to update the message printed to say something like:

On ROS versions older than ROS Lunar....

Just in case someone is still using Indigo or something

[xqms]

I haven't forgotten about this - I just discovered some other inconsistencies with the param type system, which I'd like to fix at the same time. So progress on this will take some more time.

[xqms]

Finally got around to this 🎉

The warning and some other fixes for whitespaced parameters are now in master and will sit there for a few weeks for testing before I'll release a new version into the ROS repos. Thanks again for raising the issue!

[jarvisschultz]

Awesome... thanks for taking care of this! All looks good to me