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

Fix/time #497

Merged
merged 8 commits into from
May 4, 2020
Merged

Fix/time #497

merged 8 commits into from
May 4, 2020

Conversation

rikba
Copy link
Contributor

@rikba rikba commented Apr 28, 2020

In #169 @PaulBouchier mentions that the implementation of subtraction and addition of time and duration may be erroneous. We actually observed that

ros::Time time = {1, 0};
time -= ros::Duration(0, 1000000);

indeed lead to a wrong result on our system.

This PR implements the fix provided by @chuck-h and additional unit tests.

Since I cannot run the unit tests on my machine I'm creating this draft PR first.

@rikba
Copy link
Contributor Author

rikba commented Apr 28, 2020

Ok, I was hoping Travis has a different workspace setup than I do, but I get the same redefinition error of ros::Duration here.

Can someone help me? How do I #include "ros_lib/ros/duration.h" in the unit test without redefining ros::Duration?

In file included from /home/travis/catkin_ws/src/rosserial/rosserial_client/test/time_test.cpp:2:

In file included from /home/travis/catkin_ws/src/rosserial/rosserial_client/src/ros_lib/ros/time.h:38:

/opt/ros/melodic/include/ros/duration.h:108:20: error: redefinition of

      'Duration'

class ROSTIME_DECL Duration : public DurationBase<Duration>

                   ^

/home/travis/catkin_ws/src/rosserial/rosserial_client/src/ros_lib/ros/duration.h:46:7: note: 

      previous definition is here

class Duration

      ^

[ 96%] Built target rosserial_test_publish_subscribe

1 error generated.

rosserial/rosserial_client/CMakeFiles/time_test.dir/build.make:62: recipe for target 'rosserial/rosserial_client/CMakeFiles/time_test.dir/test/time_test.cpp.o' failed

make[3]: *** [rosserial/rosserial_client/CMakeFiles/time_test.dir/test/time_test.cpp.o] Error 1

CMakeFiles/Makefile2:2431: recipe for target 'rosserial/rosserial_client/CMakeFiles/time_test.dir/all' failed

make[2]: *** [rosserial/rosserial_client/CMakeFiles/time_test.dir/all] Error 2

CMakeFiles/Makefile2:77: recipe for target 'CMakeFiles/tests.dir/rule' failed

make[1]: *** [CMakeFiles/tests.dir/rule] Error 2

Makefile:175: recipe for target 'tests' failed

make: *** [tests] Error 2

Invoking "make tests -j2 -l2" failed

The command "catkin_make tests" exited with 1.

@rikba rikba marked this pull request as ready for review April 28, 2020 22:01
@mikepurvis
Copy link
Member

How do I #include "ros_lib/ros/duration.h" in the unit test without redefining ros::Duration?

You figured out one way. Another is to wrap the include in a namespace to isolate it from its surroundings; we do this for the rosserial_test node, as it is a single binary that is simultaneously a rosserial client and a roscpp node:

https://github.com/ros-drivers/rosserial/blob/noetic-devel/rosserial_test/src/publish_subscribe.cpp#L4-L7

@mikepurvis mikepurvis changed the base branch from melodic-devel to noetic-devel May 4, 2020 17:53
@mikepurvis mikepurvis merged commit 04ddab7 into ros-drivers:noetic-devel May 4, 2020
@rikba rikba deleted the fix/time branch May 4, 2020 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants