[danfossairunit] Use serial number as valid Thing ID in discovery

[jlaur]

This eliminates inbox results for already configured Things.

Additionally, the suggested Thing ID is now compliant with .things file configuration by avoiding "-" as first character in ID. See openhab/openhab-core#4652.

A downside is that discovery now need to be able to connect to the unit to get the serial number. This is not possible while another connection to the unit is already established. However, the disovered Thing would not be able to come online anyway while such other connection exists.

[lsiepel]

Thanks, LGTM

Would be nice if @pravussum could comment.

[pravussum]

Hej, the code changes LGTM so far.
I'm just wondering about two things:

• what happens to existing things? Will a second, autodiscovered thing appear in the Inbox beside an already configured air unit thing created using the "old way"?
• will der autodiscovery broadcast try to request the serial number from the air unit over and over again (only to find that its the same and probably failing in the process once the thing holds a connection to the air unit)

[lsiepel]

Hej, the code changes LGTM so far. I'm just wondering about two things:

• what happens to existing things? Will a second, autodiscovered thing appear in the Inbox beside an already configured air unit thing created using the "old way"?

No, they are from now on matched by the new representation-property for both old and new.

• will der autodiscovery broadcast try to request the serial number from the air unit over and over again (only to find that its the same and probably failing in the process once the thing holds a connection to the air unit)

Only when manually discovering items. There does not seem to be a backgroudn discovery proces for this binding.

[jlaur]

Hej, the code changes LGTM so far.

Thanks for the review, and the others as well. 👍

• what happens to existing things? Will a second, autodiscovered thing appear in the Inbox beside an already configured air unit thing created using the "old way"?

This is handled, since the Thing handler already sets the seriaNumber property. So no, when already having a unit configured with the same serial number as a discovered once, it will not be added to the inbox.

• will der autodiscovery broadcast try to request the serial number from the air unit over and over again (only to find that its the same and probably failing in the process once the thing holds a connection to the air unit)

That's a really good question! I will test this some more. I did test it for manually triggered discovery while already having a connection from another openHAB instance, and here it will obviously fail when trying to connect. I'm afraid the same might happen for manually triggered discovery when already having the Thing online. In this case the discovery will be delayed by the 5 second500 ms timeout, and the CCM will not be discovered. I will verify to be sure. I guess the only use-case for manual discovery after already having a configured Thing would be having multiple Air units, which is probably rare.

For auto-discovery I need to check how often this might happen.

I will get back after testing and thinking about how this might be improved.

[jlaur]

will der autodiscovery broadcast try to request the serial number from the air unit over and over again (only to find that its the same and probably failing in the process once the thing holds a connection to the air unit)

For auto-discovery I need to check how often this might happen.

Auto-discovery is triggered only on component activation and when configuration for the discovery service is changed. So during openHAB startup I observed the discovery to be performed successfully quite some time before thing Thing came online (different start levels). Also it will be triggered when adding/replacing a JAR for the binding.

So yes, it will request the serial number over and over again, but not very often. And no, it doesn't seem to be the case that it will fail the process under normal circumstances because OSGi component activation happens long before reaching the start level that will initialize handlers.

[jlaur]

So during openHAB startup I observed the discovery to be performed successfully quite some time before thing Thing came online (different start levels). Also it will be triggered when adding/replacing a JAR for the binding.

I think actually that's a result of a lacking background discovery implementation:
https://www.openhab.org/docs/concepts/discovery.html#background-discovery

The binding does a one-shot discovery rather than continuously performing discovery until asked to stop:

openhab-addons/bundles/org.openhab.binding.danfossairunit/src/main/java/org/openhab/binding/danfossairunit/internal/discovery/DanfossAirUnitDiscoveryService.java

Lines 82 to 86 in 5a3a8bb

	@Override
	protected void startBackgroundDiscovery() {
	logger.debug("Start Danfoss Air CCM background discovery");
	scheduler.execute(this::discover);
	}

Usually bindings create a ScheduledFuture to continuously check for new devices.

I'm not sure if background discovery makes much sense for this binding. I don't think it needs to same convenience as for example a Zigbee binding, where it could happen more frequently that new devices shows up after a trip to IKEA etc.