search

kpsuperplane / homebridge-wiz-lan

Control Wiz lights over LAN
Apache License 2.0
115 stars 37 forks source link

Use of getCharacteristicValue creates infinite loop! #24

Closed Supereg closed 3 years ago

Supereg commented 3 years ago

The method getCharacteristicValue calls .getValue() on the characteristic object.

See https://github.com/kpsuperplane/homebridge-wiz-lan/blob/0253f46c144782a55fe6a418b8d31b18040734e9/lib/homekit-device/index.js#L87-L94

There are two issues with that

Thus, in the case of https://github.com/kpsuperplane/homebridge-wiz-lan/blob/0253f46c144782a55fe6a418b8d31b18040734e9/lib/wiz-accessory.js#L153-L155 this will actually create an infinite loop, as the get event will fire a getValue() which again fires an get event an so on.

Instead of using .getValue() just use the property .value to retrieve the current value. But if your "get" handler just consist of return .value you can just omit registering a "get" handler, as returning .value is the default behavior.

A stack trace created with the latest beta

Name@Homebridge B03F] Unhandled error thrown inside read handler for characteristic: Error: characteristic value expected string and received object
    at Characteristic.validateUserInput (/usr/local/lib/node_modules/homebridge/node_modules/hap-nodejs/src/lib/Characteristic.ts:1917:17)
    at /usr/local/lib/node_modules/homebridge/node_modules/hap-nodejs/src/lib/Characteristic.ts:1428:24
    at /usr/local/lib/node_modules/homebridge/node_modules/hap-nodejs/src/lib/util/once.ts:9:18
    at Characteristic.<anonymous> (/usr/local/lib/node_modules/homebridge-wiz-lan/lib/wiz-accessory.js:154:11)
    at Characteristic.emit (events.js:315:20)
    at /usr/local/lib/node_modules/homebridge/node_modules/hap-nodejs/src/lib/Characteristic.ts:1404:14
    at new Promise (<anonymous>)
    at Characteristic.<anonymous> (/usr/local/lib/node_modules/homebridge/node_modules/hap-nodejs/src/lib/Characteristic.ts:1402:12)
    at step (/usr/local/lib/node_modules/homebridge/node_modules/tslib/tslib.js:140:27)
    at Object.next (/usr/local/lib/node_modules/homebridge/node_modules/tslib/tslib.js:121:57)
mitch7391 commented 3 years ago

@kpsuperplane, @dotkrnl FYI this makes homebridge unusable in the latest beta and I assume it will for future stable releases once this beta moves to stable.

kpsuperplane commented 3 years ago

Hi everyone I really appreciate the effort you've done to get to the bottom of this but it's pretty difficult for me to make any patches to this plugin since I don't own an actual bulb to test on..

More than happy to approve a PR though (or even hand this repo over to one of you fine people!)

mitch7391 commented 3 years ago

@kpsuperplane I really wish I had the skill set to be able to take on maintaining this plug-in; but I sadly do not have the time to learn completely everything, only dabble as I go and struggle with the minor parts I have learned so far. This however looks like a change that would not require to have the lights if I am not mistaken; but if you just no longer have the interest to continue maintaining the plug-in I do understand that.

@Supereg I had attempted to make the change you suggested in my local install and completely killed Homebridge and then reverted the changes. I feel bad dragging you into a repo that isn't yours, but this could be a show stopper for everyone using this plug-in once the new Homebridge beta goes stable.

Supereg commented 3 years ago

@kpsuperplane To my knowledge we have an initiative where we take ownership of unmaintained plugins to find new maintainers. See here, if you are interested.

EDIT: the plugins are hosted in a dedicated GitHub org until someone proposes to take ownership. The process is also explained in the org itself, here.

In any case, I can try to have a look at it and create a PR for it. To my knowledge this should a doable fix. Though not sure why it doesn't already cause problems with the current versions of homebridge.

mitch7391 commented 3 years ago

That is really good to know about that initiative, I had no idea! This could be handy for a few of us on the repo.

Supereg commented 3 years ago

Looked at the code, the issue description is actually wrong. The plugin has its own characteristic representation with its own getValue function, which makes everything rather complicated as both are named the same but aren't the same thing. I think the same confusion occurred also when writing the code, cause https://github.com/kpsuperplane/homebridge-wiz-lan/blob/0253f46c144782a55fe6a418b8d31b18040734e9/lib/wiz-accessory.js#L147-L150 is called with the constructor like Characteristic.On whereas this example expects the internal representation having a props property, meaning the if body is never called as constructors don't have the props property.

This also explains the error a bit better, the issue seems to be somewhere else. However this is particularly hard as I don't know the code and can't run the code.

kpsuperplane commented 3 years ago

@Supereg yeah the plugin unfortunately inherits the complexity of the Kasa plugin that's it's based off of.

I tried my best to remove a lot of the cruft but it's definitely still difficult to debug without just diving head in for an hour or two

Supereg commented 3 years ago

@mitch7391 Do you have any configuration for the wiz plugin? Like discovery options? Anything special?

iRayanKhan commented 3 years ago

@Supereg I'll provide my config when I get home. As far as I remember, I just defined the plugin, and devices added themselves.

Supereg commented 3 years ago

Yeah just wanted to verify that I didn’t mess up anything in the above mentioned pull request. But i think your issue over at homebridge also included configuration for wiz and it did indeed not include anything special.

mitch7391 commented 3 years ago

This is all that is required for the config:

{
            "platform": "WizSmarthome",
            "name": "WizSmarthome"
        }
mitch7391 commented 3 years ago

@Supereg yeah the plugin unfortunately inherits the complexity of the Kasa plugin that's it's based off of.

I tried my best to remove a lot of the cruft but it's definitely still difficult to debug without just diving head in for an hour or two

@Supereg just a thought on the complexity of this plug-in due to the plug-in it is originally based off. Would it be an easier task to recreate this plug-in using the official 'template' that Homebridge has now provided? Might be a silly question, I have only just started to learn how to write some basic shell scripts myself recently; so excuse my ignorance haha.

Supereg commented 3 years ago

I don't think a rewrite is easier than just refactoring the plugin a bit (or moving to Typescript for better type awareness/safety. I think this could solve some confusion for possible future maintainers around the internal characteristic type and the HAP-NodeJS/homebridge provided one). But again, this is bot not really feasible if you don't have the hardware to test it on, even if you heavily invest into stuff like unit testing.

Supereg commented 3 years ago

Closing this issue as the above mentioned error was fixed (although initially falsely analyzed). See #25