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.

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

Allow rclcpp::Time::max() to support other clock types than RCL_SYSTEM_TIME. #2348

Closed
jrutgeer opened this issue Nov 1, 2023 · 2 comments
Closed
Assignees
Labels
enhancement New feature or request

Comments

@jrutgeer
Copy link
Contributor

jrutgeer commented Nov 1, 2023

As of yet, rclcpp::Time::max() calls the Time() constructor without specified value for rcl_clock_type_t:

Time
Time::max()
{
return Time(std::numeric_limits<int32_t>::max(), 999999999);
}

Hence, rclcpp::Time::max() returns a Time value with clock type RCL_SYSTEM_TIME:

Time(int32_t seconds, uint32_t nanoseconds, rcl_clock_type_t clock_type = RCL_SYSTEM_TIME);


This means that you can't compare time values of type RCL_ROS_TIME to rclcpp::Time::max().

I propose to change the max() implementation as follows:

Time
Time::max(rcl_clock_type_t clock_type = RCL_SYSTEM_TIME)
{
  return Time(std::numeric_limits<int32_t>::max(), 999999999, clock_type);
}

This is backwards-compatible with the current implementation, yet allows to specify RCL_ROS_TIME if needed, e.g.:

if( reported_time_ < rclcpp::Time::max(RCL_ROS_TIME) {
  [...]
}
@fujitatomoya fujitatomoya self-assigned this Nov 3, 2023
@fujitatomoya fujitatomoya added the enhancement New feature or request label Nov 3, 2023
fujitatomoya added a commit that referenced this issue Nov 3, 2023
  Ref: #2348

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Collaborator

@jrutgeer it would be appreciated if you review #2348

@fujitatomoya
Copy link
Collaborator

@jrutgeer this has been addressed by #2352, i will go ahead to close this. (i gave up backporting this to iron and humble, since this requires ABI change. see #2355 (comment))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants