From 28df7f603f878c76ce592b1778a7d256b9c449ba Mon Sep 17 00:00:00 2001 From: Shane Snover Date: Sat, 16 Dec 2023 13:41:49 -0700 Subject: [PATCH 1/3] Reorganize the code in node.rs into a couple separate files with different areas of concern to manage growing number of node actor methods --- CHANGELOG.md | 9 +- roslibrust/src/ros1/mod.rs | 4 - .../src/ros1/{node.rs => node/actor.rs} | 145 ++---------------- roslibrust/src/ros1/node/handle.rs | 60 ++++++++ roslibrust/src/ros1/node/mod.rs | 70 +++++++++ .../ros1/{xmlrpc_server.rs => node/xmlrpc.rs} | 2 +- 6 files changed, 148 insertions(+), 142 deletions(-) rename roslibrust/src/ros1/{node.rs => node/actor.rs} (77%) create mode 100644 roslibrust/src/ros1/node/handle.rs create mode 100644 roslibrust/src/ros1/node/mod.rs rename roslibrust/src/ros1/{xmlrpc_server.rs => node/xmlrpc.rs} (99%) diff --git a/CHANGELOG.md b/CHANGELOG.md index d291403d..6a5d1dfe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,16 +25,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - The build.rs example in example_package now correctly informs cargo of filesystem dependencies -- The `advertise_serveice` method in `rosbridge/client.rs` now accepts closures +- The `advertise_service` method in `rosbridge/client.rs` now accepts closures ### Fixed ### Changed - - Removed `find_and_generate_ros_messages_relative_to_manifest_dir!` this proc_macro was changing the current working directory of the compilation job resulting in a variety of strange compilation behaviors. Build.rs scripts are recommended for use cases requiring fine - grained control of message generation. - - The function interface for top level generation functions in `roslibrust_codegen` have been changed to include the list of dependent -filesystem paths that should trigger re-running code generation. Note: new files added to the search paths will not be automatically detected. + - Removed `find_and_generate_ros_messages_relative_to_manifest_dir!` this proc_macro was changing the current working directory of the compilation job resulting in a variety of strange compilation behaviors. Build.rs scripts are recommended for use cases requiring fine grained control of message generation. + - The function interface for top level generation functions in `roslibrust_codegen` have been changed to include the list of dependent filesystem paths that should trigger re-running code generation. Note: new files added to the search paths will not be automatically detected. + - Refactor the `ros1::node` module into separate smaller pieces. This should be invisible externally (and no changes to examples were required). ## 0.8.0 - October 4th, 2023 diff --git a/roslibrust/src/ros1/mod.rs b/roslibrust/src/ros1/mod.rs index 3c15c71d..f8c2e514 100644 --- a/roslibrust/src/ros1/mod.rs +++ b/roslibrust/src/ros1/mod.rs @@ -4,10 +4,6 @@ mod master_client; pub use master_client::*; -/// [xmlrpc_server] module contains the xmlrpc server that a node must host -mod xmlrpc_server; -pub(crate) use xmlrpc_server::*; - mod names; /// [node] module contains the central Node and NodeHandle APIs diff --git a/roslibrust/src/ros1/node.rs b/roslibrust/src/ros1/node/actor.rs similarity index 77% rename from roslibrust/src/ros1/node.rs rename to roslibrust/src/ros1/node/actor.rs index 45709724..47276bfa 100644 --- a/roslibrust/src/ros1/node.rs +++ b/roslibrust/src/ros1/node/actor.rs @@ -1,28 +1,18 @@ -//! This module contains the top level Node and NodeHandle classes. -//! These wrap the lower level management of a ROS Node connection into a higher level and thread safe API. - -use super::{ - names::Name, - publisher::{Publication, Publisher}, - subscriber::{Subscriber, Subscription}, +use super::ProtocolParams; +use crate::{ + ros1::{ + names::Name, + node::{XmlRpcServer, XmlRpcServerHandle}, + publisher::Publication, + subscriber::Subscription, + }, + MasterClient, ServiceCallback, }; -use crate::{MasterClient, RosMasterError, ServiceCallback, XmlRpcServer, XmlRpcServerHandle}; use abort_on_drop::ChildTask; use roslibrust_codegen::RosMessageType; -use std::{ - collections::HashMap, - net::{IpAddr, Ipv4Addr}, - sync::Arc, -}; +use std::{collections::HashMap, net::Ipv4Addr, sync::Arc}; use tokio::sync::{broadcast, mpsc, oneshot}; -#[derive(Debug)] -pub struct ProtocolParams { - pub hostname: String, - pub protocol: String, - pub port: u16, -} - #[derive(Debug)] pub enum NodeMsg { GetMasterUri { @@ -68,7 +58,7 @@ pub enum NodeMsg { #[derive(Clone)] pub(crate) struct NodeServerHandle { - node_server_sender: mpsc::UnboundedSender, + pub(crate) node_server_sender: mpsc::UnboundedSender, // If this handle should keep the underlying node task alive it will hold an // Arc to the underlying node task. This is an option because internal handles // within the node shouldn't keep it alive (e.g. what we hand to xml server) @@ -228,7 +218,7 @@ impl NodeServerHandle { /// Represents a single "real" node, typically only one of these is expected per process /// but nothing should specifically prevent that. -pub struct Node { +pub(crate) struct Node { // The xmlrpc client this node uses to make requests to master client: MasterClient, // Server which handles updates from the rosmaster and other ROS nodes @@ -248,7 +238,7 @@ pub struct Node { } impl Node { - async fn new( + pub(crate) async fn new( master_uri: &str, hostname: &str, node_name: &str, @@ -510,112 +500,3 @@ impl Node { } } } - -/// Represents a handle to an underlying [Node]. NodeHandle's can be freely cloned, moved, copied, etc. -/// This class provides the user facing API for interacting with ROS. -#[derive(Clone)] -pub struct NodeHandle { - inner: NodeServerHandle, -} - -impl NodeHandle { - // TODO builder, result, better error type - /// Creates a new node connect and returns a handle to it - /// It is idiomatic to call this once per process and treat the created node as singleton. - /// The returned handle can be freely clone'd to create additional handles without creating additional connections. - pub async fn new( - master_uri: &str, - name: &str, - ) -> Result> { - // Follow ROS rules and determine our IP and hostname - let (addr, hostname) = determine_addr().await?; - - let node = Node::new(master_uri, &hostname, name, addr).await?; - let nh = NodeHandle { inner: node }; - - Ok(nh) - } - - pub fn is_ok(&self) -> bool { - !self.inner.node_server_sender.is_closed() - } - - pub async fn get_client_uri(&self) -> Result> { - self.inner.get_client_uri().await - } - - pub async fn advertise( - &self, - topic_name: &str, - queue_size: usize, - ) -> Result, Box> { - let sender = self - .inner - .register_publisher::(topic_name, T::ROS_TYPE_NAME, queue_size) - .await?; - Ok(Publisher::new(topic_name, sender)) - } - - pub async fn subscribe( - &self, - topic_name: &str, - queue_size: usize, - ) -> Result, Box> { - let receiver = self - .inner - .register_subscriber::(topic_name, queue_size) - .await?; - Ok(Subscriber::new(receiver)) - } -} - -// TODO at the end of the day I'd like to offer a builder pattern for configuration that allow manual setting of this or "ros idiomatic" behavior - Carter -/// Following ROS's idiomatic address rules uses ROS_HOSTNAME and ROS_IP to determine the address that server should be hosted at. -/// Returns both the resolved IpAddress of the host (used for actually opening the socket), and the String "hostname" which should -/// be used in the URI. -async fn determine_addr() -> Result<(Ipv4Addr, String), RosMasterError> { - // If ROS_IP is set that trumps anything else - if let Ok(ip_str) = std::env::var("ROS_IP") { - let ip = ip_str.parse().map_err(|e| { - RosMasterError::HostIpResolutionFailure(format!( - "ROS_IP environment variable did not parse to a valid IpAddr::V4: {e:?}" - )) - })?; - return Ok((ip, ip_str)); - } - // If ROS_HOSTNAME is set that is next highest precedent - if let Ok(name) = std::env::var("ROS_HOSTNAME") { - let ip = hostname_to_ipv4(&name).await?; - return Ok((ip, name)); - } - // If neither env var is set, use the computers "hostname" - let name = gethostname::gethostname(); - let name = name.into_string().map_err(|e| { - RosMasterError::HostIpResolutionFailure(format!("This host's hostname is a string that cannot be validly converted into a Rust type, and therefore we cannot convert it into an IpAddrv4: {e:?}")) - })?; - let ip = hostname_to_ipv4(&name).await?; - return Ok((ip, name)); -} - -/// Given a the name of a host use's std::net::ToSocketAddrs to perform a DNS lookup and return the resulting IP address. -/// This function is intended to be used to determine the correct IP host the socket for the xmlrpc server on. -async fn hostname_to_ipv4(name: &str) -> Result { - let name_with_port = &format!("{name}:0"); - let mut i = tokio::net::lookup_host(name_with_port).await.map_err(|e| { - RosMasterError::HostIpResolutionFailure(format!( - "Failure while attempting to lookup ROS_HOSTNAME: {e:?}" - )) - })?; - if let Some(addr) = i.next() { - match addr.ip() { - IpAddr::V4(ip) => Ok(ip), - IpAddr::V6(ip) => { - Err(RosMasterError::HostIpResolutionFailure(format!("ROS_HOSTNAME resolved to an IPv6 address which is not support by ROS/roslibrust: {ip:?}"))) - } - } - } else { - Err(RosMasterError::HostIpResolutionFailure(format!( - "ROS_HOSTNAME did not resolve any address: {name:?}" - ))) - } -} diff --git a/roslibrust/src/ros1/node/handle.rs b/roslibrust/src/ros1/node/handle.rs new file mode 100644 index 00000000..493ca3a4 --- /dev/null +++ b/roslibrust/src/ros1/node/handle.rs @@ -0,0 +1,60 @@ +use super::actor::{Node, NodeServerHandle}; +use crate::ros1::{publisher::Publisher, subscriber::Subscriber}; + +/// Represents a handle to an underlying [Node]. NodeHandle's can be freely cloned, moved, copied, etc. +/// This class provides the user facing API for interacting with ROS. +#[derive(Clone)] +pub struct NodeHandle { + inner: NodeServerHandle, +} + +impl NodeHandle { + // TODO builder, result, better error type + /// Creates a new node connect and returns a handle to it + /// It is idiomatic to call this once per process and treat the created node as singleton. + /// The returned handle can be freely clone'd to create additional handles without creating additional connections. + pub async fn new( + master_uri: &str, + name: &str, + ) -> Result> { + // Follow ROS rules and determine our IP and hostname + let (addr, hostname) = super::determine_addr().await?; + + let node = Node::new(master_uri, &hostname, name, addr).await?; + let nh = NodeHandle { inner: node }; + + Ok(nh) + } + + pub fn is_ok(&self) -> bool { + !self.inner.node_server_sender.is_closed() + } + + pub async fn get_client_uri(&self) -> Result> { + self.inner.get_client_uri().await + } + + pub async fn advertise( + &self, + topic_name: &str, + queue_size: usize, + ) -> Result, Box> { + let sender = self + .inner + .register_publisher::(topic_name, T::ROS_TYPE_NAME, queue_size) + .await?; + Ok(Publisher::new(topic_name, sender)) + } + + pub async fn subscribe( + &self, + topic_name: &str, + queue_size: usize, + ) -> Result, Box> { + let receiver = self + .inner + .register_subscriber::(topic_name, queue_size) + .await?; + Ok(Subscriber::new(receiver)) + } +} diff --git a/roslibrust/src/ros1/node/mod.rs b/roslibrust/src/ros1/node/mod.rs new file mode 100644 index 00000000..4fa4a1d3 --- /dev/null +++ b/roslibrust/src/ros1/node/mod.rs @@ -0,0 +1,70 @@ +//! This module contains the top level Node and NodeHandle classes. +//! These wrap the lower level management of a ROS Node connection into a higher level and thread safe API. + +use crate::RosMasterError; +use std::net::{IpAddr, Ipv4Addr}; + +mod actor; +mod handle; +mod xmlrpc; +use actor::*; +pub use handle::NodeHandle; +use xmlrpc::*; + +#[derive(Debug)] +pub struct ProtocolParams { + pub hostname: String, + pub protocol: String, + pub port: u16, +} + +// TODO at the end of the day I'd like to offer a builder pattern for configuration that allow manual setting of this or "ros idiomatic" behavior - Carter +/// Following ROS's idiomatic address rules uses ROS_HOSTNAME and ROS_IP to determine the address that server should be hosted at. +/// Returns both the resolved IpAddress of the host (used for actually opening the socket), and the String "hostname" which should +/// be used in the URI. +async fn determine_addr() -> Result<(Ipv4Addr, String), RosMasterError> { + // If ROS_IP is set that trumps anything else + if let Ok(ip_str) = std::env::var("ROS_IP") { + let ip = ip_str.parse().map_err(|e| { + RosMasterError::HostIpResolutionFailure(format!( + "ROS_IP environment variable did not parse to a valid IpAddr::V4: {e:?}" + )) + })?; + return Ok((ip, ip_str)); + } + // If ROS_HOSTNAME is set that is next highest precedent + if let Ok(name) = std::env::var("ROS_HOSTNAME") { + let ip = hostname_to_ipv4(&name).await?; + return Ok((ip, name)); + } + // If neither env var is set, use the computers "hostname" + let name = gethostname::gethostname(); + let name = name.into_string().map_err(|e| { + RosMasterError::HostIpResolutionFailure(format!("This host's hostname is a string that cannot be validly converted into a Rust type, and therefore we cannot convert it into an IpAddrv4: {e:?}")) + })?; + let ip = hostname_to_ipv4(&name).await?; + return Ok((ip, name)); +} + +/// Given a the name of a host use's std::net::ToSocketAddrs to perform a DNS lookup and return the resulting IP address. +/// This function is intended to be used to determine the correct IP host the socket for the xmlrpc server on. +async fn hostname_to_ipv4(name: &str) -> Result { + let name_with_port = &format!("{name}:0"); + let mut i = tokio::net::lookup_host(name_with_port).await.map_err(|e| { + RosMasterError::HostIpResolutionFailure(format!( + "Failure while attempting to lookup ROS_HOSTNAME: {e:?}" + )) + })?; + if let Some(addr) = i.next() { + match addr.ip() { + IpAddr::V4(ip) => Ok(ip), + IpAddr::V6(ip) => { + Err(RosMasterError::HostIpResolutionFailure(format!("ROS_HOSTNAME resolved to an IPv6 address which is not support by ROS/roslibrust: {ip:?}"))) + } + } + } else { + Err(RosMasterError::HostIpResolutionFailure(format!( + "ROS_HOSTNAME did not resolve any address: {name:?}" + ))) + } +} diff --git a/roslibrust/src/ros1/xmlrpc_server.rs b/roslibrust/src/ros1/node/xmlrpc.rs similarity index 99% rename from roslibrust/src/ros1/xmlrpc_server.rs rename to roslibrust/src/ros1/node/xmlrpc.rs index ea3a3701..58015494 100644 --- a/roslibrust/src/ros1/xmlrpc_server.rs +++ b/roslibrust/src/ros1/node/xmlrpc.rs @@ -1,4 +1,4 @@ -use super::node::NodeServerHandle; +use super::NodeServerHandle; use abort_on_drop::ChildTask; use hyper::{Body, Response, StatusCode}; use log::*; From 2f5701f26e7774df85cba3c4866ff83d8a440325 Mon Sep 17 00:00:00 2001 From: Shane Snover Date: Tue, 19 Dec 2023 19:33:06 -0700 Subject: [PATCH 2/3] Moving refactoring changes to this earlier MR --- roslibrust/examples/native_ros1.rs | 2 +- roslibrust/examples/ros1_listener.rs | 2 +- roslibrust/examples/ros1_talker.rs | 2 +- roslibrust/src/lib.rs | 33 ++++++++++++++++++++++--- roslibrust/src/ros1/mod.rs | 2 ++ roslibrust/src/ros1/names.rs | 15 ++++++++--- roslibrust/src/ros1/node/actor.rs | 13 +++------- roslibrust/src/ros1/node/handle.rs | 2 +- roslibrust/src/ros1/node/mod.rs | 2 +- roslibrust/src/ros1/publisher.rs | 4 +-- roslibrust/src/ros1/subscriber.rs | 2 +- roslibrust/src/rosbridge/mod.rs | 37 ++-------------------------- 12 files changed, 57 insertions(+), 59 deletions(-) diff --git a/roslibrust/examples/native_ros1.rs b/roslibrust/examples/native_ros1.rs index ddceef8b..5e4e3673 100644 --- a/roslibrust/examples/native_ros1.rs +++ b/roslibrust/examples/native_ros1.rs @@ -5,7 +5,7 @@ #[cfg(feature = "ros1")] #[tokio::main] async fn main() -> Result<(), Box> { - use roslibrust::NodeHandle; + use roslibrust::ros1::NodeHandle; simple_logger::SimpleLogger::new() .with_level(log::LevelFilter::Debug) diff --git a/roslibrust/examples/ros1_listener.rs b/roslibrust/examples/ros1_listener.rs index 3d94b3e5..cdbc6826 100644 --- a/roslibrust/examples/ros1_listener.rs +++ b/roslibrust/examples/ros1_listener.rs @@ -3,7 +3,7 @@ roslibrust_codegen_macro::find_and_generate_ros_messages!("assets/ros1_common_in #[cfg(feature = "ros1")] #[tokio::main] async fn main() -> Result<(), Box> { - use roslibrust::NodeHandle; + use roslibrust::ros1::NodeHandle; simple_logger::SimpleLogger::new() .with_level(log::LevelFilter::Debug) diff --git a/roslibrust/examples/ros1_talker.rs b/roslibrust/examples/ros1_talker.rs index 3d40d479..934a19cf 100644 --- a/roslibrust/examples/ros1_talker.rs +++ b/roslibrust/examples/ros1_talker.rs @@ -3,7 +3,7 @@ roslibrust_codegen_macro::find_and_generate_ros_messages!("assets/ros1_common_in #[cfg(feature = "ros1")] #[tokio::main] async fn main() -> Result<(), Box> { - use roslibrust::NodeHandle; + use roslibrust::ros1::NodeHandle; simple_logger::SimpleLogger::new() .with_level(log::LevelFilter::Debug) diff --git a/roslibrust/src/lib.rs b/roslibrust/src/lib.rs index 8932461d..e3044431 100644 --- a/roslibrust/src/lib.rs +++ b/roslibrust/src/lib.rs @@ -105,6 +105,33 @@ pub use rosbridge::*; pub mod rosapi; #[cfg(feature = "ros1")] -mod ros1; -#[cfg(feature = "ros1")] -pub use ros1::*; +pub mod ros1; + +/// For now starting with a central error type, may break this up more in future +#[derive(thiserror::Error, Debug)] +pub enum RosLibRustError { + #[error("Not currently connected to ros master / bridge")] + 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), + #[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("Rosbridge server reported an error: {0}")] + ServerError(String), + #[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 + #[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. +pub type RosLibRustResult = Result; diff --git a/roslibrust/src/ros1/mod.rs b/roslibrust/src/ros1/mod.rs index f8c2e514..2ae9fdb5 100644 --- a/roslibrust/src/ros1/mod.rs +++ b/roslibrust/src/ros1/mod.rs @@ -11,5 +11,7 @@ mod node; pub use node::*; mod publisher; +pub use publisher::Publisher; mod subscriber; +pub use subscriber::Subscriber; mod tcpros; diff --git a/roslibrust/src/ros1/names.rs b/roslibrust/src/ros1/names.rs index ebf9a120..d61a3d93 100644 --- a/roslibrust/src/ros1/names.rs +++ b/roslibrust/src/ros1/names.rs @@ -1,3 +1,6 @@ +use crate::{RosLibRustError, RosLibRustResult}; +use std::fmt::Display; + lazy_static::lazy_static! { static ref GRAPH_NAME_REGEX: regex::Regex = regex::Regex::new(r"^([/~a-zA-Z]){1}([a-zA-Z0-9_/])*([A-z0-9_])$").unwrap(); } @@ -8,12 +11,12 @@ pub struct Name { } impl Name { - pub fn new(name: impl Into) -> Option { + pub fn new(name: impl Into) -> RosLibRustResult { let name: String = name.into(); if is_valid(&name) { - Some(Self { inner: name }) + Ok(Self { inner: name }) } else { - None + Err(RosLibRustError::InvalidName(name)) } } @@ -54,6 +57,12 @@ fn is_valid(name: &str) -> bool { GRAPH_NAME_REGEX.is_match(name) } +impl Display for Name { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.inner.fmt(f) + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/roslibrust/src/ros1/node/actor.rs b/roslibrust/src/ros1/node/actor.rs index 47276bfa..c72258a3 100644 --- a/roslibrust/src/ros1/node/actor.rs +++ b/roslibrust/src/ros1/node/actor.rs @@ -5,8 +5,9 @@ use crate::{ node::{XmlRpcServer, XmlRpcServerHandle}, publisher::Publication, subscriber::Subscription, + MasterClient, }, - MasterClient, ServiceCallback, + ServiceCallback, }; use abort_on_drop::ChildTask; use roslibrust_codegen::RosMessageType; @@ -142,14 +143,13 @@ impl NodeServerHandle { pub async fn register_publisher( &self, topic: &str, - topic_type: &str, queue_size: usize, ) -> Result>, Box> { let (sender, receiver) = oneshot::channel(); match self.node_server_sender.send(NodeMsg::RegisterPublisher { reply: sender, topic: topic.to_owned(), - topic_type: topic_type.to_owned(), + topic_type: T::ROS_TYPE_NAME.to_owned(), queue_size, msg_definition: T::DEFINITION.to_owned(), md5sum: T::MD5SUM.to_owned(), @@ -254,12 +254,7 @@ impl Node { let xmlrpc_server = XmlRpcServer::new(addr, xml_server_handle)?; let client_uri = format!("http://{hostname}:{}", xmlrpc_server.port()); - if let None = Name::new(node_name) { - log::error!("Node name {node_name} is not valid"); - return Err(Box::new(std::io::Error::from( - std::io::ErrorKind::InvalidInput, - ))); - } + let _ = Name::new(node_name)?; let rosmaster_client = MasterClient::new(master_uri, client_uri, node_name).await?; let mut node = Self { diff --git a/roslibrust/src/ros1/node/handle.rs b/roslibrust/src/ros1/node/handle.rs index 493ca3a4..75888ec8 100644 --- a/roslibrust/src/ros1/node/handle.rs +++ b/roslibrust/src/ros1/node/handle.rs @@ -41,7 +41,7 @@ impl NodeHandle { ) -> Result, Box> { let sender = self .inner - .register_publisher::(topic_name, T::ROS_TYPE_NAME, queue_size) + .register_publisher::(topic_name, queue_size) .await?; Ok(Publisher::new(topic_name, sender)) } diff --git a/roslibrust/src/ros1/node/mod.rs b/roslibrust/src/ros1/node/mod.rs index 4fa4a1d3..1a167764 100644 --- a/roslibrust/src/ros1/node/mod.rs +++ b/roslibrust/src/ros1/node/mod.rs @@ -1,7 +1,7 @@ //! This module contains the top level Node and NodeHandle classes. //! These wrap the lower level management of a ROS Node connection into a higher level and thread safe API. -use crate::RosMasterError; +use super::RosMasterError; use std::net::{IpAddr, Ipv4Addr}; mod actor; diff --git a/roslibrust/src/ros1/publisher.rs b/roslibrust/src/ros1/publisher.rs index bc1db0c6..b25abcca 100644 --- a/roslibrust/src/ros1/publisher.rs +++ b/roslibrust/src/ros1/publisher.rs @@ -1,6 +1,4 @@ -use crate::RosLibRustError; - -use super::tcpros::ConnectionHeader; +use crate::{ros1::tcpros::ConnectionHeader, RosLibRustError}; use abort_on_drop::ChildTask; use roslibrust_codegen::RosMessageType; use std::{ diff --git a/roslibrust/src/ros1/subscriber.rs b/roslibrust/src/ros1/subscriber.rs index 7d821a4d..7416dca1 100644 --- a/roslibrust/src/ros1/subscriber.rs +++ b/roslibrust/src/ros1/subscriber.rs @@ -1,4 +1,4 @@ -use super::tcpros::ConnectionHeader; +use crate::ros1::tcpros::ConnectionHeader; use abort_on_drop::ChildTask; use roslibrust_codegen::RosMessageType; use std::{marker::PhantomData, sync::Arc}; diff --git a/roslibrust/src/rosbridge/mod.rs b/roslibrust/src/rosbridge/mod.rs index 284b9dec..258c8602 100644 --- a/roslibrust/src/rosbridge/mod.rs +++ b/roslibrust/src/rosbridge/mod.rs @@ -30,46 +30,13 @@ mod topic_provider; mod comm; use futures_util::stream::{SplitSink, SplitStream}; -use log::*; use std::collections::HashMap; use tokio::net::TcpStream; use tokio_tungstenite::*; use tungstenite::Message; -/// For now starting with a central error type, may break this up more in future -#[derive(thiserror::Error, Debug)] -pub enum RosLibRustError { - #[error("Not currently connected to ros master / bridge")] - 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(tokio_tungstenite::tungstenite::Error), - #[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("Rosbridge server reported an error: {0}")] - ServerError(String), - // Generic catch-all error type for not-yet-handled errors - // TODO ultimately this type will be removed from API of library - #[error(transparent)] - Unexpected(#[from] anyhow::Error), -} - -/// Provides an implementation tranlating the underlying websocket error into our error type -impl From for RosLibRustError { - fn from(e: tokio_tungstenite::tungstenite::Error) -> Self { - // TODO we probably want to expand this type and do some matching here - RosLibRustError::CommFailure(e) - } -} - -/// 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. -pub type RosLibRustResult = Result; +// Doing this to maintain backwards compatibilities like `use roslibrust::rosbridge::RosLibRustError` +pub use super::{RosLibRustError, RosLibRustResult}; /// Used for type erasure of message type so that we can store arbitrary handles type Callback = Box; From dfcb4b790785169ebeb06ad6499513cd9d8144e2 Mon Sep 17 00:00:00 2001 From: Shane Snover Date: Tue, 19 Dec 2023 19:46:31 -0700 Subject: [PATCH 3/3] Fix some test imports --- roslibrust/src/ros1/master_client.rs | 2 +- roslibrust/tests/ros1_xmlrpc.rs | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/roslibrust/src/ros1/master_client.rs b/roslibrust/src/ros1/master_client.rs index 4ceaa819..4a015880 100644 --- a/roslibrust/src/ros1/master_client.rs +++ b/roslibrust/src/ros1/master_client.rs @@ -361,7 +361,7 @@ impl MasterClient { #[cfg(test)] mod test { - use crate::{MasterClient, RosMasterError}; + use super::{MasterClient, RosMasterError}; const TEST_NODE_ID: &str = "/native_ros1_test"; diff --git a/roslibrust/tests/ros1_xmlrpc.rs b/roslibrust/tests/ros1_xmlrpc.rs index 6ed96066..54837788 100644 --- a/roslibrust/tests/ros1_xmlrpc.rs +++ b/roslibrust/tests/ros1_xmlrpc.rs @@ -1,5 +1,6 @@ #[cfg(all(feature = "ros1", feature = "ros1_test"))] mod tests { + use roslibrust::ros1::NodeHandle; use roslibrust_codegen::RosMessageType; use serde::de::DeserializeOwned; use serde_xmlrpc::Value; @@ -27,8 +28,7 @@ mod tests { #[test_log::test(tokio::test)] async fn verify_get_master_uri() -> Result<(), Box> { - let node = - roslibrust::NodeHandle::new("http://localhost:11311", "verify_get_master_uri").await?; + let node = NodeHandle::new("http://localhost:11311", "verify_get_master_uri").await?; log::info!("Got new handle"); let node_uri = node.get_client_uri().await?; @@ -48,8 +48,7 @@ mod tests { #[test_log::test(tokio::test)] async fn verify_get_publications() -> Result<(), Box> { - let node = roslibrust::NodeHandle::new("http://localhost:11311", "verify_get_publications") - .await?; + let node = NodeHandle::new("http://localhost:11311", "verify_get_publications").await?; log::info!("Got new handle"); let node_uri = node.get_client_uri().await?; @@ -86,7 +85,7 @@ mod tests { #[test_log::test(tokio::test)] async fn verify_shutdown() { - let node = roslibrust::NodeHandle::new("http://localhost:11311", "verify_shutdown") + let node = NodeHandle::new("http://localhost:11311", "verify_shutdown") .await .unwrap(); log::info!("Got handle"); @@ -110,7 +109,7 @@ mod tests { #[test_log::test(tokio::test)] async fn verify_request_topic() { - let node = roslibrust::NodeHandle::new("http://localhost:11311", "verify_request_topic") + let node = NodeHandle::new("http://localhost:11311", "verify_request_topic") .await .unwrap(); log::info!("Got handle");