Closed romainreignier closed 3 years ago
To show the issue, this is a script that shows the hexa output of a simple application started manually or with rosmon. It can be ran in a Docker image, only to get a clean environment:
#!/bin/bash
apt update -q -q
apt install -q -q -y --no-install-recommends ros-$ROS_DISTRO-rosmon-core xxd python-catkin-tools psmisc
source /opt/ros/$ROS_DISTRO/setup.bash
roscore &
mkdir -p /ws/src
cd /ws/src
catkin create pkg hello
mkdir hello/launch
cd hello/launch
cat << EOF > hello.launch
<launch>
<node name="hello" pkg="hello" type="hello"/>
</launch>
EOF
cd /ws
catkin build
source devel/setup.bash
cat << EOF > hello.cpp
#include <iostream>
int main()
{
std::cout << "hello world\n";
return 0;
}
EOF
g++ -o hello hello.cpp
mkdir devel/lib/hello
cp hello devel/lib/hello
echo "### bare cout ###"
./hello | xxd
echo ""
echo "### with rosrun ###"
rosrun hello hello | xxd
echo ""
echo "### with rosmon ###"
(sleep 4 && killall rosmon)&
mon launch --disable-ui --flush-stdout hello hello.launch | xxd
#rosmon_pid=$!
#
#sleep 2
#kill $rosmon_pid
#
#xxd rosmon_out
#echo ""
To be run with :
docker run --rm -v $PWD/rosmon_cr_issue.sh:/rosmon_cr_issue.sh:ro --entrypoint /rosmon_cr_issue.sh ros:melodic
And the output:
We can see the 0x0d
character in the rosmon output but not in the others.
Note that I use the tcsetattr()
function from the libc and not directly ioctl()
like in the mentionned go code because of this comment in the glibc:
The difference here is that the termios structure used in the kernel is not the same as we use in the libc. Therefore we must translate it here.
To be sure to be compatible with the libc structure.
I have tried to add a test on this change but I currently have a segfault: https://github.com/romainreignier/rosmon/blob/test_dont_add_cr/rosmon_core/test/output/test_output.cpp
And I am not sure such a test is meaningful. But if you wish, I can try harder to make it work.
Hey @romainreignier, thanks for the PR. This indeed sounds like the right thing to do. I'll check this on the weekend.
I guess a specific unit test is not required. Thanks for trying to develop one anyways :+1:
... and for writing the docker test script. Very nice, thank you :)
Thanks, I merged the PR after minor refactoring :)
Linux creates a new unix98 pty with the
ONLCR
flag set, see: pty.c:941 and tty_io.c:125.The flag ONLCR replaces
\n
with\r\n
as done in n_tty.c:444.This can be an issue while using rosmon in a Docker container with the journald log driver.
Docker removes the trailing
\n
in log lines here. This might not be obvious, but it is tested here. And discussed here.And systemd-journald treats a log line with a non printable character, including a trailing
\r
, as a binary blob and does not displays it withjournalctl
as discussed here and here.To fix the issue, the
\r
should be removed. And from this PR, I have learned its origin and how to get rid of it.