Alternative way of giving timers a clock by sloretz · Pull Request #212 · ros2/rclpy

sloretz · 2018-07-27T18:43:13Z

I was having trouble putting feedback for #211 into words. This passes the clock into rclpy_create_timer() instead of it being returned in an unused variable, and makes use of the cleanup code in _rclpy_destroy_clock().

1) Pass clock into rclpy_create_timer 2) Make use of pycapsule destructor

sloretz · 2018-07-27T18:46:04Z

rclpy/rclpy/node.py

            if tmr.timer_handle == timer.timer_handle:
                _rclpy.rclpy_destroy_entity(tmr.timer_handle)
+                # TODO(sloretz) Store clocks on node and destroy them separately
+                _rclpy.rclpy_destroy_entity(tmr._clock._clock_handle)


A ROSTime clock would need to be fed from a time source that is tied to the node. Clocks may need to be gotten off a Node instance, so it would make sense for that instance to store and destroy them later.

sloretz · 2018-07-27T18:47:38Z

rclpy/rclpy/timer.py

-        [self.timer_handle, self.timer_pointer, _] = _rclpy.rclpy_create_timer(timer_period_ns)
+        # TODO(sloretz) Allow passing clocks in via timer constructor
+        self._clock = Clock(clock_type=ClockType.STEADY_TIME)
+        [self.timer_handle, self.timer_pointer] = _rclpy.rclpy_create_timer(


The returned clock was unused. Since a ROSTime clock may need to be retrieved off a Node it makes sense to pass the clock in, and reuse the clock creation code in Clock.

sloretz · 2018-07-27T18:48:05Z

rclpy/src/rclpy/_rclpy.c


+/// Destructor for a clock
+void
+_rclpy_destroy_clock(PyObject * pycapsule)


Moving this above to reuse it in rclpy_destroy_entity

sloretz · 2018-07-27T18:51:06Z

rclpy/src/rclpy/_rclpy.c

-    PyMem_Free(clock);
+  } else if (PyCapsule_IsValid(pyentity, "rcl_clock_t")) {
+    PyCapsule_SetDestructor(pyentity, NULL);
+    _rclpy_destroy_clock(pyentity);


The pycapsule destructor will fini the clock if the user doesn't. If the clock is destroyed explicitly then fini the clock, but set the destructor to NULL so the garbage collector doesn't do it again.

sloretz · 2018-07-27T18:54:28Z

@dirk-thomas if the changes look OK to you feel free to merge this PR. It's targeted at branch timer-with-clock

dirk-thomas

The change makes sense to me. Inline just a few minor comments. Can you also trigger a CI build for it?

dirk-thomas · 2018-07-27T20:52:44Z

rclpy/src/rclpy/_rclpy.c

+  PyObject * pyclock;

-  if (!PyArg_ParseTuple(args, "K", &period_nsec)) {
+  if (!PyArg_ParseTuple(args, "KO", &period_nsec, &pyclock)) {


Should the clock be based first and the period second (as in the other languages)?

dirk-thomas · 2018-07-27T20:54:09Z

rclpy/src/rclpy/_rclpy.c

+void
+_rclpy_destroy_clock(PyObject * pycapsule)
+{
+  rcl_clock_t * clock = (rcl_clock_t *)PyCapsule_GetPointer(pycapsule, "rcl_clock_t");


The code should check for !clock.

So currently it silently returns?

Not silent, it raises an exception. There's no return because the function signature is PyCapsule_Destructor.

Sorry, I still don't understand it. If clock is NULL that means a wrong parameter was passed - not a rcl_clock_t capsule. How is this early return different from the case where clock is not null and correctly finalized?

How is this early return different from the case where clock is not null and correctly finalized?

If PyCapsule_GetPointer() returns NULL then it raised an exception. If the clock is correctly finalized then no exception is raised.

If clock is NULL that means a wrong parameter was passed

I don't see a way to pass a wrong parameter to this function at the moment. It shouldn't get the wrong type of pointer when using it as a capsule destructor.

rclpy/rclpy/src/rclpy/_rclpy.c

Line 3184 in 5ed7ca2

return PyCapsule_New(clock, "rcl_clock_t", _rclpy_destroy_clock);

And the new call in this PR calls PyCapsule_IsValid() first. PyCapsule_GetPointer() is guaranteed not to return NULL if that passes.

Thanks for the detailed description. I guess I was kind of confused by the comment - I updated it in a new commit: a6934ca

dirk-thomas · 2018-07-27T20:55:30Z

rclpy/rclpy/node.py

            if tmr.timer_handle == timer.timer_handle:
                _rclpy.rclpy_destroy_entity(tmr.timer_handle)
+                # TODO(sloretz) Store clocks on node and destroy them separately
+                _rclpy.rclpy_destroy_entity(tmr._clock._clock_handle)


Instead of accessing a "protected" member (tmr._clock) should this be exposed through a getter?

sloretz · 2018-07-27T22:20:41Z

@dirk-thomas CI testing rclpy with this branch + rcl timer-with-clock

Linux
Linux-aarch64
- Connext not installed? Another run with connext unchecked
macOS
Windows

dirk-thomas · 2018-07-27T23:02:09Z

Connext not installed?

Fixed in ros2/ci#212 (comment)

* create timer with clock * fix wrong PyMem_Free argument * fix clock cleanup * sync with master * Alternative way of giving timers a clock (#212) * Alternative way of giving timers a clock 1) Pass clock into rclpy_create_timer 2) Make use of pycapsule destructor

Alternative way of giving timers a clock

d6bcef8

1) Pass clock into rclpy_create_timer 2) Make use of pycapsule destructor

sloretz requested a review from dirk-thomas July 27, 2018 18:43

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

sloretz commented Jul 27, 2018

View reviewed changes

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

dirk-thomas reviewed Jul 27, 2018

View reviewed changes

sloretz added 4 commits July 27, 2018 15:15

Fix uninitialized variable

a85059f

Swap order of period and clock

61fac0c

NULL clock check

d82ca21

add timer.clock getter

d4c43c6

update comment

a6934ca

dirk-thomas approved these changes Jul 27, 2018

View reviewed changes

sloretz merged commit aa42806 into timer-with-clock Jul 27, 2018

sloretz deleted the timer-with-clock-alternative branch July 27, 2018 23:14

sloretz removed the in review Waiting for review (Kanban column) label Jul 27, 2018

Conversation

sloretz commented Jul 27, 2018

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!

sloretz commented Jul 27, 2018

Uh oh!

dirk-thomas left a comment

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!

sloretz commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dirk-thomas commented Jul 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sloretz commented Jul 27, 2018 •

edited

Loading