hap-java / HAP-Java

Java implementation of the HomeKit Accessory Protocol
MIT License
152 stars 83 forks source link

add support for QR Code for pairing #118

Closed yfre closed 3 years ago

yfre commented 3 years ago

in order to support QR code, mDNS advertisement must have an additional attribute "sh" with SHA512Digest of setupId and accessory ID as value.

gjvanderheiden commented 3 years ago

That would be awesome.

gjvanderheiden commented 3 years ago

Used it and it works like a charm. I do have some review comments, a bit late, but you move so fast 8)

I would expect that HomekitAuthInfo.getSetupId() is just another interface method, no default implementation. This is a major feature you added and is really part of HAP. Other interface methods hint you in de JavaDoc which method you should use (HAPSetupCodeUtils.generateSetupId()). Now, getSetupId() always returns a new key, which isn't the purpose of this interface. I know you want to help the user, but I think you hint the user better if he/she is aware that something has to be done or ignored.

HAPSetupCodeUtils.getSetupURI() takes a pin, without the minus signs. This is noted in de JavaDoc, but as a API I would expect consistency with HomekitAuthInfo.getPin(), or just accept a int instead of String. I think i would just go for a .replace("-","") inside getSetupURI().

yfre commented 3 years ago

@gjvanderheiden thanks for testing and feedback.

yes, i was not sure whether i should keep getSetupId without default and introduce breaking API change or add default as not everyone needs QR code pairing. but i agree, it needs some documentation, how to use it, how to generate QR code afterwards and recommendation to implement own getSetupId. in general, Java-HAP documentation can be improvement it is currently not existing

and adding replace "-" is good idea, so that clients dont need to care about this.

looks like it is worth a new PR for qr code enhancements.

gjvanderheiden commented 3 years ago

@yfre a pr would be nice. To learn HAP-Java I opened the sample and gone from there. Together with the HAP doc by Apple I took off.

It would be nice to have the sample code as a separate under hap-java (hap-java/hap-java-sample or something), instead of a branch. That puzzled me for a while.

Also default boolean hasUser() {} in HomekitAuthInfo got me off track a long time. This was the reason the sample had to be paired over and over again, despite off the hole auth.info thing. this override in HomekitAuthInfo fixed it for me:

@Override public boolean hasUser() { return !authState.userKeyMap.isEmpty(); }

Again this is a default implementation in HomekitAuthInfo, meh. Not such a fan in this case.

yfre commented 3 years ago

PR is merged.