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

Adding climate.velbus support #18100

Merged
merged 5 commits into from Nov 2, 2018

Conversation

@Cereal2nd
Contributor

Cereal2nd commented Nov 1, 2018

Description:

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

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

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

  • New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

Cereal2nd added some commits Oct 30, 2018

Show resolved Hide resolved homeassistant/components/climate/velbus.py Outdated
Show resolved Hide resolved homeassistant/components/climate/velbus.py Outdated
Show resolved Hide resolved homeassistant/components/climate/velbus.py Outdated
Show resolved Hide resolved homeassistant/components/climate/velbus.py Outdated

@Cereal2nd Cereal2nd referenced this pull request Nov 1, 2018

Merged

Adding climate.velbus #7323

2 of 2 tasks complete

Cereal2nd and others added some commits Nov 1, 2018

@fabaff

fabaff approved these changes Nov 2, 2018

Looks good to me 🐦

@fabaff fabaff merged commit 3fe895c into home-assistant:dev Nov 2, 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.002%) to 93.065%
Details

@wafflebot wafflebot bot removed the in progress label Nov 2, 2018

DEPENDENCIES = ['velbus']
OPERATION_LIST = ['comfort', 'day', 'night', 'safe']

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 2, 2018

Member

This is wrong. Only states imported from the base climate component is allowed.

This comment has been minimized.

@Cereal2nd

Cereal2nd Nov 2, 2018

Contributor

I don't see how we can get all states to work in this case ...3
What do you suggest we use as a mapping?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 2, 2018

Member

I don't know the integration or what those modes do, so I don't have a suggestion at the moment.

This comment has been minimized.

@Cereal2nd

Cereal2nd Nov 3, 2018

Contributor

basically those 4 are are all temperature presets

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 3, 2018

Member

The home assistant operation state explains how the thermostat tries to adjust temperature, eg heat, cool, auto, eco. It doesn't say what temperature the thermostat should set. We have target_temperature and set_temperature for that.

If I understand correctly 'comfort', 'day', 'night', 'safe' are levels of temperature the thermostat should try to keep? If so, we shouldn't use operation mode in home assistant to change those.

This comment has been minimized.

@Cereal2nd

Cereal2nd Nov 3, 2018

Contributor

so this means i will only have on operation mode in HA (heat)
and how do i handle the temperature levels?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 3, 2018

Member

If we can set a specific temperature degree with set_temperature these modes are not required, right? I don't think we currently have any matching control options for these modes. But look at the base climate component features.

@property
def current_operation(self):
"""Return current operation ie. heat, cool, idle."""
return self._module.get_climate_mode()

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 2, 2018

Member

This should return one of the home assistant climate states.

def set_operation_mode(self, operation_mode):
"""Set new target operation mode."""
self._module.set_mode(operation_mode)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 2, 2018

Member

We need to convert from home assistant climate state to device mode and vice versa. Define two constant dicts at the module level that do this.

def set_operation_mode(self, operation_mode):
"""Set new target operation mode."""
self._module.set_mode(operation_mode)
self.schedule_update_ha_state()

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 2, 2018

Member

Remove this. Let the state update callback update state.

"""Set new target temperatures."""
if kwargs.get(ATTR_TEMPERATURE) is not None:
self._module.set_temp(kwargs.get(ATTR_TEMPERATURE))
self.schedule_update_ha_state()

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 2, 2018

Member

See above.

@balloob

This comment has been minimized.

Member

balloob commented Nov 8, 2018

@MartinHjelmare is right, this platform does not follow our climate entity specification. To avoid a future breaking change, I will remove this and it can be submitted again with the right operation modes.

@balloob balloob removed the new-platform label Nov 8, 2018

@balloob balloob referenced this pull request Nov 9, 2018

Merged

0.82 #18335

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