ZigBeeNetworkManager.refreshnode small fixes

[mikomarrache]

No description provided.

[mikomarrache]

There is one test failing, but I'm not able to make it fail locally so I'm not sure what is wrong here.

[cdjackson]

As a matter of interest, why did you change this? There is no real change to the code - you just use the currentNode instead of node to print the IEEE address, but given that these are guaranteed to be equal, this is the same thing. I don't mind changing it - maybe it's better is some view, but I'm wondering if it caused a bug or you just changed it for some other reason?

The only material change is to the comparison of the NodeDescriptor which is now different. I think the old check is more accurate as it checks if the node descriptor has changed - now it will falsely send updates as it only checks if it's not null.

[mikomarrache]

No. We started discussing internally about the meaning of the nodeAdded, nodeUpdated and nodeRemoved callbacks. Then we saw that nodeAdded was called multiple times which makes this callback meaning not so clear - i.e we expect this callback to be called only once since a node is added only once to a network. I proposed to remove one call in PR #1230. But there is still another call which is made after discovery is performed. I still don't understand why this call should be made after discovery?

[cdjackson]

Then we saw that nodeAdded was called multiple times

This is historical and I think it is documented. IIRC the calls will be made when the device is first discovered, and then again when the discovery of the descriptors etc is complete. To avoid breaking existing users code, we had to stick with the two calls, but at this stage I don't mind changing this. There is also #690 which proposes to change the notification to a single call to try and consolidate this so it might be good to do this at the same time.

I still don't understand why this call should be made after discovery?

As above - the two calls were introduced to avoid breaking existing users at the time.

[mikomarrache]

So, if I understand correctly, #690 proposes to use a separate callback (nodeDiscovered - method or enum) to notify that node discovery has been performed. Again, it relates to the discussion about where discovery should be done (extension or core). As I said in other tickets, I don't understand why the network manager is aware about discovery. I'm also not sure why a nodeDiscovered callback is required. Client code could use nodeUpdated and take decisions based on that.

Maybe what is missing here is passing old node state in the nodeUpdated callback. I think that would be a more generic approach.

[cdjackson]

So, if I understand correctly, #690 proposes to use a separate callback (nodeDiscovered - method or enum) to notify that node discovery has been performed.

It proposes to use a single method to be called when there is a node update and how the nodeAdded, nodeUpdated and nodeRemoved callbacks are provided. It is not about adding a new method relating to discovery. IIRC is proposes to use a single callback method, and a reason, rather than multiple methods and splits the dual "added" calls into two phases - once when the node is first heard, and once when the node information is known.

Again, it relates to the discussion about where discovery should be done (extension or core).

Not really. The node is updated not only when it is discovered. #690 is simply about changing the way that node updates are notified - it's not related in any way to discovery (other than of course discovery is one of the many reasons that a node listener will be called).

As I said in other tickets, I don't understand why the network manager is aware about discovery.

Fair enough, but I think I tried to explain this, and it's not really related to this issue. I've said that I'm happy to discuss discover improvements, but I still think it should be a separate "addon" as some people do not want this, and others may want to do something different. In any case, it is not related to this issue.

Maybe what is missing here is passing old node state in the nodeUpdated callback.

I need to think about how this is used. While at first thought, it seems like it might be a good idea, I'm wondering if it is really useful. To use it, you would need to perform a comparison to see what has changed - which is what is already done when the node is updated. How do you see passing the original and new node structure helping?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[cdjackson]

I got distracted by the conversation which is not really related to this PR, and the changes here are fine so merging.