Service to suggest addons via generic IP scan #3920

holgerfriedrich · 2023-12-13T00:35:20Z

This is the implementation of a generic IP-based add-on suggestion finder.
It will try to discover add-ons by scanning the IP network with specific packages.

The description is taken from the metadata.

At startup, a single scan is done in a separate thread.

The finder can be disabled, as it actively sends packets to the network which might not be desired.

Example configuration:
for KNX we want to send a multicast package to 224.0.23.12 port 3671.

The section in addon.xml looks like this:

		<discovery-method>
			<service-type>ip</service-type>
			<match-properties>
				<match-property>
					<name>type</name>
					<regex>ip_multicast</regex>
				</match-property>
				<match-property>
					<name>dest_ip</name>
					<regex>224.0.23.12</regex>
				</match-property>
				<match-property>
					<name>dest_port</name>
					<regex>3671</regex>
				</match-property>
				<match-property>
					<name>request</name>
					<regex>0x06 0x10 0x02 0x01 0x00 0x0e 0x08 0x01 $src_ip $src_port</regex>
				</match-property>
				<match-property>
					<name>timeout_ms</name>
					<regex>5000</regex>
				</match-property>
				<match-property>
					<name>response</name>
					<regex>any</regex>
				</match-property>
			</match-properties>
		</discovery-method>

Refs: #3912

mherwege · 2023-12-13T16:17:15Z

> <discovery-method>
> 			<service-type>ip</service-type>
> 			<match-properties>
> 				<match-property>
> 					<name>type</name>
> 					<regex>ip_multicast</regex>
> 				</match-property>
> 				<match-property>
> 					<name>dest_ip</name>
> 					<regex>224.0.23.12</regex>
> 				</match-property>
> 				<match-property>
> 					<name>dest_port</name>
> 					<regex>3671</regex>
> 				</match-property>
> 				<match-property>
> 					<name>request</name>
> 					<regex>0x06 0x10 0x02 0x01 0x00 0x0e 0x08 0x01 $src_ip $src_port</regex>
> 				</match-property>
> 				<match-property>
> 					<name>timeout_ms</name>
> 					<regex>5000</regex>
> 				</match-property>
> 				<match-property>
> 					<name>response</name>
> 					<regex>any</regex>
> 				</match-property>
> 			</match-properties>
> 		</discovery-method>

It feels to me like we are trying to shoe-horn the parameters for this into the existing schema. You may have noticed that the mdns discovery has an extra parameter in the schema. I would suggest to extend the schema for this.
Also, I don't like using a field called regex and filling it with something else.

I think you should keep the check for the return value to be a pure regex (you can express hex numbers in a regex as well). All the others are not matching criteria, but input parameters.

@andrewfg What do you think? In hindsight, we should probably have defined the mdnsServiceType parameter in the schema differently to make it more generalizable, something like:

<discovery-method>
 	<service-type>mdns</service-type>
 	<parameters>
 	 	<parameter>
 	 	 	<name>mdnsServiceType</name>
 	 	 	<value>...</value>
 	 	</parameter>
  	</parameters>
 	<match-properties>
		<match-property>
 			<name>...</name>
 			<regex>...</regex>
 		</match-property>
 	</match-properties>
</discovery-method>

That would be extendable without modifying the schema.

mherwege · 2023-12-13T16:28:34Z

At startup, a single scan is done in a separate thread.

Shouldn't we run this on a schedule and repeat it at regular intervals? The interval could be another configuration (next to switching this finder on/off completely).

...scovery.addon.ip/src/main/java/org/openhab/core/config/discovery/addon/ip/IpAddonFinder.java

holgerfriedrich · 2023-12-13T17:06:03Z

Thanks, Mark, for looking into this.

I wanted to see it working first, changing the schema is probably something to consider, as the regex for input seems wrong to me as well.

I am struggling a bit putting the scan from the getter function to task scheduled from the ctor... The list of candidates is now empty. I need to check later.

andrewfg · 2023-12-13T18:58:18Z

@holgerfriedrich yup, the way that you mis- use the schema is definitely a horrible hack; but as a proof of concept this approach makes sense; however I still wonder if there are other add-ons other than KNX where such a network probe is useful. ??

andrewfg · 2023-12-13T19:04:08Z

That would be extendable without modifying the schema.

@mherwege we would still have to modify (extend) the schema to support the new parameter(s) elements. But I get what you are saying about the extensibility. But we should wait until after v4.1 for this. Let is see whether other finders require a yet more sophisticated model. Note my draft for the USB device related bindings #3922 would not need extra parameters.

holgerfriedrich · 2023-12-14T00:30:07Z

Proof of concept is working,

holgerfriedrich · 2023-12-14T00:40:01Z

At startup, a single scan is done in a separate thread.

Shouldn't we run this on a schedule and repeat it at regular intervals? The interval could be another configuration (next to switching this finder on/off completely).

KNX is not something you turn on and off, so I think it is fine to trigger it once at startup.
Likely this is desired for other bindings.
We need to analyze first which devices we want to detect using this approach.

holgerfriedrich · 2023-12-14T00:44:41Z

we should probably have defined the mdnsServiceType parameter in the schema differently to make it more generalizable, something like:

<discovery-method>
 	<service-type>mdns</service-type>
 	<parameters>
 	 	<parameter>
 	 	 	<name>mdnsServiceType</name>
 	 	 	<value>...</value>

This is probably a good idea.... Should we change it before this gets adopted in more places like the marketplace?

mherwege · 2023-12-14T06:47:06Z

we should probably have defined the mdnsServiceType parameter in the schema differently to make it more generalizable, something like:
<discovery-method>
 	<service-type>mdns</service-type>
 	<parameters>
 	 	<parameter>
 	 	 	<name>mdnsServiceType</name>
 	 	 	<value>...</value>
This is probably a good idea.... Should we change it before this gets adopted in more places like the marketplace?

That is also my thought. Better change it now before release than having to change it later and have a larger impact.
It still requires quite some changes though:

Changes in core to AddonInfo and the mDNS finder
Update to the schema and upload the new schema
PR in add-ons repo to change all mDNS discovery methods

If we wait until after release, we could keep the existing mdnsServiceType for backward compatibility in the code but encourage the use of the parameter syntax.

@andrewfg @kaikreuzer What do you think? This is not an immediate need, but will avoid some hassle in the future? Is there still time to do this change?

J-N-K · 2023-12-14T06:56:08Z

Please do those changes before the release. In general I think this should also increase the version, but @kaikreuzer can probably comment on why we haven't done so in the past.

mherwege · 2023-12-14T07:28:38Z

@andrewfg I will create a PR to adapt core.

andrewfg · 2023-12-14T11:02:15Z

@mherwege Ok I will check your core PR

I will also do a PR for the developer docs

mherwege · 2023-12-14T11:12:25Z

Addon PR also created, changing all mDNS discovery definitions in the addon.xml files: openhab/openhab-addons#16060

holgerfriedrich · 2023-12-16T02:53:01Z

Adapted to #3924, will not work without

kaikreuzer · 2023-12-16T10:06:18Z

In general I think this should also increase the version, but @kaikreuzer can probably comment on why we haven't done so in the past.

Afair, we did backward compatible changes in the past and therefore kept the version, so that there wasn't any need to update the xmls in the add-ons (and therefore keep e.g. the ones in the marketplace compatible). With breaking changes, we should probably indeed increase the version. But for #3924, it is imho ok to stick to the version as there hasn't been a release yet.

Signed-off-by: Holger Friedrich <[email protected]>

holgerfriedrich · 2023-12-16T11:11:48Z

@mherwege @andrewfg thanks a lot for your work on generalizing the schema!

I have rebased to current master which contains the new schema.

@mherwege @kaikreuzer
This is now more or less at the level of a proof of concept. However, I would very much appreciate if I could get it into 4.1 to detect KNX installations. There has been more activity to make the whole chain working: discovery service to detect interfaces in the binding itself, and process suggestion finder which can already identify a running knxd deamon. The only piece missing is this one.
I know that this somehow needs to be balanced with the approach of only putting mature and already generalized features into core.

Can you give advice what are the most important pieces missing right now?

How do I care for the activation/deactivation? I just extended addons.xml in org.openhab.core and added the i18n strings until now.
Do you think it is important to clean the pattern matching for received packets? Right now it works only with the first received frame and the pattern ".*".

@kaikreuzer What it the time frame this needs to be done? Are we too late for 4.1 already?

kaikreuzer · 2023-12-16T11:37:11Z

@holgerfriedrich I am not a big fan of IP scans and all sorts of side-effects might occur here. I'd therefore prefer to not merge this as a PoC in the very last minute, but rather test it thoroughly after the 4.1 release is done.

holgerfriedrich · 2023-12-16T11:44:01Z

@holgerfriedrich I am not a big fan of IP scans and all sorts of side-effects might occur here. I'd therefore prefer to not merge this as a PoC in the very last minute, but rather test it thoroughly after the 4.1 release is done.

I get you point.... Though, IP scan sounds a bit more aggressive than it looks to me in this case: A single UDP frame to a well defined multicast addess and port. One frame sent during startup. And we can even disable it.
We are going a big step towards better experience to new users - just wanted to be part of this with KNX as well.

holgerfriedrich · 2023-12-16T18:36:36Z

Just a note, I was not able to deactivate any of the finders. I deactivated via UI, restarted, and everything was discovered and shown in the wizard (avm, etc ) - what did I wrong?

Did you wait long enough before restarting? It can take 1 minute for the change to be effective.

ok, confirmed, did the restart too early.... it works when I wait longer.

Signed-off-by: Holger Friedrich <[email protected]>

mherwege · 2023-12-17T09:22:56Z

LGTM with refinements to be done for other IP finder scenarios. But this should not stop making this availalble already.

holgerfriedrich · 2023-12-17T09:58:04Z

LGTM with refinements to be done for other IP finder scenarios. But this should not stop making this availalble already.

@kaikreuzer Are we too late? Did you already trigger the milestone build?
Please also merge openhab/openhab-addons#16055, thanks!

kaikreuzer

Thanks - just some logging improvement suggestions, then we are good to merge.

...scovery.addon.ip/src/main/java/org/openhab/core/config/discovery/addon/ip/IpAddonFinder.java

Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Holger Friedrich <[email protected]>

holgerfriedrich · 2023-12-17T11:51:09Z

Thanks - just some logging improvement suggestions, then we are good to merge.

done, thanks

Signed-off-by: Holger Friedrich <[email protected]>

kaikreuzer

Thanks!

openhab-bot · 2023-12-19T08:16:46Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/how-to-get-an-addon-to-show-up-in-the-new-addon-suggestion-finder-list/152127/6

stefan-hoehn · 2023-12-23T14:38:10Z

@mherwege I looked into this because currently the Govee binding isn't detected. After doing some research these are the ways to find out if a Govee device is on the network

by pinging "ping -c 1 239.255.255.250"
or by sending a "{"msg": {"cmd": "scan", "data": {"account_topic": "reserve"}}}" via Multicast ip 239.255.255.250:4001 and receiving back an answer on our port: 4002 (like the binding does).

Do you see any way based on this implementation to auto discover the binding ?

e.g.

``
Note that request is on 4001 but the answer would be on 4002..

<discovery-method>
			<service-type>ip</service-type>
			<discovery-parameters>
				<discovery-parameter>
					<name>type</name>
					<value>ipMulticast</value>
				</discovery-parameter>
				<discovery-parameter>
					<name>destIp</name>
					<value>239.255.255.250</value>
				</discovery-parameter>
				<discovery-parameter>
					<name>destPort</name>
					<value>4001</value>
				</discovery-parameter>
				<discovery-parameter>
					<name>request</name>
					<value>{"msg": {"cmd": "scan", "data": {"account_topic": "reserve"}}}</value>
				</discovery-parameter>
				<discovery-parameter>
					<name>timeoutMs</name>
					<value>5000</value>
				</discovery-parameter>
			</discovery-parameters>
			<match-properties>
				<match-property>
                                         <!-- not sure what to put in here ?? -->
					<name>response</name>
					<regex>.*</regex>
				</match-property>
			</match-properties>
		</discovery-method>
	</discovery-methods>

mherwege · 2023-12-23T14:43:01Z

Not at this point, but see #3936. We are looking to document different cases so we can make sure a more complete solution gets implementeren for 4.2.

stefan-hoehn · 2023-12-23T14:48:01Z

Thanks, let me know when I can try out something for #3936 or support you there.

holgerfriedrich requested a review from a team as a code owner December 13, 2023 00:35

holgerfriedrich changed the title ~~Service to suggest addons via generic IP scan~~ [WIP] Service to suggest addons via generic IP scan Dec 13, 2023

holgerfriedrich mentioned this pull request Dec 13, 2023

[knx] Metadata for discovery openhab/openhab-addons#16055

Merged

mherwege reviewed Dec 13, 2023

View reviewed changes

...scovery.addon.ip/src/main/java/org/openhab/core/config/discovery/addon/ip/IpAddonFinder.java Outdated Show resolved Hide resolved

holgerfriedrich force-pushed the pr-finder-ip branch from 46578d1 to 02dc3d7 Compare December 14, 2023 00:25

mherwege mentioned this pull request Dec 14, 2023

Modified finder discovery schema to make future finders easier to create #3924

Merged

mherwege mentioned this pull request Dec 15, 2023

Service to suggest addons based on running processes #3904

Merged

holgerfriedrich force-pushed the pr-finder-ip branch from 02dc3d7 to b63db40 Compare December 16, 2023 02:49

holgerfriedrich added 3 commits December 16, 2023 11:30

Service to suggest addons via generic IP scan

83c97ae

Signed-off-by: Holger Friedrich <[email protected]>

exception handling, delayed init

6e4f471

Signed-off-by: Holger Friedrich <[email protected]>

adapt to openhab#3924

ded7540

Signed-off-by: Holger Friedrich <[email protected]>

holgerfriedrich force-pushed the pr-finder-ip branch from b63db40 to ded7540 Compare December 16, 2023 10:30

holgerfriedrich added 3 commits December 16, 2023 20:23

settings and i18n

e4b5a9b

Signed-off-by: Holger Friedrich <[email protected]>

CamelCase

16b7032

Signed-off-by: Holger Friedrich <[email protected]>

CamelCase

73dc742

Signed-off-by: Holger Friedrich <[email protected]>

holgerfriedrich force-pushed the pr-finder-ip branch from da9b908 to 73dc742 Compare December 17, 2023 08:18

reviewer comments

acd6195

Signed-off-by: Holger Friedrich <[email protected]>

mherwege approved these changes Dec 17, 2023

View reviewed changes

holgerfriedrich changed the title ~~[WIP] Service to suggest addons via generic IP scan~~ Service to suggest addons via generic IP scan Dec 17, 2023

kaikreuzer requested changes Dec 17, 2023

View reviewed changes

holgerfriedrich and others added 6 commits December 17, 2023 12:48

reviewer comment, logging

cfcbb79

Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Holger Friedrich <[email protected]>

reviewer comment, logging

179132e

Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Holger Friedrich <[email protected]>

reviewer comment, logging

468e01b

Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Holger Friedrich <[email protected]>

reviewer comment, logging

5deb85b

Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Holger Friedrich <[email protected]>

reviewer comment, logging

73b166f

Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Holger Friedrich <[email protected]>

reviewer comment, logging

fc38fdb

Co-authored-by: Kai Kreuzer <[email protected]> Signed-off-by: Holger Friedrich <[email protected]>

spotless

933c9e6

Signed-off-by: Holger Friedrich <[email protected]>

kaikreuzer approved these changes Dec 17, 2023

View reviewed changes

kaikreuzer merged commit 8bed621 into openhab:main Dec 17, 2023
2 of 3 checks passed

kaikreuzer added this to the 4.1 milestone Dec 17, 2023

kaikreuzer added the enhancement An enhancement or new feature of the Core label Dec 17, 2023

holgerfriedrich mentioned this pull request Dec 19, 2023

Extend service to suggest addons via generic IP scan #3936

Open

11 tasks

holgerfriedrich deleted the pr-finder-ip branch December 20, 2023 23:32

jlaur mentioned this pull request Jan 13, 2024

Add IP broadcast add-on finder for suggestions #4036

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service to suggest addons via generic IP scan #3920

Service to suggest addons via generic IP scan #3920

holgerfriedrich commented Dec 13, 2023 •

edited

Loading

mherwege commented Dec 13, 2023 •

edited

Loading

mherwege commented Dec 13, 2023 •

edited

Loading

holgerfriedrich commented Dec 13, 2023

andrewfg commented Dec 13, 2023

andrewfg commented Dec 13, 2023

holgerfriedrich commented Dec 14, 2023

holgerfriedrich commented Dec 14, 2023

holgerfriedrich commented Dec 14, 2023

mherwege commented Dec 14, 2023

J-N-K commented Dec 14, 2023

mherwege commented Dec 14, 2023

andrewfg commented Dec 14, 2023

mherwege commented Dec 14, 2023

holgerfriedrich commented Dec 16, 2023

kaikreuzer commented Dec 16, 2023

holgerfriedrich commented Dec 16, 2023

kaikreuzer commented Dec 16, 2023

holgerfriedrich commented Dec 16, 2023

holgerfriedrich commented Dec 16, 2023

mherwege commented Dec 17, 2023

holgerfriedrich commented Dec 17, 2023 •

edited

Loading

kaikreuzer left a comment

holgerfriedrich commented Dec 17, 2023

kaikreuzer left a comment

openhab-bot commented Dec 19, 2023

stefan-hoehn commented Dec 23, 2023 •

edited

Loading

mherwege commented Dec 23, 2023

stefan-hoehn commented Dec 23, 2023

Service to suggest addons via generic IP scan #3920

Service to suggest addons via generic IP scan #3920

Conversation

holgerfriedrich commented Dec 13, 2023 • edited Loading

mherwege commented Dec 13, 2023 • edited Loading

mherwege commented Dec 13, 2023 • edited Loading

holgerfriedrich commented Dec 13, 2023

andrewfg commented Dec 13, 2023

andrewfg commented Dec 13, 2023

holgerfriedrich commented Dec 14, 2023

holgerfriedrich commented Dec 14, 2023

holgerfriedrich commented Dec 14, 2023

mherwege commented Dec 14, 2023

J-N-K commented Dec 14, 2023

mherwege commented Dec 14, 2023

andrewfg commented Dec 14, 2023

mherwege commented Dec 14, 2023

holgerfriedrich commented Dec 16, 2023

kaikreuzer commented Dec 16, 2023

holgerfriedrich commented Dec 16, 2023

kaikreuzer commented Dec 16, 2023

holgerfriedrich commented Dec 16, 2023

holgerfriedrich commented Dec 16, 2023

mherwege commented Dec 17, 2023

holgerfriedrich commented Dec 17, 2023 • edited Loading

kaikreuzer left a comment

Choose a reason for hiding this comment

holgerfriedrich commented Dec 17, 2023

kaikreuzer left a comment

Choose a reason for hiding this comment

openhab-bot commented Dec 19, 2023

stefan-hoehn commented Dec 23, 2023 • edited Loading

mherwege commented Dec 23, 2023

stefan-hoehn commented Dec 23, 2023

holgerfriedrich commented Dec 13, 2023 •

edited

Loading

mherwege commented Dec 13, 2023 •

edited

Loading

mherwege commented Dec 13, 2023 •

edited

Loading

holgerfriedrich commented Dec 17, 2023 •

edited

Loading

stefan-hoehn commented Dec 23, 2023 •

edited

Loading