Add device-specific config overrides in HomeAssistant extension

[gpayer]

This fixes #22892.

On the one hand the Home Assistant MQTT extension expects a standardized interface to interact with devices, on the other hand Z2M provides a quite direct access to the exposed endpoints of a device.

Unfortunately Bosch BTH-RA implemented a custom expose operating_mode, which no other device uses.

This PR introduces a self-contained method overridePayloadForNonConformingDevices where special adaptions to the produced config can be done. At the moment this is only done for Bosch BTH-RA.

Unfortunately I was only able to test this PR with a unit test. The expected payload is what I set manually in my MQTT server to get my devices working again via Home Assistant. So the payload itself is tested, but it would be great if someone could test this from end to end.

Ideas for future changes:

• use (and maybe extend?) Device.config.homeassistant to specifiy more precisely how the payload is to be adapted
• should Bosch actually fix its firmware, then this would have to added to the check as well

[Koenkk]

I didnt dive into the details yet, but cant we change the device definition to expose system_mode instead?

[gpayer]

I didnt dive into the details yet, but cant we change the device definition to expose system_mode instead?

It exposes system_mode, but system_mode is broken and unusable on this device. For further details please discuss this with @burmistrzak, they did the relevant changes to zigbee-herdman-converters.

EDIT: this is the relevant PR: https://github.com/Koenkk/zigbee-herdsman-converters/pull/7520

[burmistrzak]

@Koenkk I fully support this PR. As we have discussed in https://github.com/Koenkk/zigbee-herdsman-converters/pull/7498#issuecomment-2101341572, the TRV in question uses the Bosch-specific operating_mode (schedule | manual | pause) instead of system_mode (off | heat | cool | etc.) to switch modes.

While the system_mode attribute is available, it really only supports heat and that's what we expose. Having both mode attributes exposed, allows downstream projects to decide for themselves, which one is the better fit for their ecosystem. 😊

Beside that, auto has a different meaning in HA, so schedule from operating_mode can be cleanly mapped here as well. In other standards (incl. ZCL), auto is AFAIK only meant for thermostats that can automatically switch between a heating and cooling mode to keep the temperature within a set range.

[Koenkk]

@burmistrzak cant we trick the system_mode here? e.g. if you send system mode off its sends operating mode pause to the device

[burmistrzak]

cant we trick the system_mode here? e.g. if you send system mode off its sends operating mode pause to the device

I'd rather not mess with an already existing cluster attribute. Had all sorts of issues (especially when it comes to debugging) with faking/overriding attribute values of existing keys.

However, were system_mode missing entirely, I wouldn't hesitate to implement it as you've described. Also, keeping operating_mode and system_mode separate is in line with what we've done with the entire range of Bosch thermostats.

[Koenkk]

But given that system_mode is always heat, what is the value of exposing it? Doesn't it make more sense to just drop system_mode (which seems to be useless) and rename operating_mode to system_mode?

[burmistrzak]

But given that system_mode is always heat, what is the value of exposing it?

Well, effectively all downstream projects require system_mode to make thermostats work, so there's that. Besides, e.g. HomeKit works just fine with a single system_mode, and that's exactly how the Bosch bridge does it.

Doesn't it make more sense to just drop system_mode (which seems to be useless) and rename operating_mode to system_mode?

Overriding system_mode with the values from operating_mode is super sketchy, leads to all sorts of complex issues (because the TRV itself actually supports SystemMode), and can't really be done cleanly in this particular case. ZCL also has no concept of an internal schedule specified, but Home Assistant, on the other hand, does. So it makes total sense to expose operating_mode just for that alone to HA, but while we're at it, why not expose the other operating modes as well? We thus completely disregard system_mode, but only in the case of HA, because it actually can make use of these additional modes. 😊 That's how we've ended up with this PR.

[Koenkk]

Then I would propose to push this down to the definition. Introduce a new property on the definition.meta called overrideHaConfig. We definitely don't want device specific logic to end up in this file.

[gpayer]

@Koenkk I don't know how to do this. Could you provide some pointers and examples how this could be implemented? Unfortunately I don't have the time to dive into Z2M at an arbitrary depth.

But the end goal must be that it "just works"(TM). Every implementation that involves manual steps for owners of this type of device is not useful.

[burmistrzak]

@Koenkk Great idea! 💡 @gpayer I'll take care of this. 👌

[burmistrzak]

@gpayer Alright, once https://github.com/Koenkk/zigbee-herdsman-converters/pull/7685 is finalized and merged, you'll need to update your implantation a bit.

But we first need to define how overrideHaConfig should be structured. What about something like this?

[...]

 {
    zigbeeModel: ['RBSH-TRV0-ZB-EU'],
    model: 'BTH-RA',
    vendor: 'Bosch',
    description: 'Radiator thermostat II',
    meta: {
        overrideHaConfig: {
            climate: {
                mode_command_topic: '',
                mode_command_template: '<template>',
                mode_state_template: '<template>',
                modes: ['off', 'heat', 'auto'],
            },
        },
    },

[...]

If everything goes right, we'll be able to handle similar cases of non-compliant devices right inside the definition!

[gpayer]

@burmistrzak thanks for your work, that looks already very good. We only have to clarify how mode_command_topic works in this override.

Do you have any knowledge at your code location what the final base topic will be like? Then you could provide the complete command topic. But I assume creating the final base topic is only done later in Z2M itself.

So my suggestion would be to fill mode_command_topic in the override with its path relative to the base topic. In the case of Bosch BTH-RA this would be /set.

The default if no override is present would remain as it is currently: /set/system_mode.

What do you think?

[Koenkk]

@burmistrzak I would say:

• overrideHaConfig accepts all the generated configs and updates them in place overrideHaConfig(configs: DiscoveryEntry[]): void
• homeassistant.ts is updated like this:

const newDiscoveredTopics: Set<string> = new Set();

const configs = this.getConfigs(entity);
if (entity.isDevice()) {
    entity.definition?.meta?.overrideHaConfig?.(configs);
}

for (const config of configs) {

[burmistrzak]

So my suggestion would be to fill mode_command_topic in the override with its path relative to the base topic. In the case of Bosch BTH-RA this would be /set.

@gpayer That's how it should work! 🤓

@Koenkk Huh, so overrideHaConfig(configs: DiscoveryEntry[]): void is a function, called from homeassistant.ts with the still unmodified configs as an argument? But where in this case do we get the actual override from?

[Koenkk]

@burmistrzak yes, the configs are then updated which will the be further used by the ha extension. As an alternative we could also return the new configs, functionality it doesnt make a difference

[burmistrzak]

@Koenkk Ah, I see. So your idea essentially is an optional function in definition.meta that just gets written when we need to somehow modify the HA config of a given device? I'm not sure how developer-friendly that really is, tho...

The second option would be to simply check for definition.meta.overrideHaConfig and if it exists on a given device, we'll use that part of the config instead. Seems IMHO much more straightforward. 😅

[Koenkk]

@burmistrzak how do you plan to set the mode_command_topic? since you cannot put logic like: payload.mode_command_topic = commandTopic.substring(0, commandTopic.lastIndexOf('/system_mode')); in there

[burmistrzak]

@Koenkk Yeah, was just thinking about that... So I guess a function it is? 😅 Going for maximum flexibility here makes definitely more sense.

[Koenkk]

@burmistrzak I see no other way indeed, this would also allow for e.g. deleting entities

[burmistrzak]

@Koenkk I'll also have to define the interfaces for MockProperty & DiscoveryEntry in src/lib/types.ts, right? Or are they available somewhere to be imported?

[burmistrzak]

@gpayer Would you mind updating your PR? Please revert everything except the tests, and update the discover method as described in https://github.com/Koenkk/zigbee2mqtt/pull/23075#issuecomment-2185956661. 👀

[Koenkk]

I'll also have to define the interfaces for MockProperty & DiscoveryEntry in src/lib/types.ts, right?

That's fine!

After this PR is updated it can be merged.

[gpayer]

Will do, either this evening or tomorrow!

burmistrzak @.***> schrieb am Fr., 28. Juni 2024, 22:17:

@gpayer https://github.com/gpayer Would you mind updating your PR? Please revert everything except the tests, and update the discover method as described in #23075 (comment) https://github.com/Koenkk/zigbee2mqtt/pull/23075#issuecomment-2185956661. 👀

— Reply to this email directly, view it on GitHub https://github.com/Koenkk/zigbee2mqtt/pull/23075#issuecomment-2197581958, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANIEFQ7OV2JBC4WYMGX3ATZJXAHDAVCNFSM6AAAAABJMVOBWKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJXGU4DCOJVHA . You are receiving this because you were mentioned.Message ID: @.***>

[gpayer]

@burmistrzak I tried integrating overrideHaConfig and unfortunately this breaks a majority of tests. However the how is very confusing and I have no idea how to fix this.

It all depends on where the Bosch BTH-RA mock device appears in the list of mock devices! If it's not there, all tests except my new test succeed. At the end 27 tests fail. At the start right after the coordinator, 40 tests fail.

So this is deeply in wtf territory.

My vague assumption is that there is something bad going on with references, but I don't know enough about implementation details in Javascript VMs.

Do you have an idea?

Maybe it would be better for overrideHAConfig to not take an array but a single DiscoveryEntry and actually return the changed version.

[burmistrzak]

@gpayer Wonky! Seems like your stub definition was a bit off. Probably the model ID?

Maybe @Koenkk knows why RBSH-TRV0-ZB-EU isn't working as a mocked modelID? 🤔

Try this in zigbeeHerdsman.js instead:

[...]

const custom_clusters_2 = {
    custom_1: {
        ID: 513,
        attributes: {
            attribute_0: {ID: 16391, type: 48, manufacturerCode: 4617},
            attribute_1: {ID: 16416, type: 48, manufacturerCode: 4617},
            attribute_2: {ID: 16418, type: 48, manufacturerCode: 4617},
            attribute_3: {ID: 16448, type: 41, manufacturerCode: 4617},
            attribute_4: {ID: 16450, type: 48, manufacturerCode: 4617},
            attribute_5: {ID: 16451, type: 48, manufacturerCode: 4617},
        },
        commands: {},
        commandsResponse: {},
    },
    custom_2: {
        ID: 516,
        attributes: {
            attribute_0: {ID: 16395, type: 32, manufacturerCode: 4617},
            attribute_1: {ID: 16441, type: 48, manufacturerCode: 4617},
            attribute_2: {ID: 16442, type: 48, manufacturerCode: 4617},
            attribute_3: {ID: 16443, type: 48, manufacturerCode: 4617},
        },
        commands: {},
        commandsResponse: {},
    },
};

[...]

Put this after the first custom_clusters definition 👆

[...]

RBSH_TRV0_ZB_EU: new Device(
        'EndDevice',
        '0x18fc2600000d7ae2',
        35902,
        4617,
        [new Endpoint(1, [0, 1, 3, 4, 32, 513, 516, 2821], [1, 32, 513, 516], '0x18fc2600000d7ae2', [], {
          overrideHaConfig: (configs) => {
            const entry = configs.findIndex((e) => e.type === 'climate');
            if (entry) {
              const commandTopic = configs[entry].discovery_payload.mode_command_topic;
              configs[entry].discovery_payload.mode_command_topic = commandTopic.substring(0, commandTopic.lastIndexOf('/system_mode'));
              configs[entry].discovery_payload.mode_command_template =
                `{% set values = ` +
                `{ 'auto':'schedule','heat':'manual','off':'pause'} %}` +
                `{"operating_mode": "{{ values[value] if value in values.keys() else 'pause' }}"}`;
              configs[entry].discovery_payload.mode_state_template =
                `{% set values = ` +
                `{'schedule':'auto','manual':'heat','pause':'off'} %}` +
                `{% set value = value_json.operating_mode %}{{ values[value] if value in values.keys() else 'off' }}`;
              configs[entry].discovery_payload.modes = ['off', 'heat', 'auto'];
            }
          }
        })],
        true,
        'Battery',
        'BTH-RA',
        false,
        'Bosch',
        '20231122',
        '3.05.09',
        custom_clusters_2,
    ),

Keep it simple, and add this 👆 to the bottom of devices.

You'll also have to update your actual test, because that function you're trying to call, no longer exists.

[gpayer]

@burmistrzak thanks for your input, this definitely fixed the other tests. However it does not fix the relevant test.

I put in some debug outputs in the mqtt mock and the main problem is, that there are no messages for the topic homeassistant/climate/0x18fc2600000d7ae2/climate/config. Mostly homeassistant/sensor/0x18fc2600000d7ae2/linkquality/config is used and some other less interesting ones.

I must really say, I'm at my wits ends, I do not have the necessary deep Zigbee knowledge to work in this project. Simply creating a working mock device is so far beyond my knowledge, that it make no sense for me to be working on this atm.

I have no idea what to do and I really do not have the time to deep dive into this project.

As basically all necessary pieces seem to be there, someone more experienced should be able to finish this in a few minutes.

I'll push my last changes, maybe the help somehow.

[burmistrzak]

@gpayer That's alright. You gave it a shot. 🤝 I might pick this up myself at some point, but unfortunately can't promise anything.

Maybe @Koenkk wants to jump in, and help out with the tests. Otherwise, this will have to wait.

[Koenkk]

While fixing the tests, can you guys confirm that the discovery payload is correct?

[gpayer]

@Koenkk yes, the payload is correct. This is a copy of what I'm using right know in my live system. Only object_id was changed.

[Koenkk]

Great, I've fixed the tests (will pass once https://github.com/Koenkk/zigbee-herdsman-converters/pull/7720 is merged), can you both review this PR and https://github.com/Koenkk/zigbee-herdsman-converters/pull/7720 ?