Skip to content

Introduce BasicWindowCovering and deprecate WindowCovering #58

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

Merged
merged 1 commit into from
Mar 9, 2019

Conversation

timcharper
Copy link
Contributor

@timcharper timcharper commented Feb 3, 2019

The pattern for most accessories is to require only the required
characteristics, and then allow the specification of optional characteristics
via the inclusion of interfaces. This pattern was not followed with
WindowCovering. Worse, for reasons currently not understood, the inclusion of
the HoldPositionCharacteristic causes the homekit pairing with the bridge to be
completely unresponsive. (See issue #56)

Change is made in a backwards compatible way, so that existing implementations
that depend on WindowCovering as implemented will continue to function

Addresses #56

@andylintner
Copy link
Collaborator

Hmmm. Any idea which one caused it? I'd like to try fixing it instead of removing them - I actually have WindowCovering working in my branch of the OH2 add-on.

@timcharper
Copy link
Contributor Author

I'm not exactly sure right now; I have the sample hap app with a mock window covering; if I enable it, then everything comes crashing down and I have to remove the mock bridge and pair it again from scratch. It's really bad. Stripping it back to the required characteristics worked.

@timcharper
Copy link
Contributor Author

Perfectly reasonable to hold the merge here until we have better clarity on what's broken.

@timcharper
Copy link
Contributor Author

See #56

@timcharper timcharper force-pushed the tharper/fix-window-covering branch from c94a1fc to 5da8e07 Compare February 18, 2019 06:52
@timcharper
Copy link
Contributor Author

@beowulfe The network frames patch from @ccutrer did not fix it. However, I've narrowed it down to being caused by the inclusion of setHoldPosition characteristic; the window covering works fine with the obstruction detected characteristic.

I'm not really sure why it's broken, I've double and triple checked the HoldPositionCharacteristic implementation, the UUID is correct, it's write-only, etc. It's correct per the Homekit spec. However, including this characteristic definitely breaks HAP-Java in really bad ways! :/

@timcharper timcharper changed the title Strip back window covering to essential characteristics Remove setHoldPosition from WindowCovering Feb 18, 2019
@timcharper timcharper force-pushed the tharper/fix-window-covering branch 2 times, most recently from f3b10b2 to f220824 Compare February 18, 2019 07:34
@timcharper timcharper changed the title Remove setHoldPosition from WindowCovering Introduce BasicWindowCovering and deprecate WindowCovering Feb 18, 2019
The pattern for most accessories is to require only the required
characteristics, and then allow the specification of optional characteristics
via the inclusion of interfaces. This pattern was not followed with
WindowCovering. Worse, for reasons currently not understood, the inclusion of
the HoldPositionCharacteristic causes the homekit pairing with the bridge to be
completely unresponsive. (See issue hap-java#56)

Change is made in a backwards compatible way, so that existing implementations
that depend on WindowCovering as implemented will continue to function

Addresses hap-java#56
@timcharper timcharper force-pushed the tharper/fix-window-covering branch from f220824 to e4068ce Compare February 18, 2019 07:36
@timcharper timcharper merged commit 5bbf893 into hap-java:master Mar 9, 2019
@timcharper timcharper deleted the tharper/fix-window-covering branch March 9, 2019 18:24
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.

2 participants