-
-
Notifications
You must be signed in to change notification settings - Fork 428
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 IP broadcast add-on finder for suggestions #4036
Add IP broadcast add-on finder for suggestions #4036
Conversation
Thanks for the links. I guess I missed most of the discussions by being late to the party. 😉 Did you consider already detection by broadcast? As I do not want to interfere with your current development, perhaps you could consider the best way of adding broadcast support, if this is even feasible, and include this? The current implementation in this PR was rather quickly made from the Danfoss Air Unit discovery, and I'm not even sure if the API used is the most suitable one for the job nowadays. Hence only published as a draft. |
NP, I would not even call it late to the party - OH 4.1.1 is just using this for KNX ;-) 2 others are already in the queue which will use multicast as well.... There is not much of current development right now, besides the PR mentioned above. We need to see what are the requirements from the addons. Using broadcast seems feasible to me. We have not considered it yet, as there was no need before. It is likely not to introduce any performance issues (as the request for a network scanning in #3936). Maybe we could try to merge #3943 first. Regarding the code: I have just quickly looked into into it. I do not see where you check for timeout (parameter |
For sure! This one is low priority for me, just wanted to give it a shot, since I was randomly looking through the Danfoss Air Unit discovery code in releation to something else. Right now it would only be required for that binding, and I don't imagine many new users having such a unit (Danfoss abandoned the product last year).
|
...scovery.addon.ip/src/main/java/org/openhab/core/config/discovery/addon/ip/IpAddonFinder.java
Outdated
Show resolved
Hide resolved
The nikohomecontrol binding would need it as well to discover version 1 of the system (version 2 can be discovered using mDNS). The discovery code in the binding uses IP broadcast for both versions. |
Waiting for #4094 as well before deciding how to proceed. |
dbf5341
to
ff9497e
Compare
I decided to rebase and publish, since this PR is still WIP. Either can go first. |
I am working on the wizard to select the Primary IP at the start of setup. This will mean the scan has to be done with the configured network if it changes, and the scan will have to run again. Therefore I believe you would need to use this listener as well: Line 50 in cbb458e
|
I have implemented this, but need to clarify:
|
I think it should be for both. In general I think we should avoid using all interfaces be default. |
At initial setup, there would be no broadcast address configured. My thought was to only have a step to pick the primary IP in the wizard. My expectation was that that would make the default broadcast address change as well. I agree, as soon as you explicitely configure the primary broadcast address, this will not have an impact anymore. I didn't tedt this, but was hoping this could be a way forward, limiting network traffic without introducing finder specific parameters that are initially not set. |
I still think it does in a way. It will not set the broadcast address, but it will change the result of getConfiguredBroadcast() as none will be set. See openhab-core/bundles/org.openhab.core/src/main/java/org/openhab/core/net/NetUtil.java Line 132 in 875ebaa
As long as no broadcast address is explicitly set, it should be the default one from the primary IP. Anyway it would probably be beneficial to also have a listener for changes in broadcast address. But that is beyond the scope of this PR, and not required in initial setup. |
d8d6330
to
c0910fb
Compare
I somehow missed your reply, so responding a bit late, sorry.
You are right, thanks. I checked the Now I got this result - without any network configuration:
after changing primary address:
I have pushed a commit with the code in the screenshot from Saturday. |
@mherwege - are you okay with the current state? |
I didn’t look at the final result in detail yet, but I am OK in principle. I had little time this week, but want to check it in the context of the setup wizard primary IP address PR I am preparing, but did not finish yet. |
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.
@jlaur I just noticed that the broadcast finder seems to interpret the parameter request
differently, i.e. it does not make use of pattern replacement. You implemented a new method buildByteArray
instead of reusing buildRequestArray
. Parameter requestPlain
is not used at all.
Is there any specific reason behind this?
Otherwise LGTM.
Probably not a good reason. I think the only reason was that I didn't quite understand it, and perhaps wrongly concluded that it didn't appy to the broadcast use-case, also seeing that I found the multicast usage now: I think I totally missed the distinction between I will have another look and try to:
I guess first thing will be to decouple this: InetSocketAddress sock = (InetSocketAddress) channel.getLocalAddress(); Just to be sure, which IP address and port should be substituted for |
@jlaur
I see some overlap between Multicast and Broadcast. I'm not sure if we should merge to one function (probably too complex due to special handling for one of the cases), or at least try to share more code, e.g. for the parsing of possible responses. |
@holgerfriedrich, @mherwege - I wanted to resume this work yesterday, but could not get the suggestion finder to work at all (rebased and using latest snapshot). Today I did a clean install of latest snapshot version with the same result. Can you confirm or dismiss if this is broken for you as well? I checked recently merged PR's and found #4188 which could be related. All I see in logs with: openhab> log:set trace org.openhab.core.config.discovery.addon.ip
openhab> log:set trace org.openhab.core.config.discovery.addon is:
|
For me it still doesn't work with #4206. The work-around you mentioned didn't work either. I pulled the latest changes from main including your fix and built openhab-core\bundles\org.openhab.core.config.discovery.addon> mvn clean install I then copied this JAR into runtime/system/org/openhab/core/bundles/org.openhab.core.config.discovery.addon/4.2.0-SNAPSHOT, cleared cache and tmp and started openHAB again. But I still only see this:
Can you give me some guidance, since I assume the fix is working for you? |
042daeb
to
8e93ccf
Compare
Done, thanks. KNX is untested from my side, but it should still work since the call is just moved. Danfoss discovery still works, so it appears the
They are now using the same |
...scovery.addon.ip/src/main/java/org/openhab/core/config/discovery/addon/ip/IpAddonFinder.java
Outdated
Show resolved
Hide resolved
...scovery.addon.ip/src/main/java/org/openhab/core/config/discovery/addon/ip/IpAddonFinder.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Jacob Laursen <[email protected]>
Signed-off-by: Jacob Laursen <[email protected]>
Signed-off-by: Jacob Laursen <[email protected]>
Signed-off-by: Jacob Laursen <[email protected]>
Signed-off-by: Jacob Laursen <[email protected]>
Signed-off-by: Jacob Laursen <[email protected]>
8e93ccf
to
df22189
Compare
Add-on finders scan the network for devices supported by OH add-ons to suggest suitable add-ons. These are presented in the setup-wizard (and add-ons store). To limit network traffic, especially for IP broadcast and multicast scans, finders could limit the traffic to one subnet. This is especially relevant if the setup would be on servers with many network interfaces or when using Docker. This commit adds setting up a primary IP address to the setup, which will also default the broadcast address accordingly to the primary address. Querying add-on suggestions is delayed until after this step, and some delay is built into the process to allow suggestions finders to scan the network. See discussion in openhab/openhab-core#4036. --------- Also-by: Florian Hotze <[email protected]> Signed-off-by: Mark Herwege <[email protected]>
@jlaur I am trying to use this ipBroadcast discovery with the NikoHomeControl binding. I am using the following discovery method, but the matching does obviously not work. I actually think a regex would not even be possible in this case (although I tried), as some of the hex codes are not valid ASCII. So there may be a need for something like a matching with a binary response (written in hex).
|
I'm wondering if this would be sufficient to make it work? <match-properties>
<match-property>
<name>response</name>
<regex>0x44 0x3b</regex>
</match-property>
<match-property>
<name>response</name>
<regex>0x44 0x0c</regex>
</match-property>
<match-property>
<name>response</name>
<regex>0x44 0x0e</regex>
</match-property>
</match-properties> Otherwise, if you can propose a change that works for you, I'll be happy to include it. You can also create a PR towards my branch, but I might then have to look into GitHub permissions. Another option would be to try to get this merged soon, and enhance the response parsing for both multicast and broadcast in another iteration? |
Oh, wait, they are AND'ed, right? So it would require entirely duplicated |
Indeed, and I don't think the regex will work anyway as 0x0c is not an ASCII character. Also, the response byte array is much longer. The match is only on the first part. I am happy to get this merged first and then see how we can refine the matching logic for both. |
👍 Anything more to add from your side before asking a @openhab/add-ons-maintainers for a review? Cc @holgerfriedrich |
@jlaur Nothing from my side anymore for this PR. I believe follow-up PR's should:
|
@holgerfriedrich - gentle ping, can you check if this change looks good to you? |
@jlaur It's on my list. I will check as soon... |
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.
LGTM, thank you!
Lets add "adapting the log levels" to the list of TODOs (further work, not scope of this PR).
DEBUG does not even show which add-ons are probed.
I tested KNX, it is still working.
@holgerfriedrich - thanks! Can you add the "4.2" milestone? |
Extends #3920 to support broadcast-based scanning as well.
The broadcast implementation is based on the discovery implementation in the Danfoss Air Unit binding:
https://github.com/openhab/openhab-addons/blob/dd47b64fbb8865cd3b83ba782a79a7dc7286048f/bundles/org.openhab.binding.danfossairunit/src/main/java/org/openhab/binding/danfossairunit/internal/discovery/DanfossAirUnitDiscoveryService.java#L88-L151