Merge pull request #146 from Carter12s/reorg-native-node-module

Reorganize the code in node.rs into a couple separate files

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
 

roslibrust/examples/native_ros1.rs

@@ -5,7 +5,7 @@
 #[cfg(feature = "ros1")]
 #[tokio::main]
 async fn main() -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
-    use roslibrust::NodeHandle;
+    use roslibrust::ros1::NodeHandle;
 
     simple_logger::SimpleLogger::new()
         .with_level(log::LevelFilter::Debug)

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<dyn std::error::Error + Send + Sync>> {
-    use roslibrust::NodeHandle;
+    use roslibrust::ros1::NodeHandle;
 
     simple_logger::SimpleLogger::new()
         .with_level(log::LevelFilter::Debug)

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<dyn std::error::Error + Send + Sync>> {
-    use roslibrust::NodeHandle;
+    use roslibrust::ros1::NodeHandle;
 
     simple_logger::SimpleLogger::new()
         .with_level(log::LevelFilter::Debug)

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<T> = Result<T, RosLibRustError>;

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";
 

roslibrust/src/ros1/mod.rs

@@ -4,16 +4,14 @@
 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
 mod node;
 pub use node::*;
 
 mod publisher;
+pub use publisher::Publisher;
 mod subscriber;
+pub use subscriber::Subscriber;
 mod tcpros;

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<String>) -> Option<Self> {
+    pub fn new(name: impl Into<String>) -> RosLibRustResult<Self> {
         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::*;

roslibrust/src/ros1/node.rs → roslibrust/src/ros1/node/actor.rs

@@ -1,28 +1,19 @@
-//! 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 +59,7 @@ pub enum NodeMsg {
 
 #[derive(Clone)]
 pub(crate) struct NodeServerHandle {
-    node_server_sender: mpsc::UnboundedSender<NodeMsg>,
+    pub(crate) node_server_sender: mpsc::UnboundedSender<NodeMsg>,
     // 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)
@@ -152,14 +143,13 @@ impl NodeServerHandle {
     pub async fn register_publisher<T: RosMessageType>(
         &self,
         topic: &str,
-        topic_type: &str,
         queue_size: usize,
     ) -> Result<mpsc::Sender<Vec<u8>>, Box<dyn std::error::Error + Send + Sync>> {
         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(),
@@ -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,
@@ -264,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 {
@@ -510,112 +495,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<NodeHandle, Box<dyn std::error::Error + Send + Sync>> {
-        // 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<String, Box<dyn std::error::Error + Send + Sync>> {
-        self.inner.get_client_uri().await
-    }
-
-    pub async fn advertise<T: roslibrust_codegen::RosMessageType>(
-        &self,
-        topic_name: &str,
-        queue_size: usize,
-    ) -> Result<Publisher<T>, Box<dyn std::error::Error + Send + Sync>> {
-        let sender = self
-            .inner
-            .register_publisher::<T>(topic_name, T::ROS_TYPE_NAME, queue_size)
-            .await?;
-        Ok(Publisher::new(topic_name, sender))
-    }
-
-    pub async fn subscribe<T: roslibrust_codegen::RosMessageType>(
-        &self,
-        topic_name: &str,
-        queue_size: usize,
-    ) -> Result<Subscriber<T>, Box<dyn std::error::Error + Send + Sync>> {
-        let receiver = self
-            .inner
-            .register_subscriber::<T>(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<Ipv4Addr, RosMasterError> {
-    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:?}"
-        )))
-    }
-}