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

Don't use endpoint-prefixed state/brightness for Busch-Jaeger devices #5097

Closed
wants to merge 1 commit into from

Conversation

klada
Copy link
Contributor

@klada klada commented Dec 6, 2022

The Busch-Jaeger devices may have multiple endpoints, depending on the rocker type which is installed. The following endpoints may be present:

  • 10, 11, 12, 13: These are the endpoints for the rockers which report events. They do not report any state/brightness (genOnOff/genLevelCtrl only in output-cluster).

  • 18: Only present if the device is controlling a relay or a dimmer. This is the only endpoint which reports a state/brightness (genOnOff/ genLevelCtrl only in input-cluster).

The meta.multiEndpoint setting is required for prefixing the events reported by endpoints 10 to 13 with the endpoint name (row number in this case). However this device only has a single state (reported by endpoint 18) as it only controls up to one relay/dimmer.

This is why a prefixed state attribute such as state_relay does not make sense here and needlessly breaks both Home Assistant discovery (state of discovered lights never update) and the web frontend, which both only care about the state property.

Since these devices only have (up to) one state we can remove the endpoint prefix for both state and brightness.

⚠️ @Koenkk This has a few implications:

  • ⚠️ state_relay is not being updated any more and cannot be set any more. This is potentially a breaking change, depending on which attribute people were using in the past. I guess this can only be included in a new major Z2M release.
  • ❓ We are effectively removing the state_relay attribute with this PR, but it will rot forever in the state.json file of older installations. Is there anything we can do about that?
  • ✔️ Fixes State not updating (Busch Jäger 6711+6735) zigbee2mqtt#7077
  • ✔️ Home Assistant auto-discovery will finally be working for these devices

The Busch-Jaeger devices may have multiple endpoints, depending on the
rocker type which is installed. The following endpoints may be present:

- 10, 11, 12, 13: These are the endpoints for the rockers which report
  events. They do not report any state/brightness (genOnOff/genLevelCtrl
  only in output-cluster).

- 18: Only present if the device is controlling a relay or a dimmer.
  This is the only endpoint which reports a state/brightness (genOnOff/
  genLevelCtrl only in input-cluster).

The `meta.multiEndpoint` setting is required for prefixing the events
reported by endpoints 10 to 13 with the endpoint name (row number in
this case). However this device only has a single state (reported by
endpoint 18) as it only controls up to one relay/dimmer.

This is why a prefixed state attribute such as `state_relay` does not
make sense here and needlessly breaks both Home Assistant discovery
(state of discovered lights never update) and the web frontend, which
both only care about the `state` property.

Since these devices only have (up to) one state we can safely remove the
endpoint prefix for both `state` and `brightness`.
@Koenkk
Copy link
Owner

Koenkk commented Dec 6, 2022

This is indeed a breaking change, however I don't plan to release any new major Z2M version soon. So wondering if we shouldn't just keep this as is.

@klada
Copy link
Contributor Author

klada commented Dec 6, 2022

The question is if you would be okay with a breaking change for 1.29.x?

If you'd rather not break things at all we could also try to fix this in a non-breaking way. I guess this can be done by using a device-specific converter which processes both state and state_relay for compatibility reasons like this:

if (msg.data.hasOwnProperty('onOff')) {
    return {
        state: msg.data['onOff'] === 1 ? 'ON' : 'OFF',
        state_relay: msg.data['onOff'] === 1 ? 'ON' : 'OFF'
    };
}

This would keep setups working where people set state_relay instead of state. Do you want me to send a new PR for this so we can compare the two approaches?

@Koenkk
Copy link
Owner

Koenkk commented Dec 7, 2022

This is why a prefixed state attribute such as state_relay does not make sense here and needlessly breaks both Home Assistant discovery (state of discovered lights never update) and the web frontend, which both only care about the state property.

This seems to be the root cause which needs to be fixed instead. state_relay or state is a cosmetic issue. There are more devices using state prefixes (e.g. state_left, state_right) so I'm surprised this is not working. Can you provide more details about this?

@klada
Copy link
Contributor Author

klada commented Dec 7, 2022

@Koenkk The problem is described in Koenkk/zigbee2mqtt#7077. Short summary of the behavior/problem without my patch:

Let's assume the switch's MQTT info initially looks like this:

{"brightness_relay":254, "linkquality":153,"state":"OFF","state_relay":"OFF"}

Home Assistant discovery adds this as a switch which sets the state attribute (instead of state_relay). The Z2M web-UI also uses the state attribute instead of state_relay. Now there are multiple things that can happen here when turning on a light:

  1. ✔️ Set state to ON through MQTT (Home Assistant and the Z2M UI both do that). The light is turned on. This is working properly and the switch's MQTT payload will change to:
    {"brightness_relay":254, "linkquality":153,"state":"ON","state_relay":"ON"}
    (Although state_relay will be set to ON only after the next poll interval, but that is okay)
  2. ❌ Set state_relay to ON through MQTT. The light is turned on. But this way state is never updated and the switch's MQTT payload will look like this (even after the next poll interval):
    {"brightness_relay":254, "linkquality":153,"state":"OFF","state_relay":"ON"}
    This means state and state_relay will be out of sync. state will say OFF although the light is turned on. The reason is that the converter does not update state when the light is polled for updates.
  3. ❌ Worst scenario: turn on light directly on the switch or through a direct binding. This will also only update state_relay and the MQTT payload will look like this (even after the next poll interval):
    {"brightness_relay":254, "linkquality":153,"state":"OFF","state_relay":"ON"}
    state will stay OFF although the light is turned on.

Could the reason for this behavior be a wrong exposes definition? Maybe exposing e.switch() is just wrong here as my guess is that this is where state is coming from in the first place. Exposing this as a switch might also break the dimming functionality of the dimmer (I don't have one yet, but I am getting one soon), but that is a different issue.

@Koenkk
Copy link
Owner

Koenkk commented Dec 8, 2022

e.switch() should become e.switch().withEndpoint('relay')

@Danit2 Danit2 mentioned this pull request Dec 9, 2022
@klada klada marked this pull request as draft December 11, 2022 14:20
@klada
Copy link
Contributor Author

klada commented Dec 11, 2022

e.switch() should become e.switch().withEndpoint('relay')

Nice, this does the trick! So simple... 👍

But I still think e.light_brightness().withEndpoint('relay') might be the correct way as this way we could also support the brightness control of Busch-Jaeger dimmers. I am getting one of these dimmers really soon so I can do more tests.

So I'd suggest the following:

Don't change this now and wait until I get my hands on a dimmer so I can test e.light_brightness().withEndpoint('relay'). If this is actually working with both the switch and the dimmer I'd suggest we change the exposes definition from switch to light. This would still change Home Assistant auto-discovery. Would you be okay with this @Koenkk?

Additional information about these Busch-Jaeger devices

Unfortunately both the dimmer and the relay have the same ZigBee model string (RM01) so my guess is there is no way to separate the two from each other.

This means we'd always have to expose a brightness control (also for relays), even if the device might not support it. However this is the same thing as the Hue Bridge does with these devices. In the Hue ecosystem the Busch-Jaeger relays also are exposed as dimmable lights and the brightness control simply does nothing to them.

Danit2 added a commit to Danit2/zigbee-herdsman-converters that referenced this pull request Dec 12, 2022
Add `brightness_force_single_endpoint` and `on_off_force_single_endpoint` from PR Koenkk#5097
@Danit2 Danit2 mentioned this pull request Dec 12, 2022
@Koenkk
Copy link
Owner

Koenkk commented Dec 12, 2022

This would still change Home Assistant auto-discovery. Would you be okay with this @Koenkk?

Yes, thats fine

@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the stale label Jan 12, 2023
@github-actions github-actions bot closed this Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

State not updating (Busch Jäger 6711+6735)
2 participants