#2388 Implement color sync #2441
Conversation
sjorge
force-pushed
the
color_sync
branch
2 times, most recently
from
a0413ca
to
25881c5
Compare
sjorge
force-pushed
the
color_sync
branch
2 times, most recently
from
03d7ae8
to
2f531c4
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
sjorge
force-pushed
the
color_sync
branch
5 times, most recently
from
9644d9a
to
7786695
Compare
This comment has been minimized.
This comment has been minimized.
|
This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days |
This comment has been minimized.
This comment has been minimized.
sjorge
force-pushed
the
color_sync
branch
7 times, most recently
from
035255a
to
3863ef2
Compare
This comment has been minimized.
This comment has been minimized.
|
@sjorge for xy -> mireds no lookup is used but a function: zigbee-herdsman-converters/lib/color.js Line 217 in f67c255 |
| * From: https://github.com/usolved/cie-rgb-converter/blob/master/cie_rgb_converter.js | ||
| * @return {ColorRGB} | ||
| */ | ||
| toRGB() { |
There was a problem hiding this comment.
@blazewicz can you have another look at this, going from XY -> RGB I'm not getting the expected red/green/blues, they all seem off a bit and have negative values too which definitely doesn't seem right.
Also why the tests are failing.
There was a problem hiding this comment.
I responded in resolved thread, sorry.
https://github.com/Koenkk/zigbee-herdsman-converters/pull/2441/files#r632667617
|
@Koenkk excellent! Even hue support reading all attributes at ones, I can't find my osram bulb though :( Osram also OK: |
|
Aside from the currently unresolved issues with ColorXY.toRGB() it seems to be working for a groups too, assuming that bulbs/groups have the same color_sync value of true.
|
…past, this breaks might brake some automations
Got introduce when introducing lib/color
| recalledState = addColorMode(recalledState); | ||
| } | ||
|
|
||
| // XXX: meta.state is the groups state, we are leaking group state TO devices! |
There was a problem hiding this comment.
@Koenkk with this it works, but see the XXX comment.
There has to be a clean way of doing it. This is the last remaining issue together with the toRGB() issue
There was a problem hiding this comment.
@sjorge wasn't this the case already before? I don't understand how to color_sync causes this.
This comment has been minimized.
This comment has been minimized.
|
@blazewicz Ah nice, yeah I did notice the numbers were super close without the reverse correction, so I guess it was indeed not needed like I first though.! @Koenkk just one small issue remaining now: |
|
You might be right, with reporting this was already the case too.
I guess it’s good to go then?
~ sjorge
… On 15 May 2021, at 15:24, Koen Kanters ***@***.***> wrote:
@Koenkk commented on this pull request.
In converters/toZigbee.js:
> @@ -4075,6 +4036,20 @@ const converters = {
for (const member of entity.members) {
if (member.meta.hasOwnProperty('scenes') && member.meta.scenes.hasOwnProperty(metaKey)) {
membersState[member.getDevice().ieeeAddr] = addColorMode(member.meta.scenes[metaKey].state);
+
+ let recalledState = member.meta.scenes[metaKey].state;
+
+ // add color_mode if saved state does not contain it
+ if (!recalledState.hasOwnProperty('color_mode')) {
+ recalledState = addColorMode(recalledState);
+ }
+
+ // XXX: meta.state is the groups state, we are leaking group state TO devices!
@sjorge wasn't this the case already before? I don't understand how to color_sync causes this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
@sjorge yes, found this with the z2m tests, ready to go! Big thanks as usual 🎖️ |
lib/color.js#2453scene_addscene_recall?