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

Commit: Add demonstration of auto subscription from switch to light. (CON-1515) #1246

Open
jonsmirl opened this issue Jan 16, 2025 · 3 comments

Comments

@jonsmirl
Copy link
Contributor

jonsmirl commented Jan 16, 2025

f89603b

What is this commit doing? It looks like it is using a controller to subscribe to the light events to keep an indicator in the switch in sync. That's not how this is supposed to work. The way you keep the indicator in sync is to make a group containing the light bulb and the indicator, then the switch sends commands to the group. Doing this with subscriptions gets really complicated when there are multiple switches and lights involved.

The design of the Matter switch device is messed up and I am unable to talk to people about fixing it. If you look at my code for a switch, I am forced into making three endpoints for each switch:

    for (uint16_t i = 1; i < KEYPAD_NUM_BUTTONS; i++) {
        event = {.type = eEnumeratedEP, .enumerated = {.device_type = LOWPAN_DT_INDICATOR, .position = i}};
        ESP_LOGI(TAG, "Enumerating Keypad Switch %dGS", i);
        SendEvent(&event);
        event = {.type = eEnumeratedEP, .enumerated = {.device_type = MATTER_DT_COLOR_SWITCH, .position = i}};
        ESP_LOGI(TAG, "Enumerating Keypad Switch %dS", i);
        SendEvent(&event);
        event = {.type = eEnumeratedEP, .enumerated = {.device_type = MATTER_DT_GENERIC_SWITCH, .position = i}};
        ESP_LOGI(TAG, "Enumerating Keypad Switch %dGS", i);
        SendEvent(&event);
    }

The switch, the generic switch and then the indicator LED. This should not be three endpoints, it should be just one but I am blocked from providing feedback on the spec. My LOWPAN_DT_INDICATOR is an Extended Color Light which is attached to an LED, but I have to change the device type to keep Google/Apple/Amazon from adding it to their UI.

A correct switch endpoint would combine all of the clusters from MATTER_DT_COLOR_SWITCH, MATTER_DT_GENERIC_SWITCH, MATTER_DT_EXTENDED_COLOR onto a single, new device type. This new switch endpoint combines the three functions: it works like a matter switch, you can double tap it to send events, and it has an integrated indicator.

Then to make every thing stay in sync you add both these new switches and the light bulbs to a group. Now when any switch sends an ON command ALL of the bulbs and the indicators will see it.

@github-actions github-actions bot changed the title Commit: Add demonstration of auto subscription from switch to light. Commit: Add demonstration of auto subscription from switch to light. (CON-1515) Jan 16, 2025
@jadhavrohit924
Copy link
Contributor

What is this commit doing? It looks like it is using a controller to subscribe to the light events to keep an indicator in the switch in sync. That's not how this is supposed to work. The way you keep the indicator in sync is to make a group containing the light bulb and the indicator, then the switch sends commands to the group. Doing this with subscriptions gets really complicated when there are multiple switches and lights involved.

This commit demonstrates the subscription for one light and one switch to keep the indicator on the switch in sync. Having a group of light bulbs is a different use case which is not demonstrated in this example.

We are looking into the second part of your issue.

@jonsmirl
Copy link
Contributor Author

jonsmirl commented Jan 17, 2025

Even for a single light and switch this should use a group with two members -- the light bulb and the indicator. There should not be different mechanisms depending on the number of lights and switches involved.

A key thing to observe: the indicator in the switch is implemented as an Extended Color Light but it can't have the Extended Color Light device type (it should be part of the switch endpoint but because I can't change the spec I made LOWPAN_DT_INDICATOR). Since the indicator uses exactly the same clusters as the actual light bulb, when you send group commands it will exactly track the light bulb's state. And this works for as many switches and light bulbs as you want to group together.

This setup is why I was quite familiar with the issues in #1236

Here's my diagram from three years ago:
Image

@jonsmirl
Copy link
Contributor Author

jonsmirl commented Jan 17, 2025

You will be tempted to make a Generic Switch endpoint and then add the Color Switch and the Extended Light clusters to it. Theoretically that should work, but it won't due to this issue: project-chip/connectedhomeip#36408

What happens is Apple Home ignores the Generic Switch device type and does cluster inspection, when it does cluster inspection it finds the Extended Light clusters and then declares the device to be a light bulb even though the device type said it wasn't. In my opinion Apple Home is broken and doing cluster inspection should be banned. The function of an endpoint should be determined by it's device type, Apple should not be second guessing that.

Right now the only way to make this work is to put the indicator's Extended Light clusters onto a vendor specific endpoint -- ie LOWPAN_DT_INDICATOR. This is only because the spec needs to be fixed to support indicator LEDs on switches, once that is done there is no need for the vendor specific endpoint. Apple will also have to remove its ill-advised cluster inspection code.

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

No branches or pull requests

2 participants