Repository navigation
Better mechanism for optional characteristics needed #63
Description
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 {
}
Activity
timcharper commentedon Feb 11, 2019
@beowulfe any thoughts on this? @ccutrer WDYT?
ccutrer commentedon Feb 11, 2019
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 commentedon Feb 12, 2019
@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 commentedon Feb 12, 2019
Oh hey look, you started too: andylintner/openhab2@8ede0bc
ccutrer commentedon Feb 14, 2019
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 commentedon Feb 14, 2019
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 commentedon Feb 14, 2019
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 commentedon Feb 14, 2019
+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 commentedon Feb 14, 2019
@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 commentedon Feb 15, 2019
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 commentedon Feb 15, 2019
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
implpackage, and that might be a bit of a problem that you'd have to reach into there.andylintner commentedon Feb 15, 2019
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 commentedon Feb 18, 2019
@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 commentedon Feb 26, 2019
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 commentedon Feb 27, 2019
@ccutrer there are three possible states for characteristics with relation to an accessory:
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 commentedon Jan 7, 2020
Oh, I didn't saw this discussion. Anyway, why did you stop discussing? What is the result?
ccutrer commentedon Jan 7, 2020
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 commentedon Jan 9, 2020
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 commentedon Jan 9, 2020
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 commentedon Jan 13, 2020
Seems to work as well.
@ccutrer where do you see the advantages to use default methods?
@timcharper, @yfre what do you think?
ccutrer commentedon Jan 13, 2020
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 commentedon Mar 10, 2020
yfre commentedon Mar 16, 2020
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 commentedon Mar 16, 2020
ccutrer commentedon Jun 23, 2020
DONE