[ecobee] Rework discovery

[mhilbush]

Fixes #9496

@kaikreuzer Could I trouble you for a review please?

[kaikreuzer]

Thanks @mhilbush, looks pretty good to me!
Just 2 small comments below.

[kaikreuzer]

This check should be removed. This method is called upon manually triggered scans, so it must be performed even if background discovery is not activated.

[mhilbush]

Good catch. I'll fix this.

[mhilbush]

Fixed

[kaikreuzer]

You should check whether isBackgroundDiscoveryEnabled returns true before scheduling this job.

[mhilbush]

From what I can see, startBackgroundDiscovery is being called before the bridge handler has initialized, therefore the configuration is not available at the time the job is scheduled.

[kaikreuzer]

Hm, I see. Then keep the scheduled job active in any case and do the check within the job as you intended to do it.

[mhilbush]

Yeah, I thought it was odd that startBackgroundDiscovery can be called before the bridge thing has initialized. Especially in the case where the discovery service implements ThingHandlerService in order to get access to the bridge handler.

I also ran into something a little weird for which I'd like your opinion. Let me know if you think I should log an issue for this.

Here's the scenario:

I have a things file containing two bridge things:

Bridge ecobee:account:account "Ecobee Account" [ apiKey="XXXXXXX", apiTimeout=20, discoveryEnabled=false, refreshIntervalNormal=30, refreshIntervalQuick=5 ] {
}

Bridge zoneminder:server:server "Zoneminder Server" [ host="zoneminder.host", useSSL=false, portNumber="80", urlPath="/zm", discoveryEnabled=false, refreshInterval=30, defaultAlarmDuration=180 ] {
}

Notice that both Bridge definitions have a parameter called discoveryEnabled and that both are false.

If I change just the Zoneminder discoveryEnabled to true, I see this behavior.

2020-12-27 13:15:07.103 [INFO ] [el.core.internal.ModelRepositoryImpl] - Loading model 'test.things'
2020-12-27 13:15:07.120 [DEBUG] [.thing.internal.GenericThingProvider] - Read things from model 'test.things'
2020-12-27 13:15:07.122 [DEBUG] [.thing.internal.GenericThingProvider] - Updating thing 'ecobee:account:account' from model 'test.things'.
2020-12-27 13:15:07.136 [DEBUG] [.thing.internal.GenericThingProvider] - Updating thing 'zoneminder:server:server' from model 'test.things'.

If I change just the Zoneminder discoveryEnabled back to false, I see this behavior.

2020-12-27 13:17:36.865 [INFO ] [el.core.internal.ModelRepositoryImpl] - Loading model 'test.things'
2020-12-27 13:17:36.883 [DEBUG] [.thing.internal.GenericThingProvider] - Read things from model 'test.things'
2020-12-27 13:17:36.886 [DEBUG] [.thing.internal.GenericThingProvider] - Updating thing 'ecobee:account:account' from model 'test.things'.

[kaikreuzer]

Sounds not what one would expect. Clearly, the thing where something has changed should be updated...
If you are able to minimise this to an easily reproducible case (without requiring any hardware, so the astro binding might be a good fit), an issue in openhab-core will make sense.

[mhilbush]

@kaikreuzer I've addressed your review comments. Thanks!!

I've also posed a question for you on some other odd behavior I was seeing.

[cpmeister]

Maybe this should be synchronized instead of discoverThermostats() and discoverSensors().

[mhilbush]

I thought about that. As the thermostat and sensor discovery processes are largely independent, I decided to do it at that level. Is there a preference for doing it here instead?

[kaikreuzer]

lgtm, thanks!
@cpmeister As you commented as well, please feel free to merge when you are fine with it.