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.
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
Add time source and clock API to nodes #325
Add time source and clock API to nodes #325
Changes from 26 commits
17145d2
5de661a
7676a41
8f2e1b8
978fd2e
eda7103
213978f
1f5ea5c
19a73c3
fde0f35
b808cc4
f11b4b0
90b0938
debaac9
cc59f0a
d7bb9f2
5df379f
0fcd296
524475d
013cce5
a5bdb07
d02ccc5
9c3d987
99401aa
90fe308
0ee8954
4645935
edce20e
632e082
7e536e0
7286ce1
ed583f6
382f780
c79d5f2
e66ff75
7e7c6d9
26d5dd2
3a9b933
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 don't believe we ever need to change the value of the inner
rcl_clock_t
. I would recommend removing theMutex<>
wrapper and see if this code still compiles (we'll also need to get rid of the many.lock().unwrap()
calls). If it does compile then we don't need theMutex<>
, and we'll avoid a lot of unnecessary locking.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 looked at it and I couldn't come up with a way to remove the
Mutex
since the value in_rcl_clock
could be read and written from different threads at the same time. Still, theArc
was unnecessary since anrcl_clock_t
instance is always owned by a single clock so at least I could remove that layer of indirection 19a73c3.Specifically, the
set_ros_time
function would modify the inner_rcl_clock
by setting its internal time, at the same time thenow
function would read the internal clock and return it. I'm a bit afraid of what would happen if they were both called from concurrent threads,rcl
documentation specifies that even though the functions use atomics, concurrent calls on the sameclock
object are not safe.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.
In that case, I think it may be best to leave in the
Mutex
for now, and re-visit this if locking performance becomes an issue. Our hands are a bit tied, since we have to use thercl
internal clock functions to maintain compatibility with the rest of ROS 2... If locking mutexes becomes a bottleneck, and the underlying safety ofrcl
turns out to be the issue preventing us from removing those mutexes, we will need some hard data to prove a change is needed.