-
-
Notifications
You must be signed in to change notification settings - Fork 32.7k
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
Integrate VeSync humidifiers #62907
Integrate VeSync humidifiers #62907
Conversation
Hey there @markperdue, @webdjoe, @TheGardenMonkey, mind taking a look at this pull request as it has been labeled with an integration ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! I have an open PR for this component, so I tried to note places where we can align to avoid merge conflicts later on.
Considering there is another open PR for this integration (#61837), I'm going to wait a while before making the requested changes. I'm hoping that the other PR is merged soon so that I can rebase and then make changes, since that should be easier than trying to make the merge clean by eye and then still having to do some work when I rebase. |
f06f597
to
3c36df6
Compare
Other PR was merged, so I rebased and made the recommended changes. Still need to update my docs PR. |
3c36df6
to
aa5b178
Compare
Updated docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny nit on a doc comment but otherwise LGTM. Thanks for waiting on my other PR and integrating really nicely with those changes.
Co-authored-by: Jonathan Keslin <[email protected]>
Fixed. Thanks for reviewing! |
"Core200S": "fan", | ||
"Core300S": "fan", | ||
"Core400S": "fan", | ||
"Classic300S": "humidifier", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could also add "Dual200S": "humidifier", to this list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it looks like the master branch of pyvesync has Dual200S support, the PR for that was just merged two weeks ago (webdjoe/pyvesync#104), so it's not in the latest release, which is from November. This should be possible after the next pyvesync release, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have found my Humidifier, ostensibly a Dual200S is reporting as "LUH-D301S-WEU" so it would be good to add that model as well.
From pyvesync V2.0.0
;
{
"Device Name": "Study Hum",
"Model": "LUH-D301S-WEU",
"Subdevice No": "None",
"Status": "on",
"Online": "online",
"Type": "wifi-air",
"CID": "vsaqxxxxxxxxxxxxxxxxxxxxxxx",
"Mode": "auto",
"Humidity": "40",
"Mist Virtual Level": "1",
"Mist Level": "1",
"Water Lacks": false,
"Humidity High": false,
"Water Tank Lifted": false,
"Display": true,
"Automatic Stop Reach Target": true,
"Auto Target Humidity": "42",
"Automatic Stop": true
}
Because pyvesync
just returns the model
, every possible model as listed here in each group probably needs to be included.
What does this PR need to be merged? Anything I can do to help? |
I think it just needs review by a maintainer. This is my first Home Assistant PR, so I'm not completely sure how long that normally takes or what the process is like. |
The maintainers often seem to review PRs in bursts, often right before a new release. So I'd expect reviews next week ahead of the 2/2 release. |
https://developers.home-assistant.io/docs/core/entity/number If the difference is one, the step will be 0.1. @jspayd, @vlebourl has done a whole load of work on this via custom integration which started from this PR. You might want to take a look. https://github.com/vlebourl/custom_vesync Also, the Python library has been bumped to fix a couple of bugs. |
Heya! Just jumping in with another “this is gonna be awesome!” Any chance of this making it into the next release? |
You can install an updated version via HACS (will just work instead of the core version). https://github.com/vlebourl/custom_vesync HACS version will work better than the core version, even when this PR eventually gets released as this is still missing functionality. |
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Bump |
What's left for this to be merged? (apart from fixing the conflicts). I would really benefit from it (and @jspayd put a whole lot of effort into it). |
Don't hold your breath. I'd just use the custom integration https://github.com/vlebourl/custom_vesync |
I am going to try bump this back up. @jspayd Any chance you can resolve the merge conflicts as a start? I can give this a test soon to confirm still working. |
I'm looking for some help, I don't have a strong git background, and I'm not sure how to proceed. I was following an article I found to add to someone else's pull request (https://tighten.com/insights/adding-commits-to-a-pull-request/) and did a checkout of jspayd's branch. I then followed the steps described at https://developers.home-assistant.io/docs/development_catching_up/ for rebasing. That branch was MANY months out-of-date and there were a number of conflicts and underlying code improvements. I tried the final step of pushing the rebased changes to jspayd's branch but received permission denied.
I now have my own branch (https://github.com/chuckdeal97/hass-core/tree/jspayd-vesync-humidifier) that I am not sure how to use to push this PR ahead. |
You can't push to her branch but you can set the base of your PR to her branch and open it against her fork. Or you can just open a fresh PR against the upstream (here) and note the PR number here so the maintainers can close this one if appropriate. |
I'd recommend closing this PR in favor of a newer set of PRs that have been opened to cover the same functionality (#90455) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are merge conflicts that need resolving.
Please rebase or consider closing in favor of #90455
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Breaking change
Proposed change
night_light
property.Type of change
Additional information
Checklist
black --fast homeassistant tests
)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. - This is the latest changelog message. Most of the updates don't have changelog messages.
Untested files have been added to
.coveragerc
.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: