-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
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 the Xiaomi TV platform. #12359
Add the Xiaomi TV platform. #12359
Conversation
# No host is needed for configuration, however it can be set. | ||
PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({ | ||
vol.Optional(CONF_HOST): cv.string, | ||
vol.Optional(CONF_NAME): cv.string, |
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.
Add , default="Xiaomi TV"
Then you do not need the if
check on line 46
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 like that much better. Very elegant. Will update right away.
@property | ||
def state(self): | ||
"""Return _state variable, containing the appropriate constant.""" | ||
return self._state |
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.
Is it not possible to get the state from the TV?
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.
No, and that is the main limitation of this platform. However, as it stands there's two options, when controlling power on the TV:
- Currently, the TV sleeps and wakes up. This makes for instant turn on and off, however, it sacrifices ability to know state.
- Another option, is to turn off the TV properly, then we would know the state for sure, but you would't be able to turn it on again. When user then manually turns the TV back on with the remote, it takes a couple of minutes.
I thought the first option sounded better.
class XiaomiTV(MediaPlayerDevice): | ||
"""Represent the Xiaomi TV for Home Assistant.""" | ||
|
||
def __init__(self, ip, name=DEFAULT_TV): |
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.
undefined name 'DEFAULT_TV'
|
||
REQUIREMENTS = ['pymitv==1.0.0'] | ||
|
||
DEFAULT_NAME="Xiaomi TV" |
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.
missing whitespace around operator
|
||
@property | ||
def state(self): | ||
"""Return _state variable, containing the appropriate constant.""" |
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.
lol
Add that it's an assumed state and it's ok to merge:
|
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.
Some small things that can be improved in a future PR.
Add support for the Xiaomi TVs. | ||
|
||
For more details about this platform, please refer to the documentation at | ||
https://home-assistant.io/components/xiaomi_tv/ |
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 url is wrong. Look at an existing platform to see the correct format.
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.
Right yes, I had absolutely missed that. The correct url would be https://home-assistant.io/components/media_player.xiaomi_tv/
.
""" | ||
|
||
import logging | ||
import voluptuous as vol |
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.
Add a blank line between standard library and 3rd party imports.
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.
Just looked at another platform, and you're absolutely right. Will update in next pull request.
Description:
This pull request adds the platform Xiaomi TV, implementing the pymitv Python library.
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4653
Example entry for
configuration.yaml
(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
tox
run successfully. Your PR cannot be merged unless tests passREQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.