Observe Characteristic changes and HAP getValues

[gbrooker]

This PR provides hooks for an app using HAP to

• observe GET value requests from HAP, to support lazy updating.
  See GenericCharacteristic.onGetValue
• observe changes to any Characteristic on any accessory in one callback (e.g for updating UI)
  See Device.onCharacteristicChange

It also enforces Characteristic read/writes to occur on the main thread using a precondition and renames Device.notify(characteristicListeners: Characteristic, ..) to the clearer Device.notifyChangeOf(characteristic: Characteristic, ..) as the first parameter is the Characteristic itself, not a listener.

[Bouke]

Interesting idea; I haven't thought of this use-case. I think we should not try to combine this in the existing GenericCharacteristic, but have something like a separate ProxyCharacteristic instead;

• GenericCharacteristic: the characteristic's value is stored on the characteristic (self-contained).
• ProxyCharacteristic: the characteristic's value is provided by callback. When an iOS device subscribes, we should poll the callback periodically to fetch new values.

The dispatchPrecondition might be macOS only, as it only fails on the Linux build? We should ensure that it gets stripped from release builds, as we don't want to do such checks all the time.

Regarding the API renaming, in your proposed Device.notifyChangeOf(characteristic: Characteristic,
the characteristic bit is duplicated. Which is different from the stdlib design (defacto Swift), compare with e.g. Set's isSubset(of other: Set<Set.Element>). The original name could use some improvements nonetheless.

[gbrooker]

Interesting thought on ProxyCharacteristic, however in many cases, the response will not be instantaneous, and should not block the main queue while it determines the latest value. GenericCharacteristic is safe as it will just return the most recent value it is aware of, while in the meantime a listener to onGetValue() can initiate a request to retrieve the latest updated value from the real world device/server asynchronously, and set the Characteristic's value once it has a response. If the Home app is still viewing the accessory, it will be listening to Characteristic change events which will be called when the Characteristics value is set.

dispatchPrecondition() needs the -Ounchecked compiler option to remove the condition from builds. However it needs macOS 10.12 as a minimum build target, and it seems swift package manager only builds to 10.10 as a target, so I have removed these precondition calls from the PR.

Good idea on the renaming, I have refactored Device.notifyChangeOf(characteristic: Characteristic to Device.notifyChange(of characteristic: Characteristic

[gbrooker]

It may even make sense to call onGetValue() asynchronously, to ensure that it doesn't block the read.

[Bouke]

Interesting thought on ProxyCharacteristic, however in many cases, the response will not be instantaneous, and should not block the main queue while it determines the latest value.

Ah yes, slightly different use-case. Instead of binding to the GET event, I think a nicer approach would be to hook into the characteristic listeners. So we would have the following two events for those interested:

• first characteristic listener did subscribe (for a particular characteristic)
• last characteristic listener did unsubscribe (for a particular characteristic)

In your application, you would hook into these events; you could periodically pull the latest data from the actual devices you're bridging and update the characteristics asynchronously.

I'm thinking that the API for this should be added to the Device, instead of doing this all on the characteristic. Maybe we might even need to introduce a DeviceDelegate, as we're looking to add more and more callbacks everywhere. Instead of having to manage all those callbacks, working with a single delegate results in a cleaner API.

[gbrooker]

I considered concentrating the callback for getValue at Accessory level and at Device level, and using delegates, but I found that it was much less elegant. When dealing with more than a handful of characteristics and accessories, it would necessitate a big if or switch statement to test what characteristics have changed on which acessories and farm it out to the right bit of code to update the status.

It is much more simple and elegant for an Accessory implementation to set onGetValue listeners on each characteristic which needs to be lazily updated. For instance in an Accessory implementation:

	self.thermostat.currentTemperature.onGetValue = { currentTemp in
                self.myDevice.query(for: .temperature) { mode, value in
                    if let temperature = value {
                        DispatchQueue.main.async {
                            self.thermostat.currentTemperature.value = temperature
                            logger.debug("Current Temp: \(temperature)")
                        }
                    }
                }
            }

This pattern is also very similar to homebridge accessory implementation, making it easier for a developer to port an implementation from homebridge to HAP.

A DeviceObserver could be an alternative to Device.onCharacteristicChange(), permitting a host app to monitor changes and update its UI or data structures accordingly, however the implementation of accessories needs an onGetValue hook at individual characteristic level. In other words, The Observer pattern would serve the layer above HAP, while the Characteristic call backs serve the implementation layer of individual accessories, underneath the HAP layer.

I'm thinking I should rename it to onDidGetValue() and calling asynchronously it on the .global(.userInitiated) concurrent queue to make usage clearer for callers.

[gbrooker]

I've added a DeviceObserver protocol as you suggested, and included functions for observing Identification requests, Characteristic changes and subscription and unsubscription of listeners. Is this what you had in mind ?

[gbrooker]

I've moved things around a little in Device.swift to facilitate merging with my other PR which also introduces changes in that file.

[gbrooker]

The linux travis builds were failing, as version 1.0.12 of libsodium is hardcoded in the travis.yml file, but is no longer available for download. I have updated travis.yml to use version 1.0.16, which is what is installed by brew on the macOS build.

[Bouke]

Ideally we don't compile the library each time, but use a packaged version. However libsodium is not packaged for Ubuntu 14.04, and at the time a third-party repository included an older version that we couldn't use. So while this is ok for now, hopefully Travis provides newer Ubuntu's in the future and we can switch to a packaged version instead.

[Bouke]

I've pushed a new DeviceDelegate in af0fa00, which can be used to monitor both changes to values in Characteristics and subscribe / unsubscribe similar to this PR. I removed the callbacks, as they would all result in retain cycles and that becomes quite hard to manage when working with multiple instances of Devices. Also depending on the use-case, there's now a single point to monitor all value changes.

I haven't gotten to changing the subscribe / unsubscribe events like I've explained above here, as it probably also includes the removal of WeakObjectSet.

Please let me know how this change works for you. I know it's not quite what you had in mind, but I still think you can create a structure similarly to the callbacks by registering those with your own delegate.

[gbrooker]

Can't say this is my preference. It solves the problem of the app monitoring the HAP server, and I have updated my other PR to use your DeviceDelegate accordingly. However it doesn't help for accessory implementation. I have an idea how to address this, and will revise this branch for discussion.

[gbrooker]

I've pulled out the changes which are now covered by DeviceDelegate in the master branch, and rebased. Still a work in progress, so no need to re-review yet.

[Bouke]

However it doesn't help for accessory implementation.

What do you mean by this, what use-case isn't covered?

[gbrooker]

What do you mean by this, what use-case isn't covered?

Here is a pretty lame diagram trying to explain what I mean:

MyHAPBridgeApp creates the HAP Device. It can use the DeviceDelegate to monitor and react to events in HAP. For instance, it can display the accessories (which may have been added statically by the app, or perhaps dynamically by a config file or network monitor), and update it's UI when certain events occur.

MyLamp is a superclass of Accessory.Lightbulb and interfaces a real-life light device to HAP. It's implementation must be totally independent of MyHAPBridgeApp and any other Accessory added to the bridge, and therefore should not rely on it's caller to pass down events that were detected from callbacks under the DeviceDelegate.

Instead of the closure hooks in my original implementation, I'm experimenting now with an AccessoryDelegate, in the same vein with what we now have on Device, which would permit Accessory implementations to monitor and react to HAP requests, independently of the bridge or app. Essentially this permits a plug-in to be written to implement a particular accessory type, which could be used by any app creating a HAP Device.

I've not explained this well, but I hope you get a clearer picture of what I mean.

I'm still testing a few different use cases, so I'm not ready for review yet.

[gbrooker]

OK I've tested this in a couple of different use cases, and am fairly happy this is now ready for review.

Essentially what it provides is an ability for an Accessory implementation e.g "MySpecialLamp" to be notified after certain characteristics were queried or set by HAP (such as lamp brightness), and to act accordingly. This is provided by an AccessoryDelegate protocol, in much the same way as we now have for DeviceDelegate (which is used by the calling app, rather than the underlying accessory implementations).

Also added a function to DeviceDelegate to notify the app of changes to the accessory list, and fixed another race condition in Server, as the Evergreen.Logger framework doesn't seem to be thread safe.

[gbrooker]

I have added an example accessory which uses the AccessoryDelegate protocol to provide a Temperature sensor and Humidity sensor fed by OpenWeatherMap. In main.swift, add your own apid code from openweathermap.com, and modify the location coordinates for your preferred location.

The branch has been rebased to the latest commit on master.

The code checks fail because travis cannot find sonar-scanner.

[gbrooker]

The OpenWeather looks like a nice example of how to implement a software-only sensor. I'd rather not include it with the core HAP package. You could setup a separate add-on package, which we could reference from the README.

If you wish, I thought it would be helpful to a developer to include some example code in the hap-server target. This code is not intended to be included in the core HAP framework itself.

[gbrooker]

I realise I had inadvertently left some changes in this PR which are now covered by PR #62 and PR #64
I have backed those out.

Perhaps it would be helpful for me to summarise the changes to master that are proposed in this PR as they have been a number of evolutions since the initial posts.

This PR adds an AccessoryDelegate property to the Accessory class. An Accessory can only have one delegate. The delegate has two required methods:

/// Characteristic's value was changed by controller. Used for notifying
func characteristic<T>(
    _ characteristic: GenericCharacteristic<T>,
    ofService: Service,
    didChangeValue: T?)

/// Characteristic's value was observed by controller. Used for lazy updating
func characteristic<T>(
    _ characteristic: GenericCharacteristic<T>,
    ofService: Service,
    didGetValue: T?)

The first method is called whenever HomeKit sets the value of a characteristic. The second method is called whenever HomeKit gets the value of a characteristic.

This simplifies the implementation of an Accessory subclass, by using the delegate function in order to catch changes made by HomeKit, or to lazily update its state only when HomeKit is interested in it.

In order to implement the above, the internal function getValue() of Characteristic is split into two internal functions. jsonValue() gets the value in jsonFormat, essentially what was previously getValue(), then a new internal function getValue(fromConnection:), which is symmetrical to the existing setValue(fromConnection:), is called from the 'characteristics()' endpoint on a GET request from the HomeKit controller. getValue(fromConnection:) then notifies the AccessoryDelegate (if any) and returns jsonValue()

Example

An example of using the didGetValue method of AccessoryDelegate is given in the OpenWeatherThermometer class in hap-server. The class implements a HAP Thermometer Accessory, and uses the accompanying OpenWeather class to provide temperatures. To create a sensor, first create an instance of OpenWeather for a given location and name, and provide it to the OpenWeatherThermometer.

let weather = OpenWeather(name: "Singapore", lat: 1.35, lon: 103.8, appid: appid)
let openWeatherSensor = Accessory.OpenWeatherThermometer(weather)

In order to function, OpenWeather needs a private appid String, which you can obtain for free from https://home.openweathermap.org/api_keys

[gbrooker]

As this PR has a lot of changes along the way, would you prefer I open a fresh PR with a single commit ?

[Bouke]

As this PR has a lot of changes along the way, would you prefer I open a fresh PR with a single commit ?

No need, if/when I merge this PR I can just squash-merge it.

[gbrooker]

I've rebased this PR on the current master, so is now compatible with the nio network stack

[gbrooker]

Removed the OpenWeather class, so hopefully this PR can now be merged into master.

[Bouke]

Removed the OpenWeather class

I'm considering to add additional targets including demos and other examples. Maybe we can include the openweather as such. E.g. HAPContrib or something like that.

so hopefully this PR can now be merged into master.

Sorry, no progress in this area yet. I'd like to land on some API that feels solid to call v1 of this library.

[gbrooker]

I'm considering to add additional targets including demos and other examples. Maybe we can include the openweather as such. E.g. HAPContrib or something like that.

Sure, happy to add it back to a contrib section at some point.

so hopefully this PR can now be merged into master.

Sorry, no progress in this area yet. I'd like to land on some API that feels solid to call v1 of this library.

Why can't this be part of the v1 API ? The AccessoryDelegate is essential for all my HAP based apps, and surely doesn't cause any harm if you don't want to use it.

[makleso6]

should add libsodium.23.dylib?

[makleso6]

preferred guard let self = self, let service = self.service else { return }

[gbrooker]

Agreed

[makleso6]

preferred use AnyObject instead class

[gbrooker]

Agreed

[gbrooker]

I have rebased this on the latest master, fixed the couple of comments mentioned above, and added one more function to the accessory delegate for accessory identification.

Can we merge this into master now ?