Allow thing handler to skip device initialization and execute it on config update #409

TomTravaglino · 2019-03-11T10:36:43Z

Resolves #403

This PR provides a solution that allows the thing handler to skip device initialization if it has been done previously. Furthermore it allows to execute a device (re-)initialization by updating the thing config.

In detail:

Introduced thing property 'zigbee_initialised'. The property will be
checked before the channels are initialized and skips the device
initialization when it is 'true'. On the first initialization the
property will be set to 'true' after the channel initialization.
Introduced thing config parameter 'zigbee_initialise_device' in order
to allow the user to (re-)initialize the device by updating the
config. The config parameter will be checked when the config is
updated and executes a device initialization by disposing and initializing the thing handler

TomTravaglino · 2019-03-11T10:40:00Z

@hsudbrock As discussed, please join this PR and do a review.

hsudbrock

Thanks @TomTravaglino , I have added some comments inline.

org.openhab.binding.zigbee/src/main/java/org/openhab/binding/zigbee/ZigBeeBindingConstants.java

...nhab.binding.zigbee/src/main/java/org/openhab/binding/zigbee/handler/ZigBeeThingHandler.java

TomTravaglino · 2019-03-12T07:52:20Z

@hsudbrock I updated the PR

hsudbrock

Thanks @TomTravaglino ! Two comments regarding logging...

...nhab.binding.zigbee/src/main/java/org/openhab/binding/zigbee/handler/ZigBeeThingHandler.java

hsudbrock · 2019-03-12T10:44:57Z

@TomTravaglino Should the new config parameter also be added to the config description? (Probably as an advanced parameter?)

TomTravaglino · 2019-03-12T14:30:45Z

Should the new config parameter also be added to the config description

Yes. I add it to device.xml. And to the static thing types too?

hsudbrock · 2019-03-12T14:42:12Z

And to the static thing types too?

What would be the reason for not adding it to the static thing types?

cdjackson · 2019-03-12T15:07:54Z

Regarding this config parameter - this could get messy. I think that without this in the XML (ie ALL xmls for all things) then there might be problems. Certainly it will not be possible for users to reinitialise their devices.

I wonder if it might not be better to instead use a Config Parameter Provider to provide this to all ZigBee thing types rather than require it to be added to all XML files. This might be less error prone?

...nhab.binding.zigbee/src/main/java/org/openhab/binding/zigbee/handler/ZigBeeThingHandler.java

TomTravaglino · 2019-03-15T04:58:30Z

Regarding this config parameter - this could get messy. I think that without this in the XML (ie ALL xmls for all things) then there might be problems. Certainly it will not be possible for users to reinitialise their devices.

I wonder if it might not be better to instead use a Config Parameter Provider to provide this to all ZigBee thing types rather than require it to be added to all XML files. This might be less error prone?

@cdjackson
Are you thinking about a 'Config Parameter Provider' that can be registered as a service which can be referenced e.g. by the ZigBeeHandlerFactory and passed to the ZigBeeThingHandler?

cdjackson · 2019-03-15T07:48:25Z

Yes

TomTravaglino · 2019-03-18T12:04:47Z

I discussed the ‘Config Parameter Provider’ idea with @triller-telekom and we came to the conclusion that such a solution needs ‘some’ work to be done. In addition, the mac address is currently also defined in the XML. My PR implements that the reportings and cluster bindings are only configured once for each device. Implementing a generic provider for various kinds of thing properties is IMHO out of scope for this PR. Our proposal is to let this PR as it is and create a new issue for the ‘Config Parameter Provider’ solution. WDYT?

cdjackson · 2019-03-18T12:22:53Z

we came to the conclusion that such a solution needs ‘some’ work to be done.

Yes, but it's not much is it? And it makes this PR usable - otherwise this PR leaves hidden dependencies.

Implementing a generic provider for various kinds of thing properties is IMHO out of scope for this PR.

I disagree since without this, we end up with a dependency that will probably be forgotten, and will stop devices working. What I'm worried about is if we implement these sort of changes that are only partly working, it's fine for Qivicon where Deutsche Telekom people can manage your system and your thing definitions, but it will likely be left to me to work out the problems with openHAB.

I'm happy to move it to another PR if you plan to provide that in the coming couple of weeks, otherwise I would prefer to see it here so that this feature doesn't have hidden dependencies.

hsudbrock · 2019-03-19T14:28:07Z

I just discussed this issue with @triller-telekom. First, we see Chris' point that it might cause issues adding this to all static thing types; in particular, if there is someone out there having own static thing types that are not provided by the open source binding, then these would not get the new config parameter.

We took a look at what is currently implemented in the ZigBeeThingHandler; it already acts as a ConfigDescriptionProvider, providing additional config parameters for some things with the generic thing type. We could simply have this ConfigDescriptionProvider also provide this PR's new parameter zigbee_initialise_device. This appears (a) simpler than adding new config parameter providers as discussed above (as this is not yet provided by the openHAB core bundles, and would hence need more work), and (b) would nicely fit into the existing code regarding dynamically provided config parameters.

From the code perspective, this could look like this in the ZigBeeThingHandler.doNodeInitialisation (instead of the current code here), where we add the new config parameter for all things:

        List<ConfigDescriptionParameter> parameters = new ArrayList<ConfigDescriptionParameter>();
        parameters.add(ConfigDescriptionParameterBuilder.create("zigbee_initialise_device", Type.BOOLEAN)
                .withAdvanced(true).withDescription("Set to true to reinitialise binds and reporting on the device")
                .withLabel("Reinitialise device").withRequired(false).build());

        if (getThing().getThingTypeUID().equals(ZigBeeBindingConstants.THING_TYPE_GENERIC_DEVICE)) {
            // Dynamically create the channels from the device
            // Process all the endpoints for this device and add all channels as derived from the supported clusters
            nodeChannels = new ArrayList<>();
            for (ZigBeeEndpoint endpoint : coordinatorHandler.getNodeEndpoints(nodeIeeeAddress)) {
                logger.debug("{}: Checking endpoint {} channels", nodeIeeeAddress, endpoint.getEndpointId());
                nodeChannels.addAll(channelFactory.getChannels(getThing().getUID(), endpoint));
            }
            logger.debug("{}: Dynamically created {} channels", nodeIeeeAddress, nodeChannels.size());

            for (ZclClusterConfigHandler handler : configHandlers) {
                parameters.addAll(handler.getConfiguration());
            }
        } else {
            // We already have the correct thing type so just use the channels
            nodeChannels = getThing().getChannels();
            logger.debug("{}: Using static definition with existing {} channels", nodeIeeeAddress, nodeChannels.size());
        }

        try {
            configDescription = new ConfigDescription(new URI("thing:" + getThing().getUID()), parameters);
        } catch (URISyntaxException e) {
            logger.debug("Error creating URI for thing description:", e);
        }

WDYT, @cdjackson and @TomTravaglino ? Would this meet both your concerns?

TomTravaglino · 2019-03-19T14:41:25Z

I considered a very similar approach this morning. But to me it looks like it works for generic devices since you create a new config description. But it was not yet clear to me if you can handle it that way when a static thing type already provides its own config description. I think this approach would overwrite it. Maybe I missed something so I first wanted to discuss it with someone of you.

hsudbrock · 2019-03-19T15:02:44Z

I think this approach would overwrite it.

The ConfigDescriptionRegistry merges the config description provided by a thing type with the config descriptions provided by ConfigDescriptionProviders (in our case, the ZigBeeThingHandler). (See the implementation of ConfigDescriptionRegistry.getConfigDescription(URI uri, Locale locale) for details.) I.e., it is not overwritten.

TomTravaglino · 2019-03-19T15:10:14Z

Thanks for the input. Then the approach should be fine.

TomTravaglino · 2019-03-25T10:33:36Z

I updated the PR

TomTravaglino · 2019-03-26T07:00:12Z

PR is updated

hsudbrock

I have added my comments inline, @TomTravaglino .

hsudbrock · 2019-03-29T11:20:04Z

...nhab.binding.zigbee/src/main/java/org/openhab/binding/zigbee/handler/ZigBeeThingHandler.java

@@ -251,6 +252,9 @@ private synchronized void doNodeInitialisation() {

        List<Channel> nodeChannels;

+        List<ConfigDescriptionParameter> parameters = new ArrayList<ConfigDescriptionParameter>();


Very small comment (no need to change with this PR, unless you like to): You can use the diamond operator on the right-hand side of the assignment, i.e. new ArrayList<>(). The compiler can figure out the generic type on its own, and this makes the code somewhat less "heavy".

I will change it.

hsudbrock · 2019-03-29T12:59:56Z

...bee/src/main/java/org/openhab/binding/zigbee/internal/ZigBeeConfigDescriptionParameters.java

+ */
+public class ZigBeeConfigDescriptionParameters {
+
+    private static final String PARAM_ZIGBEE_INITIALISE_DEVICE_NAME = "zigbee_initialise_device";


Why not use the constant from ZigBeeBindingConstants here?

Will replace it.

hsudbrock · 2019-03-29T13:03:46Z

...nhab.binding.zigbee/src/main/java/org/openhab/binding/zigbee/handler/ZigBeeThingHandler.java

@@ -251,6 +252,9 @@ private synchronized void doNodeInitialisation() {

        List<Channel> nodeChannels;

+        List<ConfigDescriptionParameter> parameters = new ArrayList<ConfigDescriptionParameter>();
+        parameters.addAll(ZigBeeConfigDescriptionParameters.getParameters());


Why do you add those explicitly here, and not put them directly into the array list constructor?

No special reason. But I can change it.

hsudbrock · 2019-03-29T13:08:27Z

...nhab.binding.zigbee/src/main/java/org/openhab/binding/zigbee/handler/ZigBeeThingHandler.java

+                    }
+                } else {
+                    logger.debug(
+                            "{}: Initialization of channel {} with {} will be skipped as the device is already initialized",


This log message sounds wrong, as the channel is still initialized (but only the converter part - just the device initialization part is skipped).

Correct would be something like "Device initialization for channel {} with {} is skipped as the device is already initialized".

IMHO, it would be even better to not log this for every channel, but rather use a single log message stating that device initialization is skipped for all channels (in the case that initializeDevice is false).

I can remove this statement and put this one just before the loop:

if (!initializeDevice) { logger.debug("{}: Device initialization will be skipped as the device is already initialized", nodeIeeeAddress); }

hsudbrock · 2019-03-29T13:12:57Z

...nhab.binding.zigbee/src/main/java/org/openhab/binding/zigbee/handler/ZigBeeThingHandler.java

@@ -423,6 +439,12 @@ private synchronized void doNodeInitialisation() {
        coordinatorHandler.serializeNetwork();
    }

+    private synchronized void reinitializeDevice() {


Reading this now, the method name seems wrong. This method reinitializes the thing handler, and not the device. In fact, it will only reinitialize the device if the THING_PROPERTY_DEVICE_INITIALIZED was set to false before (which you do in handleConfigurationUpdate).

I suggest to either rename the method, or set the thing property in here, to fix this issue.

OK. I will move the statement

getThing().setProperty(ZigBeeBindingConstants.THING_PROPERTY_DEVICE_INITIALIZED, Boolean.FALSE.toString());

into this method. I think then it's clear what the method is made for.

hsudbrock · 2019-03-29T13:14:04Z

...nhab.binding.zigbee/src/main/java/org/openhab/binding/zigbee/handler/ZigBeeThingHandler.java

@@ -597,6 +624,11 @@ public void handleConfigurationUpdate(Map<String, Object> configurationParameter

        // Persist changes
        updateConfiguration(configuration);
+
+        if (initializeDevice) {


Question: What is the difference for you between initializeDevice and reinitializeDevice? (If there is none, you could use one of the words both for the variable and the method.)

I named the variable initializeDevice because it is related to the config param zigbee_initialise_device. The method name is reinitializeDevice because a re-initialization is what it does when the config param is set to true.
Do you think it's confusing or not good to understand?

Do you think it's confusing or not good to understand?

I think that it is ok, but not ideal.

cdjackson · 2019-04-01T08:03:41Z

...nhab.binding.zigbee/src/main/java/org/openhab/binding/zigbee/handler/ZigBeeThingHandler.java

+    private synchronized void reinitializeDevice() {
+        logger.debug("{}: Reinitializing device", nodeIeeeAddress);
+        dispose();
+        initialize();


This seems a bit strange - maybe this is also Hennings issue.

This method reinitializeDevice doesn't reinitialise the device - it reinitialises the thing handler and the device. I would have expected that when the user wants to reinitialise the device, it just does that.

Or, do you think that because we have updated the device, that we also need to reinitialise all the converters? I'm not sure this is the case (I'd need to think it through some more - maybe you've already done that?). The clusters won't change, so I don't think reinitialising the converters and redetecting the channels and polling is really necessary?

I could be wrong though, and maybe a full reinitialisation and repoll etc is really needed, but this wasn't my expectation when I first introduced this concept.

The idea was to re-use the existing methods and so rely on the existing initialization. However, I understand your objection and so I took a closer look at the initialization part. As far as I can see we can initialize the device without the need to re-initialize anything else. I'm implementing a method that initializes only the devices. PR will follow soon.

- Introduced thing property 'zigbee_initialised'. The property will be checked before the channels are initialized and skips the device initialization when it is 'true'. On the first initialization the property will be set to 'true' after the channel initialization. - Introduced class that provides thing config description parameters. - Introduced thing config parameter 'zigbee_initialise_device' in order to allow the user to (re-)initialize the device by updating the config. The config parameter will be checked when the config is updated and executes a device initialization. Signed-off-by: Tommaso Travaglino <[email protected]>

TomTravaglino · 2019-04-08T08:53:38Z

I updated the PR. Now the device can be re-initialized without initializing the thing handler.

I think Hennings comments have been addressed.

…t_discovered

cdjackson · 2019-04-09T22:20:08Z

Thanks @TomTravaglino this looks good to me. I'll just let CI re-run after the branch update and then it should be good to merge.

triller-telekom · 2019-04-10T11:33:09Z

@cdjackson Pipeline looks successful now, I guess its ready to be merged now. Thanks for reviewing.

cdjackson

LGTM

hsudbrock previously requested changes Mar 11, 2019

View reviewed changes

hsudbrock reviewed Mar 12, 2019

View reviewed changes

...nhab.binding.zigbee/src/main/java/org/openhab/binding/zigbee/handler/ZigBeeThingHandler.java Outdated Show resolved Hide resolved

...nhab.binding.zigbee/src/main/java/org/openhab/binding/zigbee/handler/ZigBeeThingHandler.java Outdated Show resolved Hide resolved

cdjackson reviewed Mar 12, 2019

View reviewed changes

TomTravaglino commented Mar 13, 2019

View reviewed changes

...nhab.binding.zigbee/src/main/java/org/openhab/binding/zigbee/handler/ZigBeeThingHandler.java Show resolved Hide resolved

TomTravaglino force-pushed the 403_Initialize_device_only_when_it_is_first_discovered branch 3 times, most recently from 9c0f012 to 716b95c Compare March 22, 2019 10:54

hsudbrock added Rebuild Retriggers a PR build on Jenkins and removed Rebuild Retriggers a PR build on Jenkins labels Mar 25, 2019

TomTravaglino force-pushed the 403_Initialize_device_only_when_it_is_first_discovered branch from 716b95c to ee17d78 Compare March 25, 2019 10:27

TomTravaglino force-pushed the 403_Initialize_device_only_when_it_is_first_discovered branch from 33dda63 to 6be0c7b Compare March 25, 2019 12:26

hsudbrock reviewed Mar 29, 2019

View reviewed changes

cdjackson reviewed Apr 1, 2019

View reviewed changes

TomTravaglino force-pushed the 403_Initialize_device_only_when_it_is_first_discovered branch from 6e3b706 to a5f3566 Compare April 8, 2019 07:51

Merge branch 'master' into 403_Initialize_device_only_when_it_is_firs…

ddeb639

…t_discovered

cdjackson approved these changes Apr 10, 2019

View reviewed changes

cdjackson merged commit 3e64c04 into openhab:master Apr 10, 2019

TomTravaglino deleted the 403_Initialize_device_only_when_it_is_first_discovered branch April 11, 2019 07:57

TomTravaglino mentioned this pull request May 3, 2019

NPE in some converters when calculating updated attributes #432

Closed

cdjackson added this to the 2.5 milestone Dec 8, 2019

		@@ -251,6 +252,9 @@ private synchronized void doNodeInitialisation() {

		List<Channel> nodeChannels;

		List<ConfigDescriptionParameter> parameters = new ArrayList<ConfigDescriptionParameter>();

Allow thing handler to skip device initialization and execute it on config update #409

Allow thing handler to skip device initialization and execute it on config update #409

Conversation

TomTravaglino commented Mar 11, 2019 • edited Loading

TomTravaglino commented Mar 11, 2019

hsudbrock left a comment

Choose a reason for hiding this comment

TomTravaglino commented Mar 12, 2019

hsudbrock left a comment

Choose a reason for hiding this comment

hsudbrock commented Mar 12, 2019

TomTravaglino commented Mar 12, 2019 • edited Loading

hsudbrock commented Mar 12, 2019

cdjackson commented Mar 12, 2019

TomTravaglino commented Mar 15, 2019 • edited Loading

cdjackson commented Mar 15, 2019

TomTravaglino commented Mar 18, 2019

cdjackson commented Mar 18, 2019

hsudbrock commented Mar 19, 2019

TomTravaglino commented Mar 19, 2019 • edited Loading

hsudbrock commented Mar 19, 2019

TomTravaglino commented Mar 19, 2019

TomTravaglino commented Mar 25, 2019

TomTravaglino commented Mar 26, 2019

hsudbrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomTravaglino commented Apr 8, 2019 • edited Loading

cdjackson commented Apr 9, 2019

triller-telekom commented Apr 10, 2019

cdjackson left a comment

Choose a reason for hiding this comment

TomTravaglino commented Mar 11, 2019 •

edited

Loading

TomTravaglino commented Mar 12, 2019 •

edited

Loading

TomTravaglino commented Mar 15, 2019 •

edited

Loading

TomTravaglino commented Mar 19, 2019 •

edited

Loading

TomTravaglino commented Apr 8, 2019 •

edited

Loading