Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move back "add nodes" from discovery extension to network manager #1232

Closed
mikomarrache opened this issue May 7, 2021 · 2 comments
Closed

Comments

@mikomarrache
Copy link
Contributor

After analyzing the code, we saw that the responsibility of adding nodes to the network manager has been left to the discovery extension.

Is there a reason for that? By definition, an extension should not be a critical module for the proper function of the library. But it seems that the library would not work without this extension.

I can see the ZigBeeNetworkDiscoverer class implements the ZigBeeAnnounceListener interface so that it can add nodes when device status becomes UNSECURED_JOIN, SECURED_REJOIN or UNSECURED_REJOIN. Why is that logic not in the ZigBeeNetworkManager.nodeStatusUpdate method?

I have the same question about reacting to DeviceAnnounce commands. The network manager could register itself as listener for DeviceAnnounce commands.

@cdjackson
Copy link
Member

Is there a reason for that?

At the time, it was done this way so that different discovery systems could be used as I wasn't sure if the way it was being done was best in all situations.

By definition, an extension should not be a critical module for the proper function of the library.

While I agree in principal, it is a fine line. Without the OTA extension you can't do OTA - but the library will still work. It is the same here - basic functions work and if you have a different way to discover nodes or initialise the network, then the library should work.

I am not against changing some of this if you have suggestions as I do think there is room for improvement. See also #1108 which is sort of related to the DeviceAnnounce question. In general I still think that discovery is too chatty, and the issue in #1108 needs to be resolved as a child moving to a new parent will send an announce, but it may not be able to respond to commands.

So... if you have ideas, please feel free to discuss :)

@stale
Copy link

stale bot commented Jul 8, 2021

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.

@stale stale bot added the wontfix label Jul 8, 2021
@stale stale bot closed this as completed Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants