[Network] Add arping support, iOS port knock #2380
Conversation
|
@davidgraeff, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mettke, @kaikreuzer and @J-N-K to be potential reviewers. |
davidgraeff
force-pushed
the
networkarp
branch
6 times, most recently
from
75209bc
to
d8b8e51
Compare
|
Wow the PR-message itself looks perfect, will try to review ASAP :-) |
There was a problem hiding this comment.
@davidgraeff thanks for your PR. It looks like a very good improvement. I am personally highly interested in your ExpiringCacheAsync solution. So I concentrated my comments on that part.
A few thougts in general. I like the idea of several different "cachings" methods - don't know if I can call them like that, but I will do it - and maybe combining them in the ESH core module. So one way to archive that is to be complient to the existing "caching" methods. You solved that in a very good way. May be we can implement an interface or abstract Cache class to set up more than my initial "caching" method to be collected in the ExpiringCacheMap. Could you imagine to integrate the handling of callbacks completely into the ExpiringCacheAsync class?
| * as soon as there is an updated value. | ||
| * | ||
| */ | ||
| public static interface ExpiringCacheValue<V> { |
There was a problem hiding this comment.
Why not using the Functional interface? Could this be a Consumer?
| private final ExpiringCacheUpdate cacheUpdater; | ||
| private long expiresAt = 0; | ||
| private V value; | ||
| private List<ExpiringCacheValue<V>> waitingCacheCallbacks = new LinkedList<>(); |
There was a problem hiding this comment.
Shouldn't this be final?
| public void refreshValue(ExpiringCacheValue<V> callback) { | ||
| expiresAt = 0; | ||
| waitingCacheCallbacks.add(callback); | ||
| // TODO prevent calling this more often than expire time |
There was a problem hiding this comment.
This should be done. ;-)
| * @param expiry the duration in milliseconds for how long the value stays valid | ||
| * @param cacheUpdater The cache will use this callback if a new value is needed. | ||
| */ | ||
| public ExpiringCacheAsync(long expiry, ExpiringCacheUpdate cacheUpdater) { |
There was a problem hiding this comment.
Null check for cacheUpdater to prevent NPE in refreshValue?
| * | ||
| * @return the value | ||
| */ | ||
| public void getValueAsync(ExpiringCacheValue<V> callback) { |
There was a problem hiding this comment.
Shall we call it getValue to be complient with the ExpiringCache class?
IMHO the callback should be handed over in the constructor and stored in a private property. This makes it possible to handle the callback completely internal without using a public setValue method. wdyt?
There was a problem hiding this comment.
Not a good idea, at least in my use case I need multiple consumers, not just one
There was a problem hiding this comment.
Hm, ... then you should use multiple instances of the ExpiryCache or an ExpiringCacheMap to handle multiple values. Each cached value should only be updated in one specific way. If you assign different Consumer for one value there will be a risk to retrieve a wrong value from the cache.
There was a problem hiding this comment.
ExpiryCache(Async) contains one value. When getValue() is called and the value is not expired, all consumers of that API retrieve the same value. If the value is expired they all go in a callback list. As soon as setValue() is called, all consumers get notified with the same value. Do I miss something?
There was a problem hiding this comment.
Understood. You are right. I misinterpreted the situation.
| logger.debug("ping program was interrupted"); | ||
| public void startAutomaticRefresh(ScheduledExecutorService scheduledExecutorService) { | ||
| cancelRefreshJob(); | ||
| refreshJob = scheduledExecutorService.scheduleWithFixedDelay(new Runnable() { |
There was a problem hiding this comment.
If I understand the code correctly, the ExpiringCacheAsync calls this method every time the value should be refreshed. So we can use a one-shot schedule here.
There was a problem hiding this comment.
If I understand you correctly, we use a one shot in "startAutomaticRefresh()" and then a one shot in each "requestCacheUpdate()" call? I'm not sure if we gain a lot by this change.
There was a problem hiding this comment.
Ok, maybe I should ask my question from another point of view: Why should the cache refresh call the networkService.startAutomaticRefresh() method to stop and restart all scheduled jobs? IMHO it only needs to execute one special job to refresh the requested value. Doesn't it?
There was a problem hiding this comment.
To put that line of source code in a context: startAutomaticRefresh() will cause a refresh of the value, but Kai asked basically the same question (https://github.com/openhab/openhab2-addons/pull/1989#discussion_r122158419) ("Do you really think that messing with the scheduled job is the right approach?")
My response was: "Setting a new timeout for the scheduled job makes sense to me, yes. To my understanding the user expresses the wish that as soon as he knows about the (non-)reachable state of a device, he wants the next check to happen after the given period of time. So if the GUI causes a state refresh earlier in time, the next periodic check can be postponed."
Maybe I should rename that method to: performDevicePresenseDetectionImmediately()
|
@cweitkamp Thanks for the review, I will adapt the code based on your comments. I rather not want this to be a PR blocker though. I'm not complaining but my average merge time frame here is 5 month, I don't want this class to delay the PR by waiting for the ESH core team. I will happily adapt the code in a later PR, as soon as ESH provides a comparable solution, of course. I'm not sure if we can discuss it here, or should head over to the community.openhab or ESH forum (I don't like the ESH forum that much though, who uses BBcode nowadays). I did some research and I recon that the existing ExpiringCache together with the Guava ListenableFuture may provide a similar interface to my ExpiringCacheAsync class. Maybe we can also come up with a solution to just extend ExpiringCache? |
|
@davidgraeff I don't want to block your PR. I just thought about a general improvement. I will open an issue in the ESH Repo to start a discussion on it. I think we should try to solve this without using Guava. We implemented the |
|
Just a general question regarding this binding: Should the "thing" go offline, if the target device is not reachable? At the moment it stays online except if there is a configuration error and only the channel "online" goes ON/OFF. |
Good question, @kaikreuzer, wdyt? |
|
The way as we treat it now (i.e. by having a channel "online"), it actually means that the Thing is not the remote device, but rather the "pinging service for that device" - and this one should stay ONLINE as long as it is working correctly. |
davidgraeff
force-pushed
the
networkarp
branch
2 times, most recently
from
b98db97
to
db31b64
Compare
There was a problem hiding this comment.
@davidgraeff thanks for yet another contribution!
I added some remarks can you please take a look at them?
| @@ -117,7 +117,8 @@ Things support the following channels: | |||
| | Channel Type ID | Item Type | Description | | |||
| |-----------------|--------------|----------------------------------------------- | | |||
| | online | Switch | This channel indicates whether a device is online or not | | |||
| | time | Number | This channel indicates the ping time in milliseconds. May be 0 if no time is available. | | |||
| | lastseen | DateTime | The last seen date/time of the device in question. May be 1. Jan 1970 if no time is known. | | |||
There was a problem hiding this comment.
You would make me happy by formatting the tables in the MD file as well :-D
http://www.tablesgenerator.com/markdown_tables
Select 'File' > 'Past table data ...' and past the unformatted mark down table, you can then copy paste the formatted table 👍
| @@ -32,6 +32,7 @@ | |||
| // List of all Channel ids | |||
| public static final String CHANNEL_ONLINE = "online"; | |||
| public static final String CHANNEL_TIME = "time"; | |||
There was a problem hiding this comment.
I think I would expect a latency constant here as well or instead?
There was a problem hiding this comment.
yes, that needs to be renamed, I agree.
| @@ -170,10 +183,9 @@ public void initialize() { | |||
| // values immediately to GUIs, but will query for the current state first and update | |||
| // the channels asynchronously. | |||
| updateState(CHANNEL_ONLINE, OnOffType.OFF); | |||
| updateState(CHANNEL_TIME, new DecimalType(0)); | |||
| updateState(CHANNEL_TIME, UnDefType.UNDEF); | |||
|
|
|||
There was a problem hiding this comment.
Should we assign the LASTSEEN here?
| <description>The port on which the device can be accessed</description> | ||
| <default>0</default> | ||
| </parameter> | ||
| <parameter name="refresh_interval" type="integer"> |
There was a problem hiding this comment.
XML files should be indented using tabs according to our code formatter, as can be seen in the diff you seemed to have switched tabs to spaces...? Can you please reformat the file using the ESH eclipse profile.
| <scr:component xmlns:scr="http://www.osgi.org/xmlns/scr/v1.1.0" | ||
| immediate="true" | ||
| configuration-policy="optional" | ||
| name="org.openhab.binding.network"> |
There was a problem hiding this comment.
Does this work as expected, I remember the Kai mentioned a different name... @kaikreuzer wdyt?
There was a problem hiding this comment.
I've seen this in other binding updates, that the component name of the handlerFactory is shortened to the bindings package name.
There was a problem hiding this comment.
But most important does it work, can you change the config as expected?
There was a problem hiding this comment.
I still think that this should binding.network, see also https://github.com/openhab/openhab2-addons/pull/2501
| @@ -3,71 +3,118 @@ | |||
| xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | |||
| xmlns:thing="http://eclipse.org/smarthome/schemas/thing-description/v1.0.0" | |||
| xsi:schemaLocation="http://eclipse.org/smarthome/schemas/thing-description/v1.0.0 http://eclipse.org/smarthome/schemas/thing-description-1.0.0.xsd"><!--Network Binding--> | |||
| <thing-type id="device"> | |||
There was a problem hiding this comment.
I just realized that this is breaking change? Did you realize that?
There was a problem hiding this comment.
That's true. Most people use the "ping" device, I propose therefore to add a "device" think mapping to "pingdevice" in the handler factory. It will break for people who are using the "port" parameter, but it was a bad idea in the first place to combine this in one thing. There was an issue I linked to in the PR description: When you use the port parameter, all ping parameters are not used at all. That confused people in the forum and we really should fix this rather now than later.
| * Return true if this device is recognised as IOS device | ||
| */ | ||
| public boolean isIOSdevice() { | ||
| return true; |
There was a problem hiding this comment.
Are you sure this should always be true
There was a problem hiding this comment.
I have no solution for this yet, maybe this should a configuration option for the binding or the thing. I'm pretty sure iOS devices can be recognized somehow, I just don't have one.
There was a problem hiding this comment.
Should we then leave out this method for now or add a todo. Note maybe you can base it on the MAC address but that is quite an extended list.
| * | ||
| * @return Set of interface names | ||
| */ | ||
| public static TreeSet<String> getInterfaceNames() { |
There was a problem hiding this comment.
Can it return a Set or maybe SortedSet or should it really be a TreeSet, see also next line
| return true; | ||
| } catch (SocketTimeoutException ignored) { | ||
| return false; | ||
| } catch (ConnectException e) { |
There was a problem hiding this comment.
Since you ignore them all (should you really not trace or debug log them?) you can also call them all ignored
There was a problem hiding this comment.
We actually expect one of those exceptions if the device is not reachable. I would not log this event here.
| * @param logger A slf4j logger instance to log IOException | ||
| * @return | ||
| */ | ||
| public static boolean servicePing(String host, int port, int timeout, Logger logger) { |
There was a problem hiding this comment.
Why pass the logger you can and imho should use a static logger in this case. Otherwise you lose the context where this logging came from.
There was a problem hiding this comment.
I thought static loggers are a no-go? I can use a static one here of course.
There was a problem hiding this comment.
As we are in a dynamic OSGi environment, loggers should be non-static, when ever possible and have the name logger.
davidgraeff
force-pushed
the
networkarp
branch
4 times, most recently
from
568e6dd
to
db1e51b
Compare
|
I addressed all issues including the wish to not change the scheduled job and instead issue a new discovery. I have also moved non exported packages to the "internal" namespace (like in #2393). Some files have changed considerably and need a complete new review I'm afraid. The affected classes are:
Because the cache moved from the handler to PresenceDetection, the following class is affected too:
Some new classes/interfaces are:
|
davidgraeff
changed the title
davidgraeff
force-pushed
the
networkarp
branch
2 times, most recently
from
9078617
to
b686104
Compare
davidgraeff
changed the title
|
@martinvw OMG, that is absolutely not obvious, thank you for finding the culprit. This should be documented, including the resulting error message. |
|
@davidgraeff will you change it in your code? Please note that it was/is also complaining about the about.html not exposed from the test bundle. |
|
@davidgraeff should we try to add it here in your opinion? http://www.eclipse.org/smarthome/documentation/development/testing.html |
|
I would add it to https://www.eclipse.org/smarthome/documentation/development/testing.html#unit-tests. Instead of:
It would read like:
And would duplicate the information for https://www.eclipse.org/smarthome/documentation/development/testing.html#osgi-tests:
|
|
And maybe we should add a section "common errors", where we list the runtime error and the solution for common errors |
davidgraeff
force-pushed
the
networkarp
branch
from
2905b15
to
de122e5
Compare
…seen channel, non blocking GUI refresh * Add new arping feature * Introduce separate things for tcp port device presence detection and ping presence detection, because a lot of users where confused that pings didn't work anymore if a tcp port is set. * Add discovery for tcp service devices and pingable devices. * Add/Improve comments * Introduce ExpiringCacheAsync * Use port knock on UDP port 5353 prior ARP ping to wakeup the iOS network stack * Bind DHCP sniffing to port 6767, if port 67 does not work. This way a user can iat least configure a port forward instead of not being able to use this function at all. * Only respond with cached values in handleCommand:RefreshType and do a refresh in the background. The actual ping command can halt the thread for up to 2 seconds which blocks UIs. * Remove wrong category labels. * Fix: Apparently a DHCPREQUEST message does not need to contain a REQUESTED_ADDRESS field. The source address of the udp packet can be used instead, in that case without bothering the user with an error message in the log. * Add lastseen channel incl DE translation. Rename time channel to latency channel: 'time' is missleading * Move not exported java classes to the "internal" package, except the constants class * Network: Add tests, adapt classes to be more easibly testable. Breaking change * The old thing "device" is now "pingdevice". This is not a real breaking change though, as all old configurations should still work because I map "device" to "pingdevice" in the handler factory. Signed-off-by: David Gräff <david.graeff@web.de>
davidgraeff
force-pushed
the
networkarp
branch
from
de122e5
to
a64f789
Compare
|
Ups, I haven't seen that you pushed, too. I amended and pushed over it :/ |
No problem finding it out was harder than actually changing and pushing it 😄 |
|
@kaikreuzer all green now, please take a final look :-) |
Since a user was troubled by misnaming his test classes a suggestion to improve to document to not let it happen again :-) See also https://github.com/openhab/openhab2-addons/pull/2380 Signed-off-by: Martin van Wingerden <martinvw@mtin.nl>
|
Wow @davidgraeff, what an amazing work and what thorough reviews and discussions on that PR - I just quickly scanned over it and I fully trust @martinvw's approval :-) |
|
Maybe the wrong place, but how long will it probably take until the new version is available on my OpenHAB installation? |
|
Thanks, but what if I want to wait until it becomes available officially, so I can simply update it? Do I need to wait until OH 2.2 gets released? |
|
@claell Yes |
|
Alright |
…seen channel, non blocking GUI refresh (openhab#2380) * Add new arping feature * Introduce separate things for tcp port device presence detection and ping presence detection, because a lot of users where confused that pings didn't work anymore if a tcp port is set. * Add discovery for tcp service devices and pingable devices. * Add/Improve comments * Introduce ExpiringCacheAsync * Use port knock on UDP port 5353 prior ARP ping to wakeup the iOS network stack * Bind DHCP sniffing to port 6767, if port 67 does not work. This way a user can iat least configure a port forward instead of not being able to use this function at all. * Only respond with cached values in handleCommand:RefreshType and do a refresh in the background. The actual ping command can halt the thread for up to 2 seconds which blocks UIs. * Remove wrong category labels. * Fix: Apparently a DHCPREQUEST message does not need to contain a REQUESTED_ADDRESS field. The source address of the udp packet can be used instead, in that case without bothering the user with an error message in the log. * Add lastseen channel incl DE translation. Rename time channel to latency channel: 'time' is missleading * Move not exported java classes to the "internal" package, except the constants class * Network: Add tests, adapt classes to be more easibly testable. Breaking change * The old thing "device" is now "pingdevice". This is not a real breaking change though, as all old configurations should still work because I map "device" to "pingdevice" in the handler factory. Signed-off-by: David Gräff <david.graeff@web.de>
…seen channel, non blocking GUI refresh (openhab#2380) * Add new arping feature * Introduce separate things for tcp port device presence detection and ping presence detection, because a lot of users where confused that pings didn't work anymore if a tcp port is set. * Add discovery for tcp service devices and pingable devices. * Add/Improve comments * Introduce ExpiringCacheAsync * Use port knock on UDP port 5353 prior ARP ping to wakeup the iOS network stack * Bind DHCP sniffing to port 6767, if port 67 does not work. This way a user can iat least configure a port forward instead of not being able to use this function at all. * Only respond with cached values in handleCommand:RefreshType and do a refresh in the background. The actual ping command can halt the thread for up to 2 seconds which blocks UIs. * Remove wrong category labels. * Fix: Apparently a DHCPREQUEST message does not need to contain a REQUESTED_ADDRESS field. The source address of the udp packet can be used instead, in that case without bothering the user with an error message in the log. * Add lastseen channel incl DE translation. Rename time channel to latency channel: 'time' is missleading * Move not exported java classes to the "internal" package, except the constants class * Network: Add tests, adapt classes to be more easibly testable. Breaking change * The old thing "device" is now "pingdevice". This is not a real breaking change though, as all old configurations should still work because I map "device" to "pingdevice" in the handler factory. Signed-off-by: David Gräff <david.graeff@web.de>
martinvw
changed the title
martinvw
added
the
enhancement
…seen channel, non blocking GUI refresh (openhab#2380) * Add new arping feature * Introduce separate things for tcp port device presence detection and ping presence detection, because a lot of users where confused that pings didn't work anymore if a tcp port is set. * Add discovery for tcp service devices and pingable devices. * Add/Improve comments * Introduce ExpiringCacheAsync * Use port knock on UDP port 5353 prior ARP ping to wakeup the iOS network stack * Bind DHCP sniffing to port 6767, if port 67 does not work. This way a user can iat least configure a port forward instead of not being able to use this function at all. * Only respond with cached values in handleCommand:RefreshType and do a refresh in the background. The actual ping command can halt the thread for up to 2 seconds which blocks UIs. * Remove wrong category labels. * Fix: Apparently a DHCPREQUEST message does not need to contain a REQUESTED_ADDRESS field. The source address of the udp packet can be used instead, in that case without bothering the user with an error message in the log. * Add lastseen channel incl DE translation. Rename time channel to latency channel: 'time' is missleading * Move not exported java classes to the "internal" package, except the constants class * Network: Add tests, adapt classes to be more easibly testable. Breaking change * The old thing "device" is now "pingdevice". This is not a real breaking change though, as all old configurations should still work because I map "device" to "pingdevice" in the handler factory. Signed-off-by: David Gräff <david.graeff@web.de>
Following features are added to the network binding:
configure a port forwarding to use this function. In the previous implementation the user
could not use this functionality if elevated privileges didn't work for him.
Breaking change
The old thing "device" is now "pingdevice". This is not a real breaking change though, as all old configurations should still work because I map "device" to "pingdevice" in the handler factory.
Auto discovery
The discovery searches for pingable devices as before but additionally also for service devices. At the moment it is only scanning for devices with open port 554 (windows file sharing).
Less configuration
Native ping, arp ping and dhcp sniffing are enabled by default. A feature detection is performed on thing creation. The user can disable native ping (and use the java .isReachable() method instead) in the binding configuration. ARP pings will be performed if the "arping" tool can be executed. DHCP sniffing will be enabled if the DHCP port can be opened.
More information without looking into the log
The things provide more information to the user.
Cache
The result of the device presence detection is cached for a small amount of time (2000ms, configurable). I considered the use of the new ExpiringCache class, but I'm not entirely happy with the API because I'm in need of a more asynchronous approach here. The goal is to not block the framework thread in handleCommand(..). At the moment this is realized with a new ExpiringCacheAsync class.
Fixes #2365
Fixes #1579
Fixes #1156
Fixes #2271
Fixes #2389
Fixes #2037