search

jangxx / node-magichome

An incomplete implementation of the functionality of the "Magic Home" app. Partially a port of https://github.com/Danielhiversen/flux_led to Node.js
ISC License
124 stars 26 forks source link

Clamp cold white values #30

Closed yerich closed 3 years ago

yerich commented 3 years ago

Fixes some issues that I encountered with some of my bulbs, debugged via network packet capturing. Turns out that setColorAndWarmWhite and setColorAndWhites weren't applying the mask option. Additionally, the cold white value should be clamped to [0, 255] like the other color values.

jangxx commented 3 years ago

The cold white clamp was an oversight, so thank you for fixing it.

I'm not sure I understand the rest of your fixes though. Why would we want to apply the masks in setColorAndWarmWhite and setColorAndWhites at all? The masks are only used in setColor and the respective "whites" versions, since they're required to be used for controllers which don't support setting both values at the same time (to tell the controller which parts of the color bytes to use). If you have a controller like that, you would never even call a function like setColorAndWarmWhite, since your controller does not have the functionality to set both.

The way the masks are applied in your code basically turn setColorAndWarmWhite into setWarmWhite and setColorAndWhites into setWhites if the mask parameter is set and I really don't see why we should do it that way. Principle of least surprise and all that. It might be a good idea to output an error or reject the promise if someone tries to call setColorAndWarmWhite, while having the mask parameter set, though, since doing that is an obvious mistake on the part of the user that we might want to warn them about.

yerich commented 3 years ago

Ah, my mistake, it appears based on your comment that I was indeed calling the wrong function, or using it in a different way than intended. For instance, I was calling setColorAndWhites with the first three values set to 0 instead of setWhites. I'll correct my code and my branch to call the setWhites instead. Thanks!

Though, I do agree it would probably be helpful to include some sort of warning or something that calling setColorAndWhites(0, 0, 0, 255, 0), for instance, will not work on bulbs that don't support colors and whites at the same time, as that's what I believe tripped me up.

jangxx commented 3 years ago

Okay, I have merged your PR and published version 2.6.1 containing the fixes. I have also added additional console.warn warnings about the wrong use of setColorAndWhites and setColorAndWarmWhite, which should hopefully clear up any confusion in the future.