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

Support for Temperature sensor #46

Closed
wants to merge 2 commits into from
Closed

Support for Temperature sensor #46

wants to merge 2 commits into from

Conversation

2ndalpha
Copy link

@2ndalpha 2ndalpha commented Oct 2, 2016

Comments and suggestions for improvement are more than welcome.
Currently it supports only celsius as a unit.

@robbiet480
Copy link
Member

robbiet480 commented Oct 2, 2016

At a minimum Fahrenheit also needs to be supported. There's a lot of simple conversion functions out there. It is also possible that you can set the unit (see here).

In addition, previously we (@maddox and myself, primary maintainers) have been of the opinion that homebridge-homeassistant should not support sensors since sensors will enable automation support through HomeKit which can lead to issues where you have duplicate automations running or are unable to determine what is causing actions to happen. For more info see @maddox's comments here. We have had quite a few requests recently to add sensor support though, so maybe it's time. If we did add support I may close this PR and ask you/someone to create a new one with generic sensor support. Also keep in mind that with sensor support binary_sensor support can come somewhat easily as well.

@maddox
Copy link
Collaborator

maddox commented Oct 3, 2016

have been of the opinion that homebridge-homeassistant should not support sensors since sensors will enable automation support through HomeKit which can lead to issues where you have duplicate automations running

Just to clarify my opinion: I wasn't just worried that sensors would enable automation, I just didn't think they had any business being in the homebridge plugin. I was of the opinion that you should NOT be DOING automations with HomeKit, you should be doing them in HA, which is the right way, if you have an HA set up. I don't really care if you screw things up, lol. I just didn't think they had any business in there because you shouldn't even CARE about automations in HomeKit. This bridge was created to be a UI proxy, and that's it.

That being said, @robbiet480 is right, fuck it. It's time. Let's just support sensors. Especially if NOT having sensors prevents accessories with functionality from being bridged over.

So just to be official: I'm for sensors, let's do it, because it's time and also the argument in this PR is super sound. 💥

@fastender
Copy link

wonderful news

@2ndalpha
Copy link
Author

2ndalpha commented Oct 3, 2016

I've added support for Fahrenheit.
Setting unit_system: imperial in Home Assistant conf is sufficient, it makes the conversion from C to F.

Unfortunately I wasn't able to get homekit to recognise that it's in fahrenheit.
tried c.setProps({ unit: Characteristic.Units.FAHRENHEIT }); which @robbiet480 linked to. Also original author of that code does not use it in his repo.
I tested it via Siri "what's the temperature inside", she always replies in celsius :(

Also to add to the sensor vs no sensors discussion, I just would like to see the sensor data in my Home app in iOS. Seeing all the data on the go gives me and others in the family peace of mind.

If anyone is wondering what sensor I'm using, it's Multisensor 6.

@robbiet480 robbiet480 mentioned this pull request Oct 12, 2016
@rcloran
Copy link
Contributor

rcloran commented Oct 12, 2016

HomeKit always uses Celsius internally, so whatever is returned by HomeAssistant needs to be converted to Celsius.

I opened #52 earlier without seeing that this PR existed. My phone always talks to me in °F (even though I don't want it to), and returning °C does all the right conversions for me. The logic I have there should work.

@robbiet480
Copy link
Member

Closing this in favor of #52.

@robbiet480 robbiet480 closed this Oct 12, 2016
@2ndalpha
Copy link
Author

@robbiet480 really odd way of doing things but I don't judge.
I was planning to add support for other Multisensor 6 sensors but I'ts probably waste of time

@robbiet480
Copy link
Member

@2ndalpha Please feel free to create new PRs for the other Multisensor 6 sensors. I'd rather have them all as individual PRs instead of one big one anyway. No hard feelings!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants