python-miio remote. #11891

cnrd · 2018-01-24T05:02:37Z

Description:

Original PR: #11856

I ended up changing the code so much that I'm pretty sure it warrants a new PR.

This implements the ChuangmiIr device from python-miio as a remote device.

It adds a new service called: remote.xiaomi_miio_learn_command which takes an entity_id of the device to learn commands from.

A service call to remote.send_command can either be one of the commands from config by name or a base64 command directly.

The reason for defaulting to hide the entity, is that it really just acts as a "bridge" to send commands from, so there is no reason to expose it in the UI by default.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4513

Example entry for `configuration.yaml` (if applicable):

remote:
  - platform: xiaomi_miio
    name: "some nice name" (Optional)
    host: [ip]
    token: [token]
    slot: 1 (Optional)
    timeout: 30 (Optional)
    hidden: false (Optional)
    commands: (Optional)
      some_command_callable_from_remote.send_command:
        command:
          - [base64 encoded string]
      some_other_command_callable_from_remote.send_command:
        command:
          - [base64 encoded string]
          - [base64 encoded string]

You could then create an actual UI button in this way:

script:
  some_command:
    sequence:
      - service: remote.send_command
        entity_id: 'remote.some_nice_name'
        data:
          command:
            - 'some_command_callable_from_remote.send_command'

This allows us to save arbitrary commands as part of the remote and give them easy, readable names.

Checklist:

The code change is tested and works locally.

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

Documentation added/updated in home-assistant.github.io

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.

cnrd · 2018-01-24T05:06:29Z

@syssi @rytilahti If you could take a look at this, that would be really great!
I'm pretty sure that I implemented everything that you asked for.

cnrd · 2018-01-24T06:35:17Z

I'm not really sure why the build is failing, it works when running tox locally and Travis is failing on unrelated stuff.

EDIT: Looks like a rebuild fixed it.

syssi · 2018-01-24T06:44:23Z

homeassistant/components/remote/xiaomi_miio.py

+_LOGGER = logging.getLogger(__name__)
+
+SERVICE_LEARN = 'xiaomi_miio_learn_command'
+SERVICE_SEND = 'xiaomi_miio_send_command'


It's unused. Please remove the constant.

syssi · 2018-01-24T06:46:14Z

homeassistant/components/remote/xiaomi_miio.py

+
+SERVICE_LEARN = 'xiaomi_miio_learn_command'
+SERVICE_SEND = 'xiaomi_miio_send_command'
+PLATFORM = 'python_miio'


I think the platform "xiaomi_miio" should be used here.

syssi · 2018-01-24T06:46:54Z

homeassistant/components/remote/xiaomi_miio.py

+CONF_SLOT = 'slot'
+CONF_COMMANDS = 'commands'
+
+DEFAULT_TIMEOUT = 10


These two constants aren't used anymore, too.

They are now, forgot to use them in the correct place.

syssi · 2018-01-24T06:53:38Z

homeassistant/components/remote/xiaomi_miio.py

+            return False
+
+    @asyncio.coroutine
+    def async_send_command(self, command, **kwargs):


Could you implement the complete feature set / kwargs here? cp. https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/remote/harmony.py#L209-L222

device is not required by the current implementation, so how would I handle that? (I'm not even sure what device should do for this device. I can implement the other two, though.

Don't handle / ignore the device.

syssi · 2018-01-24T06:56:26Z

homeassistant/components/remote/xiaomi_miio.py

+})
+
+COMMAND_SCHEMA = vol.Schema({
+    vol.Required(CONF_COMMAND): cv.ensure_list


vol.Required(CONF_COMMAND): vol.All(cv.ensure_list, [cv.string])

syssi · 2018-01-24T07:06:57Z

homeassistant/components/remote/xiaomi_miio.py

+        self._timeout = timeout
+        self._state = False
+        self._commands = commands
+


Please add a should_poll method. Just to make clear it's a polling entity or not.

syssi · 2018-01-24T07:34:39Z

homeassistant/components/remote/xiaomi_miio.py

+    @asyncio.coroutine
+    def _learn_command(call):
+        """Handle a learn command."""
+        entity_id = call.data.get('entity_id').split('.')[1]


Thanks for cleaning up the service name! I'm still unhappy about the format and handling of entity_ids. Could you implement a service handler? https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/remote/__init__.py#L138-L172

cnrd · 2018-01-24T08:56:02Z

@syssi Not really sure if the async_service_handler is correct, but no more hacking up the entity_ids

cnrd · 2018-01-24T16:31:24Z

@syssi @rytilahti anything else you want changed? If not, then I'll start writing the docs.

rytilahti

Added a few points that I think could use some revising. Otherwise I think it's fine to start writing the doc for this. Someone more knowledgeable on remote will hopefully chime in sooner or later.

rytilahti · 2018-01-24T22:10:19Z

homeassistant/components/remote/services.yaml

+      example: 'remote.xiaomi_miio'
+    slot:
+      description: 'Define the slot used to save the IR command (Value from 1 to 1000000) (Default: 1)'
+      example: '"slot": 1'


I think this should be just '1'. Same for the timeout below, these are supposed to be values you can input for these fields.

rytilahti · 2018-01-24T22:11:41Z

homeassistant/components/remote/xiaomi_miio.py

+    # This should be DeviceError but python-miio returns wrong except.
+    except ChecksumError as ex:
+        _LOGGER.error("Token not accepted by device : %s", ex)
+        return


This will be fixed in the next python-miio release, so before this gets merged in I hope we'll have that released and you can bump the version to 0.3.5 and change this :)

rytilahti · 2018-01-24T22:13:39Z

homeassistant/components/remote/xiaomi_miio.py

+
+        entity_id = service.data.get(ATTR_ENTITY_ID)
+
+        entity = None


I think there's no need for a newline between these two, but it may be a matter a taste.

rytilahti · 2018-01-24T22:14:42Z

homeassistant/components/remote/xiaomi_miio.py

+                    message['error']['message'] == "learn timeout"):
+                yield from hass.async_add_job(device.learn, slot)
+            yield from asyncio.sleep(1, loop=hass.loop)
+        _LOGGER.error("Timeout. No infrared command captured")


For this while block I'd on the other hand insert a a newline or two to make it easier to read.

rytilahti · 2018-01-24T22:17:15Z

homeassistant/components/remote/xiaomi_miio.py

+
+    @asyncio.coroutine
+    def async_turn_on(self, **kwargs):
+        """Turn the device on."""


I'd add pass after the comment to make it explicit that these are not supposed to do anything. Or even better, implement them properly / add a logger warning when someone tries to call them.

At the moment someone converting from another remote platform to this will struggle to see why their commands are not working as expected.

I'm going to add an error message, in case people try to turn it off or on, as I really do feel that a "switch" for this type of device is bad UX. There are multiple problems with having a "switch" for an IR "bridge":

It is always going to be optimistic, we have no way of knowing the real state of the device we are controlling.

Most devices use the same IR code for on/off, why would we even have a switch for those cases?

If at all possible I would rather the bridge device just reported the state (Online/Offline) with no possibility of turning it on and off, I feel like the would be a much better UX for this kind of device, but I don't really have any idea about how to implement that, so I'm going to throw an error, if people try turning it off or on.

cnrd · 2018-01-25T17:02:58Z

Okay I added the component docs, let's hope that travis likes me this time (Dunno why it is failing randomly).

Not sure what else there is to do, I feel like this could get merged now, and when python-miio 0.3.5 is released only a single line needs changing. What do you think @rytilahti ?

Danielhiversen · 2018-01-25T20:38:26Z

homeassistant/components/remote/xiaomi_miio.py

+        else:
+            return
+
+    # pylint: disable=R0201, W0107


Why disable W0107?
The pass statements are unnecessary

@rytilahti Recommended that I added the pass above, but I think I may have misunderstood what he meant :-)

Danielhiversen · 2018-01-25T20:40:20Z

homeassistant/components/remote/xiaomi_miio.py

+
+    @property
+    def should_poll(self):
+        """We should be polled for device up state."""


No polling needed for...........

pvizeli · 2018-01-26T11:48:54Z

homeassistant/components/remote/xiaomi_miio.py

+                                 schema=LEARN_COMMAND_SCHEMA)
+
+
+class XiaomiMiioRemote(Entity):


You need impolement this interface: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/remote/__init__.py#L177

pvizeli · 2018-01-26T11:49:17Z

homeassistant/components/remote/xiaomi_miio.py

+            return False
+
+    @asyncio.coroutine
+    def async_send_command(self, command, **kwargs):


You don't need implement the async version

pvizeli · 2018-01-26T11:51:23Z

homeassistant/components/remote/xiaomi_miio.py

+                        yield from self._send_command(local_payload)
+                else:
+                    yield from self._send_command(payload)
+                yield from asyncio.sleep(delay, loop=self.hass.loop)


Can we offload this repeat stuff into library or do we need this repeat code? If the device is not response, we could try it later.

Repeat is not about devices not responding, this code snippet from remote.harmony was linked by @syssi earlier: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/remote/harmony.py#L209-L222

num_repeats and delay are optional parameters for remote.send_command

… None

… and turn_off, fixed services.yaml examples

…des)

…t of bounds error.

cnrd · 2018-02-06T17:38:48Z

Sorry for the ugly rebase, first time doing that, but at least it worked :-)

Looks like all tests passed and python-miio is updated, so I think we are ready for a merge now?

rytilahti · 2018-02-06T18:45:42Z

Indeed, thanks a lot! (I changed the title for the merge commit a bit)

MartinHjelmare

Left comments for improvement in a future PR.

MartinHjelmare · 2018-02-06T19:10:58Z

homeassistant/components/remote/xiaomi_miio.py

+DEFAULT_SLOT = 1
+
+LEARN_COMMAND_SCHEMA = vol.Schema({
+    vol.Required(ATTR_ENTITY_ID): vol.All(str),


This should be cv.entity_id or cv.entity_ids.

MartinHjelmare · 2018-02-06T19:16:28Z

homeassistant/components/remote/xiaomi_miio.py

+        return
+
+    if PLATFORM not in hass.data:
+        hass.data[PLATFORM] = {}


Maybe you should use a key specific for the remote xiaomi_miio platform? Other xiaomi_miio platforms might want to store things in hass.data in the future.

I am pretty sure that this part is correct, if you look at the other python-miio platforms they do the same thing: Create a dict if one is not on the PLATFORM key, and save the devices in the new dict using their host (ip) as key. The host should be unique but there may be some edge cases where it is not.

See these lines in the vacuum python-miio:
https://github.com/home-assistant/home-assistant/blob/5ba02c531e4d7e68dc8b811ba2ebc7438afe6f16/homeassistant/components/vacuum/xiaomi_miio.py#L91-L103

MartinHjelmare · 2018-02-06T19:18:00Z

homeassistant/components/remote/xiaomi_miio.py

+    if PLATFORM not in hass.data:
+        hass.data[PLATFORM] = {}
+
+    friendly_name = config.get(CONF_NAME, "xiaomi_miio_" +


Use new style string formatting, not string concatenation.

MartinHjelmare · 2018-02-06T19:20:34Z

homeassistant/components/remote/xiaomi_miio.py

+    @asyncio.coroutine
+    def async_service_handler(service):
+        """Handle a learn command."""
+        if service.service != SERVICE_LEARN:


When would this happen?

MartinHjelmare · 2018-02-06T19:25:30Z

homeassistant/components/remote/xiaomi_miio.py

+
+        entity_id = service.data.get(ATTR_ENTITY_ID)
+        entity = None
+        for remote in hass.data[PLATFORM].values():


You could use a generator expression and next which is fast and clean.

entity = next(( remote for remote in hass.data[PLATFORM].values() if remote.entity_id == entity_id), None)

MartinHjelmare · 2018-02-06T19:37:03Z

homeassistant/components/remote/xiaomi_miio.py

+    @asyncio.coroutine
+    def async_turn_off(self, **kwargs):
+        """Turn the device off."""
+        _LOGGER.error("Device does not support turn_off, " +


Same as above.

MartinHjelmare · 2018-02-06T19:44:11Z

homeassistant/components/remote/xiaomi_miio.py

+        return self._name
+
+    @property
+    def device(self):


If the property isn't needed just make a regular instance attribute of it instead.

MartinHjelmare · 2018-02-06T19:44:27Z

homeassistant/components/remote/xiaomi_miio.py

+        return self._is_hidden
+
+    @property
+    def slot(self):


Same as above.

MartinHjelmare · 2018-02-06T19:44:58Z

homeassistant/components/remote/xiaomi_miio.py

+        return self._slot
+
+    @property
+    def timeout(self):


Same as above.

MartinHjelmare · 2018-02-06T19:54:24Z

homeassistant/components/remote/xiaomi_miio.py

+                        self._send_command(local_payload)
+                else:
+                    self._send_command(payload)
+                time.sleep(delay)


If the xiaomi_miio library supports asyncio it was better when this method was async, since we want to sleep here. We usually frown upon sync sleep in worker threads which this now does. On the other hand async sleep doesn't cost anything.

I'm not sure why @pvizeli wanted this to be done sync?

juanjoSanz · 2018-02-12T09:16:50Z

Hello, after updating xiaomi ir component I realized something may not be working well.

I have two devices so I created in yaml file two entries:

- platform: xiaomi_miio
name: "IR 1"

- platform: xiaomi_miio
name: "IR 2"

With that configuration, only last item ( IR 2 ) is created.

Do I make something wrong?

Thanks in advance

MartinHjelmare · 2018-02-12T09:19:05Z

Please open an issue if you suspect a bug. If you need help please use our help channels:
https://home-assistant.io/help/#communication-channels

Merged PRs should not be used for support or bug reports. Thanks!

juanjoSanz · 2018-02-12T10:39:30Z

@MartinHjelmare thanks for the advice, anyway it seems the error was in my configuration, in log I have found an error regarding token that lead me to fix the issue. Token string was miscopied

cnrd requested review from andrey-git and rytilahti as code owners January 24, 2018 05:02

homeassistant added cla-signed new-platform platform: remote.xiaomi_miio labels Jan 24, 2018

cnrd mentioned this pull request Jan 24, 2018

xiaomi_ir (python-miio ChuangmiIr device) #11856

Closed

7 tasks

homeassistant added the cla-signed label Jan 24, 2018

cnrd closed this Jan 24, 2018

cnrd reopened this Jan 24, 2018

syssi reviewed Jan 24, 2018

View reviewed changes

homeassistant added the cla-signed label Jan 24, 2018

rytilahti reviewed Jan 24, 2018

View reviewed changes

homeassistant added the cla-signed label Jan 25, 2018

cnrd mentioned this pull request Jan 25, 2018

added remote.xiaomi_miio component home-assistant/home-assistant.io#4513

Merged

2 tasks

cnrd closed this Jan 25, 2018

cnrd reopened this Jan 25, 2018

Danielhiversen reviewed Jan 25, 2018

View reviewed changes

homeassistant added the cla-signed label Jan 25, 2018

cnrd closed this Jan 25, 2018

cnrd reopened this Jan 25, 2018

pvizeli reviewed Jan 26, 2018

View reviewed changes

homeassistant added the cla-signed label Jan 26, 2018

cnrd added 18 commits February 6, 2018 18:21

fixed pylint error

1561b93

misc fixes and input validation

e7385bd

address syssi's requests except device and async_service_handler

cc1598a

forgot to run linter

5d7b91c

implemented async_service_handler

f4fd9b9

fixed delay == None, honor timeout given by user, pythonic compare of…

e1d26f6

… None

Added some whitespace for readability, added error message to turn_on…

e153425

… and turn_off, fixed services.yaml examples

fixed pylint errors

1fb4d00

readd pass for readability

141cf4c

fixed small stuff

0800f89

Use RemoteDevice, Make send_command non-async

0371fdd

Ready code for next version of python-miio (Support for pronto hex co…

7ecb8c6

…des)

cast command_optional to int, better input validation, fixed index ou…

45eec73

…t of bounds error.

revert code now in python-miio

cbf49b0

ready for python-miio 0.3.5

2827425

Removed unneccary return statements

5907d6b

require 0.3.5

aa5fbc2

Rebase and update requirements_all.txt

f7dfe02

homeassistant added the cla-signed label Feb 6, 2018

rytilahti merged commit 49c7b42 into home-assistant:dev Feb 6, 2018

MartinHjelmare reviewed Feb 6, 2018

View reviewed changes

balloob mentioned this pull request Feb 9, 2018

0.63 #12267

Merged

home-assistant locked and limited conversation to collaborators May 29, 2018

ghost added integration: xiaomi_miio and removed platform: remote.xiaomi_miio labels Mar 21, 2019

python-miio remote. #11891

python-miio remote. #11891

Conversation

cnrd commented Jan 24, 2018 • edited

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

cnrd commented Jan 24, 2018

cnrd commented Jan 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnrd Jan 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnrd commented Jan 24, 2018

cnrd commented Jan 24, 2018

rytilahti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnrd commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnrd commented Feb 6, 2018

rytilahti commented Feb 6, 2018 • edited

MartinHjelmare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanjoSanz commented Feb 12, 2018

MartinHjelmare commented Feb 12, 2018

juanjoSanz commented Feb 12, 2018 • edited

cnrd commented Jan 24, 2018 •

edited

Example entry for `configuration.yaml` (if applicable):

cnrd commented Jan 24, 2018 •

edited

cnrd Jan 24, 2018 •

edited

rytilahti commented Feb 6, 2018 •

edited

juanjoSanz commented Feb 12, 2018 •

edited