search

UniversalDevicesInc / Polyglot

Polyglot Open Source
MIT License
24 stars 2 forks source link

Node Servers and the "ST" command #39

Closed mjwesterhof closed 8 years ago

mjwesterhof commented 8 years ago

The 3AM query (actually, any scene query done programatically) actually issues the REST "command" call to execute the "ST" command, rather than just the REST "query" call. The distinction is not very clear to me - so some documentation fixes are probably in order. Nevertheless, we need to make changes to the node servers to ensure that all nodes register an event handler for the "ST" command, even those (like the Kodi parent node) that do not have any status to report.

I've committed the changes for the Hue and Kodi node servers to the development branch, along with a minor change to the nodeserver_api.py file so that it will print a better message to the log when a node server fails to have support for a command that was issued by the ISY.

This fixes a bunch of ERROR messages in the log at the 3AM query.

I'll close this after we get some feedback on stability, and after an opportunity for comment by other node server authors. :)

cjudi commented 8 years ago

I think this is what you are doing, but just to be clear; Isy v5,0 nodes should implement the 'QUERY' command instead of 'ST'. The node servers certainly can also allow for 'ST', but should not include the 'ST' command in the nodedef. In the Isy, we will be changing all remaining 'ST' command calls to 'QUERY'

The v5.0 Insteon & Z-Wave nodes currently use 'QUERY' instead of 'ST'

In general, a node server should expect to see invalid commands occasionally, and should just have them fail. Thinking out loud, it might make sense for Isy to add something like the following so the Isy won't attempt retries:

/report/request/< reqid >/unsupported

Chris Jahn

mkohanim commented 8 years ago

Thanks so very much Mike!

With kind regards,

Michel Kohanim CEO

(p) 818.631.0333 (f) 818.436.0702 http://www.universal-devices.comhttp://www.universal-devices.com/

From: Mike Westerhof [mailto:notifications@github.com] Sent: Thursday, April 7, 2016 9:15 PM To: UniversalDevicesInc/Polyglot Polyglot@noreply.github.com Subject: [UniversalDevicesInc/Polyglot] Node Servers MUST handle the "ST" command for all Nodes (#39)

The 3AM query (actually, any scene query done programatically) actually issues the REST "command" call to execute the "ST" command, rather than just the REST "query" call. The distinction is not very clear to me - so some documentation fixes are probably in order. Nevertheless, we need to make changes to the node servers to ensure that all nodes register an event handler for the "ST" command, even those (like the Kodi parent node) that do not have any status to report.

I've committed the changes for the Hue and Kodi node servers to the development branch, along with a minor change to the nodeserver_api.py file so that it will print a better message to the log when a node server fails to have support for a command that was issued by the ISY.

This fixes a bunch of ERROR messages in the log at the 3AM query.

I'll close this after we get some feedback on stability, and after an opportunity for comment by other node server authors. :)

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHubhttps://github.com/UniversalDevicesInc/Polyglot/issues/39

mjwesterhof commented 8 years ago

Ok, based on the above comments, we'll just leave this issue open for the moment. I've added positive support for the ST command to the existing node servers already, and while that's no longer necessary for 5.x ISY hosts, it doesn't sound like responding to those correctly causes any harm either. This cleans up the ERROR messages in the log files, and helps a lot in focusing on the real comms issues left during the 3AM queries.

We could add logic that tests if the ISY version is known, and >= 5, then silently drop the ST command on the floor... that would clutter up the nodeserver_manager.py code a bit, requiring it to peek into the commands (which IMO is rather like a protocol layering violation; you do it but only when absolutely necessary).

(Philosophical aside: Regardless of solution, it's important IMO that we do not let error messages creep into the log files for this. There is no such thing as an "expected error" -- if it was expected, then it's not an error, is it?! :-) Users and support staff have the right to expect that an message starting with "ERROR" in the log file does indeed reflect something bad.)

mkohanim commented 8 years ago

Hi Mike,

I agree: not necessary. We should work based on the assumption that polyglot is working against a production version of ISY UNLESS there’s a very huge difference between releases.

With kind regards,

Michel Kohanim CEO

(p) 818.631.0333 (f) 818.436.0702 http://www.universal-devices.comhttp://www.universal-devices.com/

From: Mike Westerhof [mailto:notifications@github.com] Sent: Friday, April 8, 2016 9:08 AM To: UniversalDevicesInc/Polyglot Polyglot@noreply.github.com Cc: Michel Kohanim michel@universal-devices.com Subject: Re: [UniversalDevicesInc/Polyglot] Node Servers MUST handle the "ST" command for all Nodes (#39)

Ok, based on the above comments, we'll just leave this issue open for the moment. I've added positive support for the ST command to the existing node servers already, and while that's no longer necessary for 5.x ISY hosts, it doesn't sound like responding to those correctly causes any harm either. This cleans up the ERROR messages in the log files, and helps a lot in focusing on the real comms issues left during the 3AM queries.

We could add logic that tests if the ISY version is known, and >= 5, then silently drop the ST command on the floor... that would clutter up the nodeserver_manager.py code a bit, requiring it to peek into the commands (which IMO is rather like a protocol layering violation; you do it but only when absolutely necessary).

(Philosophical aside: Regardless of solution, it's important IMO that we do not let error messages creep into the log files for this. There is no such thing as an "expected error"! Users and support staff have the right to expect that an message starting with "ERROR" in the log file, does indeed reflect something bad.)

— You are receiving this because you commented. Reply to this email directly or view it on GitHubhttps://github.com/UniversalDevicesInc/Polyglot/issues/39#issuecomment-207495735

mjwesterhof commented 8 years ago

This code has been tested and is working fine - no documentation impact (we actually don't want to document anything about this change is really just a work-around for ISY behavior that will change later on). Closing this issue.