espressif / esp-matter

Espressif's SDK for Matter
Apache License 2.0
694 stars 157 forks source link

Color light example and HLS (CON-466) #365

Closed jonsmirl closed 5 months ago

jonsmirl commented 1 year ago

The color light example is only implementing HLS support. The HLS support is fine, but it is optional. Per spec, ColorXY support is mandatory so the light has to also implement it.

jadhavrohit924 commented 1 year ago

Are you talking about color control cluster here?

jonsmirl commented 1 year ago

Yes. Look in the Device Type Spec, page 31 image

jadhavrohit924 commented 1 year ago

If you see extended colour light create api we are creating colour control cluster with color temperature and xy features

jonsmirl commented 1 year ago

The driver is not processing the ColorXY attributes.

https://github.com/espressif/esp-matter/blob/main/examples/light/main/app_driver.cpp

jonsmirl commented 1 year ago

It's not processing color temp attributes either. If you set color temp, the light should go white and then adjust to the color temp setting. There is nothing wrong with the sample code. It just isn't implementing mandatory items from the spec.

stage-01 commented 1 year ago

I have started having problems getting color light example to work on google home and apple homekit. In Home Assistant I can't get the color control on the light example to work either, but when I implement it myself by using the x and y value to change the color of the light it does work. Sadly this still doesn't work in google home and homekit.

Could this be because of the faulty implementation of ColorXY in the color light example?

jonsmirl commented 1 year ago

The esp-matter color light example is missing the mandatory implementation of ColorXY support. Missing this causes it to break in various random ways when used by Google/Apple/Amazon/HA

The color light example also needs to expose all four device types: 0x100, 0x101, 0x10c and0x10d. That is needed so that OnOff or level control switches can bind to it without controlling the color.

stage-01 commented 1 year ago

Do you know if there is any way to get the color working again through google home and homekit? I've changed the lighting example to work with ColorXY and that made the color work in HA. Still not working in the other platforms at the moment.

Is there a way to expose all device types without creating four different endpoints?

jonsmirl commented 1 year ago

You put all four device types on a single endpoint.

    epLight = endpoint::create(node, ENDPOINT_FLAG_NONE, this);
    descriptor::create(epLight, CLUSTER_FLAG_SERVER);
    endpoint::add_device_type(epLight, extended_color_light::get_device_type_id(), extended_color_light::get_device_type_version());
    endpoint::add_device_type(epLight, color_temperature_light::get_device_type_id(), color_temperature_light::get_device_type_version());
    endpoint::add_device_type(epLight, dimmable_light::get_device_type_id(), dimmable_light::get_device_type_version());
    endpoint::add_device_type(epLight, on_off_light::get_device_type_id(), on_off_light::get_device_type_version());
stage-01 commented 1 year ago

I've put all four device types on a single endpoint and I even changed the color light example to use the ColorXY values instead of the Hue and Saturation that esp uses but I still can't get any light to show up as a color light in Google Home and Homekit. It just shows color temperature setting.

As mentioned before, in CHIP tool and Home Assistant I can change the color of the light. Which means that the clusters are working but just not recognized by GH and Homekit.

I've also tried using the ESP launchpad light example on the C3 but that gave the same outcome. Is there anything I could be missing?

jonsmirl commented 1 year ago

Google has an acknowledged bug which hasn't been fixed yet. To make it work on Google remove the color temperature device type. After they fix their bug you can put it back.

stage-01 commented 1 year ago

When using the light example you dont use the color temperature device right? How would you remove the color temperature cluster?

jonsmirl commented 1 year ago

The Color Temp support is mandatory per the spec. Google just has a bug where if both are present they prioritized the color temp over the color support.

Goolge is supposed to build something like Philips Hue has at 2:08 in this video. https://www.youtube.com/watch?v=Reru0ceke0s&ab_channel=Justin_tech

On Wed, May 24, 2023 at 7:24 AM stage-01 @.***> wrote:

When using the light example you dont use the color temperature device right? How would you remove the color temperature cluster?

— Reply to this email directly, view it on GitHub https://github.com/espressif/esp-matter/issues/365#issuecomment-1560942067, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAADUX5UPUHIK372WMHH3WTXHXVYFANCNFSM6AAAAAAXTQTNKA . You are receiving this because you authored the thread.Message ID: @.***>

-- Jon Smirl @.***

stage-01 commented 1 year ago

Thanks for the help, I think Home Assistant does it in a similar way to Philips Hue as they always give te option for both color and temperature. Sadly the color temperature changing doesn't work on HA right now, it calls the color temperature cluster but it always gives 0 as a value.

For GH and Homekit I'll just have to wait for them to fix this problem.

jonsmirl commented 1 year ago

On Google if you remove CT you can see the color UI. And also if you remove Color and keep CT you can see the CT UI. They just missed the case where they are both present which is actually mandated by the spec.

stage-01 commented 1 year ago

The only way I could think of to remove the CT cluster was by edditing the endpoint.cpp and cluster.cpp from the esp-matter sdk. Seeing as the CT cluster is automatically added when creating the extended_color_light. For some reason however the light now only comes up as an On/Off light in both GH and in Homekit. In HomeAssistant the CT is not available anymore but the color is! Meaning there is still a color changing cluster on the device but apple and google aren't recognizing it.

jonsmirl commented 1 year ago

Just need to remove the device type from the endpoint. You can leave everything else.

stage-01 commented 1 year ago

How do you remove an endpoint? Removing: color_control::feature::color_temperature::get_id() | on line 221 in endpoint.cpp removes the color control from the extended color light endpoint right? This makes the color temperature go away in GH and Homekit but it still doesn't give me any color options.

jonsmirl commented 1 year ago

Here is how I make the device types for my light endpoint:

    epLight = endpoint::create(node, ENDPOINT_FLAG_NONE, this);
    descriptor::create(epLight, CLUSTER_FLAG_SERVER);
    endpoint::add_device_type(epLight, extended_color_light::get_device_type_id(), extended_color_light::get_device_type_version());
    endpoint::add_device_type(epLight, color_temperature_light::get_device_type_id(), color_temperature_light::get_device_type_version());
    endpoint::add_device_type(epLight, dimmable_light::get_device_type_id(), dimmable_light::get_device_type_version());
    endpoint::add_device_type(epLight, on_off_light::get_device_type_id(), on_off_light::get_device_type_version());

Per the spec this is legal. Google has a bug right now that it will only this works for color

    epLight = endpoint::create(node, ENDPOINT_FLAG_NONE, this);
    descriptor::create(epLight, CLUSTER_FLAG_SERVER);
    endpoint::add_device_type(epLight, extended_color_light::get_device_type_id(), extended_color_light::get_device_type_version());
    endpoint::add_device_type(epLight, dimmable_light::get_device_type_id(), dimmable_light::get_device_type_version());
    endpoint::add_device_type(epLight, on_off_light::get_device_type_id(), on_off_light::get_device_type_version());

and this for temperature

    epLight = endpoint::create(node, ENDPOINT_FLAG_NONE, this);
    descriptor::create(epLight, CLUSTER_FLAG_SERVER);
    endpoint::add_device_type(epLight, color_temperature_light::get_device_type_id(), color_temperature_light::get_device_type_version());
    endpoint::add_device_type(epLight, dimmable_light::get_device_type_id(), dimmable_light::get_device_type_version());
    endpoint::add_device_type(epLight, on_off_light::get_device_type_id(), on_off_light::get_device_type_version());

That is not what the spec says. There is issue filed with Google on this, and Google has acknowledged they are broken. They just have not fixed it yet.

This also works on Google

    epLight = endpoint::create(node, ENDPOINT_FLAG_NONE, this);
    descriptor::create(epLight, CLUSTER_FLAG_SERVER);
    endpoint::add_device_type(epLight, color_temperature_light::get_device_type_id(), color_temperature_light::get_device_type_version());
    epLight = endpoint::create(node, ENDPOINT_FLAG_NONE, this);
    descriptor::create(epLight, CLUSTER_FLAG_SERVER);
    endpoint::add_device_type(epLight, extended_color_light::get_device_type_id(), extended_color_light::get_device_type_version());

But if you do that On/off and Level control switches can't bind to the light bulb.

stage-01 commented 1 year ago

Thank you very much, I'll try this. Let's hope google will fix this quickly.

stage-01 commented 1 year ago

I've had a short break on the matter light development but I have tried to implement the method you showed. I can't really figure out where your adding this code. Pasting your example right after creating a node in the main I get an error. How did you implement the matter code? Did you use any of the example programs as a start?

Gonta01 commented 1 year ago

I've changed the lighting example to work with ColorXY and that made the color work in HA. Still not working in the other platforms at the moment.

How did you do a conversion from XY to RGB?

stage-01 commented 1 year ago

I first changed the color mode set when adding the endpoint to 1. Then in the driver attribute update callback I safe the changed X and Y value to a variable. After the CurrentY value is changed I call a function I created to convert xyY to RGB, in there I set the Y value to 1.0 because the brightness is controlled in a different way. I'm not sure if this is the best way to do it but it works quite well and the colors of the light seem right. Going back the other way from rgb to xyY is not really working out well for me though.

jonsmirl commented 1 year ago

Note, RGB attribute support is optional and ColorXY is mandatory. There is no need to implement RGB attribute support in the bulb since everyone HAS to support ColorXY. I only use RGB internally and don't expose it.

// Illumination base class
static void xyzToRGB(uint16_t cx, uint16_t cy, uint8_t cz, uint8_t *pr, uint8_t *pg, uint8_t *pb)
{
    float x = cx / MATTER_COLOR_MAX;
    float y = cy / MATTER_COLOR_MAX;
    float z = cz / MATTER_BRIGHTNESS_MAX;

    float r = x * 3.2404542 + y * -1.5371385 + z * -0.4985314;
    float g = x * -0.9692660 + y * 1.8760108 + z * 0.0415560;
    float b = x * 0.0556434 + y * -0.2040259 + z * 1.0572252;

    r = ((r > 0.0031308) ? (1.055 * pow(r, 1 / 2.4) - 0.055) : (12.92 * r)) * 255.0;
    g = ((g > 0.0031308) ? (1.055 * pow(g, 1 / 2.4) - 0.055) : (12.92 * g)) * 255.0;
    b = ((b > 0.0031308) ? (1.055 * pow(b, 1 / 2.4) - 0.055) : (12.92 * b)) * 255.0;

    *pr = r;
    *pg = g;
    *pb = b;
}
donfras commented 9 months ago

I had similar issues with the ESP matter SDK light example which I think I have resolved now. I have some ESP32-H2 devkits and an ESP32-S2 / ESP32-H2 Thread Border Router. I'm using a Thread transport and have these devices already working well on a Thread only network (no Matter involved, just sending udp messages between devices) with the same Thread Border Router. I'm now trying to learn and add Matter. I'm using very recent SDKs: the esp-idf main branch and esp-matter main branch from a couple of weeks ago . My development PC is Windows 10, I'm building the Matter examples in a WSL2 Ubuntu environment which is working well. I'm trying to control the onboard addressable RGB LED on my ESP32-H2 end device from various Matter controllers. The controllers I have tried and the results I saw with the 'out of box' ESP Matter SDK light example are as follows

Amazon Alexa Android app on my Pixel 7 phone The Alexa app commissions my ESP32-H2 end device but sees it as a color temperature device and only offers on-off, level and color temperature control. This sounds similar to what you have seen with Google Home.

Home Assistant (HA) Android app on my Pixel 7 phone The HA app commissions my ESP32-H2 end device and offers on-off, level, color temperature control as enhanced color control. This app doesn't seem to care about the actual device capabilities, it always offers all the light controls. When I set the color in the app, for example to Red, the app sends an XY command 7 to cluster 0x300. This results in the CurrentX and CurrentY attributes (attributes 3 and 4) being updated to new values. I updated the app-driver code with JonSmirl's xyY to RGB code from this ticket, and changed the ESP Matter esp-matter\device_hal\led_driver\include\led_driver.h code to expose a led_driver_set_rgb function in addition to the set_hue, set_saturation etc functions already there. This kind of worked but the colors are all wrong for me. I've looked at other similar xyy to rgb conversions out there on the web, however have now found another solution and can abandon the xyY conversion stuff ( see further down).

Google Home Android app on my Pixel 7 phone I have been unable to commission my ESP32-H2 end device via Google home. I get a 'Not a Matter Certified device' error in the app. Looks like a common issue with Google Home, for example Not a Matter-certified device · Issue #93 · google-home/sample-apps-for-matter-android · GitHub I've not had time to investigate this.

Chip tool, running on a Raspberry Pi 4 with 64 bit Ubuntu server I have been unable to commission my ESP32-H2 end device. I provide the active dataset information from my Thread network on the command line and the bluetooth commissioning steps all work, but the last commissioning step about joining the network always fails. I have logs for this but have not got very far debugging this.

Solution with Alexa and Home Assistant apps: I noted that the ESP Matter SDK implementation of the color_temperature_light::create function called from app_main in the light example, ends up calling

    color_control::create(endpoint, &(config->color_control), CLUSTER_FLAG_SERVER,
                           color_control::feature::color_temperature::get_id() | color_control::feature::xy::get_id());

You can see this code in esp-matter\components\esp_matter\esp_matter_endpoint.cpp - search for 'namespace extended_color_light {' to find the ::create and ::add functions.

The add function is hard coded to support the color_temperature and xy color control features. Thinking that the Alexa app probably had the same issue as you have seen with Google Home (prioritizing color temperature, if both color temperature and xy are available), I commented out the '| color_control::feature::color_temperature::get_id()' term. This did not help, in the Alexa app I simply lost the color temperature control, so was left with on/off and level control. Alexa did not allow extended color control or color temperature control. Then I thought perhaps Alexa only works with Hue and Saturation, so I added that:

    color_control::create(endpoint, &(config->color_control), CLUSTER_FLAG_SERVER,
                          color_control::feature::color_temperature::get_id() | color_control::feature::xy::get_id() | color_control::feature::hue_saturation::get_id());     

This fixed it in the Alexa app, Alexa now recognises my end device as supporting extended color control. Better still it sends the MoveToHueAndSaturation command 0x6 to my device instead of the MoveToColour command, resulting in the CurrentHue and CurrentSaturation attributes (0x000 and 0x001 of cluster 0x300) being updated, not CurrentX and CurrentY. This means that the xyY to RGB conversion code and the led_driver_set_rgb addition to the led_driver are not needed - the light example just works.

I then deleted the device in Alexa and commissioned again in Home Assistant. Now Home Assistant also starts sending MoveToHueAndSaturation commands instead of MoveToColour, so CurrentHue and CurrentSaturation attributes are updated. Again the addtional xyY to RGB conversion and led_driver_set_rgb code are not needed, and the onboard led colors are changed correctly to match the colors in the HA app.

It appears to me that the Alexa app only supports setting colors via by Hue and Saturation, it does not support sending MoveToColor (XY). HomeAssistant supports both commands but will use Hue and Saturation in preference to XY where both are supported by the device. I don't really want to change the ESP Matter SDK esp_matter_endpoint.cpp code, so will probably copy that code from the create and add methods into a new function in the light example app_main.cpp.

jonsmirl commented 9 months ago

The previous code used the xyz colorspace instead of xy. You probably need xy.

// Illumination base class
static void xyToRGB(uint16_t cx, uint16_t cy, uint8_t *pr, uint8_t *pg, uint8_t *pb)
{
    float x = cx / MATTER_COLOR_MAX;
    float y = cy / MATTER_COLOR_MAX;

    float X = x * (80. / y);
    float Y = 80.;
    float Z = (1 - x - y) * (80. / y);

    float r = X * 3.2404542 + Y * -1.5371385 + Z * -0.4985314;
    float g = X * -0.9692660 + Y * 1.8760108 + Z * 0.0415560;
    float b = X * 0.0556434 + Y * -0.2040259 + Z * 1.0572252;

    float min = std::min(std::min(r, g), b);
    if (min < 0)
    {
        r -= min;
        g -= min;
        b -= min;
    }

    r = ((r > 0.0031308) ? (1.055 * pow(r, 1 / 2.4) - 0.055) : (12.92 * r));
    g = ((g > 0.0031308) ? (1.055 * pow(g, 1 / 2.4) - 0.055) : (12.92 * g));
    b = ((b > 0.0031308) ? (1.055 * pow(b, 1 / 2.4) - 0.055) : (12.92 * b));

    float max = std::max(std::max(r, g), b);

    r = (r / max) * 255.;
    g = (g / max) * 255.;
    b = (b / max) * 255.;

    *pr = r;
    *pg = g;
    *pb = b;
}
jonsmirl commented 9 months ago

How to make device types and features from your code.


    extended_color_light::config_t ext_config;
    epLight = endpoint::create(node, ENDPOINT_FLAG_NONE, this);
    descriptor::create(epLight, &ext_config.descriptor, CLUSTER_FLAG_SERVER);
    endpoint::add_device_type(epLight, extended_color_light::get_device_type_id(), extended_color_light::get_device_type_version());
    endpoint::add_device_type(epLight, color_temperature_light::get_device_type_id(), color_temperature_light::get_device_type_version());
    endpoint::add_device_type(epLight, dimmable_light::get_device_type_id(), dimmable_light::get_device_type_version());
    endpoint::add_device_type(epLight, on_off_light::get_device_type_id(), on_off_light::get_device_type_version());

    on_off::create(epLight, &ext_config.on_off, CLUSTER_FLAG_SERVER | CLUSTER_FLAG_CLIENT, on_off::feature::lighting::get_id());
    level_control::create(epLight, &ext_config.level_control, CLUSTER_FLAG_SERVER | CLUSTER_FLAG_CLIENT,
                          level_control::feature::on_off::get_id() | level_control::feature::lighting::get_id());
    color_control::create(epLight, &ext_config.color_control, CLUSTER_FLAG_SERVER | CLUSTER_FLAG_CLIENT,
                          color_control::feature::color_temperature::get_id() | color_control::feature::xy::get_id() | color_control::feature::hue_saturation::get_id());
donfras commented 9 months ago

Thanks jonsmirl! As I tried to say in my comment I did get it fully working for the Alexa and Home Assistant apps just by changing the ::add method in the ESP Matter SDK file esp-matter\components\esp_matter\esp_matter_endpoint.cpp, which is exactly what you have in your last line above. I will move that into the example code as you have done above though, rather than changing the SDK esp-matter\components\esp_matter\esp_matter_endpoint.cpp. I'm not sure why that line in the SDK doesn't have that last ' | color_control::feature::hue_saturation::get_id()' term however. The example would work out the box for more Matter apps if that was the case. I don't need the xyToRGB conversion code at all now (but thanks for taking the time to provide), as with this change the Alexa and HomeAssistant apps now send the MoveToHueAndSaturation command instead of the MovetoColor (xy) command, so the driver in the light example doesn't really need to handle CurrentX and CurrentY changes at all as it never receives them (from Alexa or Home Assistant at least). I'm all good, very happy this is working for me now.

    color_control::create(epLight, &ext_config.color_control, CLUSTER_FLAG_SERVER | CLUSTER_FLAG_CLIENT,
                          color_control::feature::color_temperature::get_id() | color_control::feature::xy::get_id() | color_control::feature::hue_saturation::get_id()
jonsmirl commented 9 months ago

XY is the only mandatory color control scheme. It is probably what Apple is using, but I haven't checked.

jonsmirl commented 5 months ago

This appears to be working correctly in Google/Apple/Amazon apps now.