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

Add Tado weather support #44807

Merged
merged 13 commits into from
Mar 10, 2021
Merged

Add Tado weather support #44807

merged 13 commits into from
Mar 10, 2021

Conversation

Noltari
Copy link
Contributor

@Noltari Noltari commented Jan 4, 2021

Proposed change

Add weather support for the Tado integration.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml
tado:
  username: !secret tado_username
  password: !secret tado_password

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link

Hey there @michaelarnauts, @bdraco, mind taking a look at this pull request as its been labeled with an integration (tado) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

Copy link
Contributor

@hencoappel hencoappel left a comment

Choose a reason for hiding this comment

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

Hey,

Left some small comments. Overall looks good though. Looking forward to the change getting in!

homeassistant/components/tado/entity.py Outdated Show resolved Hide resolved
homeassistant/components/tado/weather.py Outdated Show resolved Hide resolved
@MartinHjelmare MartinHjelmare changed the title Tado: add weather support Add Tado weather support Jan 5, 2021
@bouwew
Copy link
Contributor

bouwew commented Jan 9, 2021

@Noltari I'm sorry, I don't mean to dismiss your work, but why would you want to add this to the Tado integration?
Don't you think that the company behind Tado buys the weather-data from one of the commonly available weather sources, which is probably already covered via one of the existing HA Core weather integrations?

@MatthewFlamm
Copy link
Contributor

I looked this over and it looked good to my eyes. I did notice that you have a humidity method that seems unnecessary, but you are forced by the underlying integration to implement it. I created a PR #45095 to address this.

@Noltari
Copy link
Contributor Author

Noltari commented Jan 13, 2021

@Noltari I'm sorry, I don't mean to dismiss your work, but why would you want to add this to the Tado integration?
Don't you think that the company behind Tado buys the weather-data from one of the commonly available weather sources, which is probably already covered via one of the existing HA Core weather integrations?

@bouwew You're right, but it may be interesting to access Tado weather data, since this is used by Tado to decide if your heating is powered on or not.
BTW, I've compared it with OWM and it doesn't match exactly the data provided by them. There seems to be a slight difference in the temperature and sometimes in the condition too.

Tado vs OWM

@Noltari
Copy link
Contributor Author

Noltari commented Jan 13, 2021

I looked this over and it looked good to my eyes. I did notice that you have a humidity method that seems unnecessary, but you are forced by the underlying integration to implement it. I created a PR #45095 to address this.

@MatthewFlamm Yes, there's no way to get rid of humidity, since it's required by Home Assistant's Weather Entity.

@bouwew
Copy link
Contributor

bouwew commented Jan 13, 2021

@Noltari You could add the "values of interest" as sensors that are placed under the Tado hub, or whatever it's called.
This is how we show, for instance, the outdoor temperature in the Plugwise integration:
image
This sensor can then be shown in the Simple Thermostat Card: https://github.com/nervetattoo/simple-thermostat

@bouwew
Copy link
Contributor

bouwew commented Jan 13, 2021

I've just noticed in the mentioned architecture PR that the provided weather data is very limited. Isn't it much simpler and quicker to add 3 sensors instead of adding a full weather implementation?

@Noltari
Copy link
Contributor Author

Noltari commented Jan 14, 2021

@bouwew I added a new commit exporting the weather data as normal sensors.

Copy link
Contributor

@elupus elupus left a comment

Choose a reason for hiding this comment

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

A quick glance over and most things look okey.

Noltari added a commit to Noltari/home-assistant-core that referenced this pull request Feb 8, 2021
This change was suggested by @elupus:
home-assistant#44807 (comment)

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
@Noltari Noltari requested a review from elupus February 13, 2021 11:32
@bdraco
Copy link
Member

bdraco commented Feb 26, 2021

Given the direction of the arch issue, I'd like to get a second opinion before merging: #44807 (comment)

Noltari added a commit to Noltari/home-assistant-core that referenced this pull request Mar 4, 2021
This change was suggested by @elupus:
home-assistant#44807 (comment)

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
@Noltari
Copy link
Contributor Author

Noltari commented Mar 4, 2021

@bdraco rebased on latest dev branch (conflict with 198ecb0).

@Noltari
Copy link
Contributor Author

Noltari commented Mar 10, 2021

Hey @MartinHjelmare, would you mind giving a second opinion about this?
Thanks in advance!

@MartinHjelmare MartinHjelmare removed the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Mar 10, 2021
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Also improve init to avoid calling getMe twice.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
This change was suggested by @elupus:
home-assistant#44807 (comment)

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Only sensors will be exposed until a decission regarding the lack of humidity
is made.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
@Noltari
Copy link
Contributor Author

Noltari commented Mar 10, 2021

@bdraco @MartinHjelmare rebased on latest dev branch and removed usage of weather constants.

@bdraco
Copy link
Member

bdraco commented Mar 10, 2021

Will retest shortly

Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

LGTM

Screen Shot 2021-03-10 at 10 31 01 AM

@bdraco bdraco merged commit ff09643 into home-assistant:dev Mar 10, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Mar 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants