[homekit] delay starting until a particular StartLevel

[ccutrer]

instead of a blind "delay by seconds", the new default won't even attempt to start until items are loaded (both file-based and UI-based), with an additional config to let the user delay it any further (in case they have items coming from JSR223 scripts that can't run until at least SL20 anyway).

[yfre]

LGTM
i was not aware of ReadyService. many hidden gems in OH core.
it makes it easier and clearer.

[ccutrer]

@lolodomo: can we get this merged? it's the last of the stability PRs, and I'd consider it a higher priority to get in before the next milestone build than #13879 which is a feature-add.

[lolodomo]

I'm not comfortable with this change. Why a binding should wait for items loading ? In principle, a binding is not dependent on items, it "works" with channels.
@openhab/add-ons-maintainers : WDYT ?

[ccutrer]

This isn't a binding though. It's an IO/misc addon. And it most definitely works with items, not channels.

[ccutrer]

I'm sorry if that response seemed abrupt/brash. I saw the notification and wanted to catch you while you're still online if possible, so responded from my phone. I really appreciate your reviews. I do stand by the comment though, that HomeKit is more akin to a UI than a binding. It deals exclusively with Items. It already has a kludgey mechanism of attempting to wait for items to be loaded, because of the problems that can be caused when they're not all there yet. This PR just replaces that mechanism with a proper one exposed by core specifically for this purpose.

[lolodomo]

@kaikreuzer implemented the start level stuff in the core framework, he could certainly comment/review that PR better than myself.

[J-N-K]

I'm neither Kai nor an add-ons maintainer, but just my 2 cents from core-perspective.

An IO add-on like HomeKit quite often has different needs compared to an ordinary binding. It exposes something from openHAB to the world (in most cases this is items, the cloud connector or hue emulation do something similar). This is also true for persistence add-ons which depend on items.

In this case (due to the design of HomeKit on Apple's side) there will be issues when not all items are present (IIRC correctly from the time I used HomeKit there is an issue with the "open" version of HomeKit that you can't programmatically set the location of a device (exposed item) and therefore the removal/addition of a device always requires user-interaction to set the location again).

The add-on developer decided to use an API exposed by core to specifically address the issue of elements not being present during startup. LGTM.

[lolodomo]

Sorry guys if I did not realize this binding is not a "standard" binding. Of course, I have not a perfect knowledge of the 4xx bindings in that repo.
I am just not familiar with this core API and that is the reason I suggest that PR to be reviewed by someone else. I am blocking nothing there.

[ccutrer]

Sorry guys if I did not realize this binding is not a "standard" binding. Of course, I have not a perfect knowledge of the 4xx bindings in that repo.

I am just not familiar with this core API and that is the reason I suggest that PR to be reviewed by someone else. I am blocking nothing there.

👍

No problem! I don't envy your role looking after so many pieces, and with varying levels of familiarity with openHAB by the PR author (even if they are technically the CODEOWNER for that section of code). I'll be the first to admit that specifically the HomeKit addon here has had a rocky history. I've personally been using it (and contributing to it to some extent) for about 4 years now, but in the "early days" I was barely familiar with Java, or the openHAB internals. And I'll admit having been frustrated with and ignoring this addon for large periods of time in the intervening years. Such a review from you if I had sent something like this back then would definitely be what someone-like-me-from-then would need. Gratefully, especially in the past ~6 months I've greatly leveled up both my Java skills and my familiarity with openHAB internals. Doesn't mean you are familiar with me personally, or should be expected to be. Sooo many addons and contributors!

So yeah, thanks for the review, and I apologize again if it seemed like I was upset. That was not my intent at all. I just know we're getting really close to feature freeze for 3.4.0, and this is the last of several major stability improvements for HomeKit. And time zones! I was hoping to reduce the lag between communications because I know I'm generally in a fairly separated time zone from most of the openHAB contributors.

[kaikreuzer]

I fully agree with @ccutrer and @J-N-K that the start levels make a lot of sense for such I/O add-ons.
While for bindings, the start level service ensures that thing handlers are not initialized before a certain start level is reached, we have no similar feature in place for other add-on types, so it is fair that this is done in the add-on code itself.

I would nonetheless try to keep it as simple as possible - especially as this implementation here will be the prototype and template for others to come.
Similar to bindings, I wonder if we really need the flexibility here to allow the user to choose a start level or if we can simply define, at which start level an I/O add-on should start its processing. As the idea is to expose items to the outside world, I would claim that start level 30 would be a good choice.
In such a case, the registration could be changed to

readyService.registerTracker(this, new ReadyMarkerFilter().withType(StartLevelService.STARTLEVEL_MARKER_TYPE)
    .withIdentifier(Integer.toString(StartLevelService.STARTLEVEL_STATES)));

and we would really only need to add code to onReadyMarkerAdded(), which makes the add-on start the processing and onReadyMarkerRemoved() to stop the processing again.

The code that is currently found in https://github.com/openhab/openhab-addons/pull/13877/files#diff-a0ee6722d485ec547013afd0fa1efeeb24b5bc7803781f7d212420ea94547e04R178-R213 could then be removed in favor of a lower complexity.

Wdyt?

[ccutrer]

You're probably right. And yes, that's exactly why I'm defaulting to states right now. But I had done it this way to accommodate my own config where I have JSR223 scripts that generate a large number of my items dynamically. And due to openhab/openhab-core#3199, I often reach higher start levels before that generation completes.

[ccutrer]

I've removed the configurable piece. It compiles, but I don't have time to test it tonight. I can suffer through a custom build locally until openhab/openhab-core#3199 is resolved (hopefully a custom build of a proposed resolution to that, rather than the original version of this PR). It's definitely the proper solution from the HomeKit addon's perspective to always just start at STARTLEVEL_STATES.

[kaikreuzer]

Thanks @ccutrer!
You can even simplify the code a bit more by using

readyService.registerTracker(this, new ReadyMarkerFilter().withType(StartLevelService.STARTLEVEL_MARKER_TYPE)
    .withIdentifier(Integer.toString(StartLevelService.STARTLEVEL_STATES)));

In that case, you don't need to check for the startlevel within the on... methods anymore (and you get less calls to these methods).

[kaikreuzer]

@ccutrer See #13914 for what I mean.

[ccutrer]

Thanks @ccutrer! You can even simplify the code a bit more by using

readyService.registerTracker(this, new ReadyMarkerFilter().withType(StartLevelService.STARTLEVEL_MARKER_TYPE)
    .withIdentifier(Integer.toString(StartLevelService.STARTLEVEL_STATES)));

In that case, you don't need to check for the startlevel within the on... methods anymore (and you get less calls to these methods).

👍 . I think I noticed that after I pushed last night, but had to get kids to bed. Thanks for finishing it up for me.