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

X10/Insteon: Remove Line Which Attempted to Add the X10 Device to the PLM #322

Merged
merged 1 commit into from
Nov 21, 2013

Conversation

krkeegan
Copy link
Collaborator

Symptoms: the PLM states parameter contained the entire hashes of any X10 devices. This made get_states useless on the PLM (it should return an empty array). Instead it returned a bunch of garbage. This further lead to crashed in the json and xml servers.

Cause: the $interface->add($X10_Object) command was calling the add subroutine in Device_Item. I am not even sure how the inheritance allowed this to happen. But this routine expected a list of states to be passed as the first parameter not an object.

Solution: Remove the offending call to add. It is not clear why this was used (the commit notes demonstrate that this was added when support for X10 over Insteon was added). Perhaps the drafter intended to add the X10 device as a member of the PLM? However, the PLM itself has no members, PLM scenes are allowed to have members. Alternatively, perhaps the drafter intended to add the object to the list maintained by InsteonManager? Adding some code to the X10_Item would allow this to work in theory, however it isn't clear to me why you would want this. The list is maintained so that the insteon_manager can assist in link management, something not done by X10 devices. We have also been operating without adding the X10 items to the object list for quite some time without incident. Indeed no current calls to find_members would even return an X10 device as they all specifiy Insteon packages. Finally, perhaps this is some vestige of the old Insteon code.

Removing the offending line solves the issue, and does not appear to have any adverse effects.

… PLM

Symptoms: the PLM states parameter contained the entire hashes of any X10 devices.  This made get_states useless on the PLM (it should return an empty array). Instead it returned a bunch of garbage.  This further lead to crashed in the json and xml servers.

Cause: the $interface->add($X10_Object) command was calling the add subroutine in Device_Item.  I am not even sure how the inheritance allowed this to happen.  But this routine expected a list of states to be passed as the first parameter not an object.

Solution: Remove the offending call to add.  It is not clear why this was used (the commit notes demonstrate that this was added when support for X10 over Insteon was added).  Perhaps the drafter intended to add the X10 device as a member of the PLM?  However, the PLM itself has no members, PLM scenes are allowed to have members.  Alternatively, perhaps the drafter intended to add the object to the list maintained by InsteonManager? Adding some code to the X10_Item would allow this to work in theory, however it isn't clear to me why you would want this. The list is maintained so that the insteon_manager can assist in link management, something not done by X10 devices.  We have also been operating without adding the X10 items to the object list for quite some time without incident.  Indeed no current calls to find_members would even return an X10 device as they all specifiy Insteon packages.  Finally, perhaps this is some vestige of the old Insteon code.

Removing the offending line solves the issue, and does not appear to have any adverse effects.
krkeegan added a commit that referenced this pull request Nov 21, 2013
X10/Insteon: Remove Line Which Attempted to Add the X10 Device to the PLM
@krkeegan krkeegan merged commit dcb3227 into hollie:master Nov 21, 2013
@krkeegan krkeegan deleted the fix_x10_plm_issue branch January 18, 2014 03:25
@krkeegan krkeegan restored the fix_x10_plm_issue branch January 18, 2014 17:02
@krkeegan krkeegan deleted the fix_x10_plm_issue branch January 18, 2014 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant