-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[homekit] Bugfixes, improvements and support for more accessories #5082
Conversation
Signed-off-by: Tim Harper <timcharper at gmail.com>
Also, improve logging
… characteristics at once
It causes the plugin to fail spectacularly; further investigation is required
For more information on the issue, see hap-java/HAP-Java#54
Updates trickle in one at a time; this means that if a complex accessory composed of 5 required items is added, then we get 5 individual add calls, with error messages reporting that the accessory is incomplete until the 5th comes in. By debouncing, we process all the additions in a single batch, sparing the user to sort through the noise.
Some Z-wave configurations expose them as switches, other as contact sensors.
* Homekit allows the targetMode to be specified, and the currentMode to be reported. * Improve configuration; add groupings for targetHeatingCoolingMode and currentHeatingCoolingMode value maps * Update docs * homekit:heatingCoolingMode is renamed to homekit:targetHeatingCoolingMode, for clarity
changelog etc
Signed-off-by: "Tim Harper" [email protected] |
Signed-off-by: "Cody Cutrer" [email protected] |
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution.
I have noticed that your new classes have no javadoc.
For impl
classes it is sufficient to have:
/**
* {@inheritDoc}
*/
but for new interfaces and other types of classes a javadoc is required. I have also left some inline comments.
@@ -36,79 +33,40 @@ | |||
class HomekitAccessoryRegistry { | |||
|
|||
private HomekitRoot bridge; | |||
private final List<HomekitAccessory> createdAccessories = new LinkedList<>(); | |||
private final HashMap<String, HomekitAccessory> createdAccessories = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the generic type as field type
return new NoBatteryStatus(); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check for missing newlines
@@ -46,93 +50,93 @@ | |||
* | |||
* @author Andy Lintner - Initial contribution | |||
*/ | |||
class HomekitThermostatImpl extends AbstractTemperatureHomekitAccessoryImpl<GroupItem> | |||
implements BasicThermostat, GroupedAccessory { | |||
class HomekitThermostatImpl extends AbstractTemperatureHomekitAccessoryImpl<GroupItem> implements BasicThermostat { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please annotate all (new) Impl
classes with @NonNullByDefault
?
That helps a lot to prevent NPEs to happen and fellow developers immediately know about a potential null access.
/** | ||
* Call after itemRegistry and settings are specified to initialize homekit devices | ||
*/ | ||
private void maybeInitialize() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for this class not being a @Component
?
The OSGi framework would resolve your requirements (ItemRegistry + HomekitSettings)
and you can use the activated
method to do do what you currently do in maybeInitialize
.
HomekitSettings
would be a component in that scenario as well, which it probably isn't I guess?
@@ -2,7 +2,7 @@ Manifest-Version: 1.0 | |||
Automatic-Module-Name: org.openhab.io.homekit | |||
Bundle-ClassPath: | |||
., | |||
lib/com.beowulfe.hap.hap-1.1.4.jar, | |||
lib/hap-1.1.5.jar, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you had a look at the new buildsystem?
The issue is, if I merge this PR, we add another 500kb of binary data to the repo (which is currently at 400 MB already) and with the buildsystem migration that need to happen anyway, we would render that binary data unused (git doesn't forget).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidgraeff I'll have a look; I'd much prefer to not commit this artifact to the repository. Anything you can point me to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this upgrade needed to build? Otherwise we can upgrade the library easily with the migration PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the only thing missing here, we should commit it. We have to do something about al those binaries in the source tree anyway, 500kB more or less make no difference IMO.
@timcharper Are you still working on this? |
@timcharper @ccutrer since this is one of the last two blocking PR for the bnd migration, how do you want to proceed? |
I can imagine that we perform the migration on master and additionally also on this branch. That way the guys have no additional conflicts and we can continue with the migration. |
@davidgraeff we do not use {@inheritdoc}, so rather leave it empty and add an @OverRide that should be enough 👍 |
@davidgraeff Apart from the necessary rebase, what is missing here? Do you consider the |
There is nothing blocking. There was one file in conflict that need to be resolved by the OP. And of course at the moment we need to apply the bnd migration patch to this PR as well. |
@timcharper @ccutrer Would you mind me taking this over to get this merged? Is there any chance to get hap/1.1.5 pushed to bintray? |
@J-N-K : yes, I'm fine with you pushing this through. Unfortunately @timcharper has had an arm injury, so his computer use has been severely limited. I personally don't have permissions or knowledge on how to push hap-1.1.5 to binary. Java isn't my forte, so I'm still learning the community and the tools. |
Anyone else: I put a test-version in #5581, try it if you want. Thanks. |
superseded by #5581 |
This is a monster PR which includes work from both myself (@timcharper) and @ccutrer. Over the past couple of months we have been working to improve the HAP-Java plugin and simultaneously improving the openhab2 plugin. With the release of HAP-Java 1.1.5 we are now ready to integrate this work upstream.
A detailed changelog can be found below:
2.5.0-SNAPSHOT
Fixes
HAP-Java fixes
Several fixes HAP-Java from Cody Cutrer have been included in the release:
Thermostat fixes
Previously, the mapping of target and current thermostat mode was broken. It is considered illegal to return current mode of "AUTO", and resulted in an error. The issue has been fixed in my fork of homekit (see hap-java/HAP-Java#60)
Support for an item indicating the thermostat current mode has been added.
The homekit plugin configuration screen for paper-ui has been improved, adding groupings for each set of mappings.
The tag
homekit:HeatingCoolingMode
has been changed tohomekit:TargetHeatingCoolingMode
. The old tag is supported still, but you should update your thermostat tagged item, respectively.Fix a characteristic subscription leak when the homekit addon is reloaded
Characteristic subscriptions would accumulate with each reload, causing noisy logs and a (not totally severe) memory leak. HAP-Java 1.1.5 has this issue patched. See hap-java/HAP-Java#54 for more information.
Breaking changes
Thermostat config options rename
The following config options have been renamed:
org.openhab.homekit:thermostatCoolMode
toorg.openhab.homekit:thermostatTargetModeCool
org.openhab.homekit:thermostatHeatMode
toorg.openhab.homekit:thermostatTargetModeHeat
org.openhab.homekit:thermostatAutoMode
toorg.openhab.homekit:thermostatTargetModeAuto
org.openhab.homekit:thermostatOffMode
toorg.openhab.homekit:thermostatTargetModeOff
Further, the following required config options have been specified:
org.openhab.homekit:thermostatCurrenModeCooling
org.openhab.homekit:thermostatCurrenModeHeating
org.openhab.homekit:thermostatCurrenModeOff
You will need to update your homekit configuration accordingly, either by editing your homekit config file, or by editing the configuration for the IO service using the paper UI.
New features
Added accessories
The following accessories are now supported:
NOTE: If you add motion sensors or leak sensors and they show they are unreachable, try toggling their value. For Z-wave devices the value is null until OpenHab2 receives the first value. In future release of the plugin the thing offline state will be used, and null item value will be interpreted as off.
See the README for instructions on configuring.
Allowing motion and leak accessories to be backed by Contact items
Some Z-wave devices configure these things to expose their channels as an OpenClosedType, rather than OnOffType, due to the fact that Contact items are read-only. We allow these boolean-like status accessories to be backed by either a Switch or a Contact item.
For Motion sensors, Open is considered "motion detected" (think "window open" as the actionable event). Similarly, leaks are reported if the backing Item is Open.
Debounce the refreshing of Homekit items
The plugin now waits for items to be stable for a full second before creating, removing, and deleting the associated homekit accessories. This greatly reduces the chaos, overhead, and misleading log messages when adding, removing, or changing multiple items at a time.
Contact and Occupancy Sensors support
Support is added for contact and occupancy sensors.
WindowCovering support
Merge in Epike's WindowCovering patch. The more-generic, Apple Homekit friendly label WindowCovering is used rather than Blinds. Users upgrading from Epike's patch should update their labels (Don't include both labels simultaneously, as
Blinds
is still supported, while deprecated).A fix for WindowCovering support was merged in to HAP-Java 1.1.5. WindowCovering support is now enabled.
Support for optional battery levels with leak and motion sensors
With the refactoring, devices such as leak sensors and motion sensors can now optionally report when their battery is low. Battery level can be reported as a percentage (0-100, not 0.00 - 1.00), or as a switch (where ON == Low battery). More information on how this is configured can be seen in the README.
Internal improvements
Improved strategy for dealing with groups of items as a composite thing
The approach for dealing with grouped items has been refactored extensively, paving the way for devices to optionally include extra characteristics more easily.
Since item added/removed updates from OpenHab come in as a stream of additions and removals, one at a time, debouncing should be implemented in order to suppress the error messages of "device is lacking required characteristic" while OpenHab is in the process of reconfiguring some list of items. This will be implemented soon. For now, the functionality works, but the logs are a bit noisy.