Conversation
|
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 The Overall I tried to mirror the implementation in rclcpp, where possible. It diverges in the following ways:
|
rclpy/src/rclpy/_rclpy.c
Outdated
| { | ||
| PyObject * pyclock; | ||
| PyObject * pyenabled; | ||
| if (!PyArg_ParseTuple(args, "OO", &pyclock, &pyenabled)) { |
There was a problem hiding this comment.
Could use p (link here) to convert to bool automatically instead of PyObject_IsTrue() below.
rclpy/src/rclpy/_rclpy.c
Outdated
| return NULL; | ||
| } | ||
|
|
||
| bool enabled = PyObject_IsTrue(pyenabled); |
There was a problem hiding this comment.
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__().
rclpy/rclpy/time_source.py
Outdated
| 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 |
There was a problem hiding this comment.
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()There was a problem hiding this comment.
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
| # 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ooops, I got this completely wrong. For some reason I thought 1e9 meant 1ns. My bad.
|
|
Interestingly OS X (only) is failing with |
|
Back in action!
|
Connects to #186
Requires ros2/rcl#274
Gives nodes a clock of type ROS_TIME which will reflect the time received on
/clocktopic if the Time Source has had use of ROS time activated. Since parameters aren't available in python yet, there's no support for theuse_sim_timeparameter, but the Time Source can have ROS time activated manually by callingtime_source.ros_time_is_active = True.Support for clock jump notifiers not targeted for this PR.