Better error handling and better Comments

rclrs/src/client.rs

@@ -30,12 +30,27 @@ pub struct ClientHandle {
 }
 
 impl ClientHandle {
+    /// Locks the `rcl_client_t` instance and returns a mutable reference to it.
+    ///
+    /// # Returns
+    ///
+    /// A `MutexGuard` holding a mutable reference to the `rcl_client_t` instance.
     pub(crate) fn lock(&self) -> MutexGuard<rcl_client_t> {
         self.rcl_client.lock().unwrap()
     }
 }
 
 impl Drop for ClientHandle {
+    /// Cleans up the ROS client when the `ClientHandle` instance is dropped.
+    ///
+    /// This implementation finalizes the `rcl_client_t` instance by calling `rcl_client_fini`.
+    /// It also acquires the entity lifecycle mutex to protect against the risk of global variables
+    /// in the rmw implementation being unsafely modified during cleanup.
+    ///
+    /// # Safety
+    ///
+    /// This function calls an unsafe function (`rcl_client_fini`) from the ROS client library.
+    /// The entity lifecycle mutex is locked to ensure thread safety during the cleanup process.
     fn drop(&mut self) {
         let rcl_client = self.rcl_client.get_mut().unwrap();
         let mut rcl_node = self.node_handle.rcl_node.lock().unwrap();
@@ -83,6 +98,23 @@ where
     T: rosidl_runtime_rs::Service,
 {
     /// Creates a new client.
+    ///
+    /// # Arguments
+    ///
+    /// * `node_handle` - An `Arc` reference to the `NodeHandle` associated with this client.
+    /// * `topic` - The name of the topic to which the client will send requests.
+    ///
+    /// # Returns
+    ///
+    /// A `Result` containing the newly created `Client` instance or an `RclrsError` if an error occurred
+    /// during initialization.
+    ///
+    /// # Errors
+    ///
+    /// This function may return an error if:
+    ///
+    /// - The `topic` string contains a null character.
+    /// - The initialization of the underlying `rcl_client_t` instance fails.
     pub(crate) fn new(node_handle: Arc<NodeHandle>, topic: &str) -> Result<Self, RclrsError>
     // This uses pub(crate) visibility to avoid instantiating this struct outside
     // [`Node::create_client`], see the struct's documentation for the rationale
@@ -279,10 +311,24 @@ impl<T> ClientBase for Client<T>
 where
     T: rosidl_runtime_rs::Service,
 {
+    /// Returns a reference to the `ClientHandle` associated with this client.
     fn handle(&self) -> &ClientHandle {
         &self.handle
     }
-
+    /// Executes the client by taking a response from the underlying `rcl_client_t` instance
+    /// and handling it appropriately.
+    ///
+    /// This method attempts to take a response from the client using `take_response`. If a response
+    /// is successfully taken, it is processed by either calling the associated callback or sending
+    /// the response to the associated future.
+    ///
+    /// If the `take_response` method returns a `ClientTakeFailed` error, it is treated as a
+    /// spurious wakeup and is not considered an error.
+    ///
+    /// # Returns
+    ///
+    /// `Ok(())` if the response was successfully processed or if a spurious wakeup occurred.
+    /// `Err(RclrsError)` if an error occurred while taking or processing the response.
     fn execute(&self) -> Result<(), RclrsError> {
         let (res, req_id) = match self.take_response() {
             Ok((res, req_id)) => (res, req_id),

rclrs/src/clock.rs

@@ -15,6 +15,7 @@ pub enum ClockType {
 }
 
 impl From<ClockType> for rcl_clock_type_t {
+    /// Converts a `ClockType` enum variant to the corresponding `rcl_clock_type_t` variant.
     fn from(clock_type: ClockType) -> Self {
         match clock_type {
             ClockType::RosTime => rcl_clock_type_t::RCL_ROS_TIME,
@@ -45,19 +46,37 @@ impl Clock {
     }
 
     /// Creates a new Clock with `ClockType::SteadyTime`
+    /// to update it
+    ///
+    /// # Returns
+    ///
+    /// A tuple of Clock and ClockSource
     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
+    ///
+    /// # Returns
+    ///
+    /// A tuple of Clock and ClockSource
     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`.
+    ///
+    /// # Arguments
+    ///
+    /// * `kind` - The `ClockType` to use for the new `Clock` instance.
+    ///
+    /// # Returns
+    ///
+    /// A tuple containing the new `Clock` instance and an `Option<ClockSource>`. If the `ClockType` is `RosTime`,
+    /// the `ClockSource` will be `Some`, otherwise it will be `None`.
     pub fn new(kind: ClockType) -> (Self, Option<ClockSource>) {
         let clock = Self::make(kind);
         let clock_source =

rclrs/src/subscription.rs

@@ -36,6 +36,13 @@ pub struct SubscriptionHandle {
 }
 
 impl SubscriptionHandle {
+    /// Acquires the lock on the `rcl_subscription_t` and returns a mutable reference to it.
+    ///
+    /// # Safety
+    ///
+    /// This function is marked as unsafe because it returns a mutable reference to the
+    /// `rcl_subscription_t` struct, which could potentially be misused to violate safety
+    /// guarantees.
     pub(crate) fn lock(&self) -> MutexGuard<rcl_subscription_t> {
         self.rcl_subscription.lock().unwrap()
     }

rosidl_runtime_rs/src/sequence.rs

@@ -92,6 +92,16 @@ pub struct SequenceIterator<T: SequenceAlloc> {
 // ========================= impl for Sequence =========================
 
 impl<T: SequenceAlloc> Clone for Sequence<T> {
+    /// Creates a deep copy of the sequence.
+    ///
+    /// # Panics
+    ///
+    /// This function will panic if the cloning operation fails, as indicated by the
+    /// `sequence_copy` function returning `false`.
+    ///
+    /// # Returns
+    ///
+    /// A new `Sequence` instance that is a deep copy of the original sequence.
     fn clone(&self) -> Self {
         let mut seq = Self::default();
         if T::sequence_copy(self, &mut seq) {
@@ -140,6 +150,22 @@ impl<T: SequenceAlloc> Drop for Sequence<T> {
 impl<T: SequenceAlloc + Eq> Eq for Sequence<T> {}
 
 impl<T: SequenceAlloc> Extend<T> for Sequence<T> {
+    /// Extends the sequence by appending elements from an iterator.
+    ///
+    /// # Arguments
+    ///
+    /// * `&mut self` - The mutable sequence to be extended.
+    /// * `iter` - The iterator containing the elements to be appended.
+    ///
+    /// # Panics
+    ///
+    /// This function does not panic.
+    ///
+    /// # Safety
+    ///
+    /// This function is safe to call, but it may reallocate the underlying memory
+    /// of the sequence, which could potentially invalidate any existing references
+    /// or pointers to the sequence's elements.
     fn extend<I>(&mut self, iter: I)
     where
         I: IntoIterator<Item = T>,
@@ -183,6 +209,28 @@ impl<T: SequenceAlloc> Extend<T> for Sequence<T> {
 }
 
 impl<T: SequenceAlloc + Clone> From<&[T]> for Sequence<T> {
+    /// Creates a new `Sequence` from a slice of elements.
+    ///
+    /// # Arguments
+    ///
+    /// * `slice` - A slice of elements to be used to initialize the new `Sequence`.
+    ///
+    /// # Returns
+    ///
+    /// A new `Sequence` instance containing a copy of the elements from the input slice.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use rosidl_runtime_rs::Sequence;
+    ///
+    /// let slice:Vec<i32> = vec![1, 2, 3, 4, 5];
+    /// let seq: Sequence<i32> = (&slice[..]).into();
+    ///
+    /// assert_eq!(seq.len(), 5);
+    /// assert_eq!(seq[0], 1);
+    /// assert_eq!(seq[4], 5);
+    /// ```
     fn from(slice: &[T]) -> Self {
         let mut seq = Sequence::new(slice.len());
         seq.clone_from_slice(slice);
@@ -471,14 +519,14 @@ impl<T: SequenceAlloc> Iterator for SequenceIterator<T> {
         self.idx += 1;
         Some(elem)
     }
-
+    /// returns the remaining length of the sequence
     fn size_hint(&self) -> (usize, Option<usize>) {
         let len = (self.seq.size + 1) - self.idx;
         (len, Some(len))
     }
 }
-
 impl<T: SequenceAlloc> ExactSizeIterator for SequenceIterator<T> {
+    /// Returns the length of the sequence as unsigned integer.
     fn len(&self) -> usize {
         (self.seq.size + 1) - self.idx
     }
@@ -497,11 +545,40 @@ impl Display for SequenceExceedsBoundsError {
         )
     }
 }
-
+/// Implements the `std::error::Error` trait for the `SequenceExceedsBoundsError` struct.
 impl std::error::Error for SequenceExceedsBoundsError {}
-
+/// Macro to provide default implementations for the `SequenceAlloc` trait for primitive types.
+///
+/// This macro generates an implementation of the `SequenceAlloc` trait for a given primitive type.
+/// It defines three extern "C" functions (`$init_func`, `$fini_func`, and `$copy_func`) that are
+/// linked from the "rosidl_runtime_c" library and used to initialize, finalize, and copy sequences
+/// of the specified type.
+///
+/// The `sequence_init` function allocates space for the sequence and sets its size and capacity.
+/// If the sequence's data pointer is null, it calls `$init_func` to allocate memory. Otherwise,
+/// it writes zero bytes to the existing memory region.
+///
+/// The `sequence_fini` function finalizes the sequence by calling `$fini_func`.
+///
+/// The `sequence_copy` function copies the contents of one sequence to another by calling
+/// `$copy_func`.
+///
+/// # Safety
+///
+/// The implementations of `sequence_init`, `sequence_fini`, and `sequence_copy` are marked as
+/// `unsafe` because they call the corresponding extern "C" functions, which may have undefined
+/// behavior if their preconditions are not met. However, the macro assumes that there are no
+/// special preconditions for these functions.
+///
+/// # Arguments
+///
+/// * `$rust_type:ty` - The Rust type for which the `SequenceAlloc` trait should be implemented.
+/// * `$init_func:ident` - The name of the extern "C" function used to initialize a sequence.
+/// * `$fini_func:ident` - The name of the extern "C" function used to finalize a sequence.
+/// * `$copy_func:ident` - The name of the extern "C" function used to copy a sequence.
 macro_rules! impl_sequence_alloc_for_primitive_type {
     ($rust_type:ty, $init_func:ident, $fini_func:ident, $copy_func:ident) => {
+        // Provides default implementations for SequenceAlloc for primitive types.
         #[link(name = "rosidl_runtime_c")]
         extern "C" {
             fn $init_func(seq: *mut Sequence<$rust_type>, size: usize) -> bool;
@@ -513,6 +590,21 @@ macro_rules! impl_sequence_alloc_for_primitive_type {
         }
 
         impl SequenceAlloc for $rust_type {
+            /// Initializes a sequence with a given size.
+            ///
+            /// # Safety
+            ///
+            /// This function is unsafe because it calls an unsafe function `$init_func`.
+            /// The caller must ensure that `$init_func` is safe to call with the provided arguments.
+            ///
+            /// # Arguments
+            ///
+            /// * `seq` - A mutable reference to the sequence to be initialized.
+            /// * `size` - The size of the sequence to be initialized.
+            ///
+            /// # Returns
+            ///
+            /// `true` if the sequence was successfully initialized, `false` otherwise.
             fn sequence_init(seq: &mut Sequence<Self>, size: usize) -> bool {
                 // SAFETY: There are no special preconditions to the sequence_init function.
                 unsafe {
@@ -525,10 +617,37 @@ macro_rules! impl_sequence_alloc_for_primitive_type {
                     ret
                 }
             }
+
+            /// Finalizes a sequence, freeing any associated resources.
+            ///
+            /// # Safety
+            ///
+            /// This function is unsafe because it calls an unsafe function `$fini_func`.
+            /// The caller must ensure that `$fini_func` is safe to call with the provided arguments.
+            ///
+            /// # Arguments
+            ///
+            /// * `seq` - A mutable reference to the sequence to be finalized.
             fn sequence_fini(seq: &mut Sequence<Self>) {
                 // SAFETY: There are no special preconditions to the sequence_fini function.
                 unsafe { $fini_func(seq as *mut _) }
             }
+
+            /// Copies the contents of one sequence to another.
+            ///
+            /// # Safety
+            ///
+            /// This function is unsafe because it calls an unsafe function `$copy_func`.
+            /// The caller must ensure that `$copy_func` is safe to call with the provided arguments.
+            ///
+            /// # Arguments
+            ///
+            /// * `in_seq` - A reference to the sequence to be copied from.
+            /// * `out_seq` - A mutable reference to the sequence to be copied to.
+            ///
+            /// # Returns
+            ///
+            /// `true` if the copy was successful, `false` otherwise.
             fn sequence_copy(in_seq: &Sequence<Self>, out_seq: &mut Sequence<Self>) -> bool {
                 // SAFETY: There are no special preconditions to the sequence_copy function.
                 unsafe { $copy_func(in_seq as *const _, out_seq as *mut _) }