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.

Sign up for GitHub

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

Jump to bottom

Allow multiple Airzone entries with different System IDs #135397

Merged
merged 11 commits into from
Jan 22, 2025
Merged

Allow multiple Airzone entries with different System IDs #135397

merged 11 commits into from
Jan 22, 2025

Conversation

Noltari
Copy link
Contributor

@Noltari Noltari commented Jan 11, 2025 •
edited
Loading

Proposed change

System ID 0 isn't supported on some older Airzone devices and we need to allow creating multiple config entries with the same IP address and port, but different System IDs.

For reference: devices that support System ID 0 allow listing all systems on one API call. However, on devices where System ID 0 isn't supported, the API will only return the data of 1 system at a time and there's no way of knowing which System IDs are valid, so user input is used.

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.

To help with the load of incoming pull requests:

@Noltari Noltari changed the title airzone: allow multiple config entries with different System IDs Allow multiple Airzone entries with different System IDs Jan 11, 2025
@Noltari
tests: airzone: drop id from diagnostics
6e3c6c3 
Not sure why this fails on Github CI and is needed locally...

Signed-off-by: Álvaro Fernández Rojas <[email protected]>
@Noltari
tests: airzone: CONFIG vs USER_INPUT
1203833 
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
@Noltari
airzone: migrate config entries to v2
11db4b6 
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
@Noltari
Merge remote-tracking branch 'upstream/dev' into airzone-system-0-not…
bb96ce8 
…-supported-fixes
@abmantis
Copy link
Contributor

I wonder if we could update the integration to allow the user to specify multiple system IDs and have the integration make multiple API requests (one for each ID), instead of having separate configs?

joostlek
joostlek requested changes Jan 14, 2025
View reviewed changes
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.

(whoops, didn't submit my review)

homeassistant/components/airzone/__init__.py Outdated
Comment on lines 127 to 137
if entry.version == 1:
# Add missing CONF_ID
system_id = entry.data.get(CONF_ID, DEFAULT_SYSTEM_ID)
new_data = entry.data.copy()
new_data[CONF_ID] = system_id
hass.config_entries.async_update_entry(
entry,
data=new_data,
version=2,
)

Copy link
Member

Choose a reason for hiding this comment

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

Let's only do a minor version migration because the new config entry is still able to work with the old one. This way the user can restore to a previous version

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 876dbf9
Thanks for the info, I hadn't realized we had that functionality!

@home-assistant home-assistant bot marked this pull request as draft January 14, 2025 16:29
@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.

@joostlek
Copy link
Member

You mean currently it's 1 system id == 1 config entry, and you want to move it to what?

@Noltari
Copy link
Contributor Author

Noltari commented Jan 14, 2025

I wonder if we could update the integration to allow the user to specify multiple system IDs and have the integration make multiple API requests (one for each ID), instead of having separate configs?

We could do that, but that would require more work on both the integration and the library, and honestly I don't think it's worth it since the issue I'm trying to fix here only affects a limited number of legacy devices.

@Noltari
airzone: switch to minor_version
876dbf9 
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
@Noltari Noltari marked this pull request as ready for review January 14, 2025 18:19
@home-assistant home-assistant bot requested a review from joostlek January 14, 2025 18:19
@Noltari
tests: airzone: config_flow: minimize changes
49cac23 
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
@Noltari
tests: airzone: undo changes
4daeff3 
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
joostlek
joostlek approved these changes Jan 17, 2025
View reviewed changes
@joostlek
Copy link
Member

Oh that is a good question, how many users are we talking?

@Noltari
Copy link
Contributor Author

Noltari commented Jan 17, 2025

Oh that is a good question, how many users are we talking?

I'd say is very limited, because in order to have such cases I believe that two independent HVAC systems should be connected to the same Airzone WebServer, and that WebServer should be old enough to not allow systemID = 0...
And honestly I don't think that many houses have two independent HVAC systems connected to the same WebServer...

@Noltari
Copy link
Contributor Author

Noltari commented Jan 22, 2025

@joostlek codecov/project failure isn't related to changes.

@abmantis abmantis added the ci-full-run Marks a PR to trigger a full CI run (instead of partial) label Jan 22, 2025
@abmantis abmantis closed this Jan 22, 2025
@abmantis abmantis reopened this Jan 22, 2025
@abmantis abmantis removed the ci-full-run Marks a PR to trigger a full CI run (instead of partial) label Jan 22, 2025
abmantis
abmantis requested changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@abmantis abmantis left a comment

Choose a reason for hiding this comment

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

We should also add the system ID on the entry title on config_flow.py:90 (but only for ID != 0)

@home-assistant home-assistant bot marked this pull request as draft January 22, 2025 15:59
@Noltari
airzone: add System ID to config title
55ec0f6 
Signed-off-by: Álvaro Fernández Rojas <[email protected]>
@Noltari
Copy link
Contributor Author

Noltari commented Jan 22, 2025

We should also add the system ID on the entry title on config_flow.py:90 (but only for ID != 0)

Done in 55ec0f6

@Noltari Noltari marked this pull request as ready for review January 22, 2025 17:35
@home-assistant home-assistant bot requested review from joostlek and abmantis January 22, 2025 17:35
abmantis
abmantis approved these changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@abmantis abmantis left a comment

Choose a reason for hiding this comment

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

Thanks!

@abmantis abmantis merged commit 4e494aa into home-assistant:dev Jan 22, 2025
32 checks passed
@Noltari Noltari deleted the airzone-system-0-not-supported-fixes branch January 22, 2025 18:13
@github-actions github-actions bot locked and limited conversation to collaborators Jan 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@abmantis abmantis abmantis approved these changes

@joostlek joostlek Awaiting requested review from joostlek

Assignees
No one assigned
Labels
bugfix by-code-owner cla-signed has-tests integration: airzone Quality Scale: No score small-pr PRs with less than 30 lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Airzone Integration - Two systems IDs with one webserver IP
3 participants
@Noltari @abmantis @joostlek