-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
python-miio remote. #11891
python-miio remote. #11891
Conversation
@syssi @rytilahti If you could take a look at this, that would be really great! |
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. |
_LOGGER = logging.getLogger(__name__) | ||
|
||
SERVICE_LEARN = 'xiaomi_miio_learn_command' | ||
SERVICE_SEND = 'xiaomi_miio_send_command' |
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.
It's unused. Please remove the constant.
|
||
SERVICE_LEARN = 'xiaomi_miio_learn_command' | ||
SERVICE_SEND = 'xiaomi_miio_send_command' | ||
PLATFORM = 'python_miio' |
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 think the platform "xiaomi_miio" should be used here.
CONF_SLOT = 'slot' | ||
CONF_COMMANDS = 'commands' | ||
|
||
DEFAULT_TIMEOUT = 10 |
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.
These two constants aren't used anymore, too.
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.
They are now, forgot to use them in the correct place.
return False | ||
|
||
@asyncio.coroutine | ||
def async_send_command(self, command, **kwargs): |
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.
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
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.
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.
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.
Don't handle / ignore the device.
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.
done
}) | ||
|
||
COMMAND_SCHEMA = vol.Schema({ | ||
vol.Required(CONF_COMMAND): cv.ensure_list |
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.
vol.Required(CONF_COMMAND): vol.All(cv.ensure_list, [cv.string])
self._timeout = timeout | ||
self._state = False | ||
self._commands = commands | ||
|
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.
Please add a should_poll method. Just to make clear it's a polling entity or not.
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.
done
@asyncio.coroutine | ||
def _learn_command(call): | ||
"""Handle a learn command.""" | ||
entity_id = call.data.get('entity_id').split('.')[1] |
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.
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
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.
done
@syssi Not really sure if the async_service_handler is correct, but no more hacking up the entity_ids |
@syssi @rytilahti anything else you want changed? If not, then I'll start writing the docs. |
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.
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.
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' |
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 think this should be just '1'
. Same for the timeout below, these are supposed to be values you can input for these fields.
# This should be DeviceError but python-miio returns wrong except. | ||
except ChecksumError as ex: | ||
_LOGGER.error("Token not accepted by device : %s", ex) | ||
return |
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 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 :)
|
||
entity_id = service.data.get(ATTR_ENTITY_ID) | ||
|
||
entity = None |
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 think there's no need for a newline between these two, but it may be a matter a taste.
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") |
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.
For this while block I'd on the other hand insert a a newline or two to make it easier to read.
|
||
@asyncio.coroutine | ||
def async_turn_on(self, **kwargs): | ||
"""Turn the device on.""" |
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'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.
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'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.
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 ? |
else: | ||
return | ||
|
||
# pylint: disable=R0201, W0107 |
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.
Why disable W0107?
The pass statements are unnecessary
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.
@rytilahti Recommended that I added the pass above, but I think I may have misunderstood what he meant :-)
|
||
@property | ||
def should_poll(self): | ||
"""We should be polled for device up 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.
No polling needed for...........
schema=LEARN_COMMAND_SCHEMA) | ||
|
||
|
||
class XiaomiMiioRemote(Entity): |
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.
You need impolement this interface: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/remote/__init__.py#L177
return False | ||
|
||
@asyncio.coroutine | ||
def async_send_command(self, command, **kwargs): |
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.
You don't need implement the async version
yield from self._send_command(local_payload) | ||
else: | ||
yield from self._send_command(payload) | ||
yield from asyncio.sleep(delay, loop=self.hass.loop) |
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.
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.
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.
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
… and turn_off, fixed services.yaml examples
…t of bounds error.
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? |
Indeed, thanks a lot! (I changed the title for the merge commit a bit) |
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.
Left comments for improvement in a future PR.
DEFAULT_SLOT = 1 | ||
|
||
LEARN_COMMAND_SCHEMA = vol.Schema({ | ||
vol.Required(ATTR_ENTITY_ID): vol.All(str), |
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 should be cv.entity_id
or cv.entity_ids
.
return | ||
|
||
if PLATFORM not in hass.data: | ||
hass.data[PLATFORM] = {} |
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.
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.
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 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
if PLATFORM not in hass.data: | ||
hass.data[PLATFORM] = {} | ||
|
||
friendly_name = config.get(CONF_NAME, "xiaomi_miio_" + |
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.
Use new style string formatting, not string concatenation.
@asyncio.coroutine | ||
def async_service_handler(service): | ||
"""Handle a learn command.""" | ||
if service.service != SERVICE_LEARN: |
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.
When would this happen?
|
||
entity_id = service.data.get(ATTR_ENTITY_ID) | ||
entity = None | ||
for remote in hass.data[PLATFORM].values(): |
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.
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)
@asyncio.coroutine | ||
def async_turn_off(self, **kwargs): | ||
"""Turn the device off.""" | ||
_LOGGER.error("Device does not support turn_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.
Same as above.
return self._name | ||
|
||
@property | ||
def device(self): |
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.
If the property isn't needed just make a regular instance attribute of it instead.
return self._is_hidden | ||
|
||
@property | ||
def slot(self): |
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.
Same as above.
return self._slot | ||
|
||
@property | ||
def timeout(self): |
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.
Same as above.
self._send_command(local_payload) | ||
else: | ||
self._send_command(payload) | ||
time.sleep(delay) |
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.
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?
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:
With that configuration, only last item ( IR 2 ) is created. Do I make something wrong? Thanks in advance |
Please open an issue if you suspect a bug. If you need help please use our help channels: Merged PRs should not be used for support or bug reports. Thanks! |
@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 |
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):You could then create an actual UI button in this way:
This allows us to save arbitrary commands as part of the remote and give them easy, readable names.
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
.