hap-java / HAP-Java

Java implementation of the HomeKit Accessory Protocol
MIT License
152 stars 83 forks source link

new release? #128

Closed yfre closed 3 years ago

yfre commented 3 years ago

a number of good improvements were made since last official release.
should we make a new official release? what do you think @timcharper @andylintner @ccutrer

andylintner commented 3 years ago

I can take a stab at getting new coordinates from sonatype and setting up GitHub actions to upload. Need admin access to the repo though - I appear to have lost it when migrating. @timcharper @ccutrer

ccutrer commented 3 years ago

Should be restored now

andylintner commented 3 years ago

Thanks @ccutrer

I've setup a basic maven build in Github Actions, given the changes at Travis-CI

I've also filed a ticket to register the coordinates with Sonatype's OSSRH: https://issues.sonatype.org/browse/OSSRH-66356

Once that's closed, we can start publishing snapshot builds.

Does anyone have any opinions on whether we should wait to cut a release? Anything that needs to be done first?

J-N-K commented 3 years ago

For a release you could check if all changes from https://github.com/J-N-K/HAP-Java have been incorporated. That would enable openHAB to return to regular releases.

andylintner commented 3 years ago

@J-N-K - I did a diff between your fork and the master branch here. I identified two changes. One I brought over with a few minor changes in #143, which I'd like you to look at.

The other was this change of yours: https://github.com/J-N-K/HAP-Java/commit/a6972ca1e247e730ea425da306534f5b2faab653

I'm thinking we actually want to set ev to match the value of subscriber.isPresent(), but I wanted to hear from you what that was fixing first.

The spec says

Boolean indicating if event notifications are enabled for this characteristic.

J-N-K commented 3 years ago

I have no idea. If I see it correctly, that was before my fork. All changes I made are from PR here (except the build changes). Maybe @yfre knows.

yfre commented 3 years ago

hm. in my local code base (which should be in sync with master branch here), i have this. (@andylintner as you suggested, set ev only if subscriber.isPresent. and dont set "ev" in other cases.

    return futureValue
        .exceptionally(
            t -> {
              logger.warn("Could not retrieve value " + this.getClass().getName(), t);
              return null;
            })
        .thenApply(
            value -> {
              JsonArrayBuilder perms = Json.createArrayBuilder();
              if (isReadable) {
                perms.add("pr");
              }
              if (isWritable) {
                perms.add("pw");
              }
              if (subscriber.isPresent()) {
                perms.add("ev");
              }
              JsonObjectBuilder builder =
                  Json.createObjectBuilder()
                      .add("iid", instanceId)
                      .add("type", shortType)
                      .add("perms", perms.build())
                      .add("format", format)
                      .add("description", description);
              if (isReadable) setJsonValue(builder, value);
              return builder;
            });
  }
andylintner commented 3 years ago

Oh, sorry! I read the diff backwards. It was changed recently in https://github.com/hap-java/HAP-Java/commit/87bab73be807fec98b4f51b4a97326e3fd21045a

But I think that change may break events, at least according to the spec. I'll see if I can replicate the original issue that change was supposed to fix. My initial thought is the Home app may now be enforcing the perms that indicate whether specific characteristics are changeable. It might be as simple as setting the ev property based on the same criteria we're using for the ev perm.

andylintner commented 3 years ago

Tested, and I guess it doesn't matter. The spec isn't entirely clear on this, so I'm going to just leave it as is since everything is working.

andylintner commented 3 years ago

Now that we have @J-N-K 's changes merged in, I think we're good for a release. I'll give it a few days in case anyone has any objections they want to raise.

yfre commented 3 years ago

Oh, sorry! I read the diff backwards. It was changed recently in 87bab73

But I think that change may break events, at least according to the spec. I'll see if I can replicate the original issue that change was supposed to fix. My initial thought is the Home app may now be enforcing the perms that indicate whether specific characteristics are changeable. It might be as simple as setting the ev property based on the same criteria we're using for the ev perm.

it looks like home app is not following the spec. if we send "ev:false" with color temperature then home app crashes. therefore #87bab73 was needed.

yfre commented 3 years ago

@andylintner thank you a lot for taking care of the release!

gjvanderheiden commented 3 years ago

really really would like to see the lib updates (majority of current pr's) to be in this release, as it is a major update.

andylintner commented 3 years ago

I left a comment on #130 - I'm inclined to merge your change @gjvanderheiden , but want to hear back from someone else who's tested it.

@J-N-K or someone else familiar with OpenHAB - is it going to be a problem to require the updated BouncyCastle lib? The new lib fixes a problem we were working around, but the updated code won't work with the old lib. I assume it will be fine because of OSGI magic?

J-N-K commented 3 years ago

Please note that I am no longer associated with openHAB.

This will not work OOTB, you need to update all dependencies in

https://github.com/openhab/openhab-addons/blob/main/bundles/org.openhab.io.homekit/pom.xml

You also need to check if the new versions of javax.json and javax.json-api are available in the target platform. If not, you need to remove it those from the dep.noembedding property.

andylintner commented 3 years ago

2.0.0 is released. It's been deployed on Nexus and will be available in Central in the next few hours.