Allow specifying a primary in SimpleNodeServer
Closed this issue · 33 comments
The SimpleNodeServer is a good starting point, but it's really meant for situations where your server is managing another hub, like the Hue, where the Hue Hub is the parent of all the other nodes created under it.
Instead of duplicating all the code for SimpleNodeServer, the on_add_all method could be changed to allow a different primary to be used, instead of:
primary = all_nodes[0]
the caller could specify the primary, or allow the primary to always be the node itself.
Thinking about this more, I think the Node's primary should be stored inside the Node when it's created, instead of tracking the info in a separate dictionary in node server.
Hi jimboca,
It's a good idea. Can you please update the code as you suggest and commit?
With kind regards,
Michel
Yes. Michel, I will branch and test. My worry is I don't have a Hue or Kodi, so I can not do complete testing with any fully running Node Server. It should be a pretty easy change, but will require changes to those servers.
Hi jimboca,
Thank you. Let's see if we can get Ryan to test the change. Ryan?
With kind regards,
Michel
@rmkraus I committed the changes just to show what I am thinking, have not even tried to run the code yet.
@jimboca Never ever commit untested changes to the master branch. The master branch is supposed to be a known stable working version. You branch or fork, then implement and test all relevant changes before merging with the master branch. If you commit untested or WIP updates to the master branch, you break the master branch. In my opinion, UDI should be the only one merging changes with the master branch.
But of course :-) It's on the issue-1 branch now. I understand that UDI wants a set of users to help manage polyglot, so it will be up to us to make changes using a proper methodology, test those changes, and agree what will be merged into the master.
You can test my changes by switching to the issue-1 branch.
This is all working in the issue-1 branch for my node server, and I can at least start up the hue node server without errors. But, since I don't have a hue, can anyone test it, please? :-)
Hue node server is broken. Pertinent portion of error below.
Basically, this change has fundamentally re-written how the node servers are added -- and it looks like the way the nodes are handled/constructed/added in the hue python code has gotten scrambled. The failing code creates a node on-the-fly when it discovers that a lamp has been added to the hue hub. It does so by passing arguments to a helper function which returns a Node object which is passed to the API to add that node -- the arguments to the helper function were changed (not sure they were changed correctly; this stuff is making my head hurt), but the helper function itself was not updated to know about the change in the helper function's API so that it could in turn construct the node differently based on this API change. Here's the line, simplified a bit:
lnode = self.add_node(HueColorLight(self...))
Question: This has FUNDAMENTALLY changed the API, and it seems to me that this change has proven to be very difficult to understand and it's also clearly extremely difficult to track down places where this change has broken an API call and needs fixing. Is it REALLY necessary to do this? What do we gain from this? Is there not a way for the original author to do what they need with the existing API?
My opinion, for what it's worth: Let's back out this code onto it's own branch, and let it simmer on the stovetop for a while -- the implementation is clearly not right yet, but more importantly, it requires that we re-plumb some major guts inside Polyglot, and demands that everyone who's written a node server modify the core of it to accommodate this change. Let's think about this for a while before we do this.
Traceback:
ERROR [03-19-2016 23:58:27] polyglot.nodeserver_manager: Phil: Traceback (most recent call last):
ERROR [03-19-2016 23:58:27] polyglot.nodeserver_manager: Phil: File "/home/mwester/pgdev/polyglot/node_servers/hue/hue.py", line 139, in
ERROR [03-19-2016 23:58:27] polyglot.nodeserver_manager: Phil: main()
ERROR [03-19-2016 23:58:27] polyglot.nodeserver_manager: Phil: File "/home/mwester/pgdev/polyglot/node_servers/hue/hue.py", line 135, in main
ERROR [03-19-2016 23:58:27] polyglot.nodeserver_manager: Phil: nserver.run()
ERROR [03-19-2016 23:58:27] polyglot.nodeserver_manager: Phil: File "/home/mwester/pgdev/polyglot/nodeserver_api.py", line 440, in run
ERROR [03-19-2016 23:58:27] polyglot.nodeserver_manager: Phil: self.poll()
ERROR [03-19-2016 23:58:27] polyglot.nodeserver_manager: Phil: File "/home/mwester/pgdev/polyglot/node_servers/hue/hue.py", line 79, in poll
ERROR [03-19-2016 23:58:27] polyglot.nodeserver_manager: Phil: self, manifest))
ERROR [03-19-2016 23:58:27] polyglot.nodeserver_manager: Phil: TypeError: init() takes at most 6 arguments (7 given)
The arguments to the method were changed because the Node object needs to specify who it's primary node is when it is created. The original author of Polyglot @rmkraus was asked to look at implementing this change, but having not heard back from him, @mkohanim asked me to look at the change Since i do not have a hue hub, I can not test the hue polglot, but I attempted to make the changes without being able to fully debug. This does not cause anyone to do a major rewrite of the existing node servers, you just have to add an argument to the node creation call. I will look at your issue.
Can we not address this need simply by setting the parent node to "None" or setting parent == address when creating the node? I'm not clear on why we need to change the arguments for the creation of a node as well as the add method.
BTW, I certainly agree with the original sentiment of this issue - I too noticed that the Simple Node Server is not simple at all when all you have is a single node to expose! So I'm not opposed to fixing that, and if indeed we need to make non-backward-compatible API changes to do so, then now is a better time than in the future.
Please try again, the current version should work with Hue and Kodi, but since I have neither I can not fully test.
The primary node (the hub itself) is added -- one can see that in the log file in the config record that's updated... and then as soon as the first lamp is enumerated and attempted to be added:
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: Traceback (most recent call last):
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: File "/home/mwester/pgdev/polyglot/node_servers/hue/hue.py", line 139, in <module>
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: main()
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: File "/home/mwester/pgdev/polyglot/node_servers/hue/hue.py", line 135, in main
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: nserver.run()
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: File "/home/mwester/pgdev/polyglot/nodeserver_api.py", line 448, in run
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: self.poll()
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: File "/home/mwester/pgdev/polyglot/node_servers/hue/hue.py", line 79, in poll
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: manifest)
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: File "/home/mwester/pgdev/polyglot/node_servers/hue/node_types.py", line 83, in __init__
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: super(HueColorLight, self).__init__(parent, address, name, parent, manifest)
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: File "/home/mwester/pgdev/polyglot/nodeserver_api.py", line 88, in __init__
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: self.add_node()
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: File "/home/mwester/pgdev/polyglot/nodeserver_api.py", line 185, in add_node
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: self.parent.add_node(self)
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: File "/home/mwester/pgdev/polyglot/nodeserver_api.py", line 489, in add_node
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: super(SimpleNodeServer, self).add_node(node)
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: File "/home/mwester/pgdev/polyglot/nodeserver_api.py", line 424, in add_node
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: primary_addr = node.primary.address;
ERROR [03-20-2016 10:50:33] polyglot.nodeserver_manager: Phil: AttributeError: 'HueNodeServer' object has no attribute 'address'
Is this with the latest version of the development branch? That line 424 does't make sense?
Please update and try again. Sorry, it's hard to test when you don't have one.
Tested, ran all afternoon. So this time it looks good -- the Hue node server is working.
(now we just need someone with a working kodi installation to test that one!)
Awesome, thanks for testing. Yes, hopefully someone uses Kodi?
Something associated with this set of changes I suspect is at the heart of the issues with the config file being mishandled and losing the configuration -- I went back to the most recent commit after the config-file handling changes (1eca521) and found that all was working fine.
The failure sequence is that the config file is complete prior to startup, is read upon startup, is written immediately thereafter, and is lacking all node_server elements in the json section but nothing else. The polyglot server has no node servers, and no indication in the logs appears that any node server was started or even attempted to be started.
I am suspicious that the manifest handling may be broken somewhere in the way the nodes are handled.
No other changes on the development branch after the aforementioned commit appear to touch any of the code in a manner that would seem to impact the manifest or configuration file, as far as I can tell.
Ok, I will look more.
I backed up versions until it was working and it looks like this one broke it 27fe6a7 so I reopened that issue.
I just tested backing out part of that same change -- I concur.
I fixed the config issues. All should be well now. Please test.
Ah yes, much better now. Thanks.
Cheers. Sorry about that. I can't believe I didn't notice that during my testing. Silly me. I'll do better.
This is released and working well, just need someone to verify the Kodi server before closing this issue.
Now you can pass the primary node to the init:
super(FoscamMJPEG, self).__init__(parent, address, name, primary, manifest)
This breaks any code that was not passing manifest by name since the new argument was added. Also, SimpleNodeServer tracks the nodes that are added to it instead of the individual node server having to add to the nodes hash, and it verifies the parent is the node server.
I've developed two nodeservers so far on this and it is working as expected. I think you can close this if you want.
I was leaving it open until someone can verify that the Kodi server is
working properly.
On Thu, Mar 31, 2016, 12:03 PM James notifications@github.com wrote:
I've developed two nodeservers so far on this and it is working as
expected. I think you can close this if you want.—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
#1 (comment)
Good luck 🎯
Kodi node server has been tested, and fixes have been committed. Also found some bugs in it unrelated to this issue, patched those (better fix remains a TODO).
Final things to-be-done before we can close this issue is to review the documentation (in the docs folder here in git), and see if we need to update same to document this API change. @jimboca : can you do that? Not even sure what doc tools we need to regenerate the HTML and PDFs...
@mjwesterhof Thanks for fixing Kodi! I think I updated most of the code documentation, but probably missed some and will review. I am also not sure how to update the HTML and PDF, but will try to figure it out.