Skip to content
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 rosout logging to rclrs #422

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions rclrs/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -263,7 +263,7 @@ where
pub fn service_is_ready(&self) -> Result<bool, RclrsError> {
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
Expand Down
26 changes: 26 additions & 0 deletions rclrs/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ 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);
}
}
Expand All @@ -56,6 +60,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<ContextHandle>,
}
Expand Down Expand Up @@ -142,6 +150,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()
Copy link
Collaborator

@mxgrey mxgrey Oct 30, 2024

Choose a reason for hiding this comment

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

This will get called every time a Context is created, but it's meant to be called only once. It's possible for users to create multiple Contexts in one application (we do this a lot in tests), so we'd be making redundant calls.

Similar to ros2/rclcpp#998 we should guard this from being called multiple times. We also need to make sure that rcl_logging_fini is only called after all Contexts are destructed.

This is a tricky problem to resolve in a sound way. I might suggest something like this:

struct LoggingConfiguration {
    lifecycle: Mutex<Weak<LoggingLifecycle>>
}

impl LoggingConfiguration {
    fn configure(args: &rcl_arguments_t) -> Result<Arc<LoggingLifecycle>, RclError> {
        static CONFIGURATION: LazyLock<Self> = LazyLock::new(|| Self { lifecycle: Mutex::new(Weak::new()) });
        let mut lifecycle = CONFIGURATION.lifecycle.lock().unwrap();
        if let Some(arc_lifecycle) = lifecycle.upgrade() {
            return Ok(arc_lifecycle);
        }
        let arc_lifecycle = Arc::new(LoggingLifecycle::new(args)?);
        *lifecycle = arc_lifecycle.downgrade();
        Ok(arc_lifecycle);
    }
}

struct LoggingLifecycle;
impl LoggingLifecycle {
    fn new(args: &rcl_arguments_t) -> Result<Self, RclError> {
        let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap();
        let allocator = rcutils_get_default_allocator();
        rcl_logging_configure(args, &allocator).ok()?;
        Self
    }
}

impl Drop for LoggingLifecycle {
    fn drop(&mut self) {
        let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap();
        rcl_logging_fini();
    }
}

Then we would add an Arc<LoggingLifecycle> to the ContextHandle. Once all ContextHandles are gone, the LoggingLifecycle will drop and call rcl_logging_fini. If a new context is made after that, rcl_logging_configure will be called again.

Copy link
Author

Choose a reason for hiding this comment

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

Really good pick-up and suggestion @mxgrey - thank you. Will update the code to make use of your suggestion.

};
ret?
}
Ok(Self {
handle: Arc::new(ContextHandle {
Expand Down
2 changes: 2 additions & 0 deletions rclrs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ mod clock;
mod context;
mod error;
mod executor;
mod logging;
mod node;
mod parameter;
mod publisher;
Expand Down Expand Up @@ -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::*;
Expand Down
Loading
Loading