-
-
Notifications
You must be signed in to change notification settings - Fork 28.6k
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 Xiaomi Air Purifier 3C #47924
Add Xiaomi Air Purifier 3C #47924
Conversation
Hi @abrilevskiy, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
Hey there @rytilahti, @syssi, @starkillerOG, mind taking a look at this pull request as its been labeled with an integration ( |
Looks good to me, not sure why the tests fail |
@starkillerOG , all tests are finally gets passed now. |
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
I'm testing the integration by running it as a custom component. I'm not able to change the speed of the 3C's fan in manual mode, and I can't see the attributes (like air quality ecc.). |
@notherealmarco, I also can not change the speed, but as far as I understand - it is a separate issue. Please let me know otherwise.
aqi=None - is due to filtering "1" Could you please check python-miio version (miiocli --version)? |
I'm running version 0.5.5 (as declared in the manifest.json) Don't know if this can help, but when it loads the fan entity, it throws an exception, here's the stacktrace:
|
@abrilevskiy that's what I have: And this is the configuration (I also triend setting it up from the UI):
Sould I try running the component by replacing the HA files instead of running it as a custom component? |
Config file should not be used - wizard is implemented in latest release. |
Tried both, but the result is the same. The fan entity still doesn't have all the attributes |
@notherealmarco , Could you please provide logs?
|
These are the logs, I have set the logging level to debug (
Seems to be using the right class. The attributer are there in the logs, but not on the entity in HA. I also tried miiocli (in the same container), and it shows well all the attributes. |
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 AVAILABLE_ATTRIBUTES_AIRFRESH
attributes also seem to contain some measurements that should be sensor entities and not fan attributes, eg temperature, aqi, average_aqi, co2, humidity. It would be good to move them too.
ATTR_TEMPERATURE: "temperature", | ||
ATTR_HUMIDITY: "humidity", |
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.
Temperature and humidity should also be separate sensor entities. Please move these as well.
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.
This Air Purifier model (3C) does not have neither temperature nor humidity
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.
@MartinHjelmare this comment seems unrelated to the pull request, which simply aims to add support for the 3C model.
The temperature and humidity values are already listed as attributes in the original home-assistant repo: https://github.com/home-assistant/core/blob/dev/homeassistant/components/xiaomi_miio/fan.py#L167
Could you open a separate issue to aim to improve this by splitting out the mentioned attributes as sensor entities?
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 current code is already scoped beyond adding support for the 3C model, which you can see if you read the sensor platform additions. If we only want that scope, we'll need to slim that part down.
This comment has been minimized.
This comment has been minimized.
@abrilevskiy do you need help with this PR? |
Any help needed? |
Can I help out with this? What remains to be done to approve this PR? I am waiting for this integration to control my 3C model. The PR has been open for more than 2 months now. |
The comments need to be addressed. |
I'm sorry, I'm preparing 4 exams for the university and i have no time right now to finish addressing the issues. I will continue in July, when the exam session will be over |
…n.py Co-authored-by: Kasper Malfroid <kasper@malfroid.be>
Other PRs have overtaken this PR and this PR is no longer correct. We have moved the humidifiers to a new humidifier platform in #51884. And in #51791 we have implemented percentage based fan control. I suggest we close this PR and take a new look at adding any remaining missing device support after the other work is merged. |
@MartinHjelmare Is humidifier platform will be correct for this device? This device is purifier and not humidifier, it has only fan control and pm2.5 sensor. |
No this new device should still be a fan and a sensor entity. Currently though this PR has other changes that are in conflict with the ongoing restructuring work. So at minimum we'll need to rebase here when that work is done. |
@MartinHjelmare FWIW, this PR seems to have a better implementation of fan speed percentage than the already merged #51791. This PR uses the motor speed instead of the 'fan' preset and associated 3 speed presets, so it enables finer control of fan speed. @notherealmarco can you confirm that your implementation enables more points (300-1200rpm mapped to 0-100%) in your testing? |
See #52173. P.S. Of course motor speed could be in the Fan platform instead of the build in modes, but that would not be compatible with what the uses sees on the indicators on the device. I would prefer to use the build in modes only and a additional |
I see your rationale makes sense but how many people even look at their air purifiers anymore. The The idea is to automate the fan speed and the finer control is more useful. Using the service approach means homeassistant will not display or change service-set fan speed in UI. I think this is a bigger issue that users already face (with existing favorite levels). Mapping favorite speeds to fan speed percentage will make home assistant more consistent (fan speed set via service in automation will match HASS UI fan speed slider and further integrations like Homekit also). I think we should prioritize this as defining your own custom handling of fan speed is one of the key benefits of using HASS to control air purifiers. You can use automations to override factory handling of fan speeds in response to AQI and just 3 steps doesn't make this happen. So users will have to use the service and deal with broken fan speeds in UI and beyond, again. |
newbie. how to add 3c to HA step by step please. |
This PR seems to have been abandoned. Maybe somebody can take over and finish it? |
Hi, I can help but I don't have this model at home so... please contact me on Discord (Bieniu#4759) or Messenger (Maciej Bieniek). |
@syssi @starkillerOG @rytilahti Could some of you please check this case? Not trying to push you, just wondering if anyone is working on it or it is forgotten. Maybe @arturdobo or @abrilevskiy can help if there is no capacity for development (found you guys in related issues/PRs, sorry for tagging if you can't help here) I have this model if you need help testing. |
@r4ian I would love to help, but I don't own a xiaomi air purifier, so it is hard for me to develop and test code for it. |
Breaking change
Proposed change
Type of change
Example entry for
configuration.yaml
:# Example configuration.yaml
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
.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: