[homekit] Improve multiple instance management #14016

ccutrer · 2022-12-19T20:49:11Z

No description provided.

* allow addressing individual instances for most console commands * don't restart all instances if simply adding/removing instances on config change * clear stored info when removing instances Signed-off-by: Cody Cutrer <cody@cutrer.us>

Signed-off-by: Cody Cutrer <cody@cutrer.us>

timcharper · 2022-12-23T06:51:47Z

...penhab.io.homekit/src/main/java/org/openhab/io/homekit/internal/HomekitCommandExtension.java

@@ -206,4 +254,12 @@ private void printAccessory(String id, Console console) {
            }
        });
    }
+
+    private Collection<HomekitAccessory> getIntanceAccessories(@Nullable Integer instance) {
+        if (instance != null) {


it's a shame we need two different code paths for this. Could we just refer to instance 0 in the case of non-multi-instance setups?

OHHHH I get it now. Instance being null means "all accessories on all instances".

Any opinion on using Optional over nullable integer, instead?

Not really. openHAB requires null-type annotations on everything. HAP-Java doesn't use annotations, but instead uses Optional all over. So the homekit add-on seems to be a free-for-all. Honestly, I lean slightly towards annotations - they make the null-safety-checks seem as if they're more part of the language, than an add-on library. Yes, map and orElse are a bit nicer than nesting a conditional block, but the .get() for direct access when you know it can't possibly be null is slightly more annoying.

Anyhow, I'll add a javadoc to this method more clearly spelling out what a null instance means

Fair enough :)

timcharper · 2022-12-23T06:57:35Z

bundles/org.openhab.io.homekit/src/main/java/org/openhab/io/homekit/Homekit.java

@@ -45,17 +45,42 @@ public interface Homekit {
    void allowUnauthenticatedRequests(boolean allow);

    /**
-     * returns list of HomeKit accessories registered at bridge.
+     * returns list of HomeKit accessories registered on all bridges.


Suggested change

* returns list of HomeKit accessories registered on all bridges.

* returns list of HomeKit accessories registered on all bridge instances.

timcharper · 2022-12-23T07:05:45Z

bundles/org.openhab.io.homekit/src/main/java/org/openhab/io/homekit/internal/HomekitImpl.java

+    @Override
+    public void clearHomekitPairings(int instance) {
+        if (instance < 1 || instance > authInfos.size()) {
+            return;


should we log a warning here?

Yeah, I can. It shouldn't happen from the command extension, which is already doing range checking, but if this ever gets called from say a REST API in the future, then it would be good to have.

timcharper · 2022-12-23T07:06:28Z

bundles/org.openhab.io.homekit/src/main/java/org/openhab/io/homekit/internal/HomekitImpl.java

+    @Override
+    public void pruneDummyAccessories(int instance) {
+        if (instance < 1 || instance > authInfos.size()) {
+            return;


log a warning?

timcharper · 2022-12-23T07:09:09Z

bundles/org.openhab.io.homekit/src/main/java/org/openhab/io/homekit/internal/HomekitImpl.java

-                logger.warn("cannot resolve the Pv4 address / hostname {}.",
-                        networkAddressService.getPrimaryIpv4HostAddress());
+    private void startHomekitServer(int instance) throws IOException, InvalidAlgorithmParameterException {
+        logger.trace("starting HomeKit bridge instance {}", instance + 1);


It seems like instance is sometimes 0 based, as seen in this function, and in other functions, 1 based (ie pruneDummyAccessories). This seems likely that some poor soul will get it wrong (and by poor soul, I mean myself). I'm wondering if we should normalize as 0-based at the UI level (parsing the command), and be consistent?

Yeah, I think we need to standardize on if the variable is index, it's 0-based, but if it's instance it's one-based. I'm not sure I want to be exposing to the user that things are actually 0-based, since for the average user they'll only have 1 instance, and having that instance be "instance 0" would be confusing. The instance number (as 1-based) is also part of the bridge name when you add it in HomeKit (after index 0). @yfre WDYT?

agree, even for me who used to count many things starting from 0 in the past :) would be a little strange to refer to "instance 0". i would vote to use "1" and keep it consistence in the communication to the user.

timcharper

Looks good! A few small suggestions, I think. More strongly the mixing of instance being 0-based vs 1-based, but overall looks great :)

Signed-off-by: Cody Cutrer <cody@cutrer.us>

openhab-bot · 2022-12-29T21:47:27Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/homekit-stops-working-after-some-hours-openhab-3-4-0/142502/17

yfre

LGTM

yfre · 2022-12-29T21:51:33Z

thank you. good refactoring to support improve support for multiple instances

ccutrer

Whoops, @timcharper, I guess I didn't get my comments published last week.

ccutrer · 2022-12-23T14:26:33Z

...penhab.io.homekit/src/main/java/org/openhab/io/homekit/internal/HomekitCommandExtension.java

@@ -206,4 +254,12 @@ private void printAccessory(String id, Console console) {
            }
        });
    }
+
+    private Collection<HomekitAccessory> getIntanceAccessories(@Nullable Integer instance) {
+        if (instance != null) {


Not really. openHAB requires null-type annotations on everything. HAP-Java doesn't use annotations, but instead uses Optional all over. So the homekit add-on seems to be a free-for-all. Honestly, I lean slightly towards annotations - they make the null-safety-checks seem as if they're more part of the language, than an add-on library. Yes, map and orElse are a bit nicer than nesting a conditional block, but the .get() for direct access when you know it can't possibly be null is slightly more annoying.

Anyhow, I'll add a javadoc to this method more clearly spelling out what a null instance means

ccutrer · 2022-12-23T14:40:34Z

bundles/org.openhab.io.homekit/src/main/java/org/openhab/io/homekit/internal/HomekitImpl.java

-                logger.warn("cannot resolve the Pv4 address / hostname {}.",
-                        networkAddressService.getPrimaryIpv4HostAddress());
+    private void startHomekitServer(int instance) throws IOException, InvalidAlgorithmParameterException {
+        logger.trace("starting HomeKit bridge instance {}", instance + 1);


Yeah, I think we need to standardize on if the variable is index, it's 0-based, but if it's instance it's one-based. I'm not sure I want to be exposing to the user that things are actually 0-based, since for the average user they'll only have 1 instance, and having that instance be "instance 0" would be confusing. The instance number (as 1-based) is also part of the bridge name when you add it in HomeKit (after index 0). @yfre WDYT?

ccutrer · 2022-12-23T14:43:52Z

bundles/org.openhab.io.homekit/src/main/java/org/openhab/io/homekit/internal/HomekitImpl.java

+    @Override
+    public void clearHomekitPairings(int instance) {
+        if (instance < 1 || instance > authInfos.size()) {
+            return;


Yeah, I can. It shouldn't happen from the command extension, which is already doing range checking, but if this ever gets called from say a REST API in the future, then it would be good to have.

Signed-off-by: Cody Cutrer <cody@cutrer.us>

ccutrer · 2022-12-29T22:00:58Z

@yfre : I can't find when your code comments went on GitHub, I only see them in my email. anyhow, I addressed the interfaceName getting logged. I did not change the temp var for localNetworkInterface - it's required, because networkInterface is @nullable, and the compiler continues to complain if you return it directly since theoretically another thread could change it to null between the non-null check and returning it.

ccutrer · 2023-01-03T21:58:36Z

@lolodomo and/or @jlaur : this is good to go from our end, just need a maintainer review. thanks!

jlaur

Looks good, thanks!

* [homekit] improve instance management * allow addressing individual instances for most console commands * don't restart all instances if simply adding/removing instances on config change * clear stored info when removing instances * [homekit] reset instance identity when clearing pairings * [homekit] log the actual interface we looked up Signed-off-by: Cody Cutrer <cody@cutrer.us>

ccutrer requested review from andylintner and yfre as code owners December 19, 2022 20:49

ccutrer added 2 commits December 19, 2022 13:50

[homekit] improve instance management

deb854d

* allow addressing individual instances for most console commands * don't restart all instances if simply adding/removing instances on config change * clear stored info when removing instances Signed-off-by: Cody Cutrer <cody@cutrer.us>

[homekit] reset instance identity when clearing pairings

21b6d58

Signed-off-by: Cody Cutrer <cody@cutrer.us>

ccutrer force-pushed the homekit-instance-management branch from 9e56da0 to 21b6d58 Compare December 19, 2022 20:51

ccutrer changed the title ~~Improve HomeKit instance management~~ [homekit] Improve multiple instance management Dec 19, 2022

ccutrer mentioned this pull request Dec 19, 2022

[homekit] All accessories are in "No response" state after upgrade to iOS 16.2 (and Home upgrade) #13949

Closed

jlaur added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Dec 20, 2022

timcharper reviewed Dec 23, 2022

View reviewed changes

timcharper approved these changes Dec 23, 2022

View reviewed changes

ccutrer mentioned this pull request Dec 23, 2022

[homekit] Add additional troubleshooting steps for homekit #14046

Merged

[homekit] address review comments

3c58aae

Signed-off-by: Cody Cutrer <cody@cutrer.us>

yfre approved these changes Dec 29, 2022

View reviewed changes

This was linked to issues Dec 29, 2022

[homekit] ColorTemperature Characteristic - item Dimmer not supported #12517

Closed

[homekit] HomeKit Binding #14094

Open

ccutrer commented Dec 29, 2022

View reviewed changes

This was unlinked from issues Dec 29, 2022

[homekit] ColorTemperature Characteristic - item Dimmer not supported #12517

Closed

[homekit] HomeKit Binding #14094

Open

[homekit] log the actuall interface we looked up

6da619d

Signed-off-by: Cody Cutrer <cody@cutrer.us>

jlaur approved these changes Jan 3, 2023

View reviewed changes

jlaur merged commit 47f5489 into openhab:main Jan 3, 2023

jlaur added this to the 4.0 milestone Jan 3, 2023

ccutrer deleted the homekit-instance-management branch November 29, 2023 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[homekit] Improve multiple instance management #14016

[homekit] Improve multiple instance management #14016

ccutrer commented Dec 19, 2022

timcharper Dec 23, 2022

timcharper Dec 23, 2022

ccutrer Dec 23, 2022

timcharper Jan 3, 2023

timcharper Dec 23, 2022

timcharper Dec 23, 2022

ccutrer Dec 23, 2022

timcharper Dec 23, 2022

timcharper Dec 23, 2022

ccutrer Dec 23, 2022

yfre Jan 3, 2023

timcharper left a comment

openhab-bot commented Dec 29, 2022

yfre left a comment

yfre commented Dec 29, 2022

ccutrer left a comment

ccutrer Dec 23, 2022

ccutrer Dec 23, 2022

ccutrer Dec 23, 2022

ccutrer commented Dec 29, 2022

ccutrer commented Jan 3, 2023

jlaur left a comment

	* returns list of HomeKit accessories registered on all bridges.
	* returns list of HomeKit accessories registered on all bridge instances.

[homekit] Improve multiple instance management #14016

[homekit] Improve multiple instance management #14016

Conversation

ccutrer commented Dec 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timcharper left a comment

Choose a reason for hiding this comment

openhab-bot commented Dec 29, 2022

yfre left a comment

Choose a reason for hiding this comment

yfre commented Dec 29, 2022

ccutrer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccutrer commented Dec 29, 2022

ccutrer commented Jan 3, 2023

jlaur left a comment

Choose a reason for hiding this comment