zwave-js / zwave-js-ui

Full featured Z-Wave Control Panel UI and MQTT gateway. Built using Nodejs, and Vue/Vuetify
https://zwave-js.github.io/zwave-js-ui
MIT License
942 stars 202 forks source link

[bug] GE dimmers/switches do not reflect double taps and take the device dim out of sync in HA #116

Closed blhoward2 closed 3 years ago

blhoward2 commented 3 years ago

Version

Build/Run method

zwavejs2mqtt version: 1.2.3

Describe the bug

GE dimmer/switches are capable of double tap events. In zwave2mqtt these show as events under the node, with 255 indicating double tap up and 0 indicating double tap down. (My ozw device files were set to ignore the command mapping for class 32 from qt-openzwave and I carried this over when I switched so I don't know if it was necessary.

These are the steps to make it work under qt-openzwave: https://community.home-assistant.io/t/howto-add-double-tap-support-for-ge-switches-and-dimmers-in-ozw-beta/228871/16.

The original qt-openzwave issue which has more detailed info is at: https://github.com/OpenZWave/qt-openzwave/issues/60.

Double tap up in zwavejs2mqtt now shows:

zwavejs2mqtt    | 2020-12-28 23:32:24.898 INFO ZWAVE: Node 3: value updated: 38-0-currentValue 50 => 255

(The current value goes to 255 and HA thinks the device is now at 100%, but the physical dim has not changed. They are now out of sync.)

Both regular down and double-tap down show 0 so I don't see a way to differentiate those:

zwavejs2mqtt | 2020-12-28 23:21:25.052 INFO ZWAVE: Node 3: value updated: 38-0-currentValue 50 => 0

To Reproduce Steps to reproduce the behavior:

  1. Double click up, or double click down

Expected behavior Double tap up should generate an mqtt event or message that can be acted upon, if not a sensor within HA. In zwave2mqtt I just subscribed to the topic in Appdaemon.

Additional context Add any other context about the problem here.

blhoward2 commented 3 years ago

Note that these are older zwave plus models of the GE14294, not the newer models with Central Scene states. Double tap has successfully worked under 1.4 and 1.6 though.

On a related note, the association groups show: On/Off control On/Off control Lifeline

These should say Group 1, 2, 3, as it is necessary to have the controller in Group 3 for double tap. Mine is, but is is possible to set the equivalent of auto="true" so that the controller is auto-added to Group 3?

blhoward2 commented 3 years ago

More info from logs.

Double tap up:


04:43:19.204 DRIVER « [Node 003] [REQ] [ApplicationCommand]
                      └─[BasicCCSet]
                          target value: 255
04:43:19.209 CNTRLR   [Node 003] treating BasicCC Set as a report
04:43:19.210 CNTRLR   [Node 003] [~] [Multilevel Switch] currentValue: 50 => 255        [Endpoint 0]
04:43:19.285 SERIAL « 0x01080004006702820115                                              (10 bytes)
04:43:19.286 DRIVER   Dropping message because it could not be deserialized: The command class Hail 
                      is not implemented
04:43:19.287 SERIAL » [ACK]                                                                   (0x06)

Double tap down:

04:43:59.988 DRIVER « [Node 003] [REQ] [ApplicationCommand]
                      └─[BasicCCSet]
                          target value: 0
04:43:59.991 CNTRLR   [Node 003] treating BasicCC Set as a report
04:43:59.992 CNTRLR   [Node 003] [~] [Multilevel Switch] currentValue: 255 => 0         [Endpoint 0]
04:44:03.026 SERIAL « 0x01080004006702820115                                              (10 bytes)
04:44:03.027 DRIVER   Dropping message because it could not be deserialized: The command class Hail 
                      is not implemented
04:44:03.028 SERIAL » [ACK]                  

Here is a single tap down, which also results in a value of 0:

04:46:52.879 DRIVER « [Node 003] [REQ] [ApplicationUpdateRequest]
                        payload: 0x8403110411015e5686725a8559732627702c2b7a
04:46:52.882 CNTRLR « [Node 003] Received updated node info
04:46:52.910 SERIAL « 0x01090004000303260300d7                                            (11 bytes)
04:46:52.914 CNTRLR   [Node 003] [~] [Multilevel Switch] currentValue: 0 => 0           [Endpoint 0]
04:46:52.918 SERIAL » [ACK]                                                                   (0x06)
04:46:52.925 DRIVER « [Node 003] [REQ] [ApplicationCommand]
                      └─[MultilevelSwitchCCReport]
                          current value: 0
robertsLando commented 3 years ago

@blhoward2 Could you also show me the console logs of zjs2m?

blhoward2 commented 3 years ago

I can confirm that the same issue persists on the most recent dev image, pulled today. It doesn't know what to do with the firmware generate double-up or double-down so it treats it like a normal report. The double-tap up results in the light and HA going out of sync, with my light at 50% and HA thinking it is maxed out based on the 255 value.

In ozw 1.4 these were treated as zwave events and you can act upon them in an automation. In 1.6 it became it more difficult but you could ignore the mapping per the link i posted above and get a basic set report on them. Isn't that what is happening here? is there a way to unmap the BasicCC from the report and receive the BasicCC?

Note: I have a script that resets the light to the default levels upon turning on. That accounts for the 50 --> 50 messages.

Double-tap up (from a prior double-tap up state, hence 255-->255):

Screen Shot 2021-01-02 at 10 13 38 PM

Double-tap up from off, just in case being in a former double-tap state affects it (note this one is out of order on the console log):

Screen Shot 2021-01-02 at 10 16 16 PM

Double-tap down:

Screen Shot 2021-01-02 at 10 14 02 PM

For comparison, off to normal on:

Screen Shot 2021-01-02 at 10 15 11 PM

For comparison, on to normal off:

Screen Shot 2021-01-02 at 10 15 43 PM

Console log:

Screen Shot 2021-01-02 at 10 17 08 PM

blhoward2 commented 3 years ago

I opened an upstream bug as I believe this results from this: https://github.com/zwave-js/node-zwave-js/issues/388

towerhand commented 3 years ago

I think this works in out of the box in zwave2mqtt without the user having to make any special changes unlike in qtozw.

blhoward2 commented 3 years ago

I’m working with AlCalzone in the upstream bug report. He’s already committed a change to try to provide a compat flag to fix this. The devices are non-compliant with the standard and when the standard itself became publicly available the projects made changes to how things worked to comport with the standard. Hence why it works in the older projects (both of which are based on ozw 1.4) and not the newer ones (ozw 1.6 and node-js-zwave).

I’ve left this bug report open as I’m unsure of whether downstream changes will also be required.

towerhand commented 3 years ago

Thanks, I’m keeping my eye on this. I’ll switch over from z2m once this issue is resolved.

blhoward2 commented 3 years ago

For now I’ve just removed the controller from group 3 and am relying solely on direct association and those devices calling in their state change. It mostly works fine that way except for a few odd devices I use to refresh after a double-tap event for reliability.

robertsLando commented 3 years ago

@AlCalzone

AlCalzone commented 3 years ago

@robertsLando We're already on it and need your help: https://github.com/zwave-js/node-zwave-js/issues/1277

robertsLando commented 3 years ago

@AlCalzone Sorry, catching up missed notifications :smile:

blhoward2 commented 3 years ago

Ok, the changes made upstream now work (I'll do a PR to add the compat flag to the device files.) I'm receiving the basic set events on command class 32 for double-up and double-down. I tried refreshing the node info but no sensors are added within the ui.

@robertsLando Is it possible to define a sensor or something so HA more readily receives these events? Is there an automated way to do this that won't mess up other devices that need the basic cc? Or at this point would users need to define a custom sensor?

towerhand commented 3 years ago

@blhoward2 Thanks for the heads up. I'll be installing zj2m in the next release if this is working properly. Hopefully the basic cc doesn't get activated during HA restart like Kpine mentioned in the other issue. Glad to see bug getting addressed so quickly, main reason I kept using z2m after a short trial with qtozw.

blhoward2 commented 3 years ago

PR for the final change upstream submitted (modifying the device files to take advantage of the new compat flag). https://github.com/zwave-js/node-zwave-js/pull/1303

towerhand commented 3 years ago

Would this be available if I install zj2m wiith docker using the dev tag?

blhoward2 commented 3 years ago

Not yet, no. @AlCalzone can correct me if I’m wrong but I believe the patch to node-zwave-js is built on top of zwave 6.0, which was incorporated by the pending #84 PR. There is also a second bug causing two sets of device files that needs to be rectified.

towerhand commented 3 years ago

Thanks @blhoward2, I'll wait until that issue is resolved before jumping in the testing wagon.

AlCalzone commented 3 years ago

@all you could try if the double config issue is gone with https://github.com/zwave-js/zwavejs2mqtt/pull/181. I must admit I have no clue how to run it with Docker for Windows.

blhoward2 commented 3 years ago

I’ll do so tonight.

robertsLando commented 3 years ago

I think this has been opened in node-zwave-js. Closing this here

blhoward2 commented 3 years ago

Sort of. The node fix causes the values to be sent. We still do nothing with setting up devices in HA for those, which is why I opened this one. I haven’t pushed this forward until it’s settled event vs value update as being discussed in the node one.

towerhand commented 3 years ago

taking back what i just wrote, I do see this in the handle value notification function, which appears to be making an event... is that in MQTT?

If you enable the flag publiush events in gateway all zwavejs events are published to mqtt: https://zwave-js.github.io/zwavejs2mqtt/#/guide/mqtt?id=zwave-events

Yeah, I did that intentionally, but HA doesn’t see those and it would be difficult to define a new sensor under the device watching all events.

We need a value under the node that works like the scene one now does where it resets to 0. We tried to figure out last night why we aren’t getting a 32 command class under the node at all now (we did when sending value events), and I posted in the PR itself.

Moving to this issue since is probably more relevant here.

I installed zjs2m this morning to test and see how things are evolving and try to provide info from a regular user point of view.

I can see the events getting triggered with the console and with mqtt explorer in the event topic in the root topic per the docs (/EVENTS/ZWAVE_GATEWAY-). In the old z2m it would publish them in the node topic as well and made everything a lot easier for regular user like me (/node_id/event) to set up automations to watch the event for an specific device.

blhoward2 commented 3 years ago

This belongs in the other issue. That’s what we’re working on there, at the node level. Presently the node isn’t registering the value so it isn’t showing in MQTT.

blhoward2 commented 3 years ago

@robertsLando We now have this sorted at the node level, and it works perfectly when I define a manual sensor. How do I go about defining a sensor so it is automatically created (obviously it could be called basic or something instead of double_tap so it is more generic? Where does that live in the code? Can it be done when it first gets a double_tap event (when the mqtt topic is created)? Or would a user now need to custom add this because there isn't a way to prevent it being doing on all switches (so it can't be added to the code base)?

(Below updated to reflect the /value to /event change.)

{
  "type": "sensor",
  "object_id": "double_tap",
  "discovery_payload": {
    "state_topic": "homeassistant/3/32/0/event",
    "value_template": "{{ value_json.value}}",
    "json_attributes_topic": "homeassistant/3/32/0/event",
    "device": {
      "identifiers": [
        "zwavejs2mqtt_0xeb87ad4a_node3"
      ],
      "manufacturer": "Jasco Products",
      "model": "In-Wall Dimmer Switch (GE 14294 (ZW3005))",
      "name": "nodeID_3",
      "sw_version": "5.29"
    },
    "name": "nodeID_3_double_tap",
    "unique_id": "zwavejs2mqtt_0xeb87ad4a_3-32-0-double_tap"
  },
  "discoveryTopic": "sensor/nodeID_3/double_tap/config",
  "values": [
    "32-0-event"
  ],
  "persistent": false,
  "ignoreDiscovery": false,
  "id": "sensor_double_tap"
}

We also get an undefined message in the logging:

==> zwavejs2mqtt.log <==
2021-01-14 17:17:00.907 INFO ZWAVE: Node 3: value notification: 32-0-value undefined => 255
AlCalzone commented 3 years ago

heads up: I have renamed the /value to /event to avoid confusion with target and currentValue.

blhoward2 commented 3 years ago

heads up: I'm going to rename the /value to /event to avoid confusion with target and currentValue.

Makes sense to me

blhoward2 commented 3 years ago

heads up: I have renamed the /value to /event to avoid confusion with target and currentValue.

Do my changes above capture the change, or is it now 32-0-event too?

AlCalzone commented 3 years ago

It is now 32-0-event

robertsLando commented 3 years ago

Can we close this so?

blhoward2 commented 3 years ago

Can we close this so?

I posted two questions for you a few messages up. Can we auto-create a sensor for this or would a user have to define their own? And there is still a bug with the log that causes it to say undefined.

robertsLando commented 3 years ago

Sorry missed them, too much notifications :cry:

We also get an undefined message in the logging:

undefined is the previous value, for a notification it is undefined, if you want I could remove the undefined from the log but it's not wrong as it says: I was undefined and now I'm 255

Can we auto-create a sensor for this or would a user have to define their own?

Absolutely, to cleanup some stuff would you open a dedicated issue? Like [feat] create HASS sensor for CC 32 event

AlCalzone commented 3 years ago

In the context of value notifications (stateless), displaying a transition from a previous value makes no sense IMO.

robertsLando commented 3 years ago

Ok will remove it so, no problem

blhoward2 commented 3 years ago

Absolutely, to cleanup some stuff would you open a dedicated issue? Like [feat] create HASS sensor for CC 32 event

Done

robertsLando commented 3 years ago

The log is fixed on master closing this in favor of #249