Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix invalid warning when $ROS_AUTOMATIC_DISCOVERY_RANGE is not defined #343

Merged
merged 3 commits into from
Nov 27, 2024
Merged

Fix invalid warning when $ROS_AUTOMATIC_DISCOVERY_RANGE is not defined #343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
340f38f
Fix invalid warning when is not defined
JEnoch Nov 27, 2024
7ce4c91
fix format and clippy warns
JEnoch Nov 27, 2024
3b3f58f
Fix unit test
JEnoch Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions zenoh-plugin-ros2dds/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use std::{env, fmt, time::Duration};

use regex::Regex;
use serde::{de, de::Visitor, ser::SerializeSeq, Deserialize, Deserializer, Serialize, Serializer};
use tracing::warn;
use zenoh::{key_expr::OwnedKeyExpr, qos::Priority};

pub const DEFAULT_NAMESPACE: &str = "/";
Expand Down Expand Up @@ -486,12 +487,19 @@ fn default_localhost_only() -> bool {
}

fn default_automatic_discovery_range() -> Option<RosAutomaticDiscoveryRange> {
Some(match env::var("ROS_AUTOMATIC_DISCOVERY_RANGE").as_deref() {
Ok("LOCALHOST") => RosAutomaticDiscoveryRange::Localhost,
Ok("OFF") => RosAutomaticDiscoveryRange::Localhost,
Ok("SYSTEM_DEFAULT") => RosAutomaticDiscoveryRange::SystemDefault,
_ => RosAutomaticDiscoveryRange::Subnet,
})
match env::var("ROS_AUTOMATIC_DISCOVERY_RANGE").as_deref() {
Ok("SUBNET") => Some(RosAutomaticDiscoveryRange::Subnet),
Ok("LOCALHOST") => Some(RosAutomaticDiscoveryRange::Localhost),
Ok("OFF") => Some(RosAutomaticDiscoveryRange::Localhost),
Ok("SYSTEM_DEFAULT") => Some(RosAutomaticDiscoveryRange::SystemDefault),
Ok(value) => {
warn!(
r#"Invalid value for environment variable ROS_AUTOMATIC_DISCOVERY_RANGE ("{value}"). Using "SUBNET" instead "#
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether we should have a warning here
According to here, we shouldn't change any discovery setting. I use Subnet because I think our default configuration should be Subnet.
WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to here, we shouldn't change any discovery setting

Reading this doc, the case of an invalid string is not specified. So we have 2 options:

  • either panic, as we can't return an Error in this default_automatic_discovery_range() function
  • either ignoring the invalid value and use the default one which is Subnet according to the doc. I think it's also good to log a warning in this case to point the invalid value to the user.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind. I missed the line Ok(value)

);
Some(RosAutomaticDiscoveryRange::Subnet)
}
Err(_) => None,
}
}

fn deserialize_automatic_discovery_range<'de, D>(
Expand All @@ -507,7 +515,7 @@ where
"OFF" => Ok(Some(RosAutomaticDiscoveryRange::Off)),
"SYSTEM_DEFAULT" => Ok(Some(RosAutomaticDiscoveryRange::SystemDefault)),
unknown => Err(de::Error::custom(format!(
r#"Invalid parameter {unknown} for ROS_AUTOMATICALLY_DISCOVERY_RANGE"#
r#"Invalid parameter "{unknown}" for ROS_AUTOMATICALLY_DISCOVERY_RANGE"#
))),
}
}
Expand All @@ -519,7 +527,7 @@ where
let peers: String = Deserialize::deserialize(deserializer).unwrap();
let mut peer_list: Vec<String> = Vec::new();
for peer in peers.split(';') {
if peer != "" {
if !peer.is_empty() {
peer_list.push(peer.to_owned());
}
}
Expand Down Expand Up @@ -925,7 +933,7 @@ mod tests {
assert_eq!(__required__, None);
}

#[test_case("{}", Some(RosAutomaticDiscoveryRange::Subnet); "Empty tests")]
#[test_case("{}", None; "Empty tests")]
#[test_case(r#"{"ros_automatic_discovery_range": "SUBNET"}"#, Some(RosAutomaticDiscoveryRange::Subnet); "SUBNET tests")]
#[test_case(r#"{"ros_automatic_discovery_range": "LOCALHOST"}"#, Some(RosAutomaticDiscoveryRange::Localhost); "LOCALHOST tests")]
#[test_case(r#"{"ros_automatic_discovery_range": "OFF"}"#, Some(RosAutomaticDiscoveryRange::Off); "OFF tests")]
Expand Down
2 changes: 1 addition & 1 deletion zenoh-plugin-ros2dds/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ pub async fn run(runtime: Runtime, config: Config) {
(Some(RosAutomaticDiscoveryRange::Localhost), None)
} else {
(
config.ros_automatic_discovery_range.clone(),
config.ros_automatic_discovery_range,
config.ros_static_peers.clone(),
)
};
Expand Down