Rclcpp time #311

Karsten1987 · 2017-03-15T17:10:25Z

connects to #310

prototype of rclcpp::Time::now()

For now it's mainly a convenience function, but may be object to further helper classes.
I introduced a new c++ class called Time, which by itself has a static member function now which returns basically a wrapper around rcl_time_point_value_t. In order to assign it to a message header, the same function can be called, but gets converter via the casting operator.

Karsten1987 · 2017-03-15T17:17:58Z

A few question, I'd like to have sorted out before merging:

In case of error, I have a print. Is the idea to throw an exception at this point?
Since now doesn't return the uint64_t, but rather first a class object and then a message object, how does that effect any realtime constraints?

mikaelarguedas · 2017-03-15T17:49:14Z

FYI: added a connects to, assigned you on the corresponding issue and cross referenced this on the high level ticket

tfoote

One comment otherwise lgtm

tfoote · 2017-03-16T22:13:27Z

rclcpp/include/rclcpp/rclcpp.hpp

@@ -130,6 +130,7 @@
 #include "rclcpp/parameter_client.hpp"
 #include "rclcpp/parameter_service.hpp"
 #include "rclcpp/rate.hpp"
+//#include "rclcpp/time.hpp"


Why added with a comment?

tfoote · 2017-03-16T22:25:36Z

We've used time 0 in the past to indicate time uninitialized. Keeping that behavior seems reasonable for backwards compatibility. Though since we're starting from scratch an exception might be something we could consider.

For realtime optimization might we want to simply recommend using the rcl api?

Karsten1987 · 2017-03-21T17:32:57Z

I changed the code to throw exceptions now, changed the test accordingly and also include this header directly into rclcpp.hpp

@tfoote if you could quickly verify these changes again, then i'd merge.

dirk-thomas · 2017-03-21T17:38:50Z

If rclcpp is depending on builtin_interfaces it might be a good idea to move that message package out of common_interfaces. Otherwise all common interfaces need to be released before rclcpp. We can consider that when we get close to per-package releases though.

wjwwood · 2017-03-21T17:53:09Z

Otherwise all common interfaces need to be released before rclcpp. We can consider that when we get close to per-package releases though.

What's wrong with that? Do the packages in common_interfaces depend on rclcpp?

dirk-thomas · 2017-03-21T18:04:25Z

Otherwise all common interfaces need to be released before rclcpp. We can consider that when we get close to per-package releases though.

What's wrong with that? Do the packages in common_interfaces depend on rclcpp?

No, but consider it valuable to keep dependencies which need to be released before a package can be released as small as possible.

wjwwood

lgtm, except the changes I requested in the cmake code.

wjwwood · 2017-03-21T17:56:25Z

rclcpp/CMakeLists.txt

-include_directories(include)
+include_directories(
+  include
+  ${builtin_interfaces_INCLUDE_DIRS})


If you really just want the include directories then please use target_include_directories, but you probably should just use ament_target_dependencies on the targets that need these headers.

you mean on the packages which use this header file? As this is a header-only implementation, there is no target inside rclcpp which uses this header file.

you mean on the packages which use this header file?

No I mean on the targets which use the header file.

As this is a header-only implementation, there is no target inside rclcpp which uses this header file.

If no target needs these header files then it doesn't need to be in the include_directories(). I would have thought a test used this header file in a compilation unit in this package.

wjwwood · 2017-03-21T17:56:55Z

rclcpp/CMakeLists.txt

-ament_export_include_directories(include)
+ament_export_include_directories(
+  include
+  ${builtin_interfaces_INCLUDE_DIRS})


Use ament_export_dependencies instead.

wjwwood · 2017-03-21T17:57:46Z

rclcpp/CMakeLists.txt

+  if(TARGET test_time)
+    target_include_directories(test_time PUBLIC
+      ${rcl_INCLUDE_DIRS}
+    )


If you need the rcl include directories you will also need the library (perhaps). I recommend using ament_target_dependencies here as well.

wjwwood · 2017-03-21T18:01:38Z

rclcpp/include/rclcpp/time.hpp

+
+class Time
+{
+  rcl_time_point_value_t rcl_time_;


Please put member variables at the end of the class definition in its own private section.

If you want the constructors to be private, please mark them explicitly. See: https://google.github.io/styleguide/cppguide.html#Declaration_Order

wjwwood · 2017-03-21T18:04:15Z

rclcpp/test/test_time.cpp

+TEST(TestTime, rate_basics) {
+  using builtin_interfaces::msg::Time;
+  try {
+    rclcpp::Time::now<RCL_ROS_TIME>();


Should there be RCLCPP_*_TIME classes to avoid this mixing of rclcpp and rcl things?

well, as there is no real implementation, I could typedef them for now?

wjwwood · 2017-03-21T18:07:54Z

rclcpp/test/test_time.cpp

+    FAIL();
+  } catch (const std::exception &) {
+    // test succeeded, ROStime not implemented yet
+    // TODO(karsten1987): Fix this test when ROStime is implemented


You can assert this case with these gtest macros:

https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#exception-assertions

oh that's great. Thanks for the link.

mikaelarguedas · 2017-03-21T18:16:31Z

I think that I agree with @dirk-thomas, low level interfaces used by the core would fit well in another repo. What do you guys think of rcl_interfaces?

dirk-thomas · 2017-03-21T18:25:16Z

I haven't followed the conversations around time related stuff so likely this has been mentioned before. Why do we use a custom time type in rcl (rcl_time_point_value_t) and don't reuse the datatype defined in builtin_interfaces?

Karsten1987 · 2017-03-21T19:11:26Z

I am fine with whatever solution for the message packages. I have not enough insight about the build/package process to give qualified feedback on that one.

@dirk-thomas The idea here was to provide a real c++ object for time, which gives the opportunity to have handy helper functions or operators. That would leave the generated message struct a POD.

Karsten1987 · 2017-03-21T20:24:52Z

@wjwwood I hope to have addressed all your points. But could you quickly verify that I did it the correct way?

tfoote · 2017-03-21T23:44:06Z

rclcpp/include/rclcpp/time.hpp

+#ifndef RCLCPP__TIME_HPP_
+#define RCLCPP__TIME_HPP_
+
+#include <chrono>


I don't think this chrono include is necessary anymore. There are no calls to std::chrono visible in this diff.

wjwwood · 2017-03-21T23:54:43Z

I haven't followed the conversations around time related stuff so likely this has been mentioned before. Why do we use a custom time type in rcl (rcl_time_point_value_t) and don't reuse the datatype defined in builtin_interfaces?

rcl_time_point_value_t uses a single uint64_t rather than two 32-bit ints. I circulated this around at the time of the original rcl library implementation (ros2/rcl#5), but no one commented on it. I specifically remember having in person discussions with people including @codebot, but I can't find any in depth discussion about it in the pull request.

My intention was to eventually write up some stuff about the time representation and advocate for a change in the structure of the builtin_interfaces/Time.msg file, but I never had the time to do so. However, that's a separate can of worms and introduces ROS 1 interoperability issues. In the meantime, having a single value for time in rcl greatly simplifies (in my opinion) the calculations done with it, making errors less likely.

In the future, if we decide to update the structure of the Time.msg to use a single value rather than two, then we could easily replace the rcl_time_point_value_t with the C struct generated for builtin_interfaces/Time.msg. Or we can discuss removing the type in rcl despite not changing the structure of Time.msg.

However, for the time being, I think it's functional enough to have this conversion constructor handle the conversion to the message generated type.

wjwwood

lgtm, but I agree with @tfoote that the #include <chrono> can be removed.

wjwwood · 2017-03-21T23:56:38Z

rclcpp/test/test_time.cpp

+
+TEST(TestTime, rate_basics) {
+  using builtin_interfaces::msg::Time;
+  EXPECT_ANY_THROW(rclcpp::Time::now<RCL_ROS_TIME>());


The TODO(karsten1987) that was here before still applies right?

tfoote · 2017-03-21T23:58:09Z

RE: rcl_time_point_value_t The goal was that we could push the lower level time operations down to be shared between the rclc and rclcpp and rclpy implementations for now it's all in rclcpp but once we add the not implemented rostime datasource that would be great to share. And the single datatype will make that much easier.

RE: moving the builtin_interfaces I think it makes sense to move it to rcl_interfaces repo makes sense at some point. Possibly into rcl_interfaces package too.

wjwwood · 2017-03-22T00:02:18Z

I'm actually surprised that no messages in rcl_interefaces use Time.msg or Duration.msg yet.

mikaelarguedas · 2017-03-22T00:03:00Z

While I think that putting these in the rcl_interfaces repo makes a lot of sense. I think it should stay a package on it's own. How I see it is that rcl_interfaces is defining core messages that most/all users won't use. builtin_interfaces is an hybrid package in the sense that it defines messages that we use in the core and we expect user to use them in most of their messages (either through Header or if they want to send Time/Duration information around). So I'd vote for keeping them in different packages but in the rcl_interfaces repo

Karsten1987 · 2017-03-22T00:36:37Z

So I'll merge this now if nobody has any outstanding comments. I propose we open another issue/PR for moving the builtin_interfaces to its appropriate place.

wjwwood · 2017-03-22T04:00:58Z

Looks like there is a new warning on Linux from test_time.cpp:

http://ci.ros2.org/job/ci_linux/2341/warnings22Result/

* Fix buggy if-conditions in transition functions. * Bugfix: incease number of states by one * Cleanup CMakeLists.txt and package.xml * Move goal state machine implementation details from header to C file

* Add compression factory stubs Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>

Karsten1987 added 2 commits March 13, 2017 14:53

initial commit for rclcpp::time::now()

b4a7c4b

switch between times

a0305cc

Karsten1987 self-assigned this Mar 15, 2017

Karsten1987 added the in progress Actively being worked on (Kanban column) label Mar 15, 2017

Karsten1987 force-pushed the rclcpp_time branch from 85f2217 to ded0ac4 Compare March 15, 2017 17:11

introduce time class

b2fc67b

Karsten1987 force-pushed the rclcpp_time branch from ded0ac4 to b2fc67b Compare March 15, 2017 17:14

Karsten1987 added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 15, 2017

tfoote mentioned this pull request Mar 15, 2017

Create an equivalent to ros::Time ros2/ros2#178

Closed

7 tasks

tfoote approved these changes Mar 16, 2017

View reviewed changes

Karsten1987 added 4 commits March 21, 2017 10:17

add rclcpp/time.hpp to rclcpp/rclcpp.hpp

f094559

throw exceptions on time error

4e7e1d1

fix test_time to catch exceptions

4269e22

explicit one-parameter constructor

08def40

wjwwood requested changes Mar 21, 2017

View reviewed changes

fix msvc compiler warnings

6b77e5e

address review comments

70f4ca8

tfoote reviewed Mar 21, 2017

View reviewed changes

wjwwood approved these changes Mar 21, 2017

View reviewed changes

Karsten1987 added 2 commits March 21, 2017 17:31

cleanup includes

5748751

re-add todo for fixing test once ros-time is there

22bd39d

Karsten1987 merged commit c07aee5 into master Mar 22, 2017

Karsten1987 removed the in review Waiting for review (Kanban column) label Mar 22, 2017

Karsten1987 deleted the rclcpp_time branch March 22, 2017 00:41

wjwwood mentioned this pull request Mar 22, 2017

add a function to validate topic names ros2/rmw#92

Merged

DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022

Add compression factory stubs (ros2#311)

e79efb0

* Add compression factory stubs Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rclcpp time #311

Rclcpp time #311

Karsten1987 commented Mar 15, 2017 •

edited by mikaelarguedas

Loading

Karsten1987 commented Mar 15, 2017

mikaelarguedas commented Mar 15, 2017

tfoote left a comment

tfoote Mar 16, 2017

tfoote commented Mar 16, 2017

Karsten1987 commented Mar 21, 2017

dirk-thomas commented Mar 21, 2017

wjwwood commented Mar 21, 2017

dirk-thomas commented Mar 21, 2017

wjwwood left a comment

wjwwood Mar 21, 2017

Karsten1987 Mar 21, 2017

wjwwood Mar 21, 2017

wjwwood Mar 21, 2017

wjwwood Mar 21, 2017

wjwwood Mar 21, 2017

wjwwood Mar 21, 2017

Karsten1987 Mar 21, 2017

wjwwood Mar 21, 2017

Karsten1987 Mar 21, 2017

mikaelarguedas commented Mar 21, 2017

dirk-thomas commented Mar 21, 2017

Karsten1987 commented Mar 21, 2017

Karsten1987 commented Mar 21, 2017

tfoote Mar 21, 2017

wjwwood commented Mar 21, 2017

wjwwood left a comment

wjwwood Mar 21, 2017

tfoote commented Mar 21, 2017

wjwwood commented Mar 22, 2017

mikaelarguedas commented Mar 22, 2017

Karsten1987 commented Mar 22, 2017

wjwwood commented Mar 22, 2017

Rclcpp time #311

Rclcpp time #311

Conversation

Karsten1987 commented Mar 15, 2017 • edited by mikaelarguedas Loading

Karsten1987 commented Mar 15, 2017

mikaelarguedas commented Mar 15, 2017

tfoote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfoote commented Mar 16, 2017

Karsten1987 commented Mar 21, 2017

dirk-thomas commented Mar 21, 2017

wjwwood commented Mar 21, 2017

dirk-thomas commented Mar 21, 2017

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelarguedas commented Mar 21, 2017

dirk-thomas commented Mar 21, 2017

Karsten1987 commented Mar 21, 2017

Karsten1987 commented Mar 21, 2017

Choose a reason for hiding this comment

wjwwood commented Mar 21, 2017

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfoote commented Mar 21, 2017

wjwwood commented Mar 22, 2017

mikaelarguedas commented Mar 22, 2017

Karsten1987 commented Mar 22, 2017

wjwwood commented Mar 22, 2017

Karsten1987 commented Mar 15, 2017 •

edited by mikaelarguedas

Loading