Skip to content

Remove Velux KLF disconnection on HA stop event#47465

Closed
pawlizio wants to merge 2 commits intohome-assistant:devfrom
pawlizio:velux_on_stop
Closed

Remove Velux KLF disconnection on HA stop event#47465
pawlizio wants to merge 2 commits intohome-assistant:devfrom
pawlizio:velux_on_stop

Conversation

@pawlizio
Copy link
Contributor

@pawlizio pawlizio commented Mar 5, 2021

Proposed change

The newly merged workaround #43198 for to the issue #34844 does not work if the disconnection command is initiated on HA stop event. So in order to be able to reboot the velux KLF gateway as discribed in the automation example it is necessary to remove this functionality.

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

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 @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)

async def on_hass_stop(event):
"""Close connection when hass stops."""
_LOGGER.debug("Velux interface terminated")
await self.pyvlx.disconnect()
Copy link
Member

Choose a reason for hiding this comment

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

It seems weird that we would not cleanly disconnect. Why is an automation even necessary to reboot. Seems like we're now asking each used to solve the same problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some hours of operation the KLF200 Gateway refuses new connections. If HA restarts, the connection build-up run into timeout during SSL Handshake. So if I start a reboot of the KLF200 before HA shutdown I can establish a new connection after HA is rebooted. Finall it's just a workaround. A disconnect function is considered in the destructor of pyvlx.connection, so I think it will not change much to remove this functionality on HA component side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes of course it would be much simpler for the user if we hardcode that, so they will not need to create an automation for that. Only point to consider is that KLF200 starts a wifi access point after each reboot where you can perform configurations. It's usually protected by a password if the user does not remove that. In the standard configuration this access point will be closed after some minutes. Finally my safety concerns are the reason not to hardcode this automation.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pawlizio : IMO all explicit disconnects are preferred bc then the disconnects happen in a controlled way. I guess als hass-shutdown process waits untill the disconnect is finished.

But yes, this buggy behavior of the KLF 200 gateway is annoying. There is another PR from you #42773 - where you implemented an explicit service to reboot the gateway from time to time. Isn't this the better approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

Only point to consider is that KLF200 starts a wifi access point after each reboot where you can perform configurations. [...] Finally my safety concerns are the reason not to hardcode this automation.

Valid point m-/ .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reboot service is already available in the component and yes you can start them periodically. But my preferred way was to initiate the reboot in parallel to a HA restart. Main concern was to reduce the amount of unnecessary KLF reboots and the corresponding opening of the Wifi Access Point to a bare minimum.

@pergolafabio
Copy link

i can confirm this PR is now working, my gateway is now rebooting on shutdown event with automation

@pawlizio pawlizio marked this pull request as draft March 14, 2021 17:05
@github-actions
Copy link

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.

@github-actions github-actions bot added the stale label Apr 13, 2021
@github-actions github-actions bot closed this Apr 20, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 21, 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.

SSL handshake error in Velux component

6 participants