-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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 breeze switch to Renson integration #101641
Conversation
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
@jimmyd-be Please try to better explain what this PR does. Is it in reality a single renson device, which you want to model as two fan entities? If that's the case, why is that? |
I added on the original version of this component (when it was not yet home-assistant/core) the breeze feature with a switch. https://github.com/krmarien/Home-Assistant-Sensor-Renson-Ventilation/tree/breeze To me that makes more sense as this is still one physical device (fan), it will just toggle some valves if I'm right. |
I'm marking this PR as draft since there are open questions about the implementation. |
This thread contains the most info #101641 (comment) |
I first used your implementation but after first review it was not OK for the comunity. And changed after that to the current implementation. |
It is a single device. And the device can push air from outside to inside and visa versa. The breeze function is for pushing outside air into the home for cooling. The normal fan is for filtering the air and with a temperature value to retain the temperature inside the house. |
sorry @jimmyd-be, I didn't realize the recommendation to model the breeze mode as an additional fan came from review comments. |
No problem, what do I need to change now? Keep this implementation or revert back to the switch and fan implementation? |
@jimmyd-be I'll check with @joostlek and a few others. |
Just had a quick chat with Erik, and I want to apologise for the wrong recommendation. Back then I didn't know the things I know today. I think having 1 fan entity like we already had and then have a switch or select to switch the mode would be the best. |
This reverts commit 4fc1ac22e67091a7e980aefd217652f2a88bed17.
@jimmyd-be please update the title and the description of the PR to match the implementation |
Update the PR title and description. Reverted the code back to the previous commit. |
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.
Looks good to me, just one question.
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.
LGTM, thanks @jimmyd-be 👍
Breaking change
Proposed change
Added breeze switch to manually switch to breeze mode of the ventilcation unit.
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:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: