Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

Add sensor support #52

Merged
merged 6 commits into from
Oct 26, 2016
Merged

Add sensor support #52

merged 6 commits into from
Oct 26, 2016

Conversation

rcloran
Copy link
Contributor

@rcloran rcloran commented Oct 12, 2016

No description provided.

@mention-bot
Copy link

@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.

}
return value
},
onEvent: function(old_state, new_state) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to trigger this to test it.

@@ -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 == 'sensor'){
if (entity.attributes && (entity.attributes.unit_of_measurement == '\u00B0C' || entity.attributes.unit_of_measurement == '\u00B0F') && entity.entity_id != 'sensor.vision_zp3111_multisensor_4in1_dew_point_5'){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove && entity.entity_id != 'sensor.vision_zp3111_multisensor_4in1_dew_point_5'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Derp.

@@ -68,7 +69,7 @@ adding it to your `config.json`.
"name": "HomeAssistant",
"host": "http://192.168.1.16:8123",
"password": "yourapipassword",
"supported_types": ["fan", "garage_door", "input_boolean", "light", "lock", "media_player", "rollershutter", "scene", "switch"]
"supported_types": ["climate", "fan", "garage_door", "input_boolean", "light", "lock", "media_player", "rollershutter", "scene", "sensor", "switch"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are both climate and sensor added?

@@ -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 == 'sensor'){
if (entity.attributes && (entity.attributes.unit_of_measurement == '\u00B0C' || entity.attributes.unit_of_measurement == '\u00B0F') && entity.entity_id != 'sensor.vision_zp3111_multisensor_4in1_dew_point_5'){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you not put the actual degrees symbol in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can. I wasn't sure what the source encoding for JS/node was, but it seems like UTF-8 works.

accessory = new HomeAssistantTemperature(that.log, entity, that);
}
}else if (entity_type == 'climate'){
accessory = new HomeAssistantTemperature(that.log, entity, that);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would rather leave the climate stuff out so that it can be implemented as a real thermostat service instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I didn't realise there was a thermostat service.

@robbiet480
Copy link
Member

Also i'm not sure if this is any different from #46?

 - Use UTF-8 encoded degree symbol instead of escape
 - Remove climate support
 - Remove testing stuff
@rcloran
Copy link
Contributor Author

rcloran commented Oct 12, 2016

Hah, yes, I didn't spot that. Basically the same, just a bit more complete here.

@rcloran
Copy link
Contributor Author

rcloran commented Oct 12, 2016

I see the discussion in #46 suggested a more generic sensor implementation. I'm not sure how that would look, but I'm happy to give any design suggestions you have a go.

@rcloran
Copy link
Contributor Author

rcloran commented Oct 12, 2016

#53 adds some binary sensor support in a generic kind of way. If you'd like me to refactor this to use something like that, let me know.

@robbiet480
Copy link
Member

@rcloran As for this one, I think that this is a great start for a sensor platform, shouldn't be any need to make this more generic.

@rcloran rcloran changed the title Add support for thermometers Add sensor support Oct 25, 2016
@robbiet480
Copy link
Member

@maddox Can you give this a final look through? It looks good to me.

@maddox
Copy link
Collaborator

maddox commented Oct 26, 2016

🚢

@robbiet480 robbiet480 merged commit 1adb179 into home-assistant:master Oct 26, 2016
robbiet480 pushed a commit that referenced this pull request Oct 26, 2016
* 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
This was referenced Oct 26, 2016
robbiet480 pushed a commit that referenced this pull request Oct 29, 2016
* 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants