-
Notifications
You must be signed in to change notification settings - Fork 61
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 unicast search mode #120
Conversation
Odd that it fails here, builds locally. Will check on Monday |
ping @DmitrySamoylov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this feature is indeed very beneficial.
onvif/src/discovery/mod.rs
Outdated
} | ||
|
||
let produce_devices = async move { | ||
futures::future::join_all(unicast_requests.iter().map( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's limit the concurrency when joining futures. Otherwise, we may run out of resources if too many IPs were enumerated.
https://docs.rs/futures/latest/futures/stream/trait.StreamExt.html#method.buffer_unordered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that would work with the lookup limit since some IPs will never reply. I'll try to adapt the code in a way that handles both the volume and the duration timeout.
Will keep you posted at it might not be today
I decided to go for it and applied few changes:
Here's what I'm expecting the code does:
Let me know what you think of these changes. Again feel free to comment on the use of async/await, not a world I'm well acquainted with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
Damn, I forgot to mention I have not tested that variant on a real setup. I will test when I get a setup with a camera working and open a fix |
WS Discovery specifies that multicast packets must be sent on the network to find nearby devices. This works fine in most cases, but with the advent of docker networks, some containerized applications cannot send broadcast packets (unless mounted with the
host
network option, which is not always desirable).This leads to the discovery not working in certain scenarios. While the specification of WS discovery clearly states that the process must be used in broadcast or unicast through a proxy, it was found that sending the probes directly to the devices works in most cases.
I therefore added the option to the
DiscoveryBuilder
to choose this alternative mode of searching for devices, with adebug!
warning clearly stating that some devices might be missing.The unicasts packets are sent asynchronously. Please double-check my
async
code since this is not a paradigm I often use. The goal is to send all packets and wait for all responses concurrently.