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

Split quirks loading from network and app initialization #121

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

dmulcahey
Copy link
Contributor

@dmulcahey dmulcahey commented Aug 1, 2024

This PR moves the loading of quirks so that we can split the quirk loading from the initialization of the network and the application. There are a couple HA users that hit an issue in the beta where device triggers from quirks aren't available early enough and if the integration fails to set up on the first shot (flaky stick, poor nwk conditions etc.) it prevents automations from finding triggers for ZHA devices:

Logger: homeassistant.components.automation.drukknop_slaapkamer_noor
Source: components/automation/__init__.py:842
integration: Automation (documentation, issues)
First occurred: 02:53:39 (1 occurrences)
Last logged: 02:53:39

Got error 'Unable to find trigger ('remote_button_short_press', 'remote_button_short_press')' when setting up triggers for Drukknop slaapkamer Noor

There is a corresponding HA PR: home-assistant/core#123027

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.65%. Comparing base (927d07e) to head (95e176d).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #121      +/-   ##
==========================================
- Coverage   95.65%   95.65%   -0.01%     
==========================================
  Files          61       61              
  Lines        9255     9254       -1     
==========================================
- Hits         8853     8852       -1     
  Misses        402      402              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dmulcahey dmulcahey marked this pull request as ready for review August 1, 2024 16:47
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.

Why is this part pf the gateway code at all? Quirks are global.

@puddly
Copy link
Contributor

puddly commented Aug 1, 2024

@elupus Custom quirks are reloaded from disk. This is also sort of a remnant of ZHA in Core.

@elupus
Copy link
Contributor

elupus commented Aug 1, 2024

Sure, but it does not need to be done inside the gateway class. Could be done outside of that by the caller constructor of the gateway object.

@elupus
Copy link
Contributor

elupus commented Aug 1, 2024

What i mean is, if we are changing it, it is better to remove it from gateway and make it a caller responsibility. Makes testing easier.

@dmulcahey
Copy link
Contributor Author

dmulcahey commented Aug 1, 2024

I don't have a strong opinion either way. My initial view was that the gateway should essentially function as a standalone application. Take HA out of the picture and you should be able to take the gateway and just use it. I already have plans to eventually have an add-on and we are also used by another automation platform (they use all the libs and hand rolled what is essentially this lib now... hopefully the convert too). Initial thinking was anything that would be common across all would be done in 1 spot 🤷🏻‍♂️

@puddly
Copy link
Contributor

puddly commented Aug 1, 2024

Could be done outside of that by the caller constructor of the gateway object.

Quirk loading is also somewhat tied to the lifecycle of Gateway: quirks should always be loaded before Gateway is constructed and preferably re-loaded before Gateway is re-initialized a second time. Quirks loading is pretty much a part of startup and I think it should be hidden from upstream application so I think it would be preferable to keep it a part of Gateway.

@puddly puddly merged commit c043f62 into dev Aug 1, 2024
6 checks passed
@dmulcahey dmulcahey deleted the dm/load-quirks-earlier branch August 18, 2024 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants