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

Device support for the xiaomi air purifier added. #31

Merged
merged 15 commits into from
Aug 14, 2017

Conversation

syssi
Copy link
Collaborator

@syssi syssi commented Jul 23, 2017

No description provided.

s += "limit_hum=%s%%, trans_level=%s%% " % (self.limit_hum, self.trans_level)
s += "bright=%s%%, favorite_level=%s%% " % (self.bright, self.favorite_level)
s += "filter_life_remaining=%s%%, act_det=%s%% " % (self.filter_life_remaining, self.act_det)
s += "filter_hours_used=%s%%, use_time=%s%% " % (self.filter_hours_used, self.use_time)

Choose a reason for hiding this comment

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

line too long (95 > 79 characters)

s += "buzzer=%s%%, child_lock=%s%% " % (self.buzzer, self.child_lock)
s += "limit_hum=%s%%, trans_level=%s%% " % (self.limit_hum, self.trans_level)
s += "bright=%s%%, favorite_level=%s%% " % (self.bright, self.favorite_level)
s += "filter_life_remaining=%s%%, act_det=%s%% " % (self.filter_life_remaining, self.act_det)

Choose a reason for hiding this comment

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

line too long (101 > 79 characters)

s += "mode=%s%%, led=%s%% , led_b=%s%% " % (self.mode, self.led, self.led_b)
s += "buzzer=%s%%, child_lock=%s%% " % (self.buzzer, self.child_lock)
s += "limit_hum=%s%%, trans_level=%s%% " % (self.limit_hum, self.trans_level)
s += "bright=%s%%, favorite_level=%s%% " % (self.bright, self.favorite_level)

Choose a reason for hiding this comment

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

line too long (85 > 79 characters)

s += "temperature=%s%%, humidity=%s%% " % (self.temperature, self.humidity)
s += "mode=%s%%, led=%s%% , led_b=%s%% " % (self.mode, self.led, self.led_b)
s += "buzzer=%s%%, child_lock=%s%% " % (self.buzzer, self.child_lock)
s += "limit_hum=%s%%, trans_level=%s%% " % (self.limit_hum, self.trans_level)

Choose a reason for hiding this comment

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

line too long (85 > 79 characters)

def __str__(self) -> str:
s = "<AirPurifierStatus power=%s, aqi=%s " % (self.power, self.aqi)
s += "temperature=%s%%, humidity=%s%% " % (self.temperature, self.humidity)
s += "mode=%s%%, led=%s%% , led_b=%s%% " % (self.mode, self.led, self.led_b)

Choose a reason for hiding this comment

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

line too long (84 > 79 characters)


def __str__(self) -> str:
s = "<AirPurifierStatus power=%s, aqi=%s " % (self.power, self.aqi)
s += "temperature=%s%%, humidity=%s%% " % (self.temperature, self.humidity)

Choose a reason for hiding this comment

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

line too long (83 > 79 characters)


@property
def mode(self) -> str:
return self.data["mode"] # auto, silent, favorite, medium, high, strong, idle

Choose a reason for hiding this comment

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

line too long (86 > 79 characters)

@rytilahti
Copy link
Owner

Hi Sebastian! Patches for new devices are very welcome and I think people with those devices will truly appreciate that :-) I think it is time to start considering how to deal with the issue of the separate binaries soon enough, but before that I think it makes sense not to populate the containers file with everything, but just have those in the same file as the rest of the (device-specific) implementation. What do you think?

@syssi
Copy link
Collaborator Author

syssi commented Jul 23, 2017

This is fine for me. I will just move it.


class AirPurifierStatus:
"""Container for status reports from the air purifier."""
def __init__(self, data: Dict[str, Any]) -> None:

Choose a reason for hiding this comment

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

undefined name 'Dict'
undefined name 'Any'

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

Looking good in general, I left some small notes behind.


@property
def power(self) -> str:
return self.data["power"]
Copy link
Owner

Choose a reason for hiding this comment

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

Do you have an example what type of response is expected here? A numerical or a string?


@property
def aqi(self) -> int:
return self.data["aqi"]
Copy link
Owner

Choose a reason for hiding this comment

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

aqi = air quality index?


@property
def child_lock(self) -> str:
return self.data["child_lock"]
Copy link
Owner

Choose a reason for hiding this comment

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

Is this a boolean or a string?

values = self.send(
"get_prop",
properties
)
Copy link
Owner

Choose a reason for hiding this comment

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

Considering that the get_prop is pretty generic (it's also used for the strip it seems, as well as for the yeelights), maybe it makes sense to have a list (or a mapping?) of properties and their corresponding types listed and move the status() implementation to the parent class at some point? See also the comments below regarding to property names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the properties are somehow generic but they cannot be applied to all devices. For example there are three versions of the air purifier out there all of them with different supported OperationModes. Some devices does respond with "on" and "off"... others with "true" and "false". I suggest to refactor the code if the support is stable and the knowledge about different responses stronger.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, sounds sane! 👍

Copy link
Owner

Choose a reason for hiding this comment

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

To add, do you happen to have an example also for this case? It would be nice to have example answers for all calls supported by devices.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I must confess I don't own the device. I just want to see it supported. I will ask around. :-)

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for helping to get it supported! :-) Potential tester available here: https://community.home-assistant.io/t/xiaomi-mi-air-purifier-support/14228/12 but for that we need to have a simple test script.

"""Set mode."""

# auto, silent, favorite, medium, high, strong, idle
return self.send("set_mode", [mode])
Copy link
Owner

Choose a reason for hiding this comment

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

Enumize setting and getting of the mode?

# 'limit_hum', 'trans_level', 'bright',
# 'favorite_level', 'filter1_life', 'act_det',
# 'f1_hour_used', 'use_time', 'motor1_speed']
self.data = data
Copy link
Owner

Choose a reason for hiding this comment

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

Do you have an example of a response? It would be nice to have included also.

@property
def favorite_level(self) -> str:
# Favorite level used when the mode is `favorite`. Between 0 and 16.
return self.data["favorite_level"]
Copy link
Owner

Choose a reason for hiding this comment

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

Is this an integer or str?


@property
def act_det(self) -> bool:
return self.data["act_det"] == "on"
Copy link
Owner

Choose a reason for hiding this comment

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

Any description for this , limit_hum and trans_level? The names are not really descriptive.

return self.data["temp_dec"] / 10.0

@property
def mode(self) -> OperationMode:

Choose a reason for hiding this comment

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

undefined name 'OperationMode'

)
return AirPurifierStatus(dict(zip(properties, values)))

def set_mode(self, mode: OperationMode):

Choose a reason for hiding this comment

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

undefined name 'OperationMode'

Copy link
Owner

Choose a reason for hiding this comment

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

It's better to move the class on top of the file, another possibility would be to use a forward declaration ('OperationMode'), which does not look so nice IMO and should be avoided where not really necessary (like when using it inside its own defining class).


def set_led(self, led: bool):
"""Turn led on/off."""

Copy link
Owner

Choose a reason for hiding this comment

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

No need for extra white-spaces after the docstring, odd that hound did not yell about that...


@property
def limit_hum(self) -> str:
return self.data["limit_hum"]
Copy link
Owner

Choose a reason for hiding this comment

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

I think my comment on these got lost somewhere, can we have better, more descriptive names for this, trans_level, act_det and led_b? led_brightness probably for led_b, but I have no idea what those two others could even be..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. I don't know the meaning of trans_level and act_det, too. ;-)


@property
def child_lock(self) -> str:
return self.data["child_lock"]
Copy link
Owner

Choose a reason for hiding this comment

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

Is this a boolean or "on" / "off"?


@property
def humidity_limit(self) -> str:
return self.data["limit_hum"]
Copy link
Owner

Choose a reason for hiding this comment

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

str or float/int?

@property
def favorite_level(self) -> str:
# Favorite level used when the mode is `favorite`. Between 0 and 16.
return self.data["favorite_level"]
Copy link
Owner

Choose a reason for hiding this comment

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

Considering the setter is using int, this is probably integer too, right?


@property
def trans_level(self) -> str:
return self.data["trans_level"]
Copy link
Owner

Choose a reason for hiding this comment

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

Level indicates that it's probably an integer, but this is ok and can be tuned when the payload is known..

@syssi
Copy link
Collaborator Author

syssi commented Jul 25, 2017

I will fix/clarify the open questions if some payload of a response is known. It's fine for me if the PR won't get merged awhile.

def __init__(self, data: Dict[str, Any]) -> None:
# Response of a Air Purifier Pro:
# ['power': 'on, 'aqi': 41, 'humidity': 62, 'temp_dec': 293,
# 'mode': 'auto', 'led': 'on', 'led_b': null, 'buzzer': null, 'child_lock': 'off',

Choose a reason for hiding this comment

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

line too long (91 > 79 characters)

# 'mode': 'auto', 'led': 'on', 'led_b': null, 'buzzer': null,
# 'child_lock': 'off', 'limit_hum': null, 'trans_level': null,
# 'bright': 71, 'favorite_level': 17, 'filter1_life': 77,
# 'act_det': null, 'f1_hour_used': 771, 'use_time': 2776200,
Copy link
Owner

Choose a reason for hiding this comment

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

Favorite level is 17 here, but in the comment of it says: # Favorite level used when the mode is favorite. Between 0 and 16. Those which return null should probably be marked in typing with eg. Optional[bool] (buzzer for example). Hopefully we can get some more test examples to see if those are always null..

Copy link
Owner

@rytilahti rytilahti Jul 27, 2017

Choose a reason for hiding this comment

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

Oh, to add. Those are probably encoded as Nones, trying to construct an enum out of them will cause ValueError, so trying to access led_brightness would cause a crash at least. I think

if self.data["led_b"] is not None:
    return LedBrightness(self.data["led_b"])

is probably the best solution here.

@rytilahti
Copy link
Owner

We are also going to need mirobo -d info output from this device to add support to the (upcoming) new cli tool.

@syssi
Copy link
Collaborator Author

syssi commented Aug 4, 2017

Known models: 'zhimi.airpurifier.m1', 'zhimi.airpurifier.v1', 'zhimi.airpurifier.v2', 'zhimi.airpurifier.v3', 'zhimi.airpurifier.v6', 'zhimi.humidifier.v1'.

@rytilahti
Copy link
Owner

Ok, thanks! Let's get this merged, the client tool can be added / updated later on, maybe having this in encourages someone to do that :-)

@rytilahti rytilahti merged commit 42a9d9b into rytilahti:master Aug 14, 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.

3 participants