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

use uint64_t for timer period. #974

Conversation

fujitatomoya
Copy link
Collaborator

address #967

Signed-off-by: Tomoya Fujita Tomoya.Fujita@sony.com

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

@sloretz can you take a look if you have time?

@fujitatomoya
Copy link
Collaborator Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Contributor

@Blast545 Blast545 left a comment

Choose a reason for hiding this comment

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

Makes sense to me after checking #967

@Blast545
Copy link
Contributor

@ros-pull-request-builder retest this please

@fujitatomoya
Copy link
Collaborator Author

@Blast545 thanks for checking on this, rpr job is unstable but CI all green. i need to merge ros2/rclpy#918 at the same time.

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

I think rcl_timer_callback_t in rcl/timer.h needs to be updated too:

typedef void (* rcl_timer_callback_t)(rcl_timer_t *, int64_t);

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

There are a few places where int64_t is being used to store the period still in timer.c

const int64_t period = rcutils_atomic_load_uint64_t(&timer->impl->period);

There's some math with last_call_time and next_call_time which are int64_t types which may need special treatment to avoid compile warnings about mismatched signed/unsigned types. It might be easier to make period consistently int64_t than to make it uint64_t.

@fujitatomoya
Copy link
Collaborator Author

closing in favor of #974 (review), i believe this is the correct path.

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

Successfully merging this pull request may close these issues.

None yet

3 participants