Skip to content

Add support for Stiebel Eltron heat pumps#21199

Merged
MartinHjelmare merged 26 commits intohome-assistant:devfrom
fucm:stiebel-eltron
Apr 13, 2019
Merged

Add support for Stiebel Eltron heat pumps#21199
MartinHjelmare merged 26 commits intohome-assistant:devfrom
fucm:stiebel-eltron

Conversation

@fucm
Copy link
Copy Markdown
Contributor

@fucm fucm commented Feb 18, 2019

Description:

Add initial support for a Stiebel Eltron component to support heat pumps with ISGWeb Modbus module.

It supports the climate platform currently and is prepared to add the sensor platform later on.

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

Example entry for configuration.yaml (if applicable):

modbus:
  type: tcp
  host: 192.168.1.20
  port: 502

stiebel_eltron:
  name: LWZ504e

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:

  • It does.

To do:

  • Run script script/manifest/codeowners.py
  • Resolve conflict files
  • Update docs with state mappings

@ghost ghost added the in progress label Feb 18, 2019
@fucm fucm changed the title WIP: Add support for Stiebel Eltron heat pumps Add support for Stiebel Eltron heat pumps Feb 27, 2019
@fucm
Copy link
Copy Markdown
Contributor Author

fucm commented Feb 27, 2019

WIP removed. Please review the state translation. I did not manage to get it working locally, even if it should be implemented as specified by the documentation.

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

Comment thread homeassistant/components/stiebel_eltron/__init__.py Outdated
Comment thread homeassistant/components/stiebel_eltron/__init__.py Outdated
Comment thread homeassistant/components/stiebel_eltron/__init__.py Outdated
Comment thread homeassistant/components/stiebel_eltron/__init__.py Outdated
Comment thread homeassistant/components/stiebel_eltron/__init__.py Outdated
Comment thread homeassistant/components/stiebel_eltron/climate.py Outdated
Comment thread homeassistant/components/stiebel_eltron/climate.py Outdated
Comment thread homeassistant/components/stiebel_eltron/climate.py Outdated
Comment thread homeassistant/components/stiebel_eltron/climate.py Outdated
Comment thread homeassistant/components/stiebel_eltron/strings.json Outdated
Comment thread homeassistant/components/stiebel_eltron/climate.py Outdated
@fucm
Copy link
Copy Markdown
Contributor Author

fucm commented Apr 6, 2019

Manifest file added. Can I remove the entry from CODEOWNERS?

@fucm
Copy link
Copy Markdown
Contributor Author

fucm commented Apr 6, 2019

I will resolve the conflicting files, after the review issues have been resolved.

@MartinHjelmare
Copy link
Copy Markdown
Member

The codeowners file should be automatically populated with info from the manifest file when running the script script/manifest/codeowners.py.

@fucm
Copy link
Copy Markdown
Contributor Author

fucm commented Apr 7, 2019

I will update the docs (for the state mappings) as soon as this review is done.

Some questions

  • Should I mark the conversations as resolved or is this your job in the review process?
  • Am I allowed to remove the in progress label?

BTW: Thanks!

@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Apr 8, 2019

You can resolve the comments when you think they are resolved. I'll unresolve if I disagree. 👍

The in progress label is handled automatically by a bot. Don't worry about it.

@fucm fucm force-pushed the stiebel-eltron branch from a2be7b6 to d156b38 Compare April 8, 2019 19:48
@fucm
Copy link
Copy Markdown
Contributor Author

fucm commented Apr 8, 2019

All done. I hope the code is ready now.

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

Probably try to rebase again to fix .coveragerc.

Comment thread .coveragerc Outdated
@fucm fucm force-pushed the stiebel-eltron branch from 9d03869 to 0b7361c Compare April 9, 2019 18:24
Copy link
Copy Markdown
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.

Looks good!

Please add the integration component package to .coveragerc. Minor comment below too.

Comment thread homeassistant/components/stiebel_eltron/climate.py Outdated
@fucm
Copy link
Copy Markdown
Contributor Author

fucm commented Apr 9, 2019

.coveragerc is updated.

Question: is the temperature conversion to Fahrenheit handled by the core or does it need to be implemented by the platform?

Comment thread homeassistant/components/stiebel_eltron/climate.py Outdated
@MartinHjelmare
Copy link
Copy Markdown
Member

Yes, the temperature unit conversion is handled by the core.

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

Can be merged when build passes.

@fucm
Copy link
Copy Markdown
Contributor Author

fucm commented Apr 12, 2019

What's the normal waiting time for a build check? Is 3 days normal?

@MartinHjelmare
Copy link
Copy Markdown
Member

I've restarted the workflow on circleci. Let's see what happens.

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

Sorry. We're in the middle of a major refactor. Please remove requirements and dependencies constants in the component.

Comment thread homeassistant/components/stiebel_eltron/__init__.py Outdated
Copy link
Copy Markdown
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.

Thanks!

@MartinHjelmare MartinHjelmare merged commit 0a0975b into home-assistant:dev Apr 13, 2019
@ghost ghost removed the in progress label Apr 13, 2019
@fucm fucm deleted the stiebel-eltron branch April 17, 2019 20:05
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.

5 participants