Skip to content

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

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

Jump to bottom
Merged
merged 3 commits into from
Jan 11, 2025
Merged

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

merged 3 commits into from
Jan 11, 2025

Conversation

@mherwege
Copy link
Contributor

@mherwege mherwege commented Jan 10, 2025 •
edited
Loading

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
make icmp ping and arp ping optional by thing
50a07dc 
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege mherwege added the enhancement An enhancement or new feature for an existing add-on label Jan 10, 2025
jlaur
jlaur reviewed Jan 10, 2025
View reviewed 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.

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

@mherwege
review feedback
d44a3f0 
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege
Copy link
Contributor Author

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

jlaur
jlaur requested changes Jan 10, 2025
View reviewed 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.

@mherwege
review feedback
2ef6e17 
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege
Copy link
Contributor Author

Adjustments done.

jlaur
jlaur approved these changes Jan 11, 2025
View reviewed 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.

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
@mherwege @GearrelW
[network] Make icmp ping and arp ping optional by presence thing (ope…
f0df64f 
…nhab#18083)

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
chilobo pushed a commit to chilobo/openhab-addons that referenced this pull request Feb 10, 2025
@mherwege @chilobo
[network] Make icmp ping and arp ping optional by presence thing (ope…
0b51934 
…nhab#18083)

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Christian Koch <78686276+chilobo@users.noreply.github.com>
phenix1990 pushed a commit to phenix1990/openhab-addons that referenced this pull request Jul 31, 2025
@mherwege @phenix1990
[network] Make icmp ping and arp ping optional by presence thing (ope…
319ae5b 
…nhab#18083)

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege mherwege mentioned this pull request Sep 4, 2025
Merged
Nadahar pushed a commit to Nadahar/openhab-addons that referenced this pull request Oct 3, 2025
@mherwege
[network] Make icmp ping and arp ping optional by presence thing (ope…
b021146 
…nhab#18083)

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
This was referenced Oct 3, 2025
Merged
Merged
lsiepel pushed a commit that referenced this pull request Oct 11, 2025
@Nadahar
[network] Performance refactoring 4.3.x (#19428)
b68805f 
* [network] Make icmp ping and arp ping optional by presence thing (#18083)

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@lsiepel lsiepel added backported4 Backported patches to the 4.x branch. Used for constructing release notes. backported A PR that has been cherry-picked to a patch release branch labels Oct 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jlaur jlaur jlaur approved these changes

Assignees

No one assigned

Labels

backported A PR that has been cherry-picked to a patch release branch backported4 Backported patches to the 4.x branch. Used for constructing release notes. enhancement An enhancement or new feature for an existing add-on

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

3 participants

@mherwege @jlaur @lsiepel