Closed jarvisschultz closed 3 years ago
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
https://github.com/xqms/rosmon/blob/273debb741620adc21a11583ee2d037d47be5fec/rosmon_core/src/launch/launch_config.cpp#L783
See also the comment further up: https://github.com/xqms/rosmon/blob/273debb741620adc21a11583ee2d037d47be5fec/rosmon_core/src/launch/launch_config.cpp#L637-L641
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.
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
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.
... and this is the PR that changed the behavior: https://github.com/ros/ros_comm/pull/1045 Commit: https://github.com/ros/ros_comm/commit/df86805c5b155829f736fa581424d9fe439430b9#diff-23538634097530a39f2541d19a84384c977a11aeead5de5956c2552cb0757c22
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
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.
Finally got around to this :tada:
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!
Awesome... thanks for taking care of this! All looks good to me
When using
roslaunch
, if you use acommand=
attribute inside of a param tag, the value ofstdout
is set to the parameter server as a string, regardless of the value of thetype
attribute. Inrosmon
, the value oftype
is read and respected (in other words, the correct behavior even though it's inconsistent withroslaunch
).example.launch
Running that with
roslaunch
:Running with
rosmon:
FWIW, even though the
roslaunch
behavior seems wrong, it is inline with the documentation. For thecommand
attribute they say: