ROS1: have better heuristics for selecting self-IP

[0x53A]

I installed the package, tried to connect to an existing ROS1 (noetic) system ... and crashed with ROS_HOSTNAME resolved to an IPv6 address which is not support by ROS/roslibrust.

Looking at the code, it just takes the first ip it finds, and then fails if that one is an IPv6.

https://github.com/RosLibRust/roslibrust/blob/6ae0c2451421e730055006afe001728f37430efd/roslibrust_ros1/src/node/mod.rs#L56C1-L77C2

At the very least, it should skip any v6 IPs and take the first v4 IP. Even better would be, if it tried to find an IP that's in the same subnet as the ros-master.

In my own code, I'm now using something like this:

#[derive(Parser)]
#[command(author, version, about, long_about = None)]
struct Cli {
    /// Address to bind to (e.g. 127.0.0.1:8449)
    #[arg(long, default_value = "0.0.0.0:8449")]
    bind: SocketAddr,

    /// the address of the ROS master (e.g. http://192.168.2.30:11311)
    #[arg(long)]
    ros_master: String,

    #[arg(long)]
    self_ros_ip: Option<Ipv4Addr>,
}

#[tokio::main]
async fn main() -> anyhow::Result<()> {
    println!("Starting!");

    let cli = Cli::parse();

    let self_ros_ip = {
        if let Some(ip) = cli.self_ros_ip {
            Some(ip)
        } else {
            let self_ips = get_all_self_interfaces().await?;

            // Try to find an IP in the same subnet as ROS master
            let selected_ip = if let Ok(ros_master_addr) = cli.ros_master.parse::<SocketAddr>() {
                if let SocketAddr::V4(ros_master_v4) = ros_master_addr {
                    let ros_master_ip = *ros_master_v4.ip();
                    
                    // Find first IP in same subnet as ROS master
                    self_ips.iter()
                        .find(|intf| {
                            same_subnet(intf.ip, ros_master_ip, intf.netmask)
                        })
                        .map(|intf| intf.ip)
                } else {
                    None
                }
            } else {
                None
            };

            // If no match in subnet, use first non-localhost IPv4
            selected_ip.or_else(|| {
                self_ips.iter()
                    .find(|intf| !intf.ip.is_loopback())
                    .map(|intf| intf.ip)
            })
        }
    };
    if env::var_os("ROS_IP").is_none() && let Some(self_ros_ip) = self_ros_ip {
        unsafe { env::set_var("ROS_IP", self_ros_ip.to_string()) };
    }

    // Connect to ROS master
    let ros = roslibrust::ros1::NodeHandle::new(&cli.ros_master, "Groundstation_Gateway")
        .await?;

    // ...
}


/// Get all local IPv4 addresses
async fn get_all_self_interfaces() -> anyhow::Result<Vec<Ifv4Addr>> {
    use if_addrs::get_if_addrs;
    
    let if_addrs = get_if_addrs()?;
    let ipv4_addrs: Vec<Ifv4Addr> = if_addrs
        .iter()
        .filter_map(|iface| {
            if let if_addrs::IfAddr::V4(ifv4) = &iface.addr {
                Some(ifv4.clone())
            } else {
                None
            }
        })
        .collect();
    
    Ok(ipv4_addrs)
}

/// Check if two IPv4 addresses are in the same subnet given a subnet mask
fn same_subnet(ip1: Ipv4Addr, ip2: Ipv4Addr, mask: Ipv4Addr) -> bool {
    let ip1_octets = ip1.octets();
    let ip2_octets = ip2.octets();
    let mask_octets = mask.octets();
    
    for i in 0..4 {
        if (ip1_octets[i] & mask_octets[i]) != (ip2_octets[i] & mask_octets[i]) {
            return false;
        }
    }
    true
}

That code would need be expanded for inclusion into roslibrust - the rosmaster uri can also be a hostname, not just an IP, where we would need to resolve the hostname to potentially multiple IP addresses and then see if one of our own IPs matches the subnet of one of the rosmaster ips.

[Carter12s]

Yup in favor of this change and something we should definitely upgrade.

We additionally have the problem that the ros1 / rosbridge / ros1_zenoh / mock back-ends all behave slightly differently in terms of their configuration and defaults. Would be great to clean-up and standardize the way clients are constructed so that each client is created from some ClientOptions builder type, and that type has a ::default() implementation.

[0x53A]

I'll try to send a PR for the heuristic I mentioned - the advantage is that this wouldn't be an API breaking change. The cleanup with the breaking change can then be scheduled for a major version.