Pick up SolarEdge Modbus meters and batteries wired in or out later (#180830)

homeassistant/components/solaredge_modbus/__init__.py

@@ -7,8 +7,11 @@ the ``solaredged`` library.
 """
 
 from collections.abc import Set as AbstractSet
+from datetime import datetime
+from functools import partial
 from typing import TYPE_CHECKING
 
+from modbus_connection import ModbusUnit
 from solaredged import SolarEdge, SolarEdgeConnectionError, SolarEdgeError
 
 from homeassistant.components.modbus import async_get_unit
@@ -20,8 +23,10 @@ from homeassistant.exceptions import (
     HomeAssistantError,
 )
 from homeassistant.helpers import device_registry as dr
+from homeassistant.helpers.event import async_track_time_interval
 
 from .const import (
+    ATTACHMENT_SCAN_INTERVAL,
     CONF_UNIT_ID,
     DOMAIN,
     LOGGER,
@@ -140,6 +145,7 @@ async def async_setup_entry(
         settings=settings,
         device_info=device_info,
         inverter_device_id=inverter.id,
+        attachments=_attachment_identities(solaredge),
     )
 
     if silent := solaredge.unresponsive_blocks & {
@@ -157,9 +163,85 @@ async def async_setup_entry(
 
     await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS)
 
+    # What is wired to the inverter is read while setting up, so a meter or
+    # battery added or removed later needs the entry to load again to be seen.
+    entry.async_on_unload(
+        async_track_time_interval(
+            hass,
+            partial(_async_reload_when_attachments_change, hass, entry, unit),
+            ATTACHMENT_SCAN_INTERVAL,
+        )
+    )
+
     return True
 
 
+def _attachment_identities(solaredge: SolarEdge) -> frozenset[str]:
+    """Return what the meters and batteries attached right now are known by."""
+    return frozenset(
+        [
+            *(
+                f"meter_{attachment_identity(meter, index)}"
+                for index, meter in enumerate(solaredge.meters, 1)
+            ),
+            *(
+                f"battery_{attachment_identity(battery, index)}"
+                for index, battery in enumerate(solaredge.batteries, 1)
+            ),
+        ]
+    )
+
+
+async def _async_reload_when_attachments_change(
+    hass: HomeAssistant,
+    entry: SolarEdgeModbusConfigEntry,
+    unit: ModbusUnit,
+    _now: datetime,
+) -> None:
+    """Reload the entry when the hardware wired to the inverter changed."""
+    solaredge = entry.runtime_data.solaredge
+
+    # Swapping one meter for another leaves the count alone, but the polls have
+    # been reading the new one's serial number since it was wired in.
+    if _attachment_identities(solaredge) != entry.runtime_data.attachments:
+        LOGGER.info(
+            "%s: what is attached changed, reloading to pick that up",
+            entry.title,
+        )
+        hass.config_entries.async_schedule_reload(entry.entry_id)
+        return
+
+    try:
+        probed = await SolarEdge.async_probe(unit)
+    except SolarEdgeError as err:
+        # Nothing to conclude from a probe that did not finish; the coordinators
+        # report an inverter that stopped answering.
+        LOGGER.debug("%s: could not probe for attached hardware: %s", entry.title, err)
+        return
+
+    for name, found, known in (
+        (SUBSYSTEM_METERS, len(probed.meters), len(solaredge.meters)),
+        (SUBSYSTEM_BATTERIES, len(probed.batteries), len(solaredge.batteries)),
+    ):
+        if found == known:
+            continue
+        # A block that stayed silent is taken for absent, which is not the same
+        # as the inverter saying it is gone, and reloading on that would drop a
+        # device over one timeout.
+        if found < known and name in probed.unresponsive_blocks:
+            continue
+
+        LOGGER.info(
+            "%s: %s went from %s to %s, reloading to pick that up",
+            entry.title,
+            name,
+            known,
+            found,
+        )
+        hass.config_entries.async_schedule_reload(entry.entry_id)
+        return
+
+
 def _async_remove_stale_devices(
     hass: HomeAssistant,
     entry: SolarEdgeModbusConfigEntry,

homeassistant/components/solaredge_modbus/const.py

@@ -39,3 +39,7 @@ SCAN_INTERVAL: Final = timedelta(seconds=10)
 # The control blocks hold what the site was told to do; they only move when
 # something writes them, so they do not need a live measurement's cadence.
 SETTINGS_SCAN_INTERVAL: Final = timedelta(minutes=5)
+
+# Meters and batteries are wired to an inverter by hand, usually with the power
+# off, so looking for a change now and then is often enough.
+ATTACHMENT_SCAN_INTERVAL: Final = timedelta(minutes=15)

homeassistant/components/solaredge_modbus/coordinator.py

@@ -169,6 +169,9 @@ class SolarEdgeModbusRuntimeData:
     settings: SolarEdgeModbusDataUpdateCoordinator
     device_info: DeviceInfo
     inverter_device_id: str
+    # What was attached when this entry was built, to notice a swap: a meter
+    # replaced by another one leaves the count alone.
+    attachments: frozenset[str]
 
     # The export mode and its flags share one register, which the library
     # changes by taking its cached value, flipping bits and writing it back.

homeassistant/components/solaredge_modbus/quality_scale.yaml

@@ -61,7 +61,7 @@ rules:
   docs-supported-functions: todo
   docs-troubleshooting: todo
   docs-use-cases: todo
-  dynamic-devices: todo
+  dynamic-devices: done
   entity-category: done
   entity-device-class: done
   entity-disabled-by-default: done
@@ -72,7 +72,7 @@ rules:
   repair-issues:
     status: exempt
     comment: No repairable issues are raised.
-  stale-devices: todo
+  stale-devices: done
 
   # Platinum
   async-dependency: done

tests/components/solaredge_modbus/test_init.py

@@ -7,11 +7,12 @@ from freezegun.api import FrozenDateTimeFactory
 from modbus_connection import (
     IllegalDataAddressError,
     ModbusTimeoutError,
+    ModbusUnit,
     ServerDeviceFailureError,
 )
 from modbus_connection.mock import MockModbusConnection, MockModbusUnit
 import pytest
-from solaredged import SolarEdgeConnectionError
+from solaredged import SolarEdge, SolarEdgeConnectionError
 
 from homeassistant.components.select import (
     ATTR_OPTION,
@@ -19,6 +20,7 @@ from homeassistant.components.select import (
     SERVICE_SELECT_OPTION,
 )
 from homeassistant.components.solaredge_modbus.const import (
+    ATTACHMENT_SCAN_INTERVAL,
     DOMAIN,
     SCAN_INTERVAL,
     SETTINGS_SCAN_INTERVAL,
@@ -58,6 +60,9 @@ METER_MODEL_REGISTER = 40188
 # An address inside the pooled storage and export control read.
 SITE_CONTROL_REGISTER = 57348
 
+# Where the first meter's serial number lives.
+METER_SERIAL_REGISTER = 40171
+
 EXPORT_LIMITATION_ENTITY = "select.solaredge_se10000h_export_limitation"
 EXTERNAL_PRODUCTION_ENTITY = "switch.solaredge_se10000h_external_production"
 
@@ -620,6 +625,205 @@ async def test_concurrent_control_writes_keep_both_changes(
     assert state.state == STATE_ON
 
 
+async def _tick_attachment_check(
+    hass: HomeAssistant, freezer: FrozenDateTimeFactory
+) -> int:
+    """Let the check for changed hardware run, and report what it cost.
+
+    Setting up probes the device, so a check that reloads the entry probes
+    twice: once to look, once to build the entry again.
+    """
+    probes = 0
+    probe = SolarEdge.async_probe
+
+    async def counting_probe(unit: ModbusUnit) -> SolarEdge:
+        nonlocal probes
+        probes += 1
+        return await probe(unit)
+
+    with patch.object(SolarEdge, "async_probe", counting_probe):
+        freezer.tick(ATTACHMENT_SCAN_INTERVAL)
+        async_fire_time_changed(hass)
+        await hass.async_block_till_done()
+
+    return probes
+
+
+async def test_meter_added_later_is_picked_up(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+    device_registry: dr.DeviceRegistry,
+    mock_config_entry: MockConfigEntry,
+    mock_modbus_unit: MockModbusUnit,
+) -> None:
+    """A meter wired to a running installation appears without being asked.
+
+    What is attached is read while the entry is set up, so the entry loads
+    again once a probe finds something that was not there before.
+    """
+    mock_modbus_unit.fail_read(METER_MODEL_REGISTER, IllegalDataAddressError())
+    await _setup(hass, mock_config_entry)
+
+    meter = (DOMAIN, f"{SERIAL_NUMBER}_meter_{METER_SERIAL_NUMBER}")
+    assert (
+        device_registry.async_get_device_by_identifier(
+            meter, mock_config_entry.entry_id
+        )
+        is None
+    )
+
+    # The meter is wired in and answers from now on.
+    mock_modbus_unit.fail_read(METER_MODEL_REGISTER, None)
+
+    await _tick_attachment_check(hass, freezer)
+
+    assert (
+        device_registry.async_get_device_by_identifier(
+            meter, mock_config_entry.entry_id
+        )
+        is not None
+    )
+
+
+async def test_battery_removed_later_is_dropped(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+    device_registry: dr.DeviceRegistry,
+    mock_config_entry: MockConfigEntry,
+    mock_modbus_unit: MockModbusUnit,
+) -> None:
+    """A battery taken out of a running installation stops being a device."""
+    await _setup(hass, mock_config_entry)
+
+    battery = (DOMAIN, f"{SERIAL_NUMBER}_battery_{BATTERY_SERIAL_NUMBERS[0]}")
+    assert (
+        device_registry.async_get_device_by_identifier(
+            battery, mock_config_entry.entry_id
+        )
+        is not None
+    )
+
+    mock_modbus_unit.fail_read(BATTERY_RATED_ENERGY, IllegalDataAddressError())
+
+    await _tick_attachment_check(hass, freezer)
+
+    assert (
+        device_registry.async_get_device_by_identifier(
+            battery, mock_config_entry.entry_id
+        )
+        is None
+    )
+
+
+async def test_replaced_meter_is_picked_up(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+    device_registry: dr.DeviceRegistry,
+    mock_config_entry: MockConfigEntry,
+    mock_modbus_unit: MockModbusUnit,
+) -> None:
+    """Another meter in the same place is another device, while running too.
+
+    Swapping one meter for another leaves the count alone, so what gives it
+    away is the serial number the polls have been reading all along.
+    """
+    await _setup(hass, mock_config_entry)
+
+    replacement = "7E9C55A6"
+    padded = replacement.ljust(32, "\0").encode()
+    mock_modbus_unit.holding.update(
+        {
+            METER_SERIAL_REGISTER + index: (padded[index * 2] << 8)
+            | padded[index * 2 + 1]
+            for index in range(16)
+        }
+    )
+
+    # The swap is seen without probing, so the only probe here is the reload's.
+    assert await _tick_attachment_check(hass, freezer) == 1
+
+    assert (
+        device_registry.async_get_device_by_identifier(
+            (DOMAIN, f"{SERIAL_NUMBER}_meter_{METER_SERIAL_NUMBER}"),
+            mock_config_entry.entry_id,
+        )
+        is None
+    )
+    assert (
+        device_registry.async_get_device_by_identifier(
+            (DOMAIN, f"{SERIAL_NUMBER}_meter_{replacement}"),
+            mock_config_entry.entry_id,
+        )
+        is not None
+    )
+
+
+async def test_silent_attachment_does_not_trigger_a_reload(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+    device_registry: dr.DeviceRegistry,
+    mock_config_entry: MockConfigEntry,
+    mock_modbus_unit: MockModbusUnit,
+) -> None:
+    """A meter that did not answer the probe is not a meter that was removed.
+
+    Silence is taken for absence while probing, so reloading on it would drop a
+    device, and its history, over a single timeout.
+    """
+    await _setup(hass, mock_config_entry)
+
+    meter = (DOMAIN, f"{SERIAL_NUMBER}_meter_{METER_SERIAL_NUMBER}")
+    mock_modbus_unit.fail_read(METER_MODEL_REGISTER, ModbusTimeoutError("timed out"))
+
+    assert await _tick_attachment_check(hass, freezer) == 1
+
+    assert (
+        device_registry.async_get_device_by_identifier(
+            meter, mock_config_entry.entry_id
+        )
+        is not None
+    )
+
+
+async def test_unchanged_attachments_leave_the_entry_alone(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+    mock_config_entry: MockConfigEntry,
+) -> None:
+    """Nothing changed means nothing happens, however often it is checked."""
+    await _setup(hass, mock_config_entry)
+
+    coordinator = mock_config_entry.runtime_data.readings
+
+    assert await _tick_attachment_check(hass, freezer) == 1
+
+    assert mock_config_entry.state is ConfigEntryState.LOADED
+    # A reload would have built new coordinators.
+    assert mock_config_entry.runtime_data.readings is coordinator
+
+
+async def test_a_dead_probe_leaves_the_entry_where_it_is(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+    mock_config_entry: MockConfigEntry,
+    mock_modbus_unit: MockModbusUnit,
+) -> None:
+    """An inverter that stops answering says nothing about what is wired to it.
+
+    The coordinators already report an inverter gone quiet; reloading on top of
+    that would only take the entry down with it.
+    """
+    await _setup(hass, mock_config_entry)
+
+    coordinator = mock_config_entry.runtime_data.readings
+    mock_modbus_unit.fail_requests(ModbusTimeoutError("link died"))
+
+    assert await _tick_attachment_check(hass, freezer) == 1
+
+    assert mock_config_entry.state is ConfigEntryState.LOADED
+    assert mock_config_entry.runtime_data.readings is coordinator
+
+
 async def test_setup_retry_when_device_unresponsive(
     hass: HomeAssistant,
     mock_config_entry: MockConfigEntry,