Skip to content

Daikin Climate - Better integration with Climate base component #16913

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.

Sign up for GitHub

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

Merged
MartinHjelmare merged 9 commits into from
Oct 8, 2018
Merged

Daikin Climate - Better integration with Climate base component #16913

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c97dae4
Daikin Climate - Better integration with Climate base component
MatteGary Sep 26, 2018
922af63
Build bug fixing
MatteGary Sep 27, 2018
fe5bb82
Bug fixing in Set Operation_Mode functionality
MatteGary Sep 29, 2018
4864833
Fix useless code
MatteGary Oct 1, 2018
e4f537c
Fix hound bug
MatteGary Oct 1, 2018
5ed1eb1
Bug fixing
MatteGary Oct 2, 2018
8fb9925
Trailing white space fix
MatteGary Oct 2, 2018
5d92aea
Compile fixing
MatteGary Oct 2, 2018
6a192ba
Whitespace fix
MatteGary Oct 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions homeassistant/components/climate/daikin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,15 @@
STATE_OFF: 'off',
}

DAIKIN_TO_HA_STATE = {
'fan': STATE_FAN_ONLY,
'dry': STATE_DRY,
'cool': STATE_COOL,
'hot': STATE_HEAT,
'auto': STATE_AUTO,
'off': STATE_OFF,
}

HA_ATTR_TO_DAIKIN = {
ATTR_OPERATION_MODE: 'mode',
ATTR_FAN_MODE: 'f_rate',
Expand Down Expand Up @@ -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()))

@MartinHjelmare MartinHjelmare Sep 30, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need the str copy?

@MartinHjelmare MartinHjelmare Sep 30, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@MatteGary MatteGary Sep 30, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@MartinHjelmare MartinHjelmare Sep 30, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we're here, please clean it up.

@MartinHjelmare MartinHjelmare Oct 1, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Change this to:

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

),
ATTR_FAN_MODE: list(
map(
Expand Down Expand Up @@ -136,11 +145,17 @@ def get(self, key):
elif key == ATTR_OPERATION_MODE:
# Daikin can return also internal states auto-1 or auto-7
# and we need to translate them as AUTO
value = re.sub(
tmp_value = re.sub(
'[^a-z]',
'',
self._api.device.represent(daikin_attr)[1]
).title()
)

@houndci-bot houndci-bot Sep 27, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

blank line contains whitespace

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)

@MartinHjelmare MartinHjelmare Sep 30, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@MatteGary MatteGary Sep 30, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@MartinHjelmare MartinHjelmare Sep 30, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@MatteGary MatteGary Sep 30, 2018

Copy link
Copy Markdown
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 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)

@MartinHjelmare MartinHjelmare Sep 30, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@MatteGary MatteGary Oct 1, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@MartinHjelmare MartinHjelmare Oct 1, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@MartinHjelmare MartinHjelmare Oct 1, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

else:
value = tmp_value

@houndci-bot houndci-bot Sep 27, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

trailing whitespace


if value is None:
_LOGGER.error("Invalid value requested for key %s", key)
Expand Down Expand Up @@ -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

@houndci-bot houndci-bot Sep 27, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

no newline at end of file