New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Daikin Climate - Better integration with Climate base component #16913

Merged
merged 9 commits into from Oct 8, 2018

Conversation

Projects
None yet
6 participants
@MatteGary
Contributor

MatteGary commented Sep 27, 2018

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.

Daikin Climate - Better integration with Climate base component
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

This comment has been minimized.

homeassistant commented Sep 27, 2018

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!

@@ -262,4 +277,4 @@ def swing_list(self):
def update(self):
"""Retrieve latest state."""
self._api.update(no_throttle=self._force_refresh)
self._force_refresh = False
self._force_refresh = False

This comment has been minimized.

@houndci-bot

houndci-bot Sep 27, 2018

no newline at end of file

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

This comment has been minimized.

@houndci-bot

houndci-bot Sep 27, 2018

trailing whitespace

'[^a-z]',
'',
self._api.device.represent(daikin_attr)[1]
).title()
)

This comment has been minimized.

@houndci-bot

houndci-bot Sep 27, 2018

blank line contains whitespace

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

@mynameisdaniel32

This comment was marked as outdated.

mynameisdaniel32 commented Sep 29, 2018

I've just tried to run your daikin.py file on my system (as a custom_component) and can't get it to work, when trying operations via the front end I get errors showing and nothing happens.

2018-09-29 16:57:00 ERROR (Thread-2) [custom_components.climate.daikin] Invalid value operation_mode for Auto

Thanks for looking into this, I'm happy to assist with any testing - very keen to get my Daikin unit working through HomeKit better.

@MatteGary

This comment was marked as outdated.

Contributor

MatteGary commented Sep 29, 2018

@mynameisdaniel32 Thanks for testing this!
Just to be sure that I understand what you have tried: did you try to change the Operation Mode from HomeAssistant front-end?

Bug fixing in Set Operation_Mode functionality
Fixed to .title() functionality in matching the Operation_Mode
@MatteGary

This comment was marked as outdated.

Contributor

MatteGary commented Sep 29, 2018

@mynameisdaniel32 Thanks for testing this!
Just to be sure that I understand what you have tried: did you try to change the Operation Mode from HomeAssistant front-end?

@mynameisdaniel32 I made a modification in the script, I did a brief testing and it seems to work. Can you please test it again, maybe paying attention to the temperature set as a target when you turn on the unit via HA? I would like to deep into this step to be sure that it works properly (I didn't change anything in that part from the original script), but I don't have much time until next weekend.

@mynameisdaniel32

This comment was marked as outdated.

mynameisdaniel32 commented Sep 29, 2018

I just had another play, what I was doing (and have done again) is changing the operation mode via the front end, and also via HomeKit. I've just tried your latest version and I'm getting the same errors, the first 3 attempts are via the front end (one being a script). The other being done via HomeKit.

Setting the operation mode still doesn't do anything on the unit (and reverts back to off the next time the status is checked) it just logs an error. Changing the fan and swing modes seem to work just fine. As does the temperature. There is no change to the target temperature when attempting to change the operation mode (I think previously it was resetting to 22?), but this might be because it fails?

2018-09-30 07:10:46 ERROR (Thread-4) [custom_components.climate.daikin] Invalid value operation_mode for Auto
2018-09-30 07:11:01 ERROR (Thread-3) [custom_components.climate.daikin] Invalid value operation_mode for auto
2018-09-30 07:11:37 ERROR (Thread-4) [custom_components.climate.daikin] Invalid value operation_mode for cool
2018-09-30 07:12:32 ERROR (Thread-3) [custom_components.climate.daikin] Invalid value operation_mode for auto
2018-09-30 07:13:18 ERROR (Thread-5) [custom_components.climate.daikin] Invalid value operation_mode for heat
@MatteGary

This comment was marked as outdated.

Contributor

MatteGary commented Sep 29, 2018

This is strange because I tried before and it works.
I think also that somewhere there is a bug regarding resetting the target temp to 22, because I had that behavior. But first I’ll try to make it work good for what concern operation mode. One step at the time.

I will try some more deep test using HA front-end, and after that I will move to Homekit interaction. I’ll get back at you as soon as I have some results.

@mynameisdaniel32

This comment was marked as outdated.

mynameisdaniel32 commented Sep 29, 2018

No problem - I assume what I'm doing to test is sufficient (I can get debugging logs if they'll help) and I'm not causing issues by loading this as a custom_component? When I change the version in my custom_components to match the one before this PR, things work as they did before so I figured this was okay.

@MatteGary

This comment was marked as outdated.

Contributor

MatteGary commented Sep 29, 2018

I’m still using it as custom component for testing purpose, so I believe it’s ok as soon as you remember to restart everything everytime you made some modification to the component’s code :)

@MatteGary

This comment was marked as outdated.

Contributor

MatteGary commented Sep 30, 2018

I've run some further tests, and it looks to me that it is working fine.
Here's what I tried:

  • Turn on the AC from HA front-end
  • Change the AC Operation Mode from HA front-end
  • Turn off the AC from HA front-end
  • Turn on the AC from Homekit App on iPhone
  • Turn off the AC form Homekit App on iPhone

The only bug I experienced is the "automatic" set of the target temperature to 22° C. I will look in to that, maybe in another pull request after this in order to keep thing separate.

Sorry for the silly question: are you sure that you have updated the custom_component, and after that restarted HA in order to load again the updated version? If you're about this I can try adding more logs so we can have more intel about why on your system is not working properly.

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)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

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

This comment has been minimized.

@MatteGary

MatteGary Sep 30, 2018

Contributor

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.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

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

This comment has been minimized.

@MatteGary

MatteGary Sep 30, 2018

Contributor

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)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

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.

This comment has been minimized.

@MatteGary

MatteGary Oct 1, 2018

Contributor

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

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 1, 2018

Member

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 comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 1, 2018

Member

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
@@ -76,7 +85,7 @@ def __init__(self, api):
self._force_refresh = False
self._list = {
ATTR_OPERATION_MODE: list(
map(str.title, set(HA_STATE_TO_DAIKIN.values()))
map(str, set(DAIKIN_TO_HA_STATE.values()))

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Why do we need the str copy?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

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.

This comment has been minimized.

@MatteGary

MatteGary Sep 30, 2018

Contributor

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.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Since we're here, please clean it up.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 1, 2018

Member

Change this to:

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

This comment was marked as outdated.

mynameisdaniel32 commented Sep 30, 2018

Sorry for the silly question: are you sure that you have updated the custom_component, and after that restarted HA in order to load again the updated version? If you're about this I can try adding more logs so we can have more intel about why on your system is not working properly.

All good, that is weird... Yep, grabbing the latest version of the file from this PR and putting it here: [config_dir]/custom_components/climate/daikin.py - I'm then restarting HA. I'm sure it's picking up the new file as it breaks setting Operation Mode, but if I duplicate the original daikin.py file in the same location, then restart HA again, it works again.

@mynameisdaniel32

This comment was marked as outdated.

mynameisdaniel32 commented Sep 30, 2018

Sorry for the silly question: are you sure that you have updated the custom_component, and after that restarted HA in order to load again the updated version? If you're about this I can try adding more logs so we can have more intel about why on your system is not working properly.

All good, that is weird... Yep, grabbing the latest version of the file from this PR and putting it here: [config_dir]/custom_components/climate/daikin.py - I'm then restarting HA. I'm sure it's picking up the new file as it breaks setting Operation Mode, but if I duplicate the original daikin.py file in the same location, then restart HA again, it works again.

Disregard me, I worked it out and tested again, getting the same results as you now. It seems the way I was grabbing/transferring the file from github was causing issues, I didn't suspect that earlier as it half-worked.

@MatteGary

This comment was marked as outdated.

Contributor

MatteGary commented Oct 1, 2018

Sorry for the silly question: are you sure that you have updated the custom_component, and after that restarted HA in order to load again the updated version? If you're about this I can try adding more logs so we can have more intel about why on your system is not working properly.

All good, that is weird... Yep, grabbing the latest version of the file from this PR and putting it here: [config_dir]/custom_components/climate/daikin.py - I'm then restarting HA. I'm sure it's picking up the new file as it breaks setting Operation Mode, but if I duplicate the original daikin.py file in the same location, then restart HA again, it works again.

Disregard me, I worked it out and tested again, getting the same results as you now. It seems the way I was grabbing/transferring the file from github was causing issues, I didn't suspect that earlier as it half-worked.

Oh great, the good thing is that now is working for you too!

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];

This comment has been minimized.

@houndci-bot

houndci-bot Oct 1, 2018

statement ends with a semicolon

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

This comment has been minimized.

@houndci-bot

houndci-bot Oct 1, 2018

missing whitespace after ','

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

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 1, 2018

Member

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.

This comment has been minimized.

@MatteGary

MatteGary Oct 1, 2018

Contributor

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.

This comment has been minimized.

This comment has been minimized.

@MatteGary

MatteGary Oct 2, 2018

Contributor

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 😃

This comment has been minimized.

@MatteGary

MatteGary Oct 2, 2018

Contributor

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

This comment has been minimized.

mynameisdaniel32 commented Oct 2, 2018

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

This comment has been minimized.

Contributor

MatteGary commented Oct 2, 2018

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.

Bug fixing
Change in list of Operation Mode
'',
self._api.device.represent(daikin_attr)[1]
).title()
daikin_mode = re.sub('[^a-z]', '', self._api.device.represent(daikin_attr)[1])

This comment has been minimized.

@houndci-bot

houndci-bot Oct 2, 2018

line too long (90 > 79 characters)

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

This comment has been minimized.

@houndci-bot

houndci-bot Oct 2, 2018

continuation line under-indented for visual indent

MatteGary added some commits Oct 2, 2018

@MartinHjelmare

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

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 2, 2018

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

This comment has been minimized.

Contributor

MatteGary commented Oct 8, 2018

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

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 8, 2018

Great!

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

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 93.701%
Details

@wafflebot wafflebot bot removed the in progress label Oct 8, 2018

@charlietomo

This comment has been minimized.

charlietomo commented Oct 15, 2018

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

This comment has been minimized.

Contributor

MatteGary commented Oct 15, 2018

@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

This comment has been minimized.

charlietomo commented Oct 15, 2018

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.

@balloob balloob referenced this pull request Oct 26, 2018

Merged

0.81 #17809

@charlietomo

This comment has been minimized.

charlietomo commented Oct 28, 2018

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

This comment has been minimized.

Contributor

MatteGary commented Oct 29, 2018

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

@charlietomo

This comment has been minimized.

charlietomo commented Nov 1, 2018

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

@fredrike fredrike referenced this pull request Nov 14, 2018

Merged

Fix last version bump on pydaikin #18438

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment