Handler factory: immediate = true necessary ?

openhab / openhab-addons

Add-ons for openHAB

https://www.openhab.org/

Eclipse Public License 2.0

1.88k stars 3.59k forks source link

Handler factory: immediate = true necessary ? #5852

Closed lolodomo closed 5 years ago

lolodomo commented 5 years ago

I would like to know if it is expected or not to include "immediate = true" when declaring the handler factory of a binding. Most of the bindings are without this but few of them adds "immediate = true" like for example: Daikin: https://github.com/openhab/openhab2-addons/blob/6c93abcf1b5707db455e47144f42569735d33168/bundles/org.openhab.binding.daikin/src/main/java/org/openhab/binding/daikin/internal/DaikinHandlerFactory.java#L31 Loxone: https://github.com/openhab/openhab2-addons/blob/d45ee470d57e39cf5daed07f01a6dd151ed04cce/bundles/org.openhab.binding.loxone/src/main/java/org/openhab/binding/loxone/internal/LxHandlerFactory.java#L31

This "immediate = true" seems to be added more generally on all discovery services. Is it expected too ?

cweitkamp commented 5 years ago

if it is expected or not to include "immediate = true" when declaring the handler factory

AFAIK No, it is not. We tried to align the usage more than once: #3672, #4075 and https://github.com/eclipse/smarthome/pull/5511 (see also https://github.com/eclipse/smarthome/issues/5163).

discovery services. Is it expected too ?

Depends. For MDNS or UPNP or similar it is expected, for stand-alone discovery like Astro too, for discovery services created inside a HandlerFactory like Hue not.

@openhab/2-x-add-ons-maintainers Please correct me if I am wrong.

lolodomo commented 5 years ago

For thing handler factory, we have exactly 15 bindings with "immediate = true" in this repo. Z-Wave binding is also concerned.

lolodomo commented 5 years ago

Please also note that the following bindings are not defining a configurationPid: https://github.com/openhab/openhab2-addons/blob/e97d52598a748c576051739b05c48fd147d5b72c/bundles/org.openhab.binding.meteoblue/src/main/java/org/openhab/binding/meteoblue/internal/MeteoBlueHandlerFactory.java#L37 https://github.com/openhab/openhab2-addons/blob/ddb4a5e61ba23afa75964d12e0763de435090a7f/bundles/org.openhab.binding.snmp/src/main/java/org/openhab/binding/snmp/internal/SnmpHandlerFactory.java#L38 https://github.com/openhab/openhab2-addons/blob/e796b7dc5ed06d1089afd9f08bf9a939276b6308/bundles/org.openhab.binding.elerotransmitterstick/src/main/java/org/openhab/binding/elerotransmitterstick/internal/EleroTransmitterStickHandlerFactory.java#L43 https://github.com/openhab/openhab2-addons/blob/9dbae99479eea9d9552befde5fae13b7fcfb88e1/bundles/org.openhab.binding.mqtt.generic/src/main/java/org/openhab/binding/mqtt/generic/internal/MqttThingHandlerFactory.java#L43 https://github.com/openhab/openhab2-addons/blob/9dbae99479eea9d9552befde5fae13b7fcfb88e1/bundles/org.openhab.binding.mqtt.homeassistant/src/main/java/org/openhab/binding/mqtt/homeassistant/generic/internal/MqttThingHandlerFactory.java#L44 https://github.com/openhab/openhab2-addons/blob/9dbae99479eea9d9552befde5fae13b7fcfb88e1/bundles/org.openhab.binding.mqtt.homie/src/main/java/org/openhab/binding/mqtt/homie/generic/internal/MqttThingHandlerFactory.java#L45

lolodomo commented 5 years ago

For the netatmo binding, the service with two classes looks unusual:

@Component(service = { ThingHandlerFactory.class,
        NetatmoHandlerFactory.class }, immediate = true, configurationPid = "binding.netatmo")

I assume it should be changed to:

@Component(service = ThingHandlerFactory.class, configurationPid = "binding.netatmo")

Hilbrand commented 5 years ago

Related to the netatmo binding. I think it's a left over of a refactoring by someone named @lolodomo :smile: See https://github.com/openhab/openhab2-addons/pull/3253 The NetatmoHandlerFactory was injected before the change, and thus the service was needed. After the refactoring it wasn't used anymore, so it's not needed anymore to have it in the service

lolodomo commented 5 years ago

Ok, I will fix the netatmo binding ;)