Coalesce notifications to avoid blocked accessories

[tgu]

For me this was a critical change to make my application work. I have several accessories that updates simultaneously, and sending individual notifications will eventually lead to that the accessory is blocked from further updates. The HAP Specification states that

Network-based notifications must be coalesced by the accessory using a delay of no less than 1 second. Excessive or inappropriate notifications may result in the user being notified of a misbehaving accessory and/or termination of the pairing relationship.

My proposal is to coalesce the notifications in the server

[Bouke]

As I understand this correctly, any change to the characteristic will be delayed by 0-2 seconds. And in situations when there's no pending event, it will always be 2 seconds? That would be a major regression for real-time use-cases. Can you look into a different approach where we would only delay when a second change arrives within a certain amount of time of another message? So something like this:

1. PUT /characteristics with X updates; these X updates are coalesced and sent immediately and a timer is set for 2 seconds
2. Another PUT /characteristics with Y updates within 1 second of the previous, the timer hasn't expired, these updates are kept in a buffer
3. Another PUT /characteristics with Z updates within 0.75 seconds of the previous, the timer still hasn't expired, these updates are added to the buffer
4. The timer expires, Y+Z are sent to the listeners

Also, we would need some tests for this. XCTest has facilities to test asynchronous code. I could also help with some tests if you get stuck.

[Bouke]

Oh and further: great to see you working on HAP and that it is helping your use-case. I'd be interested to hear what you're using this package for?

[tgu]

Maybe something like this, (the reader/writer synchronisation of queue needs to be adressed)

    func append2(characteristic: Characteristic) {
        DispatchQueue.main.async {
            self.queue.append(characteristic)
            if self.queue.count == 1 {
               self.nextAllowableNotificationTime = max(self.nextAllowableNotificationTime,DispatchTime.now())
                var sendCount = 0
                SomeOtherQueue.asyncAfter(deadline: self.nextAllowableNotificationTime ) {
                    defer { self.queue.removeAll() }
                    while DispatchTime.now() < self.nextAllowableNotificationTime + 1 && self.queue.count > sendCount {
                        sendCount = self.queue.count
                        usleep(10_000)  // 10 ms wait
                    }

                    guard let event = Event(valueChangedOfCharacteristics: self.queue) else {
                        return logger.error("Could not create value change event")
                    }
                    let data = event.serialized()
                    logger.info("Value changed, notifying \(String(describing: self.listener?.socket?.remoteHostname)), event: \(data)")
                    self.listener?.writeOutOfBand(data)
                    self.nextAllowableNotificationTime = .now() + 1
                }
            }
        }
    }

I am using HAP to interface my installation of OneWire and 433 Mhz devices

[Bouke]

I've committed a few tests that I think we should address:

• testDelayMultipleEvents: subsequent updates should be sent at an interval of 1 second
• testDelayMultipleEventsCoalescence: multiple events within the interval should coalesce
• testDelayMultipleEventsCoalescenceFiltering: would be nice to address, but can be handled separately, if subsequent updates cancel-out coalesced updates, only sent the 'relevant' (last) update

I've been looking into your code and also trying a stab at this myself. I'll share some code hopefully later this weekend, so we can discuss.

[Bouke]

How do you feel about this? https://github.com/Bouke/HAP/compare/8e33de8...tgu-coalesce. I've taken out a few queues. Maybe I'm overlooking a reason why those might be necessary? I haven't tested them out, besides from running the tests.

I've also fixed an issue in master where for every update two events were sent. This was something I accidentally introduced when rewriting a portion of Characteristic. I'm glad to see the new tests catching this issue.

[Bouke]

On a somewhat related note; there's currently two ways of implementing your custom Characteristic; either implementing the protocol Characteristic or by using the generic GenericCharacteristic<>. As things currently stand, I don't see much benefit from the protocol... rather only annoyances. Take setting a new value for example, could you just assign to [instance].value (GenericCharacteristic) or use the protocol's setValue? Also I can't make the protocol conform to Hashable, which results in the rather ugly workaround with ObjectIdentifier in WeakObjectSet.swift.

So I'm thinking of dropping the protocol and introducing a type-erased AnyCharacteristic which allows mixed-type collections of GenericCharacteristic<>.

My question to you is: does the current protocol approach bring you any benefits that the alternative couldn't provide?

[tgu]

I have failed to find a solution that uses only the main queue. When several values are updated in a short time then the first notification with only one update will be send immediately and the rest of the updates are queued up to the next notification. I can solve that by using two queues and let the second queue wait for more updates. Since a wait blocks the queue I have found no other way to do that. The extra complication is the need for mutual exclusion of write and read to the queue.

[Bouke]

When several values are updated in a short time then the first notification with only one update will be send immediately and the rest of the updates are queued up to the next notification.

You'd want to wait for a small amount of time for additional updates to enter the queue? Some sort of a grace period? I think I'm fine with the alternative, being getting the first update out as quickly as possible. What use case might require such a grace period that you're proposing?

[tgu]

What use case might require such a grace period that you're proposing?

In my application I read values from five sensors and make an update. With the grace period the updates are coalesced into one notification and sent with a delay of 5 ms, without there will be two notifications. Not a big deal for my application. I am happy with both alternatives.

[Bouke]

Ah ok; yeah I can see you wanting some sort of batch operation for updating the characteristics. I think it's fine to explore if and how that could be supported. I'd rather not delay all notifications if there's no need.

[Bouke]

I've merged a derivate from your PR, which should cover some use-cases. I'd be interested to see if this works for you, or if further refinements are required.