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.

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

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

Closed
wants to merge 1 commit into from

Conversation

jvolkman
Copy link

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.

@lifeisafractal
Copy link

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.

@jvolkman
Copy link
Author

jvolkman commented Nov 3, 2017

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

@lifeisafractal
Copy link

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?

@jvolkman
Copy link
Author

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

@lifeisafractal
Copy link

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

@mbevilacqua
Copy link

yep waiting on this one too.

@matthewcky2k
Copy link
Contributor

Still waiting on a merge on this one?

@adamup928
Copy link

+1 for merge

@jordemort
Copy link

Also eagerly awaiting this one

@matthewcky2k
Copy link
Contributor

@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...

@lifeisafractal
Copy link

@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.

@matthewcky2k
Copy link
Contributor

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.

@jvolkman
Copy link
Author

jvolkman commented Jan 9, 2018

@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.

@matthewcky2k
Copy link
Contributor

Thanks @jvolkman

@hopeless1
Copy link

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.

@jvolkman
Copy link
Author

Friendly ping @Fishwaldo

@1activegeek
Copy link

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!

@aaronwolen
Copy link
Contributor

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!

@1activegeek
Copy link

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.

@wQQhce2g93Ei
Copy link

Any word on merging this PR @Fishwaldo?

@jvolkman
Copy link
Author

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

@jsmath
Copy link

jsmath commented Aug 1, 2018

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.

@bibi21000
Copy link
Member

bibi21000 commented Aug 1, 2018

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

@jvolkman
Copy link
Author

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?

@loe
Copy link

loe commented Mar 4, 2019

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
Copy link

loe commented Mar 5, 2019

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.

@Fishwaldo Fishwaldo added the Investigate Issues that need more investigation by Devs label May 13, 2019
@jvolkman
Copy link
Author

jvolkman commented Jun 5, 2019

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.

@loe
Copy link

loe commented Jun 6, 2019

@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. Remove dimmer polling 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.

@danielbrunt57
Copy link
Contributor

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??

@jvolkman jvolkman closed this Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Investigate Issues that need more investigation by Devs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinite loop when verifying change