search

Bouke / HAP

Swift implementation of the Homekit Accessory Protocol
https://boukehaarsma.nl/HAP/
MIT License
366 stars 50 forks source link

AID Persistence, Add/Remove Accessories, Reachability #40

Closed gbrooker closed 6 years ago

gbrooker commented 6 years ago

Added ability to add and remove accessories from the Bridge (while paired or not), and persist the Accessory IDs over restarts of the host application. The configuration number (in mDNS TXT record), the last allocated AID, and the AID's of recognised accessories are written to a configuration key in the Device Storage.

The TXT record broadcast over mDNS is updated when the configuation changes, or the device is paired/unpaired.

Added a reachable flag to Accessory, for subclasses to indicate that the bridge has lost contact with a device. HomeKit will recognise the device is unreachable the next time it tries to read or write to the device. For more rapid updates, add a BridgingState Service to the Accessory. The subclass only has to set the reachable property, if the device has a Bridging State Service, it is updated too.

Pairing/unpairing is now performed through functions on the Device instance, so the status can be monitored.

gbrooker commented 6 years ago

Thanks for all the thoughtful comments. I think I've addressed all the issues you have raised so far. Yes I like lots of white space, must be Jony Ive's influence ;)

Let me know if you spot anything else.

gbrooker commented 6 years ago

I think I have addressed all your comments now, including the blank lines and comments within 80 characters.

However I really struggled with git & Xcode source control tonight to commit these last changes due to merging conflicts in Device, Server and characteristics(). I am afraid that some of the improvements you have added in characteristics().swift has conflicted with some of the changes I made. I think the latest version on that file on GitHub has lost a couple of my fixes. I need to double check this tomorrow.

The only comment I didn't address here was the Pairing struct. I think it is a good idea, but I would prefer to work on that in another branch, once these commits are complete. I have a couple of other additions to propose which related to those instance variables. Let's get this dynamic accessory complete and then I'll offer you a new pull request.

gbrooker commented 6 years ago

Actually it seems the opposite has happened, my changes appear to have overwritten your changes to characteristic().swift, even though I manually did the opposite in the resolve conflict process. Git really gives me a headache.... If there is something I need to do on my end, please let me know.

gbrooker commented 6 years ago

Finally got my MacBook back tonight with a brand new battery. I've fixed the issues raised by swiftlint and I corrected the error that caused the unit tests to fail (precondition in Device.addAccessories needed to allow for case of a single Accessory device). Hope we are now close to a merge to master.

gbrooker commented 6 years ago

OK, have resolved all of those as you requested, except for the precondition in addAccessories(), due to the explanation given above. There were a lot of changes to the test files, as every device/accessory initialiser needed a serial number. I have used a few digits to give each one something unique, you may prefer to use something different such as mac style addresses to serve as better examples.

As discussed, the next pull request I plan would be

Let me know if there is anything else you think needs urgently addressing.

Bouke commented 6 years ago

Thanks! I've merged the PR with a few minor changes (see https://github.com/Bouke/HAP/pull/40/commits/20832f40b4d1454176907edaaa8e02f74cb56107).

Bouke commented 6 years ago

See 87204904a23d0ba93b4158b237581e437fcfc061 where I've combined all device settings into the configuration.

It would be cool to have QR pairing available. I've looked into this before, but I couldn't find a nice way to display QR codes in the terminal. What solution are you thinking about?

Re the storage; we currently use MemoryStorage in the tests. It would good for tests to use some memory-backed configuration storage instead of file-based. That allows to easily verify the actual configuration from within the tests, without the need to read the files. I haven't though about how that case should be handled.

Bouke commented 6 years ago

FYI I've simplified storage in b55c934ebad0ecb26f71550762f6bf66703730b3.

gbrooker commented 6 years ago

Thanks for the merge, and the cleanup. Nice simplification of Storage. I note that the--recreate option in your simple main func is removing the configuration entirely, not just removing the pairings, so the AID and IID's are all lost.

QR code pairing needs a few mods to Device, and one more parameter to persist. I'll add it in a new PR soon.

What I will add will work cross platform, except for the conversion of the encoded pairing URL into an QRcode image. I will add a function which will do that on macOS using a core image filter, but I don't know how you would do that on linux. If you were running from a terminal in macOS, you could write the image to a file and display it with 'open -a Preview.app image.png' or something like that. On linux I have no idea. I note that there are some javascript libraries on github which can create a QR code from block symbols, e.g https://github.com/gtanner/qrcode-terminal Perhaps you could translate that to swift if you needed that functionality.