-
Notifications
You must be signed in to change notification settings - Fork 127
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
Merged
esteve
merged 38 commits into
ros2-rust:main
from
luca-della-vedova:luca/add_time_source
Nov 6, 2023
Merged
Changes from all commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
17145d2
WIP Add minimal TimeSource implementation
luca-della-vedova 5de661a
Minor cleanup and code reorganization
luca-della-vedova 7676a41
Minor cleanups, add debug code
luca-della-vedova 8f2e1b8
Minor cleanup
luca-della-vedova 978fd2e
Change safety note to reflect get_now safety
luca-della-vedova eda7103
Fix comment spelling
luca-della-vedova 213978f
Cleanup and add some docs / tests
luca-della-vedova 1f5ea5c
Avoid duplicated clocks in TimeSource
luca-della-vedova 19a73c3
Change _rcl_clock to just Mutex
luca-della-vedova fde0f35
Remove Mutex from node clock
luca-della-vedova b808cc4
Change Mutex<bool> to AtomicBool
luca-della-vedova f11b4b0
Avoid cloning message
luca-della-vedova 90b0938
Minor cleanup, add dependency
luca-della-vedova debaac9
Remove hardcoded use_sim_time
luca-della-vedova cc59f0a
Cleanup remaining warnings / clippy
luca-della-vedova d7bb9f2
Fix tests
luca-della-vedova 5df379f
Refactor API to use ClockSource
luca-della-vedova 0fcd296
Fix Drop implementation, finish builder and add comments
luca-della-vedova 524475d
Minor cleanups
luca-della-vedova 013cce5
Fix graph_tests
luca-della-vedova a5bdb07
Remove Arc from time source
luca-della-vedova d02ccc5
Remove Sync trait, format
luca-della-vedova 9c3d987
Add comparison function for times, use reference to clock
luca-della-vedova 99401aa
Implement Add<Duration> and Sub<Duration> for Time
luca-della-vedova 90fe308
Make node pointer Weak to avoid memory leaks
luca-della-vedova 0ee8954
Cleanups
luca-della-vedova 4645935
WIP change clock type when use_sim_time parameter is changed
luca-della-vedova edce20e
Remove need for mutex in node TimeSource
luca-della-vedova 632e082
Change get_clock() to return cloned Clock object
luca-della-vedova 7e536e0
Minor cleanup
luca-della-vedova 7286ce1
Make get_parameter pub(crate)
luca-della-vedova ed583f6
Address review feedback
luca-della-vedova 382f780
Make time_source pub(crate), remove unused APIs
luca-della-vedova c79d5f2
Merge remote-tracking branch 'origin/main' into luca/add_time_source
luca-della-vedova e66ff75
Use MandatoryParameter for use_sim_time
luca-della-vedova 7e7c6d9
Remove error variant from clock builder
luca-della-vedova 26d5dd2
Add integration test for use_sim_time = true
luca-della-vedova 3a9b933
Minor cleanup for attach_node
luca-della-vedova 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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,223 @@ | ||
use crate::rcl_bindings::*; | ||
use crate::{error::ToResult, time::Time, to_rclrs_result}; | ||
use std::sync::{Arc, Mutex}; | ||
|
||
/// Enum to describe clock type. Redefined for readability and to eliminate the uninitialized case | ||
/// from the `rcl_clock_type_t` enum in the binding. | ||
#[derive(Clone, Debug, Copy)] | ||
pub enum ClockType { | ||
/// Time with behavior dependent on the `set_ros_time(bool)` function. If called with `true` | ||
/// it will be driven by a manual value override, otherwise it will be System Time | ||
RosTime = 1, | ||
/// Wall time depending on the current system | ||
SystemTime = 2, | ||
/// Steady time, monotonically increasing but not necessarily equal to wall time. | ||
SteadyTime = 3, | ||
} | ||
|
||
impl From<ClockType> for rcl_clock_type_t { | ||
fn from(clock_type: ClockType) -> Self { | ||
match clock_type { | ||
ClockType::RosTime => rcl_clock_type_t::RCL_ROS_TIME, | ||
ClockType::SystemTime => rcl_clock_type_t::RCL_SYSTEM_TIME, | ||
ClockType::SteadyTime => rcl_clock_type_t::RCL_STEADY_TIME, | ||
} | ||
} | ||
} | ||
|
||
/// Struct that implements a Clock and wraps `rcl_clock_t`. | ||
#[derive(Clone, Debug)] | ||
pub struct Clock { | ||
_type: ClockType, | ||
_rcl_clock: Arc<Mutex<rcl_clock_t>>, | ||
// TODO(luca) Implement jump callbacks | ||
} | ||
|
||
/// A clock source that can be used to drive the contained clock. Created when a clock of type | ||
/// `ClockType::RosTime` is constructed | ||
pub struct ClockSource { | ||
_rcl_clock: Arc<Mutex<rcl_clock_t>>, | ||
} | ||
|
||
impl Clock { | ||
/// Creates a new Clock with `ClockType::SystemTime` | ||
pub fn system() -> Self { | ||
Self::make(ClockType::SystemTime) | ||
} | ||
|
||
/// Creates a new Clock with `ClockType::SteadyTime` | ||
pub fn steady() -> Self { | ||
Self::make(ClockType::SteadyTime) | ||
} | ||
|
||
/// Creates a new Clock with `ClockType::RosTime` and a matching `ClockSource` that can be used | ||
/// to update it | ||
pub fn with_source() -> (Self, ClockSource) { | ||
let clock = Self::make(ClockType::RosTime); | ||
let clock_source = ClockSource::new(clock._rcl_clock.clone()); | ||
(clock, clock_source) | ||
} | ||
|
||
/// Creates a new clock of the given `ClockType`. | ||
pub fn new(type_: ClockType) -> (Self, Option<ClockSource>) { | ||
let clock = Self::make(type_); | ||
let clock_source = | ||
matches!(type_, ClockType::RosTime).then(|| ClockSource::new(clock._rcl_clock.clone())); | ||
(clock, clock_source) | ||
} | ||
|
||
fn make(type_: ClockType) -> Self { | ||
let mut rcl_clock; | ||
unsafe { | ||
// SAFETY: Getting a default value is always safe. | ||
rcl_clock = Self::init_generic_clock(); | ||
let mut allocator = rcutils_get_default_allocator(); | ||
// Function will return Err(_) only if there isn't enough memory to allocate a clock | ||
// object. | ||
rcl_clock_init(type_.into(), &mut rcl_clock, &mut allocator) | ||
.ok() | ||
.unwrap(); | ||
} | ||
Self { | ||
_type: type_, | ||
_rcl_clock: Arc::new(Mutex::new(rcl_clock)), | ||
} | ||
} | ||
|
||
/// Returns the clock's `ClockType`. | ||
pub fn clock_type(&self) -> ClockType { | ||
self._type | ||
} | ||
|
||
/// Returns the current clock's timestamp. | ||
pub fn now(&self) -> Time { | ||
let mut clock = self._rcl_clock.lock().unwrap(); | ||
let mut time_point: i64 = 0; | ||
unsafe { | ||
// SAFETY: No preconditions for this function | ||
rcl_clock_get_now(&mut *clock, &mut time_point); | ||
} | ||
Time { | ||
nsec: time_point, | ||
clock: Arc::downgrade(&self._rcl_clock), | ||
} | ||
} | ||
|
||
/// Helper function to privately initialize a default clock, with the same behavior as | ||
/// `rcl_init_generic_clock`. By defining a private function instead of implementing | ||
/// `Default`, we avoid exposing a public API to create an invalid clock. | ||
// SAFETY: Getting a default value is always safe. | ||
unsafe fn init_generic_clock() -> rcl_clock_t { | ||
jhdcs marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let allocator = rcutils_get_default_allocator(); | ||
rcl_clock_t { | ||
type_: rcl_clock_type_t::RCL_CLOCK_UNINITIALIZED, | ||
jump_callbacks: std::ptr::null_mut::<rcl_jump_callback_info_t>(), | ||
num_jump_callbacks: 0, | ||
get_now: None, | ||
data: std::ptr::null_mut::<std::os::raw::c_void>(), | ||
allocator, | ||
} | ||
} | ||
} | ||
|
||
impl Drop for ClockSource { | ||
fn drop(&mut self) { | ||
self.set_ros_time_enable(false); | ||
} | ||
} | ||
|
||
impl PartialEq for ClockSource { | ||
fn eq(&self, other: &Self) -> bool { | ||
Arc::ptr_eq(&self._rcl_clock, &other._rcl_clock) | ||
} | ||
} | ||
|
||
impl ClockSource { | ||
/// Sets the value of the current ROS time. | ||
pub fn set_ros_time_override(&self, nanoseconds: i64) { | ||
let mut clock = self._rcl_clock.lock().unwrap(); | ||
// SAFETY: Safe if clock jump callbacks are not edited, which is guaranteed | ||
// by the mutex | ||
unsafe { | ||
// Function will only fail if timer was uninitialized or not RosTime, which should | ||
// not happen | ||
rcl_set_ros_time_override(&mut *clock, nanoseconds) | ||
.ok() | ||
.unwrap(); | ||
} | ||
} | ||
|
||
fn new(clock: Arc<Mutex<rcl_clock_t>>) -> Self { | ||
let source = Self { _rcl_clock: clock }; | ||
source.set_ros_time_enable(true); | ||
source | ||
} | ||
|
||
/// Sets the clock to use ROS Time, if enabled the clock will report the last value set through | ||
/// `Clock::set_ros_time_override(nanoseconds: i64)`. | ||
fn set_ros_time_enable(&self, enable: bool) { | ||
let mut clock = self._rcl_clock.lock().unwrap(); | ||
if enable { | ||
// SAFETY: Safe if clock jump callbacks are not edited, which is guaranteed | ||
// by the mutex | ||
unsafe { | ||
// Function will only fail if timer was uninitialized or not RosTime, which should | ||
// not happen | ||
rcl_enable_ros_time_override(&mut *clock).ok().unwrap(); | ||
} | ||
} else { | ||
// SAFETY: Safe if clock jump callbacks are not edited, which is guaranteed | ||
// by the mutex | ||
unsafe { | ||
// Function will only fail if timer was uninitialized or not RosTime, which should | ||
// not happen | ||
rcl_disable_ros_time_override(&mut *clock).ok().unwrap(); | ||
} | ||
} | ||
} | ||
} | ||
|
||
impl Drop for rcl_clock_t { | ||
fn drop(&mut self) { | ||
// SAFETY: No preconditions for this function | ||
let rc = unsafe { rcl_clock_fini(&mut *self) }; | ||
if let Err(e) = to_rclrs_result(rc) { | ||
panic!("Unable to release Clock. {:?}", e) | ||
} | ||
} | ||
} | ||
|
||
// SAFETY: The functions accessing this type, including drop(), shouldn't care about the thread | ||
// they are running in. Therefore, this type can be safely sent to another thread. | ||
unsafe impl Send for rcl_clock_t {} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
|
||
fn assert_send<T: Send>() {} | ||
fn assert_sync<T: Sync>() {} | ||
|
||
#[test] | ||
fn clock_is_send_and_sync() { | ||
assert_send::<Clock>(); | ||
assert_sync::<Clock>(); | ||
} | ||
|
||
#[test] | ||
fn clock_system_time_now() { | ||
let clock = Clock::system(); | ||
assert!(clock.now().nsec > 0); | ||
} | ||
|
||
#[test] | ||
fn clock_ros_time_with_override() { | ||
let (clock, source) = Clock::with_source(); | ||
// No manual time set, it should default to 0 | ||
assert!(clock.now().nsec == 0); | ||
let set_time = 1234i64; | ||
source.set_ros_time_override(set_time); | ||
// Ros time is set, should return the value that was set | ||
assert_eq!(clock.now().nsec, set_time); | ||
} | ||
} |
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.
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 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.