Daikin Climate - Better integration with Climate base component by MatteGary · Pull Request #16913 · home-assistant/core

MatteGary · 2018-09-27T08:51:51Z

Description:

I made some modification do the Daikin Climate Component in order to better integrate the Daikin AC interface with the base Climate Component.
Improved functionality are:

Support for localization for Operation Mode
Support for Homekit Integration (if the AC is turned On, now the status is updated in Homekit)

Breaking Changes

In order to support localization for Operation Mode, now the available options for this field are changed according to:

old version / new version (english only)

Fan | Fan Only
Dry | Dry
Cool | Cool
Hot | Heat
Auto | Auto
Off | Off

For other languages, the new version is used according to the translated version.

Made some modification in order to better integrate the Daikin AC Component with the base Climate Component. Benefits are: Support localization for Operation Mode Support for Homekit Integration (if the AC is turned On, now the status is updated in Homekit)

homeassistant · 2018-09-27T08:51:52Z

Hi @MatteGary,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

houndci-bot · 2018-09-27T08:51:59Z

homeassistant/components/climate/daikin.py

        """Retrieve latest state."""
        self._api.update(no_throttle=self._force_refresh)
-        self._force_refresh = False
+        self._force_refresh = False


no newline at end of file

houndci-bot · 2018-09-27T08:52:00Z

homeassistant/components/climate/daikin.py

+            if daikin_op_mode is not None:
+                value = DAIKIN_TO_HA_STATE.get(daikin_op_mode)
+            else:
+                value = tmp_value 


trailing whitespace

houndci-bot · 2018-09-27T08:52:00Z

homeassistant/components/climate/daikin.py

                self._api.device.represent(daikin_attr)[1]
-            ).title()
+            )
+


blank line contains whitespace

Fixed to .title() functionality in matching the Operation_Mode

MartinHjelmare · 2018-09-30T18:54:17Z

homeassistant/components/climate/daikin.py

+
+            daikin_op_mode = DAIKIN_TO_HA_STATE[tmp_value]
+            if daikin_op_mode is not None:
+                value = DAIKIN_TO_HA_STATE.get(daikin_op_mode)


How can this work? We already have the value from the dict in daikin_op_mode. We shouldn't need to get it again.

I think that I can make this part a little bit better, as soon as I can I will do that. The if statement is definetly too much. But the dictionary DAIKIN_TO_HA_STATE is necessary to map the operation_mode from the AC into the accepted one in HA climate component (there are some mismatch, like ‘heat’ vs ‘hot’). That’s way in the previous version the localization wasn’t working.

Yes, but you're using the same dictionary twice. That looks like a bug. We should only need to do one lookup.

I’m not an expert in Python (I work more with .NET tools), how can I use a key-value dictionary in “both direction”? (I mean, one time I’m using the key to get the value, and the next time I should use the value to get the key)

Dictionaries in Python are "one way" only. Keys are unique, but values don't have to be unique. It's possible to iterate over a dictionary, check for a value and then save the key(s) that match the value. What is the goal with this part of the code? If you explain that we can try to figure out the best way.

I’ll try to explain: in the original component’s code, there was no mapping between the operation mode formthe daikin AC unit and the one for the base HA climate component. The list of available operation mode and the actual operation mode was just the ones available for the Daikin AC with a starting Uppercase. This was ok for most of the functionality, but what wasn’t working was:

Localization: the operation mode was indeed something like ‘Off’, ‘Hot’, ‘Auto’, but it was just hardcoded from the component. It was ok for english user, but that didn’t match the lsbel in the climate translation file, which are ‘off’, ‘heat’, ‘auto’ (as you can see there is also a mismatch between Hot and Heat).

Homekit integration (and possibly other platform): this part wasn’t working at all. Since Homekit interact with the base HA climate component, and due to the mismatch I describe before, I wasn't able to use Homekit to get or set the status of the AC.

I managed to make it eork using a dictionary to translate Daikin Operation Mode to HA Operation Mode (DAIKIN_TO_HA_STATE) and the other way around (HA_STATE_TO_DAIKIN).

Since, as you point it out, the mapping is the same in the two direction, I can change the behavior removing the dictionary and using something like a Tuple (or at least that is what I would use in C#). Do you have any suggestion? Otherwhise tomorrow I’ll try to find sometime to fix this.
Tks

Dictionary is what we always use for translating modes between home assistant and device. You still didn't explain what you want to do with this part of the code. Do we want to end up with Daikin mode or home assistant mode, after the translation?

If it's home assistant mode, we should only use DAIKIN_TO_HA_STATE dictionary one time, not twice in a row.

This is how I would expect us to do it:

daikin_mode = re.sub( '[^a-z]', '', self._api.device.represent(daikin_attr)[1]) ha_mode = DAIKIN_TO_HA_STATE.get(daikin_mode) value = ha_mode

MartinHjelmare · 2018-09-30T18:55:09Z

homeassistant/components/climate/daikin.py

        self._list = {
            ATTR_OPERATION_MODE: list(
-                map(str.title, set(HA_STATE_TO_DAIKIN.values()))
+                map(str, set(DAIKIN_TO_HA_STATE.values()))


Why do we need the str copy?

Making it a set doesn't seem necessary either. We know what the values are and they are unique. The dictionary is a constant so it won't change unless we change it.

For this part I made just little modification to make it work properly, but the logics is the one from the original commit for the daikin component.

Since we're here, please clean it up.

Change this to:

self._list = { ATTR_OPERATION_MODE: list(HA_STATE_TO_DAIKIN), ... }

homeassistant/components/climate/daikin.py

houndci-bot · 2018-10-01T13:33:39Z

homeassistant/components/climate/daikin.py

-                if value.title() in self._list[attr]:
-                    values[daikin_attr] = value.lower()
+                if value in self._list[attr]:
+                    values[daikin_attr] = HA_STATE_TO_DAIKIN[value];


statement ends with a semicolon

houndci-bot · 2018-10-01T13:33:40Z

homeassistant/components/climate/daikin.py

-                self._api.device.represent(daikin_attr)[1]
-            ).title()
+            daikin_mode = re.sub(
+                '[^a-z]','',self._api.device.represent(daikin_attr)[1])


missing whitespace after ','

MartinHjelmare · 2018-10-01T13:45:23Z

homeassistant/components/climate/daikin.py

-            ATTR_OPERATION_MODE: list(
-                map(str.title, set(HA_STATE_TO_DAIKIN.values()))
-            ),
+            ATTR_OPERATION_MODE: list(DAIKIN_TO_HA_STATE),


This should be:

ATTR_OPERATION_MODE: list(HA_STATE_TO_DAIKIN),

We want to use the ha states so we use the keys of the HA_STATE_TO_DAIKIN dictionary.

Can you check my lasts commit? I’m quite sure that I should use the other dictionary in that point. Today I’m not able to test since I’m travelling.

We should use the same dictionary as we do here:
https://github.com/home-assistant/home-assistant/pull/16913/files#diff-f35f4028d7ed0027465e5dea53ef4acdR178

I made the modification you suggested, but I'm not able to test it right now. I'm hoping that @mynameisdaniel32 can test it sooner than I can 😃

I managed to run a quick test, and it seems to work good. As soon as I can I will run some further tests.

mynameisdaniel32 · 2018-10-02T03:37:31Z

Just did another test of this (latest version), most modes work fine, but I'm seeing problems with some.

When selecting 'hot' or 'fan' from the HA UI I get the task exceptions errors below (and nothing happens on the AC). When selecting 'heat' from HomeKit, the bottom (invalid value) error shows. All other operation modes seem to work fine.

2018-10-02 12:59:09 ERROR (MainThread) [homeassistant.core] Error doing job: Task exception was never retrieved
Traceback (most recent call last):
  File "/usr/lib/python3.5/asyncio/tasks.py", line 241, in _step
    result = coro.throw(exc)
  File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/helpers/service.py", line 224, in _handle_service_platform_call
    await getattr(entity, func)(**data)
  File "/usr/lib/python3.5/asyncio/futures.py", line 380, in __iter__
    yield self  # This tells Task to wait for completion.
  File "/usr/lib/python3.5/asyncio/tasks.py", line 304, in _wakeup
    future.result()
  File "/usr/lib/python3.5/asyncio/futures.py", line 293, in result
    raise self._exception
  File "/usr/lib/python3.5/concurrent/futures/thread.py", line 55, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/home/homeassistant/.homeassistant/custom_components/climate/daikin.py", line 239, in set_operation_mode
    self.set({ATTR_OPERATION_MODE: operation_mode})
  File "/home/homeassistant/.homeassistant/custom_components/climate/daikin.py", line 178, in set
    values[daikin_attr] = HA_STATE_TO_DAIKIN[value]
KeyError: 'hot'
2018-10-02 12:59:20 ERROR (MainThread) [homeassistant.core] Error doing job: Task exception was never retrieved
Traceback (most recent call last):
  File "/usr/lib/python3.5/asyncio/tasks.py", line 241, in _step
    result = coro.throw(exc)
  File "/srv/homeassistant/lib/python3.5/site-packages/homeassistant/helpers/service.py", line 224, in _handle_service_platform_call
    await getattr(entity, func)(**data)
  File "/usr/lib/python3.5/asyncio/futures.py", line 380, in __iter__
    yield self  # This tells Task to wait for completion.
  File "/usr/lib/python3.5/asyncio/tasks.py", line 304, in _wakeup
    future.result()
  File "/usr/lib/python3.5/asyncio/futures.py", line 293, in result
    raise self._exception
  File "/usr/lib/python3.5/concurrent/futures/thread.py", line 55, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/home/homeassistant/.homeassistant/custom_components/climate/daikin.py", line 239, in set_operation_mode
    self.set({ATTR_OPERATION_MODE: operation_mode})
  File "/home/homeassistant/.homeassistant/custom_components/climate/daikin.py", line 178, in set
    values[daikin_attr] = HA_STATE_TO_DAIKIN[value]
KeyError: 'fan'
2018-10-02 13:00:12 ERROR (Thread-2) [custom_components.climate.daikin] Invalid value operation_mode for heat

MatteGary · 2018-10-02T05:52:23Z

Just did another test of this (latest version), most modes work fine, but I'm seeing problems with some. ...

@mynameisdaniel32 thanks for your precise testing. This week I don’t have much time to do testing, but I will try to find some. Thanks a lot.

Change in list of Operation Mode

houndci-bot · 2018-10-02T09:03:29Z

homeassistant/components/climate/daikin.py

-                '',
-                self._api.device.represent(daikin_attr)[1]
-            ).title()
+            daikin_mode = re.sub('[^a-z]', '', self._api.device.represent(daikin_attr)[1])


line too long (90 > 79 characters)

houndci-bot · 2018-10-02T09:14:08Z

homeassistant/components/climate/daikin.py

-                self._api.device.represent(daikin_attr)[1]
-            ).title()
+            daikin_mode = re.sub('[^a-z]', '',
+               self._api.device.represent(daikin_attr)[1])


continuation line under-indented for visual indent

MartinHjelmare

Looks good! Let me know when you're happy with testing and we can merge.

MartinHjelmare · 2018-10-02T14:55:42Z

Please update the PR description with a paragraph about the breaking change for the release notes. This is a breaking change since we change the names of the available operating modes of this platform.

MatteGary · 2018-10-08T08:27:48Z

@MartinHjelmare I'm OK with testing, I think we can merge this PR so that everyone else can play with it.

MartinHjelmare · 2018-10-08T08:49:43Z

Great!

charlietomo · 2018-10-15T08:27:26Z

This looks like a good fix. How can I test it or when might I expect it in the main release, so my Hass.io would get updated? I am running the latest (0.80) but still see "hot" so don't think this has made it yet?

MatteGary · 2018-10-15T09:02:13Z

@charlietomo If you want to test it now you can copy the PR code into a custom component. Regarding the merge in the main release, it's all up to the usual process of PR of getting into stable release, I don't know how long will it take to see this PR in the main release.

charlietomo · 2018-10-15T09:34:41Z

Thanks. I downloaded the raw file as daikin.py and moved it into /config/custom_components/climate/daikin.py. I'm using Hass.io and this worked; things now say Heat.
More importantly for me, the Home app on iOS now functions much better. I can set the different modes :-)
I don't know what the "normal process" is, hence the question. But I'm happy with my working solution now so thanks again.

charlietomo · 2018-10-28T21:55:44Z

Just an update and thankyou message. I see that in 0.81 which I just upgraded to on Hass.io this has made it into the main release. I removed my /config/custom_components/climate/daikin.py file and everything is still working well. Thanks again for this.

MatteGary · 2018-10-29T10:20:49Z

@charlietomo you're welcome! As soon as I have time I'll have a look at the 22°C issue.

charlietomo · 2018-11-01T10:22:37Z

@MatteGary there has been some work (solved?) done on the 22°C issue over here. I haven't got it to work yet but I think that is "user error" trying to merge these new changes with other code changes to handle the 22°C issue. Ideally both changes could be merged and end up in the main release!

homeassistant added cla-needed platform: climate.daikin small-pr PRs with less than 30 lines. labels Sep 27, 2018

ghost added the in progress label Sep 27, 2018

houndci-bot reviewed Sep 27, 2018

View reviewed changes

homeassistant added cla-signed and removed cla-needed labels Sep 27, 2018

Build bug fixing

922af63

This comment has been minimized.

Sign in to view

Bug fixing in Set Operation_Mode functionality

fe5bb82

Fixed to .title() functionality in matching the Operation_Mode

This comment has been minimized.

Sign in to view

MartinHjelmare reviewed Sep 30, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

MartinHjelmare reviewed Oct 1, 2018

View reviewed changes

homeassistant/components/climate/daikin.py Show resolved Hide resolved

Fix useless code

4864833

houndci-bot reviewed Oct 1, 2018

View reviewed changes

Fix hound bug

e4f537c

MartinHjelmare reviewed Oct 1, 2018

View reviewed changes

Bug fixing

5ed1eb1

Change in list of Operation Mode

houndci-bot reviewed Oct 2, 2018

View reviewed changes

Trailing white space fix

8fb9925

houndci-bot reviewed Oct 2, 2018

View reviewed changes

MatteGary added 2 commits October 2, 2018 11:14

Compile fixing

5d92aea

Whitespace fix

6a192ba

MartinHjelmare approved these changes Oct 2, 2018

View reviewed changes

MartinHjelmare added the breaking-change label Oct 2, 2018

This was referenced Oct 3, 2018

Daikin heat mode doesn't work from HomeKit #14405

Closed

Climate Daikin operation mode change to "Off" doesn't work as expected #13001

Closed

Climate Daikin on Homekit doesn't start on heat #13122

Closed

MartinHjelmare merged commit ee5e1fa into home-assistant:dev Oct 8, 2018

ghost removed the in progress label Oct 8, 2018

balloob mentioned this pull request Oct 26, 2018

0.81 #17809

Merged

fredrike mentioned this pull request Nov 14, 2018

Fix last version bump on pydaikin #18438

Merged

4 tasks

home-assistant locked and limited conversation to collaborators Feb 5, 2019

ghost added integration: daikin and removed platform: climate.daikin labels Mar 21, 2019

Uh oh!

Conversation

MatteGary commented Sep 27, 2018 • edited by MartinHjelmare Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Breaking Changes

Uh oh!

homeassistant commented Sep 27, 2018

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!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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!

mynameisdaniel32 commented Oct 2, 2018 • edited by MartinHjelmare Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MatteGary commented Oct 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

MatteGary commented Sep 27, 2018 •

edited by MartinHjelmare

Loading

mynameisdaniel32 commented Oct 2, 2018 •

edited by MartinHjelmare

Loading

MartinHjelmare commented Oct 2, 2018 •

edited

Loading