Add IP broadcast add-on finder for suggestions

[jlaur]

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

[holgerfriedrich]

Good to see more addons using the this finder. Currently in conflict with #3943. See also discussion regarding extending the IP finder: #3936

[jlaur]

Good to see more addons using the this finder. Currently in conflict with #3943. See also discussion regarding extending the IP finder: #3936

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.

[holgerfriedrich]

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 timeoutMs). AFAIR DatagramSocket::receive() is blocking. We do the scanning sequentially: binding by binding, IP address by IP address.

[jlaur]

Maybe we could try to merge #3943 first.

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).

Regarding the code: I have just quickly looked into into it. I do not see where you check for timeout (parameter timeoutMs). AFAIR DatagramSocket::receive() is blocking. We do the scanning sequentially: binding by binding, IP address by IP address.

timeoutMs is currently only used for setting the socket timeout, and yes, receive() is blocking. Maybe the same can be achieved with DatagramChannel (i.e. channel.socket().setBroadcast(true), then channel.configureBlocking(false)).

[mherwege]

Right now it would only be required for that binding

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.

[jlaur]

Waiting for #4094 as well before deciding how to proceed.

[jlaur]

Waiting for #4094 as well before deciding how to proceed.

I decided to rebase and publish, since this PR is still WIP. Either can go first.

[mherwege]

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: https://github.com/openhab/openhab-core/blob/cbb458e0c3c35f353954f82031c5cd1b3943759c/bundles/org.openhab.core/src/main/java/org/openhab/core/net/NetworkAddressChangeListener.java#L50

[jlaur]

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:

https://github.com/openhab/openhab-core/blob/cbb458e0c3c35f353954f82031c5cd1b3943759c/bundles/org.openhab.core/src/main/java/org/openhab/core/net/NetworkAddressChangeListener.java#L50

I have implemented this, but need to clarify:

• Is it needed for multicast? It seems that the multicast implementation already iterates through all interfaces.
• Or did you consider it only for broadcast implemented by this PR? Unfortunately it doesn't seem to be working, or at least is not addressing the main issue: changed broadcast address (without any changes in primary address): https://github.com/openhab/openhab-core/blob/cbb458e0c3c35f353954f82031c5cd1b3943759c/bundles/org.openhab.core/src/main/java/org/openhab/core/net/NetworkAddressChangeListener.java#L31-L53

[mherwege]

Is it needed for multicast? It seems that the multicast implementation already iterates through all interfaces.

I think it should be for both. In general I think we should avoid using all interfaces be default.

[mherwege]

changed broadcast address (without any changes in primary address)

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.

[jlaur]

changed broadcast address (without any changes in primary address)

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.

In network settings it doesn't work like that, the two options do not impact each other:

Of course the wizard could provide logic for this, but I don't think I should second-guess about this in context of this PR. Listening to onChanged or onPrimaryAddressChanged will not detect changes in broadcast address, so probably I should save that commit for another PR? This is what I tried, but didn't help when setting the broadcast address:

Is there anything more with regard to the broadcast add-finder you think can be done in scope of this PR?

The multicast and broadcast implementations are a bit inconsistent currently, because the multicast will work better by iterating all interfaces, while broadcast will not after 2nd commit. But this was already discussed, and hopefully both can be improved in follow-up PR's. We might be able to add a listener also for changes in broadcast address, or the wizard might set this implicitly when changing the primary interface.

[mherwege]

In network settings it doesn't work like that, the two options do not impact each other

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 https://github.com/openhab/openhab-core/blob/875ebaaef7eba2420469ba57c44dbc42eee19d76/bundles/org.openhab.core/src/main/java/org/openhab/core/net/NetUtil.java#L132. 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.

[jlaur]

I somehow missed your reply, so responding a bit late, sorry.

As long as no broadcast address is explicitly set, it should be the default one from the primary IP.

You are right, thanks. I checked the NetUtils methods again and tested my code again, and now I found it working. I don't know what went wrong last time, perhaps I even had not copied the new JAR in place or cleared the cache - it was timestamped Friday.

Now I got this result - without any network configuration:

2024-04-18 16:55:42.660 [TRACE] [fig.discovery.addon.ip.IpAddonFinder] - Checking candidate: binding-danfossairunit
2024-04-18 16:55:42.784 [DEBUG] [fig.discovery.addon.ip.IpAddonFinder] - Starting broadcast scan with address 192.168.129.255
2024-04-18 16:55:43.285 [TRACE] [fig.discovery.addon.ip.IpAddonFinder] - Checking candidate: binding-ipcamera

after changing primary address:

2024-04-18 16:57:37.883 [TRACE] [fig.discovery.addon.ip.IpAddonFinder] - Checking candidate: binding-danfossairunit
2024-04-18 16:57:37.995 [DEBUG] [fig.discovery.addon.ip.IpAddonFinder] - Starting broadcast scan with address 192.168.0.255
2024-04-18 16:57:37.998 [DEBUG] [fig.discovery.addon.ip.IpAddonFinder] - Suggested add-on found: binding-danfossairunit
2024-04-18 16:57:38.499 [TRACE] [fig.discovery.addon.ip.IpAddonFinder] - Checking candidate: binding-ipcamera

I have pushed a commit with the code in the screenshot from Saturday.

[jlaur]

@mherwege - are you okay with the current state?

[mherwege]

@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.

[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?

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 buildRequestArrayPlain and buildRequestArray are coupled with DatagramChannel. Also I guess I would prefer not to over-engineer it, so at least I would like to understand what a use-case could look like, since there currently isn't any.

I found the multicast usage now: https://github.com/openhab/openhab-addons/blob/9bcb33818876f78858abe28da6f631a762afb451/bundles/org.openhab.binding.knx/src/main/resources/OH-INF/addon/addon.xml#L44-L47

I think I totally missed the distinction between request and requestPlain and just picked request as it seemed right.

I will have another look and try to:

• Implement support for requestPlain as well.
• Use the same approach for dynamic replacement of variables $srcIp, $srcPort and $uuid.

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 $srcIp and $srcPort?

[holgerfriedrich]

@jlaur It seems that we could change the interface of the generators to buildRequestArray[Plain](SocketAddress, String). Then DatagramSocket.getLocalSocketAddress() would probably give the correct sender IP and port for DatagramSocket and DatagramChannel.getLocalAddress() for DatagramChannel.

srcIp is just the sender IP, srcPort the dynamically selected port used for sending.

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.

[jlaur]

@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:

2024-05-01 21:43:24.398 [TRACE] [ery.addon.process.ProcessAddonFinder] - ProcessAddonFinder::getSuggestedAddons
2024-05-01 21:43:24.485 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: binding-deconz
2024-05-01 21:43:24.486 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: binding-onewire
2024-05-01 21:43:24.495 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: persistence-jdbc-postgresql
2024-05-01 21:43:24.496 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: binding-knx
2024-05-01 21:43:24.497 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: persistence-influxdb
2024-05-01 21:43:24.498 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: binding-growatt
2024-05-01 21:43:24.498 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: misc-metrics
2024-05-01 21:43:24.499 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: persistence-jdbc-mysql
2024-05-01 21:43:24.501 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: binding-mqtt
2024-05-01 21:43:24.503 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: persistence-mongodb
2024-05-01 21:43:24.503 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: persistence-jdbc-mariadb

[mherwege]

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.

I just installed a new snapshot and I can confirm the bug. If you go in Settings -> Add-on Management and switch on advanced settings, you can switch off a finder, save, go back and switch back on again. The finder will load and work. This is obviously not what it was meant to be. I will look at where this goes wrong. The ProcessAddonFinder is the only one which is not deinstallable, hence it shows in the log.

[mherwege]

Created https://github.com/openhab/openhab-core/pull/4206 that should fix this.

[jlaur]

Created #4206 that should fix this.

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 org.openhab.core.config.discovery.addon-4.2.0-SNAPSHOT.jar:

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:

2024-05-02 20:14:10.068 [DEBUG] [scovery.addon.AddonSuggestionService] - bundle org.openhab.core.config.discovery.addon:4.2.0.202405021809 (171)[org.openhab.core.config.discovery.addon.AddonSuggestionService(92)] : ImmediateComponentHolder configuration deleted for pid org.openhab.addons
2024-05-02 20:14:10.069 [DEBUG] [scovery.addon.AddonSuggestionService] - bundle org.openhab.core.config.discovery.addon:4.2.0.202405021809 (171)[org.openhab.core.config.discovery.addon.AddonSuggestionService(92)] : Querying state active
2024-05-02 20:14:10.069 [DEBUG] [scovery.addon.AddonSuggestionService] - bundle org.openhab.core.config.discovery.addon:4.2.0.202405021809 (171)[org.openhab.core.config.discovery.addon.AddonSuggestionService(92)] : Querying state active
2024-05-02 20:14:10.069 [DEBUG] [scovery.addon.AddonSuggestionService] - bundle org.openhab.core.config.discovery.addon:4.2.0.202405021809 (171)[org.openhab.core.config.discovery.addon.AddonSuggestionService(92)] : getting modified: modified
2024-05-02 20:14:10.069 [DEBUG] [scovery.addon.AddonSuggestionService] - bundle org.openhab.core.config.discovery.addon:4.2.0.202405021809 (171)[org.openhab.core.config.discovery.addon.AddonSuggestionService(92)] : Locating method modified in class org.openhab.core.config.discovery.addon.AddonSuggestionService
2024-05-02 20:14:10.070 [DEBUG] [scovery.addon.AddonSuggestionService] - bundle org.openhab.core.config.discovery.addon:4.2.0.202405021809 (171)[org.openhab.core.config.discovery.addon.AddonSuggestionService(92)] : Declared Method org.openhab.core.config.discovery.addon.AddonSuggestionService.modified([interface org.osgi.service.component.ComponentContext]) not found
2024-05-02 20:14:10.070 [DEBUG] [scovery.addon.AddonSuggestionService] - bundle org.openhab.core.config.discovery.addon:4.2.0.202405021809 (171)[org.openhab.core.config.discovery.addon.AddonSuggestionService(92)] : Found modified method: public void org.openhab.core.config.discovery.addon.AddonSuggestionService.modified(java.util.Map)
2024-05-02 20:14:10.070 [DEBUG] [scovery.addon.AddonSuggestionService] - bundle org.openhab.core.config.discovery.addon:4.2.0.202405021809 (171)[org.openhab.core.config.discovery.addon.AddonSuggestionService(92)] : invoking modified: modified: parameters [org.apache.felix.scr.impl.helper.ReadOnlyDictionary]
2024-05-02 20:14:10.070 [DEBUG] [scovery.addon.AddonSuggestionService] - bundle org.openhab.core.config.discovery.addon:4.2.0.202405021809 (171)[org.openhab.core.config.discovery.addon.AddonSuggestionService(92)] : invoked modified: modified
2024-05-02 20:14:10.070 [DEBUG] [scovery.addon.AddonSuggestionService] - bundle org.openhab.core.config.discovery.addon:4.2.0.202405021809 (171)[org.openhab.core.config.discovery.addon.AddonSuggestionService(92)] : No change in target property for dependency $000: currently registered: true
2024-05-02 20:14:10.071 [DEBUG] [scovery.addon.AddonSuggestionService] - bundle org.openhab.core.config.discovery.addon:4.2.0.202405021809 (171)[org.openhab.core.config.discovery.addon.AddonSuggestionService(92)] : No change in target property for dependency $001: currently registered: true
2024-05-02 20:14:10.071 [DEBUG] [scovery.addon.AddonSuggestionService] - bundle org.openhab.core.config.discovery.addon:4.2.0.202405021809 (171)[org.openhab.core.config.discovery.addon.AddonSuggestionService(92)] : No change in target property for dependency AddonFinder: currently registered: true
2024-05-02 20:14:10.071 [DEBUG] [scovery.addon.AddonSuggestionService] - bundle org.openhab.core.config.discovery.addon:4.2.0.202405021809 (171)[org.openhab.core.config.discovery.addon.AddonSuggestionService(92)] : No change in target property for dependency AddonFinderService: currently registered: true
2024-05-02 20:14:10.071 [DEBUG] [scovery.addon.AddonSuggestionService] - bundle org.openhab.core.config.discovery.addon:4.2.0.202405021809 (171)[org.openhab.core.config.discovery.addon.AddonSuggestionService(92)] : No change in target property for dependency AddonInfoProvider: currently registered: true
2024-05-02 20:14:10.071 [DEBUG] [scovery.addon.AddonSuggestionService] - bundle org.openhab.core.config.discovery.addon:4.2.0.202405021809 (171)[org.openhab.core.config.discovery.addon.AddonSuggestionService(92)] : No change in target property for dependency osgi.ds.satisfying.condition: currently registered: true
2024-05-02 20:14:10.071 [DEBUG] [scovery.addon.AddonSuggestionService] - bundle org.openhab.core.config.discovery.addon:4.2.0.202405021809 (171)[org.openhab.core.config.discovery.addon.AddonSuggestionService(92)] : Querying state active
2024-05-02 20:14:47.310 [TRACE] [ery.addon.process.ProcessAddonFinder] - ProcessAddonFinder::getSuggestedAddons
2024-05-02 20:14:47.375 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: persistence-jdbc-mysql
2024-05-02 20:14:47.376 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: binding-onewire
2024-05-02 20:14:47.376 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: persistence-influxdb
2024-05-02 20:14:47.377 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: binding-deconz
2024-05-02 20:14:47.377 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: misc-metrics
2024-05-02 20:14:47.377 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: persistence-jdbc-postgresql
2024-05-02 20:14:47.377 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: persistence-jdbc-mariadb
2024-05-02 20:14:47.378 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: binding-knx
2024-05-02 20:14:47.379 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: persistence-mongodb
2024-05-02 20:14:47.379 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: binding-mqtt
2024-05-02 20:14:47.379 [TRACE] [ery.addon.process.ProcessAddonFinder] - Checking candidate: binding-growatt

Can you give me some guidance, since I assume the fix is working for you?

[jlaur]

It seems that we could change the interface of the generators to buildRequestArray[Plain](SocketAddress, String). Then DatagramSocket.getLocalSocketAddress() would probably give the correct sender IP and port for DatagramSocket and DatagramChannel.getLocalAddress() for DatagramChannel.

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 SocketAddress returned by DatagramSocket.getLocalSocketAddress() can be cast to InetSocketAddress as well.

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.

They are now using the same buildRequestArray and buildRequestArrayPlain. scanBroadcast is not that many lines besides parameter validation, but if you have any further improvement ideas, please let me know and I can adapt. 🙂

[mherwege]

@jlaur see https://github.com/openhab/openhab-webui/pull/2565

[mherwege]

@jlaur I am trying to use this ipBroadcast discovery with the NikoHomeControl binding. The challenge I have is that I need to check for a very specific return byte array. At the moment, there only is a straight equality compare. The same is true for ipMulticast (although it is different in how the response string is expected to be). The logic for the comparison is here: https://github.com/openhab/openhab-addons/blob/283ae59bff2a6bef4872ecb8c046ea0dbc34edb2/bundles/org.openhab.binding.nikohomecontrol/src/main/java/org/openhab/binding/nikohomecontrol/internal/protocol/NikoHomeControlDiscover.java#L116

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).

			<discovery-methods>
				<discovery-method>
					<service-type>ip</service-type>
					<discovery-parameters>
						<discovery-parameter>
							<name>type</name>
							<value>ipBroadcast</value>
						</discovery-parameter>
						<discovery-parameter>
							<name>destPort</name>
							<value>10000</value>
						</discovery-parameter>
						<discovery-parameter>
							<name>request</name>
							<value>0x44</value>
						</discovery-parameter>
						<discovery-parameter>
							<name>timeoutMs</name>
							<value>5000</value>
						</discovery-parameter>
					</discovery-parameters>
					<match-properties>
						<match-property>
							<name>response</name>
							<regex>\x44[\x3b\x0c\x0e].*</regex>
						</match-property>
					</match-properties>
				</discovery-method>
			</discovery-methods>

[jlaur]

  			<match-properties>
  				<match-property>
  					<name>response</name>
  					<regex>\x44[\x3b\x0c\x0e].*</regex>
  				</match-property>
  			</match-properties>

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?

[jlaur]

I'm wondering if this would be sufficient to make it work?

Oh, wait, they are AND'ed, right? So it would require entirely duplicated discovery-methods without changing the code.

[mherwege]

I'm wondering if this would be sufficient to make it work?

Oh, wait, they are AND'ed, right? So it would require entirely duplicated discovery-methods without changing the code.

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.

[jlaur]

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

[mherwege]

@jlaur Nothing from my side anymore for this PR.

I believe follow-up PR's should:

• Allow response matching logic
• Parallelize the scanning