TimeSource and support for ROS time #210

dhood · 2018-07-20T11:13:25Z

Connects to #186

Gives nodes a clock of type ROS_TIME which will reflect the time received on /clock topic if the Time Source has had use of ROS time activated. Since parameters aren't available in python yet, there's no support for the use_sim_time parameter, but the Time Source can have ROS time activated manually by calling time_source.ros_time_is_active = True.

Support for clock jump notifiers not targeted for this PR.

dhood · 2018-07-31T00:41:28Z

Putting this in review

The description of this PR has been updated to reflect a recent change to the code: Following discussion in ros2/rclcpp#516 I understand now that all clocks managed by a time source should have the same value for ros_time_is_active. So, I have added ros_time_is_active to the TimeSource class, and made it read-only on the ROSClock class (before clocks could individually have use of ROS time configured on them, now it is 'inherited' from the TimeSource's state).

The ROSClock class has also been added recently as a specialisation of the Clock class that provides ROS time-specific methods. A time source can still take in a clock like Clock(clock_type=ROS_TIME), but the expected usage is that it will be passed a ROSClock (that is what will be stored underneath). I did this because I thought it was cleaner than having methods that only applied for ROS time in the general Clock class (e.g. like this), but am open to alternatives.

Overall I tried to mirror the implementation in rclcpp, where possible. It diverges in the following ways:

Clock callbacks have not been implemented yet
Support for parameters isn't available in rclpy yet so the TimeSource has to have ros_time_is_active set manually as opposed to with the use_sim_time parameter
A ROSClock specialisation does not exist in rclcpp, but does in rclpy.
I aimed for the end-result from Node unable to avoid using sim time if /clock published rclcpp#515 where a subscription is only created for a Timesource if sim time is in use. That is not the current state in rclcpp however.

sloretz · 2018-08-02T00:28:23Z

rclpy/src/rclpy/_rclpy.c

+{
+  PyObject * pyclock;
+  PyObject * pyenabled;
+  if (!PyArg_ParseTuple(args, "OO", &pyclock, &pyenabled)) {


Could use p (link here) to convert to bool automatically instead of PyObject_IsTrue() below.

thanks! 6a9d42b

sloretz · 2018-08-02T00:29:44Z

rclpy/src/rclpy/_rclpy.c

+    return NULL;
+  }
+
+  bool enabled = PyObject_IsTrue(pyenabled);


Unlikely to see this happen, but I think this needs to check PyErr_Occurred() in case pyenabled is an object that raises an exception in it's implementation of __bool__().

sloretz · 2018-08-02T01:05:31Z

rclpy/rclpy/time_source.py

+        if not clock.clock_type == ClockType.ROS_TIME:
+            raise ValueError('Only clocks with type ROS_TIME can be attached.')
+        # "Cast" to ROSClock subclass so ROS time methods can be used.
+        clock.__class__ = ROSClock


Not asking to change anything, just sharing because I think it's interesting. This led me discover that __new__ could be used to return a ROSClock instance when passed a ROS clock type.

Details

class Clock: def __new__(cls, *, clock_type="SYSTEM_CLOCK"): self = None if clock_type == "ROS_CLOCK": self = super().__new__(ROSClock) else: self = super().__new__(cls) self._clock_type = clock_type # create self._clock_handle here too return self # Can't have an __init__ because it would get passed clock_type argument # and ROSCLOCK doesn't wan't one # def __init__(self): # pass def all_clocks_have_this(self): print("all clocks have this") class ROSClock(Clock): # Can't have an __init__ because it would get passed clock_type argument # and ROSCLOCK doesn't wan't one # def __init__(self): # pass def only_ros_clocks_have_this(self): print("Only ros clocks have this") c1 = Clock() c2 = Clock(clock_type="STEADY_CLOCK") c3 = Clock(clock_type="ROS_CLOCK") print(repr(c1)) print(repr(c2)) print(repr(c3)) c1.all_clocks_have_this() c2.all_clocks_have_this() c3.all_clocks_have_this() c3.only_ros_clocks_have_this()

I think this approach might be better! Because there's no extra step for users to get access to ROS clock methods if they have created a ROS clock with Clock(clock_type=ClockType.ROS_TIME), they will always get a ROSClock back.

I don't immediately see any disadvantages of this (it's ok in this instance that the Clock class 'knows about' the ROSClock class), so I plan to incorporate it, thank you for the suggestion

Done in 9992b1f + cf38ae7, I think it's better to use now: happy you pointed this out!

sloretz · 2018-08-02T01:21:11Z

rclpy/test/test_time_source.py

+        # When not using sim time, ROS time should look like system time
+        now = clock.now()
+        system_now = Clock(clock_type=ClockType.SYSTEM_TIME).now()
+        assert (system_now.nanoseconds - now.nanoseconds) < 1e9


1e9 seems really small. I think this test could be flaky if the OS happens to context switch between lines 73 and 74.

Also, maybe add abs() in case there is a future bug where now is a non-zero time greater than system_now?

I was thinking 1sec is pretty generous already but it can't hurt to increase.

About the abs, I don't think it makes sense here, since we do expect the value of a later call to now() be greater. I'd rather catch that bug than mask it.

Ooops, I got this completely wrong. For some reason I thought 1e9 meant 1ns. My bad.

dhood · 2018-08-03T18:50:13Z

Linux
Linux-aarch64
macOS
Windows

sloretz

LGTM with green CI

dhood · 2018-08-03T20:57:22Z

Interestingly OS X (only) is failing with ValueError: PyCapsule_GetPointer called with invalid PyCapsule object when _rclpy.rclpy_clock_set_ros_time_override_is_enabled(self._clock_handle, enabled) is called. Not sure yet what's causing that: will have to look into it before this can be merged

dhood · 2018-08-03T21:30:41Z

Back in action!
Will go ahead and merge this now the bug's been fixed in 6ac5bf9

Linux
Linux-aarch64
macOS
Windows

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

dhood self-assigned this Jul 20, 2018

tfoote mentioned this pull request Jul 24, 2018

Implement Time Features in rclpy #186

Closed

8 tasks

dhood added 5 commits July 30, 2018 11:55

Add first version of TimeSource

3942ac4

Add ability to set if ROS time override is enabled

5451fe1

Publisher on /clock shouldn't affect non-sim-time clock

3df0a7e

Test custom getter/setter for ROS time is enabled

d64dba0

Give node a clock with ROS time

0670f5b

dhood force-pushed the ros_time branch from e8a96f8 to 0670f5b Compare July 30, 2018 18:57

dhood changed the base branch from rclpy_time to master July 30, 2018 18:57

dhood added 6 commits July 30, 2018 12:50

Set clock's use of sim time from time source's use

df7bca9

Make setting of Clock's use of ROS time private

be9aaca

Only subscribe to clock topic if ROS time is active

5789eb5

More tests

a835e8b

Error if the _node has been removed but not sub

69a98ca

Create ROSClock specialization

c1b59b5

dhood force-pushed the ros_time branch from 6e4b278 to c1b59b5 Compare July 31, 2018 00:03

Remove checks for clock type of ROS_TIME in ROSClock

cb6245c

dhood mentioned this pull request Jul 31, 2018

Copy allocator passed in to clock init ros2/rcl#274

Merged

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

sloretz reviewed Aug 2, 2018

View reviewed changes

dhood added 3 commits August 2, 2018 18:12

Parse predicate instead of using PyObject_IsTrue

6a9d42b

Return a ROSClock from Clock constructor if appropriate

9992b1f

Correctly initialise clock type for ROSClocks instantiated directly

cf38ae7

dhood mentioned this pull request Aug 3, 2018

Set error message when clock type is not ROS_TIME ros2/rcl#275

Merged

sloretz approved these changes Aug 3, 2018

View reviewed changes

Predicate is parsed to an int not bool

6ac5bf9

dhood merged commit 98cdade into master Aug 3, 2018

dhood deleted the ros_time branch August 3, 2018 21:44

dhood removed the in review Waiting for review (Kanban column) label Aug 3, 2018

mikaelarguedas mentioned this pull request Aug 18, 2018

Enable ros2tpoic hz command ros2/ros2cli#133

Merged

dhood mentioned this pull request Sep 20, 2018

Respect the use_sim_time parameter for rclpy nodes #240

Closed

2 tasks

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

TimeSource and support for ROS time #210

TimeSource and support for ROS time #210

dhood commented Jul 20, 2018 •

edited

dhood commented Jul 31, 2018

sloretz Aug 2, 2018

dhood Aug 3, 2018

sloretz Aug 2, 2018

sloretz Aug 2, 2018

dhood Aug 2, 2018

dhood Aug 3, 2018

sloretz Aug 2, 2018

dhood Aug 2, 2018

sloretz Aug 2, 2018

dhood commented Aug 3, 2018 •

edited

sloretz left a comment

dhood commented Aug 3, 2018

dhood commented Aug 3, 2018

TimeSource and support for ROS time #210

TimeSource and support for ROS time #210

Conversation

dhood commented Jul 20, 2018 • edited

dhood commented Jul 31, 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

dhood commented Aug 3, 2018 • edited

sloretz left a comment

Choose a reason for hiding this comment

dhood commented Aug 3, 2018

dhood commented Aug 3, 2018

dhood commented Jul 20, 2018 •

edited

dhood commented Aug 3, 2018 •

edited