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

Add device-specific config overrides in HomeAssistant extension #23075

Merged
merged 8 commits into from
Jul 3, 2024
Merged

Add device-specific config overrides in HomeAssistant extension #23075

merged 8 commits into from
Jul 3, 2024

Conversation

gpayer
Copy link
Contributor

@gpayer gpayer commented Jun 16, 2024

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
Copy link
Owner

Koenkk commented Jun 16, 2024

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

@gpayer
Copy link
Contributor Author

gpayer commented Jun 16, 2024

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: Koenkk/zigbee-herdsman-converters#7520

@burmistrzak
Copy link

@Koenkk I fully support this PR.
As we have discussed in Koenkk/zigbee-herdsman-converters#7498 (comment), 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
Copy link
Owner

Koenkk commented Jun 17, 2024

@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
Copy link

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
Copy link
Owner

Koenkk commented Jun 22, 2024

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
Copy link

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
Copy link
Owner

Koenkk commented Jun 23, 2024

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
Copy link
Contributor Author

gpayer commented Jun 23, 2024

@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
Copy link

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

@burmistrzak
Copy link

@gpayer Alright, once Koenkk/zigbee-herdsman-converters#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
Copy link
Contributor Author

gpayer commented Jun 24, 2024

@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
Copy link
Owner

Koenkk commented Jun 24, 2024

@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
Copy link

burmistrzak commented Jun 25, 2024

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
Copy link
Owner

Koenkk commented Jun 25, 2024

@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
Copy link

@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
Copy link
Owner

Koenkk commented Jun 27, 2024

@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
Copy link

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

@Koenkk
Copy link
Owner

Koenkk commented Jun 28, 2024

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

@burmistrzak

This comment was marked as outdated.

@burmistrzak
Copy link

@gpayer Would you mind updating your PR?
Please revert everything except the tests, and update the discover method as described in #23075 (comment). 👀

@Koenkk
Copy link
Owner

Koenkk commented Jun 29, 2024

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
Copy link
Contributor Author

gpayer commented Jun 29, 2024 via email

gpayer added 2 commits June 29, 2024 22:36
* override implementation for Bosch BTH-RA
* unit test including mockup for this device
@gpayer
Copy link
Contributor Author

gpayer commented Jun 29, 2024

@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
Copy link

burmistrzak commented Jun 30, 2024

@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
Copy link
Contributor Author

gpayer commented Jun 30, 2024

@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
Copy link

burmistrzak commented Jun 30, 2024

@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
Copy link
Owner

Koenkk commented Jul 1, 2024

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

@gpayer
Copy link
Contributor Author

gpayer commented Jul 1, 2024

@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
Copy link
Owner

Koenkk commented Jul 1, 2024

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

@gpayer
Copy link
Contributor Author

gpayer commented Jul 1, 2024

@Koenkk looks good! I like it.

@burmistrzak
Copy link

@Koenkk Yey! Thanks for helping out! 🥳
While I can't verify the HA payload myself, I trust @gpayer's judgment here.

Quick question regarding tests: What modelID exactly are we mocking here? The Zigbee model?

@Koenkk
Copy link
Owner

Koenkk commented Jul 2, 2024

Quick question regarding tests: What modelID exactly are we mocking here? The Zigbee model?

Yes, the zigbee modelId

@burmistrzak
Copy link

@Koenkk Alright, then we should probably change the mock test to use the actual Zigbee model. So RBSH-TRV0-ZB-EU instead of BTH-RA.
Other than that, this PR seems to be good to go. 👌

@Koenkk
Copy link
Owner

Koenkk commented Jul 3, 2024

That's what's used currently? https://github.com/Koenkk/zigbee2mqtt/pull/23075/files#diff-e29b04f07db2e50de3bef35203be101f6520fdb1e1cbde5d90fe7e34e84937b4R395

@Koenkk Koenkk merged commit 4210edf into Koenkk:dev Jul 3, 2024
1 of 11 checks passed
@gpayer gpayer deleted the ha-ext-support-operating-mode branch July 3, 2024 21:41
@burmistrzak
Copy link

That's what's used currently?

@Koenkk Whoops! You're totally correct. I still had the previous code snippet in mind. 🙈

Great to see this merged! We used this super specific issue to add some really useful functionality.
Good job everyone! 💪

@dierochade
Copy link

hei folks.

just gave the dev branch a try and my 7 thermostats just seem FINE.

Well done!

@burmistrzak
Copy link

@dierochade Nice! 🥳
Just to confirm, you did reverse your temporary workaround in HA, right?

@dierochade
Copy link

Just to confirm, you did reverse your temporary workaround in HA, right?

confirmed...

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.

4 participants