leiweibau / Pi.Alert

Scan the devices connected to your WIFI / LAN and alert you the connection of unknown devices. It also warns if a "always connected" device disconnects. In addition, it is possible to check web services for availability. For this purpose HTTP status codes and the response time of the service are evaluated.
https://leiweibau.net
GNU General Public License v3.0
406 stars 28 forks source link

[Feature Request] Configurable timeout for ping command #172

Closed hspindel closed 11 months ago

hspindel commented 11 months ago

A while ago I suggested a programmable timeout for ICMP pings. That idea didn't fly - I couldn't find a way to make it work either.

The problem I am seeing is that ping response on my network is occasionally wonky. A device doesn't respond to a ping and pialert thinks it's down. This is not a pialert problem as I see occasional non-response to pings even issued from the command line. I have no idea why some devices don't always respond to pings. It may have something to do with being a WiFi device (possible connectivity interruptions?).

Anyway, I experimented with a fix and came up with this:


def icmp_monitoring():

print("\nStart ICMP Monitoring...")
print("    Get Host/Domain List...")
icmphosts = get_icmphost_list()
icmphostscount = len(icmphosts)
print("        List contains " + str(icmphostscoun
print("    Flush previous ping results...")
flush_icmphost_current_scan()
print("    Ping Hosts...")

scantime = startTime.strftime("%Y-%m-%d %H:%M")

icmphosts_all = len(icmphosts)
icmphosts_online = 0
icmphosts_offline = 0

while icmphosts:
    for host_ip in icmphosts:
        for i in range(10):
            icmp_status = ping(host_ip)
            if icmp_status == "1":
                break;

The change is only in the "while icmphosts" loop to retry the ping up to 10 times before assuming there's no response. Note that "10" is arbitrary and could even be a programmable value in the pialert.ini file. If the value is set to 1, the behavior should be identical to the current implementation without the change so maybe that should be the default.

The change fixes all my issues with ICMP probing.

I hope this contribution is useful to the project. Note that while I was a programmer I don't know python and simply used some web examples to guide me. If there's a more idiomatic way to write this, feel free to change it.

Thank you.

leiweibau commented 11 months ago

Good work. I will get to work on the implementation in the next few days. However, for ease of configuration, I will take a different approach.

leiweibau commented 11 months ago

Now that I am taking care of the feature request you inspired, I would close this "issue"

hspindel commented 11 months ago

Yes, close it.

Thank you.

hspindel commented 11 months ago

Reopening. Downloading the latest update causes the ping issues I've had to reappear. Couldn't find a related change in pialert.py. Am I just being impatient?

leiweibau commented 11 months ago

Yes you are. 😉 The changes are not yet included in the installation package. You would have to download the repository and copy the files manually.

You can use the Update Check to find out if there is an update. If it does not show anything, then there is no update.

hspindel commented 11 months ago

Sorry for my impatience. There was a new version available and I mistakenly thought it would include the ping change.

I did manually download pialert.py from the repository. Looks to me like the setting ICMP_ONLINE_TEST in pialert.conf controls the number of pings to try?

Question about the implementation (hope you don't mind)? Looks to me like it always performs the full n number of ICMP_ONLINE_TEST pings regardless of the output and accepts the result of the nth ping as the final result. This is different from my suggestion, which performed only up to n pings, and stops as soon as a successful ping is seen. If I'm looking at this right, then it's not going to address the issue I'm seeing because the nth ping is just as likely as the first ping to fail.

leiweibau commented 11 months ago

It should not make any difference if you send 5 or 10 pings to check if the host was reachable after each ping or if you send 5 or 10 pings one after the other to check if there was 100% packet loss at the end.

leiweibau commented 11 months ago

But you are not completly wrong. I need to change this line too

https://github.com/leiweibau/Pi.Alert/blob/b3eec9c27aa6679fe06dd0f7fe3a58ceb726ee16/back/pialert.py#L2022

However, I admit that your approach also has certain advantages. I will think about it again.

hspindel commented 11 months ago

Okay, thank you.

Will wait to see what you come up with.

leiweibau commented 11 months ago

Current debug output

Start ICMP Monitoring...
    Get Host/Domain List...
        List contains 4 entries
    Flush previous ping results...
    Ping Hosts...
Host 10.100.0.2 retry 1
Host 10.100.0.2 RTT 56.099
Host 10.100.0.3 retry 1
Host 10.100.0.3 retry 2
Host 10.100.0.3 retry 3
Host 10.100.0.3 retry 4
Host 10.100.0.3 retry 5
Host 10.100.0.4 retry 1
Host 10.100.0.4 retry 2
Host 10.100.0.4 RTT 39.778
Host 10.100.0.5 retry 1
Host 10.100.0.5 retry 2
Host 10.100.0.5 retry 3
Host 10.100.0.5 retry 4
Host 10.100.0.5 retry 5
        Online Host(s)  : 2
        Offline Host(s) : 2
hspindel commented 11 months ago

Looks good. Stops as soon as it sees a success, which is exactly what's needed.

Thank you.

leiweibau commented 11 months ago

Update released with https://github.com/leiweibau/Pi.Alert/commit/9d8ac4fbd29257948e90fd01c419ed8aaecdf79d

hspindel commented 11 months ago

Have tested the update for several hours and seems to work well.

Thank you!