Add methods to get supported cluster list from converters #517

cdjackson · 2019-11-14T06:00:37Z

This PR is incomplete and will fail CI - do not merge!!

From recent versions of the Z-Smart Systems ZigBee library (1.2.6 I think), we need to tell the framework what clusters we support. This is to allow the framework to only process messages that the application supports, and more importantly, to reject ones that are not supported with the appropriate error.

This is required for ZigBee certification and there is a test for this.

To achieve this, I've added two methods to the converters, and the converter factory to allow the coordinator handler to get the list of clusters that are implemented by the system. Each cluster will return a Set of cluster IDs it supports through the Set<Integer> getSupportedClientClusters() and Set<Integer> getSupportedServerClusters() methods. The converter factory has a similar set of methods and will return the consolidated Set of cluster IDs supported by all converters so that this can be passed to the framework.

@hsudbrock or @triller-telekom you should review this concept please.

Design questions - I have one thought - the framework methods set the client/server clusters the framework supports, but the converters internally use client/server clusters on the remote device, so the naming is reversed. Within the converter, does the getSupportedServerClusters method return the server clusters consumed by the converter, or the server clusters implemented by the converted?

This PR currently only implements the initial concept, and the methods in a single converter for comment.

Note that we probably also need to add the converter factory provider to the coordinator factories as this information is required in the ZigBeeCoordinatorHandler class (I've added this as a parameter on the constructor, but not hooked this all together yet).

Signed-off-by: Chris Jackson [email protected]

cdjackson · 2019-11-17T13:03:17Z

I have renamed getSupportedClientClusters and getSupportedServerClusters to getImplementedClientClusters and getImplementedServerClusters to try and make it easier to understand that the definition is about what is implemented in the converter. Otherwise there is confusion about "supported" - if it means supported by the converter, or supported by the remote device, since they are opposite.

cdjackson · 2019-11-17T13:16:11Z

I've now added the ZigBeeChannelConverterFactory to the Ember coordinator factory. Other than updating the other coordinators, and all of the converters to add the implemented clusters, this should be complete.

@hsudbrock or @triller-telekom I would really appreciate it if you can take a quick look at this ASAP as I would like to get this included into the latest OH for the 2.5 release so we can update the ZSS libraries. (Thanks).

triller-telekom · 2019-11-18T09:08:29Z

Hi Chris,

sorry to get back to you this late again. I wish I could use more time on this here...

While seeing the latest changes in ZSS I also thought about a way that the converters should announce their used clusters by themselves. It certainly makes sense to define the clusters where we use them, so in the converters is a good idea.

I also like "implemented" over "supported" due to the confusion mentioned by you.

However, what we should not miss, is those clusters which are used by instances of com.zsmartsystems.zigbee.app.ZigBeeApplication. Those have to be registered to the ZigbeeNetworkManager as well. For example the ZclIasZoneClient which is in ZSS.

I don't know if you plan to register them within ZSS, or if any program who wants to make use of these applications has to register the clusters.

cdjackson · 2019-11-18T10:55:42Z

Thanks for your thoughts @triller-telekom.

I don't know if you plan to register them within ZSS, or if any program who wants to make use of these applications has to register the clusters.

Yes, there is a plan to do this, but it's not all joined up - zsmartsystems/com.zsmartsystems.zigbee#411 is basically linked to this. In the first instance I might add these explicitly to the binding to make sure it's registered, but the above issue is becoming more important now that we have code to reject packets received from clusters that aren't registered...

You will find as well that there are now 2 APIs - one to set the clusters in theZigBeeManager, and another that is setting the clusters in the dongle and these should be aligned. In the past we never bothered to set the clusters in the Ember dongle as it was not really required, but you may find that certification requires this ;)

I will progress with this then and try and finish it tonight or tomorrow - I think it's all done now so it just needs testing. Do you want to review? Or should I ask - do you have time to review ;)

triller-telekom · 2019-11-18T11:04:49Z

Do you want to review? Or should I ask - do you have time to review ;)

Rather the later, but I can try to have a deeper look at this PR during this week. Sorry can't promise.

t-8ch · 2019-12-02T21:16:08Z

...ee.cc2531/src/main/java/org/openhab/binding/zigbee/cc2531/internal/CC2531HandlerFactory.java

+    @Nullable
+    private ZigBeeChannelConverterFactory zigbeeChannelConverterFactory;
+
+    @Reference


Adding the @Reference directly on the field would remove the need for the set and unset methods.

t-8ch · 2019-12-02T21:19:10Z

...n/java/org/openhab/binding/zigbee/internal/converter/ZigBeeConverterAtmosphericPressure.java

@@ -50,6 +54,16 @@

    private ZclPressureMeasurementCluster cluster;

+    @Override
+    public Set<Integer> getImplementedClientClusters() {
+        return Stream.of(ZclPressureMeasurementCluster.CLUSTER_ID).collect(Collectors.toSet());


Collections.singleton()

t-8ch · 2019-12-02T21:24:20Z

...inding.zigbee/src/main/java/org/openhab/binding/zigbee/handler/ZigBeeCoordinatorHandler.java

@@ -439,6 +445,13 @@ private synchronized void initialiseZigBee() {
                return;
        }

+        // Add all the clusters that we are supporting.
+        // If we don't do this, the framework will reject any packets for clusters we have not stated support for.
+        channelFactory.getImplementedClientClusters().stream()


According to the OSGi references in the handler factories the channelFactory can be null.
This would throw a NPE here.

t-8ch · 2019-12-02T21:25:13Z

...inding.zigbee/src/main/java/org/openhab/binding/zigbee/handler/ZigBeeCoordinatorHandler.java

+        // If we don't do this, the framework will reject any packets for clusters we have not stated support for.
+        channelFactory.getImplementedClientClusters().stream()
+                .forEach(clusterId -> networkManager.addSupportedClientCluster(clusterId));
+        channelFactory.getImplementedServerClusters().stream()


Could be reduced to channelFactory.getImplementedServerClusters().forEach(networkManager::addSupportedServerCluster)

cdjackson · 2019-12-02T22:01:17Z

Thanks for the review @t-8ch

Signed-off-by: Chris Jackson <[email protected]>

cdjackson · 2020-01-02T15:55:02Z

Closes #531

Signed-off-by: Chris Jackson <[email protected]>

cdjackson · 2020-01-02T16:51:42Z

This now updates to the ZSmart System libraries 1.2.8.

openhab/openhab-distro#1043

openhab-bot · 2020-01-02T17:05:14Z

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

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

openhab-bot · 2020-08-24T16:27:34Z

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

https://community.openhab.org/t/mijia-windows-door-sensor-model-mccgq01lm/104082/1

cdjackson force-pushed the supported_clusters branch from 04b4686 to e1d1c09 Compare November 17, 2019 12:51

cdjackson requested a review from hsudbrock November 17, 2019 13:16

cdjackson force-pushed the supported_clusters branch from 4cd8dde to 47030ef Compare November 17, 2019 20:51

cdjackson force-pushed the supported_clusters branch from 47030ef to e5abeb8 Compare November 23, 2019 21:01

t-8ch reviewed Dec 2, 2019

View reviewed changes

cdjackson changed the base branch from master to 2.5.x December 22, 2019 19:46

cdjackson force-pushed the supported_clusters branch 3 times, most recently from fc01e2b to 07e942e Compare December 25, 2019 20:37

cdjackson added 5 commits December 31, 2019 16:46

Add methods to get supported cluster list from converters

5435e5e

Signed-off-by: Chris Jackson <[email protected]>

Rename getSupportedXxxClusters to getImplementedXxxClusters

2a4cd87

Signed-off-by: Chris Jackson <[email protected]>

Add ZigBeeChannelConverterFactory to Ember coordinator constructor

1588ea1

Signed-off-by: Chris Jackson <[email protected]>

Update remaining converters, handlers and factories

53050a0

Signed-off-by: Chris Jackson <[email protected]>

Update remaining converters, handlers and factories

5febd00

Signed-off-by: Chris Jackson <[email protected]>

cdjackson force-pushed the supported_clusters branch 2 times, most recently from 7ea289b to a3287ac Compare January 2, 2020 12:14

Update Binary Input converter

f1e42a1

Signed-off-by: Chris Jackson <[email protected]>

cdjackson force-pushed the supported_clusters branch from a3287ac to f1e42a1 Compare January 2, 2020 16:00

cdjackson changed the title ~~[WIP] Add methods to get supported cluster list from converters~~ Add methods to get supported cluster list from converters Jan 2, 2020

cdjackson merged commit f7cd109 into openhab:2.5.x Jan 2, 2020

cdjackson deleted the supported_clusters branch January 2, 2020 20:51

This was referenced Jan 3, 2020

Cluster x not found for received endpoint command #429

Closed

Coordinator properties are missing in Paper UI since 2.5 M2 #545

Closed

Reminder for newer Z-Smart systems versions #531

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add methods to get supported cluster list from converters #517

Add methods to get supported cluster list from converters #517

cdjackson commented Nov 14, 2019 •

edited

Loading

cdjackson commented Nov 17, 2019

cdjackson commented Nov 17, 2019

triller-telekom commented Nov 18, 2019

cdjackson commented Nov 18, 2019

triller-telekom commented Nov 18, 2019

t-8ch Dec 2, 2019

t-8ch Dec 2, 2019

t-8ch Dec 2, 2019

t-8ch Dec 2, 2019

cdjackson commented Dec 2, 2019

cdjackson commented Jan 2, 2020

cdjackson commented Jan 2, 2020

openhab-bot commented Jan 2, 2020

openhab-bot commented Aug 24, 2020

Add methods to get supported cluster list from converters #517

Add methods to get supported cluster list from converters #517

Conversation

cdjackson commented Nov 14, 2019 • edited Loading

cdjackson commented Nov 17, 2019

cdjackson commented Nov 17, 2019

triller-telekom commented Nov 18, 2019

cdjackson commented Nov 18, 2019

triller-telekom commented Nov 18, 2019

t-8ch Dec 2, 2019

Choose a reason for hiding this comment

t-8ch Dec 2, 2019

Choose a reason for hiding this comment

t-8ch Dec 2, 2019

Choose a reason for hiding this comment

t-8ch Dec 2, 2019

Choose a reason for hiding this comment

cdjackson commented Dec 2, 2019

cdjackson commented Jan 2, 2020

cdjackson commented Jan 2, 2020

openhab-bot commented Jan 2, 2020

openhab-bot commented Aug 24, 2020

cdjackson commented Nov 14, 2019 •

edited

Loading