search

hap-java / HAP-Java

Java implementation of the HomeKit Accessory Protocol
MIT License
153 stars 82 forks source link

Added support for battery levels. #14

Closed gdombiak closed 8 years ago

gdombiak commented 8 years ago

Hi Andy,

Added battery level characteristic. Out of the box, contact sensors, motion sensors and locks can now report battery levels if implementations implement battery level characteristic.

Tested this and iOS app now reports battery usage for those 3 things. :)

Once again, I'm impressed by how easy is to add things to your library. A sign of such a great job. :)

Let me know if you do not agree with the approach of these changes.

Thanks, Gaston

andylintner commented 8 years ago

Thanks @gdombiak! I haven't checked in the simulator, but is it the case that the battery level characteristic can be added to any service? If so, maybe we can do the check in AbstractServiceImpl.getCharacteristics(), rather than repeating it in each service? (The constructor would need to be changed to receive the Accessory)

gdombiak commented 8 years ago

Hi Andy,

That's a very good idea. After work, I will apply your suggestion and generalize this improvement so that it can work with any accessory that implements that interface.

Thanks, Gaston

gdombiak commented 8 years ago

Andy,

I noticed that WindowCoveringService is the only service that does not add a Name characteristic. Is this on purpose or something that was missed? If it was missed, I can move adding the Name characteristic to AbstractServiceImpl.

Thanks, Gaston

gdombiak commented 8 years ago

Since this is a library and it seems like we are coding a minor new version, I do not want to break backwards compatibility. This is why I'm adding a new constructor to AbstractServiceImpl that will take the type and the HomekitAccessory. The existing constructor will pass null as an accessory to the new constructor.

Let me know if you are ok with this direction. Thanks, Gaston

gdombiak commented 8 years ago

Done with changes. I assumed that Name was missing from WindowCoveringService and that you were ok with deprecating old constructor to maintain backwards compatibility.

Let me know if you think otherwise. Thanks, Gaston

andylintner commented 8 years ago

Thanks @gdombiak - looks good! My general feeling is people shouldn't be relying on the .impl packaged classes. If there was a change that made sense, I wouldn't let compatability in those packages stand in the way. However, you made the right call here - it was easy enough to maintain compatibility.