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

Xiaomi Air Conditioner Companion support #129

Merged
merged 8 commits into from Nov 28, 2017

Conversation

syssi
Copy link
Collaborator

@syssi syssi commented Nov 23, 2017

No description provided.

import enum
from typing import Any, Dict, Optional
from collections import defaultdict
from .device import Device, DeviceException

Choose a reason for hiding this comment

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

'.device.DeviceException' imported but unused

import logging
import enum
from typing import Any, Dict, Optional
from collections import defaultdict

Choose a reason for hiding this comment

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

'collections.defaultdict' imported but unused

@@ -0,0 +1,102 @@
import logging
import enum
from typing import Any, Dict, Optional

Choose a reason for hiding this comment

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

'typing.Any' imported but unused
'typing.Dict' imported but unused
'typing.Optional' imported but unused

@@ -0,0 +1,102 @@
import logging
import enum

Choose a reason for hiding this comment

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

'enum' imported but unused

@@ -0,0 +1,102 @@
import logging

Choose a reason for hiding this comment

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

'logging' imported but unused

@coveralls
Copy link

coveralls commented Nov 23, 2017

Coverage Status

Coverage increased (+0.02%) to 46.061% when pulling 1a6906f on syssi:feature/acpartner into 3bed026 on rytilahti:master.

@coveralls
Copy link

coveralls commented Nov 23, 2017

Coverage Status

Coverage decreased (-0.06%) to 45.977% when pulling a5717d2 on syssi:feature/acpartner into 3bed026 on rytilahti:master.

@syssi syssi changed the title First draft of the Xiaomi Air Conditioner Companion support WIP: Xiaomi Air Conditioner Companion support Nov 26, 2017
@syssi
Copy link
Collaborator Author

syssi commented Nov 26, 2017

@rytilahti Do you like to take a look into the unit test? The response format of the AC companion is unusual, so the device dummy implementation isn't sufficient.

@rytilahti
Copy link
Owner

There's a WIP PR on homeassistant for this, which contains additional features if I'm not mistaken: home-assistant/core#10761 . @syssi do you think it'd make sense to merge this now, and do incremental changes assuming the authors of that PR are interested doing that?


def learn(self):
"""Learn an infrared command."""
return self.send("start_ir_learn", [30])
Copy link
Owner

Choose a reason for hiding this comment

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

The magic 30 should be extracted into a constant and/or documented some other way.

# Device model: lumi.acpartner.v2
#
# Response of "get_model_and_state":
# ['010500978022222102', '010201190280222221', '2']
Copy link
Owner

Choose a reason for hiding this comment

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

It would be nice to have some sort of description for specific byte indexes (and include a short, informal description of the protocol if possible) already here for readability.

@rytilahti
Copy link
Owner

@syssi considering that the protocol is that much different, it may make sense to avoid using the dummydevice and make the tests specific for this device? Although if it works like it is currently, that's fine too :-)

@syssi
Copy link
Collaborator Author

syssi commented Nov 26, 2017

@rytilahti I'm aware of home-assistant/core#10761 and we are in contact already cp. #76

@syssi syssi changed the title WIP: Xiaomi Air Conditioner Companion support Xiaomi Air Conditioner Companion support Nov 26, 2017
from unittest import TestCase
from miio import AirConditioningCompanion
from miio.airconditioningcompanion import OperationMode, FanSpeed
from .dummies import DummyDevice

Choose a reason for hiding this comment

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

'.dummies.DummyDevice' imported but unused

@coveralls
Copy link

coveralls commented Nov 26, 2017

Coverage Status

Coverage increased (+13.03%) to 59.064% when pulling 323973a on syssi:feature/acpartner into 3bed026 on rytilahti:master.

@coveralls
Copy link

coveralls commented Nov 26, 2017

Coverage Status

Coverage increased (+12.8%) to 58.85% when pulling 323973a on syssi:feature/acpartner into 3bed026 on rytilahti:master.

@syssi
Copy link
Collaborator Author

syssi commented Nov 26, 2017

@rytilahti Could you check the sendcmd method of home-assistant/core#10761? There is a proper state needed to assemble a full featured command. Do you think the method can be migrated to python-miio? Where should be maintained the state?

@syssi
Copy link
Collaborator Author

syssi commented Nov 26, 2017

I would be happy about an early merge.

@rytilahti
Copy link
Owner

Yes, the part definitely should be merged (after cleaning it up!). I'm unsure what type of state maintaining is required here, if it's just the state to do a new request, I think that detail can be hidden / a cached state message can be used? For now I think we can merge this.

@rytilahti rytilahti merged commit 151ec30 into rytilahti:master Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants