Use of getCharacteristicValue creates infinite loop!

[Supereg]

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

See

homebridge-wiz-lan/lib/homekit-device/index.js

Lines 87 to 94 in 0253f46

	getCharacteristicValue(characteristic) {
	this.log.debug(
	'[%s] getCharacteristicValue',
	this.name,
	this.platform.getCharacteristicName(characteristic)
	);
	return this.getCharacteristic(characteristic).getValue();
	}

There are two issues with that

• getValue() doesn't return anything at all. Its a callback based method, where the resulting value is passed into the callback passed to the method
• getValue() is the function called when handling a request. It will query the the read handler of the plugin. e.g. what is set up for the GET request.

Thus, in the case of

homebridge-wiz-lan/lib/wiz-accessory.js

Lines 153 to 155 in 0253f46

	c.on('get', cb =>
	cb(null, this.hkDevice.getCharacteristicValue(characteristic))
	);

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]

@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]

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]

@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]

@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]

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]

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

homebridge-wiz-lan/lib/wiz-accessory.js

Lines 147 to 150 in 0253f46

	const props = this.hkDevice.getCharacteristicProps(characteristic);
	if (props) {
	c.setProps(props);
	}

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]

@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]

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

[iRayanKhan]

@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]

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]

This is all that is required for the config:

{
            "platform": "WizSmarthome",
            "name": "WizSmarthome"
        }

[mitch7391]

@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]

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]

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