Support for resuming Hue event stream

[duvholt]

Before this patch the Hue app goes out of sync with the current Bifrost state if changes are happening while the app is closed. For example by changing brightness in zigbee2mqtt directly. This seems to be because Hue doesn't do a full refresh when opening the app and instead relies on using Last-Event-Id to resume the event stream where it left off.

I've therefore implemented support for storing the last 32 events in a circular buffer (actually a VecDeque) which can be sent when the client connects if it provides an event-id. I did some quick reverse engineering of the Hue bridge and it looks like it just stores the last 15 events so it's very much possible to fill this buffer with other changes and miss an entity update. Since z2m seems to be more chatty I think it makes to increase the number of stored events even more. I doubt it will affect performance and memory usage much, but I haven't looked into it.

I ended up moving event id handling down to Resources and instead generate it when first receiving the event. I'm not super happy about putting prev_ts and idx directly in Resources, so I'll gladly take suggestions on how to improve that.

[chrivers]

Very exciting! Thank you for working on this :)

I was wondering about this problem too, but I didn't realize that the hue app sends a "last seen"-identifier!

I'll take a look at it immediately 👍

[chrivers]

There's some really good stuff in here - and it's super exciting that you found a way to make the stream resumable :)

I also agree with you that prev_ts and idx don't sit well in Resources.

I think I see a solution;

1. We introduce a new struct HueEventStream (could put it in src/server/hueevents.rs or similar)

2. ..which contains both the channel, as well as the buffering mechanics (LastEvents may or may not survive this refactor)

3. ..which brings prev_ts and idx into this local scope, where they are relevant

It's a rough sketch - but you see what I mean?

[duvholt]

Yep, that makes sense to me.

I did a quick test with 1024 events in the buffer and didn't really notice any difference in performance or memory usage. 1024 is probably overkill so maybe we can set it to something like 128? We could also be fancy and only keep the last relevant change per entity, but I'm not sure if that's worth the trouble.

Another possible optimization I noticed is to chunk changes. Not sure if that would make any difference, but I noticed that the hue bridge does it for changes to grouped lights.

[chrivers]

1024 events in rust structs would only cost a handful of KB, so no worries there.

I think 128 is a good starting point. Let's make the queue size an argument to HueEventStream::new(), and maybe put a const DEFAULT_QUEUE_SIZE on it too?

We shouldn't spend time trying to be overly fancy here, I don't think it's worth it.

In the future, we could do something fancier, like adding an internal "last modified" timestamp to all objects, allowing us to synthesize history for clients that connect after the queue limit. Or, as an alternative, we could have bifrost simply synthesize the entire state as big chunky update, if someone connects with an old timestamp.

Or maybe just make the request fail, if the timestamp is expired? That might be enough to make the app connect from the top?

In any case, all that is overkill for now :)

[duvholt]

The HueEventStream abstraction is much nicer than what I started with. Do you think it should be used via Resources like I did now or does it make sense to refer to it directly?

[chrivers]

Nice improvement! Seems to be working well :)

I think we should collapse these:

    pub fn hue_channel(&self) -> Receiver<(String, EventBlock)> {
        self.hue_event_stream.subscribe()
    }

    pub fn hue_events_sent_after_id(&self, id: &str) -> Vec<(String, EventBlock)> {
        self.hue_event_stream.events_sent_after_id(id)
    }

into

    pub fn hue_event_stream(&self) -> &HueEventStream {
        &self.hue_event_stream
    }

Then we use this to get a reference to the event stream, and call the functions on there.

Also, I'm not a big fan of having the String in the tuple type for the events. How about this instead (feel free to come up with a better name):

struct HueEventRecord {
    timestamp: DateTime<Utc>,
    index: u32,
    block: HueEventBlock,
}

This could then have a function on it to generate the sse timestamps.

What do you think?

[duvholt]

Updated PR with suggestions now. Ended up just using HueEventRecord since naming things is hard 😄

[chrivers]

This is looking really good!

I just have some nitpicks (which means we're very close to merging 😆)

Oh, and can you please rebase on top of master? It's had some updates since your branch.

Thank you very much for this exciting new feature :)

[duvholt]

I think I've addressed all the feedback now (which I appreciate as the code is much better now than what I hacked together yesterday 😄 )

[chrivers]

Just tested it - it works great!

Thank you very much for your contribution! :)