search

normen / homebridge-milighthub-platform

Homebridge plugin to control MiLight Hub
6 stars 0 forks source link

Some minor improvements & homebridge 1.0.0 compatibility #4

Closed Zer0x00 closed 4 years ago

Zer0x00 commented 4 years ago
normen commented 4 years ago

Thanks. What was the incompatibility? I gotta check my other plugins if they're compatible to hb 1.0..

Zer0x00 commented 4 years ago

It was only the reference in package.json, nothing too extraordinary: https://github.com/normen/homebridge-milighthub-platform/commit/d474aca60f9cf2c51a74853a898013a87e9e71fa

Afaik homebridge itself published a workaround for this issue: https://github.com/homebridge/homebridge/pull/2522

normen commented 4 years ago

I did some cleanups towards a 1.0.0 release, did you have anything you wanted to add before that release?

Zer0x00 commented 4 years ago

Yes there are two more things I want to add/change:

I wanted to mitigate both of these issues by introducting a single "smoothLightning" flag but didn't have any time yet to work on this plugin.

normen commented 4 years ago

1) Should be an easy fix, I guess I simply don't store the cstate properly when the lamp is turned off, I'll check that. 2) Is pretty much the behavior of the MiLights, right? If you send a command with "turn on" and "level 1" then if it was at level 100 before it will flash. I saw other plugins work around that in some ways but I also saw the whole plugin become quite error-prone with various issues cropping up between HomeKits "I send every value by itself in a random order" and MiLights "if you send this command along with it I'll do something else". But if you have a good idea on how to avoid this I trust you with this 🙂

normen commented 4 years ago

https://github.com/normen/homebridge-milighthub-platform/commit/f04f82bf5fb0b2c8ce772eb79a9320790e03b57d might fix the first issue

Zer0x00 commented 4 years ago

Thanks, but unfortunately it didn't change the behaviour of the first issue.

Regarding #2 I was thinking about setting the brightness to 1 during "turn off"-command, afaik the MiLight Hub does handle this well but nothing sure at this point, have to check it twice before realizing ;)

normen commented 4 years ago

Alright, I put the last touches on this and ran a code formatter on it. As theres no real changes for users until now I won't publish a new version and I'll leave the repo to you for now.

normen commented 4 years ago

I shamelessly lied, I had to push a version fixing a bug when no rgbcctMode is set at all in config.json.

Zer0x00 commented 4 years ago

Nice find :)

Thank you that you are awaiting the last changes for me to push, but this will take some time.

I am also currently evaluating a grouping function as well, as I know that the lamps support 3 or 4 pairings (and I need more). If you do more than the supported number of pairings, the first pairing is simply forgotten. So my workaround would be to integrate a grouping in this plugin but I don't know how well this will work especially the timing & making sure that all commands get to the lamps.

But time will tell if I have enough motivation to realize all of these, especially the grouping function could mean a major rework of the MiLightHubPlatform class. :/

normen commented 4 years ago

Yeah, groups are ugly, I think they don't even update the single lights within the milight hub, thats why I avoided them. You can use them with the plugin but as said it's not very nice.

If at all I'd of course like the plugin to handle this "automagically" somehow. Maybe it can be groups of groups? Like the plugin could check if it's a group alias and if it has the same prefix as another group alias they get displayed as one, using the prefix name.

Zer0x00 commented 4 years ago

It's on my personal roadmap, I'll try to realize this as beautiful as I can 👍

normen commented 4 years ago

I got your comment about HomeKit setting 100% brightness via eMail -- don't we have the "currentState"? It could only possibly get out of sync when HomeBridge just rebooted right? Maybe we can just send "our" value to HK? But I guess theres a reason you deleted the comment, maybe you found another way.

normen commented 4 years ago

Come to think of it - we should be able to solve this all in the applyDesignatedState method, it should have all the needed info. I know it's a bit convoluted but from watching all the other plugins and the various workarounds that were introduced over time I think that it is easier to have one complicated function in one place that handle all the logic rather than some "simple" flags and callbacks all over the code.

Zer0x00 commented 4 years ago

Yes, I've deleted my comment since I found an other way: You are able to push your value to HK like you stated.

It's already implemented in my fork. Implementation of function: https://github.com/Zer0x00/homebridge-milighthub-platform/commit/d32fd3274d0b1ae111ce2b311a04a25c2f8078ca

Pushing correct value to HK: https://github.com/Zer0x00/homebridge-milighthub-platform/commit/c5a2829c7b9ffb9712a19404c233bd4468aa521d

normen commented 4 years ago

I was looking at the whole code around getting the model characteristic of the light.. Maybe I didn't understand the actual purpose but I think it can be replaced by two things:

1) The accessory object coming from homebridge has a context field that stores any JSON-compatible information we put in it. So you can just go milight.accessory.context.myVar = "Hello". Maybe thats a better way to store this info - which I suppose is just "was this lamp stored with rgbcct mode or not"? 2) To provide backwards compatibility and to get to that "Model" information in general, in case I missed the actual purpose - You can just go accessory.getService(Service.AccessoryInformation).getCharacteristic(Characteristic.Model) to get that - instead of using that array and the model String.

I can implement the changes no problem, just wanted to check back if there was another purpose to this.

Zer0x00 commented 4 years ago

Thanks for this, there was no other purpose. Please feel free to implement the changes, sadly I do not have any time right now.