Add unit tests for rclcpp Rate objects #182

tfoote · 2015-12-15T08:47:45Z

Connects to #181

First adding unit tests for coverage. Next debug problem.

The problem from 181 is reproduced in these unit tests:

08:42:55 In file included from /Users/osrf/jenkins/workspace/ci_osx/ws/src/ros2/rclcpp/rclcpp/test/test_rate.cpp:19:
08:42:55 /Users/osrf/jenkins/workspace/ci_osx/ws/src/ros2/rclcpp/rclcpp/include/rclcpp/rate.hpp:74:20: error: no viable overloaded '+='
08:42:55     last_interval_ += period_;
08:42:55     ~~~~~~~~~~~~~~ ^  ~~~~~~~
08:42:55 /Users/osrf/jenkins/workspace/ci_osx/ws/src/ros2/rclcpp/rclcpp/test/test_rate.cpp:32:5: note: in instantiation of member function 'rclcpp::rate::GenericRate<std::__1::chrono::system_clock>::sleep' requested here
08:42:55   r.sleep();
08:42:55     ^
08:42:55 /Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/chrono:782:79: note: candidate function not viable: no known conversion from 'duration<[...], ratio<[...], 1000000000>>' to 'const duration<[...], ratio<[...], 1000000>>' for 1st argument
08:42:55     __attribute__ ((__visibility__("hidden"), __always_inline__)) time_point& operator+=(const duration& __d) {__d_ += __d; return *this;}
08:42:55                                                                               ^
08:42:55 In file included from /Users/osrf/jenkins/workspace/ci_osx/ws/src/ros2/rclcpp/rclcpp/test/test_rate.cpp:19:
08:42:55 /Users/osrf/jenkins/workspace/ci_osx/ws/src/ros2/rclcpp/rclcpp/include/rclcpp/rate.hpp:81:24: error: no viable overloaded '='
08:42:55         last_interval_ = now + period_;
08:42:55         ~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~
08:42:55 /Users/osrf/jenkins/workspace/ci_osx/ws/src/ros2/rclcpp/rclcpp/test/test_rate.cpp:32:5: note: in instantiation of member function 'rclcpp::rate::GenericRate<std::__1::chrono::system_clock>::sleep' requested here
08:42:55   r.sleep();
08:42:55     ^
08:42:55 /Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/chrono:750:56: note: candidate function (the implicit copy assignment operator) not viable: no known conversion from 'time_point<[...], duration<[...], ratio<[...], 1000000000>>>' to 'const time_point<[...], duration<[...], ratio<[...], 1000000>>>' for 1st argument
08:42:55 class __attribute__ ((__type_visibility__("default"))) time_point
08:42:55                                                        ^
08:42:55 /Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/chrono:750:56: note: candidate function (the implicit move assignment operator) not viable: no known conversion from 'time_point<[...], duration<[...], ratio<[...], 1000000000>>>' to 'time_point<[...], duration<[...], ratio<[...], 1000000>>>' for 1st argument
08:42:55 class __attribute__ ((__type_visibility__("default"))) time_point

tfoote · 2015-12-16T10:47:02Z

CI is passing:

http://ci.ros2.org/job/ci_linux/737/
http://ci.ros2.org/job/ci_osx/619/
http://ci.ros2.org/job/ci_windows/809/

However the fix is not optimal as it pulls all durations to the lowest common denominator. Clang uses microseconds whereas gcc uses nanoseconds for the high resolution clock. https://stackoverflow.com/questions/22205290/cast-from-time-point-duration-to-duration-fails-on-clang

Does anyone have a better suggestion for this?

gerkey · 2015-12-16T19:20:44Z

It does seem a shame to throw away resolution, but if that is how clang is doing it, then it doesn't seem that we have much choice. We'll likely end up using clang eventually on Linux, too.

Also, would we really be losing any capabilities? I.e., do we expect to run on a system that supports better-than-microsecond resolution timers? Looking around a bit, it seems that Linux + RT-PREEMPT can only get down to ~10us.

wjwwood · 2015-12-16T20:10:20Z

I proposed a different way to fix this in #185.

Basically, you force the time point object stored by the class to be nanosecond precision, but reuse the representation that the clock uses. This works fine since time_point<..., Ratio=nano> = time_point<..., Ration=micro> is allowed, and that's different from the original problem which was time_point<..., Ratio=micro> = time_point<..., Ration=micro> + duration<..., Ration=nano> which is not allowed due to implicit loss of precision.

tfoote · 2015-12-17T01:45:09Z

This is close, however I'm having trouble getting osx to link the unit tests correctly.

Windows is happy: http://ci.ros2.org/job/ci_windows/817/

Initially I was getting this error:

dyld: Library not loaded: @rpath/librosidl_typesupport_introspection_c.dylib
  Referenced from: /Users/osrf/jenkins/workspace/ci_osx/ws/install/rcl_interfaces/lib/librcl_interfaces__rosidl_typesupport_introspection_c.dylib
  Reason: image not found

Adding an explicit linking to rmw_implementation_LIBRARIES fixed that but I only got a little farther to

-- run_test.py: invoking following command in '/Users/osrf/jenkins/workspace/ci_osx/ws/build/rclcpp':
 - /Users/osrf/jenkins/workspace/ci_osx/ws/build/rclcpp/test_rate --gtest_output=xml:/Users/osrf/jenkins/workspace/ci_osx/ws/build/rclcpp/test_results/rclcpp/test_rate.gtest.xml
dyld: Library not loaded: @rpath/librosidl_typesupport_introspection_c.dylib
  Referenced from: /Users/osrf/jenkins/workspace/ci_osx/ws/install/rcl_interfaces/lib/librcl_interfaces__rosidl_typesupport_introspection_c.dylib
  Reason: image not found
-- run_test.py: return code -5
-- run_test.py: generate result file '/Users/osrf/jenkins/workspace/ci_osx/ws/build/rclcpp/test_results/rclcpp/test_rate.gtest.xml' with failed test
-- run_test.py: verify result file '/Users/osrf/jenkins/workspace/ci_osx/ws/build/rclcpp/test_results/rclcpp/test_rate.gtest.xml'

From there I tried adding rcl_interfaces as a run dependency as well as a built time one to make sure the package is available for the tests. And that didn't work either:

-- run_test.py: invoking following command in '/Users/osrf/jenkins/workspace/ci_osx/ws/build/rclcpp':
 - /Users/osrf/jenkins/workspace/ci_osx/ws/build/rclcpp/test_rate --gtest_output=xml:/Users/osrf/jenkins/workspace/ci_osx/ws/build/rclcpp/test_results/rclcpp/test_rate.gtest.xml
dyld: Library not loaded: @rpath/librosidl_typesupport_introspection_c.dylib
  Referenced from: /Users/osrf/jenkins/workspace/ci_osx/ws/install/rcl_interfaces/lib/librcl_interfaces__rosidl_typesupport_introspection_c.dylib
  Reason: image not found
-- run_test.py: return code -5
-- run_test.py: generate result file '/Users/osrf/jenkins/workspace/ci_osx/ws/build/rclcpp/test_results/rclcpp/test_rate.gtest.xml' with failed test
-- run_test.py: verify result file '/Users/osrf/jenkins/workspace/ci_osx/ws/build/rclcpp/test_results/rclcpp/test_rate.gtest.xml'

I'm trying one more iteration with an explicit test_depend but I'm not sure where to look next. Does anyone have a suggestion?

dirk-thomas · 2015-12-17T06:09:55Z

Without more context it is difficult to suggest anything but the usual:

have you checked if these libraries exist?
what does ldd output for the executable / the libraries?
if it doesn't use absolute path how is the environment variable for looking up libraries when the test is being run?

tfoote · 2015-12-17T19:34:08Z

The libraries exist where I would expect them. Looking at the output of otool -L there are a bunch of @rpath symbols which appear to be the problem. I commented out all my additional attempts to link additional libraries and just linked against the needed rclcpp library.

I still had all the rpaths and looking at librclcpp it itself has the rpaths. (oddly including an rpath to link against itself?

https://gist.github.com/tfoote/b2a693ae2f19aa5c203b

on rosidl_typesupport_introspection_c necessary for ros2/rclcpp#182

tfoote · 2015-12-17T22:07:27Z

CI kickedoff but backlogged:

CI linux is diabled? @dirk-thomas
http://ci.ros2.org/job/ci_windows/830
http://ci.ros2.org/job/ci_osx/650

tfoote · 2015-12-18T02:37:41Z

More robust tests CI:

http://ci.ros2.org/job/ci_linux/791
http://ci.ros2.org/job/ci_osx/660
http://ci.ros2.org/job/ci_windows/841

tfoote · 2015-12-18T03:55:59Z

Our assumptions about sleep's behavior were incorrect. updated new CI:
http://ci.ros2.org/job/ci_linux/794/
http://ci.ros2.org/job/ci_osx/662
http://ci.ros2.org/job/ci_windows/843/

tfoote · 2015-12-18T09:41:42Z

http://ci.ros2.org/job/ci_osx/663/
http://ci.ros2.org/job/ci_linux/796
http://ci.ros2.org/job/ci_windows/847/

dirk-thomas · 2015-12-18T09:49:18Z

rclcpp/test/test_rate.cpp

+
+/*
+   Tests that funcion_traits calculates arity of several functors.
+ */


Docblock seems to be unrelated?

Yeah, not quite accurate.

gerkey · 2015-12-18T18:16:13Z

+1

We can hopefully tighten up the underrun bounds after addressing #190.

Add unit tests for rclcpp Rate objects

dirk-thomas · 2016-04-27T00:42:11Z

rclcpp/CMakeLists.txt

+      ${rosidl_generator_cpp_INCLUDE_DIRS}
+    )
+    target_link_libraries(test_rate
+      ${PROJECT_NAME}${target_suffix}


What is this line supposed to do? There is no target_suffix in this scope. It will have a random value depending on how call_for_each_rmw_implementation above has iterated through the rmw implementations.

Fixed in #213.

* Remove error msg from valid case * Document RCL_RET_{CLIENT/SERVICE}_TAKE_FAILED

* Add support for specifying max bagfile size in storage_options Signed-off-by: Zachary Michaels <zmichaels11@gmail.com> * Add helper functions in Writer required for bagfile splitting Signed-off-by: Zachary Michaels <zmichaels11@gmail.com> * Store max_bagfile_size when Writer is opened Signed-off-by: Zachary Michaels <zmichaels11@gmail.com> * Uncrustify Signed-off-by: Zachary Michaels <zmichaels11@gmail.com> * Apply suggestions from PR Signed-off-by: Zachary Michaels <zmichaels11@gmail.com> * Add ROSBAG2_STORAGE_PUBLIC to MAX_BAGFILE_SIZE_NO_SPLIT This should fix the issue on Windows Signed-off-by: Zachary Michaels <zmichaels11@gmail.com> * Renamed private function in Writer to not end in `_` Signed-off-by: Zachary Michaels <zmichaels11@gmail.com>

tfoote self-assigned this Dec 15, 2015

tfoote added the in progress Actively being worked on (Kanban column) label Dec 15, 2015

tfoote force-pushed the rate_tests branch 5 times, most recently from ee1129e to ca229a1 Compare December 16, 2015 10:31

tfoote mentioned this pull request Dec 16, 2015

different fix for the rate failing #185

Merged

tfoote force-pushed the rate_tests branch 2 times, most recently from 08ea444 to d2475dd Compare December 17, 2015 00:49

tfoote added a commit to ros2/rmw_connext that referenced this pull request Dec 17, 2015

adding missing dependencies

5e54e6c

on rosidl_typesupport_introspection_c necessary for ros2/rclcpp#182

tfoote mentioned this pull request Dec 17, 2015

adding missing dependencies ros2/rmw_connext#119

Closed

tfoote force-pushed the rate_tests branch from e8ddf6d to 4d332f9 Compare December 17, 2015 22:04

tfoote mentioned this pull request Dec 18, 2015

rate.sleep will sometimes return slightly early from a sleep #190

Closed

tfoote added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Dec 18, 2015

dirk-thomas reviewed Dec 18, 2015
View reviewed changes

tfoote added 3 commits December 18, 2015 03:41

adding basic unit tests for rate

6d79b5e

softening tolerances on timing to pass on osx

aa81807

updating tests to be less vulnerable to load

ebdf394

tfoote added 2 commits December 18, 2015 03:41

adding coverage and an epsilon

fac550c

fixing comment

8af64b9

tfoote force-pushed the rate_tests branch from f45c82b to 8af64b9 Compare December 18, 2015 11:42

tfoote added a commit that referenced this pull request Dec 18, 2015

Merge pull request #182 from ros2/rate_tests

4d47ef3

Add unit tests for rclcpp Rate objects

tfoote merged commit 4d47ef3 into master Dec 18, 2015

tfoote removed the in review Waiting for review (Kanban column) label Dec 18, 2015

tfoote deleted the rate_tests branch December 18, 2015 19:20

dirk-thomas reviewed Apr 27, 2016
View reviewed changes

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022

Remove error message when client take returns false (ros2#182)

0ff5b98

* Remove error msg from valid case * Document RCL_RET_{CLIENT/SERVICE}_TAKE_FAILED

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests for rclcpp Rate objects #182

Add unit tests for rclcpp Rate objects #182

tfoote commented Dec 15, 2015

tfoote commented Dec 16, 2015

gerkey commented Dec 16, 2015

wjwwood commented Dec 16, 2015

tfoote commented Dec 17, 2015

dirk-thomas commented Dec 17, 2015

tfoote commented Dec 17, 2015

tfoote commented Dec 17, 2015

tfoote commented Dec 18, 2015

tfoote commented Dec 18, 2015

tfoote commented Dec 18, 2015

dirk-thomas Dec 18, 2015

tfoote Dec 18, 2015

gerkey commented Dec 18, 2015

dirk-thomas Apr 27, 2016 •

edited

Loading

Add unit tests for rclcpp Rate objects #182

Add unit tests for rclcpp Rate objects #182

Conversation

tfoote commented Dec 15, 2015

tfoote commented Dec 16, 2015

gerkey commented Dec 16, 2015

wjwwood commented Dec 16, 2015

tfoote commented Dec 17, 2015

dirk-thomas commented Dec 17, 2015

tfoote commented Dec 17, 2015

tfoote commented Dec 17, 2015

tfoote commented Dec 18, 2015

tfoote commented Dec 18, 2015

tfoote commented Dec 18, 2015

dirk-thomas Dec 18, 2015

Choose a reason for hiding this comment

tfoote Dec 18, 2015

Choose a reason for hiding this comment

gerkey commented Dec 18, 2015

dirk-thomas Apr 27, 2016 • edited Loading

Choose a reason for hiding this comment

dirk-thomas Apr 27, 2016 •

edited

Loading