Make rclcpp::Duration support scale operation #437

jwang11 · 2018-02-13T11:21:54Z

Duration scale is a convinient operation which had supported in ROS.
This commit make ROS2 support it.

Signed-off-by: jwang jing.j.wang@intel.com

tfoote · 2018-02-14T02:11:21Z

rclcpp/src/rclcpp/duration.cpp

+Duration
+Duration::operator*(double scale) const
+{
+  return Duration(rcl_duration_.nanoseconds * scale);


Please add underflow and overflow checking here. We are checking + and - operations we should check * too.

dirk-thomas · 2018-02-14T02:15:19Z

rclcpp/test/test_duration.cpp

@@ -54,6 +54,10 @@ TEST(TestDuration, operators) {
  EXPECT_EQ(sub.nanoseconds(), (rcl_duration_value_t)(young.nanoseconds() - old.nanoseconds()));
  EXPECT_EQ(sub, young - old);

+  rclcpp::Duration scale = old * 3;
+  EXPECT_EQ(scale.nanoseconds(), (rcl_duration_value_t)(old.nanoseconds() * 3));
+  EXPECT_EQ(scale, old * 3);


This line doesn't seem to test anything. scale is defined just as old * 3 two lines above. So this will always be true if the operator is deterministic.

I know it, the reason I add this check is that the tests for operation +/- both include it, just keep consistent

rclcpp::Duration add = old + young; EXPECT_EQ(add.nanoseconds(), (rcl_duration_value_t)(old.nanoseconds() + young.nanoseconds())); EXPECT_EQ(add, old + young); rclcpp::Duration sub = young - old; EXPECT_EQ(sub.nanoseconds(), (rcl_duration_value_t)(young.nanoseconds() - old.nanoseconds())); EXPECT_EQ(sub, young - old);

mikaelarguedas · 2018-02-15T19:15:11Z

Thanks @jwang11 for the contribution.
@tfoote of @dirk-thomas to run CI and merge if green

tfoote

I had a few comments inline on the range checking. And we should make sure to have unit tests for the range checking to validate that they're working.

tfoote · 2018-02-15T23:34:09Z

rclcpp/src/rclcpp/duration.cpp

+bounds_check_duration_scale(int64_t dns, double scale, uint64_t max)
+{
+  auto abs_dns = (uint64_t)std::abs(dns);
+  auto abs_scale = (uint64_t)std::abs(scale);


Casting this to be an unsigned integer I think is going to do the wrong thing here. It's used lower as if it's still a float.

tfoote · 2018-02-15T23:35:57Z

rclcpp/src/rclcpp/duration.cpp

+  auto abs_scale = (uint64_t)std::abs(scale);
+
+  if ((abs_scale >= 1.0 && abs_dns > (uint64_t)(max / abs_scale)) ||
+    (abs_scale < 1.0 && (uint64_t)(abs_dns * abs_scale > max)))


If we're below an absolute scaling factor of 1 the value can only get smaller which will never overflow or underflow a signed integer. I think this whole 2nd case can be dropped.

tfoote · 2018-02-15T23:39:22Z

rclcpp/src/rclcpp/duration.cpp

+    (abs_scale < 1.0 && (uint64_t)(abs_dns * abs_scale > max)))
+  {
+    if ((dns > 0 && scale > 0) || (dns < 0 && scale < 0)) {
+      throw std::overflow_error("addition leads to int64_t overflow");


This should be 'duration scaling' not 'addition'

tfoote · 2018-02-15T23:39:27Z

rclcpp/src/rclcpp/duration.cpp

+    if ((dns > 0 && scale > 0) || (dns < 0 && scale < 0)) {
+      throw std::overflow_error("addition leads to int64_t overflow");
+    } else {
+      throw std::underflow_error("addition leads to int64_t underflow");


This should be 'duration scaling' not 'addition'

tfoote · 2018-02-15T23:43:05Z

rclcpp/test/test_duration.cpp

@@ -54,6 +54,9 @@ TEST(TestDuration, operators) {
  EXPECT_EQ(sub.nanoseconds(), (rcl_duration_value_t)(young.nanoseconds() - old.nanoseconds()));
  EXPECT_EQ(sub, young - old);

+  rclcpp::Duration scale = old * 3;
+  EXPECT_EQ(scale.nanoseconds(), (rcl_duration_value_t)(old.nanoseconds() * 3));


This should also have tests for the underflow and overflow. In particular I'd like to see at least all 4 cases of underflow and overflow with positive and negative scaling factors.

Thanks for comments, they are all reasonable.

dirk-thomas · 2018-02-15T23:55:02Z

Please make sure to run the unit tests of the package locally. The linters will e.g. enforce to use C++ cast rather than C casts.

updated code

tfoote · 2018-02-21T02:30:09Z

CI:
Linux:
Linux arm64:
OSX:
Windows:

tfoote

Windows also failed with a gateway issue so i'm going to retrigger it.

tfoote · 2018-02-22T01:50:08Z

rclcpp/src/rclcpp/duration.cpp

+bounds_check_duration_scale(int64_t dns, double scale, uint64_t max)
+{
+  auto abs_dns = static_cast<uint64_t>(std::abs(dns));
+  auto abs_scale = std::abs(scale);


This is apparently ambiguous on OSX: https://ci.ros2.org/job/ci_osx/3306/consoleFull

03:04:22 [ 38%] Building CXX object CMakeFiles/rclcpp.dir/src/rclcpp/contexts/default_context.cpp.o 03:04:23 [ 39%] Building CXX object CMakeFiles/rclcpp.dir/src/rclcpp/duration.cpp.o 03:04:23 /Users/osrf/jenkins/workspace/ci_osx/ws/src/ros2/rclcpp/rclcpp/src/rclcpp/duration.cpp:189:20: error: call to 'abs' is ambiguous 03:04:23 auto abs_scale = std::abs(scale); 03:04:23 ^~~~~~~~ 03:04:23 /usr/include/stdlib.h:137:6: note: candidate function 03:04:23 int abs(int) __pure2; 03:04:23 ^ 03:04:23 /Library/Developer/CommandLineTools/usr/include/c++/v1/stdlib.h:115:44: note: candidate function 03:04:23 inline _LIBCPP_INLINE_VISIBILITY long abs( long __x) _NOEXCEPT {return labs(__x);} 03:04:23 ^ 03:04:23 /Library/Developer/CommandLineTools/usr/include/c++/v1/stdlib.h:117:44: note: candidate function 03:04:23 inline _LIBCPP_INLINE_VISIBILITY long long abs(long long __x) _NOEXCEPT {return llabs(__x);} 03:04:23 ^ 03:04:23 /Users/osrf/jenkins/workspace/ci_osx/ws/src/ros2/rclcpp/rclcpp/src/rclcpp/duration.cpp:203:13: error: no member named 'isnormal' in namespace 'std' 03:04:23 if (!std::isnormal(scale)) { 03:04:23 ~~~~~^ 03:04:23 2 errors generated. 03:04:23 make[2]: *** [CMakeFiles/rclcpp.dir/build.make:212: CMakeFiles/rclcpp.dir/src/rclcpp/duration.cpp.o] Error 1 03:04:23 make[1]: *** [CMakeFiles/Makefile2:855: CMakeFiles/rclcpp.dir/all] Error 2 03:04:23 make: *** [Makefile:141: all] Error 2 03:04:23

You want std::fabs: http://en.cppreference.com/w/cpp/numeric/math/fabs

Though, it should probably work anyways...

You might just need to #include <cmath>. The standard seems to have been inconsistent on that in the past, which would allow for two different implementations to give you different results.

Ok, I'll explicitly add #include , thank you

dhood · 2018-02-23T21:27:23Z

Linux
Linux-aarch64
macOS
Windows

dhood · 2018-02-24T00:43:50Z

rclcpp/src/rclcpp/duration.cpp

+    this->rcl_duration_.nanoseconds,
+    scale,
+    std::numeric_limits<rcl_duration_value_t>::max());
+  return Duration(rcl_duration_.nanoseconds * scale);


this is giving a warning on windows: http://ci.ros2.org/job/ci_windows/4078/

'argument': conversion from 'double' to 'rcl_duration_value_t', possible loss of data

I guess a explicit cast can avoid this warning, however locally I have no windows build environment, please help re-trigger CI to check.

sloretz · 2018-02-27T00:47:41Z

CI with 3dacb27

Linux
Linux-aarch64
macOS
Windows

jwang11 · 2018-03-06T00:37:36Z

Windows warning disappeared. From log, the failure on macOS and Windows are python test script to execute command, which should not relate to this PR.

dhood · 2018-03-08T19:06:48Z

rclcpp/src/rclcpp/duration.cpp

+Duration
+Duration::operator*(double scale) const
+{
+  if (!std::isnormal(scale)) {


@tfoote is it appropriate to prevent users from having a scale of 0 here? Seems too strict

Yeah, a zero scale value would be valid. I think isfinite would be a better check.

I agree, in theory zero is valid to scale although this operation is meaningless. Ok, I'll update code from isnormal to isfinite which allow both normal and zero.

Duration scale is a convinient operation which had supported in ROS. This commit make ROS2 support it. Signed-off-by: jwang <jing.j.wang@intel.com>

Signed-off-by: jwang <jing.j.wang@intel.com>

*) Keep abs_scale double, not cast to uint64_t *) drop unnecessary check under condition abs_scale < 1.0 *) Correct warning text *) Add overflow/underflow test when do scaling Signed-off-by: jwang <jing.j.wang@intel.com>

Signed-off-by: jwang <jing.j.wang@intel.com>

jwang11 · 2018-03-16T00:45:39Z

If this PR is still under review by someone? Actually I have a porting work from ROS1 to ROS2 which reply on this patch, is it possible expedite the process? thanks

tfoote · 2018-03-16T00:57:01Z

@jwang11 It's good to comment back when the existing reviewers comments are addressed so we know it's ready to look at again.

I've restarted CI:

Linux
Linux-aarch64
macOS
Windows

tfoote · 2018-03-16T02:11:46Z

Unfortunately the branch was now out of date. I rebased it to https://github.com/ros2/rclcpp/tree/duration_scale

And started CI again.

Linux
Linux-aarch64
macOS
Windows

tfoote · 2018-03-16T07:54:44Z

The only failures are composition 2 on osx and 4 on windows: ros2/build_farmer#86

tfoote · 2018-03-16T07:58:08Z

Squashed rebased and merged in af6e86c

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

tfoote reviewed Feb 14, 2018

View reviewed changes

dirk-thomas reviewed Feb 14, 2018

View reviewed changes

mikaelarguedas added the in review Waiting for review (Kanban column) label Feb 14, 2018

tfoote previously requested changes Feb 15, 2018

View reviewed changes

tfoote reviewed Feb 22, 2018

View reviewed changes

dhood reviewed Feb 24, 2018

View reviewed changes

dhood reviewed Mar 8, 2018

View reviewed changes

jwang11 force-pushed the master branch from 3dacb27 to 14cc1fc Compare March 9, 2018 01:06

jwang11 and others added 8 commits March 9, 2018 16:46

Make rclcpp::Duration support scale operation

efd04b4

Duration scale is a convinient operation which had supported in ROS. This commit make ROS2 support it. Signed-off-by: jwang <jing.j.wang@intel.com>

add bound check and drop unused test

08331fe

Signed-off-by: jwang <jing.j.wang@intel.com>

fix several issues according to comments

f2c9376

*) Keep abs_scale double, not cast to uint64_t *) drop unnecessary check under condition abs_scale < 1.0 *) Correct warning text *) Add overflow/underflow test when do scaling Signed-off-by: jwang <jing.j.wang@intel.com>

add tests for negative base for underflow overflow checks

910fe01

Explictly include <cmath> to avoid ambiguous in OSX

b6b04c2

Signed-off-by: jwang <jing.j.wang@intel.com>

ordering fixup

db32422

avoid windows warning for argument conversion

13db0da

Signed-off-by: jwang <jing.j.wang@intel.com>

Update scale check to allow zero

14cc1fc

Signed-off-by: jwang <jing.j.wang@intel.com>

tfoote approved these changes Mar 16, 2018

View reviewed changes

tfoote closed this Mar 16, 2018

tfoote removed the in review Waiting for review (Kanban column) label Mar 16, 2018

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

Correct launch_testing.assert.assertExitCodes usage (ros2#437)

7984f94

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make rclcpp::Duration support scale operation #437

Make rclcpp::Duration support scale operation #437

jwang11 commented Feb 13, 2018

tfoote Feb 14, 2018

dirk-thomas Feb 14, 2018

jwang11 Feb 14, 2018 •

edited

Loading

mikaelarguedas commented Feb 15, 2018

tfoote left a comment

tfoote Feb 15, 2018

tfoote Feb 15, 2018

tfoote Feb 15, 2018

tfoote Feb 15, 2018

tfoote Feb 15, 2018

jwang11 Feb 16, 2018

dirk-thomas commented Feb 15, 2018

tfoote commented Feb 21, 2018 •

edited

Loading

tfoote left a comment

tfoote Feb 22, 2018

wjwwood Feb 22, 2018

wjwwood Feb 22, 2018

wjwwood Feb 22, 2018

jwang11 Feb 22, 2018

dhood commented Feb 23, 2018

dhood Feb 24, 2018

jwang11 Feb 24, 2018

sloretz commented Feb 27, 2018

jwang11 commented Mar 6, 2018

dhood Mar 8, 2018

tfoote Mar 8, 2018

jwang11 Mar 9, 2018

jwang11 commented Mar 16, 2018

tfoote commented Mar 16, 2018 •

edited

Loading

tfoote commented Mar 16, 2018

tfoote commented Mar 16, 2018

tfoote commented Mar 16, 2018

Make rclcpp::Duration support scale operation #437

Make rclcpp::Duration support scale operation #437

Conversation

jwang11 commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwang11 Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

mikaelarguedas commented Feb 15, 2018

tfoote 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

dirk-thomas commented Feb 15, 2018

tfoote commented Feb 21, 2018 • edited Loading

tfoote 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

dhood commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloretz commented Feb 27, 2018

jwang11 commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwang11 commented Mar 16, 2018

tfoote commented Mar 16, 2018 • edited Loading

tfoote commented Mar 16, 2018

tfoote commented Mar 16, 2018

tfoote commented Mar 16, 2018

jwang11 Feb 14, 2018 •

edited

Loading

tfoote commented Feb 21, 2018 •

edited

Loading

tfoote commented Mar 16, 2018 •

edited

Loading