time operators by Karsten1987 · Pull Request #351 · ros2/rclcpp

Karsten1987 · 2017-08-06T03:57:37Z

connects to #352

mikaelarguedas · 2017-08-06T22:08:50Z

rclcpp/include/rclcpp/time.hpp

+  Time(const builtin_interfaces::msg::Time & time_msg)  // NOLINT
+  : rcl_time_(get_empty_time_point())
+  {
+    rcl_time_.nanoseconds = RCL_S_TO_NS(time_msg.sec);


this overlap the comments in ros2/rcutils#44 but nothing prevents time_msg.sec to be negative here. (same everywhere below)

dirk-thomas · 2017-08-07T18:14:22Z

rclcpp/include/rclcpp/time.hpp

+  Time(int32_t seconds, uint32_t nanoseconds)
+  : rcl_time_(get_empty_time_point())
+  {
+    uint64_t ns = RCL_S_TO_NS(static_cast<uint64_t>(seconds));


Doesn't this need to check if seconds is negative?

Same below.

should be corrected, however I only found one spot, where it was missing. Where's the other one?

Nvm, all other static_casts have a check like if (seconds < 0) { before.

dirk-thomas · 2017-08-07T18:15:05Z

rclcpp/include/rclcpp/time.hpp

+    rcl_time_.nanoseconds = nanoseconds;
+  }
+
+  Time(const builtin_interfaces::msg::Time & time_msg)  // NOLINT


Why does this require NOLINT?

lint wants it to be explicit, however as it's used as a copy constructor it can't be explicit (code won't compile wo/).

Karsten1987 · 2017-08-08T17:57:45Z

OK, so I think that's up for a final review.
I ended up splitting the file into a .hpp and .cpp, got rid of the template functions and extended the tests for type and overflow checking.

Given the underlying c API, I decided to write only one class Time which takes the respective timesource as parameter, defaulting to RCL_SYSTEM_TIME. I propose leaving that API as is for now until we have more concrete use cases (e.g. sim time) in which we might want to reconsider.

CI:
linux:
mac:
win:

wjwwood · 2017-08-08T18:40:50Z

rclcpp/src/rclcpp/time.cpp

+
+  if (ret != RCL_RET_OK) {
+    rclcpp::exceptions::throw_from_rcl_error(
+      ret, "could not initialize time source: ");


I think the : is redundant:

rclcpp/rclcpp/src/rclcpp/exceptions.cpp

Lines 59 to 61 in b906768

if (!prefix.empty()) {

formated_prefix += ": ";

}

Here and below.

wjwwood · 2017-08-08T18:41:59Z

rclcpp/src/rclcpp/time.cpp

+  rcl_time_(init_time_point(rcl_time_source_))
+{
+  if (seconds < 0) {
+    throw std::runtime_error("can't convert a negative time msg to rclcpp::Time");


I would recommend this message instead, which I think is more to the point:

"cannot store a negative time point in rclcpp::Time"

Also below in at least one more place.

wjwwood · 2017-08-08T18:44:32Z

rclcpp/src/rclcpp/time.cpp

+    throw std::runtime_error("can't convert a negative time msg to rclcpp::Time");
+  }
+
+  uint64_t ns = RCL_S_TO_NS(static_cast<uint64_t>(seconds));


nitpick: you could assign this directly into rcl_time_.nanoseconds which is also a uint64_t.

wjwwood · 2017-08-08T18:56:44Z

rclcpp/src/rclcpp/time.cpp

+Time::operator==(const rclcpp::Time & rhs) const
+{
+  if (rcl_time_.time_source->type != rhs.rcl_time_.time_source->type) {
+    throw std::runtime_error("can't compare times with different time sources");


I think we need to have a better overview of how this class should work. For example, I think that the fact that you cannot compare two Time objects with different sources is a runtime error is very inconvenient. Ideally it would be a compiler error when you try to compare a "system based" time point and a "steady based" time point. This pattern makes more sense when you're abstracting "system based" time point from "ROS based" time point because they have the same reference point. We might need to rethink the way it is setup in rcl as well to reflect this.

Agreed. I don't know if there should be three different time classes around. Open for opinions. I just basically need the default time class (and their operators) as for now.

wjwwood · 2017-08-08T18:58:20Z

rclcpp/src/rclcpp/time.cpp

+
+  auto ns = rcl_time_.nanoseconds + rhs.rcl_time_.nanoseconds;
+  if (ns < rcl_time_.nanoseconds) {
+    throw std::runtime_error("addition leads to uint64_t overflow");


Probably http://en.cppreference.com/w/cpp/error/overflow_error instead of runtime.

wjwwood · 2017-08-08T18:58:34Z

rclcpp/src/rclcpp/time.cpp

+
+  auto ns = rcl_time_.nanoseconds - rhs.rcl_time_.nanoseconds;
+  if (ns > rcl_time_.nanoseconds) {
+    throw std::runtime_error("subtraction leads to uint64_t underflow");


http://en.cppreference.com/w/cpp/error/underflow_error too

wjwwood · 2017-08-08T18:59:53Z

rclcpp/test/test_time.cpp

+    FAIL();
+  } catch (const std::exception &) {
+    SUCCEED();
+  }


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

woooaaaah, that's so cool that you can actually put a complete block of code in it. thanks for that. didn't know it :)

If the invoked code is supposed to raise a specific exception I would suggest to use EXPECT_THROW rather than EXPECT_ANY_THROW.

wjwwood

+1, my comments were addressed to my satisfaction (though there were some things pointed out in the meeting that I missed)

tfoote

LGTM. I fixed the clcok typo.

mikaelarguedas · 2017-08-08T22:28:10Z

typo 2: 2c33365

dhood · 2017-08-14T16:40:50Z

@Karsten1987 these tests are failing on windows debug nightlies: http://ci.ros2.org/job/nightly_win_deb/568/testReport/junit/(root)/TestTime/operators/

…ation_tests_against_rmw_implementations Enable test_action_communication to compile against available rmw

time operators

80bd20a

Karsten1987 added the in review Waiting for review (Kanban column) label Aug 6, 2017

Karsten1987 self-assigned this Aug 6, 2017

mikaelarguedas reviewed Aug 6, 2017

View reviewed changes

Karsten1987 mentioned this pull request Aug 7, 2017

rclcpp::Time operators #352

Closed

explicitely cast to uint64_t and prevent overflow

89b6b6f

dirk-thomas reviewed Aug 7, 2017

View reviewed changes

Karsten1987 added 2 commits August 7, 2017 11:29

check for negative seconds .. again

05577aa

split into hpp/cpp

f598ea2

Karsten1987 force-pushed the time_operators branch from 2394c80 to f598ea2 Compare August 8, 2017 00:11

Karsten1987 and others added 3 commits August 7, 2017 17:56

export symbols

2f23015

change test macro

4d07f95

fix unsigned comparison

a9db3d9

wjwwood reviewed Aug 8, 2017

View reviewed changes

Karsten1987 and others added 3 commits August 8, 2017 12:29

address comments

c193d31

test for specific exception

e9eeab5

Fix typo

d8419f6

wjwwood approved these changes Aug 8, 2017

View reviewed changes

tfoote approved these changes Aug 8, 2017

View reviewed changes

Karsten1987 merged commit def973e into master Aug 8, 2017

Karsten1987 deleted the time_operators branch August 8, 2017 22:18

Karsten1987 removed the in review Waiting for review (Kanban column) label Aug 8, 2017

mikaelarguedas mentioned this pull request Aug 15, 2017

Time operator tests failing on Windows Debug #357

Closed

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

Merge pull request ros2#351 from apojomovsky/apojomovsky/run_communic…

a30ccdf

…ation_tests_against_rmw_implementations Enable test_action_communication to compile against available rmw

Conversation

Karsten1987 commented Aug 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Karsten1987 Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Karsten1987 commented Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

tfoote left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaelarguedas commented Aug 8, 2017

Uh oh!

dhood commented Aug 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Karsten1987 commented Aug 6, 2017 •

edited

Loading

Karsten1987 Aug 7, 2017 •

edited

Loading

Karsten1987 commented Aug 8, 2017 •

edited

Loading

tfoote left a comment •

edited

Loading