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.

Sign up for GitHub

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

Jump to bottom

Initial support for lumi.curtain.hagl05 #851

Merged
merged 10 commits into from
Nov 6, 2020
Merged

Initial support for lumi.curtain.hagl05 #851

merged 10 commits into from
Nov 6, 2020

Conversation

in7egral
Copy link
Contributor

@in7egral in7egral commented Nov 4, 2020

lumi.curtain.hagl05 is codename for the "Xiaomiyoupin Curtain Controller (Wi-Fi)" (sometimes referred as Aqara A1)

I finished a couple of tests on my own device (sorry, no python tests for this version yet).

rytilahti
rytilahti requested changes Nov 5, 2020
View reviewed changes
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, looks good! I left some minor changes that should be addressed prior merging this.

miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
@in7egral
Copy link
Contributor Author

in7egral commented Nov 5, 2020

Hi! Thanks for reviewing the code! Changes have been made.

rytilahti
rytilahti reviewed Nov 5, 2020
View reviewed changes
miio/curtain_youpin.py Outdated Show resolved Hide resolved
rytilahti
rytilahti reviewed Nov 5, 2020
View reviewed changes
miio/curtain_youpin.py Outdated Show resolved Hide resolved
@in7egral
Copy link
Contributor Author

in7egral commented Nov 5, 2020

Well done!

@in7egral
Copy link
Contributor Author

in7egral commented Nov 5, 2020

Didn't notice the boolean suggestion, very good idea!

rytilahti
rytilahti requested changes Nov 5, 2020
View reviewed changes
Copy link
Owner

@rytilahti rytilahti left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Some more comments, mostly:

  • No newlines before ending the docstring, if it is a one-liner. Docstrings should end with a dot and be descriptive.
  • Return boolean values as booleans for status container properties.
  • All functionality should return something, simply add returns for commands, this way the library users can access the response payload if they wish to do so.

edit: Also, please update README.md accordingly!

miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
in7egral and others added 2 commits November 5, 2020 20:26
@in7egral @rytilahti
Apply suggestions from code review
d249922 
lumi.curtain.hagl05 curtain: one-liner comments, returns for the setters

Co-authored-by: Teemu R. <tpr@iki.fi>
lumi.curtain.hagl05: updated Readme, docstrings updated, returns for …
958f6b1 
…setters
rytilahti
rytilahti reviewed Nov 5, 2020
View reviewed changes
miio/curtain_youpin.py Outdated Show resolved Hide resolved
miio/curtain_youpin.py Outdated Show resolved Hide resolved
rytilahti
rytilahti approved these changes Nov 6, 2020
View reviewed changes
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 again for the PR, let's merge it! 🥇

@rytilahti rytilahti merged commit 006a349 into rytilahti:master Nov 6, 2020
@rytilahti rytilahti added this to the 0.5.4 milestone Nov 6, 2020
@in7egral
Copy link
Contributor Author

in7egral commented Nov 7, 2020

Small fix required (no reason to make new PR for this): manual_enabled -> is_manual_enabled in the _MAPPING.

P.S. I am currently working on supporting HA.

@syssi
Copy link
Collaborator

syssi commented Nov 7, 2020

@in7egral I would be happy about another PR nevertheless. :-)

@in7egral
Copy link
Contributor Author

in7egral commented Nov 9, 2020

I'm going to add unit tests today before PR.

@in7egral
Copy link
Contributor Author

in7egral commented Dec 2, 2020

I am still fighting with HomeKit support which will help me figure out how to properly work with this device.

@alexeypetrenko
Copy link
Contributor

Sorry for bringing up an old PR on somewhat unrelated topic.

Do you know if there is a component for Home Assistant, which uses this library and supports this curtain?

If there is not, maybe you can point in the right direction?
Is there a way to contribute directly to https://github.com/home-assistant/core/tree/dev/homeassistant/components/xiaomi_miio?
If ther is not - do I have to write my own custom component to get this curtains working?

@in7egral
Copy link
Contributor Author

I have no immediate plans to support this type of curtain in HA. And it’s not that easy. HA has strict rules for styling code and styling plugins (and it's very cool), but I'm not a big python expert. I connected the curtain with HomeKit with this plug-in and I'm happy. The plugin itself will recieve a new version - some fixes and improvements, test patterns, etc.

P.S. If I can help with HA - contact me please.

xvlady pushed a commit to xvlady/python-miio that referenced this pull request May 9, 2021
@in7egral @rytilahti @xvlady
Initial support for lumi.curtain.hagl05 (rytilahti#851)
a64d6b7 
* Added support for the Xiaomi Smart Curtain Motor (Wi-Fi version)

* module refactoring: Xiaomi Smart Curtain Motor

* lumi.curtain.hagl05 curtain: @rytilahti suggestions applied

* fixed parameter naming

* lumi.curtain.hagl05 curtain: removed unnecessary DeviceError processing

* lumi.curtain.hagl05 curtain: added hints for the ValueError

* lumi.curtain.hagl05 curtain: added boolean types for two-states parameters

* Apply suggestions from code review

lumi.curtain.hagl05 curtain: one-liner comments, returns for the setters

Co-authored-by: Teemu R. <tpr@iki.fi>

* lumi.curtain.hagl05: updated Readme, docstrings updated, returns for setters

* lumi.curtain.hagl05 curtain: refactoring with code checks

Co-authored-by: Teemu R. <tpr@iki.fi>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rytilahti rytilahti rytilahti approved these changes

Assignees
No one assigned
Labels
miot new device
Projects
None yet
Milestone
0.5.4
Development

Successfully merging this pull request may close these issues.
4 participants
@in7egral @syssi @alexeypetrenko @rytilahti