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

Dynamically add Airzone entities #121891

Merged
merged 13 commits into from
Jul 13, 2024
Merged

Conversation

Noltari
Copy link
Contributor

@Noltari Noltari commented Jul 13, 2024

Proposed change

Reload Airzone config entry on new devices in order to expose new devices and entities.

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

  • 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
  • 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.

To help with the load of incoming pull requests:

Signed-off-by: Álvaro Fernández Rojas <[email protected]>
@joostlek
Copy link
Member

Why don't we dynamically add devices?

@Noltari
Copy link
Contributor Author

Noltari commented Jul 13, 2024

Why don't we dynamically add devices?

Honestly because I don't know how to do it...
Could you point me to any integration doing that?

@joostlek
Copy link
Member

I recently implemented this in Mealie todo list. Also knocki's event has this.

@Noltari Noltari changed the title Reload Airzone entry on new devices Dynamically detect Airzone sensors entities Jul 13, 2024
@Noltari
Copy link
Contributor Author

Noltari commented Jul 13, 2024

I recently implemented this in Mealie todo list. Also knocki's event has this.

Thanks @joostlek,
I assume that I need to create separate PRs for each platform, am I correct?
BTW, I reused this one for the sensors platform.

@joostlek
Copy link
Member

I dont exactly know how much work goes into one of them, but i dont think it would be that big of a deal to put them into one

@Noltari Noltari marked this pull request as draft July 13, 2024 15:44
Noltari added 5 commits July 13, 2024 17:47
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
@Noltari Noltari changed the title Dynamically detect Airzone sensors entities Dynamically add Airzone entities Jul 13, 2024
@Noltari Noltari marked this pull request as ready for review July 13, 2024 16:03
@Noltari
Copy link
Contributor Author

Noltari commented Jul 13, 2024

I dont exactly know how much work goes into one of them, but i dont think it would be that big of a deal to put them into one

@joostlek many thanks for your help :)

Let me know if it's too much for a single PR and I will split it into separate PRs.

Comment on lines 95 to 113
async_add_entities(
AirzoneSystemBinarySensor(
coordinator,
description,
entry,
system_zone_id,
systems_data.get(system_zone_id),
)
for system_zone_id in new_systems
for description in SYSTEM_BINARY_SENSOR_TYPES
if description.key in systems_data.get(system_zone_id)
)
added_systems.update(new_systems)

zones_data = coordinator.data.get(AZD_ZONES, {})
received_zones = set(zones_data)
new_zones = received_zones - added_zones
if new_zones:
async_add_entities(
Copy link
Member

Choose a reason for hiding this comment

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

Can we combine the call? Async_add_entities is quite expensive iirc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f9c4dbb

assert state is None

mock_hvac.return_value = HVAC_MOCK
async_fire_time_changed(hass, utcnow() + SCAN_INTERVAL)
Copy link
Member

Choose a reason for hiding this comment

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

Please use the freezer as well

Copy link
Contributor Author

@Noltari Noltari Jul 13, 2024

Choose a reason for hiding this comment

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

Done in 4b66f35

@home-assistant home-assistant bot marked this pull request as draft July 13, 2024 16:15
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

Signed-off-by: Álvaro Fernández Rojas <[email protected]>
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
@Noltari Noltari marked this pull request as ready for review July 13, 2024 18:21
@home-assistant home-assistant bot requested a review from joostlek July 13, 2024 18:21
Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

Oh and please wrap all the coordinator.add_listeners in entry.async_on_unload to avoid having listeners alive after the integration is unloaded

entry,
added_hotwater = True

if not added_webserver and AZD_WEBSERVER in coordinator.data:
Copy link
Member

Choose a reason for hiding this comment

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

Can this be added on runtime? Or is this something that is there by default?

You could also make separate listeners for these and if the webserver is already set up, we don't attach the listener

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually you're right, so on a second thought I've removed the webserver and water heater from the listeners since those can't be dynamically added.

Noltari added 2 commits July 13, 2024 21:31
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
@Noltari Noltari requested a review from joostlek July 13, 2024 19:43
Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

You can also look at dynamically deleting them in the future to keep HA in sync with the device and improve the user experience even more :)

@joostlek joostlek merged commit c044417 into home-assistant:dev Jul 13, 2024
25 checks passed
@Noltari Noltari deleted the airzone-new-devices branch July 13, 2024 21:24
Noltari added a commit to Noltari/home-assistant-core that referenced this pull request Jul 14, 2024
Commit c044417 incorrecly renamed system_id to system_zone_id.

Fixes: c044417 ("Dynamically add Airzone entities (home-assistant#121891)")
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Jul 14, 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.

2 participants