Add Sensibo climate platform by andrey-git · Pull Request #7379 · home-assistant/core

andrey-git · 2017-04-30T15:27:24Z

Description:

Add Sensibo a/c remote as a climate platform.

To make it work correctly also added a new target_temp_step attribute to climate (defaults to None) so that UI can know the temperature granularity the platform supports.
home-assistant/frontend#270 is the UI counterpart.

Documentation is in progress...

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2546

Example entry for `configuration.yaml` (if applicable):

climate:
  - platform: sensibo
  - api_key: deadbeafdeadbeaf
  - id: unitid

Checklist:

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
New dependencies are only imported inside functions that use them ([example][ex-import]).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

mention-bot · 2017-04-30T15:27:26Z

@andrey-git, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabaff and @turbokongen to be potential reviewers.

balloob · 2017-04-30T18:41:16Z

+    devices = []
+    try:
+        for dev in (
+                yield from client.async_get_devices(_INITIAL_FETCH_FIELDS)):


Please wrap this in async_timeout.timeout to make sure that we don't wait forever.

There is a default timeout on operations of 5 minutes: https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client.py#L37

I'll pass in lower timeout.

Would it be cleaner if the client session additionally had a default read and connect timeouts? https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client.py#L55

There are many things that can cause a request to take forever. If you look at the intro of the docs, they suggest to use async_timeout.timeout: http://aiohttp.readthedocs.io/en/stable/

balloob · 2017-04-30T18:43:44Z

+    def max_temp(self):
+        """Return the maximum temperature."""
+        return self._current_capabilities['temperatures'][
+            'C' if self.unit_of_measurement == TEMP_CELSIUS else 'F'][


You should not have to do these conversions yourself. If you return the unit that the device uses as temperature_unit, HASS will take care of converting it correctly.

(this comment applies to all other conversions too)

I think the device uses C internally, but the API uses the unit selected in the app. That's the unit I return in temperature_unit.

The device API also returns min/max temperature range in both C and F.
I'm picking an appropriate JSON field here, not converting anything.

The only place I'm converting units is current_temperature because that field is always in C unlike target_temperature which is according to temperature_unit

Since both fields are available, I would suggest you just always pick C and let Home Assistant do the conversion if necessary.

Actually the JSON looks like this

"C": {"isNative": True, "values": [16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30]}, "F": {"isNative": False, "values": [61, 63, 64, 66, 68, 70, 72, 73, 75, 77, 79, 81, 82, 84, 86]}}

So not all whole-F values are accepted.
I'll add a fix to make sure correct F temperature is set.

As for min and max - it would be safest to stick to the provided list. What is the advantage of converting?

balloob · 2017-04-30T18:44:29Z

+        """Turn Sensibo unit on."""
+        yield from self._client.async_set_ac_state_property(
+            self._id, 'on', False)
+        yield from self.async_update_ha_state(True)


should_poll is not defined and so defaults to True, this means that Hass will call this for you after calling any service. You can remove all these statements.

Could you point me to the code where this happens?
I would like to get an immediate update, i.e not as part of 15s update cycle here: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/helpers/entity_component.py#L377

Have a look at the service handler: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/climate/__init__.py#L274-L287

balloob · 2017-04-30T18:44:41Z

+    def async_update(self):
+        """Retrieve latest state."""
+        try:
+            data = yield from self._client.async_get_device(


Please add a timeout.

Added to _client constructor

That should be using async_timeout.timeout.

balloob · 2017-04-30T18:46:20Z

+            data = yield from self._client.async_get_device(
+                self._id, _FETCH_FIELDS)
+            self._do_update(data)
+        except aiohttp.client_exceptions.ClientConnectorError:


what about all the other errors? Shouldn't you just listen for the base error ClientError

andrey-git added 3 commits April 30, 2017 18:02

Add Sensibo climate platform

393481b

Force update after running a service

205d1a9

Add sensibo to .coveragerc

136e484

andrey-git added integration: climate new-platform labels Apr 30, 2017

homeassistant added cla-signed integration: climate new-platform platform: climate.sensibo labels Apr 30, 2017

balloobbot added platform requirement and removed platform: climate.sensibo labels Apr 30, 2017

balloob requested changes Apr 30, 2017

View reviewed changes

Use 10s timeout

6c39817

homeassistant added the cla-signed label May 1, 2017

andrey-git added Ready for review For Paulus labels May 1, 2017

Fix schema. Remove print.

55ebbd8

homeassistant added the cla-signed label May 1, 2017

andrey-git mentioned this pull request May 1, 2017

Add documentation for Sensibo climate platform home-assistant/home-assistant.io#2546

Merged

balloob removed For Paulus Ready for review labels May 2, 2017

Better handle unit conversions.

6d6aa72

homeassistant added the cla-signed label May 2, 2017

andrey-git added For Paulus Ready for review labels May 2, 2017

balloob approved these changes May 3, 2017

View reviewed changes

balloob merged commit 11dc724 into home-assistant:dev May 3, 2017

andrey-git deleted the sensibo branch May 3, 2017 15:33

balloob mentioned this pull request May 5, 2017

0.44 #7444

Merged

home-assistant locked and limited conversation to collaborators Aug 12, 2017

Uh oh!

Conversation

andrey-git commented Apr 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

mention-bot commented Apr 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

andrey-git commented Apr 30, 2017 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):