From 239dab35d1bb8fa76c084d3d6765f1d2b1e473f4 Mon Sep 17 00:00:00 2001 From: geoff-imdex Date: Fri, 11 Oct 2024 09:12:50 +0800 Subject: [PATCH 1/4] Initial logging implementation --- rclrs/src/context.rs | 25 +++ rclrs/src/lib.rs | 2 + rclrs/src/logging.rs | 334 ++++++++++++++++++++++++++++++++++++++++ rclrs/src/node.rs | 28 +++- rclrs/src/rcl_wrapper.h | 1 + 5 files changed, 389 insertions(+), 1 deletion(-) create mode 100644 rclrs/src/logging.rs diff --git a/rclrs/src/context.rs b/rclrs/src/context.rs index 90c8fbd3c..e2a1bc10d 100644 --- a/rclrs/src/context.rs +++ b/rclrs/src/context.rs @@ -36,6 +36,9 @@ impl Drop for rcl_context_t { rcl_shutdown(self); rcl_context_fini(self); } + + // SAFETY: No preconditions for rcl_logging_fini + rcl_logging_fini(); } } } @@ -56,6 +59,10 @@ unsafe impl Send for rcl_context_t {} /// - middleware-specific data, e.g. the domain participant in DDS /// - the allocator used (left as the default by `rclrs`) /// +/// The context also configures the rcl_logging_* layer to allow publication to /rosout +/// (as well as the terminal). TODO: This behaviour should be configurable using an +/// "auto logging initialise" flag as per rclcpp and rclpy. +/// pub struct Context { pub(crate) handle: Arc, } @@ -142,6 +149,24 @@ impl Context { rcl_init_options_fini(&mut rcl_init_options).ok()?; // Move the check after the last fini() ret?; + + // TODO: "Auto set-up logging" is forced but should be configurable as per rclcpp and rclpy + // SAFETY: + // * Lock the mutex as we cannot guarantee that rcl_* functions are protecting their global variables + // * Context is validated before we reach this point + // * No other preconditions for calling this function + let ret = { + let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); + // Note: shadowing the allocator above. The rcutils_get_default_allocator provides us with mechanisms for allocating and freeing + // memory, e.g. calloc/free. As these are function pointers and will be the same each time we call the method, it is safe to + // perform this shadowing + // Alternate is to call rcl_init_options_get_allocator however, we've freed the allocator above and restructuring the call + // sequence is unnecessarily complex + let allocator = rcutils_get_default_allocator(); + + rcl_logging_configure(&rcl_context.global_arguments, &allocator).ok() + }; + ret? } Ok(Self { handle: Arc::new(ContextHandle { diff --git a/rclrs/src/lib.rs b/rclrs/src/lib.rs index 4924b36cb..3a22c6da8 100644 --- a/rclrs/src/lib.rs +++ b/rclrs/src/lib.rs @@ -11,6 +11,7 @@ mod clock; mod context; mod error; mod executor; +mod logging; mod node; mod parameter; mod publisher; @@ -38,6 +39,7 @@ pub use clock::*; pub use context::*; pub use error::*; pub use executor::*; +pub use logging::*; pub use node::*; pub use parameter::*; pub use publisher::*; diff --git a/rclrs/src/logging.rs b/rclrs/src/logging.rs new file mode 100644 index 000000000..e3efd4a4a --- /dev/null +++ b/rclrs/src/logging.rs @@ -0,0 +1,334 @@ +// Copyright (c) 2019 Sequence Planner +// SPDX-License-Identifier: Apache-2.0 AND MIT +// Adapted from https://github.com/sequenceplanner/r2r/blob/89cec03d07a1496a225751159cbc7bfb529d9dd1/r2r/src/utils.rs +// Further adapted from https://github.com/mvukov/rules_ros2/pull/371 + +use std::ffi::CString; +use std::sync::Mutex; +use std::time::Duration; + +use crate::rcl_bindings::*; + +// Used to protect calls to rcl/rcutils in case those functions manipulate global variables +static LOG_GUARD: Mutex<()> = Mutex::new(()); + +/// Calls the underlying rclutils logging function +/// Don't call this directly, use the logging macros instead. +/// +/// # Panics +/// +/// This function might panic in the following scenarios: +/// - A logger_name is provided that is not a valid c-string, e.g. contains extraneous null characters +/// - The user-supplied "msg" is not a valid c-string, e.g. contains extraneous null characters +/// - When called if the lock is already held by the current thread. +/// - If the construction of CString objects used to create the log output fail, +/// although, this highly unlikely to occur in most cases +#[doc(hidden)] +pub fn log(msg: &str, logger_name: &str, file: &str, line: u32, severity: LogSeverity) { + // currently not possible to get function name in rust. + // see https://github.com/rust-lang/rfcs/pull/2818 + let function = CString::new("").unwrap(); + let file = CString::new(file).unwrap(); + let location = rcutils_log_location_t { + function_name: function.as_ptr(), + file_name: file.as_ptr(), + line_number: line as usize, + }; + let format = CString::new("%s").unwrap(); + let logger_name = CString::new(logger_name) + .expect("Logger name is a valid c style string, e.g. check for extraneous null characters"); + let message = CString::new(msg) + .expect("Valid c style string required, e.g. check for extraneous null characters"); + let severity = severity.to_native(); + + let _guard = LOG_GUARD.lock().unwrap(); + // SAFETY: Global variables are protected via LOG_GUARD, no other preconditions are required + unsafe { + rcutils_log( + &location, + severity as i32, + logger_name.as_ptr(), + format.as_ptr(), + message.as_ptr(), + ); + } +} + +/// Logging severity +#[doc(hidden)] +pub enum LogSeverity { + Unset, + Debug, + Info, + Warn, + Error, + Fatal, +} + +impl LogSeverity { + fn to_native(&self) -> RCUTILS_LOG_SEVERITY { + use crate::rcl_bindings::rcl_log_severity_t::*; + match self { + LogSeverity::Unset => RCUTILS_LOG_SEVERITY_UNSET, + LogSeverity::Debug => RCUTILS_LOG_SEVERITY_DEBUG, + LogSeverity::Info => RCUTILS_LOG_SEVERITY_INFO, + LogSeverity::Warn => RCUTILS_LOG_SEVERITY_WARN, + LogSeverity::Error => RCUTILS_LOG_SEVERITY_ERROR, + LogSeverity::Fatal => RCUTILS_LOG_SEVERITY_FATAL, + } + } +} + +#[derive(Debug)] +/// Specify when a log message should be published +pub enum LoggingOccurrence { + /// The log message will always be published (assuming all other conditions are met) + Always, + /// The message will only be published on the first occurrence (Note: no other conditions apply) + Once, + /// The log message will not be published on the first occurrence, but will be published on + /// each subsequent occurrence (assuming all other conditions are met) + SkipFirst, +} + +/// Specify conditions that must be met for a log message to be published +/// +/// The struct provides the following convenience functions to construct conditions that match +/// behaviour available in the `rclcpp` and `rclpy` libraries. +/// +/// When will my log message be output? +/// +/// - `Once`: A message with the [`LoggingOccurrence::Once`] value will be published once only +/// regardless of any other conditions +/// - `SkipFirst`: A message with the [`LoggingOccurrence::SkipFirst`] value will never be published +/// on the first encounter regardless of any other conditions. After the first +/// encounter, the behaviour is identical to the [`LoggingOccurrence::Always`] setting. +/// - `Always`: The log message will be output if all additional conditions are true: +/// - The current time + the `publish_interval` > the last time the message was published. +/// - The default value for `publish_interval` is 0, i.e. the interval check will always pass +/// - The `log_if_true` expression evaluates to TRUE. +/// - The default value for the `log_if_true` field is TRUE. +pub struct LogConditions { + /// Specify when a log message should be published (See[`LoggingOccurrence`] above) + pub occurs: LoggingOccurrence, + /// Specify the publication interval of the message. A value of ZERO (0) indicates that the + /// message should be published every time, otherwise, the message will only be published once + /// the specified interval has elapsed. + /// This field is typically used to limit the output from high-frequency messages, e.g. instead + /// of publishing a log message every 10 milliseconds, the `publish_interval` can be configured + /// such that the message is published every 10 seconds. + pub publish_interval: Duration, + /// The log message will only published if the specified expression evaluates to true + pub log_if_true: bool, +} + +impl LogConditions { + /// Default construct an instance + pub fn new() -> Self { + Self { + occurs: LoggingOccurrence::Always, + publish_interval: Duration::ZERO, + log_if_true: true, + } + } + + /// Only publish this message the first time it is encountered + pub fn once() -> Self { + Self { + occurs: LoggingOccurrence::Once, + publish_interval: Duration::ZERO, + log_if_true: true, + } + } + + /// Do not publish the message the first time it is encountered + pub fn skip_first() -> Self { + Self { + occurs: LoggingOccurrence::SkipFirst, + publish_interval: Duration::ZERO, + log_if_true: true, + } + } + + /// Do not publish the first time this message is encountered and publish + /// at the specified `publish_interval` thereafter + pub fn skip_first_throttle(publish_interval: Duration) -> Self { + Self { + occurs: LoggingOccurrence::SkipFirst, + publish_interval, + log_if_true: true, + } + } + + /// Throttle the message to the supplied publish_interval + /// e.g. set `publish_interval` to 1000ms to limit publishing to once a second + pub fn throttle(publish_interval: Duration) -> Self { + Self { + occurs: LoggingOccurrence::Always, + publish_interval, + log_if_true: true, + } + } + + /// Permitting logging if the supplied expression evaluates to true + /// Uses default LoggingOccurrence (Always) and publish_interval (no throttling) + pub fn log_if_true(log_if_true: bool) -> Self { + Self { + occurs: LoggingOccurrence::Always, + publish_interval: Duration::ZERO, + log_if_true, + } + } +} + +/// log_with_conditions +/// Helper macro to log a message using the ROS2 RCUTILS library +/// +/// The macro supports two general calling formats: +/// 1. Plain message string e.g. as per println! macro +/// 2. With calling conditions that provide some restrictions on the output of the message +/// (see [`LogConditions`] above) +/// +/// It is expected that, typically, the macro will be called using one of the wrapper macros, +/// e.g. log_debug!, etc, however, it is relatively straight forward to call the macro directly +/// if you really want to. +/// +/// # Examples +/// +/// ``` +/// log_debug!(&node.name(), "Simple message"); +/// log_debug!(&node.name(), "Simple message {some_variable}"); +/// log_fatal!(&node.name(), "Simple message from {}", node.name()); +/// log_warn!(&node.name(), LogConditions::once(), "Only log this the first time"); +/// log_error!(&node.name(), +/// LogConditions::skip_first_throttle(Duration::from_millis(1000)), +/// "Noisy error that we expect the first time"); +/// +/// log_info!(&node.name(), LogConditions { occurs: LoggingOccurrence::Always, +/// publish_interval: Duration::from_millis(1000), +/// log_if_true: count % 10 == 0, }, +/// "Manually constructed LogConditions"); +/// ``` +/// +/// # Panics +/// +/// It is theoretically possible for the call to panic if the Mutex used for the throttling is +/// poisoned although this should not be possible. +#[macro_export] +macro_rules! log_with_conditions { + // The variable args captured by the $(, $($args:tt)*)?)) code allows us to omit (or include) + // parameters in the simple message case, e.g. to write + // ``` + // log_error!(, "Log with no params"); // OR + // log_error!(, "Log with useful info {}", error_reason); + ($severity: expr, $logger_name: expr, $msg_start: literal $(, $($args:tt)*)?) => { + $crate::log(&std::fmt::format(format_args!($msg_start, $($($args)*)?)), $logger_name, file!(), line!(), $severity); + }; + ($severity: expr, $logger_name: expr, $conditions: expr, $($args:tt)*) => { + // Adding these use statements here due an issue like this one: + // https://github.com/intellij-rust/intellij-rust/issues/9853 + // Note: that issue appears to be specific to jetbrains intellisense however, + // observed same/similar behaviour with rust-analyzer/rustc + use std::sync::Once; + use std::time::SystemTime; + + let log_conditions: $crate::LogConditions = $conditions; + let mut allow_logging = true; + match log_conditions.occurs { + // Create the static variables here so we get a per-instance static + $crate::LoggingOccurrence::Once => { + static LOG_ONCE: std::sync::Once = std::sync::Once::new(); + LOG_ONCE.call_once(|| { + $crate::log(&std::fmt::format(format_args!($($args)*)), $logger_name, file!(), line!(), $severity); + }); + allow_logging = false; + } + $crate::LoggingOccurrence::SkipFirst => { + // Noop, just make sure we exit the first time... + static SKIP_FIRST: std::sync::Once = std::sync::Once::new(); + SKIP_FIRST.call_once(|| { + // Only disable logging the first time + allow_logging = false; + }); + + } + // Drop out + $crate::LoggingOccurrence::Always => (), + } + + // If we have a throttle period AND logging has not already been disabled due to SkipFirst settings + if log_conditions.publish_interval > std::time::Duration::ZERO && allow_logging { + let mut ignore_first_timeout = false; + // Need to initialise to a constant + static LAST_LOG_TIME: Mutex = Mutex::new(std::time::SystemTime::UNIX_EPOCH); + + static INIT_LAST_LOG_TIME: std::sync::Once = std::sync::Once::new(); + // Set the last log time to "now", but let us log the message the first time we hit this + // code, i.e. initial behaviour is expired. + // Note: If this is part of a SkipFirst macro call, we will only hit this code on the second iteration. + INIT_LAST_LOG_TIME.call_once(|| { + let mut last_log_time = LAST_LOG_TIME.lock().unwrap(); + *last_log_time = std::time::SystemTime::now(); + ignore_first_timeout = true; + }); + + let mut last_log_time = LAST_LOG_TIME.lock().unwrap(); + if std::time::SystemTime::now() >= *last_log_time + log_conditions.publish_interval { + // Update our time stamp + *last_log_time = std::time::SystemTime::now(); + } + else if !ignore_first_timeout { + // Timer has not expired (and this is not the first time through here) + allow_logging = false; + } + } + + // At this point we've validated the skip/throttle operations, and we now check that any + // expression supplied also evaluates to true, e.g. if timer has expired but expression is + // false, we won't print + if (allow_logging && log_conditions.log_if_true) + { + $crate::log(&std::fmt::format(format_args!($($args)*)), $logger_name, file!(), line!(), $severity); + } + }; +} + +/// Debug log message. +#[macro_export] +macro_rules! log_debug { + ($logger_name: expr, $($args:tt)*) => {{ + $crate::log_with_conditions!($crate::LogSeverity::Debug, $logger_name, $($args)*); + }} +} + +/// Info log message. +#[macro_export] +macro_rules! log_info { + ($logger_name: expr, $($args:tt)*) => {{ + $crate::log_with_conditions!($crate::LogSeverity::Info, $logger_name, $($args)*); + }} +} + +/// Warning log message. +#[macro_export] +macro_rules! log_warn { + ($logger_name: expr, $($args:tt)*) => {{ + $crate::log_with_conditions!($crate::LogSeverity::Warn, $logger_name, $($args)*); + }} +} + +/// Error log message. +#[macro_export] +macro_rules! log_error { + ($logger_name: expr, $($args:tt)*) => {{ + $crate::log_with_conditions!($crate::LogSeverity::Error, $logger_name, $($args)*); + }} +} + +/// Fatal log message. +#[macro_export] +macro_rules! log_fatal { + ($logger_name: expr, $($args:tt)*) => {{ + $crate::log_with_conditions!($crate::LogSeverity::Fatal, $logger_name, $($args)*); + }} +} diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs index 97684d6bc..ae19a5e4f 100644 --- a/rclrs/src/node.rs +++ b/rclrs/src/node.rs @@ -44,7 +44,7 @@ unsafe impl Send for rcl_node_t {} /// It's a good idea for node names in the same executable to be unique. /// /// ## Remapping -/// The namespace and name given when creating the node can be overriden through the command line. +/// The namespace and name given when creating the node can be overridden through the command line. /// In that sense, the parameters to the node creation functions are only the _default_ namespace and /// name. /// See also the [official tutorial][1] on the command line arguments for ROS nodes, and the @@ -440,6 +440,21 @@ impl Node { pub fn builder(context: &Context, node_name: &str) -> NodeBuilder { NodeBuilder::new(context, node_name) } + + /// Returns the logger name of the node. + pub fn logger_name(&self) -> &str { + let rcl_node = self.handle.rcl_node.lock().unwrap(); + // SAFETY: No pre-conditions for this function + let name_raw_ptr = unsafe { rcl_node_get_logger_name(&*rcl_node) }; + if name_raw_ptr.is_null() { + return ""; + } + // SAFETY: The returned CStr is immediately converted to a string slice, + // so the lifetime is no issue. The ptr is valid as per the documentation + // of rcl_node_get_logger_name. + let name_cstr = unsafe { CStr::from_ptr(name_raw_ptr) }; + name_cstr.to_str().unwrap_or("") + } } // Helper used to implement call_string_getter(), but also used to get the FQN in the Node::new() @@ -514,4 +529,15 @@ mod tests { Ok(()) } + + #[test] + fn test_logger_name() -> Result<(), RclrsError> { + // Use helper to create 2 nodes for us + let graph = construct_test_graph("test_topics_graph")?; + + assert_eq!(graph.node1.logger_name(), "graph_test_node_1"); + assert_eq!(graph.node2.logger_name(), "graph_test_node_2"); + + Ok(()) + } } diff --git a/rclrs/src/rcl_wrapper.h b/rclrs/src/rcl_wrapper.h index fe97cb4e5..ececf491f 100644 --- a/rclrs/src/rcl_wrapper.h +++ b/rclrs/src/rcl_wrapper.h @@ -2,6 +2,7 @@ #include #include #include +#include #include #include #include From 6ad25da679891bb8c3f702868c0bdd6b2a6e3fd1 Mon Sep 17 00:00:00 2001 From: Geoff Talbot Date: Mon, 21 Oct 2024 01:32:07 +0000 Subject: [PATCH 2/4] * Moved logging_fini to follow correct call sequence based on rclcpp --- rclrs/src/context.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/rclrs/src/context.rs b/rclrs/src/context.rs index e2a1bc10d..e9b7ad9d5 100644 --- a/rclrs/src/context.rs +++ b/rclrs/src/context.rs @@ -34,11 +34,12 @@ impl Drop for rcl_context_t { // SAFETY: The entity lifecycle mutex is locked to protect against the risk of // global variables in the rmw implementation being unsafely modified during cleanup. rcl_shutdown(self); + + // SAFETY: No preconditions for rcl_logging_fini + rcl_logging_fini(); + rcl_context_fini(self); } - - // SAFETY: No preconditions for rcl_logging_fini - rcl_logging_fini(); } } } From d5a6bc4fb62cd153b7b9017abd19853c11282b4a Mon Sep 17 00:00:00 2001 From: geoff-imdex Date: Wed, 23 Oct 2024 12:19:59 +0800 Subject: [PATCH 3/4] Primary change to fix builder's node variable going out of scope --- rclrs/src/client.rs | 6 +++--- rclrs/src/logging.rs | 10 ++++++++++ rclrs/src/node.rs | 24 ++++++++++++++++++------ rclrs/src/node/builder.rs | 4 ++-- rclrs/src/node/graph.rs | 14 +++++++------- rclrs/src/publisher.rs | 4 ++-- rclrs/src/service.rs | 4 ++-- rclrs/src/subscription.rs | 4 ++-- 8 files changed, 46 insertions(+), 24 deletions(-) diff --git a/rclrs/src/client.rs b/rclrs/src/client.rs index b308f1de2..f0f8f05e5 100644 --- a/rclrs/src/client.rs +++ b/rclrs/src/client.rs @@ -43,7 +43,7 @@ impl Drop for ClientHandle { // SAFETY: The entity lifecycle mutex is locked to protect against the risk of // global variables in the rmw implementation being unsafely modified during cleanup. unsafe { - rcl_client_fini(rcl_client, &mut *rcl_node); + rcl_client_fini(rcl_client, &mut **rcl_node); } } } @@ -115,7 +115,7 @@ where unsafe { rcl_client_init( &mut rcl_client, - &*rcl_node, + &**rcl_node, type_support, topic_c_string.as_ptr(), &client_options, @@ -263,7 +263,7 @@ where pub fn service_is_ready(&self) -> Result { let mut is_ready = false; let client = &mut *self.handle.rcl_client.lock().unwrap(); - let node = &mut *self.handle.node_handle.rcl_node.lock().unwrap(); + let node = &mut **self.handle.node_handle.rcl_node.lock().unwrap(); unsafe { // SAFETY both node and client are guaranteed to be valid here diff --git a/rclrs/src/logging.rs b/rclrs/src/logging.rs index e3efd4a4a..e50a2290d 100644 --- a/rclrs/src/logging.rs +++ b/rclrs/src/logging.rs @@ -196,7 +196,16 @@ impl LogConditions { /// # Examples /// /// ``` +/// use rclrs::{log_debug, log_error, log_fatal, log_info, log_warn, LogConditions, LoggingOccurrence}; +/// use std::sync::Mutex; +/// use std::time::Duration; +/// use std::env; +/// +/// let context = rclrs::Context::new(env::args()).unwrap(); +/// let node = rclrs::Node::new(&context, "test_node").unwrap(); +/// /// log_debug!(&node.name(), "Simple message"); +/// let some_variable = 43; /// log_debug!(&node.name(), "Simple message {some_variable}"); /// log_fatal!(&node.name(), "Simple message from {}", node.name()); /// log_warn!(&node.name(), LogConditions::once(), "Only log this the first time"); @@ -204,6 +213,7 @@ impl LogConditions { /// LogConditions::skip_first_throttle(Duration::from_millis(1000)), /// "Noisy error that we expect the first time"); /// +/// let count = 0; /// log_info!(&node.name(), LogConditions { occurs: LoggingOccurrence::Always, /// publish_interval: Duration::from_millis(1000), /// log_if_true: count % 10 == 0, }, diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs index ae19a5e4f..0d141721c 100644 --- a/rclrs/src/node.rs +++ b/rclrs/src/node.rs @@ -71,10 +71,15 @@ pub struct Node { /// This struct manages the lifetime of an `rcl_node_t`, and accounts for its /// dependency on the lifetime of its `rcl_context_t` by ensuring that this /// dependency is [dropped after][1] the `rcl_node_t`. +/// Note: we capture the rcl_node_t returned from rcl_get_zero_initialized_node() +/// to guarantee that the node handle exists until we drop the NodeHandle +/// instance. This addresses an issue where previously the address of the variable +/// in the builder.rs was being used, and whose lifespan was (just) shorter than the +/// NodeHandle instance. /// /// [1]: pub(crate) struct NodeHandle { - pub(crate) rcl_node: Mutex, + pub(crate) rcl_node: Mutex>, pub(crate) context_handle: Arc, } @@ -83,9 +88,10 @@ impl Drop for NodeHandle { let _context_lock = self.context_handle.rcl_context.lock().unwrap(); let mut rcl_node = self.rcl_node.lock().unwrap(); let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); + // SAFETY: The entity lifecycle mutex is locked to protect against the risk of // global variables in the rmw implementation being unsafely modified during cleanup. - unsafe { rcl_node_fini(&mut *rcl_node) }; + unsafe { rcl_node_fini(&mut **rcl_node) }; } } @@ -370,7 +376,7 @@ impl Node { let mut domain_id: usize = 0; let ret = unsafe { // SAFETY: No preconditions for this function. - rcl_node_get_domain_id(&*rcl_node, &mut domain_id) + rcl_node_get_domain_id(&**rcl_node, &mut domain_id) }; debug_assert_eq!(ret, 0); @@ -445,7 +451,7 @@ impl Node { pub fn logger_name(&self) -> &str { let rcl_node = self.handle.rcl_node.lock().unwrap(); // SAFETY: No pre-conditions for this function - let name_raw_ptr = unsafe { rcl_node_get_logger_name(&*rcl_node) }; + let name_raw_ptr = unsafe { rcl_node_get_logger_name(&**rcl_node) }; if name_raw_ptr.is_null() { return ""; } @@ -535,8 +541,14 @@ mod tests { // Use helper to create 2 nodes for us let graph = construct_test_graph("test_topics_graph")?; - assert_eq!(graph.node1.logger_name(), "graph_test_node_1"); - assert_eq!(graph.node2.logger_name(), "graph_test_node_2"); + assert_eq!( + graph.node1.logger_name(), + "test_topics_graph.graph_test_node_1" + ); + assert_eq!( + graph.node2.logger_name(), + "test_topics_graph.graph_test_node_2" + ); Ok(()) } diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs index 011d5d2f3..14d4dae27 100644 --- a/rclrs/src/node/builder.rs +++ b/rclrs/src/node/builder.rs @@ -277,7 +277,7 @@ impl NodeBuilder { let rcl_context = &mut *self.context.rcl_context.lock().unwrap(); // SAFETY: Getting a zero-initialized value is always safe. - let mut rcl_node = unsafe { rcl_get_zero_initialized_node() }; + let mut rcl_node = Box::new(unsafe { rcl_get_zero_initialized_node() }); unsafe { // SAFETY: // * The rcl_node is zero-initialized as mandated by this function. @@ -287,7 +287,7 @@ impl NodeBuilder { // global variables in the rmw implementation being unsafely modified during cleanup. let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); rcl_node_init( - &mut rcl_node, + &mut *rcl_node, node_name.as_ptr(), node_namespace.as_ptr(), rcl_context, diff --git a/rclrs/src/node/graph.rs b/rclrs/src/node/graph.rs index 343fa61d3..e18675a21 100644 --- a/rclrs/src/node/graph.rs +++ b/rclrs/src/node/graph.rs @@ -139,7 +139,7 @@ impl Node { unsafe { let rcl_node = self.handle.rcl_node.lock().unwrap(); rcl_get_topic_names_and_types( - &*rcl_node, + &**rcl_node, &mut rcutils_get_default_allocator(), false, &mut rcl_names_and_types, @@ -169,7 +169,7 @@ impl Node { unsafe { let rcl_node = self.handle.rcl_node.lock().unwrap(); rcl_get_node_names( - &*rcl_node, + &**rcl_node, rcutils_get_default_allocator(), &mut rcl_names, &mut rcl_namespaces, @@ -217,7 +217,7 @@ impl Node { unsafe { let rcl_node = self.handle.rcl_node.lock().unwrap(); rcl_get_node_names_with_enclaves( - &*rcl_node, + &**rcl_node, rcutils_get_default_allocator(), &mut rcl_names, &mut rcl_namespaces, @@ -266,7 +266,7 @@ impl Node { // SAFETY: The topic_name string was correctly allocated previously unsafe { let rcl_node = self.handle.rcl_node.lock().unwrap(); - rcl_count_publishers(&*rcl_node, topic_name.as_ptr(), &mut count).ok()? + rcl_count_publishers(&**rcl_node, topic_name.as_ptr(), &mut count).ok()? }; Ok(count) } @@ -282,7 +282,7 @@ impl Node { // SAFETY: The topic_name string was correctly allocated previously unsafe { let rcl_node = self.handle.rcl_node.lock().unwrap(); - rcl_count_subscribers(&*rcl_node, topic_name.as_ptr(), &mut count).ok()? + rcl_count_subscribers(&**rcl_node, topic_name.as_ptr(), &mut count).ok()? }; Ok(count) } @@ -333,7 +333,7 @@ impl Node { unsafe { let rcl_node = self.handle.rcl_node.lock().unwrap(); getter( - &*rcl_node, + &**rcl_node, &mut rcutils_get_default_allocator(), node_name.as_ptr(), node_namespace.as_ptr(), @@ -369,7 +369,7 @@ impl Node { unsafe { let rcl_node = self.handle.rcl_node.lock().unwrap(); getter( - &*rcl_node, + &**rcl_node, &mut rcutils_get_default_allocator(), topic.as_ptr(), false, diff --git a/rclrs/src/publisher.rs b/rclrs/src/publisher.rs index 2935ca322..54273bc19 100644 --- a/rclrs/src/publisher.rs +++ b/rclrs/src/publisher.rs @@ -38,7 +38,7 @@ impl Drop for PublisherHandle { // SAFETY: The entity lifecycle mutex is locked to protect against the risk of // global variables in the rmw implementation being unsafely modified during cleanup. unsafe { - rcl_publisher_fini(self.rcl_publisher.get_mut().unwrap(), &mut *rcl_node); + rcl_publisher_fini(self.rcl_publisher.get_mut().unwrap(), &mut **rcl_node); } } } @@ -111,7 +111,7 @@ where // variables in the rmw implementation being unsafely modified during cleanup. rcl_publisher_init( &mut rcl_publisher, - &*rcl_node, + &**rcl_node, type_support_ptr, topic_c_string.as_ptr(), &publisher_options, diff --git a/rclrs/src/service.rs b/rclrs/src/service.rs index ac43e51a8..ac8d1a2a6 100644 --- a/rclrs/src/service.rs +++ b/rclrs/src/service.rs @@ -41,7 +41,7 @@ impl Drop for ServiceHandle { // SAFETY: The entity lifecycle mutex is locked to protect against the risk of // global variables in the rmw implementation being unsafely modified during cleanup. unsafe { - rcl_service_fini(rcl_service, &mut *rcl_node); + rcl_service_fini(rcl_service, &mut **rcl_node); } } } @@ -116,7 +116,7 @@ where // variables in the rmw implementation being unsafely modified during initialization. rcl_service_init( &mut rcl_service, - &*rcl_node, + &**rcl_node, type_support, topic_c_string.as_ptr(), &service_options as *const _, diff --git a/rclrs/src/subscription.rs b/rclrs/src/subscription.rs index 36c241d19..47d93de1a 100644 --- a/rclrs/src/subscription.rs +++ b/rclrs/src/subscription.rs @@ -49,7 +49,7 @@ impl Drop for SubscriptionHandle { // SAFETY: The entity lifecycle mutex is locked to protect against the risk of // global variables in the rmw implementation being unsafely modified during cleanup. unsafe { - rcl_subscription_fini(rcl_subscription, &mut *rcl_node); + rcl_subscription_fini(rcl_subscription, &mut **rcl_node); } } } @@ -129,7 +129,7 @@ where // variables in the rmw implementation being unsafely modified during cleanup. rcl_subscription_init( &mut rcl_subscription, - &*rcl_node, + &**rcl_node, type_support, topic_c_string.as_ptr(), &subscription_options, From 673d478a908735d940ef5d28b1d1863918976d10 Mon Sep 17 00:00:00 2001 From: geoff-imdex Date: Mon, 28 Oct 2024 07:01:10 +0800 Subject: [PATCH 4/4] Fix linting issue with use statement --- rclrs/src/logging.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/rclrs/src/logging.rs b/rclrs/src/logging.rs index e50a2290d..e4f789c4d 100644 --- a/rclrs/src/logging.rs +++ b/rclrs/src/logging.rs @@ -3,9 +3,7 @@ // Adapted from https://github.com/sequenceplanner/r2r/blob/89cec03d07a1496a225751159cbc7bfb529d9dd1/r2r/src/utils.rs // Further adapted from https://github.com/mvukov/rules_ros2/pull/371 -use std::ffi::CString; -use std::sync::Mutex; -use std::time::Duration; +use std::{ffi::CString, sync::Mutex, time::Duration}; use crate::rcl_bindings::*;