Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clock API improvements #580

Merged
merged 4 commits into from
Mar 13, 2020
Merged

Clock API improvements #580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
79792b5
Improve rcl clock API documentation.
hidmic Feb 20, 2020
a018f15
Improve rcl clock API error checking.
hidmic Feb 20, 2020
03d4a76
Address peer review comments.
hidmic Feb 21, 2020
ca78af8
Refine rcl_clock_t API thread-safety documentation.
hidmic Mar 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
224 changes: 224 additions & 0 deletions rcl/include/rcl/time.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,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.
*/
Expand All @@ -161,6 +169,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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should make it clear elsewhere but the allocator calls are assumed to be thread safe. For now I think this can stay but it’s basically true everywhere we use an allocator but I don’t know if we mention it explicitly like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think we're being this explicit elsewhere. To avoid so much repeat though, I wonder if we'd be better off documenting a basic thread-safety target for the library to attain e.g. all functions are reentrant but concurrent use of the same objects isn't safe, and then further qualifying it on a case-by-case basis e.g. the default allocator can be used concurrently, this function is fully thread-safe, this function is not reentrant, etc.

*
* \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
Expand All @@ -183,6 +203,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
Expand All @@ -199,6 +234,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
Expand All @@ -218,6 +264,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
Expand All @@ -234,6 +295,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
Expand All @@ -255,6 +327,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
Expand All @@ -273,6 +360,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
Expand All @@ -294,6 +393,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
Expand All @@ -314,6 +427,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.
Expand All @@ -331,6 +452,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
Expand All @@ -349,6 +481,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
dirk-thomas marked this conversation as resolved.
Show resolved Hide resolved
*
* <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
Expand All @@ -365,6 +512,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
Expand All @@ -382,6 +544,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
Expand All @@ -401,6 +576,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
Expand All @@ -420,11 +610,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.
*/
Expand All @@ -437,11 +644,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.
*/
Expand Down
3 changes: 3 additions & 0 deletions rcl/src/rcl/time.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down