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 chuangmi.camera.ipc009 support #516

Merged
merged 3 commits into from
Jun 18, 2019
Merged

Add chuangmi.camera.ipc009 support #516

merged 3 commits into from
Jun 18, 2019

Conversation

impankratov
Copy link
Contributor

@impankratov impankratov commented Jun 4, 2019

Support for basic commands of Xiaomi Mijia 360 1080p camera.
Commands were tested using miio through command line.
I'm no python developer, so any feedback regarding code organization will be appreciated.

miio/discovery.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jun 4, 2019

Coverage Status

Coverage decreased (-0.2%) to 71.973% when pulling 612d059 on impankratov:master into 0f8f608 on rytilahti:master.

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.

Thanks for the PR! This looks good to me, I added a couple of things you could adjust and I will merge this after those are done.

@property
def power(self) -> str:
""" Camera power """
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.

Is this returning the state of the device? If yes, maybe check for the wanted value and return boolean instead of str.

Also, the docstring comments begin and end without a whitespace, please add dots to end of sentences, too.

Copy link
Contributor Author

@impankratov impankratov Jun 7, 2019

Choose a reason for hiding this comment

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

Thanks for review!

If yes, maybe check for the wanted value and return boolean instead of str.

Got it, will figure how to do it (I think I saw something like that in some other device class) and fix.

If yes, maybe check for the wanted value and return boolean instead of str.

Sure, will fix this as well.

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

Choose a reason for hiding this comment

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

Same as above, if this is boolean, check the value and return a boolean. I will not add more comments on this below, so check the values of those and adjust the types accordingly.


@property
def max_client(self) -> int:
return self.data["max_client"]
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 know what these undocumented functions do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, I think max client refers to maximum clients that could connect to camera (via mihome plugin) to stream video simultaneously. However it returns zero, so it's kind of not useful anyway. Should I remove it completely?

Copy link
Owner

@rytilahti rytilahti Jun 7, 2019

Choose a reason for hiding this comment

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

No, let's keep it just for the sake of completeness, who knows if the situation changes with some newer firmwares etc. Simply add comments to those stating that they are unknown/unused at the moment.

def __init__(self, data: Dict[str, Any]) -> None:
"""
Request:
["power", "motion_record", "light", "full_color", "flip", "improve_program", "wdr", "track", "sdcard_status", "watermark", "max_client", "night_mode", "mini_level"]

Choose a reason for hiding this comment

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

line too long (172 > 100 characters)

@impankratov
Copy link
Contributor Author

@rytilahti sorry for such a delay, got the fixes you've requested. However, I left some properties which couldn't be mapped to boolean unchanged. For example: motion_record can have 3 possible values: on/off/stop.

@rytilahti
Copy link
Owner

Looks good to me, thanks! 💯

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.

4 participants