Time, Duration, Clock wrapping rcl #209

dhood · 2018-07-18T06:34:57Z

connects to #186

Still a work-in-progress but input is welcome.

The scope of this PR is the first few steps of #186 (comment). It excludes TimeSource/ROS time implementation to avoid the PR getting too large.

sloretz · 2018-07-18T17:31:51Z

rclpy/src/rclpy/_rclpy.c

+    return NULL;
+  }
+
+  rcl_time_point_t * time_point = (rcl_time_point_t *) PyMem_Malloc(sizeof(rcl_time_point_t));


Need to check time_point before using. If PyMem_Malloc() fails then time_point will be NULL.

sloretz · 2018-07-18T17:44:48Z

rclpy/src/rclpy/_rclpy.c

+  time_point->nanoseconds = nanoseconds;
+  time_point->clock_type = clock_type;
+
+  return PyCapsule_New(time_point, "rcl_time_point_t", NULL);


This should have a PyCapsule_Destructor to free time_point if the capsule gets garbage collected.

void _rclpy_destroy_time_point(PyObject * pycapsule) { PyMem_Free(PyCapsule_GetPointer(pycapsule, "rcl_time_point_t")); } ... PyCapsule_New(time_point, "rcl_time_point_t", _rclpy_destroy_time_point);

The rest of rclpy is not using capsule destructors, but that's a bug imo.

thanks for the pointer!! hadn't seen that before. 2c8ba12

sloretz · 2018-07-18T17:48:09Z

rclpy/src/rclpy/_rclpy.c

+/// Returns the nanoseconds value of the time point
+/**
+ * Raises ValueError if pytime_point is not a time point capsule
+ * Raises RuntimeError if the time point valule cannot be retrieved


valule -> value

Where is RuntimeError raised?

sloretz · 2018-07-18T17:58:20Z

rclpy/src/rclpy/_rclpy.c

+
+  duration->nanoseconds = nanoseconds;
+
+  return PyCapsule_New(duration, "rcl_duration_t", NULL);


Same capsule destructor needed here

sloretz · 2018-07-18T18:02:11Z

rclpy/src/rclpy/_rclpy.c

+    return NULL;
+  }
+
+  rcl_clock_t * clock = (rcl_clock_t *)PyMem_Malloc(sizeof(rcl_clock_t));


check if clock is NULL or is this relying on rcl_clock_init to do that?

sloretz · 2018-07-18T18:02:35Z

rclpy/src/rclpy/_rclpy.c

+    return NULL;
+  }
+
+  return PyCapsule_New(clock, "rcl_clock_t", NULL);


Need capsule destructor here too

sloretz · 2018-07-18T18:06:29Z

rclpy/src/rclpy/_rclpy.c

+    return NULL;
+  }
+
+  rcl_time_point_t * time_point = (rcl_time_point_t *) PyMem_Malloc(sizeof(rcl_time_point_t));


Need to check for NULL

sloretz · 2018-07-18T18:07:42Z

rclpy/src/rclpy/_rclpy.c

+    return NULL;
+  }
+
+  return PyCapsule_New(time_point, "rcl_time_point_t", NULL);


Capsule destructor needed

sloretz · 2018-07-18T18:41:50Z

rclpy/test/test_time.py

+
+    def test_time_operators(self):
+        time1 = Time(nanoseconds=1, clock_type=ClockType.STEADY_TIME)
+        time2 = time1


What is this supposed to test? Assignment makes time2 and time1 be references to the same instance of Time.

>>> t1 = Time(nanoseconds=1) >>> t2 = t1 >>> id(t2) 140468253530040 >>> id(t1) 140468253530040 >>> id(t2) == id(t1) True

ah, my intent was more to check how the copies interacted. Updated in 5a1430e

Don't want users accidentally doing Time() == 0 and getting False

dhood · 2018-07-19T08:35:11Z

Putting this in review:

Linux
Linux-aarch64
macOS
Windows

dhood · 2018-07-20T00:06:29Z

CI without typo in args:

Linux
Linux-aarch64
macOS
Windows

sloretz

LGTM. My comments are about code that could be removed, but don't change anything.

sloretz · 2018-07-24T18:16:40Z

rclpy/src/rclpy/_rclpy.c

+  }
+
+  unsigned PY_LONG_LONG nanoseconds = PyLong_AsUnsignedLongLong(pylong_nanoseconds);
+  if ((unsigned PY_LONG_LONG)-1 == nanoseconds && PyErr_Occurred()) {


nit, PyErr_Occurred() alone is enough to justify returning NULL.

I must have been following how to adapt to overflow errors, which we're not doing. Check relaxed in 69e838f

sloretz · 2018-07-24T18:29:30Z

rclpy/src/rclpy/_rclpy.c

+  }
+
+  unsigned PY_LONG_LONG nanoseconds = PyLong_AsUnsignedLongLong(pylong_nanoseconds);
+  if ((unsigned PY_LONG_LONG)-1 == nanoseconds && PyErr_Occurred()) {


nit, same as above

sloretz · 2018-07-24T19:19:30Z

rclpy/test/test_time.py

+            duration = Duration(nanoseconds=-1)
+
+    def test_time_operators(self):
+        # Check that modifying the original doesn't affect a copy


This code doesn't make a copy; though, I don't think a copy test is necessary.

# make a reference to a new instance of Time() (instance 1) called time1 time1 = Time(nanoseconds=2, clock_type=ClockType.SYSTEM_TIME) # make another reference to (instance 1) called time2 time2 = time1 # make time 1 a reference to a new instance of Time() (instance 2) time1 = Time(nanoseconds=1, clock_type=ClockType.STEADY_TIME)

Which is the same as

time2 = Time(nanoseconds=2, clock_type=ClockType.SYSTEM_TIME) time1 = Time(nanoseconds=1, clock_type=ClockType.STEADY_TIME)

A copy would need to be created with copy.copy() or copy.deepcopy(). A shallow copy test isn't necessary because Time appears to be immutable.

import copy # make a reference to a new instance of Time() (instance 1) called time1 time1 = Time(nanoseconds=2, clock_type=ClockType.SYSTEM_TIME) # make a reference to a new instance of Time() (instance 2) called time2 whose members have been shallow copied from time1. time2 = copy.copy(Time(nanoseconds=2, clock_type=ClockType.SYSTEM_TIME)) # Not implemented, time properties don't have setter's # time2.nanoseconds = 5 # time2.clock_type = ClockType.ROS_TIME # If time was mutable this would fail because time2._time_handle and time1._time_handle are references to the same pycapsule # assert time2.nanosseconds != time1.nanoseconds

and a deepcopy test isn't needed because Time() doesn't support it (would need to implement __getstate__() or __setstate__())

# time2 = copy.deepcopy(time1) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/usr/lib/python3.6/copy.py", line 180, in deepcopy y = _reconstruct(x, memo, *rv) File "/usr/lib/python3.6/copy.py", line 280, in _reconstruct state = deepcopy(state, memo) File "/usr/lib/python3.6/copy.py", line 150, in deepcopy y = copier(x, memo) File "/usr/lib/python3.6/copy.py", line 240, in _deepcopy_dict y[deepcopy(key, memo)] = deepcopy(value, memo) File "/usr/lib/python3.6/copy.py", line 169, in deepcopy rv = reductor(4) TypeError: can't pickle PyCapsule objects

I'm with you that this wasn't really testing anything of interest; removed in 81d5568 thanks!

dhood · 2018-07-26T00:57:05Z

rclpy/src/rclpy/_rclpy.c

+  rcl_ret_t ret_clock = rcl_clock_fini(clock);
+  PyMem_Free(clock);
+  if (ret_clock != RCL_RET_OK) {
+    PyErr_Format(PyExc_RuntimeError,


@sloretz I had overlooked that I wasn't calling fini on the clock (added in 0cbbc52).

If something goes wrong in the fini when it gets garbage collected it will cause this sort of output:

RuntimeError: Failed to fini 'rcl_clock_t': error not set During handling of the above exception, another exception occurred: self = <contextlib._GeneratorContextManager object at 0x7f1210674cf8>, type = None, value = None, traceback = None def __exit__(self, type, value, traceback): if type is None: try: > next(self.gen) E SystemError: PyEval_EvalFrameEx returned a result with an error set /usr/lib/python3.5/contextlib.py:66: SystemError ----------- generated xml file: /home/dhood/ros2_ws/build/rclpy/test_results/rclpy/rclpytests.xunit.xml ----------- ============================================ 2 failed in 0.06 seconds ============================================= Exception ignored in: 'garbage collection' RuntimeError: Failed to fini 'rcl_clock_t': error not set Fatal Python error: unexpected exception during garbage collection Current thread 0x00007f1216872700 (most recent call first): Aborted (core dumped)

I think if there was an issue then this will certainly let the user know about it. Since you were the one that mentioned adding the destructor for garbage collection, I wonder if you've read about any other approaches to signalling issues in the destructor, or if this is the way to go?

Wow, the user can't miss that. Does PyErr_WarnEx() cause a fatal error as well?

I haven't read about other approaches. The only example I found in the cpython repo is this capsule destructor in ctypes which is seems to be silent about errors.

Using PyErr_WarnFormat, you end up getting something like:

----------- generated xml file: /home/dhood/ros2_ws/build/rclpy/test_results/rclpy/rclpytests.xunit.xml ----------- ================================================ warnings summary ================================================= src/ros2/rclpy/rclpy/test/test_clock.py::TestClock::test_clock_construction /home/dhood/ros2_ws/src/ros2/rclpy/rclpy/test/test_clock.py:33: RuntimeWarning: Failed to fini 'rcl_clock_t': error not set clock = Clock(clock_type=ClockType.STEADY_TIME) /home/dhood/ros2_ws/src/ros2/rclpy/rclpy/test/test_clock.py:35: RuntimeWarning: Failed to fini 'rcl_clock_t': error not set clock = Clock(clock_type=ClockType.SYSTEM_TIME) /usr/lib/python3.5/unittest/case.py:600: RuntimeWarning: Failed to fini 'rcl_clock_t': error not set testMethod() src/ros2/rclpy/rclpy/test/test_clock.py::TestClock::test_clock_now /home/dhood/ros2_ws/src/ros2/rclpy/rclpy/test/test_clock.py:53: RuntimeWarning: Failed to fini 'rcl_clock_t': error not set clock = Clock(clock_type=ClockType.STEADY_TIME) /usr/lib/python3.5/unittest/case.py:600: RuntimeWarning: Failed to fini 'rcl_clock_t': error not set testMethod() -- Docs: http://doc.pytest.org/en/latest/warnings.html ====================================== 2 passed, 5 warnings in 0.03 seconds =======================================

But I think now it's misleading because it doesn't make mention of the garbage collector so the line numbers are confusing. We do raise exceptions if there are fini errors usually, so if there aren't any alternatives in between, now I think that the exception is the right thing even if it's perhaps a bit extreme...

dhood · 2018-07-26T22:13:37Z

@sloretz from discussion in ros2/rclcpp#525, this implementation needs to allow negative Time and Durations, which currently it doesn't. My thinking, to assist reviewing, is to merge this PR with its current implementation (negative Time and Duration not supported) and create a followup that supports negatives, so that it's just the diff that needs to be reviewed incrementally. sound good?

sloretz · 2018-07-26T22:15:41Z

@dhood Sounds good to me

dhood · 2018-07-26T22:20:01Z

Thanks!

Linux
Linux-aarch64
macOS
Windows

dhood added 9 commits July 17, 2018 19:03

Minimal Time class

1a3ca66

Pass sec and ns to constructor

797c6da

Add Duration, difference between time points

69db0cd

Conversions to/from Time msg

9281ce4

Conversions to/from Duration msg

2162665

First version of Clock class

3b0d626

Rename functions

5ec565f

Give Times a clock type

5ce62b1

Support duration subtracted from time

cc3778c

dhood added the in progress Actively being worked on (Kanban column) label Jul 18, 2018

dhood self-assigned this Jul 18, 2018

sloretz reviewed Jul 18, 2018

View reviewed changes

dhood added 4 commits July 18, 2018 13:48

Store msg as sec and nanosec

76676b4

Check PyMem_Malloc successful

4b46324

Update docstrings

0b9a5eb

Add destructors for pycapsules

2c8ba12

dhood force-pushed the rclpy_time branch from 11dc1a7 to 2c8ba12 Compare July 18, 2018 21:07

dhood added 9 commits July 18, 2018 14:41

Copy test is for independence of objects

5a1430e

Default to ros time in from_msg, don't enforce it

f53a2b6

Comment updates

8f5ebad

Comparators for Time

38039b4

Don't let time be compared with other types

65e32cf

Don't want users accidentally doing Time() == 0 and getting False

Comparators for Duration

9b84d8f

Add/update __repr__

3f8ac95

Error if too-large value passed in

ae8b216

Errors for over/underflow from addition/subtraction

4cffbf1

dhood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jul 19, 2018

Document why __eq__ raises on diff types

ea2fbc0

dhood mentioned this pull request Jul 20, 2018

TimeSource and support for ROS time #210

Merged

sloretz approved these changes Jul 24, 2018

View reviewed changes

tfoote mentioned this pull request Jul 24, 2018

Implement Time Features in rclpy #186

Closed

8 tasks

dhood added 3 commits July 25, 2018 16:46

Remove copy test (update variable usage in other tests)

81d5568

Relax check for nanosec conversion

69e838f

Update clock destructor to call rcl_clock_fini

0cbbc52

dhood commented Jul 26, 2018

View reviewed changes

sloretz approved these changes Jul 26, 2018

View reviewed changes

dhood merged commit 5ed7ca2 into master Jul 26, 2018

dhood removed the in review Waiting for review (Kanban column) label Jul 26, 2018

dhood mentioned this pull request Jul 27, 2018

create timer with clock #211

Merged

dhood deleted the rclpy_time branch July 30, 2018 23:46

dhood mentioned this pull request Aug 10, 2018

Allow Duration to be negative #220

Merged

dirk-thomas mentioned this pull request Sep 21, 2018

Functionality around time ros2/ros2#530

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Time, Duration, Clock wrapping rcl #209

Time, Duration, Clock wrapping rcl #209

dhood commented Jul 18, 2018

sloretz Jul 18, 2018

sloretz Jul 18, 2018

dhood Jul 18, 2018

sloretz Jul 18, 2018

sloretz Jul 18, 2018

sloretz Jul 18, 2018

sloretz Jul 18, 2018

sloretz Jul 24, 2018

sloretz Jul 18, 2018

sloretz Jul 18, 2018

sloretz Jul 24, 2018

sloretz Jul 18, 2018

sloretz Jul 18, 2018

dhood Jul 18, 2018

dhood commented Jul 19, 2018

dhood commented Jul 20, 2018 •

edited

sloretz left a comment

sloretz Jul 24, 2018

dhood Jul 25, 2018

sloretz Jul 24, 2018

sloretz Jul 24, 2018

dhood Jul 25, 2018

dhood Jul 26, 2018

sloretz Jul 26, 2018

dhood Jul 26, 2018 •

edited

dhood commented Jul 26, 2018

sloretz commented Jul 26, 2018

dhood commented Jul 26, 2018


		duration->nanoseconds = nanoseconds;

		return PyCapsule_New(duration, "rcl_duration_t", NULL);

Time, Duration, Clock wrapping rcl #209

Time, Duration, Clock wrapping rcl #209

Conversation

dhood commented Jul 18, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhood commented Jul 19, 2018

dhood commented Jul 20, 2018 • edited

sloretz 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

dhood Jul 26, 2018 • edited

Choose a reason for hiding this comment

dhood commented Jul 26, 2018

sloretz commented Jul 26, 2018

dhood commented Jul 26, 2018

dhood commented Jul 20, 2018 •

edited

dhood Jul 26, 2018 •

edited