Merge pull request #219 from RosLibRust/cleanups-from-re-org-v2

More documentation improvements

README.md

@@ -50,9 +50,9 @@ async fn main() -> roslibrust::Result<()> {
     relay(ros).await?;
     }
 
+    // Relay messages over a zenoh connection compatible with zenoh-ros1-plugin / zenoh-ros1-bridge
     #[cfg(feature = "zenoh")]
     {
-    // Relay messages over a zenoh connection compatible with zenoh-ros1-plugin / zenoh-ros1-bridge
     let ros = roslibrust::zenoh::ZenohClient::new(zenoh::open(zenoh::Config::default()).await.unwrap());
     relay(ros).await?;
     }

roslibrust_codegen/src/lib.rs

@@ -1,3 +1,13 @@
+//! A library for generating rust type definitions from ROS IDL files
+//! Supports both ROS1 and ROS2.
+//! Generated types implement roslibrust's MessageType and ServiceType traits making them compatible with all roslibrust backends.
+//!
+//! This library is a pure rust implementation from scratch and requires no ROS installation.
+//!
+//! See [example_package](https://github.com/RosLibRust/roslibrust/tree/master/example_package) for how best to integrate this crate with build.rs
+//!
+//! Directly depending on this crate is not recommended. Instead access it via roslibrust with the `codegen` feature enabled.
+
 use log::*;
 use proc_macro2::TokenStream;
 use quote::quote;

roslibrust_common/Cargo.toml

@@ -13,10 +13,3 @@ serde = { workspace = true }
 md5 = "0.7"
 # Used for async trait definitions
 futures = "0.3"
-
-# THESE DEPENDENCIES WILL BE REMOVED
-# We're currently leaking these error types in the "generic error type"
-# We'll clean this up
-tokio = { workspace = true }
-tokio-tungstenite = { version = "0.17" }
-serde_json = "1.0"

roslibrust_common/src/lib.rs

@@ -1,37 +1,49 @@
 //! # roslibrust_common
 //! This crate provides common types and traits used throughout the roslibrust ecosystem.
 
-/// For now starting with a central error type, may break this up more in future
+/// The central error type used throughout roslibrust.
+///
+/// For now all roslibrust backends must coerce their errors into this type.
+/// We may in the future allow backends to define their own error types, for now this is a compromise.
+///
+/// Additionally, this error type is returned from all roslibrust function calls so failure types must be relatively generic.
 #[derive(thiserror::Error, Debug)]
 pub enum Error {
-    #[error("Not currently connected to ros master / bridge")]
+    /// Is returned when communication is fully lost.
+    /// While this error is being returned messages should be assumed to be being lost.
+    /// Backends are expected to be "self-healing" and when connection is restored existing Publishers, Subscribers, etc.
+    /// should resume functionality without needing to be recreated.
+    #[error("No connection to ROS backend")]
     Disconnected,
-    // TODO we probably want to eliminate tungstenite from this and hide our
-    // underlying websocket implementation from the API
-    // currently we "technically" break the API when we change tungstenite verisons
-    #[error("Websocket communication error: {0}")]
-    CommFailure(#[from] tokio_tungstenite::tungstenite::Error),
+    /// Some backends aren't able to conclusively determine if an operation has failed.
+    /// Timeout will be returned if an operation takes a unexpectedly long time.
+    /// For the `rosbridge` backend where this is most frequently encountered the timeout is configurable on the client.
     #[error("Operation timed out: {0}")]
-    Timeout(#[from] tokio::time::error::Elapsed),
-    #[error("Failed to parse message from JSON: {0}")]
-    InvalidMessage(#[from] serde_json::Error),
-    #[error("TCPROS serialization error: {0}")]
+    Timeout(String),
+    /// When a message is received but the backend is unable to serialize/deserialize it to the Rust type representing the message type.
+    ///
+    /// This error is also returned in the event of an md5sum mismatch.
+    #[error("Serialization error: {0}")]
     SerializationError(String),
+    /// When the backend "server" reports an error this type is returned.
+    ///
+    /// This can happen when there are internal issues on the rosbridge_server, or with xmlrpc communication with the ros1 master.
     #[error("Rosbridge server reported an error: {0}")]
     ServerError(String),
+    /// Returned when there is a fundamental networking error.
+    ///
+    /// Typically reserved for situations when ports are unavailable, dns lookups fail, etc.
     #[error("IO error: {0}")]
     IoError(#[from] std::io::Error),
+    /// When a topic name is used that isn't a valid topic name.
     #[error("Name does not meet ROS requirements: {0}")]
     InvalidName(String),
-    // Generic catch-all error type for not-yet-handled errors
-    // TODO ultimately this type will be removed from API of library
+    /// Backends are free to return this error if they encounter any error that doesn't cleanly fit in the other categories.
     #[error(transparent)]
     Unexpected(#[from] anyhow::Error),
 }
 
-/// Generic result type used as standard throughout library.
-/// Note: many functions which currently return this will be updated to provide specific error
-/// types in the future instead of the generic error here.
+/// Generic result type used throughout roslibrust.
 pub type Result<T> = std::result::Result<T, Error>;
 
 /// Fundamental traits for message types this crate works with

roslibrust_rosbridge/src/client.rs

@@ -364,7 +364,9 @@ impl ClientHandle {
 
         // Having to do manual timeout logic here because of error types
         let recv = if let Some(timeout) = client.opts.timeout {
-            tokio::time::timeout(timeout, rx).await?
+            tokio::time::timeout(timeout, rx)
+                .await
+                .map_err(|e| Error::Timeout(format!("Service call timed out: {e:?}")))?
         } else {
             rx.await
         };
@@ -386,8 +388,8 @@ impl ClientHandle {
                 match serde_json::from_value(msg) {
                     Ok(s) => return Err(Error::ServerError(s)),
                     Err(_) => {
-                        // Return the error from the origional parse
-                        return Err(Error::InvalidMessage(e));
+                        // Return the error from the original parse
+                        return Err(Error::SerializationError(e.to_string()));
                     }
                 }
             }
@@ -677,7 +679,10 @@ impl Client {
             match stream.next().await {
                 Some(Ok(msg)) => msg,
                 Some(Err(e)) => {
-                    return Err(e.into());
+                    return Err(Error::IoError(std::io::Error::new(
+                        std::io::ErrorKind::Other,
+                        e,
+                    )));
                 }
                 None => {
                     return Err(Error::Unexpected(anyhow!("Wtf does none mean here?")));
@@ -781,7 +786,9 @@ where
     F: futures::Future<Output = Result<T>>,
 {
     if let Some(t) = timeout {
-        tokio::time::timeout(t, future).await?
+        tokio::time::timeout(t, future)
+            .await
+            .map_err(|e| Error::Timeout(format!("{e:?}")))?
     } else {
         future.await
     }
@@ -811,6 +818,9 @@ async fn connect(url: &str) -> Result<Socket> {
     let attempt = tokio_tungstenite::connect_async(url).await;
     match attempt {
         Ok((stream, _response)) => Ok(stream),
-        Err(e) => Err(e.into()),
+        Err(e) => Err(Error::IoError(std::io::Error::new(
+            std::io::ErrorKind::Other,
+            e,
+        ))),
     }
 }

roslibrust_rosbridge/src/comm.rs

@@ -1,8 +1,9 @@
+use crate::MapError;
 use crate::Writer;
 use anyhow::bail;
 use futures_util::SinkExt;
 use log::debug;
-use roslibrust_common::{Result, RosMessageType};
+use roslibrust_common::{Error, Result, RosMessageType};
 use serde_json::json;
 use std::{fmt::Display, str::FromStr, string::ToString};
 use tokio_tungstenite::tungstenite::Message;
@@ -122,7 +123,7 @@ impl RosBridgeComm for Writer {
         );
         let msg = Message::Text(msg.to_string());
         debug!("Sending subscribe: {:?}", &msg);
-        self.send(msg).await?;
+        self.send(msg).await.map_to_roslibrust()?;
         Ok(())
     }
 
@@ -135,7 +136,7 @@ impl RosBridgeComm for Writer {
         );
         let msg = Message::Text(msg.to_string());
         debug!("Sending unsubscribe: {:?}", &msg);
-        self.send(msg).await?;
+        self.send(msg).await.map_to_roslibrust()?;
         Ok(())
     }
 
@@ -150,7 +151,7 @@ impl RosBridgeComm for Writer {
         );
         let msg = Message::Text(msg.to_string());
         debug!("Sending publish: {:?}", &msg);
-        self.send(msg).await?;
+        self.send(msg).await.map_to_roslibrust()?;
         Ok(())
     }
 
@@ -171,7 +172,7 @@ impl RosBridgeComm for Writer {
         );
         let msg = Message::Text(msg.to_string());
         debug!("Sending advertise: {:?}", &msg);
-        self.send(msg).await?;
+        self.send(msg).await.map_to_roslibrust()?;
         Ok(())
     }
 
@@ -191,7 +192,7 @@ impl RosBridgeComm for Writer {
         );
         let msg = Message::Text(msg.to_string());
         debug!("Sending call_service: {:?}", &msg);
-        self.send(msg).await?;
+        self.send(msg).await.map_to_roslibrust()?;
         Ok(())
     }
 
@@ -205,7 +206,7 @@ impl RosBridgeComm for Writer {
         };
         let msg = Message::Text(msg.to_string());
         debug!("Sending unadvertise: {:?}", &msg);
-        self.send(msg).await?;
+        self.send(msg).await.map_to_roslibrust()?;
         Ok(())
     }
 
@@ -219,7 +220,7 @@ impl RosBridgeComm for Writer {
             }
         };
         let msg = Message::Text(msg.to_string());
-        self.send(msg).await?;
+        self.send(msg).await.map_to_roslibrust()?;
         Ok(())
     }
 
@@ -232,7 +233,9 @@ impl RosBridgeComm for Writer {
             }
         };
         let msg = Message::Text(msg.to_string());
-        self.send(msg).await?;
+        self.send(msg)
+            .await
+            .map_err(|e| Error::IoError(std::io::Error::new(std::io::ErrorKind::Other, e)))?;
         Ok(())
     }
 
@@ -258,7 +261,7 @@ impl RosBridgeComm for Writer {
         };
         let msg = Message::Text(msg.to_string());
         debug!("Sending service_response: {:?}", &msg);
-        self.send(msg).await?;
+        self.send(msg).await.map_to_roslibrust()?;
         Ok(())
     }
 }

roslibrust_rosbridge/src/lib.rs

@@ -221,6 +221,27 @@ impl<T: RosMessageType> Publish<T> for crate::Publisher<T> {
     }
 }
 
+/// Defines a conversion from a tungstenite error to a roslibrust error
+/// Since neither error type is owned by this crate we use a trait to define the conversion
+trait MapError {
+    type T;
+    fn map_to_roslibrust(self) -> Result<Self::T>;
+}
+
+// Implementation of conversion from tungstenite error to roslibrust error for a result
+impl<T: Send + Sync> MapError for std::result::Result<T, tokio_tungstenite::tungstenite::Error> {
+    type T = T;
+    fn map_to_roslibrust(self) -> Result<T> {
+        match self {
+            Ok(t) => Ok(t),
+            Err(e) => Err(Error::IoError(std::io::Error::new(
+                std::io::ErrorKind::Other,
+                e,
+            ))),
+        }
+    }
+}
+
 #[cfg(test)]
 mod test {
     use roslibrust_common::*;

roslibrust_zenoh/src/lib.rs

@@ -1,10 +1,13 @@
 //! A crate for interfacing to ROS1 via the [zenoh-ros1-plugin / zenoh-ros1-bridge](https://github.com/eclipse-zenoh/zenoh-plugin-ros1).
-
+//!
+//! It is not recommended to depend on this crate directly, but instead access it via [roslibrust](https://docs.rs/roslibrust/latest/roslibrust/) with the `zenoh` feature enabled.
 use roslibrust_common::*;
 
 use log::*;
 use zenoh::bytes::ZBytes;
 
+/// A wrapper around a normal zenoh session that adds roslibrust specific functionality.
+/// Should be created via [ZenohClient::new], and then used via the [TopicProvider] and [ServiceProvider] traits.
 pub struct ZenohClient {
     session: zenoh::Session,
 }
@@ -16,6 +19,8 @@ impl ZenohClient {
     }
 }
 
+/// The publisher type returned by [TopicProvider::advertise] on [ZenohClient]
+/// This type is self de-registering, and dropping the publisher will automatically un-advertise the topic.
 pub struct ZenohPublisher<T> {
     publisher: zenoh::pubsub::Publisher<'static>,
     _marker: std::marker::PhantomData<T>,
@@ -39,6 +44,11 @@ impl<T: RosMessageType> Publish<T> for ZenohPublisher<T> {
 // Using type alias here, I have no idea why zenoh has this type so deep
 type ZenohSubInner =
     zenoh::pubsub::Subscriber<zenoh::handlers::FifoChannelHandler<zenoh::sample::Sample>>;
+
+/// The subscriber type returned by [TopicProvider::subscribe] on [ZenohClient].
+/// This type is self de-registering, and dropping the subscriber will automatically unsubscribe from the topic.
+/// This type is generic on the message type that will be received.
+/// It is typically used with types generated by roslibrust's codegen.
 pub struct ZenohSubscriber<T> {
     subscriber: ZenohSubInner,
     _marker: std::marker::PhantomData<T>,
@@ -135,6 +145,8 @@ fn mangle_service(service: &str, type_str: &str, md5sum: &str) -> (String, Strin
     )
 }
 
+/// The client type returned by [ServiceProvider::service_client] on [ZenohClient]
+/// This type allows calling a service multiple times without re-negotiating the connection each time.
 pub struct ZenohServiceClient<T: RosServiceType> {
     session: zenoh::Session,
     zenoh_query: String,
@@ -194,8 +206,8 @@ impl<T: RosServiceType> Service<T> for ZenohServiceClient<T> {
     }
 }
 
-/// The "holder type" returned when advertising a service
-/// Dropping this will stop the service server
+/// The type returned by [ServiceProvider::advertise_service] on [ZenohClient].
+/// This type is self de-registering, and dropping the server will automatically un-advertise the service.
 pub struct ZenohServiceServer {
     // Dropping this will stop zenoh's declaration of the queryable
     _queryable: zenoh::query::Queryable<()>,