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.

Sign up for GitHub

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

Jump to bottom

Pre-PR - Issues/Question for Switch().withState #7740

Closed
robertalexa opened this issue Jun 9, 2021 · 48 comments
Closed

Pre-PR - Issues/Question for Switch().withState #7740

robertalexa opened this issue Jun 9, 2021 · 48 comments
Labels
problem Something isn't working

Comments

@robertalexa
Copy link
Contributor

@sjorge @Koenkk

Hey guys,

I am trying to put together a new PR for the Danfoss and Hive (again).

Looking randomly through issues I have seen some screenshots from HA for entities belonging to a tuya thermostats. Those entities were actual switches in HA (you could toggle them) compared to the entities I get for the Hive thermostat which are just plain text.

I have checked the code and found the "switch" vs "binary" that I was using. So far so good.

Here is my code, above is the new switch and below is the old binary:

exposes.switch()
    .withState('window_open_external', true, 'Set if the window is open or close. This setting will ' +
        'trigger a change in the internal window and heating demand. `false` (windows are closed) or ' +
        '`true` (windows are open)', ea.ALL, true, false),
exposes.binary('window_open_external', ea.ALL, true, false)
    .withDescription('Set if the window is open or close. This setting will trigger a change in the internal ' +
        'window and heating demand. `false` (windows are closed) or `true` (windows are open)'),

Here is what I get in the Frontend of z2m:
image
image

Now do you notice how the exposes entity has the name of "state"? That is not right.

So I looked further in z-h-c/lib/exposes.js

class Switch extends Base {
    constructor() {
        super();
        this.type = 'switch';
        this.features = [];
    }

    withState(property, toggle, description, access=a.ALL, value_on='ON', value_off='OFF') {
        assert(!this.endpoint, 'Cannot add feature after adding endpoint');
        const feature = new Binary('state', access, value_on, value_off).withProperty(property).withDescription(description);
        if (toggle) {
            feature.withValueToggle('TOGGLE');
        }

        this.features.push(feature);
        return this;
    }
}

The switch, when passed the withState method will create an push a feature form the Binary class, with a HARDCODED name of "state".

Surely the hardcoded "state" should be replaced with property, thus passing the chosen name.

Now I feel that is the right approach, but I have doubts about all the other Switches that currently exist in code, for example (to name a few)

window_detection: () => new Switch().withState('window_detection', true, 'Enables/disables window detection on the device', access.STATE_SET),
valve_detection: () => new Switch().withState('valve_detection', true).setAccess('state', access.STATE_SET),
auto_lock: () => new Switch().withState('auto_lock', false, 'Enable/disable auto lock', access.STATE_SET, 'AUTO', 'MANUAL'),
away_mode: () => new Switch().withState('away_mode', false, 'Enable/disable away mode', access.STATE_SET),

Does that mean that all these entities currently show up as "state" in the Frontend of z2m?

My approach would not harm anything as far as I can tell, because all .withState will have property defined, and then the new binary will be using it, but I just want some feedback before I write code and do a PR.

Looking forward to hearing from you,
Rob
@robertalexa robertalexa added the problem Something isn't working label Jun 9, 2021
@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 9, 2021

I think binary is correct here, a Switch() is just a fancy binary expose anyway with a hardcoded label of state.

I think the proper thing to do is update the way they are converted into the homeassistant config z2m generates. (lib/extenstions/homeassistant.js).

We should probably expose binary expose that are writable the same as a switch (but with the proper free form label instead of the hardcoded 'state'). And expose readonly binaries like we do now as just a text element.

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 9, 2021 •
edited
Loading

zigbee2mqtt/lib/extension/homeassistant.js

Lines 332 to 354 in 9bd4693

} else if (expose.type === 'binary') {
const lookup = {
occupancy: {device_class: 'motion'},
battery_low: {device_class: 'battery'},
water_leak: {device_class: 'moisture'},
vibration: {device_class: 'vibration'},
contact: {device_class: 'door'},
smoke: {device_class: 'smoke'},
gas: {device_class: 'gas'},
carbon_monoxide: {device_class: 'safety'},
presence: {device_class: 'presence'},
};
discoveryEntry = {
type: 'binary_sensor',
object_id: expose.endpoint ? `${expose.name}_${expose.endpoint}` : `${expose.name}`,
discovery_payload: {
value_template: `{{ value_json.${expose.property} }}`,
payload_on: expose.value_on,
payload_off: expose.value_off,
...(lookup[expose.name] || {}),
},
};

I think this needs to be updates to something like (not tested):

                } else if (expose.type === 'binary') {
                    const lookup = {
                        occupancy: {device_class: 'motion'},
                        battery_low: {device_class: 'battery'},
                        water_leak: {device_class: 'moisture'},
                        vibration: {device_class: 'vibration'},
                        contact: {device_class: 'door'},
                        smoke: {device_class: 'smoke'},
                        gas: {device_class: 'gas'},
                        carbon_monoxide: {device_class: 'safety'},
                        presence: {device_class: 'presence'},
                    };
                    if (expose.access & 0b010) { // FIXME: need to check if the access is something settable, then we become a switch
                        discoveryEntry = {
                            type: 'switch',
                            object_id: expose.endpoint ? `switch_${expose.name}_${expose.endpoint}` : `switch_${expose.name}`,
                            discovery_payload: { /// FIXME: not sure this payload is correct
                                value_template: `{{ value_json.${expose.property} }}`,
                                payload_on: expose.value_on,
                                payload_off: expose.value_off,
                                command_topic: true,
                                command_topic_prefix: expose.endpoint ? expose.endpoint : undefined,
                                ...(lookup[expose.name] || {}),
                            },
                        };
                    } else {
                        discoveryEntry = {
                            type: 'binary_sensor',
                            object_id: expose.endpoint ? `${expose.name}_${expose.endpoint}` : `${expose.name}`,
                            discovery_payload: {
                                value_template: `{{ value_json.${expose.property} }}`,
                                payload_on: expose.value_on,
                                payload_off: expose.value_off,
                                ...(lookup[expose.name] || {}),
                            },
                        };
                    }

Notice the two FIXME:

  • Not sure the first if I added works, but we should basically check if we have the 'SET' bit, then we become a switch otherwise we are either just state, get, or state+get so we stay a normal binary sensor like we currently expose.
  • not sure I build the payload propery to include all the fields a switch needs.

@robertalexa
Copy link
Contributor Author

@sjorge Thanks for the quick feedback.

I see what you mean about Switch vs Binary, but would you be kind to explain the choice of using a Switch in the case of the below? Just trying to understand the decision making between this and the Danfoss/Hive.

window_detection: () => new Switch().withState('window_detection', true, 'Enables/disables window detection on the device', access.STATE_SET),
valve_detection: () => new Switch().withState('valve_detection', true).setAccess('state', access.STATE_SET),
auto_lock: () => new Switch().withState('auto_lock', false, 'Enable/disable auto lock', access.STATE_SET, 'AUTO', 'MANUAL'),
away_mode: () => new Switch().withState('away_mode', false, 'Enable/disable away mode', access.STATE_SET),

Also could you share some light on:

Does that mean that all these entities currently show up as "state" in the Frontend of z2m?

If they do, how is that not confusing as hell, because surely there is more than 1 of those entities present for 1 device, so multiple state properties? :)

Back to the Binary, not sure if if (expose.access & 0b010) this will be readable there, no idea. Also finding it super difficult to test things in a home assistant environment with the addon, and with custom code..... I fully understand what you are trying to do, but don't know.

The other option is to check the actual property, like it is done here, using expose.property instead, and doing a manual check for certain properties. But then again, the same name might be used for other devices, that might not have SET access, and making a new, bigger issue.
image

Rob

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 9, 2021 •
edited
Loading

I see what you mean about Switch vs Binary, but would you be kind to explain the choice of using a Switch in the case of the below? Just trying to understand the decision making between this and the Danfoss/Hive.

https://github.com/Koenkk/zigbee-herdsman-converters/blob/36cf29c23c1d8bfd08fb080d8947acecb99aca3e/lib/exposes.js#L60-L77

class Switch extends Base {
    constructor() {
        super();
        this.type = 'switch';
        this.features = [];
    }

    withState(property, toggle, description, access=a.ALL, value_on='ON', value_off='OFF') {
        assert(!this.endpoint, 'Cannot add feature after adding endpoint');
        const feature = new Binary('state', access, value_on, value_off).withProperty(property).withDescription(description);
        if (toggle) {
            feature.withValueToggle('TOGGLE');
        }

        this.features.push(feature);
        return this;
    }
}

A switch is just a 'wrapper' around a binary expose item.
Basically a Switch is a 'thing' that has a changeable (usually) ON and OFF state.

Usually they perform one function, turn on or off the flow of water (valve with a tap), turn on or off the flow of power to a set of lights (light switch).

Binary's are used for things describe state or part of a larger system. Example a contact sensor is either closed or open, a TRV might have a 'child lock' that is engaged or not.

new Switch() creates a 'standalone' controllable binary state
new Binary() creates a binary state , this can be controllable (e.g.keypad lockout on a TRV), non controllable (e.g. window open detection), non controllable contact sensor, ...

Does that mean that all these entities currently show up as "state" in the Frontend of z2m?

Yes, and it is confusing as hell. I actually fixed keypad_lockout already in Koenkk/zigbee-herdsman-converters#2373 by switching it from Lock() to a Binary() because it was confusing having multiple 'state' items.

I believe the ones you mentioned above should indeed all be Binary from an expose perspective (zigbee2mqtt). That way they will look correct in the frontend. Additionally we should update homeassistant.js to pass these as switches (homeassistant perspective IF they are writable... does that make sense?

For zigbee2mqtt a Binary can be both readonly or writable, which to me makes sense.
For homeassistant a Binary is always readonly and a switch is always writable...

@robertalexa
Copy link
Contributor Author

@sjorge
I was fully aware of everything you said up to "Yes, and it is confusing as hell."

And that is exactly why as asked because by looking at the code, my deduction was that one TRV can have 5 "state" properties as switches.... and I didn't want to believe it :)
image

Thanks for clarifying.

PS: this is a can of worms for sure ....

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 9, 2021 •
edited
Loading

And that is exactly why as asked because by looking at the code, my deduction was that one TRV can have 5 "state" properties as switches.... and I didn't want to believe it :)

Lets look at the danfoss

property zigbee2mqtt homeassistant
keypad_lockout binary(unlock,lock1), writable switch
mounted_mode_active binary (true,false) binary_sensor
mounted_mode_control binary (true,false), writable switch
thermostat_vertical_orientation binary(true,false), writable switch
viewing_direction binary(vertical,horizontal), writable switch
heat_available binary(true,false) binary_sensor
heat_required binary(true,false) binary_sensor
setpoint_change_source enum(0=local,1=?schedule?,2=zigbee) ?
window_open_internal binary(true,false) binary_sensor
window_open_external binary(true,false), writable switch
day_of_week enum(...) ?
trigger_time number, writable ?
algorithm_scale_factor enum(...), writable ?
load_estimate number ?

So yes, that would be 5 switchs for homeassistant and 4 binary_sensors and a few things I have no idea about.

Some things i just noticed:

  • heat_available is marked as access=ALL, is that correct? I don't think we can tell the device hey you have excess heat available to you?
  • viewing_direction would make more sense to expose as an Enum I think as it doesn't have a yes/no/true/false/on/off state, a fixed 2 distinct things 'vertical' and 'horizontal'

@robertalexa
Copy link
Contributor Author

Yes that is correct. heat_available is R/W
image

All the accesses I gave for these 2 devices were according to the Danfoss specs document and tested on my Hive, which responded the same way.

However, as I mentioned in the property description, I have no idea how changing this property impacts the device. The documentation was not clear either. I know it is self explanatory, you tell the TRV there is no heating available, but what effect does that have is unclear to me.

Thinking from the angle of "automations" the only switches that make sense are:

  • keypad_lockout
  • mounted_mode_control
  • window_open_external

All the others that you have marked as switches are "cool features" but there is no point in changing viewing direction or thermostat orientation cause the radiator is the way it is, it doesn't really change. For the above mentioned reason, i think heat_available should also be skipped. Personally I think they should be plain text in HA to avoid accidental changes.

To help you visualise the entities in HA with the device "as-is":
image

(Personally I keep most of them disabled, just enabled them now to show you)

I hope this answers your queries?

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 9, 2021

However, as I mentioned in the property description, I have no idea how changing this property impacts the device. The documentation was not clear either. I know it is self explanatory, you tell the TRV there is no heating available, but what effect does that have is unclear to me.

Oh, that makes more sense as in you tell the TRV there is heat available not the TRV telling something it has heat available.
I guess if the boiler is off it could tell the TRV to not waste battery opening/closing the valve.

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 9, 2021

Thinking from the angle of "automations" the only switches that make sense are:

* keypad_lockout

* mounted_mode_control

* window_open_external

All the others that you have marked as switches are "cool features" but there is no point in changing viewing direction or thermostat orientation cause the radiator is the way it is, it doesn't really change. For the above mentioned reason, i think heat_available should also be skipped. Personally I think they should be plain text in HA to avoid accidental changes.

The hass config that z2m generates is based on the expose information, so to remove say the orientation you need to remove the expose field. That also means it disappears from the frontend.

I think the documentation now also gets partially generated from the expose information though, so if you remove the expose for these we probably need to manually document those. I do think it makes sense to not expose those at all, probably the same for mounted_mode_control. This seems like an action you don't really do often and is probably OK to manually send the mqtt message to it.

Aside from battery and the basic climate expose I think only these really make sense to have:

  • keypad_lockout
  • window_open_external
  • window_open_internal
  • heat_required

As those seem like you'd want to potentially key automations of heat_required and window_open_internal and might want to set window_open_external in some automation and keypad_lockout manually.

@robertalexa
Copy link
Contributor Author

I agree with you, but my testing didn't show a change in the trv behaviour. So I don't know what the pid does internally when you turn heat available off.

Realistically, it doesnt matter. A different thermostat will control the central heating from some sort of scheduler, say node red. Whoever set the schedule or behaviour will be aware of it and deal with the trvs too.

But i don't mind setting heat available as a switch too. Personally I won't use it on the basis of not understanding the mechanics behind it.

Still, i have no idea how to approach the ha entity to be a switch, i understand how, I don't know if we can read the access of the attribute at that point.

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 9, 2021 •
edited
Loading

Still, i have no idea how to approach the ha entity to be a switch, i understand how, I don't know if we can read the access of the attribute at that point.

They should all be binary exposed with the proper access set (which is already the case I think)

Then just need to update lib/extention/homeassistant.js as mentioned here: #7740 (comment)

I think the code I linked is a pretty good starting point, it might even work but I don't have HA setup so can't test.

@robertalexa
Copy link
Contributor Author

Sorry just now seen the second reply. I chose to expose them all to the frontend to give all the flexibily of the device. If a person clicks the wrong buttons.... Sorry but you can't fix stupid.

However i agree they don't need to be changed to switches in ha. Only available via frontend and mqtt.

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 9, 2021 •
edited
Loading

However i agree they don't need to be changed to switches in ha. Only available via frontend and mqtt.

That's were part of the complexity lies, the expose information describes the device and it's properties e.g. binary state for x, binary state for y, a mode with values X Y or Z and if they are readable, writable, ... this information is then turned into something homeassistant can used by lib/extention/homeassistant.js.

As you pointed out originally, using 'Switch()' as a short hand for 'Binary()' that is writable is incorrect as it hardcode a bunch of stuff like the label being state confusing the frontend for example and other things consuming the expose info. (Also the linked PR about keypad_lockout a few comments ago, where the same conclusion was reached that keypad_lockout was a writable binary instead of a switch)

This reason some of those exposes used Switch() and not Binary() is probably because lib/extention/homeassistant.js doesn't understand the concept that a binary can be writable (and needs to translated differently for ha).

So there are 2 'issues' that need addressing I guess

  • homeassistant.js should treat binary readonly and writable as 2 different things for ha
  • expose information for some devices should be updated to writable binaries instead

This does mean that everything will be exported to ha as a binary_sensor or a switch depending on if it is writable or not, but I guess same as the frontend, the user can 'disable' the device's entity (at least according to the ha docs I am reading)

I guess this might count as a 3rd issue:
Expose needs a way to mark entries as 'important', those that are not could be for example then be hidden behind a collapsible thingy in the frontend and not send to ha, unless for example a new configuration property is set?

@robertalexa
Copy link
Contributor Author

To be perfectly honest with you, here is how I see this.

  • The attributes exposed in z2m - talking about binaries only - have the correct access set, could be SET, GET (and the variations)
  • They behave correctly in Frontend, and allow the users to do everything the device can do. This means if you do not know what you are doing, you can mess up your TRV, but the descriptions should prevent this if they are healthy and explanatory.
  • These exposes also generate the documentation correctly.
  • For HA, we need a mechanism, like you suggest, to differentiate between a GET binary and a SET binary. If they are just GET, they get exposed to HA the same way they are now, which is a binary_sensor, which only displays information in HA but does not allow editing. If they are also SET, they need to be exposed as switch, which in HA can be toggled.
  • It is perfectly acceptable to pass all of the attributes to HA, because in HA the user can choose to disable the entities that they do not need. Here is the same TRV I have showed you above, but with some of the entities disabled:
    image
    image

Personally I feel this is sufficient and not over engineered, while still allowing full control in Frontend and as much control as the user wants in HA.

Your approach with access seems correct, and as long as the access is correct in the device, the correct entity will be created in HA, without further code changes (on other or new devices).

I know I have basically summarised a lot from what you said, it was more of a recap and an intent to show you the HA side visually.

I actually fixed keypad_lockout already in Koenkk/zigbee-herdsman-converters#2373 by switching it from Lock() to a Binary() because it was confusing having multiple 'state' items.

I believe the ones you mentioned above should indeed all be Binary from an expose perspective (zigbee2mqtt). That way they will look correct in the frontend. Additionally we should update homeassistant.js to pass these as switches (homeassistant perspective IF they are writable... does that make sense?

For zigbee2mqtt a Binary can be both readonly or writable, which to me makes sense.
For homeassistant a Binary is always readonly and a switch is always writable...

I have only realised you have edited that comment. I fully agree with you on the massive difference between Binaries between z2m and ha.

I will do my best to make my HA setup work while using the Addon and making changes to code. And hopefully we can find a solution for this that will help not just my scenarion but all the SET Binaries.

Thanks a lot for your input in this thread, though for the most time we both agreed with each other over many number of comments :)

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 9, 2021 

* For HA, we need a mechanism, like you suggest, to differentiate between a GET binary and a SET binary. If they are just GET, they get exposed to HA the same way they are now, which is a binary_sensor, which only displays information in HA but does not allow editing. If they are also SET, they need to be exposed as switch, which in HA can be toggled.

This already exists, that is what the access property is for, it's a bit value

https://github.com/Koenkk/zigbee-herdsman-converters/blob/36cf29c23c1d8bfd08fb080d8947acecb99aca3e/lib/exposes.js#L413-L438

If the set bit is set, it's writable. Hence my check in the rough draft code above:

 if (expose.access & 0b010) { // FIXME: need to check if the access is something settable, then we become a switch

Not sure it is correct, bitwise anding/oring always confuses me -_-

@robertalexa
Copy link
Contributor Author

Yep, noted this info since you first mentioned it. This is the approach I will try, just need to get everything setup with a test HA.

Will keep you updated. If I get to something that works, I will submit a PR and link it.

Rob

@robertalexa
Copy link
Contributor Author

I am honestly unable to make any changes while part of HA addon, in order to work this out.

While in HA addon you end up with pre build dockers, if you make changes to files and restart the addon your changes are lost.

I have no idea how to approach this to test.

If either of you can point me in the right directions that would be appreciated, otherwise, I am unsure how to work this out in a way that is not coding blindly and hoping for the best. @sjorge @Koenkk

Rob

@robertalexa
Copy link
Contributor Author

Note: Just found this which is exactly what i need
https://github.com/zigbee2mqtt/hassio-zigbee2mqtt/blob/67ac8969854cc1e192b3ef9f40ca3bd7ed44b90e/CONTRIBUTING.md#developing-against-the-home-assistant-container

Tomorrow is another day :)

@robertalexa
Copy link
Contributor Author

@sjorge
I have managed to get this working. Thank f*** for the above instruction. I will probably do a PR for .io to have a link to it, for anyone else wondering.

Using the code you have suggested I started by dumping some bits, and they all behave as expected.

Zigbee2MQTT:debug 2021-06-10 11:21:56: ---------HA--------

Zigbee2MQTT:debug 2021-06-10 11:21:56: name keypad_lockout

Zigbee2MQTT:debug 2021-06-10 11:21:56: access 7

Zigbee2MQTT:debug 2021-06-10 11:21:56: has set

Zigbee2MQTT:debug 2021-06-10 11:21:56: ---------HA--------

Zigbee2MQTT:debug 2021-06-10 11:21:56: name mounted_mode_active

Zigbee2MQTT:debug 2021-06-10 11:21:56: access 5

Zigbee2MQTT:debug 2021-06-10 11:21:56: ---------HA--------

Zigbee2MQTT:debug 2021-06-10 11:21:56: name mounted_mode_control

Zigbee2MQTT:debug 2021-06-10 11:21:56: access 7

Zigbee2MQTT:debug 2021-06-10 11:21:56: has set

Zigbee2MQTT:debug 2021-06-10 11:21:56: ---------HA--------

Zigbee2MQTT:debug 2021-06-10 11:21:56: name thermostat_vertical_orientation

Zigbee2MQTT:debug 2021-06-10 11:21:56: access 7

Zigbee2MQTT:debug 2021-06-10 11:21:56: has set

Zigbee2MQTT:debug 2021-06-10 11:21:56: ---------HA--------

Zigbee2MQTT:debug 2021-06-10 11:21:56: name heat_available

Zigbee2MQTT:debug 2021-06-10 11:21:56: access 7

Zigbee2MQTT:debug 2021-06-10 11:21:56: has set

Zigbee2MQTT:debug 2021-06-10 11:21:56: ---------HA--------

Zigbee2MQTT:debug 2021-06-10 11:21:56: name heat_required

Zigbee2MQTT:debug 2021-06-10 11:21:56: access 5

Zigbee2MQTT:debug 2021-06-10 11:21:56: ---------HA--------

Zigbee2MQTT:debug 2021-06-10 11:21:56: name setpoint_change_source

Zigbee2MQTT:debug 2021-06-10 11:21:56: access 1

Zigbee2MQTT:debug 2021-06-10 11:21:56: ---------HA--------

Zigbee2MQTT:debug 2021-06-10 11:21:56: name window_open_external

Zigbee2MQTT:debug 2021-06-10 11:21:56: access 7

Zigbee2MQTT:debug 2021-06-10 11:21:56: has set

As you can see, some of the attributes are logging "has set", and that is as expected.

Onto the next phase - Home Assistant
image

The entities got created correctly as switches
image

Now onto the not so nice part, changing them from home assistant triggers the action as expected but z2m is throwing an error.
For example, window_open_external -> toggle to ON position

Zigbee2MQTT:debug 2021-06-10 11:46:19: Received MQTT message on 'zigbee2mqtt/Rob's TRV/set' with data 'True',
Zigbee2MQTT:error 2021-06-10 11:46:19: Invalid JSON 'True', skipping...

While doing it from the frontend:

Zigbee2MQTT:debug 2021-06-10 11:54:59: Received MQTT message on 'zigbee2mqtt/Rob's TRV/set' with data '{"window_open_external":true}',
Zigbee2MQTT:debug 2021-06-10 11:54:59: Publishing 'set' 'window_open_external' to 'Rob's TRV'

Seems like there is something else to look at. It also makes me wonder how if the z2m "switch" is throwing the same error when toggled from HA, or maybe that was already dealt with.

I now need to leave the house for the day but I am hoping to resume this later on this evening / tomorrow.

Feel free to have a look if you please while I am out of action.

PS: I am really surprised that no one ever questioned the Binary entities in HA, because obviously this impacts all devices, and surely there are some devices implemented in z2m that support toggling in Frontend but not in HA. But there is a start for everything I guess.

Speak soon,
Rob

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 10, 2021

Looks like it’s sending the string ‘True’ instead of a Boolean value true.

I’m guessing something is wrong in the payload_on and payload_off we send to ha in the discovery portion.

@robertalexa
Copy link
Contributor Author

I am out but just wanted to reply quick.

The payload is incomplete. Shouldn't it include the whole thing rather than just the value?
{"window_open_external":true}

The same applies to lock1 for keypad lock which is a string anyway. But the attribute is missing too.

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 10, 2021 via email

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 10, 2021

Hmmm based on the z2m logs, it is indeed not sending the property name and just a bare True/False, so you’re probably right. value_template is probably only used for extracting the value

@robertalexa
Copy link
Contributor Author

No idea.

But if what you say is right then keypad unlock should work. Cause the value is string lock1. Yet it throws the same error.

I will continue digging when i get back home.

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 10, 2021

I quickly scanned over the ha discovery spec, I think we're missing:

                            command_topic: true,
                            command_topic_prefix: expose.endpoint ? expose.endpoint : undefined,

value_template tells HA which item to pull out of the json payload
payload_on tells HA what value is 'ON' (e.g. 'ON', 'OPEN', ...)
payload_off same as on but for the off value
command_topic tells HA there is separate topic for setting
command_topic_prefix tells what the prefix should be.

@robertalexa
Copy link
Contributor Author

The thing is, that is exactly what i thought, and i have included those lines. You too included them in you code sample.

I didn't have time to dumb them and test. But i should be able to have a look in a couple of hours

@robertalexa
Copy link
Contributor Author

robertalexa commented Jun 10, 2021 •
edited
Loading

@sjorge
I guess this answers it

Zigbee2MQTT:debug 2021-06-10 21:44:20: -------HA--------


Zigbee2MQTT:debug 2021-06-10 21:44:20: name: window_open


Zigbee2MQTT:debug 2021-06-10 21:44:20: endpoint: undefined


Zigbee2MQTT:debug 2021-06-10 21:44:20: -------HA--------


Zigbee2MQTT:debug 2021-06-10 21:44:20: name: window_open_force


Zigbee2MQTT:debug 2021-06-10 21:44:20: endpoint: undefined


Zigbee2MQTT:debug 2021-06-10 21:44:20: -------HA--------


Zigbee2MQTT:debug 2021-06-10 21:44:20: name: keypad_lockout


Zigbee2MQTT:debug 2021-06-10 21:44:20: endpoint: undefined

For the sake of the length, I have edited the comment and not included all the attributes, but you have to believe me

None of them have an endpoint defined.

But also looking at the code further, in z-h-c/lib/exposes.js
image

Seems like there is a method .withEndpoint, which I am assuming should be passed to provide that endpoint. Searching for it in the devices folder it seems that only e.switch() makes use of it so far.

I am unsure now if I am in uncharted territory or just approaching this wrong at this point....

@Koenkk Could you please have a look at the last few comments, and tell me if I am on the wrong track or not. Thanks

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 10, 2021 via email

@robertalexa
Copy link
Contributor Author

I think that endpoint as-is in homeassistant is only populated if you expose the attribute .withEndpoint in your device file (hive.js)

But yet again, in our case, the endpoint would be "hvacThermostat" (unless i understand it wrong) which needs to be passed the attribute in json format. So... that still wouldn't work anyway?

PS: I am now incredibly close to saying "f*** it" and just handle it in mqtt instead of via HA entity, but I feel like is the lazy approach.

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 10, 2021 •
edited
Loading

Here is a device were non default endpoint should be used:
https://github.com/Koenkk/zigbee-herdsman-converters/blob/1e56d65b44828b0eb434e0e519d37f433c0788ee/devices/ubisys.js#L107

They are also exposed like this:
https://github.com/Koenkk/zigbee-herdsman-converters/blob/1e56d65b44828b0eb434e0e519d37f433c0788ee/devices/ubisys.js#L98

You'd change those using
topic=zigbee2mqtt/name/l1/set value=ON|OFF
topic=zigbee2mqtt/name/l2/set value=ON|OFF

To control either line 1 or line 2.

If you have the single switch version of that:
https://github.com/Koenkk/zigbee-herdsman-converters/blob/1e56d65b44828b0eb434e0e519d37f433c0788ee/devices/ubisys.js#L15

There is no endpoint provided to the expose because you set it using
topic=zigbee2mqtt/name/set value=ON|OFF

So I think it's OK for them to be undefined.

Edit: so for the Hive/Danfoss I would expect
command_topic: true
command_topic_prefix: undefined

@robertalexa
Copy link
Contributor Author

I do not understand what message you are trying to convey with your above message, I have seen that code too.

Command_topic_prefix will be added to the front, it will be postfix that I need.

You can send to topic zigbee2mqtt/0x842e14fffe2ad08a/set/window_open_external with just the value.

The issues with getting the postfix part attached.

I might stop for the night before my eyes fall out of my skull

@robertalexa
Copy link
Contributor Author

It would be property rather than endpoint that would be needed i guess

Zigbee2MQTT:debug 2021-06-10 22:24:44: -------HA--------


Zigbee2MQTT:debug 2021-06-10 22:24:44: name: window_open_external


Zigbee2MQTT:debug 2021-06-10 22:24:44: endpoint: undefined


Zigbee2MQTT:debug 2021-06-10 22:24:44: expose: {"type":"binary","name":"window_open_external","property":"window_open_external","access":7,"value_on":true,"value_off":false,"description":"Set if the window is open or close. This setting will trigger a change in the internal window and heating demand. `false` (windows are closed) or `true` (windows are open)"}

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 10, 2021

After re-reading https://www.home-assistant.io/integrations/switch.mqtt/

command_topic: "zigbee2mqtt/name/set"
or maybe
command_topic: "zigbee2mqtt/name/set/property"

but... why is it only ever set to true?

@robertalexa
Copy link
Contributor Author 

if (expose.access & 0b010) {                                                                                                                                  
                        discoveryEntry = {                                                                                                                                        
                            type: 'switch',                                                                                                                                       
                            object_id: expose.endpoint ? `switch_${expose.name}_${expose.endpoint}` : `switch_${expose.name}`,                                                    
                            discovery_payload: {                                                                                                                                  
                                value_template: `{{ value_json.${expose.property} }}`,                                                                                            
                                payload_on: expose.value_on,                                                                                                                      
                                payload_off: expose.value_off,                                                                                                                    
                                command_topic: true,                                                                                                                              
                                command_topic_prefix: expose.endpoint ? expose.endpoint : undefined,                                                                              
                                command_topic_postfix: expose.property,                                                                                                           
                                ...(lookup[expose.name] || {}),                                                                                                                   
                            },                                                                                                                                                    
                        };                                                                                                                                                        
                    } else {

Yep, postfix fixes the endpoint issue.

Now only final hurdle.

image

The values are actually True and False as strings....

@robertalexa
Copy link
Contributor Author

After re-reading https://www.home-assistant.io/integrations/switch.mqtt/

command_topic: "zigbee2mqtt/name/set"
or maybe
command_topic: "zigbee2mqtt/name/set/property"

but... why is it only ever set to true?

Who is set to true?

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 10, 2021 •
edited
Loading

command_topic in homeassistant.js seems to be set to true but the docs mention it should be the topic to publish too.

But seems you fixed the topic bit by setting postfix, so it's now probably just the payload_on/off t hat is encoded in a way ha doesn't like.

@robertalexa
Copy link
Contributor Author

robertalexa commented Jun 10, 2021 •
edited
Loading

command_topic in homeassistant.js seems to be set to true but the docs mention it should be the topic to publish too.

But seems you fixed the topic bit by setting postfix, so it's now probably just the payload_on/off t hat is encoded in a way ha doesn't like.

Nope
image

LE: as in yes, it is the topic, but it is used as a flag at that point.

@robertalexa
Copy link
Contributor Author 

Zigbee2MQTT:debug 2021-06-10 22:29:55: -------HA--------,
Zigbee2MQTT:debug 2021-06-10 22:29:55: name: keypad_lockout,
Zigbee2MQTT:debug 2021-06-10 22:29:55: endpoint: undefined,
Zigbee2MQTT:debug 2021-06-10 22:29:55: expose: {"type":"binary","name":"keypad_lockout","property":"keypad_lockout","access":7,"value_on":"lock1","value_off":"unlock","description":"Enables/disables physical input on the device"},
Zigbee2MQTT:debug 2021-06-10 22:29:55: -------HA--------,
Zigbee2MQTT:debug 2021-06-10 22:29:55: name: window_open_external,
Zigbee2MQTT:debug 2021-06-10 22:29:55: endpoint: undefined,
Zigbee2MQTT:debug 2021-06-10 22:29:55: expose: {"type":"binary","name":"window_open_external","property":"window_open_external","access":7,"value_on":true,"value_off":false,"description":"Set if the window is open or close. This setting will trigger a change in the internal window and heating demand. `false` (windows are closed) or `true` (windows are open)"},

2 entities as an example, value_on and value_off are correct, all looks good.

When HA sends commands for keypad - lock1 and unlock it works fine.

When HA sends commands for window external - the values are False and True, as strings, which then don't work in the zigbee side of things.

@robertalexa
Copy link
Contributor Author

Mqtt documentation states the following:
image

So having values as boolean in z2m will not play ball with that, cause they will be converted to strings, but when they come back as strings things are not good anymore.

Unless I misunderstand, but I don't think I do.

Would that mean that the values might need to not be booleans in z2m?

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 10, 2021 via email

@robertalexa
Copy link
Contributor Author

I don’t think so, as mqtt doesn’t have the concept of a Boolean it’s just a string. I think it might need a check we’re we check for the type, if string pass it as-is, if Boolean pass it as lower case true or lower case false (probably with double quotes around it) ~ sjorge
…
On 11 Jun 2021, at 00:05, Robert Alexa @.***> wrote:  Mqtt documentation states the following: So having values as boolean in z2m will not play ball with that, cause they will be converted to strings, but when they come back as strings things are not good anymore. Unless I misunderstand, but I don't think I do. Would that mean that the values might need to not be booleans in z2m? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

This is literally what I was thinking about. But where should we check this? Do you mean in toZigbee specific for each device?

For example:
image

This is what I was thinking, but will this be the most appropriate solution? Plus what about all the other devices that might be behaving similarly? Because there will be a silent failure. In my case, HA sends "True", this gets set on the device. The device does not reject the value, the toggle in Frontend changes to false (but basically because it isn't either true or false (bool)). HA will show as switch is on, Frontend will show it as off, and the device will be set to an invalid value...

Thanks

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 10, 2021 •
edited
Loading

No, inside homeassistant.js I’m on my phone so this is hard to write:

payload_on: (typeof expose.value_on != “boolean”) ? expose.value_on : (expose.value_on ? “true” : “false”);

not sure on the typeof, it might be “bool” instead

@robertalexa
Copy link
Contributor Author

robertalexa commented Jun 10, 2021 •
edited
Loading

Given that there are 2 payloads this is what we need.

payload_on: typeof expose.value_on == "boolean" ? "true" : expose.value_on,                   
payload_off: typeof expose.value_off == "boolean" ? "false" : expose.value_off,

Doing this breaks the switches in HA, they are always off now.....

image
image

Setting the toggle to on will correctly send the payload and z2m updates properly, but the HA toggle goes back to off.

Zigbee2MQTT:debug 2021-06-11 00:00:25: Received MQTT message on 'zigbee2mqtt/TRV/set/heat_available' with data 'true'


Zigbee2MQTT:debug 2021-06-11 00:00:25: Publishing 'set' 'heat_available' to 'TRV'


Zigbee2MQTT:info  2021-06-11 00:00:28: MQTT publish: topic 'zigbee2mqtt/TRV', payload '{"algorithm_scale_factor":1,"battery":51,"day_of_week":"thursday","heat_available":true,"heat_required":false,"keypad_lockout":"unlock","linkquality":110,"load_estimate":23,"local_temperature":25.68,"mounted_mode_active":true,"mounted_mode_control":true,"occupied_heating_setpoint":20,"system_mode":"heat","thermostat_vertical_orientation":true,"trigger_time":660,"viewing_direction":null,"window_open_external":false,"window_open_internal":"quarantine"}'

image

image

I will probably sleep on this as it is super late already... but damn it is so annoying

@robertalexa
Copy link
Contributor Author

hey @sjorge

I have figured it out.

This is the current state of the code:
image

Here is how I judged this:
image
https://www.home-assistant.io/integrations/switch.mqtt/

I have tested both boolean and string values (definition in z2m) and both z2m and ha works correctly and follow each other with the above code.

I am wondering if for the sake of saving confusion later I should set state_on/off to be the same as payload_on/off, as in t verify boolean. Purely so the next person looking at it doesn't think it is retarded. Or maybe I will just add a comment to the code pointing to this issue, and this will clarify for future use.

Do you have any remarks before I push this as a PR?

PS: thank you kindly for your input throughout this, couldn't have done it without bouncing ideas with you.

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 11, 2021

Do we still need the typeof stuff with state_on and state_off ?

If we can just have both state_on / payload_on just set to expose.value_on it would remove most confusion I think?

@robertalexa
Copy link
Contributor Author

robertalexa commented Jun 11, 2021 •
edited
Loading

Already tried, and no, the value passed to z2m is (string) "True" which fails silently. It is invalid but the device will not reject with error message.

LE: scrap the rest of the message, I am an idiot.

In short, we still need to check for bool in order to have valid and correctly translated data

@sjorge
Copy link
Sponsor Contributor

sjorge commented Jun 11, 2021

Just a comment is probably fine then.

@robertalexa robertalexa mentioned this issue Jun 11, 2021
Merged
@robertalexa
Copy link
Contributor Author

I have submitted a PR. As a result, I will close this issue and we can continue the conversation there if we need to.

@robertalexa robertalexa mentioned this issue Jun 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
problem Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@sjorge @robertalexa