From 85e7fea258891b4aa616b3f7112218a14bfc7998 Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Tue, 31 May 2022 11:42:37 +0000
Subject: [PATCH 01/22] Add methods related to node options to NodeBuilder

---
 rclrs/src/node/builder.rs | 148 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 145 insertions(+), 3 deletions(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 99309d050..136998234 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -13,6 +13,10 @@ use std::sync::Arc;
 ///
 /// The default values for optional fields are:
 /// - `namespace: "/"`
+/// - `domain_id: ROS_DOMAIN_ID`
+/// - `use_global_arguments: true`
+/// - `arguments: []`
+/// - `enable_rosout`: true`
 ///
 /// # Example
 /// ```
@@ -40,6 +44,10 @@ pub struct NodeBuilder {
     context: Arc<Mutex<rcl_context_t>>,
     name: String,
     namespace: String,
+    domain_id: usize,
+    use_global_arguments: bool,
+    arguments: Vec<String>,
+    enable_rosout: bool,
 }
 
 impl NodeBuilder {
@@ -80,10 +88,18 @@ impl NodeBuilder {
     /// [2]: https://docs.ros2.org/latest/api/rmw/validate__node__name_8h.html#a5690a285aed9735f89ef11950b6e39e3
     /// [3]: NodeBuilder::build
     pub fn new(context: &Context, name: &str) -> NodeBuilder {
+        let mut domain_id = 0;
+        let ret = unsafe { rcl_get_default_domain_id(&mut domain_id) };
+        debug_assert_eq!(ret, 0);
+
         NodeBuilder {
             context: context.handle.clone(),
             name: name.to_string(),
             namespace: "/".to_string(),
+            domain_id,
+            use_global_arguments: true,
+            arguments: vec![],
+            enable_rosout: true,
         }
     }
 
@@ -142,6 +158,102 @@ impl NodeBuilder {
         self
     }
 
+    /// Sets node specific domain id
+    ///
+    /// domain id is used in DDS to calculate UDP port that will be used for discovery and communication.
+    /// [This article][1] describes how to calculate UDP port number from domain id.
+    /// From formula in above article, available domain id is between 0 to 232.
+    /// To skip about background described above, you can safely choose between 0 to 101.
+    ///
+    /// For detailed descriptions, see [here][2]
+    ///
+    /// # Example
+    /// ```
+    /// # use rclrs::{Context, Node, NodeBuilder, RclrsError};
+    /// let context = Context::new([])?;
+    /// let node = context
+    ///   .create_node_builder("my_node")
+    ///   .domain_id(42)
+    ///   .build()?;
+    /// assert_eq!(node.domain_id(), 42);
+    /// # Ok::<(), RclrsError>(())
+    /// ```
+    ///
+    /// [1]: https://community.rti.com/content/forum-topic/statically-configure-firewall-let-omg-dds-traffic-through
+    /// [2]: https://docs.ros.org/en/foxy/Concepts/About-Domain-ID.html
+    pub fn domain_id(mut self, domain_id: usize) -> Self {
+        self.domain_id = domain_id;
+        self
+    }
+
+    /// Sets flag for using global arguments.
+    /// If false then only use arguments passed to this builder,
+    /// otherwise use global arguments also.
+    ///
+    /// # Example
+    /// ```
+    /// # use rclrs::{Context, Node, NodeBuilder, RclrsError};
+    /// let args = ["--ros-args", "--remap", "my_node:__node:=your_node"]
+    ///   .iter()
+    ///   .map(|s| s.to_string())
+    ///   .collect::<Vec<_>>();
+    /// let context = Context::new(args)?;
+    /// // Use global arguments
+    /// let node = context
+    ///   .create_node_builder("my_node")
+    ///   .use_global_arguments(true)
+    ///   .build()?;
+    /// assert_eq!(node.name(), "your_node");
+    ///
+    /// // Not use global arguments
+    /// let node = context
+    ///   .create_node_builder("my_node")
+    ///   .use_global_arguments(false)
+    ///   .build()?;
+    /// assert_eq!(node.name(), "my_node");
+    /// # Ok::<(), RclrsError>(())
+    pub fn use_global_arguments(mut self, enable: bool) -> Self {
+        self.use_global_arguments = enable;
+        self
+    }
+
+    /// Sets node specific command line arguments.
+    ///
+    /// Command line arguments can mix ROS specific and not specific arguments.
+    /// ROS specific arguments are wrapped between `--ros-args` and `--`.
+    /// Note that `--ros-args` followed by only ROS specific arguments is valid pattern.
+    ///
+    /// below arguments examples are valid:
+    ///   --ros-args -r from:=to -- user_arg0 user_arg1
+    ///   --ros-args -p name:=value
+    ///    
+    /// # Example
+    /// ```
+    /// # use rclrs::{Context, Node, NodeBuilder, RclrsError};
+    /// let context = Context::new([])?;
+    /// let node_args = ["--ros-args", "--remap", "my_node:__node:=your_node"]
+    ///   .iter()
+    ///   .map(|s| s.to_string())
+    ///   .collect::<Vec<_>>();
+    /// let node = context
+    ///   .create_node_builder("my_node")
+    ///   .arguments(node_args)
+    ///   .build()?;
+    /// assert_eq!(node.name(), "your_node");
+    /// # Ok::<(), RclrsError>(())
+    /// ```
+    pub fn arguments(mut self, arguments: impl IntoIterator<Item = String>) -> Self {
+        self.arguments = arguments.into_iter().collect();
+        self
+    }
+
+    /// Enables rosout.
+    /// If false, rosout logging is disabled.
+    pub fn enable_rosout(mut self, enable: bool) -> Self {
+        self.enable_rosout = enable;
+        self
+    }
+
     /// Builds the node instance.
     ///
     /// Node name and namespace validation is performed in this method.
@@ -149,7 +261,10 @@ impl NodeBuilder {
     /// For example usage, see the [`NodeBuilder`][1] docs.
     ///
     /// # Panics
-    /// When the node name or namespace contain null bytes.
+    /// If any of below conditions are filled, panic occurs
+    ///  - node name contains null byte
+    ///  - namespace contains null byte
+    ///  - any of node argument strings contain null byte.
     ///
     /// [1]: crate::NodeBuilder
     pub fn build(&self) -> Result<Node, RclrsError> {
@@ -159,11 +274,38 @@ impl NodeBuilder {
         // SAFETY: No preconditions for this function.
         let mut node_handle = unsafe { rcl_get_zero_initialized_node() };
 
+        // SAFETY: No preconditions for this function.
+        let mut node_options = unsafe { rcl_node_get_default_options() };
+
+        let cstr_args = self
+            .arguments
+            .iter()
+            .map(|s| CString::new(s.as_str()).unwrap())
+            .collect::<Vec<_>>();
+        let cstr_arg_ptrs = cstr_args.iter().map(|s| s.as_ptr()).collect::<Vec<_>>();
+        // SAFETY: No preconditions for this function.
+        let mut arguments = unsafe { rcl_get_zero_initialized_arguments() };
         unsafe {
             // SAFETY: No preconditions for this function.
-            let context_handle = &mut *self.context.lock();
+            rcl_parse_arguments(
+                cstr_arg_ptrs.len() as i32,
+                cstr_arg_ptrs.as_ptr(),
+                rcutils_get_default_allocator(),
+                &mut arguments,
+            )
+        }
+        .ok()?;
+
+        node_options.domain_id = self.domain_id;
+        node_options.arguments = arguments;
+        node_options.use_global_arguments = self.use_global_arguments;
+        node_options.enable_rosout = self.enable_rosout;
+        //SAFETY: No preconditions for this function.
+        node_options.allocator = unsafe { rcutils_get_default_allocator() };
+
+        unsafe {
             // SAFETY: No preconditions for this function.
-            let node_options = rcl_node_get_default_options();
+            let context_handle = &mut *self.context.lock();
 
             // SAFETY: The node handle is zero-initialized as expected by this function.
             // The strings and node options are copied by this function, so we don't need

From b04b17acba9503a73ec83847bd83d8d89fbb279c Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Wed, 1 Jun 2022 13:52:54 +0000
Subject: [PATCH 02/22] Format comment and move line to put near from related
 ones

---
 rclrs/src/node/builder.rs | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 136998234..dd0873c3b 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -271,9 +271,6 @@ impl NodeBuilder {
         let node_name = CString::new(self.name.as_str()).unwrap();
         let node_namespace = CString::new(self.namespace.as_str()).unwrap();
 
-        // SAFETY: No preconditions for this function.
-        let mut node_handle = unsafe { rcl_get_zero_initialized_node() };
-
         // SAFETY: No preconditions for this function.
         let mut node_options = unsafe { rcl_node_get_default_options() };
 
@@ -300,9 +297,11 @@ impl NodeBuilder {
         node_options.arguments = arguments;
         node_options.use_global_arguments = self.use_global_arguments;
         node_options.enable_rosout = self.enable_rosout;
-        //SAFETY: No preconditions for this function.
+        // SAFETY: No preconditions for this function.
         node_options.allocator = unsafe { rcutils_get_default_allocator() };
 
+        // SAFETY: No preconditions for this function.
+        let mut node_handle = unsafe { rcl_get_zero_initialized_node() };
         unsafe {
             // SAFETY: No preconditions for this function.
             let context_handle = &mut *self.context.lock();

From a6ce30f335d0819d512476f72a280509a208137c Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Thu, 2 Jun 2022 10:41:10 +0000
Subject: [PATCH 03/22] Remove domain_id because of erased field from galactic

---
 rclrs/src/node/builder.rs | 35 -----------------------------------
 1 file changed, 35 deletions(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index dd0873c3b..f27e1c805 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -44,7 +44,6 @@ pub struct NodeBuilder {
     context: Arc<Mutex<rcl_context_t>>,
     name: String,
     namespace: String,
-    domain_id: usize,
     use_global_arguments: bool,
     arguments: Vec<String>,
     enable_rosout: bool,
@@ -88,15 +87,10 @@ impl NodeBuilder {
     /// [2]: https://docs.ros2.org/latest/api/rmw/validate__node__name_8h.html#a5690a285aed9735f89ef11950b6e39e3
     /// [3]: NodeBuilder::build
     pub fn new(context: &Context, name: &str) -> NodeBuilder {
-        let mut domain_id = 0;
-        let ret = unsafe { rcl_get_default_domain_id(&mut domain_id) };
-        debug_assert_eq!(ret, 0);
-
         NodeBuilder {
             context: context.handle.clone(),
             name: name.to_string(),
             namespace: "/".to_string(),
-            domain_id,
             use_global_arguments: true,
             arguments: vec![],
             enable_rosout: true,
@@ -158,34 +152,6 @@ impl NodeBuilder {
         self
     }
 
-    /// Sets node specific domain id
-    ///
-    /// domain id is used in DDS to calculate UDP port that will be used for discovery and communication.
-    /// [This article][1] describes how to calculate UDP port number from domain id.
-    /// From formula in above article, available domain id is between 0 to 232.
-    /// To skip about background described above, you can safely choose between 0 to 101.
-    ///
-    /// For detailed descriptions, see [here][2]
-    ///
-    /// # Example
-    /// ```
-    /// # use rclrs::{Context, Node, NodeBuilder, RclrsError};
-    /// let context = Context::new([])?;
-    /// let node = context
-    ///   .create_node_builder("my_node")
-    ///   .domain_id(42)
-    ///   .build()?;
-    /// assert_eq!(node.domain_id(), 42);
-    /// # Ok::<(), RclrsError>(())
-    /// ```
-    ///
-    /// [1]: https://community.rti.com/content/forum-topic/statically-configure-firewall-let-omg-dds-traffic-through
-    /// [2]: https://docs.ros.org/en/foxy/Concepts/About-Domain-ID.html
-    pub fn domain_id(mut self, domain_id: usize) -> Self {
-        self.domain_id = domain_id;
-        self
-    }
-
     /// Sets flag for using global arguments.
     /// If false then only use arguments passed to this builder,
     /// otherwise use global arguments also.
@@ -293,7 +259,6 @@ impl NodeBuilder {
         }
         .ok()?;
 
-        node_options.domain_id = self.domain_id;
         node_options.arguments = arguments;
         node_options.use_global_arguments = self.use_global_arguments;
         node_options.enable_rosout = self.enable_rosout;

From a343aba43b8e06efd2fb0631080c9930b03a71ee Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Fri, 3 Jun 2022 00:02:30 +0900
Subject: [PATCH 04/22] Fix SAFETY comments

Co-authored-by: Nikolai Morin <nnmmgit@gmail.com>
---
 rclrs/src/node/builder.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index f27e1c805..02690974f 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -265,7 +265,7 @@ impl NodeBuilder {
         // SAFETY: No preconditions for this function.
         node_options.allocator = unsafe { rcutils_get_default_allocator() };
 
-        // SAFETY: No preconditions for this function.
+        // SAFETY: Getting a zero-initialized value is always safe.
         let mut node_handle = unsafe { rcl_get_zero_initialized_node() };
         unsafe {
             // SAFETY: No preconditions for this function.

From e11f01e0f966004977da686947f0e9512758985c Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Fri, 3 Jun 2022 00:18:06 +0900
Subject: [PATCH 05/22] Fix SAFETY docs for rcl_parse_arguments

Co-authored-by: Nikolai Morin <nnmmgit@gmail.com>
---
 rclrs/src/node/builder.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 02690974f..f8fbf68b1 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -249,7 +249,8 @@ impl NodeBuilder {
         // SAFETY: No preconditions for this function.
         let mut arguments = unsafe { rcl_get_zero_initialized_arguments() };
         unsafe {
-            // SAFETY: No preconditions for this function.
+            // SAFETY: This function does not store the ephemeral cstr_args_ptrs
+            // pointers. We are passing in a zero-initialized arguments struct as expected.
             rcl_parse_arguments(
                 cstr_arg_ptrs.len() as i32,
                 cstr_arg_ptrs.as_ptr(),

From 3785db04401569e018579e7f8492198fb1862d2d Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Thu, 2 Jun 2022 15:50:18 +0000
Subject: [PATCH 06/22] Separate procedure related to node_options in build()

---
 rclrs/src/node/builder.rs | 63 +++++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index f8fbf68b1..4e97a63f5 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -236,35 +236,7 @@ impl NodeBuilder {
     pub fn build(&self) -> Result<Node, RclrsError> {
         let node_name = CString::new(self.name.as_str()).unwrap();
         let node_namespace = CString::new(self.namespace.as_str()).unwrap();
-
-        // SAFETY: No preconditions for this function.
-        let mut node_options = unsafe { rcl_node_get_default_options() };
-
-        let cstr_args = self
-            .arguments
-            .iter()
-            .map(|s| CString::new(s.as_str()).unwrap())
-            .collect::<Vec<_>>();
-        let cstr_arg_ptrs = cstr_args.iter().map(|s| s.as_ptr()).collect::<Vec<_>>();
-        // SAFETY: No preconditions for this function.
-        let mut arguments = unsafe { rcl_get_zero_initialized_arguments() };
-        unsafe {
-            // SAFETY: This function does not store the ephemeral cstr_args_ptrs
-            // pointers. We are passing in a zero-initialized arguments struct as expected.
-            rcl_parse_arguments(
-                cstr_arg_ptrs.len() as i32,
-                cstr_arg_ptrs.as_ptr(),
-                rcutils_get_default_allocator(),
-                &mut arguments,
-            )
-        }
-        .ok()?;
-
-        node_options.arguments = arguments;
-        node_options.use_global_arguments = self.use_global_arguments;
-        node_options.enable_rosout = self.enable_rosout;
-        // SAFETY: No preconditions for this function.
-        node_options.allocator = unsafe { rcutils_get_default_allocator() };
+        let node_options = self.create_node_options()?;
 
         // SAFETY: Getting a zero-initialized value is always safe.
         let mut node_handle = unsafe { rcl_get_zero_initialized_node() };
@@ -294,4 +266,37 @@ impl NodeBuilder {
             subscriptions: std::vec![],
         })
     }
+
+    fn create_node_options(&self) -> Result<rcl_node_options_t, RclrsError> {
+        // SAFETY: No preconditions for this function.
+        let mut node_options = unsafe { rcl_node_get_default_options() };
+
+        let cstring_args = self
+            .arguments
+            .iter()
+            .map(|s| CString::new(s.as_str()).unwrap())
+            .collect::<Vec<_>>();
+        let cstring_arg_ptrs = cstring_args.iter().map(|s| s.as_ptr()).collect::<Vec<_>>();
+        // SAFETY: Getting a zero-initialized value is always safe.
+        let mut arguments = unsafe { rcl_get_zero_initialized_arguments() };
+        unsafe {
+            // SAFETY: This function does not store the ephemeral cstr_args_ptrs
+            // pointers. We are passing in a zero-initialized arguments struct as expected.
+            rcl_parse_arguments(
+                cstring_arg_ptrs.len() as i32,
+                cstring_arg_ptrs.as_ptr(),
+                rcutils_get_default_allocator(),
+                &mut arguments,
+            )
+        }
+        .ok()?;
+
+        node_options.arguments = arguments;
+        node_options.use_global_arguments = self.use_global_arguments;
+        node_options.enable_rosout = self.enable_rosout;
+        // SAFETY: No preconditions for this function.
+        node_options.allocator = unsafe { rcutils_get_default_allocator() };
+
+        Ok(node_options)
+    }
 }

From d9416dd5490d8257371b2cb5abb737bf643bfc91 Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Fri, 3 Jun 2022 22:13:52 +0900
Subject: [PATCH 07/22] Replace docs about arguments method

Co-authored-by: Nikolai Morin <nnmmgit@gmail.com>
---
 rclrs/src/node/builder.rs | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 4e97a63f5..fed585626 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -183,31 +183,31 @@ impl NodeBuilder {
         self
     }
 
-    /// Sets node specific command line arguments.
+    /// Sets node-specific command line arguments.
     ///
-    /// Command line arguments can mix ROS specific and not specific arguments.
-    /// ROS specific arguments are wrapped between `--ros-args` and `--`.
-    /// Note that `--ros-args` followed by only ROS specific arguments is valid pattern.
-    ///
-    /// below arguments examples are valid:
-    ///   --ros-args -r from:=to -- user_arg0 user_arg1
-    ///   --ros-args -p name:=value
+    /// These arguments are parsed the same way as those for [`Context::new()`][1].
+    /// However, the node-specific command line arguments have higher precedence than the arguments
+    /// used in creating the context.
     ///    
     /// # Example
     /// ```
     /// # use rclrs::{Context, Node, NodeBuilder, RclrsError};
-    /// let context = Context::new([])?;
-    /// let node_args = ["--ros-args", "--remap", "my_node:__node:=your_node"]
-    ///   .iter()
-    ///   .map(|s| s.to_string())
-    ///   .collect::<Vec<_>>();
+    /// // Usually, this would change the name of "my_node" to "context_args_node":
+    /// let context_args = ["--ros-args", "--remap", "my_node:__node:=context_args_node"]
+    ///   .map(String::from);
+    /// let context = Context::new(context_args)?;
+    /// // But the node arguments will change it to "node_args_node":
+    /// let node_args = ["--ros-args", "--remap", "my_node:__node:=node_args_node"]
+    ///   .map(String::from);
     /// let node = context
     ///   .create_node_builder("my_node")
     ///   .arguments(node_args)
     ///   .build()?;
-    /// assert_eq!(node.name(), "your_node");
+    /// assert_eq!(node.name(), "node_args_node");
     /// # Ok::<(), RclrsError>(())
     /// ```
+    ///
+    /// [1]: crate::Context::new
     pub fn arguments(mut self, arguments: impl IntoIterator<Item = String>) -> Self {
         self.arguments = arguments.into_iter().collect();
         self

From 35cce29b9a5e2673ce80c90fddd7c62d67c619ae Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Fri, 3 Jun 2022 22:14:31 +0900
Subject: [PATCH 08/22] Replace docs about use_global_arguments method

Co-authored-by: Nikolai Morin <nnmmgit@gmail.com>
---
 rclrs/src/node/builder.rs | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index fed585626..ef9df4a46 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -152,32 +152,32 @@ impl NodeBuilder {
         self
     }
 
-    /// Sets flag for using global arguments.
-    /// If false then only use arguments passed to this builder,
-    /// otherwise use global arguments also.
+    /// Enables or disables using global arguments.
+    ///
+    /// The "global" arguments are those used in [creating the context][1].
     ///
     /// # Example
     /// ```
     /// # use rclrs::{Context, Node, NodeBuilder, RclrsError};
-    /// let args = ["--ros-args", "--remap", "my_node:__node:=your_node"]
-    ///   .iter()
-    ///   .map(|s| s.to_string())
-    ///   .collect::<Vec<_>>();
-    /// let context = Context::new(args)?;
-    /// // Use global arguments
-    /// let node = context
-    ///   .create_node_builder("my_node")
-    ///   .use_global_arguments(true)
-    ///   .build()?;
-    /// assert_eq!(node.name(), "your_node");
-    ///
-    /// // Not use global arguments
-    /// let node = context
+    /// let context_args = ["--ros-args", "--remap", "__node:=your_node"]
+    ///   .map(String::from);
+    /// let context = Context::new(context_args)?;
+    /// // Ignore the global arguments:
+    /// let node_without_global_args = context
     ///   .create_node_builder("my_node")
     ///   .use_global_arguments(false)
     ///   .build()?;
-    /// assert_eq!(node.name(), "my_node");
+    /// assert_eq!(node_without_global_args.name(), "my_node");
+    /// // Do not ignore the global arguments:
+    /// let node_with_global_args = context
+    ///   .create_node_builder("my_other_node")
+    ///   .use_global_arguments(true)
+    ///   .build()?;
+    /// assert_eq!(node_with_global_args.name(), "your_node");
     /// # Ok::<(), RclrsError>(())
+    /// ```
+    ///
+    /// [1]: crate::Context::new
     pub fn use_global_arguments(mut self, enable: bool) -> Self {
         self.use_global_arguments = enable;
         self

From b5323a31e15b73546c57ce27751b373ef2f23a10 Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Fri, 3 Jun 2022 13:14:50 +0000
Subject: [PATCH 09/22] Add Drop impls

---
 rclrs/src/node/builder.rs | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 4e97a63f5..5f30f5bd1 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -13,10 +13,9 @@ use std::sync::Arc;
 ///
 /// The default values for optional fields are:
 /// - `namespace: "/"`
-/// - `domain_id: ROS_DOMAIN_ID`
 /// - `use_global_arguments: true`
 /// - `arguments: []`
-/// - `enable_rosout`: true`
+/// - `enable_rosout: true`
 ///
 /// # Example
 /// ```
@@ -267,6 +266,8 @@ impl NodeBuilder {
         })
     }
 
+    /// Creates node options.
+    /// options are overrided by field values of builder.    
     fn create_node_options(&self) -> Result<rcl_node_options_t, RclrsError> {
         // SAFETY: No preconditions for this function.
         let mut node_options = unsafe { rcl_node_get_default_options() };
@@ -280,7 +281,7 @@ impl NodeBuilder {
         // SAFETY: Getting a zero-initialized value is always safe.
         let mut arguments = unsafe { rcl_get_zero_initialized_arguments() };
         unsafe {
-            // SAFETY: This function does not store the ephemeral cstr_args_ptrs
+            // SAFETY: This function does not store the ephemeral cstring_args_ptrs
             // pointers. We are passing in a zero-initialized arguments struct as expected.
             rcl_parse_arguments(
                 cstring_arg_ptrs.len() as i32,
@@ -300,3 +301,21 @@ impl NodeBuilder {
         Ok(node_options)
     }
 }
+
+impl Drop for rcl_arguments_t {
+    fn drop(&mut self) {
+        // SAFETY: Do not finish this struct except here.
+        unsafe {
+            rcl_arguments_fini(self);
+        }
+    }
+}
+
+impl Drop for rcl_node_options_t {
+    fn drop(&mut self) {
+        // SAFETY: Do not finish this struct except here.
+        unsafe {
+            rcl_node_options_fini(self);
+        }
+    }
+}

From 58d02fa7acb52851968cc7144bbe3e5d56a34dc8 Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Fri, 3 Jun 2022 13:19:07 +0000
Subject: [PATCH 10/22] Add reference for details about command line arguments

---
 rclrs/src/node/builder.rs | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 2468b0ee0..912a28704 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -187,6 +187,8 @@ impl NodeBuilder {
     /// These arguments are parsed the same way as those for [`Context::new()`][1].
     /// However, the node-specific command line arguments have higher precedence than the arguments
     /// used in creating the context.
+    ///
+    /// For more details about command line arguments, see [here][2].
     ///    
     /// # Example
     /// ```
@@ -207,6 +209,7 @@ impl NodeBuilder {
     /// ```
     ///
     /// [1]: crate::Context::new
+    /// [2]: https://design.ros2.org/articles/ros_command_line_arguments.html
     pub fn arguments(mut self, arguments: impl IntoIterator<Item = String>) -> Self {
         self.arguments = arguments.into_iter().collect();
         self

From 612d7d5733961c8791067211545dfa262bc8f115 Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Fri, 3 Jun 2022 13:21:24 +0000
Subject: [PATCH 11/22] remove wasted line

---
 rclrs/src/node/builder.rs | 1 -
 1 file changed, 1 deletion(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 912a28704..261c189ec 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -38,7 +38,6 @@ use std::sync::Arc;
 ///
 /// [1]: crate::Node
 /// [2]: crate::Node::builder
-///
 pub struct NodeBuilder {
     context: Arc<Mutex<rcl_context_t>>,
     name: String,

From c2d90493608a6729e4c6930056f44288f44a25a9 Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Sat, 4 Jun 2022 07:40:24 +0900
Subject: [PATCH 12/22] Add more detailed comments for enable_rosout method

Co-authored-by: Nikolai Morin <nnmmgit@gmail.com>
---
 rclrs/src/node/builder.rs | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 912a28704..8de1c4664 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -215,8 +215,12 @@ impl NodeBuilder {
         self
     }
 
-    /// Enables rosout.
-    /// If false, rosout logging is disabled.
+    /// Enables or disables logging to rosout.
+    ///
+    /// When enabled, log messages are published to the `/rosout` topic in addition to
+    /// standard output.
+    ///
+    /// This option is currently unused in `rclrs`.
     pub fn enable_rosout(mut self, enable: bool) -> Self {
         self.enable_rosout = enable;
         self

From bb0996285efd1b19eeb1192ef8eac6122d95fd55 Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Sat, 4 Jun 2022 00:33:58 +0000
Subject: [PATCH 13/22] Add null check to prevent from calling post process
 twice

---
 rclrs/src/node/builder.rs | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 261c189ec..2127f4d0f 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -281,7 +281,6 @@ impl NodeBuilder {
             .collect::<Vec<_>>();
         let cstring_arg_ptrs = cstring_args.iter().map(|s| s.as_ptr()).collect::<Vec<_>>();
         // SAFETY: Getting a zero-initialized value is always safe.
-        let mut arguments = unsafe { rcl_get_zero_initialized_arguments() };
         unsafe {
             // SAFETY: This function does not store the ephemeral cstring_args_ptrs
             // pointers. We are passing in a zero-initialized arguments struct as expected.
@@ -289,12 +288,11 @@ impl NodeBuilder {
                 cstring_arg_ptrs.len() as i32,
                 cstring_arg_ptrs.as_ptr(),
                 rcutils_get_default_allocator(),
-                &mut arguments,
+                &mut node_options.arguments,
             )
         }
         .ok()?;
 
-        node_options.arguments = arguments;
         node_options.use_global_arguments = self.use_global_arguments;
         node_options.enable_rosout = self.enable_rosout;
         // SAFETY: No preconditions for this function.
@@ -306,9 +304,13 @@ impl NodeBuilder {
 
 impl Drop for rcl_arguments_t {
     fn drop(&mut self) {
-        // SAFETY: Do not finish this struct except here.
-        unsafe {
-            rcl_arguments_fini(self);
+        if !self.impl_.is_null() {
+            // SAFETY: `rcl_arguments_t.impl_` must be not NULL.
+            // However, after calling `rcl_node_options_fini`, this field become NULL.
+            // To prevent from calling drop twice, put NULL check.
+            unsafe {
+                rcl_arguments_fini(self).ok().unwrap();
+            }
         }
     }
 }
@@ -317,7 +319,7 @@ impl Drop for rcl_node_options_t {
     fn drop(&mut self) {
         // SAFETY: Do not finish this struct except here.
         unsafe {
-            rcl_node_options_fini(self);
+            rcl_node_options_fini(self).ok().unwrap();
         }
     }
 }

From 804bbb20bca54e60e210f5eddd4c45110cdcf502 Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Sat, 4 Jun 2022 12:40:57 +0900
Subject: [PATCH 14/22] Fix valid string statements

Co-authored-by: Nikolai Morin <nnmmgit@gmail.com>
---
 rclrs/src/node/builder.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index f1360fe25..63cafc139 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -232,10 +232,10 @@ impl NodeBuilder {
     /// For example usage, see the [`NodeBuilder`][1] docs.
     ///
     /// # Panics
-    /// If any of below conditions are filled, panic occurs
-    ///  - node name contains null byte
-    ///  - namespace contains null byte
-    ///  - any of node argument strings contain null byte.
+    /// If any of the below conditions are fulfilled, a panic occurs:
+    ///  - The node name contains a null byte
+    ///  - The namespace contains a null byte
+    ///  - Any of the node arguments contain a null byte
     ///
     /// [1]: crate::NodeBuilder
     pub fn build(&self) -> Result<Node, RclrsError> {

From 1d469632e72959efa7a1cb7b980e9421a0f1797e Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Sat, 4 Jun 2022 03:43:27 +0000
Subject: [PATCH 15/22] Exclude context lock from unsafe block

---
 rclrs/src/node/builder.rs | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 63cafc139..267b4e7b0 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -242,13 +242,11 @@ impl NodeBuilder {
         let node_name = CString::new(self.name.as_str()).unwrap();
         let node_namespace = CString::new(self.namespace.as_str()).unwrap();
         let node_options = self.create_node_options()?;
+        let context_handle = &mut *self.context.lock();
 
         // SAFETY: Getting a zero-initialized value is always safe.
         let mut node_handle = unsafe { rcl_get_zero_initialized_node() };
         unsafe {
-            // SAFETY: No preconditions for this function.
-            let context_handle = &mut *self.context.lock();
-
             // SAFETY: The node handle is zero-initialized as expected by this function.
             // The strings and node options are copied by this function, so we don't need
             // to keep them alive.

From aded175f206aca45151305f60255ef332e3490f1 Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Thu, 9 Jun 2022 15:31:33 +0000
Subject: [PATCH 16/22] merge from upstream

---
 rclrs/src/node/builder.rs | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 267b4e7b0..1cf388cf3 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -231,16 +231,18 @@ impl NodeBuilder {
     ///
     /// For example usage, see the [`NodeBuilder`][1] docs.
     ///
-    /// # Panics
-    /// If any of the below conditions are fulfilled, a panic occurs:
-    ///  - The node name contains a null byte
-    ///  - The namespace contains a null byte
-    ///  - Any of the node arguments contain a null byte
-    ///
     /// [1]: crate::NodeBuilder
     pub fn build(&self) -> Result<Node, RclrsError> {
-        let node_name = CString::new(self.name.as_str()).unwrap();
-        let node_namespace = CString::new(self.namespace.as_str()).unwrap();
+        let node_name =
+            CString::new(self.name.as_str()).map_err(|err| RclrsError::StringContainsNul {
+                err,
+                s: self.name.clone(),
+            })?;
+        let node_namespace =
+            CString::new(self.namespace.as_str()).map_err(|err| RclrsError::StringContainsNul {
+                err,
+                s: self.namespace.clone(),
+            })?;
         let node_options = self.create_node_options()?;
         let context_handle = &mut *self.context.lock();
 

From 39ca296d5a5453108a1841f79772a34089279a6c Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Thu, 9 Jun 2022 15:34:22 +0000
Subject: [PATCH 17/22] Remove Drop for rcl_arguments_t

---
 rclrs/src/node/builder.rs | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 1cf388cf3..0e8d70902 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -306,19 +306,6 @@ impl NodeBuilder {
     }
 }
 
-impl Drop for rcl_arguments_t {
-    fn drop(&mut self) {
-        if !self.impl_.is_null() {
-            // SAFETY: `rcl_arguments_t.impl_` must be not NULL.
-            // However, after calling `rcl_node_options_fini`, this field become NULL.
-            // To prevent from calling drop twice, put NULL check.
-            unsafe {
-                rcl_arguments_fini(self).ok().unwrap();
-            }
-        }
-    }
-}
-
 impl Drop for rcl_node_options_t {
     fn drop(&mut self) {
         // SAFETY: Do not finish this struct except here.

From cc893cadd700b84264cab1583944ed17480ba439 Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Thu, 9 Jun 2022 15:56:08 +0000
Subject: [PATCH 18/22] Add statement about rcl_arguments_t fininalization

---
 rclrs/src/node/builder.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 0e8d70902..07c20e3d6 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -310,6 +310,7 @@ impl Drop for rcl_node_options_t {
     fn drop(&mut self) {
         // SAFETY: Do not finish this struct except here.
         unsafe {
+            // This also finalizes the `rcl_arguments_t` contained in `rcl_node_options_t`,
             rcl_node_options_fini(self).ok().unwrap();
         }
     }

From aad27b4faca00b4fb9cbb0da23624751a8c59118 Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Thu, 9 Jun 2022 16:49:45 +0000
Subject: [PATCH 19/22] Fix comments

---
 rclrs/src/node/builder.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 07c20e3d6..faa7ce01d 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -310,7 +310,7 @@ impl Drop for rcl_node_options_t {
     fn drop(&mut self) {
         // SAFETY: Do not finish this struct except here.
         unsafe {
-            // This also finalizes the `rcl_arguments_t` contained in `rcl_node_options_t`,
+            // This also finalizes the `rcl_arguments_t` contained in `rcl_node_options_t`.
             rcl_node_options_fini(self).ok().unwrap();
         }
     }

From 523ec5b3177094edc4e4ce20cdc393a4ab4632dc Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Thu, 9 Jun 2022 17:27:01 +0000
Subject: [PATCH 20/22] Add failsafe code like NulError checker in build

---
 rclrs/src/node/builder.rs | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index faa7ce01d..85dbc4216 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -273,15 +273,31 @@ impl NodeBuilder {
     }
 
     /// Creates node options.
-    /// options are overrided by field values of builder.    
+    ///
+    /// Any fields not present in the builder will have their default value.
+    /// For detail about default values, see [`NodeBuilder`][1] docs.
+    ///
+    /// [1]: crate::NodeBuilder
     fn create_node_options(&self) -> Result<rcl_node_options_t, RclrsError> {
         // SAFETY: No preconditions for this function.
         let mut node_options = unsafe { rcl_node_get_default_options() };
 
-        let cstring_args = self
+        let (cstring_args_results, mut parse_err_results): (Vec<_>, Vec<_>) = self
             .arguments
             .iter()
-            .map(|s| CString::new(s.as_str()).unwrap())
+            .map(|s| match CString::new(s.as_str()) {
+                Ok(cstr) => Ok(cstr),
+                Err(err) => Err(RclrsError::StringContainsNul { s: s.clone(), err }),
+            })
+            .partition(Result::is_ok);
+
+        if let Some(err) = parse_err_results.pop() {
+            return Err(err.unwrap_err());
+        }
+
+        let cstring_args = cstring_args_results
+            .into_iter()
+            .map(|r| r.unwrap())
             .collect::<Vec<_>>();
         let cstring_arg_ptrs = cstring_args.iter().map(|s| s.as_ptr()).collect::<Vec<_>>();
         // SAFETY: Getting a zero-initialized value is always safe.

From 33e65d0871ae535cb80e56b094c2c01753f2fc51 Mon Sep 17 00:00:00 2001
From: SoyaOhnishi <soya.ohnishi@gmail.com>
Date: Fri, 10 Jun 2022 02:55:03 +0000
Subject: [PATCH 21/22] Fix to deal with erroneous case of cstring cast

---
 rclrs/src/node/builder.rs | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 85dbc4216..f8e74da4a 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -282,23 +282,15 @@ impl NodeBuilder {
         // SAFETY: No preconditions for this function.
         let mut node_options = unsafe { rcl_node_get_default_options() };
 
-        let (cstring_args_results, mut parse_err_results): (Vec<_>, Vec<_>) = self
+        let cstring_args = self
             .arguments
             .iter()
             .map(|s| match CString::new(s.as_str()) {
                 Ok(cstr) => Ok(cstr),
                 Err(err) => Err(RclrsError::StringContainsNul { s: s.clone(), err }),
             })
-            .partition(Result::is_ok);
+            .collect::<Result<Vec<_>, _>>()?;
 
-        if let Some(err) = parse_err_results.pop() {
-            return Err(err.unwrap_err());
-        }
-
-        let cstring_args = cstring_args_results
-            .into_iter()
-            .map(|r| r.unwrap())
-            .collect::<Vec<_>>();
         let cstring_arg_ptrs = cstring_args.iter().map(|s| s.as_ptr()).collect::<Vec<_>>();
         // SAFETY: Getting a zero-initialized value is always safe.
         unsafe {

From 74277017e9cb78c5e5dad02f945c45cf44c8b1d7 Mon Sep 17 00:00:00 2001
From: Nikolai Morin <nnmmgit@gmail.com>
Date: Fri, 10 Jun 2022 10:45:01 +0200
Subject: [PATCH 22/22] Remove outdated safety comment

---
 rclrs/src/node/builder.rs | 1 -
 1 file changed, 1 deletion(-)

diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index f8e74da4a..b94812ead 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -292,7 +292,6 @@ impl NodeBuilder {
             .collect::<Result<Vec<_>, _>>()?;
 
         let cstring_arg_ptrs = cstring_args.iter().map(|s| s.as_ptr()).collect::<Vec<_>>();
-        // SAFETY: Getting a zero-initialized value is always safe.
         unsafe {
             // SAFETY: This function does not store the ephemeral cstring_args_ptrs
             // pointers. We are passing in a zero-initialized arguments struct as expected.