ZDO_SIMPLE_DESC_RSP timeout

[cdjackson]

@presslab-us it looks like your changes in 3972948 might have broken something. I'm now getting the following error getting the ZDO_SIMPLE_DESC_RSP during startup (at least when the network isn't restored off disk) -:

00:02:28 445  DEBUG  Inspecting node #0 (00:12:4B:00:02:F1:DB:5B) / end point 1.
00:02:28 446  DEBUG  -> ZDO_SIMPLE_DESC_REQ (Packet: length = 5, apiId = 0x25 0x04, full data = 0xfe 0x05 0x25 0x04 0x00 0x00 0x00 0x00 0x01 0x25, checksum = 0x25, error = false, errorMessage = null) 
00:02:28 452  DEBUG  <- ZDO_SIMPLE_DESC_REQ_SRSP (ZDO_SIMPLE_DESC_REQ_SRSP{Status=SUCCESS(0)})
00:02:34 463  DEBUG  Inspecting ZigBee EndPoint <0,1> failed during it 1-th attempts. Waiting for 1000ms before retrying
00:02:34 464  DEBUG  Inspecting node #0 (00:12:4B:00:02:F1:DB:5B) / end point 1.
00:02:34 464  DEBUG  -> ZDO_SIMPLE_DESC_REQ (Packet: length = 5, apiId = 0x25 0x04, full data = 0xfe 0x05 0x25 0x04 0x00 0x00 0x00 0x00 0x01 0x25, checksum = 0x25, error = false, errorMessage = null) 
00:02:34 469  DEBUG  <- ZDO_SIMPLE_DESC_REQ_SRSP (ZDO_SIMPLE_DESC_REQ_SRSP{Status=SUCCESS(0)})

It seems if I revert back to the previous master on the 21st June (your fix of the not implemented exception) that this works ok again, so it seems to be the change on the 24th to add the HA meter clusters that has caused this...

Are you also seeing this?

Cheers
Chris

[cdjackson]

It looks like the following is causing it in ApplicationFrameworkLayer -:

        final int[] clusters = new int[clusterSet.size()];
        if (clusters.length > 33) {
            throw new RuntimeException("Too many default clusters.");
        }

If I change this to 34 and add another cluster to the cluster set as per your change to add the measurement cluster, then it stops working properly...

[presslab-us]

Ahh, yes cleared my persistence and I did see that error. I think I have found the reason, too. The ZDO_SIMPLE_DESC_RSP only supports 33 clusters. This seems to be a limitation of the TI Z-Stack Monitor API.

We could just change it back to 33, but the problem there is that these new clusters can't be bound, and therefore no attribute reporting. It's only going to get worse as we add support for more clusters.

zigbee4java doesn't allow access to any endpoints of node #0, as that is the coordinator node. Let me figure out what it actually uses this information for. Have you found any problems other than the error message?

[presslab-us]

Z-Stack also limits the AF_REGISTER to 33 clusters. I don't see why we can't just create a new endpoint when we run out. I'll give this a shot.

As an aside while looking at the code I think I see another bug. Here is the code that deals with the coordinator endpoints. It looks like it registers the output clusters if it's a coordinator, and if not it adds the endpoint to the network list.
https://github.com/tlaukkan/zigbee4java/blob/master/zigbee-api/src/main/java/org/bubblecloud/zigbee/network/discovery/EndpointBuilder.java#L157-L166

However the code here also registers the output clusters. It looks to me like it ends up being done twice.
https://github.com/tlaukkan/zigbee4java/blob/master/zigbee-api/src/main/java/org/bubblecloud/zigbee/network/impl/ApplicationFrameworkLayer.java#L202

[presslab-us]

With this commit b380e13 I no longer see the problem. Let me know if you have any more issues with it.

It also allows for greater than 33 clusters by using multiple endpoints. I'll look at the other potential bug later, to see if it's really an issue.

[cdjackson]

Great- that fixes it thanks. I don't notice any other issues at the moment, but my network is small (I'm currently in a hotel as I'm travelling for work so have just grabbed a couple of devices to play with :) ).

Can I make a suggestion - I don't think developing directly into the master branch is a wise move. I think we should develop in separate branches and then create a PR to pull across the changes so that someone else can have the chance to comment on the changes... It's not related to this change as I suspect no-one would have noticed it, but I think it's good to have someone review changes for a bit of a sanity check...

Just a thought :)

[presslab-us]

Heh, yeah I thought you might be thinking as such, knowing your style of a pull request for small changes. ;) I know I asked @tlaukkan a while ago if changing master was preferred vs a branch, and at least at the time he said going right to master was fine.

In the case of this issue the changes to the existing code was pretty small (just a few lines), but one of those lines caused the error above. One solution would be to create a work branch where the unstable changes live until they are tested/commented on and then moved to a stable branch eventually.

I'm not sure the overhead of numerous pull requests is appropriate at this point... If the project were more mature and it was used in production then that might be more appropriate. Even for commits though anyone can comment on them and things can be changed, so it's not like using commit vs pull request doesn't allow code review.

Let's see what @tlaukkan thinks about all this. I think something like a work branch would isolate the master and allow us to review the code before merging back into master. Using pull requests seems like unnecessary overhead at this stage of development. But whatever Tommi thinks is appropriate is fine by me.

[cdjackson]

Hey Ryan,
I was just thinking of trying to get some sort of review for changes - at least anything significant. Again, this isn’t related to this change - I just thought I’d raise it :) I don’t suggest to change the way anyone codes and commits, but as there are a few of us working on this it just seemed like a good idea to avoid / reduce any screwups… I do the PRs to make sure others have visibility of the changes before they’re merged, otherwise you need to go through all the commits to find what’s changed…

It was just a thought aimed at providing a level of review to keep up the code quality - I personally think it helps for anything more than a few lines (not just for changing timer values or comments etc) but if others think it’s not needed them I’m fine with that…

Cheers
Chris