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

Add humidifier entity for Vesync devices #134333

Merged
merged 28 commits into from
Jan 13, 2025

Conversation

iprak
Copy link
Contributor

@iprak iprak commented Dec 31, 2024

Proposed change

This request introduces humidifier entities (and related sensors) for Vesync devices.

image

This is not a complete representation of the humidifier. The humidifier exposes some binary states and allows number mist level to be set. The former is being introduced in #134221 and I will introduce the latter in another PR.

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

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:

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:

@home-assistant
Copy link

Hey there @markperdue, @webdjoe, @TheGardenMonkey, @cdnninja, 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.

Copy link
Contributor

@cdnninja cdnninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on us shifting to checking the class used by the library vs keeping a full list of models? It seems a key pain point is sometimes the underlying library supports a variant and we miss adding it.

I was hoping for two approaches. 1. When can just validate the value exists use that. 2. If needing to know for sure if it's a type like fan or humidifier check the class.

@iprak
Copy link
Contributor Author

iprak commented Dec 31, 2024

Thoughts on us shifting to checking the class used by the library vs keeping a full list of models? It seems a key pain point is sometimes the underlying library supports a variant and we miss adding it.

I was hoping for two approaches. 1. When can just validate the value exists use that. 2. If needing to know for sure if it's a type like fan or humidifier check the class.

I think you are suggesting checking device.type against VeSyncHumid200S or VeSyncSuperior6000S or VeSyncHumid1000S in is_humidifier ? That does sound better.

I do see there is this humidifier VeSyncSuperior6000S which does not derive from VeSyncHumid200300S and so would its own handling.

@cdnninja
Copy link
Contributor

Something along those lines yes. Also for other purposes. I am finding a key challenge with the way it is written today is it depends on the vesync library stating something is a "fan" or "switch" when a platform of switch in HA doesn't align, switch can be used for many things, including screens, power, modes etc. So the platforms themselves need an easy way to check support. I think the binary sensor code does that for most values reasonable well, just need a way to check methods in addition.

For fan and humidifier I feel like a method like you mentioned above makes sense. Check if it is one of the classes. That way variants are covered without us mapping everything.

If interested could you join the HA discord so we can message on this? Love the work you are doing!

Ps I have a refactored draft of switch ready as well. I am waiting for items to be merged though since lots outstanding on that front.

@iprak
Copy link
Contributor Author

iprak commented Jan 1, 2025

I have adjusted the is_humidifier function and it works well. Seems the addition of humidifier has lowered the test coverage .. so I will write some more tests to get it above 80%

@iprak iprak force-pushed the add_humidifier_entity branch from 3b0ace5 to adca316 Compare January 1, 2025 04:09
@cdnninja
Copy link
Contributor

cdnninja commented Jan 1, 2025

I agree with #134166 (review) joostlek comment that we should move our platform checks within the platform. It would greatly simplify some of the code. Thinking as we add new platforms we should do that.

As well do you see any value in the discovery call back? I don't think it will be used, just seems to complicate things?

@iprak
Copy link
Contributor Author

iprak commented Jan 1, 2025

I agree with #134166 (review) joostlek comment that we should move our platform checks within the platform. It would greatly simplify some of the code. Thinking as we add new platforms we should do that.

As well do you see any value in the discovery call back? I don't think it will be used, just seems to complicate things?

The discovery service would only be used if one add/removes devices to the account. It feels more intuitive to "Reload" the integration which should update the devices or would that not remove the older entities? Maybe that is still okay, one can remove them manually.

@cdnninja
Copy link
Contributor

cdnninja commented Jan 1, 2025

Correct but with the coordinator change we only call updates to the devices so we will never see a new device?

@iprak
Copy link
Contributor Author

iprak commented Jan 1, 2025

Correct but with the coordinator change we only call updates to the devices so we will never see a new device?

Yes and I think that is okay. Maybe that is why all integrations have the reload option. I have not seen or used an integration which queries for device update but my integration usage is limited.

@cdnninja
Copy link
Contributor

cdnninja commented Jan 1, 2025

Some do, I agree reload makes more sense here.

@cdnninja
Copy link
Contributor

cdnninja commented Jan 1, 2025

@home-assistant add-label new-feature

@cdnninja
Copy link
Contributor

cdnninja commented Jan 3, 2025

Quick note in another PR they wanted to keep the callbacks and potentially add a scheduled device list update. So for this we should keep it in. Eventually we can add a PR to check for devices from time to time. I am thinking this can be re-based and go for review? I don't think it actually depends on the binary_sensor beyond potential test conflicts which could be quickly updated once binary is merged?

@cdnninja
Copy link
Contributor

cdnninja commented Jan 3, 2025

I also wonder if switch changes should be removed from this to make it easier to get across the finish line. Then we can add those features after #134409 gets merged.

@iprak
Copy link
Contributor Author

iprak commented Jan 3, 2025

I also wonder if switch changes should be removed from this to make it easier to get across the finish line. Then we can add those features after #134409 gets merged.

That is a good idea. I will keep humidifier simple for now and then add sensor/switches later.

@iprak iprak force-pushed the add_humidifier_entity branch from adca316 to db8cfe0 Compare January 5, 2025 03:21
@iprak iprak marked this pull request as ready for review January 5, 2025 03:50
@iprak
Copy link
Contributor Author

iprak commented Jan 5, 2025

This is ready for review but there is this check "Has at least one of the required labels (breaking-change, bugfix, code-quality, dependency, deprecation, new-feature, new-integration)" which is failing and I am not sure what I need to do to address that.

The documentation PR only mentions 2 humidifiers. I can only verify the functionality on the model I have.

@joostlek Would you be able to review this too?

@iprak
Copy link
Contributor Author

iprak commented Jan 5, 2025

@home-assistant add-label new-feature

@iprak iprak requested a review from cdnninja January 5, 2025 03:56
homeassistant/components/vesync/humidifier.py Outdated Show resolved Hide resolved
homeassistant/components/vesync/switch.py Outdated Show resolved Hide resolved
homeassistant/components/vesync/sensor.py Outdated Show resolved Hide resolved
@iprak iprak force-pushed the add_humidifier_entity branch from c130e59 to 8a157ed Compare January 5, 2025 20:03
@iprak iprak force-pushed the add_humidifier_entity branch from 277acee to 8967f55 Compare January 10, 2025 13:45
@iprak
Copy link
Contributor Author

iprak commented Jan 10, 2025

Resolved conflicts

@iprak iprak marked this pull request as ready for review January 10, 2025 13:50
homeassistant/components/vesync/humidifier.py Outdated Show resolved Hide resolved
homeassistant/components/vesync/humidifier.py Outdated Show resolved Hide resolved
homeassistant/components/vesync/humidifier.py Outdated Show resolved Hide resolved
homeassistant/components/vesync/humidifier.py Outdated Show resolved Hide resolved
homeassistant/components/vesync/humidifier.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft January 10, 2025 13:54
@iprak iprak marked this pull request as ready for review January 11, 2025 01:41
@home-assistant home-assistant bot requested a review from joostlek January 11, 2025 01:41
@iprak iprak requested a review from RSully January 13, 2025 16:38
@joostlek joostlek merged commit d986fe7 into home-assistant:dev Jan 13, 2025
34 checks passed
@neilime
Copy link

neilime commented Jan 13, 2025

Thank you for your work guys! I can't wait to use it for my installation

@joostlek joostlek added the noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) label Jan 13, 2025
@cdnninja
Copy link
Contributor

Amazing work! This one has been an ask for many years from the community so very excited it is done!

@github-actions github-actions bot locked and limited conversation to collaborators Jan 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed has-tests integration: vesync new-feature new-platform noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) Quality Scale: No score
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants