Discovery service created twice

Development Add-ons
1

Hello,

As I’m progressing on my AirZone binding, I have started implementing a discovery service and it works just fine when used with the “scan” button.
However, I am facing an issue with the background discovery overrides that I have just added.

Basically, startBackgroundDiscovery is not called on the same instance as the one used to call startScan and that means the first instance is never informed about the bridge handler it needs to perform its work.

I placed breakpoints in my constructors and the parameterless one is called very early in the binding life, then startBackgroundDiscovery is called on that instance.
Then, later, when the bridge handler has been created (via createHandler in the factory), a new instance of the discovery service is created and appears to replace the previous one.

To sum up, here is what I observed:

openHAB starts
binding gets loaded
AirZoneDiscoveryService() is called to created instance1
instance1.startBackgroundDiscovery is called which starts a scheduled task that will instance1.startScan on a regular basis.
A little bit later, AirZoneHandlerFactory.createHandler() is called which leads to AirZoneHandlerFactory.registerDeviceDiscoveryService
This method then calls AirZoneDiscoveryService(Localization) to create instance2
instance2.addBridge is called

When the Scan button is pressed in MainUI, it is instance2.startScan that is called and which works properly because the bridgeHandlers collection is not empty.
But because instance1 was never seen by the factory, the instance1.addBridge method is never called and instance1.startScan will exit early because of that.

I tried removing the @Component annotation on the AirZoneDiscoveryService class declaration and it made the first instance not being created, but startBackgroundDiscovery is never called in that case.

I believe I have missed something to have AirZoneHandlerFactory.discoveryService be automatically populated by instance1 but I’m at loss as to what’s missing, nothing came up from my look at other bindings.

If you want to have a look, the code is available here:

AirZoneDiscoveryService.java
AirZoneHandlerFactory.java

Note that I know it’s not in perfect shape when it comes to styling, but I’m looking at having it work first, then I’ll clean it up according to the linter rules.

Many thanks for your help.

2

Not sure of actual implementation, but if you do have a relationship between thing handler and discovery service, it might be better to use ThingHandlerService returned from ThingHandler#getServices call (it returns classes).
It is a bit counter-intuitive in wiring but it allows to tie ThingHandler and discovery service a bit closer. This is also how other addons sometimes use it. I use similar pattern also in third party addons I do.

The background discovery, when you are based on AbstractDiscoveryService is a bit convoluted. You might need to override isBackgroundDiscoveryEnabled (force to true), also double check the activate() method call which can revert default value to value coming from config as well as modified().

3

Thanks, I did read about the ThingHandlerService but could not see its usage in the binding that served as inspiration for mine (Velux, Russound)

Now, searching at all binding sources, I see that quite a few are indeed implementing this interface, but I also see some services that require a bridge instance via setter method or constructor parameters, override startBackgroundDiscovery and yet do not implement ThingHandlerService. Here are those that I identified:

class name @Component annotation
EleroChannelDiscoveryService -
HarmonyDeviceDiscoveryService -
HDPowerViewDeviceDiscoveryService -
hdpowerview.ShadeDiscoveryService -
jeelink.SensorDiscoveryService -
KM200GatewayDiscoveryService -
MelCloudDiscoveryService -
MillheatDiscoveryService -
NeatoAccountDiscoveryService -
NeoHubDiscoveryService -
SensiboDiscoveryService -
siemensrds.RdsDiscoveryService -
SleepIQBedDiscoveryService -
SmartthingsDiscoveryService X
TadoDiscoveryService -
VeluxDiscoveryService X
WeatherCompanyDiscoveryService -
ZWayDeviceDiscoveryService -

I’m thus left with a series of hypothesis:

  1. This never worked in openHAB 3
  2. This got broken in openHAB 4
  3. I missed something when I copied the implementation

The first is highly improbable, I believe it would have been noticed already.
The second may be possible, but I also believe someone would have noticed it before me.
So I’m left with the last one and trying to pinpoint what I did wrong.

But as it turns out, I may not need the background discovery implementation anyway as I already have a background task for refreshing the various channels (AirZone does not have notifications).
So I could call the discoverZones method from that refresh background task and be done with it, no need for another task, no need to register the service with the @Component annotation.

This just leaves the feeling that something is amiss with the bindings above, but I have no way to test them properly as I don’t have the devices for them.

4

I can comment on some of your hypotheses. :slight_smile: Thing handler services been working with OH 3.x, I do keep almost identical code for releases from 3.0.x up to 3.4.x, beyond UoM there was very few changes and no changes which would interfere discovery api. Whether it is broken in 4.x - this I don’t know, as I haven’t started porting code to that release.

If you do have something which can be obtained from already existing connection you might consider turning your thing definition into an “extensible” thing which is able to auto-discover channels. That’s something I did once or twice and which works fine, without necessity to implement dynamic thing type provider.

5

Thanks, but there is only one thing type and its channels are always the same, so there is no need to have any dynamic creation for them.

6

I had a look on your links, and what you currently do in handler factory is quite close to the concept of ThingHandlerService. In fact this is pretty much what base thing handler class does.

Once you have a working connection (as far I can see), you can only discover zones which can be fetched from existing connection. Discovery service you are trying to attach is fine to become a ThingHandlerService for bridge (connection). You might have an upper level discovery service which can identify bridges (connections), however this will usually have little to do with TCP, more likely with UDP and multicasts/broadcasts.
If you go towards thing handler service, then you do not need @Component annotation. It is used to generate some XML descriptors which are then used to activate services (repeating this for others, not necessarily you, who might get into similar problem). I am not sure why you intend to track created thing handlers at the factory level. Framework guarantees lifecycle callbacks with initialize() and dispose() calls for handlers.

Cheers,
Łukasz

7

Thanks for that review, it’s very kind of you.

Yes, I believe so. But looking more closely at all this, it became obvious this was overcomplex for no valid reason so I simplified it a lot in the most recent commits. No need for background discovery, no need for bridge handler tracking, much simpler.

Yes, this may come up one day as it appears other automation systems are capable of that, but it must be an undocumented feature of the API, it’s definitely not in the PDF I have here.

That’s to update “Number of bridges” and “Number of things” properties on the binding itself but that’s just a feature copied over from the Velux binding I based my work upon as it is one for which I have the hardware here, allowing me to compare behaviors. But that was not necessarily the wisest decision I took when starting my own binding.

Thanks again for taking the time to review my code.