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

[DiffDrive] Test fixing #318

Merged
merged 9 commits into from
Mar 19, 2018
Merged

Conversation

artivis
Copy link
Contributor

@artivis artivis commented Mar 14, 2018

Fixing diff_drive_odom_frame_test.

In some cases the test testOdomTopic might be calling getLastOdom() before any odom message is received. It thus evaluate a default constructed msg.

I also turned the while loop that wait for the controller to be up into a function. It also ensure that ros::ok() otherwise launching manually a test without the controller get stuck in an infinite loop...

Jeremie Deray added 2 commits March 14, 2018 15:58
  avoid testing on a default constructed message.
@artivis
Copy link
Contributor Author

artivis commented Mar 14, 2018

@bmagyar @efernandez Hola.
Let me know if I should update all other tests with the newly added function waitForController.

@artivis artivis changed the title Test fixing [DiffDrive] Test fixing Mar 14, 2018
@artivis
Copy link
Contributor Author

artivis commented Mar 14, 2018

The failing CI job may again be due to the same issue.

ros_controllers/diff_drive_controller/test/diff_drive_multiple_cmd_vel_publishers_test.cpp:58
Expected: (fabs(roll_new - roll_old)) < (EPS), actual: nan vs 0.01

@bmagyar
Copy link
Member

bmagyar commented Mar 14, 2018

I've restarted the jobs again. Please try adding the same fix to all the tests that seem affected. We shall see.

@bmagyar
Copy link
Member

bmagyar commented Mar 14, 2018

This should fix #292

@artivis
Copy link
Contributor Author

artivis commented Mar 14, 2018

This does not fully solves #292 but seems to help - over more than a dozen run this particular test did not fail again.
There are other issues, one taking place in tests where a ros::Duration::sleep is used to wait for the controller to execute an expected trajectory - e.g. diff_drive_test - testForward. Those tests seems to fail by a very little overshoot. I'll try to do some monitoring tomorrow, my best guess by now is that the thread may wake up a little too late (machine under heavy load, cosmic ray...), thus the controller does 1/2/3... more iterations of its loop than expected. A possible fix would be that the expected trajectory/distance traveled ain't a hard-coded value. E.g. from the link above:

const ros::Duration actual_elapsed = new_odom.header.stamp - old_odom.header.stamp;
const double expected_traveled = cmd_vel.linear.x * actual_elapsed;
...
EXPECT_NEAR(expected_traveled, sqrt(dx*dx + dy*dy), POSITION_TOLERANCE);

@bmagyar What's your take about that ??

@artivis
Copy link
Contributor Author

artivis commented Mar 14, 2018

Currently failing the kind of issue I was trying to describe in my previous comment:

[diff_drive_controller.rosunit-skid_steer_test/testTurn][FAILURE]---------------
/root/catkin_ws/src/ros_controllers/diff_drive_controller/test/diff_drive_test.cpp:124
The difference between fabs(new_odom.twist.twist.angular.z) and 3.14159265358979323846/10.0 is 0.1029772675980104, 
which exceeds EPS, where fabs(new_odom.twist.twist.angular.z) evaluates to 0.41713653295698971,
3.14159265358979323846/10.0 evaluates to 0.31415926535897931,
and EPS evaluates to 0.01.

@bmagyar
Copy link
Member

bmagyar commented Mar 14, 2018 via email

// check if the robot traveled 1 meter in XY plane, changes in z should be ~~0
const double dx = new_odom.pose.pose.position.x - old_odom.pose.pose.position.x;
const double dy = new_odom.pose.pose.position.y - old_odom.pose.pose.position.y;
const double dz = new_odom.pose.pose.position.z - old_odom.pose.pose.position.z;
EXPECT_NEAR(sqrt(dx*dx + dy*dy), 1.0, POSITION_TOLERANCE);
EXPECT_NEAR(sqrt(dx*dx + dy*dy), expected_distance, POSITION_TOLERANCE);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bmagyar here is the proposed change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really... Now l.74 fails (sometime)...
This quite looks like a clock issue. I will try later having the diffbot publishing it's own clock and use_sim_time true.

@artivis artivis mentioned this pull request Mar 15, 2018
Diffbot<> robot;
ROS_WARN_STREAM("period: " << robot.getPeriod().toSec());
controller_manager::ControllerManager cm(&robot, nh);

ros::Rate rate(1.0 / robot.getPeriod().toSec());
ros::Publisher clock_publisher = nh.advertise<rosgraph_msgs::Clock>("/clock", 1);
Copy link
Contributor Author

@artivis artivis Mar 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bmagyar @efernandez The simulation now publishes its own clock which should fix time/integration erratic failures.

@artivis
Copy link
Contributor Author

artivis commented Mar 17, 2018

joint_trajectory_controller is the one failing as of now... :s

@bmagyar
Copy link
Member

bmagyar commented Mar 18, 2018

I kept restarting the travis jobs in the last 2 days. We have now around 10 runs when there was no failure from the patched up tests, nice job!

@artivis
Copy link
Contributor Author

artivis commented Mar 18, 2018

👍

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reliability of diff_drive_controller tests has dramatically increased on Travis, thanks for these changes!

@artivis
Copy link
Contributor Author

artivis commented Mar 19, 2018

You're welcome ;)

7675t added a commit to 7675t/ros_controllers that referenced this pull request Apr 21, 2018
alcantara09 pushed a commit to alcantara09/ros_controllers that referenced this pull request May 12, 2021
Fix time-sensitive tests of diff_drive_controller
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

Successfully merging this pull request may close these issues.

2 participants