Better mechanism for optional characteristics needed

[timcharper]

TL;DR

The current mechanism of implementing additional interfaces to add support for optional characteristics is non-conducive to supporting dynamic sets of optional characteristics.

Background and current workaround

In my fork of Openhab-homekit, I support low battery notification. However, whether or not an accessory actually has low-battery notification support is based on the items configuration. As a workaround, in order to avoid creating two classes for sensors that potentially have battery status reporting support, I create a faux-battery notification implementation that always returns batteryLow=false).

Limitations of providing substitute static-values

Such workarounds are not possible when adding a characteristic that fundamentally changes the presentation of an accessory in homekit. For example, with the thermostat, the addition of optional characteristics Characteristic.CoolingThresholdTemperature and Characteristic.HeatingThresholdTemperature causes the UI to show a temperature range, rather than a single temperature, when the AUTO TargetCoolingHeatingState is active.

Proposal and way forward

I propose that accessories classes only specify getters, setters, and subscribers for their base class.

Then, for optional characteristics, I propose that the accessory has an overridable getter which gets some optional characteristic, with the default value being none:

@Override
protected Optional<HeatingThresholdCharacteristicSupport> heatingThresholdTemperatureCharacteristic() {
  heatingThresholdItem.map(item -> {

    new HeatingThresholdCharacteristicSupport() {
      @Override
      CompletableFuture<Double> getHeatingThresholdTemperature() {
        ...
      }
      ...
    }
  });
}

The HeatingThermostat trait can be modified such that the interface methods are lifted out in to HeatingThresholdCharacteristicSupport, retaining backwards compatibility (and potentially marking as deprecated for removal in next minor release of HAP-Java). After such a lift, the interface would be effectively empty:

package com.beowulfe.hap.accessories.thermostat;

import com.beowulfe.hap.HomekitCharacteristicChangeCallback;
import java.util.concurrent.CompletableFuture;

@Deprecated
public interface HeatingThermostat extends BasicThermostat, HeatingThresholdCharacteristicSupport {
}

[timcharper]

@beowulfe any thoughts on this? @ccutrer WDYT?

[ccutrer]

I definitely agree this needs reworked. For example, LightBulb - I'm pretty sure HomeKit supports bulbs that are both dimmable and color setting. Fans can have various controls that are all optional. On security system sensors (motion, leak, contacts, etc.) they can all support tamper and fault statuses, in addition to battery. I'm not a seasoned Java developer, so I'm not sure the best way to accomplish it. In Ruby I would have a mixin that can be added to individual instances at runtime - right now on the BatteryStatus implementation in @timcharper's fork of the OpenHAB HomeKit addon, it's annoying that you have to re-implement the subscription and getter/setters for each sensor. My initial "fixit" for making them dynamic per object was adding an "isSupported()" type method for each optional characteristic, but I like the "getXCharacteristic" method better - it removes the weirdness of "isSupported", it reduces the actual implementation duplication when a characteristic is shared by different types of accessories, and it models the HAP protocol better. So... overall 👍 from me.

[ccutrer]

@beowulfe : any thoughts on this? I'm starting work in the OpenHAB service using item metadata rather than tags (so that I can specify a bridge association), and it'd be great to have this resolved first so that I don't have to pull the DimmableLightbulb/ColorfulLightbulb confusion through.

[ccutrer]

Oh hey look, you started too: andylintner/openhab2@8ede0bc

[ccutrer]

I think @timcharper's idea is best, and I've taken a first stab at it with BatteryStatus: https://github.com/ccutrer/HAP-Java/tree/optional_characteristics

It's working just fine with a mock motion sensor in the sample project. Next step is to apply it to all the battery-capable accessories, and then refactor Lightbulb to do the same with brightness, and HSV, and deprecating DimmableLightbulb and ColorfulLightbulb (and giving them default implementations that return this).

[ccutrer]

Ahhhh, so much cleaner: ccutrer/openhab-addons@6372403 (the last few commits of https://github.com/ccutrer/openhab2-addons/commits/homekit; this is my temporary integration branch so I'm often rebasing things around)

[timcharper]

This is awesome; oh a side now (since we don't have a channel to discuss this I'm using this thread here), I think feel like it would be good to focus on getting a new point release out, and then integrating existing openhab changes with master, and then focus on tackling the metadata change. WDYT? I'm really eager to start merging.

[ccutrer]

+1. The framing fix is most important to get into end users hands ASAP. But then I also want to make it easier to switch to optional characteristics, and that's much easier to remove classes if they never made it into an actual release. What's the status of your thermostat work? Did any of it get merged after 1.1.4 (and now should be changed to use the optional characteristic stuff)?

[timcharper]

@ccutrer thermostat does not use optional characteristic stuff; it uses the same static-signal workaround that is used by battery-less sensor accessories. But it would totally benefit from it. Along with some other accessories which I'd like to support, such as air quality sensors and humidistats

[andylintner]

Agreed we should get out a point release. Anything else you'd like to see in it?

Regarding the root issue, I do like the proposal. Tangentially, I have been wondering if the Accessory interfaces were a good idea. They make for simpler development, but it doesn't really match what HomeKit allows. An accessory is really just a collection of services, and maybe we should represent them as such in code...

So, Accessory could turn into an interface with a getServices() method.

[ccutrer]

You mean like https://github.com/beowulfe/HAP-Java/blob/master/src/main/java/com/beowulfe/hap/HomekitAccessory.java#L68 ? :)

I think the accessory structure we have now is decent, as long as it easily allows composing optional attributes. I am curious about how the Home app reacts to an accessory with multiple services. It's nice having the spec now. The spec talks about profiles where certain devices (locks, IP cameras, doorbells) should be an accessory composed of multiple services. I have an August Pro lock that is native HomeKit, and in the HomeKit UI it shows only the lock itself, but it does have a door contact physically in it (and exposed in the August app). I wonder if it's exposing the door contact as a separate service to HomeKit, but homekit just isn't showing it. All this to say... for the vast majority of use cases, the accessory structure we have now is sufficient. In the event you want to implement something more custom, the interface does exist to allow you to do so. Though the characteristics are inside of the impl package, and that might be a bit of a problem that you'd have to reach into there.

[andylintner]

Haha - it was such a good idea, I had it twice!

Yes, maybe it's just a matter of moving things out of the impl package.

[timcharper]

@beowulfe my thought is, lets get another release with the protocol fixes, streamlining, optimizations etc from @ccutrer; once that is done, we (I?) can send a PR to openhab2 to bump the dependency, and then I can start sending my PRs up and integrating.

Then, I'd say, perhaps we could begin work on 1.2.x of the plugin, and start to tackle the optional characteristics head-on. I find it rather convenient to express the required attributes via the superclasses; I think this is useful. But, yes, we could add constructors for all characteristics such that they can take lambdas (in addition to the trait, for backwards compatibility), and then push the onus towards the implementor to make services that include the required characteristics and desired optional ones. Then, we can deprecate all of the accessory-specific base classes in favor of this generic one. This is probably a simpler solution, and it's the approach taken by the javascript hap implementation.

[ccutrer]

The more I look at this, the more I'm leaning towards getting rid of all the helper classes, and letting/forcing the implementor to construct accessories out of whatever services+characteristics they want. I say this as I look into my mini-split air conditioner. It is able to control fan speed, and vane swing. HomeKit looks like it should support those things, but not as part of the Thermostat service. I think you're supposed to implement the fanv2 service as an additional service with the thermostat, in a single accessory. And the same for slats (vanes) - which in fact a required attribute is if it's horizontal or vertical (my mini-split only has vertical, but its API implies it's possible to have both. so you'd provide two copies of the service in such an occasion). I'm going to play with this some more when I can to see how the Home app reacts to such accessories. Long story short - all this composition seems to me that the "here's the most common structure of an accessory" helper classes are more of a hindrance than a help once you get past the basics.

[timcharper]

@ccutrer there are three possible states for characteristics with relation to an accessory:

• Required: Homekit considers it an error to not include this accessory
• Optional: Homekit it fine to exclude it, but will support and augment the UI if it is included
• Not allowed: Homekit will ignore the addition of the characteristic. Including it is erroneous.

I'm rather of the opinion that the type system can be helpful in preventing mistakes such as associating a battery level characteristic with a battery powered accessory, when actually the supported combination is a battery status characteristic.

[sobeos]

Oh, I didn't saw this discussion. Anyway, why did you stop discussing? What is the result?

[ccutrer]

Because life happened. Both personally, and work. ccutrer@fadd8c1 is a massive refactor I started, but haven't gotten any feedback in. The basic idea is to stop having go-between classes for each accessory type, that need to be defined for every permutation, and instead expose services and characteristics directly, allowing the implementer to build up the complete accessory exactly as they want. This is more similar to how the javascript HAP library does it.

[sobeos]

No offense! I fully understand, but in my oppionion it is a little bit sad, because the library is very good. And a lot of great work was done in the past. But many optional characteristics are missing.
So the refactoring is really basic and important. I fear else there will live many forks with many different directions.
At least we should decide the new architecture. I can help implementing. Others perhaps as well?
I would stay as near to the apple specification and names as possible.
The approach sounds fine for me.
What is the current project developer setup?
Who has write rights? Who has to aggree on the refactoring? Who is validating pull requests? Who is answering issues?
What can the next steps look like?
Sorry for pushing, but I want to finsih my tv integration. Therefore I need at least a common pattern for optional characteristics.

[andylintner]

I really like what you've done here @ccutrer - https://github.com/ccutrer/HAP-Java/blob/fadd8c16deac33507671612a70e6077c73c0d48f/src/main/java/io/github/hapjava/services/CarbonMonoxideSensorService.java#L18

It matches the HAP spec nicely, while still keeping things strongly typed. If we're voting, I vote in favor of that refactor.

[sobeos]

Seems to work as well.
@ccutrer where do you see the advantages to use default methods?
@timcharper, @yfre what do you think?

[ccutrer]

I'm not sure I understand the question. It's both been a while since I banged on the refactor, and I'm not a Java expert, so am definitely prone to possibly using the wrong tool for the job (or not understanding the full ramifications of any particular tool).

[yfre]

Seems to work as well.
@ccutrer where do you see the advantages to use default methods?
@timcharper, @yfre what do you think?
@sobeos
i agree. @ccutrer approach looks like a good fit to HAP spec.
i vote for refactoring and happy to support it. got some free time.

[yfre]

so, i have finish the refactoring started by @ccutrer
please take a look on PR #101
@timcharper @sobeos

it is major refactoring and has breaking changes.
it will allow us to have optional characteristics and in general it fits much better to HAP spec structure and naming.

i have update also the sample/example #102

As next i will upgrade OH homekit addon to this HAP version

[sobeos]

Hello Eugen, I tried to push the project beginning of the year. There was not too much feedback. Looks like the project is dead. Unfortunately. I have taken another path. I wish you the best. Maybe you are more successful. Am Mo., 16. März 2020 um 14:03 Uhr schrieb eugen <notifications@github.com>:

…

so, i have finish the refactoring started by @ccutrer <https://github.com/ccutrer> please take a look on PR #101 <#101> @timcharper <https://github.com/timcharper> @sobeos <https://github.com/sobeos> it is major refactoring and has breaking changes. it will allow us to have optional characteristics and in general it fits much better to HAP spec structure and naming. i have update also the sample/example #102 <#102> As next i will upgrade OH homekit addon to this HAP version — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#63 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWLX6QD3SF7OU66MIMJR5DRHYPT5ANCNFSM4GUDATTQ> .

[ccutrer]

DONE