Skip to content
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

Merged
merged 4 commits into from Feb 16, 2018
Merged

Add the Xiaomi TV platform. #12359

merged 4 commits into from Feb 16, 2018

Conversation

simse
Copy link
Contributor

@simse simse commented Feb 12, 2018

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):

media_player:
  - platform: xiaomi_tv
    host: 192.168.0.41 # Optional
    name: Mi TV 3s # Optional

Checklist:

  • The code change is tested and works locally.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

# 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,
Copy link
Member

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

Copy link
Contributor Author

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
Copy link
Member

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?

Copy link
Contributor Author

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):

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"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing whitespace around operator

@simse simse changed the title Added the Xiaomi TV platform. Add the Xiaomi TV platform. Feb 13, 2018

@property
def state(self):
"""Return _state variable, containing the appropriate constant."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol

@balloob
Copy link
Member

balloob commented Feb 16, 2018

Add that it's an assumed state and it's ok to merge:

    @property
    def assumed_state(self) -> bool:
        """Return True if unable to access real state of the entity."""
        return True

@balloob balloob merged commit dd7bffc into home-assistant:dev Feb 16, 2018
Copy link
Member

@MartinHjelmare MartinHjelmare left a 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/
Copy link
Member

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.

Copy link
Contributor Author

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
Copy link
Member

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.

Copy link
Contributor Author

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.

@balloob balloob mentioned this pull request Feb 22, 2018
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants