Implement validity checks for time points#1018
Conversation
|
Signed-off-by: methylDragon <methylDragon@gmail.com>
0a0acaf to
282c5ae
Compare
sloretz
left a comment
There was a problem hiding this comment.
LGTM once linter is happy
| } | ||
|
|
||
| bool | ||
| rcl_time_point_value_valid(rcl_time_point_value_t time_point_value) |
There was a problem hiding this comment.
The code looks correct, but I doubt this function will be used much. I would recommend having just the other one.
There was a problem hiding this comment.
Interestingly enough, rcl_clock_get_now only allows you to get an rcl_time_point_value_t, so I was provisioning this one for that usage path.
Signed-off-by: methylDragon <methylDragon@gmail.com>
|
@ros-pull-request-builder retest this please |
| rcl_time_point_t uninitialized; | ||
| (void) uninitialized; | ||
| // Not reliably detectable due to random values. | ||
| // ASSERT_FALSE(rcl_clock_valid(&uninitialized)); |
There was a problem hiding this comment.
As we talked about in person @methylDragon, we should just not have stuff in the tests like this (I know you're copying from the clock tests, but I consider them to be not ideal as well) and instead we should document that using uninitialized values with any of our functions is undefined behavior, e.g.:
Lines 96 to 99 in e47ed58
Node is just an example, we should ideally document that all structures have to be zero initialized before being used (unless we have specific cases where we want to avoid that for performance reasons).
I don't see the rcl_..._zero_initialized_...() like functions for time/clock objects. We should probably clean them up to have these, again unless there are specific scenarios where we want to avoid that for performance reasons. Like it might be ok to use an uninitialized time point if passing it to get time (which is write only maybe). Avoiding an unnecessary initialization state. But functions that read the struct, like "is valid" should definitely be documented to not work with uninitialized values. In which case we don't need commented blocks like these in the tests.
fujitatomoya
left a comment
There was a problem hiding this comment.
according to the comment #1018 (comment), i think you already have consensus of this change, right. that is okay but it seems that this needs to be fixed.
| bool | ||
| rcl_time_point_value_valid(rcl_time_point_value_t time_point_value) | ||
| { | ||
| return time_point_value > 0; |
There was a problem hiding this comment.
I think this implementation should be in rcutils?
| /// Check if the time point is valid. | ||
| /** | ||
| * This function returns true if the time point appears to be valid. | ||
| * It will check that the type is not uninitialized, and that the time point value is non-zero. |
There was a problem hiding this comment.
it does not seem to check the clock_type is initialized or not...?
There was a problem hiding this comment.
Oh, I meant it as check if the struct pointer (not the clock type) is initialized..
| } | ||
|
|
||
| bool | ||
| rcl_time_point_valid(rcl_time_point_t * time_point) |
There was a problem hiding this comment.
even if rcl_clock_type_t is RCL_CLOCK_UNINITIALIZED, this will return true if time_point->nanoseconds is greater than zero.
There was a problem hiding this comment.
One question I had was whether a time point with non-zero time but associated with an uninitialized clock type should also be uninitialized? I interpreted the docs to mean that all that matters is the time point value.
That's as intended, this is meant to check for time point validity wrt. the docs. The rclcpp clock validity check ends up checking for clock validity in addition to time validity. I think I can raise this in the next ROS meeting regarding this 🤔
| rcl_time_point_t valid, invalid; | ||
|
|
||
| valid.nanoseconds = 1000; | ||
| valid.clock_type = RCL_ROS_TIME; |
There was a problem hiding this comment.
missing test case for RCL_CLOCK_UNINITIALIZED, so that we can detect that implementation is not completed?
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
This reverts commit e47ed58.
This reverts commit e47ed58. Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
Precursor for ros2/rclcpp#2040
Codifies the notion of zero-time being invalid as defined in the design docs, will be used in downstream rclcpp validity checks.
One question I had was whether a time point with non-zero time but associated with an uninitialized clock type should also be uninitialized? I interpreted the docs to mean that all that matters is the time point value.