ZigBee binding discusion

Add-ons Bindings
, ,
1

Good day everyone,

I have been looking into ZigBee binding for OH2 source code developed by @chris and I am amazed with his incredible work, though I do see a few problems with it.

From what I understand Chris’ ZigBee binding does not support the rescan feature, meaning that when initial scan for devices is complete it is impossible to add new devices without restarting the bundle and the dongle. Also in source code ZigBeeCoordinatorHandler is doing so much work, it evens handles a part of device discovery service (ZigBeeCoordinatorHandler calls ZigBeeDiscoveryService to start device discovery, but then ZigBeeDiscoveryService calls ZigBeeCoordinatorHandler to handle the device discovery) and I honestly don’t understand why. There are also a few methods called for no reason, or their return values not checked. Furthermore, the pool request for initial contribution of ZigBee binding to openhab2-addons repository has failed for some unknown reason (if you click on Details you get a 404 error). I know @Kai wanted to move ZigBee binding to a separate repository, but I cant find it yet. Has that been done?

Adding the rescan functionality shouldn’t be too big of a problem for me, though I would appreciate if someone pointed me the right way :slight_smile: .

A bigger issue for me would be refactoring of the code (mainly ZigBeeCoordinatorHandler), to help it keep more maintainable, so I would like to discuss such a possibility here. Any thoughts for such a possibility?

2

No - if you enable discovery it should find new devices. Starting discovery should call the methods to start a scan.

From memory, the coordinator handler handles all the interaction with the coordinator. It handles the notifications from the coordinator when new devices are found, and it calls the discovery handler to add the device into the system. Maybe this could be split differently, but the coordinator is the heart of the network, so it’s going to be a large class. That said, it’s not actually very big (500 lines I think…

Fur sure the zigbee binding cold do with some TLC, so please feel free.

No - we have just moved the ZWave binding only 2 days ago and this was a trial.

Maybe I’m lost by what you mean, but discovery should already be handled in the binding (if that’s what you mean by rescan?).

3

Maybe I can clarify the rescan problem a little bit. We tried the Zigbee binding and the detection of new devices works quite fine in the first 30 seconds. Then the discovery mode ends and we have not found a button the reactivate the descovery mode to add new devices.
For example within the z-wave binding there is a icon on the top to reactive the scanning mode to detect and add new devices. Pressing this button in the Zigbee Bindung at least on our systems does not reactivate the discovery mode. Maybe only a “Click” handler is not working properly.
Currently we have to restart Openhab to add new devices, which I assume is not the intended way to add ZigBee Devices.
So our idea is to fix or add such a “Click” handler to start thje discovery mode. Maybe you can give us a hint if the handler is just missing or if there is a bug, why it is not called on our systems.

4

Thats strange, because when I am trying to add new devices to the network after some time, they can not be found. Also in PaperUI there is no scan button in Inbox for ZigBee binding. There is also the problem of dungle timing out if you start/restart OH2.

5

I’d need to check if the latest code has changed anything - maybe the discovery service is no longer registered properly with some changes that have taken place in ESH. If the discovery service isn’t registered then there will be no discovery option in HABmin.

These dongles have a habit of locking up unfortunately. I’m not sure if that’s what you are seeing or not - again, if you like to propose a PR, then please feel free :wink:

6

@chris just as a short clarification. @Saukijanas is working in the office next to me and we had a discussion this morning about contributing to the Zigbee Binding in which I proposed to first discuss ideas witihin the OH2 community.

7

In PaperUI, or in HABmin, this should be the discovery button. In HABmin, if ZigBee is not listed, then it means there is something wrong with the registering of the discovery handler. This should work (and did previously) so may now be an incompatability. You don’t need a ‘click handler’ though (not sure what that is) - this should be handled through standard ESH processes.

8

To be honest, have not had the chance to have a deeper look into the ESH details and mechanisms. I am quite sure that my “clickHandler” is the Discovery Handler which should be enabled/executed when the discovery button is pressed. I was think in terms of plain old java applications with OnClick methods and all that stuff :wink:

9

Hi Chris,
Was looking at the zigbee binding this weekend as well and also found that discovery seems only be triggered the first time and never ever after. The framework doesn’t seem to recognize the discovery service registration. Hence even typing ‘smarthome discovery start zigbee’ results in an error message that discovery is not available for the zigbee binding.
Tried to look the code for the service registration, but could not detect 123 why that does not seem to work as expected

10

Yes - I also noticed this last night. I’ve bene busy with a couple of other things over the past couple of days but I’ll take a look at why the discovery service isn’t registering.

Cheers
Chris

11

To be exact, when you run smarthome:discovery start [zigbee bundle id] in your log you get a warning for nternal.DiscoveryServiceRegistryImpl saying " No disccovery service for binding id [zigbee bundle id] found! " where [zigbee bundle id] is you zigbee bundle id or binding call?

I looked over how ESH suggests to implement a discovery service for a binding, and went through your code again, I noticed that you use a default constructor with only a TIMEOUT variable when calling AbstractDiscoveryService this constructor by default sets background discovery service to be true, but you haven’t implemented any background discovery method in ZigBeeDiscoveryService. Was this intentional and I just dont understand something simple or could it be part of our problem? I tried changing it in line 42 of ZigBeeDiscoveryService

super(SEARCH_TIME);

into

super(null,SEARCH_TIME,false);

but it hasn’t helped, so it’s could be something more.

Also, I noticed some strange behavior from the binding: when a thing is discovered and added from Inbox section in PaperUI, the bundle has to be restarted to finalize the initialization, otherwise it will be stuck initializing in PaperUI → Configuration → Things. I haven’t tested this in HABmin though…

12

Yes - as stated above, I checked this a couple of days back and found that the discovery service is not being registered for some reason. I need to find why but I’ve had some other things to work on over the past 2 days…

Why do you want to implement background discovery?

IMHO we don’t need the background discovery which is why it’s not implemented. For ZigBee (and also ZWave) I use the active scan where the user requests a scan rather than having scanning enabled all the time which is (IMHO) a bad idea for security reasons., For the normal discovery, when someone clicks the discovery button, startScan is called so there is no background discovery needed.

Again, at the moment the discovert service is not being registered, so this code is not running I think. If you check in the debugger this should be obvious but that’s how it seemed when I ran it on Tuesday.

Strange - I’m surprised that this locks up the UI, and I’m more surprised that a locked up UI is resolved by restarting the binding.

This might be associated with configuration changes - I’ll try and have a quick look at this later. There have been a lot of changes in ESH since the ZigBee binding was written and I can guess that some of these might now cause some problems in the binding.

13

Ahh, a little miscommunication, let me clear things up. I don’t want to implement a background discovery service, I only state what I think ZigBeeDiscoveryService is doing when AbstractDiscoveryService is called with:

super(TIMEOUT)

which, from what I understand, leads to ZigBeeDiscoveryService calling default background discovery method (which is empty), and to avoid it we should call:

super(null, SEARCH_TIME, false)

This is because AbstractDiscoveryService constructor is overloaded 3 times and calling it like this will actualy call all of the constructors with default values sequentially, and one of those default values (backgroundDiscoveryEnabledByDefault) is set to true.

To be honest, I am not sure how will ESH handle an empty background discovery method, it might just ignore it all together, in which case changing this wouldn’t really do much.

Thanks for the tip, I will try to check it out with debugger and see what I can come up with.

14

I will take a look at this tonight as well - I started on Tuesday night, but had to finalise the persistence issues in ESH and HABmin issues, so didn’t finish this. I don’t think it can be too big an issue to resolve :slight_smile:

15

I’ve found the issue with the service registration. When I last updated the binding I missed out a method in the discovery handler. I’ll push an update soon.

16

Thanks Chris! It’s working fine now. Though I have a few questions relating clusters. It would be great if you could anwser them.

My goal now is to implement a few clusters: Basic, Identify, Groups, Scenes, Relative Humidity Measurement and Temperature Measurement . I found ZigBee Cluster Library Specification online and based my work around it and a few clusters you have implemented.

First I grapeled the Basic cluster, which holds the device information. It has a lot of parameters (12 to be exact) so I am not sure how to update channel with them. I’ve written the code like this:

Quick note, these calls are wraped in try catch block!

// Can I update ChannelState for multiple parameters like this?
updateChannelState(new DecimalType(zclVer));
updateChannelState(new DecimalType(appVer));
updateChannelState(new DecimalType(hwVer));
updateChannelState(new DecimalType(stackVer));
updateChannelState(new DecimalType(pwrSource));
updateChannelState(new StringType(manufName));
updateChannelState(new StringType(modelID));
updateChannelState(new StringType(dateCode));
updateChannelState(new StringType(locDesc));
updateChannelState(new DecimalType(physEnv));
updateChannelState(RawType.valueOf(alarmMask)); // get raw type from string

Also, the last parameter alarmMask is a bitmask and I am not sure how to handle it as well. I get it like this:

String alarmMask = (String) attrAlarmMask.getValue(); // Should I cast it to string, or is there a better way?

Identify cluster (it’s responsible for device Identification mode) was actually straight forward, but Groups cluster wasn’t. (This cluster is responsible for endpoint grouping) It has only one 8 bit parameter called NameSupport.This parameter’s MSB shows whether NameSupport is available or not (it’s read only), nothing difficult there, the intresting bit is commands. There are 6 of them and ZigBee API supports all of them, but I am not sure how to seperate a couple of commands from one another. To be specific, I don’t know how to identify these commands:

public void addGroupIfIdentifying(int groupId, String name); 
public void removeAllGroup();

Furthermore, in some clusters (like Groups or Basic) I am not sure how to implement receivedReport() and what to proccess inside it.

When I was working with Scenes cluster I couldn’t implement method:

AddSceneResponse addScene(AddScenePayload scenePayload)

How can I get AddScenePayload from a command?

While we are discussing this, when should I dispose of converter?

I haven’t yest started to implement Temperature Measurement and Relative Humidity Measurement so pointers would be appreciated!

P.S. Sorry for bad post format, for some reason in some parts of the post things like Bold, Italic, end of line are just not recognized. Don’t know why :cry:

17

Why would you associate a channel to static information? I don’t really see the point in this - you could add it as a property - at least that would be my recommendation. Or maybe you have a usecase to use this as a channel that I’ve not thought of?

I’m not familiar with what this is without reading up - what do you want to do with this though? Depending on what you want to do with it should determine how to process it as it needs to be useful to the end user in the use case you define.

I think you should look at what you are trying to provide the user here - I might be wrong, but it looks like you are simply trying to provide all possible information in all clusters as channels, and I really don’t think this is the way to go. I suggest we discuss this first?

For information, humidity and temperature are already implemented here, and I think I also implemented the power monitoring but I need to check that. I haven’t pushed these classes but I’ll look to update this shortly.

This is used to process the reports - clearly this is dependant on the cluster and some clusters probably don’t support reports (I doubt basic does for starters). If reporting isn’t supported, then this method isn’t needed and you should remove it.

Why can’t you implement it?

Converters are used in the thing and should therefore be disposed of when the thing is disposed of. The garbage collector should sort this out I think?

18

I’am looking for for a better solution to control my Hue lamps with OpenHAB. This binding would be perfect for this. I’m currently waiting for the OH2 release. (I don’t want to use beta software) But I have a question regarding this binding. Will this binding also support the Hue Tap Switch?

19

I’m not sure - I think so, but I’ve only tried it with Hue bulbs and don’t personally have the Tap to test.

20

Yeah, that is a way better idea, to do that I would have to use method:

updateProperties(Map<String, String> properties);

Am I correct?

In ZigBee API method AddSceneResponse addScene(AddScenePayload scenePayload) the variable scenePayload is very contrived in the end I just opted out of doing it and chose a simpler solution.

Strange, I couldn’t find these clusters in handler/clusters directory…