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.

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

Add: Add network discovery and upgrade AutoCreate #16

Merged

Conversation

RaulTrombin
Copy link
Member

This PR adds network discovery and upgrade AutoCreate method on Device_Manager.

image

@RaulTrombin RaulTrombin force-pushed the Add_udp_device_discover branch from e5c8b5d to 621a68e Compare August 30, 2024 15:29
@RaulTrombin RaulTrombin marked this pull request as ready for review August 30, 2024 15:31
Comment on lines 18 to 69
fn from_response(response: &str) -> Option<Self> {
let lines: Vec<&str> = response
.split("\r\n")
.filter(|line| !line.is_empty())
.collect();

if lines.len() != 4 {
warn!(
"Expected 4 lines but found {}. Response: {:?}",
lines.len(),
lines
);
return None;
}

let device_name = lines[0].trim().to_string();
let manufacturer = lines[1].trim().to_string();

let mac_address_line = lines[2].trim();
let ip_address_line = lines[3].trim();

if !mac_address_line.starts_with("MAC Address:- ")
|| !ip_address_line.starts_with("IP Address:- ")
{
warn!("auto_create: network: Unexpected format in MAC Address or IP Address lines.");
return None;
}

let mac_address = mac_address_line
.trim_start_matches("MAC Address:- ")
.to_string();
let ip_address_str = ip_address_line.trim_start_matches("IP Address:- ").trim();

let cleaned_ip_address_str = ip_address_str
.split('.')
.map(|octet| {
let trimmed = octet.trim_start_matches('0');
if trimmed.is_empty() {
"0"
} else {
trimmed
}
})
.collect::<Vec<&str>>()
.join(".");

let ip_address = match cleaned_ip_address_str.parse::<Ipv4Addr>() {
Ok(ip) => ip,
Err(err) => {
warn!(
"auto_create: network: Failed to parse IP address: {cleaned_ip_address_str}, details: {err}"
);
return None;
}
};

Some(DiscoveryResponse {
device_name,
manufacturer,
mac_address,
ip_address,
})
}
Copy link
Member

Choose a reason for hiding this comment

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

Regex with capture groups might be well-fitted here, for example, something like (untested):

Suggested change
fn from_response(response: &str) -> Option<Self> {
let lines: Vec<&str> = response
.split("\r\n")
.filter(|line| !line.is_empty())
.collect();
if lines.len() != 4 {
warn!(
"Expected 4 lines but found {}. Response: {:?}",
lines.len(),
lines
);
return None;
}
let device_name = lines[0].trim().to_string();
let manufacturer = lines[1].trim().to_string();
let mac_address_line = lines[2].trim();
let ip_address_line = lines[3].trim();
if !mac_address_line.starts_with("MAC Address:- ")
|| !ip_address_line.starts_with("IP Address:- ")
{
warn!("auto_create: network: Unexpected format in MAC Address or IP Address lines.");
return None;
}
let mac_address = mac_address_line
.trim_start_matches("MAC Address:- ")
.to_string();
let ip_address_str = ip_address_line.trim_start_matches("IP Address:- ").trim();
let cleaned_ip_address_str = ip_address_str
.split('.')
.map(|octet| {
let trimmed = octet.trim_start_matches('0');
if trimmed.is_empty() {
"0"
} else {
trimmed
}
})
.collect::<Vec<&str>>()
.join(".");
let ip_address = match cleaned_ip_address_str.parse::<Ipv4Addr>() {
Ok(ip) => ip,
Err(err) => {
warn!(
"auto_create: network: Failed to parse IP address: {cleaned_ip_address_str}, details: {err}"
);
return None;
}
};
Some(DiscoveryResponse {
device_name,
manufacturer,
mac_address,
ip_address,
})
}
fn from_response(response: &str) -> Result<Self> {
let re = Regex::new(r"(?m)^(?P<device_name>.+)\r?\n\
(?P<manufacturer>.+)\r?\n\
MAC Address:- (?P<mac_address>[\dA-Fa-f-]+)\r?\n\
IP Address:- (?P<ip_address>\d{3}\.\d{3}\.\d{3}\.\d{3})\r?\n$").ok()?;
let caps = re.captures(response)?;
let ip_address = caps["ip_address"]
.split('.')
.map(|x| x.parse::<u8>().unwrap())
.collect::<Vec<u8>>()
.as_slice()
.try_into()
.map(Ipv4Addr::from)
.ok()?;
Some(DiscoveryResponse {
device_name: caps["device_name"].to_string(),
manufacturer: caps["manufacturer"].to_string(),
mac_address: caps["mac_address"].to_string(),
ip_address,
})
}

let socket = std::net::UdpSocket::bind("0.0.0.0:0").unwrap(); // Bind to any available port
socket.set_broadcast(true).unwrap(); // Enable broadcast

let broadcast_addr = "255.255.255.255:30303";
Copy link
Member

Choose a reason for hiding this comment

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

why this port?

Copy link
Member Author

Choose a reason for hiding this comment

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

specific for ping360 discovery,
"The Discovery message is sent by the host computer to the sonar as a broadcast
message to IP address 255.255.255.255 using port 30303. Any sonars connected will
respond with a Discovery Response message"

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have such comment in the code

"Failed to create device for port {}: {:?}",
port_info.port_name, err
);
error!("Failed to create device for source {:?}: {:?}", source, err);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
error!("Failed to create device for source {:?}: {:?}", source, err);
error!("Failed to create device for source {source:?}: {err:?}");

Comment on lines 115 to 116
Err(e) => {
warn!("auto_create: network: Timeout or error receiving response: {e}");
Copy link
Member

Choose a reason for hiding this comment

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

watch out for your consistency:

Suggested change
Err(e) => {
warn!("auto_create: network: Timeout or error receiving response: {e}");
Err(err) => {
warn!("auto_create: network: Timeout or error receiving response: {err}");

Comment on lines 43 to 61
let cleaned_ip_address_str = ip_address_str
.split('.')
.map(|octet| {
let trimmed = octet.trim_start_matches('0');
if trimmed.is_empty() {
"0"
} else {
trimmed
}
})
.collect::<Vec<&str>>()
.join(".");
Copy link
Member

Choose a reason for hiding this comment

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

you are only doing this because the regex above doesn't match your requirements.

The following is an IPv4 regex that ignoress the padded zeros (regexr.com/2rjkb):

(([2]([5][0-5]|[0-4][0-9]))|([1][0-9]{2})|([1-9]?[0-9]))(\.(([2]([5][0-5]|[0-4][0-9]))|([1][0-9]{2})|([1-9]?[0-9]))){3}

Copy link
Member

Choose a reason for hiding this comment

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

for readability, you can separate the regex parts into their own strings, and combine them with format!() and/or concat!()

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, no success here:
https://regexr.com/85i2n

Copy link
Member

Choose a reason for hiding this comment

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

IP Address:-\s(0{0,2}(?<octet_1>[0-9]{1,3})\.0{0,2}(?<octet_2>[0-9]{1,3})\.0{0,2}(?<octet_3>[0-9]{1,3})\.0{0,2}(?<octet_4>[0-9]{1,3}))

then use those four groups instead

Copy link
Member Author

Choose a reason for hiding this comment

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

=/
image

Copy link
Member

Choose a reason for hiding this comment

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

have you tried it in rust?

Copy link
Member

Choose a reason for hiding this comment

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

correction:

IP\sAddress:-\s*0{0,2}(?<octet_1>[0-9]{1,3})\.0{0,2}(?<octet_2>[0-9]{1,3})\.0{0,2}(?<octet_3>[0-9]{1,3})\.0{0,2}(?<octet_4>[0-9]{1,3})

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed to use PCRE, but seems I not managed how to achieve it yet, even directly in Rust didn't work the same
https://regexr.com/85i2n

Copy link
Member

@joaoantoniocardoso joaoantoniocardoso Sep 5, 2024

Choose a reason for hiding this comment

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

It says it is working on that link. To use it, it should be something like:

let ip_address_str = format!("{}.{}.{}.{}",
    &captures["octet_1"],
    &captures["octet_2"],
    &captures["octet_3"],
    &captures["octet_4"],
);

@RaulTrombin RaulTrombin force-pushed the Add_udp_device_discover branch from 1f87f35 to 277dbcb Compare September 5, 2024 23:22
@joaoantoniocardoso joaoantoniocardoso merged commit 4dceeba into bluerobotics:master Sep 6, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants