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

Allow /get for color_mode #2283

Merged
merged 7 commits into from
Mar 17, 2021
Merged

Allow /get for color_mode #2283

merged 7 commits into from
Mar 17, 2021

Conversation

sjorge
Copy link
Contributor

@sjorge sjorge commented Feb 23, 2021

As mentioned in the discussion linked in Koenkk/zigbee2mqtt#6402

Here is a local diff I've been carrying for a while, it allows manually getting color_mode and actually parses the random numbers into something more descriptive.

I use this to manually drop either color_temp, color.x + color.y, or color.hue + color.saturation from the json payload before processing, so I also have the correct color info.

@Koenkk
Copy link
Owner

Koenkk commented Feb 24, 2021

I want to wait to merge this once there is more clarity in: home-assistant/architecture#519

Besides changes in the pr also all converters have to be updated to return the correct color_mode (e.g.

if (key == 'color') {
)

@sjorge
Copy link
Contributor Author

sjorge commented Feb 24, 2021

Yeah, this is just what I had so far as a set of patches I carry (when not testing stuff).

I don't like the proposal there of adding 'new' color modes not defined in the ZCL though, at least not as an attribute called color_mode, then we get into the 'system_mode' mess where HA and ZCL don't agree on what should be in it. HA is a consume of all of this, but it's not the only one.

Probably a few things that need to happen in addition to this:

  • update color and color_temp converters to also switch color_mode
  • (optionally) filter out the non active mode values from the color payload.

I can append this to this PR, or we could do them in follow up PR's

@sjorge
Copy link
Contributor Author

sjorge commented Mar 16, 2021

Seems home-assistant/core#47720 got merged, sadly it does not really map to ZCL's color_mode cleanly (the on/off and dimmer stuff)

mapping ZCL color_mode updates to the following should work pretty well though and it probably a good start.

COLOR_MODE_COLOR_TEMP = "color_temp"
COLOR_MODE_HS = "hs"
COLOR_MODE_XY = "xy"

I'll rebase the PR later tonight and rename ct -> color_temp. I will also set color_mode when /set is called. The filtering of the inactive keys for color and color_temp I'll do in a follow up PR. Not sure how we can do this for now as it looks like just returned color with the active attributes have them merged with the old values too 🤔

@sjorge sjorge force-pushed the gcm branch 4 times, most recently from 02e7269 to 895ee9f Compare March 16, 2021 15:50
Attribute Value | Attributes that Determine the Color |
| --- | --- |
| 0x00 | CurrentHue and CurrentSaturation |
| 0x01 | CurrentX and CurrentY            |
| 0x02 | ColorTemperatureMireds           |
@sjorge
Copy link
Contributor Author

sjorge commented Mar 16, 2021

@Koenkk that should cover setting color_mode correctly I think.

Edit: tested with a Innr E14, Tint GU10, and Ikea FLOALT

@sjorge
Copy link
Contributor Author

sjorge commented Mar 16, 2021

Found an issue with scene_recall not updating color_mode, retesting with extra change.

@sjorge
Copy link
Contributor Author

sjorge commented Mar 16, 2021

Thats... annoying

Zigbee2MQTT:debug 2021-03-16 17:25:24: Received Zigbee message from 'office/light/desk/bulb', type 'readResponse', cluster 'lightingColorCtrl', data '{"colorMode":2}' from endpoint 1 with groupID 0
Zigbee2MQTT:debug 2021-03-16 17:25:46: Received Zigbee message from 'office/light/desk/bulb', type 'attributeReport', cluster 'lightingColorCtrl', data '{"colorMode":0}' from endpoint 1 with groupID 0

I have to wait ~1-2 sec before reading colorMode, I guess we'll need to just set it based on what is in state instead...

@sjorge
Copy link
Contributor Author

sjorge commented Mar 16, 2021

OK, 2nd way give the expected behavior, would still have liked to just read it. But I couldn't find a way to read it after X timeout. Oh well.

@Koenkk can you double check this? That should at least got the color_mode attribute added with hs/xy/color_temp depending on the last set color mode.

@Koenkk Koenkk merged commit 73ecaa3 into Koenkk:master Mar 17, 2021
@sjorge sjorge deleted the gcm branch March 17, 2021 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants