Skip to content

improve pairing reliability with iOS 16.2 #175

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
Merged
yfre merged 3 commits into from
Dec 20, 2022
Merged

improve pairing reliability with iOS 16.2 #175

yfre merged 3 commits into from
Dec 20, 2022

Conversation

@ccutrer
Copy link
Member

@ccutrer ccutrer commented Dec 17, 2022

also keep track of admin permissions for users

note that those permissions aren't actually enforced yet

@ccutrer
implement List Pairings request
f04a4d6 
also keep track of admin permissions for users

note that those permissions aren't actually enforced yet
yfre
yfre approved these changes Dec 17, 2022
View reviewed changes
Copy link
Contributor

@yfre yfre left a comment

Choose a reason for hiding this comment

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

Thank you Cody
LGTM
I hope it fixes ios16.2 issue

ccutrer
ccutrer commented Dec 17, 2022
View reviewed changes
src/main/java/io/github/hapjava/server/impl/pairing/PairingUpdateController.java
} else {
e.add(MessageType.SEPARATOR);
}
e.add(MessageType.USERNAME, username);
Copy link
Member Author

@ccutrer ccutrer Dec 17, 2022

Choose a reason for hiding this comment

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

There's a bug here I realized as I was thinking about this last night while doing other things. We need to strip the MAC off the username we stored before returning it to iOS.

Copy link
Contributor

@yfre yfre Dec 17, 2022

Choose a reason for hiding this comment

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

Right, missed that one as well.

Copy link
Contributor

@yfre yfre Dec 17, 2022

Choose a reason for hiding this comment

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

probably at some point we should think about redesign of AuthInfo to store mac and user id separate. but it would be a breaking change..

@ktgeek
Copy link

ktgeek commented Dec 17, 2022 •
edited
Loading

So, I got things working again and I think @ccutrer's change made a difference, but I'm not 100% sure.

I downloaded the PR, did a quick String username_sans_mac = username.replaceFirst(AuthInfo.getMac(), ""); and used that in place of line 53 as Cody suggested. Recompiled the HomeKit add-on and put it in place. Unfortunately, that didn't work. I decided to unpair and repair to see how that would go, but still no dice.

Not sure why I decided to do this, but I restored a offsite backup of my /var/lib/openhab/jsondb from the beginning of the month and compared the users in the two HomeKit files. I only had one user in the new file, and 4, including the one being reported in the error, in the old file.

I shutdown OpenHAB, edited the new homekit.json and added the entire stanza for the old user. I started OpenHAB back up and HomeKit worked again! I noticed in scanning the logs that another user threw that error, so I shutdown openhab to add that one, and while editing the new homekit.json I noticed the original user I transported over was no longer in the HomeKit.json, and there were now 3 other entries that didn't exist a moment ago.

My theory is that during the upgrade process the HomeKit hubs wanted to list the associations and then clear them out. Until it could successfully do that it was holding on to old pairings. This PR allows the listing to work so the process can complete, but I was in a bad state since I unpaired and few times in here. I don't know when my old pairings that caused the error disappeared, but since stuff stopped working immediately after the upgrade, maybe OpenHAB acted on some sort of clear/reset that the Hubs didn't understand/acknowledge? I am still coming up to speed on this side of stuff.

I think a good test would be someone who hasn't upgraded to get this change in place before they upgrade, then do the upgrade and see if it succeeds.

(On the issue in openhab-addons I know @yfre suggested copying just the user over and keeping the value, but I think we might be able to get people to work if they moved the full json entry over. I'm going to go over there and suggest that.)

@ktgeek ktgeek mentioned this pull request Dec 17, 2022
Closed
@ktgeek
Copy link

ktgeek commented Dec 17, 2022

I had two home hub devices (an AppleTV and a HomePod mini) unplugged while I was doing the other tinkering. When I pulled one back in, my Unknown user error came back and HomeKit broke again. I transported the user over again, and I'm working again. Same dance when I plugged the other in.

I now wonder if the HomeKit upgrade process as it happens on each hub needs to clear out the same keys, but we get confused because we've already done it? (I have 4 AppleTVs and 4 HomePod minis... I wonder if the people who are working have the simple case of only one hub?)

@ccutrer
Copy link
Member Author

ccutrer commented Dec 18, 2022 •
edited
Loading

I now wonder if the HomeKit upgrade process as it happens on each hub needs to clear out the same keys, but we get confused because we've already done it? (I have 4 AppleTVs and 4 HomePod minis... I wonder if the people who are working have the simple case of only one hub?)

I have two AppleTVs.

@yfre
Copy link
Contributor

yfre commented Dec 18, 2022

@ccutrer is it ready for merge? Looks good for me and fixes at least the listing issue.
But was not sure as it marked as draft

@ccutrer
Copy link
Member Author

ccutrer commented Dec 18, 2022

@ccutrer is it ready for merge? Looks good for me and fixes at least the listing issue.
But was not sure as it marked as draft

I'm happy with the code for this now, but I'm hesitant to merge it not ever having tested it. HomeKit only did these requests the one time when I upgraded my Home on Friday. I can't in good conscious say code is ready to merge that I've never actually seen run.

I'm trying to work through a full pairing process on 16.2 (as my limited time permits), and I'm hoping by the end of that I'll have seen this work at least once. Regardless, I'll keep GitHub updated with my progress.

@ccutrer ccutrer changed the title implement List Pairings request improve pairing reliability with iOS 16.2 Dec 19, 2022
@ccutrer ccutrer marked this pull request as ready for review December 19, 2022 20:58
@ccutrer
refactor pairing to match HAP spec more closely
d1d6fb0 
mostly renames, but significant change is that when we don't recognize
a user, return an error code instead of throwing an exception and returning
a 500 with no content.

several responses are now generated with PairingResponse, rather than as
a raw byte array, so that the reader can more easily follow what the response
is.

several more trace loggings showing the details of the pairing process have
been added, for debug use

also ensure that we check if the MAC has changed when refreshing auth info
yfre
yfre approved these changes Dec 20, 2022
View reviewed changes
Copy link
Contributor

@yfre yfre left a comment

Choose a reason for hiding this comment

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

thank you for refactoring and making it more complete.
it was probably the oldest part of the code, from prior to HAP public-spec time.
LGTM

@yfre yfre merged commit 5d57643 into hap-java:master Dec 20, 2022
@ccutrer ccutrer deleted the list-pairings branch January 1, 2023 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yfre yfre yfre approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ccutrer @ktgeek @yfre