Skip to content
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

Support knx operation types #19546

Merged
merged 4 commits into from Dec 29, 2018
Merged

Support knx operation types #19546

merged 4 commits into from Dec 29, 2018

Conversation

marvin-w
Copy link
Contributor

@marvin-w marvin-w commented Dec 23, 2018

Description:

Updated xknx version to 0.9.3
Adjusted climate component due to changes in the underlying library.

Release notes

  • Breaking change:
    Refactored climate component to be able to support operation types. See the documentation for the new operation modes.
  • the lovelace climate card can now be used with a knx climate device
  • fixed a bug where updating the operation mode wouldn't update the lovelace climate card

Related issue (if applicable): fixes #14531

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

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@homeassistant
Copy link
Contributor

Hi @marvin-w,

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!

@marvin-w
Copy link
Contributor Author

/cc @Julius2342

@marvin-w marvin-w changed the title [WIP][Component.KNX] Updated version to 0.9.3 [Component.KNX] Updated version to 0.9.3 Dec 23, 2018
homeassistant/components/climate/knx.py Outdated Show resolved Hide resolved
homeassistant/components/climate/knx.py Outdated Show resolved Hide resolved
Marvin Wichmann added 2 commits December 24, 2018 20:57
Adjusted climate component due to changes in the underlying library.
@marvin-w marvin-w changed the title [Component.KNX] Updated version to 0.9.3 Updated xknx version to 0.9.3 Dec 24, 2018
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

We can't accept this until the correct operation modes are used for the home assistant user interface, ie current_operation, operation_list and async_set_operation_mode.

homeassistant/components/climate/knx.py Outdated Show resolved Hide resolved
homeassistant/components/climate/knx.py Outdated Show resolved Hide resolved
homeassistant/components/climate/knx.py Outdated Show resolved Hide resolved
homeassistant/components/climate/knx.py Outdated Show resolved Hide resolved
homeassistant/components/climate/knx.py Outdated Show resolved Hide resolved
homeassistant/components/climate/knx.py Show resolved Hide resolved
homeassistant/components/climate/knx.py Outdated Show resolved Hide resolved
homeassistant/components/climate/knx.py Show resolved Hide resolved
@marvin-w
Copy link
Contributor Author

@MartinHjelmare: Thank you for reviewing this. I think I've addressed all review comments. One thing I'm not sure about is the mapping of the operation modes. I'm not particularly happy with some of the mappings since they don't fit the HA operation modes completly.

Also we can't support all operation modes. I think this is ok for now and I'll take care of that at a later stage.

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Great!

@MartinHjelmare
Copy link
Member

Please add a paragraph in the PR description about the breaking change for the release notes.

@marvin-w
Copy link
Contributor Author

@MartinHjelmare I added the paragraph in the PR description.

@MartinHjelmare
Copy link
Member

I tweaked the message a bit. Please make sure to mention in the docs how the mapping is done between home assistant operation mode and knx mode.

@MartinHjelmare MartinHjelmare changed the title Updated xknx version to 0.9.3 Support knx operation types Dec 29, 2018
@MartinHjelmare MartinHjelmare merged commit 338077f into home-assistant:dev Dec 29, 2018
@ghost ghost removed the in progress label Dec 29, 2018
@marvin-w marvin-w deleted the feature/xknx-bumped-to-latest branch December 30, 2018 16:44
@farmio farmio mentioned this pull request Jan 6, 2019
@balloob balloob mentioned this pull request Jan 10, 2019
@ghost ghost removed the platform: climate.knx label Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KNX Climate Control - Can only decrease. Increase breaks the GUI
5 participants