Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ros_comm 1.4.1 breaks socketcan_bridge tests #1408

Closed
mathias-luedtke opened this issue May 23, 2018 · 12 comments
Closed

ros_comm 1.4.1 breaks socketcan_bridge tests #1408

mathias-luedtke opened this issue May 23, 2018 · 12 comments

Comments

@mathias-luedtke
Copy link

mathias-luedtke commented May 23, 2018

I just noticed that one test started to fail for melodic shadow-fixed (1.4.1), but works for main (1.3.6).
https://travis-ci.org/ipa-mdl/ros_canopen/builds/381469509

The failing test, runs ros::spinOnce() to fetch one message.

Digging through the commits #1365 raised my attention.
I will run further tests, but this seems to change the behaviour at least for this test.

@mathias-luedtke
Copy link
Author

I was not able to fix this test with an additional wait. It might be that the message got lost.
So it looks more like #1393 is the cause.

The only external depedencies are

executing command [apt-get install -y ros-melodic-roscpp]
executing command [apt-get install -y ros-melodic-class-loader]
executing command [apt-get install -y ros-melodic-message-runtime]
executing command [apt-get install -y ros-melodic-std-msgs]
executing command [apt-get install -y ros-melodic-roslint]
executing command [apt-get install -y ros-melodic-rostest]
executing command [apt-get install -y ros-melodic-rosunit]
executing command [apt-get install -y ros-melodic-message-generation]
executing command [apt-get install -y libboost-all-dev]

@dirk-thomas
Copy link
Member

@guillaumeautran Can you please take a closer look at this.

@mathias-luedtke
Copy link
Author

I was not able to dermine the cause of this problem, perhaps it's the combination of different commits.
However, I was able to come up with a test case.
For some reason the /rosout handling seems to be involved.

@mikepurvis
Copy link
Member

Assuming you're on Linux, I'd be doubtful that #1393 could directly be the issue, since that was a fix for non-epoll systems such as macOS.

Does the test still fail if you build ros_comm from source but override HAVE_EPOLL=0 in this spot?

https://github.com/ros/ros_comm/pull/1217/files#diff-af96143090deaa643970030f962c964dR70

That would kick it back to the legacy poll implementation for handling IO.

@guillaumeautran
Copy link
Contributor

I am taking a look right now but the specific tests passes for me at this time. Still doing some more digging.

@mathias-luedtke
Copy link
Author

mathias-luedtke commented May 23, 2018

Assuming you're on Linux

The test case fails on travis, ROS buildfarm (#1409) and in a local docker environment (run_ci).

Does the test still fail if you build ros_comm from source but override HAVE_EPOLL=0 in this spot?

I will push a commit to the test case PR

I did not have a deeper look at the other PRs.

@guillaumeautran
Copy link
Contributor

So, looks like a setup/teardown problem. I was able to reproduce the issue (only happens on the ros_comm melodic branch) but a soon as I comment out the first test case, the second test (which was failing originally) starts to pass.
Something must not have been cleaned up properly between the tests.

@mathias-luedtke
Copy link
Author

The only other difference of package versions is rosconsole: 1.13.6 -> 1.13.7

@mathias-luedtke
Copy link
Author

Something must not have been cleaned up properly between the tests.

If I add a ros::NodeHandle to main(), both tests pass.

@mathias-luedtke
Copy link
Author

Finally, I ran git bisect. The test case started to fail with d21a33c (#1241)

@dirk-thomas
Copy link
Member

Finally, I ran git bisect. The test case started to fail with d21a33c (#1241)

@trobertson-cpr FYI

@mathias-luedtke
Copy link
Author

I have resolved this by adding the NodeHandle.
Nevertheless, #1241 changed the behavior and affects at least the logging performance in rospy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants