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

Added PulseLight add-in #141

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

coxfrederic
Copy link

I needed a custom add-in for a lightbulb that should be able to be turned on/off in Homekit but always sends a value of 1 to KNX. This is because I simulate pushing a button on a 433Mhz remote to turn on / off my lights for the hood of my kitchen. The PulseLight does exactly this. Since it is not possible to read any status I didn't include this in PulseLight. This means that Homekit is responsible for remembering the state.

@coxfrederic coxfrederic mentioned this pull request Feb 2, 2019
var newValue; //Value for Homekit
if(field === "On"){
if((knxValue? 1:0) !== (oldValue? 1:0)){
this.myAPI.setValue('On', newValue);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you never set newvalue to anything, what will be sent to homekit?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code might be removed, as Homekit is the only source for setting the light there is no state to be send. Homekit remembers the state itself based on the signal send to KNX. There is no status to listen to for this PulseLight. I used existing code as base and tested it so it worked. Might be that there is code that could be removed / does not do anything special?

onKNXValueChange(field, oldValue, knxValue) {
var newValue; //Value for Homekit
if(field === "On"){
if((knxValue? 1:0) !== (oldValue? 1:0)){
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't you compare old and knx values directly?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code might be removed, as Homekit is the only source for setting the light there is no state to be send. Homekit remembers the state itself based on the signal send to KNX. There is no status to listen to for this PulseLight. I used existing code as base and tested it so it worked. Might be that there is code that could be removed / does not do anything special?

@snowdd1
Copy link
Owner

snowdd1 commented Feb 11, 2019

Hi @coxfrederic Fréderic, thanks for submitting your PR. I have to admit I don't understand the code completely.

I thought it should send a 1 to KNX regardless of the prior state? And it should remember the state somewhere (outside homekit)?

I left some comments in the code view.

@coxfrederic
Copy link
Author

Hi @coxfrederic Fréderic, thanks for submitting your PR. I have to admit I don't understand the code completely.

I based it on an existing addin because I'm new to all this,sorry this might make it harder to understand the code.

I thought it should send a 1 to KNX regardless of the prior state?

This is correct

And it should remember the state somewhere (outside homekit)?

No, only inside of homekit it should remember the state

I left some comments in the code view.

Thanks, I tried to reply to them, hope it is more clear now. Thanks for your time and help!

@coxfrederic
Copy link
Author

One important detail: If in homekit the state is "off" it should not send a 1 to KNX if for automatisation reason the value should be set to off. I fixed it now and it works but before it would put the light on again. So the code from the PR works for me, been using it for a few days and works perfectly here.

@coxfrederic
Copy link
Author

Can this be included? I just updated my homebridge-knx installation and it failed since PulseLight.js was missing for handling my knx_config file, so I added the PulseLight.js in lib/addins myself so the installation works now.
If I need to make any changes let me know. Thanks.

@snowdd1
Copy link
Owner

snowdd1 commented Jun 11, 2020

Hi Fréderic,

I would like you to add a comprehensive note what the pulselight add-in really does, what the use case is, because I think that is not self-explanatory.
The code lines you commented on as "might be removed" - please decide whether they are useful and can stay in the code, or if tey need removal.
Please add those commits to your branch, they get added to the PR automatically.

Best regards
Raoul

@coxfrederic
Copy link
Author

Hi Raoul,

Regarding the use case:

I have a KNX installation setup with multiple lights, and all work fine in HomeKit. There was one "light" that I could not add in HomeKit, and that was the light of my kitchen hood. This works with a special remote and it could not be connected to the KNX installation. So what I ended up doing (well actually my father-in-law) was to attach the remote to the KNX installation. By doing so (we actually pulled the remote apart and re-connected the wires) we now have a "hidden" remote behind our kitchen which gets a pulse from the KNX installation. This pulse will change the status of the light. So I can not switch it on or off, all I can do is say "change status" and it will change the light.

So I tried using a regular light in HomeKit for this, but that didn't work, because it would not send a pulse if the light went off. Also I had some problems if the light was on, and HomeKit scenes would need it to be on, it would turn off again .. So then I create the PulseLight.js and tested with it until it worked for my use-case.

Regarding the code:

I need to take a closer look and run some tests, because I don't understand the code myself entirely. I just adjusted small parts to make it work for my use case.

@snowdd1 snowdd1 marked this pull request as draft June 17, 2020 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants