Disables accessory control when NO_RESPONSE was triggered

[radionoise]

Here is the improvement of NO_RESPONSE implementation. If NO_RESPONSE was triggered and Home.app was not reopened, user still can control device as described here #48. The improvement fixes this undesired behavior.

[Shaquu]

@radionoise thanks for PR!
Could you move code from HAPServiceNode.js to CharacteristicUtils.js?
Maybe do it like this. Merge those two _if_s using || in getSupported into one if (both push to supported array). Add your code in this one if case.

@crxporter and @NorthernMan54
You were active in Issue #48
Could you test this change if it works properly according to you?

[radionoise]

Done

[crxporter]

I’ll be ready to check later today - is there a branch to download with this change alongside the newest dev branch changes?

I struggle with figuring out which version to download...

[radionoise]

I moved the code to getSupported function. However, getSupported function name doesn't suit well because we are creating subscription for set events here. I think that we should extract it to another function, or change the function name to something like getSupportedAndSubscribe or something. @Shaquu what do you think?

[Shaquu]

@radionoise well we can change to the name you have proposed :)
Can you update upstream for @crxporter to let him test on your branch with the newest version?

[Shaquu]

What about the first IF?

if (characteristic.props.perms.indexOf("pw") > -1) {
    supported.read.push(cKey);

    //What in this case?
}

if (
    characteristic.props.perms.indexOf("pr") +
    characteristic.props.perms.indexOf("ev") >
    -2
) {
    supported.write.push(cKey);

    characteristic.on("set", function(newVal, callback) {
	                    callback(node.accessory.reachable === false ? "no response" : null);
	                });
}

[radionoise]

@Shaquu I didn't really get it about merging two IFs in getSupported function. What did you mean exactly? I moved my code into getSupported function as you proposed to get rid of the extra forEach loop.

About first if clause. I think we should subscribe only to writable characteristics.

[radionoise]

@Shaquu I've updated my feature branch with your dev one. @crxporter can test it in my branch https://github.com/radionoise/node-red-contrib-homekit-bridged/tree/feature/no-response-improved

[Shaquu]

I was writing about merging ifs since you were adding subscriber on all elements in supported array previously. And when you moved it to getSupported then you omitted some characteristics. I will not argue. I am not a specialist :) And what you say has a sense - we should only subscribe to writable.
I assume you tested it yourself and it works fine for you?

[radionoise]

Yes that works fine for me. We can wait for @crxporter feedback. I think he would like to test it :).

[crxporter]

I didn’t get to it yesterday, I will test today.

[crxporter]

This works much better than before. I'll put my "stamp of approval" on it.

I used the @radionoise no-response-improved branch.

Some notes will need to be made in the documentation so things are clear that if you are using the On characteristic to send "NO_REPSONSE" then you have to send a new "On" value to clear the no response status. Not a problem, just as always users need to know what they're doing.

I tested with a dimmer. I'm able to send On ture/false/NO_RESPONSE and it works great. I like that if I click it after sending no response (but Home app hadn't updated) then it bumps over to no response. This is how native homekit things usually work! BUT if I set On to NO_RESPONSE and then send brightness values - the switch will still update brightness levels.

Again this isn't a problem just an observation how it works.

@Shaquu from my point of view, 0.6.0 (with the no response added) is ready - no breaking changes from previous release. Only potential is a little extra work switching parent/child nodes if 0.6.0 assumes everything is child.

[Shaquu]

@radionoise could you add some information about features from this PR in README?

@crxporter thanks for tests. It looks like I have to really look close to those child settings as it is not what suppose to happen.

3 yay, 0 nay
As soon as README is ready I will merge it to dev branch.

[radionoise]

I've improved it a little bit more. Now NO_RESPONSE can be triggered on optional characteristic too. Also you can reset it by sending valid value of any characteristic (not only the one you triggered NO_RESPONSE previously). @crxporter can you test it again please?

@Shaquu I will update README as soon as @crxporter will confirm that everything works as expected :)

[Shaquu]

One word. We have a file for tests. If you ever come to idea how to make it better to test our stuff then please tell me or update it yourself. Tests are running using mocha module (we start it from root of project with npm test).

Good to see you still try to improve things, it's a good idea.

[crxporter]

Tested, looks good.

This is going to be much easier to use / explain to users now that the NO_RESPONSE state will go away with ANY other characteristic sent instead of having to send back to the same as before.

[radionoise]

Updated README. @Shaquu It seems that PR is ready for merge

[Shaquu]

@radionoise thank you very much for contribution! I am really happy to see you guys working on that module :)

@crxporter thanks for tips and help with testing!