Refactor ZigBeeBaseChannelConverter to split initialisation #342

cdjackson · 2018-12-21T23:30:51Z

This implements the split in the ZigBeeBaseChannelConverter to separate device configuration and converter initialisation.

initializeDevice is used to perform the device configuration.
initializeConverter is used to perform tine converter initialisation.

Currently both are called at the same time, but as discussed this can be split in a later PR.

I've implemented the split in the OnOffConverter as an example.

Closes #338
Signed-off-by: Chris Jackson chris@cd-jackson.com

openhab-bot · 2018-12-30T23:21:57Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/zigbee-binding/15763/1782

hsudbrock · 2019-01-03T11:02:35Z

...ng.zigbee/src/main/java/org/openhab/binding/zigbee/converter/ZigBeeBaseChannelConverter.java

@@ -177,6 +177,16 @@ public void initialize(ZigBeeThingHandler thing, Channel channel, ZigBeeCoordina
        this.coordinator = coordinator;
    }

+    /**


Should the class-level documentation (which describes the life cycle of a converter) be updated to reflect this change?

Yep - thanks. I'll update.

hsudbrock

Hi Chris - thanks for the PR, I left my comments inline.

hsudbrock · 2019-01-03T11:32:10Z

.../src/main/java/org/openhab/binding/zigbee/internal/converter/ZigBeeConverterSwitchOnoff.java

-    public boolean initializeConverter() {
-        clusterOnOffClient = (ZclOnOffCluster) endpoint.getOutputCluster(ZclOnOffCluster.CLUSTER_ID);
-        clusterOnOffServer = (ZclOnOffCluster) endpoint.getInputCluster(ZclOnOffCluster.CLUSTER_ID);
+    public boolean initializeDevice() {


If there is an error when binding to the cluster, or when setting reporting, then the polling period is adjusted to POLLING_PERIOD_HIGH. But adapting the polling period is converter initialisation, not device initialisation (and should, hence, be done in initializeConverter).

That would mean that if, in the future, as suggested in #338, the device is initialised only once, then the adjustment of the polling period would be lost when the converter is initialised a second time.

I don't see a straightforward 'fix' for this, though, because the information whether the polling period needs to be adjusted due to bind/reporting configuration issues is not persisted anywhere.

Do you have any idea on how to deal with this issue?

I would hope (!!) that we can get a clean initialisation of the bind/reporting - even if it takes a few retries... In the event that this doesn't work (eg if the bind table is full - I know some Hue bulbs only have a single entry so it's only possible to bind one cluster!) then we probably should store that elsewhere (ie configuration).

I have another PR waiting for review (I think) which allows user configuration of polling period, so in the event that reporting is not initialised properly, the polling configuration can be updated.

Sounds like a viable solution; I will take a look at that other PR.

hsudbrock · 2019-01-03T11:35:02Z

.../src/main/java/org/openhab/binding/zigbee/internal/converter/ZigBeeConverterSwitchOnoff.java

-        clusterOnOffClient = (ZclOnOffCluster) endpoint.getOutputCluster(ZclOnOffCluster.CLUSTER_ID);
-        clusterOnOffServer = (ZclOnOffCluster) endpoint.getInputCluster(ZclOnOffCluster.CLUSTER_ID);
+    public boolean initializeDevice() {
+        ZclOnOffCluster clusterOnOffClient = (ZclOnOffCluster) endpoint.getOutputCluster(ZclOnOffCluster.CLUSTER_ID);


Minor comment: Why use a local variable here, instead of assigning directly to the field clusterOnOffClient? (Same for the clusterOnOffServer in the following line.)

My rational is that this method is transitory - ie it shouldn't retain state etc. Ok, I agree that we could use the field instead of the local variable, but to me at least the local variable should be initialised in the initializeConverter method, and disposed in the disposeConverter method, and used in the listeners. These methods should not (can not!) rely on the initializeDevice method initialising this field and I just felt it made it clearer not to use it....

I don't overly mind though if you think it's better to use the field instead for some reason?

Hm, I think that it is often not a good idea to use the same name for a local variable and a class-level field, because it makes reading the code harder (and leads to confusion like mine in the review).

I, personally, would prefer to either use a different name for the local variables, or to simply add a one-line comment // Use local copy of the cluster objects, as the corresponding fields are only initialized in initializeConverter - here probably the comment, as it appears somewhat artificial to use a different name. But this is probably personal preference...

Yes, you're right - it would be clearer if the name were different. I will implement one or both of your suggestions - thanks.

Signed-off-by: Chris Jackson <chris@cd-jackson.com>

cdjackson · 2019-01-05T10:24:28Z

@hsudbrock I've updated this PR to improve the docs...

hsudbrock

Thanks for updating the JavaDoc!

I think that we could merge this PR now, as (for now) initializeDevice is called on every converter initialization. Before introducing code that causes initializeDevice to not always be called, the initialization of the On/Off converter should be adapted using configurable polling periods as you suggested in the comments.

cdjackson · 2019-01-06T15:44:47Z

I don’t see any option to change this now - it requires the other PR to be merged before this can be changed.

…

On 6 Jan 2019, at 14:57, Henning Sudbrock ***@***.***> wrote: @hsudbrock approved this pull request. Thanks for updating the JavaDoc! I think that we could merge this PR now, as (for now) initializeDevice is called on every converter initialization. Before introducing code that causes initializeDevice to not always be called, the initialization of the On/Off converter should be adapted using configurable polling periods as you suggested in the comments. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#342 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_kQ2b3xmOzjU33RkyhdvhOxquqVy7Gks5vAg7vgaJpZM4ZfNjn>.

hsudbrock · 2019-01-07T08:09:57Z

I don’t see any option to change this now - it requires the other PR to be merged before this can be changed.

That's why I suggest to merge this now - and to simply take care that the OnOffConverter has been 'fixed' with the help of the other (not yet merged) PR before actually exploiting the split of the initialization introduced in this PR.

OK for you?

cdjackson · 2019-01-07T08:28:29Z

Sure - that's fine.

This was referenced Dec 23, 2018

[enhancement] Make polling/reporting interval configurable #305

Closed

Add ReportingConfiguration handler #344

Merged

hsudbrock self-requested a review January 3, 2019 10:35

hsudbrock reviewed Jan 3, 2019

View reviewed changes

hsudbrock suggested changes Jan 3, 2019

View reviewed changes

Refactor BaseConverter to split initialisation

70a5b89

Signed-off-by: Chris Jackson <chris@cd-jackson.com>

cdjackson force-pushed the converter_refactor branch from 860d8db to 70a5b89 Compare January 5, 2019 10:23

Merge branch 'master' into converter_refactor

e4fc5c5

hsudbrock approved these changes Jan 6, 2019

View reviewed changes

cdjackson closed this Jan 7, 2019

cdjackson reopened this Jan 7, 2019

cdjackson merged commit 45f74b9 into openhab:master Jan 7, 2019

cdjackson deleted the converter_refactor branch January 7, 2019 08:28

TomTravaglino mentioned this pull request Mar 6, 2019

Initialize device only when it is first discovered #403

Closed

cdjackson added this to the 2.5 milestone Dec 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ZigBeeBaseChannelConverter to split initialisation #342

Refactor ZigBeeBaseChannelConverter to split initialisation #342

cdjackson commented Dec 21, 2018

openhab-bot commented Dec 30, 2018

hsudbrock Jan 3, 2019

cdjackson Jan 3, 2019

hsudbrock left a comment

hsudbrock Jan 3, 2019

cdjackson Jan 3, 2019

cdjackson Jan 3, 2019

hsudbrock Jan 3, 2019

hsudbrock Jan 3, 2019

cdjackson Jan 3, 2019

hsudbrock Jan 3, 2019

cdjackson Jan 3, 2019

cdjackson commented Jan 5, 2019

hsudbrock left a comment

cdjackson commented Jan 6, 2019 via email

hsudbrock commented Jan 7, 2019

cdjackson commented Jan 7, 2019

Refactor ZigBeeBaseChannelConverter to split initialisation #342

Refactor ZigBeeBaseChannelConverter to split initialisation #342

Conversation

cdjackson commented Dec 21, 2018

openhab-bot commented Dec 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

cdjackson commented Jan 5, 2019

hsudbrock left a comment

Choose a reason for hiding this comment

cdjackson commented Jan 6, 2019 via email

hsudbrock commented Jan 7, 2019

cdjackson commented Jan 7, 2019