Refactor Velux integration

[pawlizio]

Proposed change

Config Flow added for Velux component
Tilt functionality added for Velux cover
Workaround for #34844 by providing a reboot_gateway service

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
velux:
  host: "192.168.xxx.xxx"
  password: !secret velux_password

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request:

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:

• Documentation added/updated for www.home-assistant.io

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:

• I have reviewed two other open pull requests in this repository.

[probot-home-assistant]

Hey there @Julius2342, mind taking a look at this pull request as its been labeled with an integration (velux) you are listed as a codeowner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[epenet]

Two quick general comments:

• please use references in the config flow translations (Translations: Use references in config flow #40578)
• to avoid breaking change, use an import routine to migrate from YAML to ConfigEntry (

  core/homeassistant/components/nut/sensor.py

  Lines 60 to 67 in 9ee97cb

  	async def async_setup_platform(hass, config, async_add_entities, discovery_info=None):
  	"""Import the platform into a config entry."""
  	hass.async_create_task(
  	hass.config_entries.flow.async_init(
  	DOMAIN, context={"source": SOURCE_IMPORT}, data=config
  	)
  	)

  )

[pawlizio]

Two quick general comments:

• please use references in the config flow translations (Translations: Use references in config flow #40578)
• to avoid breaking change, use an import routine to migrate from YAML to ConfigEntry (

  core/homeassistant/components/nut/sensor.py

  Lines 60 to 67 in 9ee97cb

  	async def async_setup_platform(hass, config, async_add_entities, discovery_info=None):
  	"""Import the platform into a config entry."""
  	hass.async_create_task(
  	hass.config_entries.flow.async_init(
  	DOMAIN, context={"source": SOURCE_IMPORT}, data=config
  	)
  	)

  
  )

Added referenced and corrected the Type of Change description, YAML to ConfigEntry was already included, so no breaking change.

[epenet]

I also suggest that you split this into 3 PR?

• one for the config flow (this will most likely take longest to review)
• one for adding tilt functionnality
• one for reboot gateway

Also, config_flow requires 100% test coverage so you will need to update .coveragerc with the list of files, and build some actual tests for the config_flow:
homeassistant/components/velux/*
homeassistant/components/velux/cover.py
homeassistant/components/velux/scene.py

Note: I'm just a contributor not an official reviewer

[epenet]

The translation files (en.json) should have the strings in plain text - only the strings.json should have the translation keys, also for easy of review I suggest that the order of the strings matches between the en.json and strings.json

Suggested change

	"already_configured_device": "[%key:common::config_flow::error::already_configured_device%]",
	"already_configured": "The device has already been configured.",

[pawlizio]

I updated the translation file by using python3 -m script.translations develop. The order is changed automatically.

[epenet]

Ref #42895

[epenet]

@pawlizio did you see my note about test coverage? It is compulsory to have tests which cover the whole of the config_flow and until you have that the config_flow will never get merged into the core.

[pawlizio]

@epenet: Yes I saw, but not sure when I'll find the time to learn how to write that unit tests and finally to do it. I'm not a professional software developer, so it will take me some time which I do not have now. For the moment I'm fine with my custom component

[mtdcr]

@pawlizio I'd recommend rebasing this once #43198 got merged, so the code stays in good shape.

@frenck Could you please remove the 'breaking-change' label?

[pawlizio]

@pawlizio I'd recommend rebasing this once #43198 got merged, so the code stays in good shape.

@mtdcr The reboot function is already included in this pull request:

core/homeassistant/components/velux/__init__.py

Lines 45 to 48 in a81e9fe

	async def async_reboot_gateway(service_call):
	await gateway.reboot_gateway()
	hass.services.async_register(DOMAIN, "reboot_gateway", async_reboot_gateway)

So if this would be integrated first, #43198 should be closed. If #43198 will be merged first it will cause a conflict which needs to be resolved. Can this be solved by a rebase?

I created #43198 because I assume such small pull request will be integrated much faster and I'm not sure if this one will ever be merged due to missing unit tests.

[mtdcr]

If #43198 will be merged first it will cause a conflict which needs to be resolved. Can this be solved by a rebase?

Yes, that's what I meant. Rebasing will probably cause a conflict, which you will have to solve manually, but the result will be a PR without the reboot part. If you'll face difficulties, I can try to assist. Just ping me by mail then (obi at saftware dot de).

The same procedure should be followed when splitting out tilt functionality. In the meantime, you can mark this PR as work in progress by prefixing the title with WIP: .

I'm not sure if this one will ever be merged due to missing unit tests.

I think I'll be able to help with the unit tests later this year. I'm interested in the conversion to config flow, because it allows to start Home Assistant with the Velux gateway being offline (or still being busy rebooting).

[dumpfheimer]

Not stale 😬

[balloob]

This is the one that has been automatically generated https://github.com/home-assistant/core/blob/dev/script/scaffold/templates/config_flow/tests/test_config_flow.py

Please use that as a basis for your tests.

[github-actions]

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

[dumpfheimer]

🥷

[github-actions]

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

[dumpfheimer]

I would still very much appreciate seeing this PR in HA

[pergolafabio]

Same here

[GeertvanHorrik]

To not just +1 this, I am happy to financially contribute to the person implementing this, either via GitHub sponsorship or open collective. If more people follow, there should be enough incentive for someone to pick this up.

[dumpfheimer]

Side note: I have been using this PR for some months now, without any issues.
I would still very much appreciate it being merged.

How can we support the process?

[pergolafabio]

Side note: I have been using this PR for some months now, without any issues.
I would still very much appreciate it being merged.

How can we support the process?

Do you still need to reboot the gateway? On boot or on restart?

[dumpfheimer]

Actually I have 3 automations that restart the gateway:

1. If HA comes up and the devices stay unavailable
2. If one of the devices becomes unavailable for 30 minutes
3. Timed reboot at 02:00 in the night with a "stop" command after some time to cause a reconnect
   (I "reboot" the gateway by killing its power supply)

To clarify my statement:
The whole KLF200 thing is not working flawlessly - but they all seem (to me) to be KLF200 Firmware issues.

To me this PR has introduced no new issues but improved the experience in HA

[pergolafabio]

Can you post those automations?

[dumpfheimer]

Here you go.
As mentioned, I reboot the gateway by cutting its power supply. That is said shell_command.restart_klf.

velux_reboot_and_reconnect_at_night.txt
velux_auto_reboot_when_unavailable.txt
velux_automation_hass_start.txt

[Ra72xx]

For me it is always unpredictable if the Velux gateway is available after a restart of HA. Is there a reason why the pull request does not get merged?
Also the restart service of the integration does not seem to work for me, I always have to cut the power supply.

[dumpfheimer]

There seems to be an issue when pyvlx disconnects before disabling the house monitor. But I believe these are issues that should be discussed over at pyvlx, not here in the PR

[dumpfheimer]

@pawlizio I just saw that you have deleted the branch, could you kindly tell us if you are still on to this?

[pergolafabio]

Here you go.
As mentioned, I reboot the gateway by cutting its power supply. That is said shell_command.restart_klf.

velux_reboot_and_reconnect_at_night.txt
velux_auto_reboot_when_unavailable.txt
velux_automation_hass_start.txt

strange, i cant download those 3 files?

[github-actions]

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.