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

Fix and refactor current tahoma integration #41267

Closed

Conversation

iMicknl
Copy link
Contributor

@iMicknl iMicknl commented Oct 5, 2020

This is quite a big pull request to review unfortunately, due to the changes we had to make to the original tahoma integration. The tahoma integration is broken since the 2020.3.4-12 firmware update and has an active alert on Home Assistant Alerts.

@vlebourl, @tetienne and me worked on rewriting this component for the last months and we think it is now ready to be added to core. We have had quite some testers using the custom component and we had tons of feedback.

We will split the changes in multiple PR's to core, however we couldn't ignore that the first one would be bigger than expected. It was not possible to only touch parts of this integration, without fixing others, to bring this integration back in line with the current standards.

Future PR's will add support for additional platforms like alarm_control_panel, climate and light plus some other fixes that would clutter this pr to much.

Breaking change

  • Entities now use entity ids based on their unique device url.
  • Changed MotionSensor, SmokeSensor, ContactSensor mapping from sensor to binary_sensor.
  • Changed GarageDoor mapping from switch to cover.
  • Changed OnOffLight from switch to light (light not in this PR).
  • Stateless devices (RTS) will be marked as assumed_state = True.
  • Support for excluding devices has been removed. Use the disable entities feature, which is provided by Home Assistant.

Proposed change

  • Almost all Somfy devices are now supported out of the box since we moved away from a hardcoded list of supported devices. Every device will be mapped based on their category and available states and commands.
    • For example this integrations now supports pergolas, gates, swimming pools, curtains, electricity sensors, humidity sensors, temperature sensors, air sensors, heating systems, rgb lights.
  • Supports Config Flow and Options Flow
  • Uses new async Python API with better exception handling and retry logic
  • Uses DataUpdateCoordinator to update all entity states via one call. Implements an event listener constantly listening to new events (even outside Home Assistant) to be more reliable and to consume less resources, which will result in no more TooManyRequests exceptions.
  • Improve cover platform with support for tilt position and indication if a cover is opening or closing.
  • All cover movement can now be cancelled, even when action initiated outside Home Assistant.
  • Adds services to set cover position with low speed and to set cover to my position (preset).

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
tahoma:
  username: YOUR_USERNAME
  password: YOUR_PASSWORD

Since this integration now fully supports the config flow, the YAML configuration could possibly be removed in the future.

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][dev-checklist]
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.
    • Only tests have been added for Config Flow, future PR's could introduce more test coverage

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

  • Documentation added/updated for [www.home-assistant.io][docs-repository]

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

  • The [manifest file][manifest-docs] 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][quality-scale]:

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

To help with the load of incoming pull requests:

Signed-off-by: Mick Vleeshouwer <mick@imick.nl>
@iMicknl iMicknl force-pushed the refactor_tahoma_integration branch from 717e7f7 to 22b64c1 Compare October 5, 2020 13:24
@MartinHjelmare
Copy link
Member

Why do we have to add the config flow in this PR? I'd expect this PR to just change the library and all interfaces to the library that needs to change.

@vlebourl
Copy link
Contributor

vlebourl commented Oct 5, 2020

The whole logic has been changed along side the API. I'm not sure it makes any sense to compare it to the existing core integration. Although it replaces the old one with the same domain name, I would argue in favor of treating it as a new integration. Not sure whether that's possible though.

@MartinHjelmare
Copy link
Member

It's not a new integration. We don't need to change this much in this PR. Please strip it down to the bare minimum in each PR. This PR should just change the library and the required interfaces. It should not add any new features.

@vlebourl
Copy link
Contributor

vlebourl commented Oct 5, 2020

fair enough :)

@iMicknl
Copy link
Contributor Author

iMicknl commented Oct 5, 2020

It's not a new integration. We don't need to change this much in this PR. Please strip it down to the bare minimum in each PR. This PR should just change the library and the required interfaces. It should not add any new features.

I understand that a PR should be small, since this is very hard to review. We can separate the Config Flow for sure, however breaking down all the new functionality into separate PR's will be hard... I could start with removing the Config Flow + some extra added features in cover.py, but I am not sure if that will fully solve your concerns.

Indeed, this PR also offers new functionality, since we worked for months on it as a custom component. The main issue is that the integration is core is broken, but still has many platforms like sensor, binary_sensor, cover, lock and switch. I can keep them as is, however that will result in a broken integration when it is merged.

Another possibility could be to remove the broken tahoma integration and submit this as the somfy_tahoma integration with just one platform.

@MartinHjelmare
Copy link
Member

What needs to change in the platforms, besides the library interface, to not have a broken integration?

@iMicknl
Copy link
Contributor Author

iMicknl commented Oct 5, 2020

What needs to change in the platforms, besides the library interface, to not have a broken integration?

Almost all code, since the new async API is using different endpoints and responses in combination with the DataUpdateCoordinator.

I will strip as much code as possible and update this PR, however the code in the platforms needs fundamental changes, which I could probably not work around. But the ConfigFlow and OptionsFlow could be separated in another PR, same for the services added.

I don't prefer to work from zero again to adapt the old code to the new async API, since there are many fundamental differences around handling commands / states and not relying on a hardcoded list.
This will take a lot of work and in that case I rather keep this integration as a custom component.

@MartinHjelmare
Copy link
Member

It's ok to update the platforms to make them work with the new library. Just don't add any new user facing features.

Signed-off-by: Mick Vleeshouwer <mick@imick.nl>
Signed-off-by: Mick Vleeshouwer <mick@imick.nl>
Signed-off-by: Mick Vleeshouwer <mick@imick.nl>
@iMicknl
Copy link
Contributor Author

iMicknl commented Oct 5, 2020

It's ok to update the platforms to make them work with the new library. Just don't add any new user facing features.

Great to hear and thanks for your review. I have removed the Config Flow, Options Flow and registered services. This should eliminate all user facing features. I don't believe that we are able to get this pull request any smaller.

Do you think that this one is small enough to be reviewed?

@iMicknl
Copy link
Contributor Author

iMicknl commented Nov 23, 2020

We just received confirmation from Somfy that they allow the use of the internal TaHoma API! 🥳 There are a few restrictions, which require us to make small changes to this PR.

  • Somfy’s position is to allow you to perform a fetch of the events every 30 seconds (when no active session is open).
  • As already said, when an active session is open, you can fetch the events every seconds. As you are not likely aware of an “active session” with HA or Homey (the way we defined it – meaning the end-user is physically connected to an interface), we accept by extrapolation that you use such poll rate when you have to follow the execution of a device or group of devices.

This is not a definitive solution, Somfy will be monitoring the traffic generated and evaluate.

@iMicknl
Copy link
Contributor Author

iMicknl commented Dec 5, 2020

Closing in favour of #43966.

@iMicknl iMicknl closed this Dec 5, 2020
Dev automation moved this from Needs review to Cancelled Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment