Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

Refactor cover, fixing some bugs #73

Merged
merged 1 commit into from
Dec 15, 2016
Merged

Refactor cover, fixing some bugs #73

merged 1 commit into from
Dec 15, 2016

Conversation

rcloran
Copy link
Contributor

@rcloran rcloran commented Nov 13, 2016

Cover devices may or may not support setting their position (ie, an arbitrary
value between 0 and 100) instead of just opening or closing.

Before this change, it was assumed that rollershutters could have their
set, but garage doors could not. This is not correct.

This refactors the cover accessory to make this separation more clear. Instead
of if statements scattered through the code, we build a class hierarchy to
clearly separate garage doors and rollershutters - of the "binary" type, and
the type we previously assumed all were.

It also moves towards a more consistent way of handling the door states, using the HAP constants.

At the same time, I moved all of the logic for choosing the cover type into
the cover factory, instead of doing it all in index.js

It also fixes some bugs - onEvent handling works for rollershutters now.

Cover devices may or may not support setting their position (ie, an arbitrary
value between 0 and 100) instead of just opening or closing.

Before this change, it was assumed that rollershutters could have their
set, but garage doors could not. This is not correct.

This refactors the cover accessory to make this separation more clear. Instead
of if statements scattered through the code, we build a class hierarchy to
clearly separate garage doors and rollershutters - of the "binary" type, and
the type we previously assumed all were.

At the same time, I moved all of the logic for choosing the cover type into
the cover factory, instead of doing it all in index.js
@mention-bot
Copy link

@rcloran, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nvella, @robbiet480 and @maddox to be potential reviewers.

@rcloran
Copy link
Contributor Author

rcloran commented Nov 13, 2016

This is really hard to read as a diff :( Much easier to just read the whole file (which I think is now clearer than it was, but that may just be personal preference).

@landerss0n
Copy link

I'm using this PR atm, it's working great. Seems to fix #70 aswell.

@heinemml
Copy link

My Homematic Shutters we're not showing at all in Homekit. I could also observe the crash of homebridge reported in #70 (comment)

With this PR it works. Thanks!

@robbiet480 robbiet480 merged commit 4cd3926 into home-assistant:master Dec 15, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants