home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
72.81k stars 30.51k forks source link

GE Zwave Dimmer (14294) - Toggling On Results in Odd UI Behavior #47388

Closed bholdmeyer closed 3 years ago

bholdmeyer commented 3 years ago

The problem

When toggling on a light controlled by a GE 14294 Dimmer Switch, the switch in the UI moves to the "on" position for approximately 2 seconds then moves back to the "off" position for approximately 4 seconds and then moves back again to the "on" position and now stays in that position. The light itself turned on with the original click to the switch in the UI, and the switching back and forth happening in the UI did not affect the state of the light. This behavior does not happen when turning off the light from the UI, the switch goes to the off position and stays there and the light turns off.

It reminds me of the behavior that I used to experience with these switches on the original Z-Wave integration when turning the light off, but that was I believe due to the dimmer slowing dimming the light and trying to read the state before it had dimmed all the way down to off. The workaround that I was using in the old integration was to just increase the dimming steps, so that it dimmed faster and was able to get to the off level before the UI checked the state again. I have tried this method for the current issue and even raised the dim steps and dim rate up to the maximum/fastest values, but the issue in the UI for toggling on still persists.

I do not believe this was happening with the original release of the Z-Wave JS integration/server.

One other thing I noticed if I activate the debug log in the zwavejs2mqtt addon and I toggle one of the dimmers on, the log does not show value updated for the node from 0=>99 until the strange toggling behavior has finished moving back and forth and settled on the "on" position. The log immediately shows the updated value for the node from 99=>0 when turning the switch off.

What is version of Home Assistant Core has the issue?

2021.3.0

What was the last working version of Home Assistant Core?

2021.2.0

What type of installation are you running?

Home Assistant OS

Integration causing the issue

Z-Wave JS

Link to integration documentation on our website

https://www.home-assistant.io/integrations/zwave_js

Example YAML snippet

# Put your YAML below this line

Anything in the logs that might be useful for us?

# Put your logs below this line
probot-home-assistant[bot] commented 3 years ago

Hey there @home-assistant/z-wave, mind taking a look at this issue as its been labeled with an integration (zwave_js) you are listed as a codeowner for? Thanks! (message by CodeOwnersMention)

kpine commented 3 years ago

When you turn on a light in zwave-js (that does not support supervision), it refreshes the state after 5 seconds. The state of the switch is unknown when you toggle the switch from off to on, thus the actual state is not known until 5 seconds later. You can avoid the "UI toggle" by setting the brightness manually, because zwave-js itself provides an optimistic value. If for some reason the value does not get accepted by the device, the value will still jump 5 seconds later. This 5-second delay refresh in zwave-js may change in the future.

To actually solve this will likely require adding in special logic to the zwave-js light platform to optimistically modify the state in HA to some temporary value. The question is what would that value be? 100%? 50%? A cached last known value before the switch was off (which may be incorrect in some cases)?

PrplHaz4 commented 3 years ago

To actually solve this will likely require adding in special logic to the zwave-js light platform to optimistically modify the state in HA to some temporary value. The question is what would that value be? 100%? 50%? A cached last known value before the switch was off (which may be incorrect in some cases)?

I appreciate the analysis - since I switched, I've been having a tough time reasoning around why these specific dimmers seemed to be a lot more predictable with SmarthThings than with Home Assistant.

RE: the cached last-known value: shouldn't that be the target value instead? Optimistically assuming the dimmer reached the value it was set to? I think OZW was attempting something like that but never got around to trying it.

kpine commented 3 years ago

There is no target value when you toggle the switch on, that's the problem. In other words, when you say "toggle on", it simply tells the switch to turn on to the last known state. zwave-js is not tracking historical values so it cannot optimistically set some value back to the application.

When you turn on the switch with a brightness value, zwave-js knows the target and simply tells the application that's the new value, refreshing it with the actual value (hopefully the same) 5 seconds later.

The above part about the 5-second refresh only applies to switches that don't support the Supervision CC, which includes older versions of these GE switches.

PrplHaz4 commented 3 years ago

There is no target value when you toggle the switch on, that's the problem. In other words, when you say "toggle on", it simply tells the switch to turn on to the last known state. zwave-js is not tracking historical values so it cannot optimistically set some value back to the application.

When you turn on the switch with a brightness value, zwave-js knows the target and simply tells the application that's the new value, refreshing it with the actual value (hopefully the same) 5 seconds later.

The above part about the 5-second refresh only applies to switches that don't support the Supervision CC, which includes older versions of these GE switches.

I definitely can see cases where I'd want the "default" behavior to vary too. I'm less likely to mind my hall lights coming on at 75% every time it's toggled, but my bedroom I'd prefer they return to the previous value.

For "older versions" do you mean firmware versions or model numbers? Most of mine are 14294 v5.26 which appears to be on the early side...

It appears most of the ST handlers (similarly) poll the value after the dimming duration and hope it's right I guess... ref: https://github.com/mwav3/smartthingscode/blob/master/devicetypes/mwav3/ge-jasco-zwave-plus-dimmer-switch.src/ge-jasco-zwave-plus-dimmer-switch.groovy

def on() {
    def cmds = []
    cmds << zwave.basicV1.basicSet(value: 0xFF).format()
    cmds << zwave.switchMultilevelV2.switchMultilevelGet().format()
    def numberofzsteps = (100 / (device.currentValue("zwaveSteps")))
    def calcdelay = (numberofzsteps * (10*(device.currentValue("zwaveDelay")))).longValue()
    def delay = Math.max(Math.min(calcdelay, 10000), 1000)
    delayBetween(cmds, delay)
}
NYZack commented 3 years ago

I'm able to mitigate this problem completely with an automation that immediately refreshes the light entity’s value in zwave-js as soon as it is turned on, using the service zwave_js.refresh_value in Home Assistant. Is there any down-side to making this the default behavior in HA? Zwave-js could still refresh after 5 seconds to return the final value.

Perhaps this generates unnecessary network traffic, but would it really be that demanding?

marcelveldt commented 3 years ago

Yes it is really demanding and bad for your network. Most issues with Z-wave are because of too much traffic so unneeded polling should be avoided at all times. My best advice would be to just use devices that report that status automatically (don't need polling). For cases where the light needs to be polled, Z-Wave JS will automatically refresh the value after 5 seconds, this may be improved though over time.

Your current solution of doing a one-time poll after turn-on is a correct one. We will not do it by default as there is no way to tell which dimmers need this "hack" so we leave it up the user now.

PrplHaz4 commented 3 years ago

Your current solution of doing a one-time poll after turn-on is a correct one. We will not do it by default as there is no way to tell which dimmers need this "hack" so we leave it up the user now.

Thanks @marcelveldt!

Per @kpine (again) Z-wave JS should currently be doing this refresh automatically after 5s

kpine commented 3 years ago

I'm able to mitigate this problem completely with an automation that immediately refreshes the light entity’s value in zwave-js as soon as it is turned on, using the service zwave_js.refresh_value in Home Assistant. Is there any down-side to making this the default behavior in HA? Zwave-js could still refresh after 5 seconds to return the final value.

Why do you care if the UI and entity state refresh immediately or 5 seconds later? Is the time that critical? You are eventually getting the correct value after the ~5 seconds, right?

Perhaps this generates unnecessary network traffic, but would it really be that demanding?

Yes, it is quite demanding as Marcel as outlined. Adding a default refresh would not work for all switches, because it would capture the transitional state (which may be still be OFF, see bad OZW behavior) and would only double the number of refreshes that already exist. Furthermore, if the device supports Supervision you can avoid the refresh completely.

My best advice would be to just use devices that report that status automatically (don't need polling).

Unfortunately this isn't necessarily a fool-proof solution. If the transition time of the switch is longer than the UI refresh timeout, then it doesn't matter whether a device reports updates or not. Besides, zwave-js is already doing the update, it's just 5 seconds later, for non-supervised commands. For supervised commands, you get an update when the command finishes.

NYZack commented 3 years ago

Why do you care if the UI and entity state refresh immediately or 5 seconds later? Is the time that critical? You are eventually getting the correct value after the ~5 seconds, right?

It is problematic; I'm not sure how to explain it, but turning a switch on in a UI and then seeing it immediately turn off isn't ideal behavior. When you turn on a light, you don't expect to have to watch the UI for 5 seconds afterward to verify that it "took". In addition, it causes Echo devices (for instance) to report that the light is disconnected every time you use Echo to turn the light on.

Yes, it is quite demanding as Marcel as outlined. Adding a default refresh would not work for all switches, because it would capture the transitional state (which may be still be OFF, see bad OZW behavior) and would only double the number of refreshes that already exist. Furthermore, if the device supports Supervision you can avoid the refresh completely.

I'm certainly no expert on this, but one additional polling request only when Z-wave devices (maybe just Z-wave dimmers) are turned on doesn't seem like that much traffic. A transitional state seems more likely to be accurate than reflecting "off" in all cases for 5 seconds (unless the device doesn't need polling). Expecting users to replace their dimmers or proactively know to get devices that support Supervision is expecting a lot, I think. My question is whether the vast majority of users wouldn't find a single automatic poll more beneficial than the current behavior. Maybe the integration could have an option to enable or disable a routine extra polling request.

blhoward2 commented 3 years ago

Why do you care if the UI and entity state refresh immediately or 5 seconds later? Is the time that critical? You are eventually getting the correct value after the ~5 seconds, right?

I agree it is problematic. It isn’t the brightness that is an issue its the toggle going back off. When this was happening when flipping off it made my wall mounted dashboards go completely bonkers and it was hard to tell if you’d missed hitting the switch or not. It also drove my wife crazy and the WAF went WAY down. She’d hit it again thinking something was wrong which would then turn it off. More importantly it makes the entire UI feel flaky because it just isn’t how a UI should react. It defies all user expectations. Forget about explaining it when family members were visiting or my kids.

We fixed this when turning off at the node level by assuming the state was what you set it to for a few seconds and then correcting it if the current value didn’t come in time.

NYZack commented 3 years ago

We fixed this when turning off at the node level by assuming the state was what you set it to for a few seconds and then correcting it if the current value didn’t come in time.

I used to have the problem for turning dimmers both on and off with the old Z-wave integration. With zwave-js, it's not a problem when turning them off anymore, with no hacks.

blhoward2 commented 3 years ago

We fixed this when turning off at the node level by assuming the state was what you set it to for a few seconds and then correcting it if the current value didn’t come in time.

I used to have the problem for turning dimmers both on and off with the old Z-wave integration. With zwave-js, it's not a problem when turning them off anymore, with no hacks.

It’s hacked. It’s just invisible to you. We fixed it on our side.

NYZack commented 3 years ago

By the way, this is a problem for all four kinds of dimmers I own - GE Jasco 14294, Inovelli LZW31-SN, Honeywell 39339, and Leviton DZPD3. Maybe I've been very unlucky in the dimmers I've chosen to buy, but this doesn't seem to be an isolated issue.

marcelveldt commented 3 years ago

This isn't as simple as you might think. Some dimmers will even return a completely false value when you refresh it right after turn on (as they're still transitioning). As @blhoward2 noticed, this should be handled already in the zwavejs driver itself (it refreshes right after a set and 5 seconds after if the device needs this). If your device does not refresh automatically something is off.

blhoward2 commented 3 years ago

@marcelveldt That’s not exactly how it works. The way we fixed it is to assume the value is set immediately and treat it like it is by updating the currentValue immediately, verify, then fix it later by updating the state if its wrong. Couldn’t the same thing be done for turning on? The switch goes on, the UI holds the on state with some value (max?) until its either confirmed or reverts it if the confirmation doesn’t come? That’s exactly what we do when turning it off. Trust but verify versus verify and then set. The refresh and then update way just never worked with different transition times

With this PR, we no longer refresh or update values in the lower level commandClasses APIs. Instead this now happens in the SET_VALUE API if the low-level command succeeded. A few non-critical CCs (like name/location) no longer automatically poll and instead just assume it worked.

Also, we now update currentValue immediately where applicable and we update the newly set value in the value DB if the command was successful, so UIs should no longer flicker between different states.

https://github.com/zwave-js/node-zwave-js/pull/1522

blhoward2 commented 3 years ago

@kpine

There is no target value when you toggle the switch on, that's the problem. In other words, when you say "toggle on", it simply tells the switch to turn on to the last known state. zwave-js is not tracking historical values so it cannot optimistically set some value back to the application.

When I turn on my newer GE dimmers, using the mqtt integration, the brightness initially goes to 100% and then it drops back down after it gets the updated state. For mine this is so fast you can barely see it as they send an updated currentValue before they ever get to the refresh. Why isn’t that a better behavior all around than turning back off entirely and then flipping back on? While both are technically incorrect one way conforms much more to user expectations.

marcelveldt commented 3 years ago

@blhoward2 I think the best approach would be to just remember the last value that was set on the dimmer and return that optimistically when a "turn on" (setValue 255) is performed. We could do that in the integration but maybe even better to do so in the driver itself so the behavior is the same between implementations (e.g. ha vs mqtt).

blhoward2 commented 3 years ago

Not a bad idea. I’ll pass it on to @AlCalzone

kpine commented 3 years ago

@blhoward2

There is no target value when you toggle the switch on, that's the problem. In other words, when you say "toggle on", it simply tells the switch to turn on to the last known state. zwave-js is not tracking historical values so it cannot optimistically set some value back to the application.

When I turn on my newer GE dimmers, using the mqtt integration, the brightness initially goes to 100% and then it drops back down after it gets the updated state. For mine this is so fast you can barely see it as they send an updated currentValue before they ever get to the refresh. Why isn’t that a better behavior all around than turning back off entirely and then flipping back on? While both are technically incorrect one way conforms much more to user expectations.

This section of my comment you've quoted is of me explaining the design of node-zwave-js, so I'm not sure why your question is directed at me (you tagged me?). I have no opposition to fixing this UI problem, I'd like it to be solved. In the part of the comment you didn't quote, I literally described a possible solution in HA that appears to be what zwavejs2mqtt does? 🤷‍♂️

To actually solve this will likely require adding in special logic to the zwave-js light platform to optimistically modify the state in HA to some temporary value. The question is what would that value be? 100%? 50%? A cached last known value before the switch was off (which may be incorrect in some cases)?

blhoward2 commented 3 years ago

@kpine it wasn’t meant to be an attack. I merely meant to further conversation about the fact that HA doesn’t have a value to optimistically set back. Even if it doesn’t there is still a way to fix it. It wasn’t specifically meant to refute something you said.

AlCalzone commented 3 years ago

I think we talked about this already. As far as I see it, the problem comes from the fact that HA unoptimistically resets the UI back to the previous state when the update takes too long to arrive, although zwave-js tells HA that the command was accepted by the device.

IMO this info should be used to display some kind of pending state until the update comes, like this transition from on to off: ezgif com-video-to-gif

Tracking the previous value for every change adds unnecessary complexity and has the risk of communicating wrong values to applications. And I'm sure if that happens, there will also be complaints (as we've already seen with the EcoDim devices).

NYZack commented 3 years ago

I don't think it's purely a UI issue. When the icon shows a pending state, does HA think the entity is on? Or off? If on, then what is its brightness? Isn't that the root of the problem presented here?

I guess HA could assume the entity is off (as it does now) and show a pending state, but how will (for instance) a lighting group switch respond to this? It seems that it would stay off for 5 seconds. Not sure that's a good solution. Or what if one turns the lights on by toggling the lighting group switch? Right now the lighting-group toggle does the same suboptimal thing - toggles on, then off, then on again. Should there be a pending state for lighting groups also?

AlCalzone commented 3 years ago

I don't think it's purely a UI issue

Is it not? I think telling the user that his command was executed is a UI issue.

Let me outline what happens on the zwave-js side when you set the level (0-99) on a device:

  1. setValue(..., level) gets called -> queues message
  2. message gets sent, device sends acknowledgement, Promise returned by setValue resolves to true. Here you could notify the user that it worked. zwave-js does that in step 4 by optimistically setting currentValue
  3. We now know that the command was received by the device but not what it did with it.
  4. zwave-js assumes that the command was executed and optimistically sets the currentValue. Also, a verification GET is scheduled for 5s later, just in case. Some devices report the actual status before that.

Now if you turn the device ON instead (255), this happens:

  1. setValue(..., 255) gets called -> queues message
  2. message gets sent, device sends acknowledgement, Promise returned by setValue resolves to true. Here you could notify the user that it worked.
  3. We now know that the command was received by the device but not what it did with it.
  4. zwave-js cannot optimistically set the currentValue, because it does not know the (target) brightness. The only optimistic assumption can be that the device is ON, which does not translate to a currentValue. That could be any level from 1-99.
  5. A verification GET is scheduled for 5s later, just in case. Some devices report the actual status before that, which we hope for.

If on, then what is its brightness? Isn't that the root of the problem presented here?

Exactly. If we were to communicate a brightness, we would be lying. As a library we need to support more use cases than just the HA UI and if we're lying, that's not good. The device could have changed its brightness without zwave-js knowing. If it does not report its brightness on its own within 5 seconds after turning it on, I'd say it is unlikely we get reports of brightness changes unless we control it.

I guess HA could assume the entity is off (as it does now) and show a pending state, but how will (for instance) a lighting group switch respond to this? It seems that it would stay off for 5 seconds. Not sure that's a good solution

I'm not saying it should stay off. I'm saying that if the command worked, the UI should optimistically indicate that the device is on and that it is waiting for confirmation.

Just a side note: Z-Wave per se is a stateless protocol. Any attempt at mirroring the network state is bound to be incorrect. We can do our best where the chances are very high that we get it right (e.g. when setting a level), but that's about it. I think the best approach is to be honest about that - users should be able to understand this: "I did what you asked, it looks like it worked, but I don't know yet"

kpine commented 3 years ago

HA can assume an "on" state, but not necessarily have the correct brightness until later. Here's an example with my GE switch that is slow to ramp. Assuming "on" optimistically, but not caring about the brightness results in this new behavior:

light

I think this would work for most cases? I see some other integrations doing this very thing. I don't think it's realistic to have "realtime" brightness values.

blhoward2 commented 3 years ago

That looks perfectly fine to me, actually. @alcalzone’s idea may be prettier in effect but that’s perfectly functional.

AlCalzone commented 3 years ago

That's not bad if you ask me

kpine commented 3 years ago

I'm not an expert (so someone correct me if I'm wrong), but in HA I don't think there's currently anyway to represent a transitional/pending state for a light entity. It's either on, off, or unavailable. The unavailable state would not be a good choice.

NYZack commented 3 years ago

Maybe I'm being naive, and I'm happy to admit it, but here's what I meant to convey when I said it's not purely a UI issue. As far as I understand it, there's no UI element for "Z-wave light" in HA. There's a UI element for "light", which represents the value of the underlying light entity (which is a Z-wave light, in this setting). If the UI element has to show "on" with a gradually increasing brightness, or some kind of pending state, then the underlying entity has to have this behavior first. Otherwise, HA has to change its logic fairly substantially.

So I think this is really a driver issue, or an issue for zwave-js. If it's settled on the driver side, then the entity can be turned on, possibly with a gradually increasing brightness to ... what? What is the logic behind the brightness for the entity? One suggestion was 100%; another was to somehow store the previous brightness until zwave-js returns the 5-second result. I'm not sure if a switch can be "on" with a brightness of 0, but I guess that's another option. I suppose another option would be to arbitrarily set a brightness of 1 until zwave-js reports the state after 5 seconds. That is not a lie - at some point in time a dimmer has to be at 1% on its way up to some (unknown) brightness.

kpine commented 3 years ago

I personally am struggling to understand what you are asking for. Could you list, step by step, what you think should happen when an HA light entity (z-wave, with brightness support) is toggled from off to on? Do you have other integrations to compare with?

I'm not sure if a switch can be "on" with a brightness of 0

It seems it can, as I demonstrated in that animated gif. The state of the entity is on or off, the brightness is just an attribute.

NYZack commented 3 years ago
  1. I turn on a z-wave light toggle in the HA UI.
  2. The toggle reflects the "on" state. The underlying entity reflects "on".
  3. The light turns on (probably to the previously set brightness, but I don't think that has anything to do with HA).
  4. The toggle stays on. The underlying entity stays on.
  5. At some point, the UI element and the underlying entity reflect the true brightness of the light. At least for me, I don't care if there's a delay with regard to this.

The question (at least as I see it) is, what brightness do the UI element and the underlying entity (which it reflects) report until zwave-js returns the actual brightness after 5 seconds or so?

I don't know how this is handled for other integrations.

kpine commented 3 years ago

That is exactly the behavior I'm showing in that animated gif. 😆

Personally I don't care what the brightness is until the real value is refreshed. None of them are valid. Faking the brightness is more complicated than just letting it get refreshed naturally, so zero seems fine to me. Maybe an HA core dev can chime in.

Also not all switches get a new value update after 5 seconds. This is only for switches that don't report a an updated value on their own. Many switches report a new value much sooner, so the 5 second refresh is redundant.

NYZack commented 3 years ago

@kpine I had no quarrel with your animated gif. It would be great. My point (again, maybe I'm being dense) is that there isn't a dedicated UI element for Z-wave lights or Z-wave dimmers. So your GIF would have to represent what the underlying entity does. In that case, again, what value does the slider represent? Zero, until zwave-js reports the actual value? That would be fine with me. Not sure if there's any logic to prevent an "on" switch from having a brightness of 0 (the HA UI only lets me adjust a brightness as low as 1), but, if so - great. (I would vote for 1 rather than 0, since an "on" switch has to have some minimal brightness, but I don't feel strongly.)

kpine commented 3 years ago

Ok, we are just talking in circles now. The gif wasn't a mock up, sorry that wasn't clear. I modified the zwave-js light integration to assume the state. As you've said, there's no special frontend code related to zwave-js lights, so the frontend is reacting to the optimistic state of "on" that is being stored.

Current code:

  1. Toggle switch in frontend to on
  2. Frontend assumes state is "on", it sets a timeout for 2 seconds
  3. HA zwave-js integration sends the "on" command to node-zwave-js
  4. After two seconds the frontend has not seen a state update, so it resets the switch toggle UI to the current state (off)
  5. After five seconds node-zwave-js refreshes the value and sends the updated brightness to HA
  6. HA zwave-js integration handles the value update and updates the entity state
  7. Frontend sees the state change and updates the switch toggle to on and updates the brightness slider/value

Vs. my demo code:

  1. Toggle switch in frontend on
  2. Frontend assumes state is "on", it sets a timeout for 2 seconds
  3. HA zwave-js integration sends "on" command to node-zwave-js
  4. HA zwave-js integration sets its internal state to "on" (brightness remains at 0 since it was off), updates entity state
  5. After two seconds the optimistic entity state change was already seen by the frontend, so it keeps the switch UI toggle on
  6. After five seconds node-zwave-js refreshes the value and sends the updated brightness to HA
  7. HA zwave-js integration handles the value update and updates the entity state
  8. Frontend sees the state change and updates the the brightness slider/value (state remains on)

0 is a valid brightness value. It's true, the frontend doesn't let you set that value. I'm not concerned much with that, I was just interested in demonstrating that it was possible to avoid the UI glitch by optimistically assuming the state of the entity.

Note that many switches will already send the updated value w/o a manual refresh prior to the two second timeout in the frontend, so they don't see this problem.

blhoward2 commented 3 years ago

I think this looks good and suggest it be PRd. It seems to solve the issue fine.

marcelveldt commented 3 years ago

I'll put together a PR later today with the proposed fix. I'm going to add in a little guard that if no value received within 10 seconds, it will revert the optimistic state.

kpine commented 3 years ago

I'm going to add in a little guard that if no value received within 10 seconds, it will revert the optimistic state.

Keep in mind, when duration is supported an update might not come in until ~duration seconds instead.

jlallen75 commented 3 years ago

I'm seeing this on\off\on issue as well with the following switches. GE/Jasco | In-Wall Smart Fan Control | 14287 / ZW4002 Honeywell | In-Wall Smart Dimmer | 39351 / ZW3005

It looks like all my non dimmer switches are fine as well as my Zooz dimmers.

blhoward2 commented 3 years ago

@marcelveldt @kpine Any update on having this PRd? Can we PR in kpine's version? There should be no need for a guard because node-zwave-js will itself revert the state if it isn't confirmed after a period of time. Right, @AlCalzone?

AlCalzone commented 3 years ago

Almost. node-zwave-js will request an update from the device after 5 seconds, which should either confirm the new value or revert to the old one. However, there's a possibility that this request won't get any response. In that case node-zwave-js currently does nothing, because it would have to guess.

Nick-116 commented 3 years ago

Has this issue been resolved yet? I was told it was solved but not merged. I am also having the same issue as mentioned.

kpine commented 3 years ago

I have a proof of concept change here. I made that change just to prove to myself it was possible. I don't intend to submit a PR though. First, it needs testing which I don't have time to do, secondly the UI glitch doesn't bother me because I rarely use it, and lastly I'm not fully convinced it's the right thing to do:

However, there's a possibility that this request won't get any response. In that case node-zwave-js currently does nothing, because it would have to guess.

I don't think the integration code really knows if the command succeeded or not, and we rely on the value refresh to correct the state (maybe there's an exception that could be handled to revert the optimistic state)? I think it'd be worse, on failure, to show a state that is probably incorrect, than to have a weird UI glitch that doesn't occur for all devices.

I personally don't want to implement anything more complicated, such as what Marcel has suggested, maybe that is the right solution though. 🤷‍♂️ Someone else is welcome to take my simple patch and develop it into a proper PR.

blhoward2 commented 3 years ago

However, there's a possibility that this request won't get any response. In that case node-zwave-js currently does nothing, because it would have to guess.

@AlCalzone Shouldn't this cause the device to be marked dead if we attempt a value refresh for a mains device and receive no response? Under what circumstances would this arise?

AlCalzone commented 3 years ago

If the node ACKs the GET, but does not respond, it won't be marked as dead.

blhoward2 commented 3 years ago

I don't see what's wrong with HA reverting the state in the unlikely event we are in a situation where the device Acks but then doesn't respond. I mean assuming it failed would be the same thing that's happening instantly now it just would happen a few seconds later. The difference is it's a once in a blue moon theoretical event versus one that happens every time now.

@kpine What is your concern with that approach?

github-actions[bot] commented 3 years ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.