cgarwood / homeassistant-zwave_mqtt

Limited Pre-Release of the new OZW1.6 Z-Wave component. Currently has limited platform support. Check the README for more details.
72 stars 8 forks source link

Automatic creation of binary sensors from Notification command class #69

Closed kpine closed 4 years ago

kpine commented 4 years ago

I mainly only have one gripe with the OZW 1.4 implementation of Z-Wave in HA, which is that for simple devices like door/window or motion sensors, etc., all HA users are required to create their own binary template sensors, for example. It's a necessity because OZW 1.4 doesn't provide any mechanism to know which events are supported.

Now with OZW 1.6, it provides values and labels for the supported events (I'm not actually sure how it creates them, some devices support getting the supported values, for others maybe it creates the values on the fly when it sees them?). It seems entirely possible that from the set of notification values, HA could create a binary sensor for each. Here is sample mqtt data for some of my nodes:

{
    "Label": "Access Control",
    "Value": {
        "List": [
            {
                "Value": 0,
                "Label": "Clear"
            },
            {
                "Value": 22,
                "Label": "Door/Window Open"
            },
            {
                "Value": 23,
                "Label": "Door/Window Closed"
            }
        ],
        "Selected": "Door/Window Closed",
        "Selected_id": 23
    },
...
}
{
    "Label": "Home Security",
    "Value": {
        "List": [
            {
                "Value": 0,
                "Label": "Clear"
            },
            {
                "Value": 3,
                "Label": "Tampering -  Cover Removed"
            },
            {
                "Value": 8,
                "Label": "Motion Detected at Unknown Location"
            }
        ],
        "Selected": "Clear",
        "Selected_id": 0
    },
...
}

In this integration, and with the native 1.4 integration, all I get is the "home_security" and "access_control" entities with the raw numbers. These sensors are generally confusing to new users.

Instead, it seems like now it would be possible (?) to create a binary sensor for each Value in the List, which would be "on" when it is selected. The downside would be the number of new entities created (there are already a lot, and N-times more in case of multi-channel devices). There would be some entities that perform the same function, such as the "Door/Window Closed" and "Door/Window Opened".

It's very easy to disable entities with the entity registry now though, and it would no longer be necessary to create manual template sensors.

marcelveldt commented 4 years ago

Great idea. This is indeed something that can be approved over the current integration.

cgarwood commented 4 years ago

In the case of access_control we could probably add code to combine the 22 and 23 values into a single binary_sensor

I think the home_security CC makes sense to have separate binary sensors for each value.

Could we ignore the "clear" value on both as clear should mean all the binary sensors from that CC are off?

marcelveldt commented 4 years ago

Shouldn't we treat the Notification CommandClass as events only ? Motion/door sensors etc will have their own binary sensor already through binary command class.

Schermafbeelding 2020-04-21 om 08 30 19
kpine commented 4 years ago

Notifications are not only events, they are also states. See SDS13713, which defines all the notification types and values, and also defines state variables. See also SDS13781 Section "4.74.1 Terminology". I'm not an expert, but here is my understanding below. Perhaps someone can correct anything I've gotten wrong.

The state variable "Door state" is represented by values 22 and 23 for the access control notification type. It does not have an idle value, it's always either 22 or 23.

Some state variables are represented with the same notification type. For example, "Cover status" (tamper, value 3), "Motion sensor status" (value 8) are separate variables but both are set by the home security notification type. The "Clear" value (idle) indicates when those states have transitioned back. Depending on the version of the CC class, the Clear value will or will not indicate which state variable is going idle. I couldn't tell if OZW supports that behavior, or if it's just a complete Clear for all. I think OZW also does auto-clearing of these values if the CC versions don't support idle, which sounds to have caused some problems. This change may be a surprise to HA users as well.

It would be quite the departure to represent all notifications as events instead of entities.

CC Binary is deprecated and only supported by very old devices. Some of the Binary sensors you see are either a ZWave FW bug or OZW bug as they are not supported by the device and are non-functional. I have several Monoprice door sensors (rebadged Vision) and none of the technical docs shows support for CC Binary, only CC Notification, yet for some reason I'm given a binary sensor.

A twist with OZW 1.6 compared to 1.4 is that V3 and up notification class is supported, which means some of the notifications have event data attached to them, for example the door open notification might tell you if the door is in a tilt position instead of fully open.

Fishwaldo commented 4 years ago

As @kpine mentioned - Most newer devices are "migrating" to the Notification CC to communicate both state and events, and command classes like Binary Sensor etc have/are being depreciated.

The Challenge - The Notification Class is quite complicated and at least in OZW 1.6 - Dynamically updateable (via a config file).

For Information: 1) Each "ValueID" Index represents a different "type" of notification - You can map the index to different functions in HASS. 2) Each ValueID is a List, and for newer devices, only exposes "events" or "States" that are valid for that device. (eg, if a Smoke Detector doesn't support a test mode, you wont see it in the states) 3) Older devices that don't support full discovery will expose all possible options but obviously only a few of them are triggered. 4) All ValueID's support a "Clear" option. For newer devices, the device will automatically set the ValueID to Clear when the event timesout, or the state is no longer valid. 4a) Older Devices wont send a clear value, but OZW will automatically "timeout" the value and reset it to clear after a configurable amount of time

This in effect turns everything into a "State" rather than a "Event"

The list of current ValueID's and possible options are configured in this file

I would take the different "AlarmType" elements and map them to things like motion, smoke etc in HASS.

The "AlarmEvent" is all possible variations of that State (its not just Binary).

The "AlarmEventParam" is a optional additional data transmitted by the device that includes some ancilary data that might be interesting. The ID field indicates the "index" that parameter will appear on. The type of param (Boolean, Integer etc) is unique for each index (so index 265 is always going to be a byte value - but depending upon the event that triggered it, might contain different data unique to the ValueID Index that changed. - eg, Index 256 is the "Previous Event Cleared" - But it relates the the AlarmEvent that just changed most recently back to "Clear")

When a "Event" occurs that has a ancillary parameter, OZW will create the ValueID (you will get a ValueAdded Notification) - Populate the Value with the data (ValueChanged Notification), and later when the event goes out of scope, delete the Value (ValueDeleted Notification). This means they are created and destroyed as needed.

A common request from users that demonstrates this - A Lock with a Keypad. Newer devices should expose a ValueID with index 6 (Access Control). When the lock is operated via a code, then it will change to "Keypad Lock Operation" (id 5) and a new ValueID will be created with Index 260 that contains the UserCode that unlocked the lock. When the lock returns to the "Clear" state, the ValueID at 260 will be deleted. Not all devices will send Ancilary data (even if the state supports it) so if we don't receive any additional data then these params will not be exposed.

My Recommendation - consider a "config" file to do the mapping between the Notification CC ValueID Indexes and your internal representations - As OZW can update different types of Notification Events via a Config file (without a recompiling etc, just drop a new version in the config folder) you may want to enable a way to easily update your mapping (or allow users to "customize" the mapping some way - Its possible some vendors "abuse" the AlarmType to represent something else).

As far as the individual states, how granular you go, obviously is completely upto you - but be aware of "gotchas" like the Smoke Sensor having states that do not indicate actual smoke detected (some states are like Replacement Required etc) - So mapping anything other than "Clear" might cause "false" alerts etc. Its not just a case of "True/False"

For backwards compatibility, you should still handle BinarySensor CC Values etc. - The question I can't answer though, is for a device that has both BinarySensor and Notification CC sending events, how you handle that in HASS (essentially getting two events)

Finally, some devices will still expose the old "Alarm_Level" and "Alarm_Type" values - these are from version 1 of the CC specification, and are not standardized at all. The values they contain are completely arbitrary and decided by the individual vendors. (no standardization at all - hence the are mostly depreciated - but some vendors still use them on occasions)

marcelveldt commented 4 years ago

Thanks for the detailed responses @kpine and @Fishwaldo We're currently looking at the options of how to deal with this and translate it as user-friendly as possible to Hass.

marcelveldt commented 4 years ago

@kpine would it be possible for you to give this branch a quick testdrive ? https://github.com/marcelveldt/homeassistant-zwave_mqtt/tree/userfriendly-sensors

Just manually update the files. Please note: breaking change so your entities will probably be messed up.

The changes in this PR will basically do what you requested, create (binary) sensors for most important vaues and some more generic sensors for the more advanced sensors. Also it will disable the very advancer sensors stuff by default.

kpine commented 4 years ago

Sure, I can give it a try. I'll report back later.

marcelveldt commented 4 years ago

This has been merged. We want to get some user feedback as entities are now being generated in a more readable/userfriendly way.

@kpine I'll close this issue for now. Feel free to report your findings by opening a new issue or re-open this one! I'm very curious if you like the changes

kpine commented 4 years ago

Just a few comments. For my door sensor, here's what I get:

image

The open/close sensor is working fine, I can only nitpick on the name, Instead of door_window_open I'd probably just go either with the very simple binary_sensor.<device_name>_door, or a very predictable name that follows a specific format:

image

The spec already defines variable names, which can be unfriendly though (see "Barrier performing initialization process status"!). Something like binary_sensor.<device_name>_<notification_type>_<state_variable> would be very predictable, although at times a handful (in this case, it would be binary_sensor.<device_name>_access_control_door_state).

I just wonder how you will decide the entitity names for variables supported in the future.

The home security sensor has some issues. My door sensor only supports this tamper value,

image

but I don't get a tamper binary sensor. It looks like you didn't add a case for that notification value, so I that's to be expected. However, I get this weird "home_security" binary sensor instead. It looks like you are creating this as a fallback. I don't think that should be the case, if a binary value is not support just leave it up to the raw sensor value instead, like has been done. Since the notification value could cover multiple variables or more than one state, it doesn't really make sense to make it a binary sensor.

For the cover tamper based on the naming scheme above it I would be something like binary_sensor.<device_name>_home_security_cover_status, which is On when the value is 3 and Off otherwise. Maybe binary_sensor.<device_name>_tamper or _cover_removed are better.

Similar problems with a 4-in-1 motion sensor. The 4-in-1 supports both motion and cover status values for home security. image (Included the non-supported value 7 for completeness).

The motion binary sensor was created correctly, named binary_sensor.<device_name>_motion_detected, and the raw home_security sensor is there but hidden. OK, that's fine. No weird binary "home_security" sensor, which is good.

I don't think the behavior of the state changes is correct though. Consider this sequence:

  1. Motion is detected (OZW sends value 8), binary sensor goes on.
  2. While the motion sensor is still active, the cover is quickly removed (OZW sends value 3).
  3. The motion binary sensor turns Off, even though no clear event has been sent. The raw home_security sensor reports the last value 3.
  4. Put the cover back on, binary motion is still off, raw value goes to 0. (OZW sends value 0 "Clear").

In this case, I don't believe HA should turn the motion sensor off at step 3. It should only do so once OZW has sent the Clear value. The motion detector itself does not change the value while the cover is off. It should be assumed on until told otherwise, at least as I understand it.

marcelveldt commented 4 years ago

@kpine thanks for the feedback!

1) The entity default name is generated like this: device_name_sensor_name. In your case the device name is "door/window sensor" so that explains the somewhat strange name. To not overly complex the code I'd say to leave this as-is now. You can easily edit the entity names to your own preference. The big win here is that you get binary sensors and you do not have to fiddle with the raw values anymore.

2) At this time, we only add some code in for the door/window sensor and the motion sensor based on Notification CC. All other will fallback to the more generic sensors. I will add one more for the tamper protection though.

3) The weird binary sensor sensor is indeed an experiment from me and @cgarwood where we want to get some feedback for. Before there was only the normal "sensor" entity, containing a raw int value for the specific Value in the notification CC but there was not userfriendly. We still add these sensors for backwards compatibility reasons but in a disabled-by-default state. As an additional solution we introduced the binary sensor that "something is going on" with that specific Notification Class with an attribute of the label. Maybe we should not create the binary sensor and instead do our best to create more value-specific binary sensors like we did with the door/window and motion. We'll discuss this a bit more, thanks for the feedback.

4) About the state change of the motion sensor. Valid point but needs a bit of investigation as we currently only do a strict compare against the value on --> value == motion_detected and we do not track the state. I'll take a look if that can be improved.

marcelveldt commented 4 years ago

See #108

kpine commented 4 years ago

Great! I'll take a look.

marcelveldt commented 4 years ago

Closed by #108