Add binary_sensor support #53

rcloran · 2016-10-12T22:26:37Z

No description provided.

mention-bot · 2016-10-12T22:26:39Z

@rcloran, thanks for your PR! By analyzing the history of the files in this pull request, we identified @robbiet480, @maddox and @shmuelzon to be potential reviewers.

rcloran · 2016-10-12T22:40:38Z

This might not be enough to generally support more types of motion sensors, but I figure that those that need extra functionality (door is actually an example, and I should make it more complete) can extend the binary sensor

robbiet480 · 2016-10-12T22:28:08Z

accessories/binary_sensor.js

+  this.entity_id = data.entity_id
+  if (data.attributes && data.attributes.friendly_name) {
+    this.name = data.attributes.friendly_name
+  }else{


(nitpicking) } else { plz

robbiet480 · 2016-10-12T22:35:02Z

index.js

@@ -186,6 +188,12 @@ HomeAssistantPlatform.prototype = {
          accessory = new HomeAssistantSwitch(that.log, entity, that, 'input_boolean')
        }else if (entity_type == 'fan'){
          accessory = new HomeAssistantFan(that.log, entity, that)
+        }else if (entity_type == 'binary_sensor'){
+          if (entity.attributes && entity.attributes.sensor_class == 'opening') {
+            accessory = new HomeAssistantBinarySensor(that.log, entity, that, Service.Door, Characteristic.CurrentPosition, 100, 0)


Looks like for Service.Door these characteristics are required:

Characteristic.CurrentPosition

Characteristic.PositionState

Characteristic.TargetPosition

Also, it would be good to allow users to specify whether this is a door or window with a customize parameter (something like homebridge_opening_binary_sensor_type). If door, use Service.Door and if window use Service.Window. Otherwise you're going to get a lot of false positives for those that have sensors on things other than doors.

You could cheat though and not listen to me entirely and use the (what seems to be much simpler) Service.ContactSensor

Throwing out some other super easy to implement binary_sensors for extra credit:

moisture - Service.LeakSensor

occupancy - Service.OccupancySensor

smoke - Service.SmokeSensor

Oh also, opening could be used with Service.ContactSensor.

robbiet480 · 2016-10-12T22:42:04Z

@rcloran Instead of adding a bunch of }else if (entity.attributes && entity.attributes.sensor_class == 'motion') conditionals can we just check for the existence of the sensor_class attribute and then have a switch case inside the 1 conditional?

robbiet480 · 2016-10-12T22:43:32Z

@rcloran FYI I'm around on Gitter if you wanted to chat more about this

rcloran · 2016-10-13T00:11:44Z

I don't have any of the other sensors to test with, so I'll need to do a little research, but I'm happy to add them in here.

rcloran · 2016-10-13T00:13:18Z

README.md

@@ -27,6 +27,8 @@ Here's a list of the devices that are currently exposed:
 * **Rollershutter** - exposed as a garage door
 * **Fan** - on/off/speed
 * **Input boolean** - on/off
+* **Door** - open/close state exposed


Not sure how you'd like this updated.

Service.ContactSensor is convenient, but it is semantically wrong, so we get some bad UI - for example, Siri cannot answer "Is the Front Door open?"

rcloran · 2016-10-13T01:37:48Z

index.js

+                opening_type = Service.Window;
+              }else{
+                opening_type = Service.Door;
+              }


We could do some heuristics around here - if friendly name or entity id contains the string window, make it a window, etc. If it gets any fancier than this, I'd be inclined to put that in HomeAssistantOpening's constructor. What do you think?

~~I think since you're looking at the ha sensor and conditionally setting characteristics here already, this is probably the place to do this as well.~~

I thought this was actually in the sensor, heh.I think the runtime decisions of characteristics should probably be in binary_sensor.js.

rcloran · 2016-10-13T02:04:05Z

Do you think Service.ContactSensor and sensor_class connectivity match?

maddox · 2016-10-13T14:30:24Z

package.json

@@ -22,7 +22,7 @@
    "url": "http://github.com/home-assistant/homebridge-homeassistant/issues"
  },
  "engines": {
-    "node": ">=0.12.0",
+    "node": ">=6.0.0",


Try not to change the minimum required version of node in feature pull requests like this. We're going to bump it soon, but it will be on it's own pull request. There's nothing in this pull that really requires this anyways.

I agree that I don't need to bump the version to use the feature (class keyword) that I need to achieve the goal (inheritance). @robbiet480 said this would be OK, which is why I went ahead with it.

In a later commit, I reduced this to ">=4.3.2", which is the version that introduced the class keyword. It's also the minimum version that's required by the homebridge package, so it doesn't actually represent anything different in what users will need to have set up.

I'm at node -v 4.3.7 but getting errors when running your modifications... Probably something simple I've overlooked and any help appreciated:

SyntaxError: Block-scoped declarations (let, const, function, class) not yet supported outside strict mod

rcloran · 2016-10-15T21:22:06Z

accessories/binary_sensor.js

+        this.offValue = Characteristic.SmokeDetected.SMOKE_NOT_DETECTED
+        break
+      default:
+        return null


This is no good. null from the constructor is ignored.

robbiet480 · 2016-10-26T02:02:39Z

@maddox Can you also look at this one too. Thx.

maddox · 2016-10-26T04:03:02Z

🚢

* Add support for thermometers * Handle onEvent * Address review comments - Use UTF-8 encoded degree symbol instead of escape - Remove climate support - Remove testing stuff * Change to a more generic sensor class * Update README * jshint suggested changes

…stant into rcloran-door

* Add uuid_base to all accessories Currently, homebridge uses the accessory type and display name to generate the UUID. That means that if two devices have the same display name in the house, we will crash on startup. It also means that renaming a device will have all of its HomeKit configuration (like which room it is in) lost. This change will break many peoples' setups - devices will be put in the default room, and they will need to re-organize them. * Add sensor support (#52) * Add support for thermometers * Handle onEvent * Address review comments - Use UTF-8 encoded degree symbol instead of escape - Remove climate support - Remove testing stuff * Change to a more generic sensor class * Update README * jshint suggested changes * Add binary_sensor support (#53) * Add binary_sensor support with doors and motion sensors * Change from Service.Door to Service.ContactSensor * Formatting changes * Move back to Service.Door, add Service.Window Service.ContactSensor is convenient, but it is semantically wrong, so we get some bad UI - for example, Siri cannot answer "Is the Front Door open?" * Add some more trivial binary sensors * Correct node requirement * Move specializations into constructor * Fix null return from constructor by making it a factory function * jshint suggested changes * Add sensor support (#52) * Add support for thermometers * Handle onEvent * Address review comments - Use UTF-8 encoded degree symbol instead of escape - Remove climate support - Remove testing stuff * Change to a more generic sensor class * Update README * jshint suggested changes * Update binary_sensor and sensor to use uuid_base

robbiet480 · 2016-10-30T22:26:01Z

@rcloran Going back for a minute now that I actually have tried this out...

You said "Service.ContactSensor is convenient, but it is semantically wrong, so we get
some bad UI - for example, Siri cannot answer "Is the Front Door open?"

I agree that that's annoying but at the same time I don't know if presenting binary_sensors as supporting state changes from HomeKit is a good idea either. I'm surprised you can't ask Siri if a contact sensor is open/closed since it is HomeKit defined...

rcloran · 2016-10-30T23:23:46Z

Hrm - there may have been something else wrong while I was trying it out, I
guess. Unfortunately I'm traveling this week, so won't be able to test any
actual interactions until next Sunday.
On Sun, Oct 30, 2016 at 15:26, Robbie Trencheny notifications@github.com
wrote:

@rcloran https://github.com/rcloran Going back for a minute now that I
actually have tried this out...

You said "Service.ContactSensor is convenient, but it is semantically
wrong, so we get
some bad UI - for example, Siri cannot answer "Is the Front Door open?"

I agree that that's annoying but at the same time I don't know if
presenting binary_sensors as supporting state changes from HomeKit is a
good idea either. I'm surprised you can't ask Siri if a contact sensor is
open/closed since it is HomeKit defined...

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#53 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA-P5S0XZr_M6O4oQHyr6A73JvEppV1tks5q5Rl6gaJpZM4KVSZ_
.

robbiet480 · 2016-10-30T23:58:40Z

@rcloran I changed it to be ContactSensor and Siri continued to work so I pushed the change to master.

Add binary_sensor support with doors and motion sensors

7d00b58

robbiet480 added the Hacktoberfest label Oct 12, 2016

rcloran mentioned this pull request Oct 12, 2016

Add sensor support #52

Merged

robbiet480 suggested changes Oct 12, 2016

View reviewed changes

Change from Service.Door to Service.ContactSensor

e3b4377

rcloran commented Oct 13, 2016

View reviewed changes

rcloran added 2 commits October 12, 2016 17:13

Formatting changes

7a3b9b3

Move back to Service.Door, add Service.Window

1efaab1

Service.ContactSensor is convenient, but it is semantically wrong, so we get some bad UI - for example, Siri cannot answer "Is the Front Door open?"

rcloran commented Oct 13, 2016

View reviewed changes

Add some more trivial binary sensors

3eb3d64

rcloran changed the title ~~Add binary_sensor support with doors and motion sensors~~ Add binary_sensor support Oct 13, 2016

rcloran mentioned this pull request Oct 13, 2016

Add uuid_base to all accessories #54

Merged

maddox reviewed Oct 13, 2016

View reviewed changes

rcloran added 2 commits October 13, 2016 17:17

Correct node requirement

2d57b40

Move specializations into constructor

ec21190

rcloran commented Oct 15, 2016

View reviewed changes

rcloran added 2 commits October 24, 2016 21:30

Fix null return from constructor by making it a factory function

e76171c

jshint suggested changes

398bf0b

robbiet480 approved these changes Oct 26, 2016

View reviewed changes

rcloran and others added 2 commits October 26, 2016 12:09

Add sensor support (#52)

1adb179

* Add support for thermometers * Handle onEvent * Address review comments - Use UTF-8 encoded degree symbol instead of escape - Remove climate support - Remove testing stuff * Change to a more generic sensor class * Update README * jshint suggested changes

Merge branch 'door' of https://github.com/rcloran/homebridge-homeassi…

a7bf9ea

…stant into rcloran-door

robbiet480 merged commit d445afa into home-assistant:master Oct 26, 2016

robbiet480 mentioned this pull request Oct 26, 2016

Add binary_sensor support #48

Closed

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add binary_sensor support #53

Add binary_sensor support #53

rcloran commented Oct 12, 2016

mention-bot commented Oct 12, 2016

rcloran commented Oct 12, 2016

robbiet480 Oct 12, 2016

robbiet480 Oct 12, 2016

robbiet480 Oct 12, 2016

robbiet480 Oct 12, 2016

robbiet480 Oct 12, 2016 •

edited

robbiet480 commented Oct 12, 2016

robbiet480 commented Oct 12, 2016

rcloran commented Oct 13, 2016

rcloran Oct 13, 2016

rcloran Oct 13, 2016 •

edited

maddox Oct 13, 2016 •

edited

maddox Oct 13, 2016

rcloran commented Oct 13, 2016

maddox Oct 13, 2016

rcloran Oct 15, 2016

rcloran Oct 15, 2016

EndianX Oct 20, 2016

rcloran Oct 15, 2016

robbiet480 commented Oct 26, 2016

maddox commented Oct 26, 2016

robbiet480 commented Oct 30, 2016

rcloran commented Oct 30, 2016

robbiet480 commented Oct 30, 2016

Add binary_sensor support #53

Add binary_sensor support #53

Conversation

rcloran commented Oct 12, 2016

mention-bot commented Oct 12, 2016

rcloran commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbiet480 Oct 12, 2016 • edited

Choose a reason for hiding this comment

robbiet480 commented Oct 12, 2016

robbiet480 commented Oct 12, 2016

rcloran commented Oct 13, 2016

Choose a reason for hiding this comment

rcloran Oct 13, 2016 • edited

Choose a reason for hiding this comment

maddox Oct 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcloran commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbiet480 commented Oct 26, 2016

maddox commented Oct 26, 2016

robbiet480 commented Oct 30, 2016

rcloran commented Oct 30, 2016

robbiet480 commented Oct 30, 2016

robbiet480 Oct 12, 2016 •

edited

rcloran Oct 13, 2016 •

edited

maddox Oct 13, 2016 •

edited