Skip to content

Hand Fronius Modbus coordinators found by a re-scan to all platforms - #181402

Open
farmio wants to merge 5 commits into
home-assistant:devfrom
farmio:fronius-rescan-control-entities
Open

Hand Fronius Modbus coordinators found by a re-scan to all platforms#181402
farmio wants to merge 5 commits into
home-assistant:devfrom
farmio:fronius-rescan-control-entities

Conversation

@farmio

@farmio farmio commented Sep 5, 2026

Copy link
Copy Markdown
Member

Breaking change

Proposed change

An inverter that was asleep when the integration was set up, or one whose inverter control via Modbus was only enabled afterwards, gets its Modbus coordinator from the hourly re-scan instead of from setup. That path is reported to the platforms through SOLAR_NET_DISCOVERY_NEW, and two things went wrong with it.

The controls never appeared. Only the sensor platform listens to that dispatcher signal; number.py and switch.py enumerated the coordinators once in async_setup_entry. A settings coordinator that came up on a re-scan was created and polled, but produced no entities until Home Assistant was restarted. Both platforms now subscribe as well, ignoring the coordinators that are not theirs.

The sensor platform picked the wrong entity class. It tells the Modbus inverter coordinator from the SolarAPI one by the list it is in, but the dispatcher ran before the caller had appended it, so the test always failed and the re-scanned MPPT sensors were built as InverterSensor. They were given the SolarAPI unique_id format - 12345678-mppt_1_power_dc instead of 12345678-modbus-mppt_1_power_dc - which the added assertion in test_modbus_retried_after_setup shows directly. The coordinator is now kept in its list before anything is told about it, which is also what the new platform subscriptions rely on.

Retrying the two coordinators separately came along with it, because the fix does not stand without it. The re-scan guard only looked at the readings coordinators, so an inverter with an MPPT model never reached the settings path a second time - enabling inverter control via Modbus on the device needed a restart of Home Assistant - while a controls-only inverter satisfied the guard on every scan and piled up another settings coordinator every hour. Measured on the latter, three settings coordinators after two re-scans; with the platforms now listening, that pile turns into Platform fronius does not generate unique IDs errors, so it had to be fixed here rather than left to the follow-up.

The dispatcher reaches every platform, including those a coordinator has nothing for, where indexing its descriptions raised KeyError: <EntityPlatforms.SENSOR> on every late settings discovery. The dispatcher logs that rather than raising it, which is why it went unnoticed. Each platform now takes only the coordinators that carry its descriptions, which also replaces the list membership test the number and switch platforms started out with.

The discovery signal is now per config entry. It was one string for the whole integration, so with two Fronius devices configured each entry's platforms were handed the other entry's coordinators - building entities on the wrong entry, and in the sensor platform with the wrong class, because it tells the coordinators apart by lists belonging to one entry. That predates the Modbus support, but the new subscriptions would have inherited it. There is no test for it: two set up entries need two mocked hosts, and mock_responses clears the previous host's requests, while two entries on one host collide in the device registry. The re-scan tests cover that both ends of the signal agree.

Entities from 2026.9.0 are moved over. The Modbus sensors shipped in that release, so an affected installation carries entities with the SolarAPI-shaped unique_id, which the fixed platform would leave behind as a stale entity holding the entity ID and history. The two key sets do not overlap - checked against every description list in the integration - so a Modbus key without the -modbus- marker can only come from this bug. An entity whose place is already taken is left alone rather than raising.

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:
  • Link to developer documentation pull request:
  • Link to frontend pull request:

Checklist

  • I understand the code I am submitting and can explain how it works.
  • 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.
  • Any generated code has been carefully reviewed for correctness and compliance with project standards.

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 diff between library versions and ideally a link to the changelog/release notes is added to the PR description.

To help with the load of incoming pull requests:

An inverter that was asleep, or one whose Modbus control was enabled
after the integration was set up, gets its coordinator from the hourly
re-scan. That reaches the platforms through SOLAR_NET_DISCOVERY_NEW,
which only the sensor platform listens to - the number and switch
platforms enumerated the coordinators once at setup, so the controls
showed up only after Home Assistant was restarted.

The sensor platform also built the wrong entity class for such a
coordinator. It tells the two Modbus coordinators apart by the list
they are in, and the dispatcher ran before the caller had put it there,
so re-scanned MPPT sensors were given the unique_id of the SolarAPI
inverter sensors.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Late settings discovery can raise in the sensor subscriber, coordinator retry logic remains incorrect, and existing incorrect unique IDs are not migrated.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes late discovery of Fronius Modbus entities.

Changes:

  • Registers number and switch platforms for discovery events.
  • Classifies newly discovered Modbus sensors correctly.
  • Adds late-discovery regression tests.
File summaries
File Description
homeassistant/components/fronius/__init__.py Tracks coordinators before dispatching discovery.
homeassistant/components/fronius/number.py Adds late-discovered number entities.
homeassistant/components/fronius/switch.py Adds late-discovered switch entities.
tests/components/fronius/test_modbus.py Tests late Modbus entity discovery.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 3
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread homeassistant/components/fronius/__init__.py
Comment thread homeassistant/components/fronius/__init__.py
Comment thread tests/components/fronius/test_modbus.py
farmio and others added 2 commits September 5, 2026 22:58
Both are set up from the same re-scan, but the guard only looked at the
readings coordinators: an inverter with an MPPT model never reached the
settings path again, so enabling inverter control on the device needed a
restart of Home Assistant - while a controls-only inverter satisfied the
guard on every scan and piled up another settings coordinator each hour.
With the platforms now listening, that pile turned into "does not
generate unique IDs" errors.

The dispatcher reaches every platform, including the ones a coordinator
has nothing for, where indexing its descriptions raised a KeyError. Each
platform now takes only the coordinators that carry its descriptions.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Those entities carry `<inverter>-<key>` instead of
`<inverter>-modbus-<key>`, so the fixed platform would register the
entity again and leave the one holding the history behind. The key sets
of the two do not overlap, so a Modbus key without the marker can only
come from that, and an entity whose place is already taken is left
alone.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 5, 2026 21:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The global discovery signal can route coordinators through the wrong config entry’s platform callbacks.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread homeassistant/components/fronius/number.py
Comment thread homeassistant/components/fronius/sensor.py
Comment thread homeassistant/components/fronius/switch.py
The signal for coordinators found after setup was one string for the
whole integration, so with two Fronius devices configured each entry's
platforms were handed the other's coordinators - building entities on
the wrong entry, and in the sensor platform with the wrong class,
because it tells the coordinators apart by lists that belong to one
entry.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 5, 2026 21:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The controls-retry test does not exercise the reported MPPT-present regression path.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

tests/components/fronius/test_modbus.py:487

  • Exercise the MPPT-present path in this retry test. With include_mppt_model=False, the readings-coordinator list remains empty, so the previous readings-only guard would still retry settings and this test would not catch the reported regression where an existing MPPT coordinator prevents controls enabled later from being discovered.
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Without an MPPT model the readings list stays empty, so the old guard
would have retried the controls anyway - the case it actually blocked
is an inverter that has both.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 6, 2026 06:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The cross-entry discovery isolation fix lacks a regression test covering multiple config entries.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

homeassistant/components/fronius/const.py:24

  • Add a two-config-entry regression test for discovery isolation. The new re-scan tests use only one entry, so they would still pass if the sender and all platform listeners accidentally reverted to one shared signal; the cross-entry entity leak this helper fixes is therefore unprotected.
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants