-
Notifications
You must be signed in to change notification settings - Fork 36
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 default platform for xiaomi_miio HA core integration #66
Add default platform for xiaomi_miio HA core integration #66
Conversation
@ikaruswill what's your recommended way to test this card (with more than one device)? I've tried wherever possible to leave the old platforms untouched, and to always handle when related entities don't exist, but it'd be great to try this out on a few other devices. |
@ian-craig Thanks so much for taking the time to make this PR. Your changes are much welcome! About testing, to be honest, there's no easy way to test this out. What we can do however is to release beta versions suffixed with What I'd suggest is:
What do you think? |
Out of curiosity, what model of Xiaomi fan are you running? |
Awesome work! I have 2 dmaker_fan_p18 fans, and I'm willing to try a beta version. |
@lassieee Nice! Thanks for volunteering, we have 1 tester! I'm running the |
p5 model here, can test -beta before summer heat comes |
Great to see so much community support and people volunteering to test. I have a I've created 4 smaller PRs which cover the minor unrelated changes (#67, #68, #70), and an initial refactor (#69), but don't actually add the new |
Thanks for your work, gonna get down to reviewing them now! |
@ian-craig LGTM, the most appreciated change imo is the trick that can allow support for any fan models without the need to hard-code model versions. Could you resolve the merge conflicts? Once that's done we can put up a |
fan-xiaomi.js
Outdated
@@ -743,7 +848,7 @@ LED | |||
|
|||
// LED | |||
let activeElement = fanboxa.querySelector('.c3') | |||
if (led_brightness < 2) { | |||
if (led_brightness > 0) { |
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.
@ikaruswill This didn't make sense to me. I had intepreted "brightness" to be a percentage, but does led_brightness
mean something different when coming from attributes via the xiaomi_miio_fan 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.
Bsaed on what I know, brightness isn't a percentage with the xiaomi_miio_fan
integration:
Supported values are 0 (Bright), 1 (Dim), 2 (Off).
Even with the official integration, the brightness is (annoyingly) either Number
entities or Select
Entities depending on models
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.
Ugh. Okay, I've updated the PR so that it's only using > 0
when using the number.*
entity, and otherwise using < 2
on the attribute.
To support other models through the default
platform fully we'd need to also support select.*
brightness entities, but without an example of that I'm not sure I'd implement it correctly.
If anyone with other fans can share their list of entities and the attributes for each I'd be happy to have a go at broadening support.
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 the following entities for my zhimi.fan.za5
(Pardon the cute name)
And the following attributes for fan device:
speed_list:
- 'off'
- low
- medium
- high
- Nature
- Normal
preset_modes:
- Nature
- Normal
direction: null
oscillating: false
speed: Normal
percentage: null
percentage_step: 1
preset_mode: Normal
friendly_name: Gusty
supported_features: 15
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 don't have my za4 connected and accessible right now, but based on what I remember, the LED brightness is set with a select entity, with bright
, dim
, off
options.
It's corroborates with the HA docs:
Controls the brightness of the LEDs (bright, dim, off)
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 pushed an update to add a select entity... though I can't really test this. Do you think this is sufficient to push out a beta? I expect there will be some models the LED state doesn't show for, but until we get some more data we can't do much about that.
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.
Yeah I think it's all good for us to push out the beta by now. Will do so within the day. Thanks for the help with supporting the select entity for the LED.
Looks like a bug in that last minute change I did for the select entity. I'll send a PR shortly. |
Home Assistant's core Xiaomi Miio integration now creates related entities for sensors and controls which aren't part of the default "fan" service. With this, it's possible to fully control a Xiaomi fan without the
xiaomi_miio_fan
service.This PR adds a new
default
platform config which uses the built infan
service and related entities. Finding the related entities requires an async call, so there's a decent amount of refactoring to make sure we're configured before rendering fully.I've also added support for a few newer attributes.
Full list of changes:
set hass
getter into config/create/update functions, and ensure we aren't re-creating the entire card unnecessarily one very state changethis.config.platform === 'default'
which checks for features by looking for the relevant entities (no need to hard-code model versions anymore)The diff for this is pretty huge, sorry about that. I can try to split it up if you'd prefer.
Related issues: #64 #61