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 2 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
178 changes: 178 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,15 @@ 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
Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify why the function isn't (same for the others)?

Copy link
Contributor Author

@hidmic hidmic Feb 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I was unsure about this actually, as I have not found an explicit statement of what we mean by thread-safe here. I meant it in the strictest sense i.e. concurrent access to the same object (which is an argument purely because of a language limitation) is safe. If by thread-safe we mean safe concurrent access for different objects or with no regard for objects (i.e. no shared global state), then all these functions here are, including those that add and remove callbacks.

I wonder if Yes and No for values are enough. Maybe a third Conditional value would help us in being a bit more precise.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe @wjwwood can clarify how the Thread-Safe flag is used throughout the existing API.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really think we need a third state for Thread-Safe. This one clearly isn't thread-safe; if you pass the same object into this method from two threads at the same time, that will leave the clock object in an undefined state. So it looks to me like "Thread-Safe: No" is appropriate here. The same goes for all of the APIs below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the same, but it's also true that there're varying degrees of thread (un)safety. You can manage different clocks in different threads without issues. Functions that access global state don't even allow that.

Copy link
Member

Choose a reason for hiding this comment

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

And I think that kind of differentiated information would be very valuable for the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wjwwood bump

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I thought I replied to this, but maybe I was dreaming, I was talking about it with the Apex folks recently too.

The idea is that you can call it from multiple threads concurrently, but also generally that it isn't affected by other functions being called concurrently. If there's any threading considerations like "can be called concurrently but not on the same object" or "can be called concurrently but not concurrently with fini on the same object, etc...".

Basically, I consider the footnote to be more important than the Yes or No. If you say "Yes" without a note, that would mean to me that the function is most likely pure functional and has no global, or input/output, side effects. If it accesses global state or modifies an object passed by non-const pointer, then you should consider having a No or Yes with a note.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, so it could be either so long as there's a note. I'll add one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There, see ca78af8. It's a bit verbose but it is accurate.

* Uses Atomics | No
* Lock-Free | Yes
* <i>[1] if `clock_type` is `RCL_ROS_TIME`</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
Expand All @@ -183,6 +200,17 @@ 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 | Yes
dirk-thomas marked this conversation as resolved.
Show resolved Hide resolved
* Thread-Safe | No
* Uses Atomics | No
* Lock-Free | Yes
*
* \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 +227,14 @@ 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 | No
* Thread-Safe | No
* Uses Atomics | No
* Lock-Free | Yes
*
* \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 +254,17 @@ 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 | Yes
* Thread-Safe | No
* Uses Atomics | No
* Lock-Free | Yes
*
* \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 +281,14 @@ 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
* Uses Atomics | No
* Lock-Free | Yes
*
* \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 +310,18 @@ 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 | Yes
dirk-thomas marked this conversation as resolved.
Show resolved Hide resolved
* Thread-Safe | No
* Uses Atomics | No
* Lock-Free | Yes
*
* <i>[1] if jump callbacks were added</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 +340,14 @@ 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
* Uses Atomics | No
* Lock-Free | Yes
*
* \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 +369,17 @@ 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 | Yes [1]
* Thread-Safe | No
* Uses Atomics | No
* Lock-Free | Yes
* <i>[1] if jump callbacks were added</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 +400,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 +425,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 +454,18 @@ 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
* ------------------ | -------------
* Allocates Memory | No
* Thread-Safe | No
* Uses Atomics | No
* Lock-Free | Yes
dirk-thomas marked this conversation as resolved.
Show resolved Hide resolved
*
* \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 +482,18 @@ 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
* ------------------ | -------------
* Allocates Memory | No
* Thread-Safe | No
* Uses Atomics | No
* Lock-Free | Yes
*
* \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 +511,17 @@ 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
* Uses Atomics | No
* Lock-Free | Yes
*
* \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 +541,18 @@ 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
* ------------------ | -------------
* Allocates Memory | No
* Thread-Safe | No
* Uses Atomics | Yes
* Lock-Free | Yes
*
* \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 +572,24 @@ 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
* Uses Atomics | No
* Lock-Free | Yes
*
* \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 +602,24 @@ 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
* Uses Atomics | No
* Lock-Free | Yes
*
* \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