Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 support for VeSync Fans #36132
Add support for VeSync Fans #36132
Changes from 14 commits
79cbada
9dacf26
24b0cba
297656c
1df6daa
f08e003
1dce26f
a760325
ac77241
5dbf5ec
c982180
f3123b1
cebf990
a807b25
33f56fb
9d0ca91
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
async_discover
could be a callback since we don't await inside.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.
Nothing is checking this return value.
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.
Since the fan can still return
SPEED_AUTO
, and we have taken it out ofFAN_SPEEDS
this puts us in an invalid state. Unfortunately it doesn't look like we can tell what speed the fan is actually running it so it leaves us in a position where violating the spec might make more sense since auto can be set from outside Home Assistant.I think its ok to add back in as long as we refactor after home-assistant/architecture#127 resolves this.
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.
Sounds good and yes I will refactor when a decision is made.
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.
It's not ok to violate our architecture design until the architecture has been changed. Please remove the auto speed.
We can return
None
in that case.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.
Air quality should be a separate sensor entity if it means a measurement of air quality and not a fan setting.
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.
The 'fan' reports an air quality which ranges between bad to excellent, so just want to clarify if that makes sense and is still OK to keep as a sensor. I am assuming yes.
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.
It makes sense to make it a sensor in this case 👍
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.
Remove this case.