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

[network] Make icmp ping and arp ping optional by presence thing #18083

Merged
merged 3 commits into from
Jan 11, 2025

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented Jan 10, 2025

I was trying to use the Network binding to create a ping reply log to my router. It turned out to be hard to get a good result. While the presence thing has been optimized for presence detection, it does not allow good and comparable ping latency tracking.

The issues I ran into:

  • Pinging a local IPv4 address started an ICMP ping and an ARP ping. Only the ICMP ping got the latency from the reply, the ARP ping estimated the latency in the binding with considerable delay and overhead. As both measurements were returned at slightly different times, it caused the measurements to heavily fluctuate, making them non-comparable and irrelevant.
    A workaround was to use an IPv6 destination address, as that would exclude ARP ping, but it is still only a workaround.
  • I could not disable ARP ping without disabling it for the whole binding.

This PR does:

  • Create 2 configuration parameters by presence thing to allow disabling ARP ping and/or ICMP ping (disabling both probably not the best choice, but for presence detection one could solely rely on DHCP advertisements as well). This allows having only one type of reply and have comparable outcomes.
  • Extend the ARP ping logic to extract the latency from the response. This has been tested on an RPi with the response I got there. If the format from other tools is different, this can be enhanced. But as it stands now, it will not change things if the format is not supported, but will improve when the format is recognized.

@mherwege mherwege added the enhancement An enhancement or new feature for an existing add-on label Jan 10, 2025
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Nice, thanks! Two comments, otherwise LGTM. And thanks for bringing my attention to NetworkUtils, this helped me reassure my approach in #18082. 😄

Signed-off-by: Mark Herwege <[email protected]>
@mherwege
Copy link
Contributor Author

@jlaur Thanks for the feedback. I have done the requested changes.

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

I found a few more things to update after renaming the ICMP ping parameter.

Signed-off-by: Mark Herwege <[email protected]>
@mherwege
Copy link
Contributor Author

Adjustments done.

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM

@jlaur jlaur merged commit 3c90a0d into openhab:main Jan 11, 2025
2 checks passed
@jlaur jlaur added this to the 5.0 milestone Jan 11, 2025
@mherwege mherwege deleted the network branch January 11, 2025 11:13
GearrelW pushed a commit to GearrelW/openhab-addons that referenced this pull request Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants