Stop asking a SolarEdge inverter about blocks it never answers (#184290)

homeassistant/components/solaredge_modbus/__init__.py

@@ -33,6 +33,7 @@ from homeassistant.helpers.event import async_track_time_interval
 from .const import (
     ATTACHMENT_SCAN_INTERVAL,
     CONF_UNIT_ID,
+    DISCOVERY_SUBSYSTEMS,
     DOMAIN,
     LOGGER,
     SCAN_INTERVAL,
@@ -237,13 +238,24 @@ async def _async_reload_when_attachments_change(
         return
 
     try:
-        probed = await SolarEdge.async_probe(unit)
+        probed = await SolarEdge.async_probe(
+            unit, assume_absent=entry.runtime_data.settled_silent_blocks
+        )
     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
 
+    # Silent while setting up and silent again here. A block that answered at
+    # setup and merely blipped now is not settled, or one timeout would hide a
+    # later removal until the entry loads again. Blocks that finding hardware
+    # depends on are never settled: picking up what was wired in later is what
+    # asking is for.
+    entry.runtime_data.settled_silent_blocks = (
+        probed.unresponsive_blocks & solaredge.unresponsive_blocks
+    ) - DISCOVERY_SUBSYSTEMS
+
     known = _probed_blocks(solaredge)
     for name, found in _probed_blocks(probed).items():
         if found == known[name]:

homeassistant/components/solaredge_modbus/const.py

@@ -29,9 +29,18 @@ SUBSYSTEM_BATTERIES: Final = "batteries"
 SUBSYSTEM_EXPORT_CONTROL: Final = "export_control"
 SUBSYSTEM_GRID_STATUS: Final = "grid_status"
 SUBSYSTEM_METERS: Final = "meters"
+SUBSYSTEM_MMPPT: Final = "mmppt"
 SUBSYSTEM_STORAGE_CAPACITY: Final = "storage_capacity"
 SUBSYSTEM_STORAGE_CONTROL: Final = "storage_control"
 
+# The blocks that finding hardware depends on, so they are worth a timeout
+# however long they stay quiet. Meters and batteries each bring a device, and
+# the multiple-MPPT block decides the offset the meters are looked for at:
+# taking it for absent would look for them at the wrong addresses for good.
+DISCOVERY_SUBSYSTEMS: Final = frozenset(
+    {SUBSYSTEM_BATTERIES, SUBSYSTEM_METERS, SUBSYSTEM_MMPPT}
+)
+
 # The writable control blocks, as an UpdateReport names them. Export control's
 # read spans storage control, so the library reads and reports the two as one.
 SUBSYSTEM_ADVANCED_POWER_CONTROL: Final = "advanced_power_control"

homeassistant/components/solaredge_modbus/coordinator.py

@@ -168,10 +168,18 @@ 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]
 
+    # Blocks that answered nothing while setting up and nothing again on the
+    # first check after it. Asking costs a full timeout each, which on a shared
+    # link is time every other inverter spends waiting, so they are taken for
+    # absent until the entry loads again. Empty until that first check, so a
+    # block that was merely busy at setup still gets picked up.
+    settled_silent_blocks: frozenset[str] = frozenset()
+
     @property
     def solaredge(self) -> SolarEdge:
         """Return the polled device, which both coordinators share."""

homeassistant/components/solaredge_modbus/manifest.json

@@ -9,6 +9,6 @@
   "iot_class": "local_polling",
   "loggers": ["modbus_connection", "solaredged", "tmodbus"],
   "quality_scale": "platinum",
-  "requirements": ["solaredged==0.4.0"],
+  "requirements": ["solaredged==0.5.0"],
   "zeroconf": ["_solaredge-modbus._tcp.local."]
 }

requirements_all.txt

@@ -3124,7 +3124,7 @@ sofar-modbus==0.17.0
 solaredge-web==0.4.0
 
 # homeassistant.components.solaredge_modbus
-solaredged==0.4.0
+solaredged==0.5.0
 
 # homeassistant.components.solarlog
 solarlog_cli==0.7.1

tests/components/solaredge_modbus/test_init.py

@@ -57,6 +57,9 @@ INVERTER_REGISTER = 40069
 # The register the probe counts meters by.
 METER_MODEL_REGISTER = 40188
 
+# The identifier the multiple-MPPT probe reads, which sets the meter offset.
+MMPPT_REGISTER = 40121
+
 # An address inside the pooled storage and export control read.
 SITE_CONTROL_REGISTER = 57348
 
@@ -643,10 +646,12 @@ async def _tick_attachment_check(
     probes = 0
     probe = SolarEdge.async_probe
 
-    async def counting_probe(unit: ModbusUnit) -> SolarEdge:
+    async def counting_probe(
+        unit: ModbusUnit, *, assume_absent: frozenset[str] = frozenset()
+    ) -> SolarEdge:
         nonlocal probes
         probes += 1
-        return await probe(unit)
+        return await probe(unit, assume_absent=assume_absent)
 
     with patch.object(SolarEdge, "async_probe", counting_probe):
         freezer.tick(ATTACHMENT_SCAN_INTERVAL)
@@ -1042,3 +1047,117 @@ async def test_setup_error_when_link_settings_are_in_use(
         await _setup(hass, mock_config_entry)
 
     assert mock_config_entry.state is ConfigEntryState.SETUP_ERROR
+
+
+async def test_a_block_that_stays_silent_stops_being_asked(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+    mock_config_entry: MockConfigEntry,
+    mock_modbus_unit: MockModbusUnit,
+) -> None:
+    """A block silent at setup is looked for once more, then left alone.
+
+    Asking costs a full timeout each time, and the link is shared, so that is
+    time every other inverter on it spends queued behind the question.
+    """
+    mock_modbus_unit.fail_read(POWER_CONTROL_REGISTER, ModbusTimeoutError("timed out"))
+    await _setup(hass, mock_config_entry)
+
+    asked: list[frozenset[str]] = []
+    probe = SolarEdge.async_probe
+
+    async def recording_probe(
+        unit: ModbusUnit, *, assume_absent: frozenset[str] = frozenset()
+    ) -> SolarEdge:
+        asked.append(assume_absent)
+        return await probe(unit, assume_absent=assume_absent)
+
+    with patch.object(SolarEdge, "async_probe", recording_probe):
+        for _ in range(2):
+            freezer.tick(ATTACHMENT_SCAN_INTERVAL)
+            async_fire_time_changed(hass)
+            await hass.async_block_till_done()
+
+    assert asked[0] == frozenset(), "the first check still looks for it"
+    assert "power_control" in asked[1], "a block still silent is not asked again"
+
+
+async def test_a_block_that_blips_once_keeps_being_asked(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+    mock_config_entry: MockConfigEntry,
+    mock_modbus_unit: MockModbusUnit,
+) -> None:
+    """A block that answered at setup is not settled by one timeout later.
+
+    Settling it would hide the block going away for good, since a block taken
+    for absent is reported back as though it had been silent again.
+    """
+    await _setup(hass, mock_config_entry)
+
+    asked: list[frozenset[str]] = []
+    probe = SolarEdge.async_probe
+
+    async def recording_probe(
+        unit: ModbusUnit, *, assume_absent: frozenset[str] = frozenset()
+    ) -> SolarEdge:
+        asked.append(assume_absent)
+        return await probe(unit, assume_absent=assume_absent)
+
+    with patch.object(SolarEdge, "async_probe", recording_probe):
+        mock_modbus_unit.fail_read(
+            POWER_CONTROL_REGISTER, ModbusTimeoutError("timed out")
+        )
+        freezer.tick(ATTACHMENT_SCAN_INTERVAL)
+        async_fire_time_changed(hass)
+        await hass.async_block_till_done()
+
+        mock_modbus_unit.fail_read(POWER_CONTROL_REGISTER, None)
+        freezer.tick(ATTACHMENT_SCAN_INTERVAL)
+        async_fire_time_changed(hass)
+        await hass.async_block_till_done()
+
+    assert "power_control" not in asked[1], "a blip settled a block that answered"
+
+
+@pytest.mark.parametrize(
+    ("register", "subsystem"),
+    [
+        pytest.param(BATTERY_RATED_ENERGY, "batteries", id="batteries"),
+        pytest.param(METER_MODEL_REGISTER, "meters", id="meters"),
+        pytest.param(MMPPT_REGISTER, "mmppt", id="mmppt"),
+    ],
+)
+async def test_a_block_discovery_depends_on_keeps_being_asked(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+    mock_config_entry: MockConfigEntry,
+    mock_modbus_unit: MockModbusUnit,
+    register: int,
+    subsystem: str,
+) -> None:
+    """A block that finding hardware depends on is asked however quiet it stays.
+
+    Meters and batteries each bring a device. The multiple-MPPT block decides
+    the offset the meters are looked for at, so taking it for absent would look
+    for them at the wrong addresses for good.
+    """
+    mock_modbus_unit.fail_read(register, ModbusTimeoutError("timed out"))
+    await _setup(hass, mock_config_entry)
+
+    asked: list[frozenset[str]] = []
+    probe = SolarEdge.async_probe
+
+    async def recording_probe(
+        unit: ModbusUnit, *, assume_absent: frozenset[str] = frozenset()
+    ) -> SolarEdge:
+        asked.append(assume_absent)
+        return await probe(unit, assume_absent=assume_absent)
+
+    with patch.object(SolarEdge, "async_probe", recording_probe):
+        for _ in range(2):
+            freezer.tick(ATTACHMENT_SCAN_INTERVAL)
+            async_fire_time_changed(hass)
+            await hass.async_block_till_done()
+
+    assert subsystem not in asked[1], "a block discovery depends on was settled"