From d4842c8be3fbbb3cfb5ba8aa0299ecdd3092a836 Mon Sep 17 00:00:00 2001 From: darkrain-nl Date: Tue, 8 Sep 2026 18:55:42 +0200 Subject: [PATCH] Allow removing a Sofar battery pack that is no longer wired (#181283) --- homeassistant/components/sofar/__init__.py | 51 ++++++- homeassistant/components/sofar/const.py | 4 + homeassistant/components/sofar/coordinator.py | 13 +- .../components/sofar/quality_scale.yaml | 2 +- homeassistant/components/sofar/sensor.py | 24 +--- tests/components/sofar/test_init.py | 135 ++++++++++++++++++ 6 files changed, 206 insertions(+), 23 deletions(-) diff --git a/homeassistant/components/sofar/__init__.py b/homeassistant/components/sofar/__init__.py index 1af5d8b5ffca..e336d9ba9de3 100644 --- a/homeassistant/components/sofar/__init__.py +++ b/homeassistant/components/sofar/__init__.py @@ -2,6 +2,7 @@ from datetime import timedelta import logging +from typing import TYPE_CHECKING from modbus_connection import ModbusError, ModbusTcpParams from sofar_modbus.modern.device import SofarInverter, identify @@ -12,6 +13,7 @@ from homeassistant.components.sensor import ( SensorExtraStoredData, SensorStateClass, ) +from homeassistant.config_entries import ConfigEntryState from homeassistant.const import CONF_HOST, CONF_PORT, Platform from homeassistant.core import HomeAssistant from homeassistant.exceptions import ConfigEntryError @@ -23,7 +25,13 @@ from homeassistant.helpers import ( ) from homeassistant.helpers.typing import ConfigType -from .const import CONF_UNIT_ID, DOMAIN, SCAN_INTERVAL, SETTINGS_SCAN_INTERVAL +from .const import ( + BATTERY_COMPONENTS, + CONF_UNIT_ID, + DOMAIN, + SCAN_INTERVAL, + SETTINGS_SCAN_INTERVAL, +) from .coordinator import SofarConfigEntry, SofarDataUpdateCoordinator, SofarRuntimeData from .sensor import SENSOR_DESCRIPTIONS from .services import async_setup_services @@ -153,6 +161,47 @@ async def async_setup_entry(hass: HomeAssistant, entry: SofarConfigEntry) -> boo return True +def _battery_pack_number(serial: str, identifier: str) -> int | None: + """The battery pack a device identifier names, if it names one.""" + prefix = f"{serial}_battery_" + if not identifier.startswith(prefix): + return None + suffix = identifier.removeprefix(prefix) + number = int(suffix) if suffix.isdecimal() else None + return number if number in BATTERY_COMPONENTS else None + + +async def async_remove_config_entry_device( + hass: HomeAssistant, + config_entry: SofarConfigEntry, + device_entry: dr.AnyDeviceEntry, +) -> bool: + """Allow removing a battery pack the inverter no longer reports.""" + serial = config_entry.unique_id + if TYPE_CHECKING: + assert serial is not None + runtime_data = ( + config_entry.runtime_data + if config_entry.state is ConfigEntryState.LOADED + else None + ) + packs: set[int] = set() + for domain, identifier in device_entry.identifiers: + if domain != DOMAIN: + continue + if identifier == serial or identifier.startswith(f"{serial}_pv_string_"): + return False + if (number := _battery_pack_number(serial, identifier)) is None: + continue + if runtime_data is not None and runtime_data.pack_is_wired(number): + return False + packs.add(number) + + if runtime_data is not None: + runtime_data.wired_packs -= packs + return True + + async def async_unload_entry(hass: HomeAssistant, entry: SofarConfigEntry) -> bool: """Unload a config entry.""" return await hass.config_entries.async_unload_platforms(entry, PLATFORMS) diff --git a/homeassistant/components/sofar/const.py b/homeassistant/components/sofar/const.py index f96f6c38c4dc..627e1ba50f95 100644 --- a/homeassistant/components/sofar/const.py +++ b/homeassistant/components/sofar/const.py @@ -10,3 +10,7 @@ SCAN_INTERVAL = 5 SETTINGS_SCAN_INTERVAL = 60 CONF_UNIT_ID = "unit_id" + +BATTERY_COMPONENTS = { + n: "battery_1_2" if n <= 2 else "battery_3_8" for n in range(1, 9) +} diff --git a/homeassistant/components/sofar/coordinator.py b/homeassistant/components/sofar/coordinator.py index 200743c0b1ed..93e45962c8aa 100644 --- a/homeassistant/components/sofar/coordinator.py +++ b/homeassistant/components/sofar/coordinator.py @@ -1,7 +1,7 @@ """Data update coordinator for Sofar devices.""" from collections.abc import Awaitable, Callable -from dataclasses import dataclass +from dataclasses import dataclass, field from datetime import timedelta import logging from typing import override @@ -16,7 +16,7 @@ from homeassistant.core import HomeAssistant from homeassistant.helpers import device_registry as dr from homeassistant.helpers.update_coordinator import DataUpdateCoordinator, UpdateFailed -from .const import ATTR_MANUFACTURER, DOMAIN +from .const import ATTR_MANUFACTURER, BATTERY_COMPONENTS, DOMAIN _LOGGER = logging.getLogger(__name__) @@ -128,6 +128,7 @@ class SofarRuntimeData: readings: SofarDataUpdateCoordinator settings: SofarDataUpdateCoordinator inverter_device_id: str + wired_packs: set[int] = field(default_factory=set) @property def served_components(self) -> frozenset[str]: @@ -137,6 +138,14 @@ class SofarRuntimeData: device.settings_components ) + def pack_is_wired(self, number: int) -> bool: + """Whether a pack has answered, so it physically exists.""" + component_name = BATTERY_COMPONENTS[number] + if component_name not in self.served_components: + return False + component = getattr(self.readings.device, component_name) + return bool(getattr(component, f"battery_voltage_{number}", None)) + def coordinator_for(self, component: str) -> SofarDataUpdateCoordinator: """Which coordinator owns a given component's data.""" if component in self.readings.device.readings_components: diff --git a/homeassistant/components/sofar/quality_scale.yaml b/homeassistant/components/sofar/quality_scale.yaml index 43cf2d912eaf..b394b2af1542 100644 --- a/homeassistant/components/sofar/quality_scale.yaml +++ b/homeassistant/components/sofar/quality_scale.yaml @@ -66,7 +66,7 @@ rules: icon-translations: done reconfiguration-flow: done repair-issues: todo - stale-devices: todo + stale-devices: done # Platinum async-dependency: done diff --git a/homeassistant/components/sofar/sensor.py b/homeassistant/components/sofar/sensor.py index 9ba1c5338db2..6e6bcedfd6de 100644 --- a/homeassistant/components/sofar/sensor.py +++ b/homeassistant/components/sofar/sensor.py @@ -6,7 +6,6 @@ from datetime import date from enum import IntEnum from typing import cast, override -from sofar_modbus.modern.device import SofarInverter from sofar_modbus.modern.enums import FeedinLimitationMode, PassiveModeTimeoutAction from homeassistant.components.sensor import ( @@ -32,6 +31,7 @@ from homeassistant.const import ( from homeassistant.core import HomeAssistant, callback from homeassistant.helpers.entity_platform import AddConfigEntryEntitiesCallback +from .const import BATTERY_COMPONENTS from .coordinator import SofarConfigEntry from .entity import SofarEntity, SofarEntityDescription @@ -46,7 +46,6 @@ async def async_setup_entry( """Set up the Sofar Inverter Modbus sensor platform.""" runtime_data = entry.runtime_data served = runtime_data.served_components - device = runtime_data.readings.device async_add_entities( _sensor_class(description)(runtime_data, description) @@ -54,15 +53,14 @@ async def async_setup_entry( if description.component in served and not _is_battery_pack(description) ) - wired: set[int] = set() - @callback def _async_add_wired_packs() -> None: """Add a pack's sensors the first time it reports a voltage.""" + wired = runtime_data.wired_packs new = { number - for number in _BATTERY_COMPONENTS - if number not in wired and _pack_is_wired(device, served, number) + for number in BATTERY_COMPONENTS + if number not in wired and runtime_data.pack_is_wired(number) } if not new: return @@ -86,15 +84,6 @@ def _is_battery_pack(description: SofarSensorDescription) -> bool: return description.part is not None and description.part[0] == "battery" -def _pack_is_wired(device: SofarInverter, served: frozenset[str], number: int) -> bool: - """Whether a pack has answered, so it physically exists.""" - component_name = _BATTERY_COMPONENTS[number] - if component_name not in served: - return False - component = getattr(device, component_name) - return bool(getattr(component, f"battery_voltage_{number}", None)) - - def _sensor_class( description: SofarSensorDescription, ) -> type[SofarSensor | SofarTotalSensor]: @@ -191,9 +180,6 @@ _PV_STRING_COMPONENTS = { 9: "pv_9_10", 10: "pv_9_10", } -_BATTERY_COMPONENTS = { - n: "battery_1_2" if n <= 2 else "battery_3_8" for n in range(1, 9) -} _PV_STRING_MEASUREMENTS = ( _PartMeasurement( @@ -1516,4 +1502,4 @@ SENSOR_DESCRIPTIONS: tuple[SofarSensorDescription, ...] = ( SENSOR_DESCRIPTIONS += _part_sensors( "pv_string", _PV_STRING_COMPONENTS, _PV_STRING_MEASUREMENTS -) + _part_sensors("battery", _BATTERY_COMPONENTS, _BATTERY_MEASUREMENTS) +) + _part_sensors("battery", BATTERY_COMPONENTS, _BATTERY_MEASUREMENTS) diff --git a/tests/components/sofar/test_init.py b/tests/components/sofar/test_init.py index f48148ab87c5..7a973e44388b 100644 --- a/tests/components/sofar/test_init.py +++ b/tests/components/sofar/test_init.py @@ -19,6 +19,7 @@ from homeassistant.config_entries import ConfigEntryState from homeassistant.const import STATE_UNAVAILABLE from homeassistant.core import HomeAssistant from homeassistant.helpers import device_registry as dr, entity_registry as er +from homeassistant.setup import async_setup_component from . import ( MOCK_HW_VERSION, @@ -31,6 +32,7 @@ from . import ( ) from tests.common import MockConfigEntry, async_fire_time_changed +from tests.typing import WebSocketGenerator PV_POWER_REGISTER = 0x0586 BATTERY_3_VOLTAGE_REGISTER = 0x0612 @@ -577,3 +579,136 @@ async def test_battery_pack_appears_once_its_block_answers( entity_id = entity_registry.async_get_entity_id(SENSOR_DOMAIN, DOMAIN, unique_id) assert entity_id is not None assert hass.states.get(entity_id).state == "51.5" + + +async def _setup_hybrid( + hass: HomeAssistant, connection: MockModbusConnection +) -> MockConfigEntry: + """Set up a hybrid entry against a connection the caller still owns.""" + entry = MockConfigEntry( + domain=DOMAIN, + unique_id=MOCK_HYBRID_SERIAL, + data=MOCK_USER_INPUT, + title=MOCK_HYBRID_MODEL, + ) + entry.add_to_hass(hass) + with patch( + "homeassistant.components.sofar.async_get_unit", + side_effect=lambda hass, entry, params, unit_id: connection.for_unit(unit_id), + ): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done(wait_background_tasks=True) + return entry + + +@pytest.mark.parametrize( + ("identifiers", "removable"), + [ + pytest.param({(DOMAIN, MOCK_HYBRID_SERIAL)}, False, id="inverter"), + pytest.param( + {(DOMAIN, f"{MOCK_HYBRID_SERIAL}_battery_1")}, False, id="wired_pack" + ), + pytest.param( + {(DOMAIN, f"{MOCK_HYBRID_SERIAL}_pv_string_2")}, False, id="pv_string" + ), + pytest.param( + {(DOMAIN, f"{MOCK_HYBRID_SERIAL}_battery_9")}, True, id="pack_off_the_map" + ), + pytest.param( + {(DOMAIN, f"{MOCK_HYBRID_SERIAL}_battery_x")}, True, id="unparseable_part" + ), + pytest.param( + {(DOMAIN, f"{MOCK_HYBRID_SERIAL}_battery_\u00b2")}, + True, + id="non_decimal_digit", + ), + pytest.param( + {(DOMAIN, f"{MOCK_HYBRID_SERIAL}_gizmo_1")}, True, id="unknown_part_kind" + ), + pytest.param({("other", MOCK_HYBRID_SERIAL)}, True, id="foreign_identifier"), + ], +) +async def test_only_absent_battery_packs_can_be_removed( + hass: HomeAssistant, + hass_ws_client: WebSocketGenerator, + device_registry: dr.DeviceRegistry, + identifiers: set[tuple[str, str]], + removable: bool, +) -> None: + """Test removal is refused for anything the inverter still reports.""" + assert await async_setup_component(hass, "config", {}) + connection = MockModbusConnection() + seed_hybrid_inverter(connection.for_unit(1)) + entry = await _setup_hybrid(hass, connection) + + device = device_registry.async_get_or_create( + config_entry_id=entry.entry_id, identifiers=identifiers + ) + client = await hass_ws_client(hass) + response = await client.remove_device(device.id) + + assert response["success"] is removable + assert (device_registry.async_get(device.id) is None) is removable + + +async def test_an_absent_pack_is_removable_while_the_entry_is_unloaded( + hass: HomeAssistant, + hass_ws_client: WebSocketGenerator, + device_registry: dr.DeviceRegistry, +) -> None: + """Test a pack whose liveness cannot be checked is taken on trust.""" + assert await async_setup_component(hass, "config", {}) + connection = MockModbusConnection() + seed_hybrid_inverter(connection.for_unit(1)) + entry = await _setup_hybrid(hass, connection) + + await hass.config_entries.async_unload(entry.entry_id) + await hass.async_block_till_done() + + device = device_registry.async_get_device_by_identifier( + (DOMAIN, f"{MOCK_HYBRID_SERIAL}_battery_1"), entry.entry_id + ) + assert device is not None + client = await hass_ws_client(hass) + + assert (await client.remove_device(device.id))["success"] + assert device_registry.async_get(device.id) is None + + +async def test_a_removed_pack_comes_back_without_a_restart( + hass: HomeAssistant, + freezer: FrozenDateTimeFactory, + hass_ws_client: WebSocketGenerator, + device_registry: dr.DeviceRegistry, + entity_registry: er.EntityRegistry, +) -> None: + """Test removing a pack does not bar it from being added again.""" + assert await async_setup_component(hass, "config", {}) + connection = MockModbusConnection() + unit = connection.for_unit(1) + seed_hybrid_inverter(unit) + entry = await _setup_hybrid(hass, connection) + + unit.holding[BATTERY_3_VOLTAGE_REGISTER] = 0 + freezer.tick(timedelta(seconds=SCAN_INTERVAL)) + async_fire_time_changed(hass) + await hass.async_block_till_done() + + device = device_registry.async_get_device_by_identifier( + (DOMAIN, f"{MOCK_HYBRID_SERIAL}_battery_3"), entry.entry_id + ) + assert device is not None + client = await hass_ws_client(hass) + assert (await client.remove_device(device.id))["success"] + + unique_id = f"{MOCK_HYBRID_SERIAL}_battery_voltage_3" + assert entity_registry.async_get_entity_id(SENSOR_DOMAIN, DOMAIN, unique_id) is None + + unit.holding[BATTERY_3_VOLTAGE_REGISTER] = 515 + freezer.tick(timedelta(seconds=SCAN_INTERVAL)) + async_fire_time_changed(hass) + await hass.async_block_till_done() + + entity_id = entity_registry.async_get_entity_id(SENSOR_DOMAIN, DOMAIN, unique_id) + assert entity_id is not None + assert hass.states.get(entity_id).state == "51.5"