open-zwave icon indicating copy to clipboard operation
open-zwave copied to clipboard

Don't trigger Value::OnValueRefreshed() when verifying changes and value hasn't changed

Open jvolkman opened this issue 8 years ago • 29 comments

When a Value is set to verify change, and the value hasn't changed (bOriginalEqual is true), don't call Value::OnValueRefreshed(). Also, don't call OnValueChanged() since it's already called when the changing value is confirmed.

This should fix #1220.

jvolkman avatar Oct 23 '17 06:10 jvolkman

I was able to confirm the bug describe by #1220 in a setup using home-assistant, python-openzwave and a HS-HD100+ dimmer. Applying this patch did fix the issue. Is there any update on getting this merged? There is a PR in home-assistant that depends on this to get merged.

lifeisafractal avatar Nov 02 '17 23:11 lifeisafractal

Sorry @lifeisafractal, haven't heard anything yet about getting this merged.

jvolkman avatar Nov 03 '17 16:11 jvolkman

I hate to keep bumpping this issue, but the home-assistant PR I mentioned above is on hold until this gets merged and it's a pretty major pain point for myself and many other people. Is there anyone in particular to ping about checking this PR out seeing as there hasn't been any activity or feedback either good or bad?

lifeisafractal avatar Nov 28 '17 14:11 lifeisafractal

I think that person is @Fishwaldo but it looks like he's been on hiatus for a bit.

jvolkman avatar Nov 29 '17 21:11 jvolkman

Cool, and totally understandable, just wanted to see if there was something I could do.

lifeisafractal avatar Dec 01 '17 15:12 lifeisafractal

yep waiting on this one too.

mbevilacqua avatar Dec 09 '17 18:12 mbevilacqua

Still waiting on a merge on this one?

matthewcky2k avatar Jan 07 '18 09:01 matthewcky2k

+1 for merge

adamup928 avatar Jan 08 '18 03:01 adamup928

Also eagerly awaiting this one

jordemort avatar Jan 08 '18 15:01 jordemort

@lifeisafractal how did you apply the patch to your HA openzwave install? since this PR is still open with no ETA I would like to stop having to restart my zwave network every day...

matthewcky2k avatar Jan 09 '18 15:01 matthewcky2k

@matthewcky2k it was actually a pretty lengthy and challenging process (especially compiling on host with a R-Pi). I had to checkout the python-openzwave project. The developer setup for that was a little tricky. It took me a while to figure out how to use the build system to produce a python-openzwave package that used a locally modified copy of openzwave. Then, you have to make sure that your python virtual environment that HomeAssistant uses points to this modified copy of pyhton-openzwave. All told, it took a full evening to figure it out, and it was a while ago that I did this so I unfortunately cant help with a step-by-step. The complications of this setup are a big driver for me wanting this patch to get merged, there isn't a simple way to get this one line fix in.

lifeisafractal avatar Jan 09 '18 16:01 lifeisafractal

I had a look myself and suspected as such.. was hoping you had a quick fix but thanks for the reply hopefully @Fishwaldo can review this and merge soon.

matthewcky2k avatar Jan 09 '18 16:01 matthewcky2k

@matthewcky2k I use hass.io and maintain a custom build of the homeassistant image with this patch and a HASS patch similar to the one @lifeisafractal mentioned. Building it is a repeatable process codified in this Dockerfile. At the least, it should help you figure out how to build a custom pyopenzwave package with this change.

jvolkman avatar Jan 09 '18 19:01 jvolkman

Thanks @jvolkman

matthewcky2k avatar Jan 10 '18 11:01 matthewcky2k

I hope this gets merged soon. I was able to get my dimmers to update correctly using the 'refresh_value: true' parameter in home assistant, but I'd definitely prefer a better integrated solution.

hopeless1 avatar Jan 25 '18 13:01 hopeless1

Friendly ping @Fishwaldo

jvolkman avatar Jan 26 '18 18:01 jvolkman

Just ran into this one myself in switching from OpenHAB to HomeAssistant. Due to the number of devices, I’d prefer to have a clean solution as outlined here. +1 for the merge here. And thanks for the awesome work thus far on OZW!

1activegeek avatar Apr 11 '18 04:04 1activegeek

I experience this same issue with my ZWP dimmers (similar to the HS-HD100) and running this branch fixed it for me as well. Hope to see this merged soon!

aaronwolen avatar Apr 24 '18 15:04 aaronwolen

I'm curious, would this update potentially avoid the delay that is noticed? I'm using the workaround right now, and I've noticed that automation happening based on state, won't actually trigger until that refresh happens (aka the 1-2 sec delay). I'm hoping if this gets merged, and the fix isn't needed, that the trigger will happen faster.

1activegeek avatar Apr 24 '18 15:04 1activegeek

Any word on merging this PR @Fishwaldo?

wQQhce2g93Ei avatar May 25 '18 02:05 wQQhce2g93Ei

Ping? Is there a better way to fix this problem?

jvolkman avatar Jul 10 '18 21:07 jvolkman

I am also eagerly waiting on this change to hopefully fix the on/off status on the home assistant web ui from flipping back and forth when turning on/off a GE zwave dimmer. This is probably my biggest annoyance with using zwave devices in home assistant right now.

jsmath avatar Aug 01 '18 15:08 jsmath

Hi, if you really need this patch, there is at least 3 ways with python-openzwave :

@lifeisafractal sed -i -e 's|github.com/OpenZWave/open-zwave|github.com/yourrepo/open-zwave|g' pyozw_* should do the work And if you launch setup in your virtual env ... It will surely use your updated release

use the 'dev' flavor with LOCAL_OPENZWAVE

build and install the patched version of openzwave and use the 'shared' flavor

Look at docs

HTH

bibi21000 avatar Aug 01 '18 16:08 bibi21000

I've actually stopped using this patch in my own home, since it seems to cause my HomeSeer dimmers to not actually turn completely off at times (the dimmer level LEDs are off, but the actual light is still on at its lowest level). Possibly due to the fact that with verification enabled there is no delay between subsequent status requests.

It would make more sense to add a delay between requests and not flood the network, but I'm not sure how to properly do that in OZW. Is there a way to enqueue a command for processing at a later time?

jvolkman avatar Nov 28 '18 21:11 jvolkman

I've actually stopped using this patch in my own home, since it seems to cause my HomeSeer dimmers to not actually turn completely off at times (the dimmer level LEDs are off, but the actual light is still on at its lowest level). Possibly due to the fact that with verification enabled there is no delay between subsequent status requests.

It would make more sense to add a delay between requests and not flood the network, but I'm not sure how to properly do that in OZW. Is there a way to enqueue a command for processing at a later time?

You can set "refresh_value" on a per device basis in Home Assistant, this will refresh the device in 5 seconds. "delay" will let you change this to something other than 5.

loe avatar Mar 04 '19 19:03 loe

I've actually stopped using this patch in my own home, since it seems to cause my HomeSeer dimmers to not actually turn completely off at times (the dimmer level LEDs are off, but the actual light is still on at its lowest level). Possibly due to the fact that with verification enabled there is no delay between subsequent status requests.

I have this problem as well, but I am not running this patch, I suspect it is something else. It typically happens to me when I turn off a large number of devices simultaneously (i.e. I have a single script that turns off all lights on first floor when going upstairs to bed.) but I haven't been able to reliably reproduce it. I am using mostly HS-WD100+ dimmers, with a few WD200+ and security enabled on all.

loe avatar Mar 05 '19 18:03 loe

I have this problem as well, but I am not running this patch, I suspect it is something else. It typically happens to me when I turn off a large number of devices simultaneously (i.e. I have a single script that turns off all lights on first floor when going upstairs to bed.) but I haven't been able to reliably reproduce it. I am using mostly HS-WD100+ dimmers, with a few WD200+ and security enabled on all.

@loe my hunch was that it was related to heavy request load on the device during a dimming operation, but maybe it's just related to a large amount of simultaneous activity in general as would be the case in your scenario.

jvolkman avatar Jun 05 '19 22:06 jvolkman

@jvolkman I made a few changes which have fixed all my issues, and am not interested in spending the time to root cause it.

  1. Stopped using security everywhere, only on locks or security sensors. The overhead is high.
  2. I patched the python wrapper to stop double polling on turn off. OpenZWave/python-openzwave#160
  3. Configured a single delayed poll at the top of the stack, in Home Assistant.

All of these have significantly reduced the traffic on the network and I haven’t seen the weird behavior since.

I think your hunch is right, it’s too much load on that node. Could be fun to map your network and see if the node with the issue is also relaying lots of messages. Perhaps it is the combination of replying to all the status queries and relaying messages (from a mass-off event like my script) from other nodes that triggers the overload.

loe avatar Jun 06 '19 18:06 loe

You can set "refresh_value" on a per device basis in Home Assistant, this will refresh the device in 5 seconds. "delay" will let you change this to something other than 5.

Can this be done in OZW 1.6.1240 / Home Assistant 0.116.1? and how? mqtt.publish??

danielbrunt57 avatar Oct 09 '20 05:10 danielbrunt57