-
Notifications
You must be signed in to change notification settings - Fork 163
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
Clock has multiple time jump callbacks #284
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
f8f243b
Clock has multiple time jump callbacks
sloretz caccea7
Reword comments to make them clearer
sloretz 0e323b9
Fini generic clock after confirming correct clock type
sloretz c897952
Check allocator and remove unnececsary ?:
sloretz 36abcea
Check callbacks not called when clock type is set but not changed?
sloretz 56329f8
Fix allocator check
sloretz 15a350f
test_time no ASAN detections
sloretz 23ea467
More documentation
sloretz c263895
Explicitly say values with the wrong sign are invalid
sloretz 4339044
backwards duration absolute value
sloretz f547688
Backwards duration passed as a positive number
sloretz ff78a21
Positive forward, negative backward
sloretz 800a0e5
spelling
dirk-thomas b9e0bc8
0 -> 0u
sloretz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is following the same enums from
rclcpp
so my comment doesn't have to be considered now. Currently the callback can determine if the clock stayed the same (1 and 4) or from which clock it has changed to whoch new clock (2 and 3).I was just wondering if we ever add a third clock beside
ROS_TIME
andSYSTEM_TIME
the naming of the enums would be "problematic". With a third clock the enum would need to cover all 3x3 cases. Maybe it would be better to just put the type of the clock before and after the jump into thercl_time_jump_t
? If they are equal the clock hasn't changed. And if ROS_TIME is either in the "before" value or in the "after" value (but not both) it was "activated" / "deactivated".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point.
I'm not sure about any of the enums in this struct. It doesn't really describe if a clock's type changed. Instead describes if a ROS clock is getting time from
system_time
or a time source like/clock
. Are you saying we might add a 3rd source of time to a ROS clock in the future?Since jump callbacks are called both before and after the clock update a callback could get that info by querying the clock before and after. How about I remove this struct and add
bool time_source_changed
inrcl_time_jump_t
instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe - sounds reasonable to me.
It would be good if @tfoote could provide some context why this API / approach was chosen in
rclcpp
. Maybe there is something we are missing.