Skip to content

Revert setting communication delay in Risco init#113497

Merged
joostlek merged 1 commit into
home-assistant:devfrom
OnFreund:risco_comm_delay
Mar 15, 2024
Merged

Revert setting communication delay in Risco init#113497
joostlek merged 1 commit into
home-assistant:devfrom
OnFreund:risco_comm_delay

Conversation

@OnFreund
Copy link
Copy Markdown
Contributor

Proposed change

Revert setting the communication delay during Risco init, which can prevent the integration from loading during boot, in a way that's hard to recover from.

More info:
#101349 introduced a communication delay between connecting to a Risco panel and sending the first command. This was needed for some panels to work. The communication delay was set either through the config flow, or during init.
The former was ok. The latter seemed ok at the time based on two assumptions:

  1. The conditions under which it would be set are rare.
  2. Even if it's set, it's harmless other than a small delay during start.

Both assumptions proved to be false:

  1. In order for this to happen, Risco init needs to fail at least once, but then succeed in one of 3 retries. This didn't seem like something that would happen often, but recent optimizations to HA boot has made it more probable. Risco needs to "cool off" after disconnection before it can accept a new connection. When HA restarts quickly, the first attempt can still be during the cool off time, while the last attempt would be after it.
  2. If the delay ends up being 3 seconds, and with HA initializing many integrations during boot, the 3 seconds can stretch longer, thus missing the required keep alive message (this is hypothesized, but is the most plausible explanation). The missed keep alive makes it extremely hard to disconnect gracefully, and in some cases restarting HA is the only way to recover.

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)
  • Deprecation (breaking change to happen in the future)
  • 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
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format 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.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@OnFreund
Copy link
Copy Markdown
Contributor Author

CC @FredericMa

@joostlek joostlek added this to the 2024.3.2 milestone Mar 15, 2024
@joostlek joostlek merged commit 99e29b7 into home-assistant:dev Mar 15, 2024
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Mar 15, 2024

2. If the delay ends up being 3 seconds, and with HA initializing many integrations during boot, the 3 seconds can stretch longer,

The event loop shouldn't get blocked for more than 100ms in 2024.4.x during startup anymore from python imports (or hopefully anything else). Previous version had event loop blocks > 10s when importing large integrations

@OnFreund OnFreund deleted the risco_comm_delay branch March 15, 2024 11:34
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 16, 2024
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.

RISCO integration error after upgrade core to 2024.2.4 or 2024.2.5

4 participants