Refactor switch for vesync #134409

cdnninja · 2025-01-01T20:23:42Z

Proposed change

Refactor of switch sensor to support it offering more switches in the future, such as screen on off. This one will wait on the update coordinator merge first then update to reflect those changes. Creating a draft PR for early feedback.

I do not own a vesync switch or outlet so I would love to have someone test this on real hardware.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

home-assistant · 2025-01-01T20:23:49Z

Hey there @markperdue, @webdjoe, @TheGardenMonkey, mind taking a look at this pull request as it has been labeled with an integration (vesync) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of vesync can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign vesync Removes the current integration label and assignees on the pull request, add the integration domain after the command.
@home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
@home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

cdnninja · 2025-01-01T20:24:42Z

@iprak FYI. As two side notes you should add yourself as a code owner on this integration, your support has been incredible. Would love to connect more on the HA discord too.

cdnninja · 2025-01-01T20:27:11Z

I am thinking this PR should probably split the removal of the VeSyncDevice entity to a different PR. I don't see value in that class. Just complicates things. Thoughts?

cdnninja · 2025-01-04T00:44:18Z

My one thought is I am not certain the names of the entities are unchanged for the two existing. I also can't test that hardware as I don't own it.

iprak · 2025-01-04T02:10:58Z

I am thinking this PR should probably split the removal of the VeSyncDevice entity to a different PR. I don't see value in that class. Just complicates things. Thoughts?

Sure, it definitely helps to keep PR small.

cdnninja · 2025-01-04T02:24:39Z

I am thinking this PR should probably split the removal of the VeSyncDevice entity to a different PR. I don't see value in that class. Just complicates things. Thoughts?

Sure, it definitely helps to keep PR small.

Other portion has been split to #134499.

homeassistant/components/vesync/switch.py

homeassistant/components/vesync/common.py

homeassistant/components/vesync/strings.json

joostlek · 2025-01-19T10:40:08Z

homeassistant/components/vesync/switch.py

+        # Other types of wall switches support dimming.  Those use light.py platform.
+        exists_fn=lambda device: is_wall_switch(device) or is_outlet(device),


Okay, but how come only these 2 are switches? As in, is_wall_switch doesnt sound to incorporate dimming

I thought I had handled this different maybe lost in a merge - so here is the challenge. The old lookup table takes two model of switches and calls them a dimmer switch. Goal is to not need that lookup table for models in the future.

https://github.com/cdnninja/core/blob/09ae388f4e947f508cedda70b806a839326ea4af/homeassistant/components/vesync/const.py#L51-L52

These devices in the old method didn't create a switch entity just a light entity. Currently with my code it would also get a switch. Should we leave it like that so if dimmable switch or outlet you still get a switch toggle? This is the easier way, other option is adjust the exists to exclude the type of VeSyncDimmerSwitch which is a type of switch within the library.

@iprak Thoughts?

I don't think we need to provided a separate switch for dimmer. LightEntity extends from ToggleEntity and so it provided on/off capability natively.

This is what I see for a dimmer switch

Went back to fix this and realized I was correct up front. In pyvesync vesyncswitch has two child classes, VeSyncWallSwitch and VeSyncDimmerSwitch. The above filters to just wall switch so dimmer isn't used. As such this won't show for dimmers. No changes required. I updated the helper comment some to try make this more clear.

homeassistant/components/vesync/switch.py

joostlek · 2025-01-19T10:41:41Z

homeassistant/components/vesync/switch.py

        self.device.turn_off()
+        self.schedule_update_ha_state()


Why do we do that with the state?

Without it HA toggles it back the other way until next scan interval which gives it a poor user experience as it appears to have not worked.

Co-authored-by: Joost Lekkerkerker <[email protected]>

iprak

I added some code comments which I encountered while trying to use your branch.

I have added humidifier related switches on top of your branch with some adjustments. You can take a look at my branch here- https://github.com/iprak/core-fork/tree/added-switch-for-humidifier. I will submit a PR for that separately once yours is approved.

homeassistant/components/vesync/switch.py

iprak · 2025-01-25T13:48:17Z

homeassistant/components/vesync/switch.py

-    def turn_on(self, **kwargs: Any) -> None:
-        """Turn the device on."""
-        self.device.turn_on()
+    def __init__(


We need to define _attr_unique_id otherwise adding another switch entity fails.

Something like this will work - self._attr_unique_id = f"{super().unique_id}-{description.key}"

For the two devices exposed today if we change this it will be a breaking change. However I could add a filter to only do the above unique ID on ones other than the outlet and switch on we offer today. It would set us up for future.

Thoughts on that approach?

I suppose that will work. @joostlek might have reviewed code which ran into similar shortcoming and if the _attr_unique_id was defined as a breaking change.

Oh that is a good catch. If that is the case we should migrate first. I don't feel like merging this as it will create entities with not unique unique ids. So we should have a preliminary PR where we migrate all the unique ids of the switches to something else and then we can set up new ones

Okay, today this code doesn't conflict or impact unique ID, however since trying to setup for growth makes sense we fix this now. I will update this code to have the unique ID as mentioned above and chat with iprak around getting a PR for that migration that will merge first.

iprak · 2025-01-25T13:49:03Z

homeassistant/components/vesync/switch.py

        self.device.turn_off()
+        self.schedule_update_ha_state()


Shouldn't schedule_update_ha_state be only called if turn_off succeeds?

Some other spots have it this way - my theory was after touching devices we should check the source data regardless to ensure we are updated. Both approaches though in most cases would show correctly I would think. Which do you prefer?

The turn_on/off return true if operation succeeds and I think that should be respected and used as the basis of state refresh. Yes I do see some integration call this all the time and I would like @joostlek 's input on this.

I think without this, the entity would show the new data when it is refreshed but I am not sure if the underlying framework does the same. Some other integrations invoke a refresh and I think that takes care of cases where multiple entities get updated in response to one action.

Switch returns true if the web call succeeded, we should use that return value to schedule_update_ha_state.

iprak · 2025-01-25T13:49:20Z

homeassistant/components/vesync/switch.py

-        self.switch = switch
+    def turn_on(self, **kwargs: Any) -> None:
+        """Turn the entity on."""
+        self.device.turn_on()


Shouldn't schedule_update_ha_state be only called if turn_on succeeds?

cdnninja · 2025-01-25T16:21:15Z

I added some code comments which I encountered while trying to use your branch.

I have added humidifier related switches on top of your branch with some adjustments. You can take a look at my branch here- iprak/core-fork@added-switch-for-humidifier. I will submit a PR for that separately once yours is approved.

Hopefully it is okay - I factored in some of your items for on_fn and off_fn into this one so it is ready to add future switches easily.

iprak · 2025-01-25T18:05:31Z

I added some code comments which I encountered while trying to use your branch.
I have added humidifier related switches on top of your branch with some adjustments. You can take a look at my branch here- iprak/core-fork@added-switch-for-humidifier. I will submit a PR for that separately once yours is approved.

Hopefully it is okay - I factored in some of your items for on_fn and off_fn into this one so it is ready to add future switches easily.

I think I might need to rework this for humidifier. The pyvesync methods for humidifier related switches do not update the internal device state. It is inconsistent, some do and some don't. So I might have to force a refresh instead of state update.

cdnninja · 2025-01-25T19:00:54Z

I added some code comments which I encountered while trying to use your branch.
I have added humidifier related switches on top of your branch with some adjustments. You can take a look at my branch here- iprak/core-fork@added-switch-for-humidifier. I will submit a PR for that separately once yours is approved.

Hopefully it is okay - I factored in some of your items for on_fn and off_fn into this one so it is ready to add future switches easily.

I think I might need to rework this for humidifier. The pyvesync methods for humidifier related switches do not update the internal device state. It is inconsistent, some do and some don't. So I might have to force a refresh instead of state update.

Okay - I suspect we will have a few things like that as we work through adding items to it. Hopefully the base sets us up for that.

joostlek · 2025-01-28T16:37:21Z

homeassistant/components/vesync/switch.py

-    def turn_on(self, **kwargs: Any) -> None:
-        """Turn the device on."""
-        self.device.turn_on()
+    def __init__(


Oh that is a good catch. If that is the case we should migrate first. I don't feel like merging this as it will create entities with not unique unique ids. So we should have a preliminary PR where we migrate all the unique ids of the switches to something else and then we can set up new ones

cdnninja · 2025-01-29T02:52:56Z

Unique IDs added, tests will fail until migration code is done as I think the tests need to exist in that. Once this branch is updated with the migration and tests PR it should pass.

cdnninja · 2025-02-01T22:11:40Z

Unique ID migration PR is ready for review. Once that is merged I will update this branch and it will also be ready for review.

iprak · 2025-02-03T17:57:29Z

homeassistant/components/vesync/switch.py

        self.device.turn_off()
+        self.schedule_update_ha_state()


Switch returns true if the web call succeeded, we should use that return value to schedule_update_ha_state.

iprak · 2025-02-03T17:57:43Z

homeassistant/components/vesync/const.py

@@ -32,6 +37,9 @@
 VeSyncHumidifierDevice = VeSyncHumid200300S | VeSyncSuperior6000S
 """Humidifier device types"""

+VeSyncFanDevice = VeSyncAirBypass | VeSyncAir131
+"""Humidifier device types"""


This is incorrect.

Refactor switch sensor so we can use it for more.

0d67c7b

home-assistant bot added cla-signed code-quality has-tests integration: vesync by-code-owner Quality Scale: No score labels Jan 1, 2025

Merge branch 'dev' into sensorRefactor

033cac8

cdnninja changed the title ~~Refactor switch sensor so we can use it for more.~~ Refactor switch sensor for vesync Jan 1, 2025

cdnninja changed the title ~~Refactor switch sensor for vesync~~ Refactor switch for vesync Jan 2, 2025

cdnninja added 6 commits January 2, 2025 05:48

Adjust order so all platforms match between PRs

3a663de

Correct type

6604d26

Attempt to fix mypy error

f4352fd

Correct mypy erros.

f143594

Correct Errors

1337372

Add callbacks

7b0510d

cdnninja mentioned this pull request Jan 3, 2025

Add humidifier entity for Vesync devices #134333

Merged

19 tasks

cdnninja added 2 commits January 3, 2025 11:56

Merge branch 'dev' into sensorRefactor

8db10fb

Add coordinator as per other PR

0ce2086

cdnninja marked this pull request as ready for review January 4, 2025 00:43

cdnninja added 3 commits January 5, 2025 10:33

Merge branch 'dev' into sensorRefactor

0fc9e81

Merge issue

4a2fa3f

ruff

0288768

joostlek requested changes Jan 6, 2025

View reviewed changes

cdnninja added 3 commits January 15, 2025 22:16

Remove unused code to increase test coverage

139d4e1

switch to async updates and refresh

10e9925

Revert last change

8b38c47

joostlek requested changes Jan 19, 2025

View reviewed changes

home-assistant bot marked this pull request as draft January 19, 2025 10:41

cdnninja and others added 3 commits January 19, 2025 08:25

Update homeassistant/components/vesync/common.py

85fdfdd

Co-authored-by: Joost Lekkerkerker <[email protected]>

Update homeassistant/components/vesync/switch.py

70e2cdf

Co-authored-by: Joost Lekkerkerker <[email protected]>

Feedback Items

c6d2088

cdnninja marked this pull request as ready for review January 19, 2025 16:03

home-assistant bot requested a review from joostlek January 19, 2025 16:03

Improved comments.

27c255e

iprak suggested changes Jan 25, 2025

View reviewed changes

feedback elif

1013880

Feedback

51d1ead

joostlek requested changes Jan 28, 2025

View reviewed changes

home-assistant bot marked this pull request as draft January 28, 2025 16:37

Add unique ID

d963ec7

cdnninja mentioned this pull request Feb 1, 2025

Migrate unique ID in vesync switches #137099

Merged

19 tasks

Merge branch 'dev' into sensorRefactor

b39778e

cdnninja marked this pull request as ready for review February 3, 2025 15:49

home-assistant bot requested review from joostlek and iprak February 3, 2025 15:49

iprak suggested changes Feb 3, 2025

View reviewed changes

If for on / off

bde6507

iprak approved these changes Feb 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor switch for vesync #134409

Refactor switch for vesync #134409

cdnninja commented Jan 1, 2025 •

edited

Loading

home-assistant bot commented Jan 1, 2025

cdnninja commented Jan 1, 2025

cdnninja commented Jan 1, 2025

cdnninja commented Jan 4, 2025

iprak commented Jan 4, 2025

cdnninja commented Jan 4, 2025

joostlek Jan 19, 2025

cdnninja Jan 19, 2025

iprak Jan 19, 2025

cdnninja Jan 19, 2025 •

edited

Loading

joostlek Jan 19, 2025

cdnninja Jan 19, 2025

iprak left a comment

iprak Jan 25, 2025

cdnninja Jan 25, 2025

iprak Jan 26, 2025

joostlek Jan 28, 2025

cdnninja Jan 29, 2025

iprak Jan 25, 2025

cdnninja Jan 25, 2025

iprak Jan 25, 2025 •

edited

Loading

iprak Feb 3, 2025

cdnninja Feb 3, 2025

iprak Jan 25, 2025

cdnninja commented Jan 25, 2025

iprak commented Jan 25, 2025

cdnninja commented Jan 25, 2025

joostlek Jan 28, 2025

cdnninja commented Jan 29, 2025

cdnninja commented Feb 1, 2025

iprak Feb 3, 2025

iprak Feb 3, 2025

		# Other types of wall switches support dimming. Those use light.py platform.
		exists_fn=lambda device: is_wall_switch(device) or is_outlet(device),

Refactor switch for vesync #134409

Are you sure you want to change the base?

Refactor switch for vesync #134409

Conversation

cdnninja commented Jan 1, 2025 • edited Loading

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Jan 1, 2025

cdnninja commented Jan 1, 2025

cdnninja commented Jan 1, 2025

cdnninja commented Jan 4, 2025

iprak commented Jan 4, 2025

cdnninja commented Jan 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdnninja Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iprak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iprak Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdnninja commented Jan 25, 2025

iprak commented Jan 25, 2025

cdnninja commented Jan 25, 2025

Choose a reason for hiding this comment

cdnninja commented Jan 29, 2025

cdnninja commented Feb 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdnninja commented Jan 1, 2025 •

edited

Loading

cdnninja Jan 19, 2025 •

edited

Loading

iprak Jan 25, 2025 •

edited

Loading