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

Support for Dual Roller Shutter #66

Closed
wants to merge 0 commits into from
Closed

Conversation

madzrobz
Copy link
Contributor

@madzrobz madzrobz commented May 7, 2021

Support for separately positioning the upper or lower curtain of dual roller shutters added.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 81.314% when pulling 5efe3c4 on madzrobz:master into bb748d1 on Julius2342:master.

@madzrobz
Copy link
Contributor Author

madzrobz commented May 7, 2021

The question remains, how to integrate with home assistant.

I still believe that simply adding a "curtain" parameter to the cover.set_position function would be sufficient and simpler then exposing three covers to home assistant (as discussed in 2019).

I did not explore whether/how KLF 200 reports position changes on the sub-covers. I'm not sure whether we would be able to maintain the correct position of these sub-covers in home assistant. Thus only providing a one-way function call to set the position of the sub-curtains seems to be perfectly fine for me.

@Julius2342
Copy link
Owner

Julius2342 commented May 8, 2021

Looks good to me. I think it is fine to add pylint-ignore for the too-many-branches and too-many-return statements warning.

And I would like to ask you to add a unit test :-)

"""Dual RollerShutter object."""

async def set_position(self, position, wait_for_completion=True, curtain="both"):
"""Set dual roller shutter to the desired position.
Copy link
Owner

Choose a reason for hiding this comment

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

The problem here is, that this definition conflichts with the base implementation. I think the way around it could be to add a **kwargs to the base class (not a big fan of this approach, bc it makes refactoring hard).

Another idea would be to add an additional method to the DualRollerShutter.

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

3 participants