Minor extension to generic ip discovery

[holgerfriedrich]

Add new replacement $uuid in request frame.
This is needed for discovery of ipcamera devices.

Add new parameter listenPort (required for govee).

Add new parameter requestPlain as alternative to request (enhance readability of request for text/xml/json based req., govee and ipcamera)

Improve resource handling:

• autocloseable
• restrict to 5 addresses (moved to next PR)
• respect system.config.network.useOnlyOneAddress setting (moved to next PR)

Refs: #3936

[holgerfriedrich]

@Skinah FYI

[Skinah]

Thank you for the changes.

[andrewfg]

@holgerfriedrich the ZWay binding is probably a candidate to be suggested via your IP addon finder. => WDYT?

https://github.com/openhab/openhab-addons/tree/main/bundles/org.openhab.binding.zway#discovery

[holgerfriedrich]

@holgerfriedrich the ZWay binding is probably a candidate to be suggested via your IP addon finder. => WDYT?

https://github.com/openhab/openhab-addons/tree/main/bundles/org.openhab.binding.zway#discovery

Sorry, @andrewfg, I got you wrong in my first reply. Let's discuss this #3936. I will copy your comment to the other issue.

[holgerfriedrich]

@mherwege I think this PR is ready for review. It contains extensions needed for govee and ipcamera bindings. @stefan-hoehn just confirmed it working for govee and approved my PR openhab/openhab-addons#16109.

[andrewfg]

support / test if we go for zwave instead...

Just for the avoidance of doubt:

• The Z-Wave binding is based on a Z-Wave USB stick attached to the PC which will be suggested by my USB finder.
• The ZWay binding is based on a Z-Wave bridge server somewhere on the LAN which would be suggested by your IP finder.

[holgerfriedrich]

@mherwege Could you please take a look at this?
The PR contains minor extensions needed for govee and ipcamera.
Additionally, it improves the resource behavior (autocloseable DatagramChannel).

[holgerfriedrich]

@openhab/core-maintainers could you pls have a look? Auto-detect for 2 add-ons is depending on additional functions introduced in this PR:

• [govee] Addon info for suggestions openhab-addons#16109 (govee)
• [ipcamera] Addon info for suggestions openhab-addons#16090 (ipcamera)

Thanks a lot!

[mherwege]

In principle, this LGTM.

But I am wondering if we should not introduce use of #3981 at the same time. As we add requests being sent out to all interfaces, we should probably restrict the interfaces before getting too much into trouble.

[holgerfriedrich]

@mherwege thanks for looking into this.

But I am wondering if we should not introduce use of #3981 at the same time. As we add requests being sent out to all interfaces, we should probably restrict the interfaces before getting too much into trouble.

I had implemented a fallback to the main interface in case too many interfaces were found, but removed it for now to keep the PR cleaner.
I am in doubt that we would see an issue here because the requests are strictly sequential: we sent only one frame every few seconds (timeout is currently set to 5sec).

What we will need to think about - but for me it is not yet scope of this PR - is either shortening the timeouts or doing requests more or less in parallel. Otherwise we will not finish the scan before the wizard suggests plugins during when OH is installed.

[mherwege]

I had implemented a fallback to the main interface in case too many interfaces were found, but removed it for now to keep the PR cleaner.

Agree to keep it out of here, but that’s where it should probably be in sync with the work on #3981, so for a follow-up PR.

What we will need to think about - but for me it is not yet scope of this PR - is either shortening the timeouts or doing requests more or less in parallel. Otherwise we will not finish the scan before the wizard suggests plugins during when OH is installed.

Maybe define CompletableFutures with timeout for each request and start them in parallel. Agree we will need more throughput on this. We can delay initial configuration in the UI a bit, showing a preparing screen, within reason.

[holgerfriedrich]

@mherwege @andrewfg should we go forward merging this PR as it is?
Obviously, there is more work to do as described in #3936. This is just a first step.

[andrewfg]

I would suggest to use CompletableFuture as @mherwege suggests. Then the scans can all be executed in parallel on separate threads.

[mherwege]

I would suggest to use CompletableFuture as @mherwege suggests. Then the scans can all be executed in parallel on separate threads.

I tend to agree here. It now becomes easy to add discovery info to add-ons. If a number of them are added in the addon repo, suddenly we have code running a sequence of scans potentially each timing out. We should avoid having to come back to this quickly when discovery info gets added to the addon.xml files.

[andrewfg]

suddenly we have code running a sequence of scans potentially each timing out

In particular in OH core, there should be no long running processes on main threads.

[andrewfg]

I am thinking that the approach below would perhaps be appropriate..

openhab/openhab-addons#16315

[mherwege]

I think there even is a bigger theme for OH and networking to look at. I created an issue for it, but have had little response so far: #4047
Anyway, whatever direction that issue goes, already working on concurrency would be a big improvement.

[openhab-bot]

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

https://community.openhab.org/t/oh-4-1-1-startup-takes-more-than-an-hour/153310/33

[andrewfg]

@holgerfriedrich apropos threading .. to be a bit more helpful I would suggest refactoring your code lines 265..395 into a separate method, and at line #264 call that method via something like List<Future> myFutures .. add(scheduler.submit(() -> thatMethod())).

And on shutdown you should also do myFutures.forEach(f -> cancel());

[holgerfriedrich]

I agree that better handling of multiple addresses and interfaces is desired for IP discovery. Which solution to implement is not yet clear, maybe there will be a result out of #4047.

Parallelization is required as well when more add-ons start using this. Thanks, @mherwege and @andrewfg for pointing at CompletableFuture and implementations. There are some technical questions not yet clarified, e.g. how to make sure that discovery of different add-ons at the same time does not cause problems. Just imagine two add-ons using the same server port, etc...

I still think that both is out of scope of this PR and tracked #3936.

The PR adds new config options and has already been validated with the corresponding add-on.

[andrewfg]

@holgerfriedrich HERE is some code to do the scans asynchronously (not tested).

[wborn]

Thanks for the extension!

[andrewfg]

@holgerfriedrich shall I convert my code to a new PR?

[holgerfriedrich]

@holgerfriedrich shall I convert my code to a new PR?

Thanks, @andrewfg. I have created #4094 based on your example above. Hope you are fine with that.