SecKatie / ha-wyzeapi

Home Assistant Integration for Wyze devices.
722 stars 112 forks source link

Cloud control of color bulbs #310

Closed viveksjain closed 2 years ago

viveksjain commented 2 years ago

Is your feature request related to a problem? Please describe. I see that #302 switched to local control of bulbs. I can see why this would be preferable for most people, unfortunately my server runs in a different network so I'd like to keep cloud control.

Describe the solution you'd like Is it possible to have some option to toggle cloud control for bulbs?

Additional context Tangentially related, the only error I found in the logs was

2021-12-17 01:22:42 WARNING (MainThread) [wyzeapy.services.base_service] Failed to connect to bulb 7C78B218FB97

Would be nice if it included the IP/port it was trying to access, that would have made it immediately obvious what the issue was rather than spending a while debugging it.

JoeSchubert commented 2 years ago

You make a good point. It's definitely not a use case that I think was considered. It's also a good thing to get figured out now, since I'm guessing that as Matter moves on we'll probably see more and more local control start showing up.

So, there's a few ways that I could see this being implemented.

  1. A global config flag
  2. A per device config flag
  3. During initialization check if the device can be connected to, if it can't default to cloud control (what if the device was just offline right then?)
  4. Check during each command. Basically, try local control and default to cloud control if local fails. This would introduce a slightly longer lag for each service if it's getting controlled by cloud ... But it would also probably be the easiest to implement.

I don't have bulbs, so I wouldn't even be able to try any of these.

Thoughts @JoshuaMulliken ?

BradSimard commented 2 years ago

my bulbs are on the same network as my server, but I'm getting the log you mentioned (and it fails to properly interact with the bulbs):

Logger: wyzeapy.services.base_service Source: .local/lib/python3.9/site-packages/wyzeapy/services/base_service.py:613 First occurred: 8:10:03 PM (1 occurrences) Last logged: 8:10:03 PM

Failed to connect to bulb 7C78B232371B

randomevents commented 2 years ago

@yoinx I'd think either option 1 or 2 would be the better option. There's not a small number of people that use the bulbs, especially the color ones, as an enhancement to existing light switches or automation, so the primary power control doesn't rest with the bulbs themselves.

tggman commented 2 years ago

my bulbs are on the same network as my server, but I'm getting the log you mentioned (and it fails to properly interact with the bulbs):

Logger: wyzeapy.services.base_service Source: .local/lib/python3.9/site-packages/wyzeapy/services/base_service.py:613 First occurred: 8:10:03 PM (1 occurrences) Last logged: 8:10:03 PM

Failed to connect to bulb 7C78B232371B

This is the same error I randomly get with my color bulbs. They work for a bit, but seem to stop responding to commands after some time. In my case, the bulbs fail to respond to commands but seem to always revert back to the correct status after the failed command. See issue #315. It's worth noting that most of my IoT are on the same Wi-Fi AP but the AP is in BRIDGE mode to my main router where my HA server is sitting.

brg468 commented 2 years ago

I'm working on setting up the first option (global config flag), and I'd like to have somebody do a little testing if you are able. @tggman you might be the logical choice since you've gone through this process before, but if anybody else is interested I can walk you through the steps. I'll probably have it ready for testing tomorrow.

brg468 commented 2 years ago

Ok here's the new branches for testing:

https://github.com/brg468/wyzeapy/tree/global-local-control-option https://github.com/brg468/ha-wyzeapi/tree/global-local-control-toggle

Basically the same deal as before: Place wyzeapy in config folder. Replace current wyzeapi in custom_components with the new one here. Make sure you replace the entire folder.

Once thats done I'd recommend adding this to your configuration.yaml

logger:
  default: info
  logs:
    custom_components.wyzeapi: debug
    wyzeapy.services.bulb_service: debug

From there you should be good to go. There will be a new configure button on the integrations page. You may need to clear your browser cache if its not showing up initially. Give it a try and let me know if its working correctly. Thanks for the help!

tggman commented 2 years ago

Installed as instructed above. Upon restart, all 8 of my color bulbs had the Cloud: false attribute and no relevant errors in the log (debug level).

Relative to the Wyze Home Assistant Integration, I saw the configure button without having to clear my (Chrome) browser cache. Here is the dialog I saw when I hit configure: image and received this dialog after toggling it off image

Observations Use Local Control option OFF:

I then toggled the Use Local Control option back ON

Observations Use Local Control option ON:

Question: Is it safe to assume that any color bulb that fail to function as expected will revert to the cloud even when the Use Local Control is ON?

brg468 commented 2 years ago

So what I think is happening: When the option is toggled between local/cloud, the integration is reloaded (as it should be) but the update listeners are not unsubscribing, which results in multiple updates being fired for each device. That is causing the flip-flop of the cloud attribute. This seems to be a bug in the integration itself, as I pointed out in the issue I just opened. I'm hoping for some help in resolving that before this can be implemented. That being said, toggling the local/cloud option and then restarting HA seems to fix the issue and everything works correctly from what I can tell.

Question: Is it safe to assume that any color bulb that fail to function as expected will revert to the cloud even when the Use Local Control is ON?

Yes the cloud fallback will still work in the event that you have local selected and the bulb loses connection.

Thanks @tggman for your detailed review, I wouldn't have noticed this without you catching it.

tggman commented 2 years ago

Thanks @brg468 for taking the time to explain the issue. I appreciate it.

As a containment action (i.e. until a method of unsubscribing the update listeners is found), would it make sense to include a message indicating a reboot is required as part of the response to changing the option?

brg468 commented 2 years ago

Thanks @brg468 for taking the time to explain the issue. I appreciate it.

As a containment action (i.e. until a method of unsubscribing the update listeners is found), would it make sense to include a message indicating a reboot is required as part of the response to changing the option?

I think I've got the updater issue figured out. I'll probably get a PR in to correct that issue separately than double back and get this added. In the meantime, the fallback logic is still in the current release so that should keep everybody up and running for now.

brg468 commented 2 years ago

@tggman changes to the branches above are posted if you want to test again.

One noticeable difference: This no longer reloads the whole integration, which helps solve other issues that came up. As far as the local control attribute goes, it won't immediately update when you change the toggle, it will update on its next update cycle (about 30 seconds or so). The functionality will be immediate however, so as soon as you command the bulb it will use whatever state you have picked and the attribute will change . Let me know how it goes.

tggman commented 2 years ago

@brg468 I just downloaded and started the test of the latest.

Specifically: I downloaded the code from:

...and

Question: The "Local Control" attribute name is mixed case and contains a space. Was that intentional?

brg468 commented 2 years ago
  • I see that apparently the "cloud:" attribute was changed to "Local Control:" --- is that expected/correct?

Yes I changed it and updated the logic behind it. Seems more straight forward to me.

The "Local Control" attribute name is mixed case and contains a space. Was that intentional?

Like this? Local Control true Yea its just pulling true/false from the code and that's how its returned

tggman commented 2 years ago

@brg468 I meant relative to standard attribute naming conventions (if there is one). I've just never seen an attribute name in mixed case and a space - but I am by no means a expert.

brg468 commented 2 years ago

I see what you’re saying. I hadn’t looked in dev tools, but you’re right that is convention. I’ll fix it. Thanks!

tggman commented 2 years ago

@tggman I am incorporating a couple other changes in a new pre-release and if everything is good for a few more days I will make it stable.

_Originally posted by @JoshuaMulliken in https://github.com/JoshuaMulliken/ha-wyzeapi/issues/315#issuecomment-1030369610

@JoshuaMulliken and @brg468 I just updated to ha-wyzapi 0.1.5-alpha.1 via HACS and the version of light.py and potentially wyzeapy that are part of that release are older/different. I only know this because the Cloud attribute was renamed Local Control**. If this is known and understood, never mind.

**side note: A month or 2 back I suggested the wyzeapy version ID be listed somewhere (e.g. as an attribute or a separate entity) but @yoinx didn't agree. Since then I've seen it come up at least one other time and now here is another example of where it might be useful.

brg468 commented 2 years ago

So that release incorporates https://github.com/JoshuaMulliken/ha-wyzeapi/pull/337, which is a fix for a different issue. It doesn’t include anything for this issue.

tggman commented 2 years ago

I don't know how the release process works, but as long as you all know what I'm testing which is:

brg468 commented 2 years ago

That will most likely cause issues because they are based on different wyzeapy versions. Can you test with my branch in your custom_components folder?

tggman commented 2 years ago

That is what I had in place before I updated via HACS today. So yes, I can revert back now if that is what makes the most sense to everybody.

tggman commented 2 years ago

@brg468 I just downloaded and started the test of the latest.

Specifically: I downloaded the code from:

I reverted back to the above... sorry if I'm the cause of confusion, but I want to make sure everyone knows what I'm evaluating/testing.

tggman commented 2 years ago

I see what you’re saying. I hadn’t looked in dev tools, but you’re right that is convention. I’ll fix it. Thanks!

@brg468 I just noticed the device model attribute also contains a space.

tggman commented 2 years ago

@brg468 its been 3 days and everything appears to be working as expected. I've had no control issues and 4 of my 8 bulbs reverted seamlessly to cloud control (local control: false) with no loss in functionality. As expected, when a bulb first connects to the cloud, there is a noticeable but 100% acceptable delay of around 2s. After that, bulb control response feels good.

Question: Is there any attempt made to automatically fall back to local control (e.g. once a day or after x control commands)? Adding that as an option in the integration would seem to make the most sense to me at least.

brg468 commented 2 years ago

Good to hear it’s working as expected.

Question: Is there any attempt made to automatically fall back to local control (e.g. once a day or after x control commands)? Adding that as an option in the integration would seem to make the most sense to me at least.

It would probably be possible, but since the functionally is there even in cloud mode and the response time is still good (it’s the same as the Wyze app in my experience) I think it might cause more issues than it’s worth. Just my opinion though.

tggman commented 2 years ago

Is there anything more I can do or should be doing to help get these and any of the other pertinent changes (e.g. #315) into the latest wyzeapy/ha-wyzapi release?

brg468 commented 2 years ago

@tggman I don't think so.. I believe its confirmed to be working, so just waiting on it getting merged at this point. Thanks again for helping test all this.

SecKatie commented 2 years ago

Now that the code for this is merged into master and released as a stable, I am closing this issue.