Skip to content

Implement rcl_clock_time_started#1021

Merged
methylDragon merged 2 commits intoros2:rollingfrom
methylDragon:time-valid
Dec 5, 2022
Merged

Implement rcl_clock_time_started#1021
methylDragon merged 2 commits intoros2:rollingfrom
methylDragon:time-valid

Conversation

@methylDragon
Copy link
Copy Markdown
Contributor

@methylDragon methylDragon commented Dec 5, 2022 •
edited
Loading

This PR reverts #1018 and implements rcl_clock_time_started instead, which checks if an rcl_clock_t has started (i.e., outputs a non-zero time when queried for current time.)

This was from an internal discussion that concluded that checking for time validity on a time point doesn't make sense (std::chrono's time points have no concept of validity, a time point reflecting (0, 0) is valid.) Instead, what matters is if a clock has a time point or not.

@methylDragon methylDragon mentioned this pull request Dec 5, 2022
Merged
@methylDragon methylDragon force-pushed the time-valid branch 2 times, most recently from b3c2184 to d3a766b Compare December 5, 2022 09:57
@methylDragon methylDragon marked this pull request as ready for review December 5, 2022 09:58
@methylDragon
Copy link
Copy Markdown
Contributor Author

methylDragon commented Dec 5, 2022

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

audrow
audrow approved these changes Dec 5, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

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

Looks good to me!

fujitatomoya
fujitatomoya reviewed Dec 5, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

although this change itself looks good to me, i would suggest that the following procedure.

  1. revert #1018 only, and merge this revert commit in rolling.
  2. then apply rcl_clock_time_started.

it is up to package maintainers, but highly recommended. each PR should have complete fix as less dependencies as possible.

@methylDragon methylDragon mentioned this pull request Dec 5, 2022
Merged
@methylDragon
Copy link
Copy Markdown
Contributor Author

methylDragon commented Dec 5, 2022 •
edited
Loading

although this change itself looks good to me, i would suggest that the following procedure.

  1. revert Implement validity checks for time points #1018 only, and merge this revert commit in rolling.
  2. then apply rcl_clock_time_started.

it is up to package maintainers, but highly recommended. each PR should have complete fix as less dependencies as possible.

Fair point! #1022

I've reconfigured this PR @fujitatomoya , the revert change is split off.
I also added a clarificatory comment in the tests: https://github.com/ros2/rcl/pull/1021/files

I will merge on green CI

@methylDragon methylDragon force-pushed the time-valid branch 2 times, most recently from 3aaab01 to 78f1934 Compare December 5, 2022 19:34
@methylDragon
Copy link
Copy Markdown
Contributor Author

methylDragon commented Dec 5, 2022

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

@methylDragon methylDragon changed the title Implement rcl_clock_time_started and revert #1018 Implement rcl_clock_time_started Dec 5, 2022
fujitatomoya
fujitatomoya approved these changes Dec 5, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm with one comment

methylDragon and others added 2 commits December 5, 2022 14:13
@methylDragon
Implement rcl_clock_time_started
dfc1424 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon @fujitatomoya
Fix typo
51a0841 
Signed-off-by: methylDragon <methylDragon@gmail.com>

Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@methylDragon methylDragon merged commit 09d86e2 into ros2:rolling Dec 5, 2022
@methylDragon methylDragon deleted the time-valid branch December 5, 2022 22:22
@methylDragon
Copy link
Copy Markdown
Contributor Author

methylDragon commented Jun 12, 2023

@Mergifyio backport humble

@mergify
Copy link
Copy Markdown

mergify bot commented Jun 12, 2023 •
edited
Loading

backport humble

✅ Backports have been created

Details

mergify bot pushed a commit that referenced this pull request Jun 12, 2023
@methylDragon @fujitatomoya @mergify
Implement rcl_clock_time_started (#1021)
d6f66e1 
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 09d86e2)
@mergify mergify bot mentioned this pull request Jun 12, 2023
Merged
@methylDragon methylDragon mentioned this pull request Jun 12, 2023
Merged
methylDragon added a commit that referenced this pull request Jun 13, 2023
@mergify @methylDragon
Implement rcl_clock_time_started (#1021) (#1078)
e1f19be 
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 09d86e2)

Co-authored-by: methylDragon <methylDragon@gmail.com>
@stan-guer stan-guer mentioned this pull request Jul 28, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@audrow audrow audrow approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

@wjwwood wjwwood Awaiting requested review from wjwwood

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@methylDragon @audrow @fujitatomoya