From 4bc0b893c3a80cb5497bd89d397e87e75f08742f Mon Sep 17 00:00:00 2001 From: carter Date: Tue, 7 Jan 2025 13:09:11 -0700 Subject: [PATCH 1/2] More documentation improvements --- README.md | 2 +- roslibrust_codegen/src/lib.rs | 10 ++++++++++ roslibrust_zenoh/src/lib.rs | 18 +++++++++++++++--- 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index ec834a1..3069b3b 100644 --- a/README.md +++ b/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?; } diff --git a/roslibrust_codegen/src/lib.rs b/roslibrust_codegen/src/lib.rs index eda3639..7baa421 100644 --- a/roslibrust_codegen/src/lib.rs +++ b/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; diff --git a/roslibrust_zenoh/src/lib.rs b/roslibrust_zenoh/src/lib.rs index da5a0d3..4607220 100644 --- a/roslibrust_zenoh/src/lib.rs +++ b/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 { publisher: zenoh::pubsub::Publisher<'static>, _marker: std::marker::PhantomData, @@ -39,6 +44,11 @@ impl Publish for ZenohPublisher { // Using type alias here, I have no idea why zenoh has this type so deep type ZenohSubInner = zenoh::pubsub::Subscriber>; + +/// 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 { subscriber: ZenohSubInner, _marker: std::marker::PhantomData, @@ -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 { session: zenoh::Session, zenoh_query: String, @@ -194,8 +206,8 @@ impl Service for ZenohServiceClient { } } -/// 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<()>, From 11e4bf246ea9f8da1f854cff420c5cc10a8cbecc Mon Sep 17 00:00:00 2001 From: carter Date: Tue, 7 Jan 2025 13:46:03 -0700 Subject: [PATCH 2/2] Fix common error type to not leak specifics --- Cargo.lock | 3 -- roslibrust_common/Cargo.toml | 7 ----- roslibrust_common/src/lib.rs | 44 +++++++++++++++++++----------- roslibrust_rosbridge/src/client.rs | 22 +++++++++++---- roslibrust_rosbridge/src/comm.rs | 23 +++++++++------- roslibrust_rosbridge/src/lib.rs | 21 ++++++++++++++ 6 files changed, 78 insertions(+), 42 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 263a01d..c978ced 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2610,10 +2610,7 @@ dependencies = [ "futures", "md5", "serde", - "serde_json", "thiserror 2.0.6", - "tokio", - "tokio-tungstenite 0.17.2", ] [[package]] diff --git a/roslibrust_common/Cargo.toml b/roslibrust_common/Cargo.toml index f2aed08..78db222 100644 --- a/roslibrust_common/Cargo.toml +++ b/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" diff --git a/roslibrust_common/src/lib.rs b/roslibrust_common/src/lib.rs index c7aefd2..8b8d2f9 100644 --- a/roslibrust_common/src/lib.rs +++ b/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 = std::result::Result; /// Fundamental traits for message types this crate works with diff --git a/roslibrust_rosbridge/src/client.rs b/roslibrust_rosbridge/src/client.rs index 308675a..e25a32f 100644 --- a/roslibrust_rosbridge/src/client.rs +++ b/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>, { 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 { 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, + ))), } } diff --git a/roslibrust_rosbridge/src/comm.rs b/roslibrust_rosbridge/src/comm.rs index 6792521..53763fb 100644 --- a/roslibrust_rosbridge/src/comm.rs +++ b/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(()) } } diff --git a/roslibrust_rosbridge/src/lib.rs b/roslibrust_rosbridge/src/lib.rs index a23450c..6dda9f4 100644 --- a/roslibrust_rosbridge/src/lib.rs +++ b/roslibrust_rosbridge/src/lib.rs @@ -221,6 +221,27 @@ impl Publish for crate::Publisher { } } +/// 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; +} + +// Implementation of conversion from tungstenite error to roslibrust error for a result +impl MapError for std::result::Result { + type T = T; + fn map_to_roslibrust(self) -> Result { + 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::*;