-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
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 |
@Koenkk I fully support this PR. While the Beside that, |
@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 |
But given that |
Well, effectively all downstream projects require
Overriding |
Then I would propose to push this down to the definition. Introduce a new property on the |
@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. |
@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 [...]
{
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! |
@burmistrzak thanks for your work, that looks already very good. We only have to clarify how 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 The default if no override is present would remain as it is currently: What do you think? |
@burmistrzak I would say:
|
@gpayer That's how it should work! 🤓 @Koenkk Huh, so |
@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 |
@Koenkk Ah, I see. The second option would be to simply check for |
@burmistrzak how do you plan to set the |
@Koenkk Yeah, was just thinking about that... So I guess a function it is? 😅 |
@burmistrzak I see no other way indeed, this would also allow for e.g. deleting entities |
This comment was marked as outdated.
This comment was marked as outdated.
@gpayer Would you mind updating your PR? |
That's fine! After this PR is updated it can be merged. |
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)
<#23075 (comment)>.
👀
—
Reply to this email directly, view it on GitHub
<#23075 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANIEFQ7OV2JBC4WYMGX3ATZJXAHDAVCNFSM6AAAAABJMVOBWKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJXGU4DCOJVHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
* override implementation for Bosch BTH-RA * unit test including mockup for this device
@burmistrzak I tried integrating 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. 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 |
@gpayer Wonky! Seems like your stub definition was a bit off. Probably the model ID? Maybe @Koenkk knows why Try this in [...]
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 [...]
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 You'll also have to update your actual test, because that function you're trying to call, no longer exists. |
@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 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. |
While fixing the tests, can you guys confirm that the discovery payload is correct? |
@Koenkk yes, the payload is correct. This is a copy of what I'm using right know in my live system. Only |
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 ? |
@Koenkk looks good! I like it. |
Yes, the zigbee |
@Koenkk Alright, then we should probably change the mock test to use the actual Zigbee model. So |
@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. |
hei folks. just gave the dev branch a try and my 7 thermostats just seem FINE. Well done! |
@dierochade Nice! 🥳 |
confirmed... |
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:
Device.config.homeassistant
to specifiy more precisely how the payload is to be adapted