Clock API improvements (#580)

* Improve rcl clock API documentation.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

* Improve rcl clock API error checking.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

* Address peer review comments.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

* Refine rcl_clock_t API thread-safety documentation.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

rcl/include/rcl/time.h

@@ -159,6 +159,14 @@ typedef struct rcl_time_point_t
  * are not invalid.
  * Note that if data is uninitialized it may give a false positive.
  *
+ * <hr>
+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | No
+ * Thread-Safe        | Yes
+ * Uses Atomics       | No
+ * Lock-Free          | Yes
+ *
  * \param[in] clock the handle to the clock which is being queried
  * \return true if the source is believed to be valid, otherwise return false.
  */
@@ -171,6 +179,18 @@ rcl_clock_valid(rcl_clock_t * clock);
 /**
  * This will allocate all necessary internal structures, and initialize variables.
  *
+ * <hr>
+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | Yes [1]
+ * Thread-Safe        | No [2]
+ * Uses Atomics       | No
+ * Lock-Free          | Yes
+ *
+ * <i>[1] If `clock_type` is `RCL_ROS_TIME`</i>
+ * <i>[2] Function is reentrant, but concurrent calls on the same `clock` object are not safe.
+ *        Thread-safety is also affected by that of the `allocator` object.</i>
+ *
  * \param[in] clock_type the type identifying the time source to provide
  * \param[in] clock the handle to the clock which is being initialized
  * \param[in] allocator The allocator to use for allocations
@@ -193,6 +213,21 @@ rcl_clock_init(
  * Passing a clock with type RCL_CLOCK_UNINITIALIZED will result in
  * RCL_RET_INVALID_ARGUMENT being returned.
  *
+ * This function is not thread-safe with any other function operating on the same
+ * clock object.
+ *
+ * <hr>
+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | No
+ * Thread-Safe        | No [1]
+ * Uses Atomics       | No
+ * Lock-Free          | Yes
+ *
+ * <i>[1] Function is reentrant, but concurrent calls on the same `clock` object are not safe.
+ *        Thread-safety is also affected by that of the `allocator` object associated with the
+ *        `clock` object.</i>
+ *
  * \param[in] clock the handle to the clock which is being finalized
  * \return `RCL_RET_OK` if the time source was successfully finalized, or
  * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
@@ -209,6 +244,17 @@ rcl_clock_fini(
  * This will allocate all necessary internal structures, and initialize variables.
  * It is specifically setting up a RCL_ROS_TIME time source.
  *
+ * <hr>
+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | Yes
+ * Thread-Safe        | No [1]
+ * Uses Atomics       | No
+ * Lock-Free          | Yes
+ *
+ * <i>[2] Function is reentrant, but concurrent calls on the same `clock` object are not safe.
+ *        Thread-safety is also affected by that of the `allocator` object.</i>
+ *
  * \param[in] clock the handle to the clock which is being initialized
  * \param[in] allocator The allocator to use for allocations
  * \return `RCL_RET_OK` if the time source was successfully initialized, or
@@ -228,6 +274,21 @@ rcl_ros_clock_init(
  * It is specifically setting up a `RCL_ROS_TIME` time source. It is expected
  * to be paired with the init fuction.
  *
+ * This function is not thread-safe with any other function operating on the same
+ * clock object.
+ *
+ * <hr>
+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | No
+ * Thread-Safe        | No [1]
+ * Uses Atomics       | No
+ * Lock-Free          | Yes
+ *
+ * <i>[1] Function is reentrant, but concurrent calls on the same `clock` object are not safe.
+ *        Thread-safety is also affected by that of the `allocator` object associated with the
+ *        `clock` object.</i>
+ *
  * \param[in] clock the handle to the clock which is being initialized
  * \return `RCL_RET_OK` if the time source was successfully finalized, or
  * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
@@ -244,6 +305,17 @@ rcl_ros_clock_fini(
  * This will allocate all necessary internal structures, and initialize variables.
  * It is specifically setting up a `RCL_STEADY_TIME` time source.
  *
+ * <hr>
+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | No
+ * Thread-Safe        | No [1]
+ * Uses Atomics       | No
+ * Lock-Free          | Yes
+ *
+ * <i>[1] Function is reentrant, but concurrent calls on the same `clock` object are not safe.
+ *        Thread-safety is also affected by that of the `allocator` object.</i>
+ *
  * \param[in] clock the handle to the clock which is being initialized
  * \param[in] allocator The allocator to use for allocations
  * \return `RCL_RET_OK` if the time source was successfully initialized, or
@@ -265,6 +337,21 @@ rcl_steady_clock_init(
  * It is specifically setting up a steady time source. It is expected to be
  * paired with the init fuction.
  *
+ * This function is not thread-safe with any other function operating on the same
+ * clock object.
+ *
+ * <hr>
+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | No
+ * Thread-Safe        | No [1]
+ * Uses Atomics       | No
+ * Lock-Free          | Yes
+ *
+ * <i>[1] Function is reentrant, but concurrent calls on the same `clock` object are not safe.
+ *        Thread-safety is also affected by that of the `allocator` object associated with the
+ *        `clock` object.</i>
+ *
  * \param[in] clock the handle to the clock which is being initialized
  * \return `RCL_RET_OK` if the time source was successfully finalized, or
  * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
@@ -283,6 +370,18 @@ rcl_steady_clock_fini(
  * This will allocate all necessary internal structures, and initialize variables.
  * It is specifically setting up a system time source.
  *
+ * <hr>
+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | No
+ * Thread-Safe        | No [1]
+ * Uses Atomics       | No
+ * Lock-Free          | Yes
+ *
+ * <i>[1] Function is reentrant, but concurrent calls on the same `clock` object are not safe.
+ *        Thread-safety is also affected by that of the `allocator` object associated with the
+ *        `clock` object.</i>
+ *
  * \param[in] clock the handle to the clock which is being initialized
  * \param[in] allocator The allocator to use for allocations
  * \return `RCL_RET_OK` if the time source was successfully initialized, or
@@ -304,6 +403,20 @@ rcl_system_clock_init(
  * It is specifically setting up a system time source. It is expected to be paired with
  * the init fuction.
  *
+ * This function is not thread-safe with any function operating on the same clock object.
+ *
+ * <hr>
+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | No
+ * Thread-Safe        | No [1]
+ * Uses Atomics       | No
+ * Lock-Free          | Yes
+ *
+ * <i>[1] Function is reentrant, but concurrent calls on the same `clock` object are not safe.
+ *        Thread-safety is also affected by that of the `allocator` object associated with the
+ *        `clock` object.</i>
+ *
  * \param[in] clock the handle to the clock which is being initialized.
  * \return `RCL_RET_OK` if the time source was successfully finalized, or
  * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
@@ -324,6 +437,14 @@ rcl_system_clock_fini(
  * The value will be computed as duration = finish - start. If start is after
  * finish the duration will be negative.
  *
+ * <hr>
+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | No
+ * Thread-Safe        | Yes
+ * Uses Atomics       | No
+ * Lock-Free          | Yes
+ *
  * \param[in] start The time point for the start of the duration.
  * \param[in] finish The time point for the end of the duration.
  * \param[out] delta The duration between the start and finish.
@@ -341,6 +462,17 @@ rcl_difference_times(
 /**
  * This function will populate the data of the time_point_value object with the
  * current value from it's associated time abstraction.
+ *
+ * <hr>
+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | No
+ * Thread-Safe        | Yes
+ * Uses Atomics       | Yes [1]
+ * Lock-Free          | Yes
+ *
+ * <i>[1] If `clock` is of `RCL_ROS_TIME` type.</i>
+ *
  * \param[in] clock The time source from which to set the value.
  * \param[out] time_point_value The time_point value to populate.
  * \return `RCL_RET_OK` if the last call time was retrieved successfully, or
@@ -359,6 +491,21 @@ rcl_clock_get_now(rcl_clock_t * clock, rcl_time_point_value_t * time_point_value
  * such that the time source will report the set value instead of falling
  * back to system time.
  *
+ * This function is not thread-safe with `rcl_clock_add_jump_callback`,
+ * nor `rcl_clock_remove_jump_callback` functions when used on the same
+ * clock object.
+ *
+ * <hr>
+ * Attribute          | Adherence [1]
+ * ------------------ | -------------
+ * Allocates Memory   | No
+ * Thread-Safe        | No [2]
+ * Uses Atomics       | No
+ * Lock-Free          | Yes
+ *
+ * <i>[1] Only applies to the function itself, as jump callbacks may not abide to it.</i>
+ * <i>[2] Function is reentrant, but concurrent calls on the same `clock` object are not safe.</i>
+ *
  * \param[in] clock The clock to enable.
  * \return `RCL_RET_OK` if the time source was enabled successfully, or
  * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
@@ -375,6 +522,21 @@ rcl_enable_ros_time_override(rcl_clock_t * clock);
  * such that the time source will report the system time even if a custom
  * value has been set.
  *
+ * This function is not thread-safe with `rcl_clock_add_jump_callback`,
+ * nor `rcl_clock_remove_jump_callback` functions when used on the same
+ * clock object.
+ *
+ * <hr>
+ * Attribute          | Adherence [1]
+ * ------------------ | -------------
+ * Allocates Memory   | No
+ * Thread-Safe        | No [2]
+ * Uses Atomics       | No
+ * Lock-Free          | Yes
+ *
+ * <i>[1] Only applies to the function itself, as jump callbacks may not abide to it.</i>
+ * <i>[2] Function is reentrant, but concurrent calls on the same `clock` object are not safe.</i>
+ *
  * \param[in] clock The clock to disable.
  * \return `RCL_RET_OK` if the time source was disabled successfully, or
  * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
@@ -392,6 +554,19 @@ rcl_disable_ros_time_override(rcl_clock_t * clock);
  * time overide is enabled. If it is enabled, the set value will be returned.
  * Otherwise this time source will return the equivalent to system time abstraction.
  *
+ * This function is not thread-safe with `rcl_enable_ros_time_override` nor
+ * `rcl_disable_ros_time_override` functions when used on the same clock object.
+ *
+ * <hr>
+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | No
+ * Thread-Safe        | No [1]
+ * Uses Atomics       | No
+ * Lock-Free          | Yes
+ *
+ * <i>[1] Function is reentrant, but concurrent calls on the same `clock` object are not safe.</i>
+ *
  * \param[in] clock The clock to query.
  * \param[out] is_enabled Whether the override is enabled..
  * \return `RCL_RET_OK` if the time source was queried successfully, or
@@ -411,6 +586,21 @@ rcl_is_enabled_ros_time_override(
  * If queried and override enabled the time source will return this value,
  * otherwise it will return the system time.
  *
+ * This function is not thread-safe with `rcl_clock_add_jump_callback`,
+ * nor `rcl_clock_remove_jump_callback` functions when used on the same
+ * clock object.
+ *
+ * <hr>
+ * Attribute          | Adherence [1]
+ * ------------------ | -------------
+ * Allocates Memory   | No
+ * Thread-Safe        | No [2]
+ * Uses Atomics       | Yes
+ * Lock-Free          | Yes
+ *
+ * <i>[1] Only applies to the function itself, as jump callbacks may not abide to it.</i>
+ * <i>[2] Function is reentrant, but concurrent calls on the same `clock` object are not safe.</i>
+ *
  * \param[in] clock The clock to update.
  * \param[in] time_value The new current time.
  * \return `RCL_RET_OK` if the time source was set successfully, or
@@ -430,11 +620,28 @@ rcl_set_ros_time_override(
  * The user_data pointer is passed to the callback as the last argument.
  * A callback and user_data pair must be unique among the callbacks added to a clock.
  *
+ * This function is not thread-safe with `rcl_clock_remove_jump_callback`,
+ * `rcl_enable_ros_time_override`, `rcl_disable_ros_time_override` nor
+ * `rcl_set_ros_time_override` functions when used on the same clock object.
+ *
+ * <hr>
+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | Yes
+ * Thread-Safe        | No [1]
+ * Uses Atomics       | No
+ * Lock-Free          | Yes
+ *
+ * <i>[1] Function is reentrant, but concurrent calls on the same `clock` object are not safe.
+ *        Thread-safety is also affected by that of the `allocator` object associated with the
+ *        `clock` object.</i>
+ *
  * \param[in] clock A clock to add a jump callback to.
  * \param[in] threshold Criteria indicating when to call the callback.
  * \param[in] callback A callback to call.
  * \param[in] user_data A pointer to be passed to the callback.
  * \return `RCL_RET_OK` if the callback was added successfully, or
+ * \return `RCL_RET_BAD_ALLOC` if a memory allocation failed, or
  * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
  * \return `RCL_RET_ERROR` an unspecified error occurs.
  */
@@ -447,11 +654,28 @@ rcl_clock_add_jump_callback(
 
 /// Remove a previously added time jump callback.
 /**
+ * This function is not thread-safe with `rcl_clock_add_jump_callback`
+ * `rcl_enable_ros_time_override`, `rcl_disable_ros_time_override` nor
+ * `rcl_set_ros_time_override` functions when used on the same clock object.
+ *
+ * <hr>
+ * Attribute          | Adherence
+ * ------------------ | -------------
+ * Allocates Memory   | Yes
+ * Thread-Safe        | No [1]
+ * Uses Atomics       | No
+ * Lock-Free          | Yes
+ *
+ * <i>[1] Function is reentrant, but concurrent calls on the same `clock` object are not safe.
+ *        Thread-safety is also affected by that of the `allocator` object associated with the
+ *        `clock` object.</i>
+ *
  * \param[in] clock The clock to remove a jump callback from.
  * \param[in] threshold Criteria indicating when to call callback.
  * \param[in] callback The callback to call.
  * \param[in] user_data A pointer to be passed to the callback.
  * \return `RCL_RET_OK` if the callback was added successfully, or
+ * \return `RCL_RET_BAD_ALLOC` if a memory allocation failed, or
  * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or
  * \return `RCL_RET_ERROR` the callback was not found or an unspecified error occurs.
  */

rcl/src/rcl/time.c

@@ -232,6 +232,9 @@ rcl_ret_t
 rcl_difference_times(
   rcl_time_point_t * start, rcl_time_point_t * finish, rcl_duration_t * delta)
 {
+  RCL_CHECK_ARGUMENT_FOR_NULL(start, RCL_RET_INVALID_ARGUMENT);
+  RCL_CHECK_ARGUMENT_FOR_NULL(finish, RCL_RET_INVALID_ARGUMENT);
+  RCL_CHECK_ARGUMENT_FOR_NULL(delta, RCL_RET_INVALID_ARGUMENT);
   if (start->clock_type != finish->clock_type) {
     RCL_SET_ERROR_MSG("Cannot difference between time points with clocks types.");
     return RCL_RET_ERROR;