Fix solis_write_holding_register host matching and swallowed errors (#12)

get_controller matched on a hub's .host property, which delegates to
whichever protocol is active and can be the literal string
"soliscloud" - so host-based service calls silently stopped matching
after failover. It now also accepts a protocol-stable serial, and
host/slave matching reads the hub's .modbus sub-controller directly.

The service handler fired writes into an untracked hass.create_task,
so failures vanished silently. It now awaits the write directly and
raises HomeAssistantError on failure, and requires an explicit target
when multiple inverters are configured instead of broadcasting to all
of them.

Fixes #12

Author: fboundy

custom_components/solisconnect/__init__.py

@@ -9,7 +9,7 @@
 from homeassistant.config_entries import ConfigEntry
 from homeassistant.const import Platform
 from homeassistant.core import HomeAssistant, ServiceCall
-from homeassistant.exceptions import ConfigEntryError
+from homeassistant.exceptions import ConfigEntryError, HomeAssistantError, ServiceValidationError
 from homeassistant.helpers.device_registry import DeviceEntry
 
 from .const import (
@@ -60,6 +60,8 @@
         vol.Required("address"): vol.Coerce(int),
         vol.Required("value"): vol.Coerce(int),
         vol.Optional("host"): vol.Coerce(str),
+        vol.Optional("slave"): vol.Coerce(int),
+        vol.Optional("serial"): vol.Coerce(str),
     }
 )
 SCHEME_TIME_SET = vol.Schema({vol.Required("entity_id"): vol.Coerce(str), vol.Required("time"): vol.Coerce(str)})
@@ -73,18 +75,31 @@ async def async_remove_config_entry_device(hass: HomeAssistant, config_entry: Co
 async def async_setup(hass: HomeAssistant, entry: ConfigEntry):
     """Set up the Modbus integration."""
 
-    def service_write_holding_register(call: ServiceCall):
+    async def service_write_holding_register(call: ServiceCall):
         address = call.data.get("address")
         value = call.data.get("value")
         host = call.data.get("host")
         slave = call.data.get("slave", 1)
+        serial = call.data.get("serial")
 
-        if host:
-            controller = get_controller(hass, host, slave)
-            hass.create_task(controller.async_write_holding_register(int(address), int(value)))
+        if serial or host:
+            controller = get_controller(hass, host, slave, serial=serial)
+            if controller is None:
+                raise ServiceValidationError(f"No configured SolisConnect inverter matches serial={serial!r} host={host!r} slave={slave}")
         else:
-            for controller in hass.data[DOMAIN][CONTROLLER].values():
-                hass.create_task(controller.async_write_holding_register(int(address), int(value)))
+            controllers = list(hass.data.get(DOMAIN, {}).get(CONTROLLER, {}).values())
+            if not controllers:
+                raise ServiceValidationError("No configured SolisConnect inverter found")
+            if len(controllers) > 1:
+                raise ServiceValidationError("Multiple SolisConnect inverters are configured; specify a target 'serial' or 'host' to disambiguate")
+            controller = controllers[0]
+
+        try:
+            await controller.async_write_holding_register(int(address), int(value))
+        except HomeAssistantError:
+            raise
+        except Exception as error:
+            raise HomeAssistantError(f"Writing register {address}={value} failed: {error}") from error
 
     async def service_set_time(call: ServiceCall) -> None:
         """Service to update a Solis time entity."""

custom_components/solisconnect/helpers.py

@@ -245,15 +245,31 @@ def get_controller_from_entry(hass: HomeAssistant, config_entry: ConfigEntry):
     return hass.data[DOMAIN][CONTROLLER].get(config_entry.entry_id)
 
 
-def get_controller(hass: HomeAssistant, host: str, slave: int = 1):
+def get_controller(hass: HomeAssistant, host: str | None = None, slave: int = 1, serial: str | None = None):
     """
-    Search for a controller matching the host and slave ID.
-    Used by services (write_holding_register) that only know the IP/Host.
+    Find a stored controller (a SolisInverterHub, per config entry) by inverter serial or
+    by its configured Modbus host/slave. Used by services (write_holding_register) that
+    only know a target host or serial.
+
+    Serial is protocol-stable and preferred: a hub's `host`/`device_id` properties delegate
+    to whichever protocol is currently active (e.g. the literal string "soliscloud" while
+    cloud is active), so host/slave matching instead reads the hub's `.modbus` sub-controller
+    directly, which always reflects its real configured Modbus link regardless of which
+    protocol is currently active.
     """
     for controller in hass.data[DOMAIN][CONTROLLER].values():
-        # Check if this controller matches the requested host and slave
-        # Use getattr to be safe if controller hasn't fully initialized
-        if getattr(controller, "host", None) == host and getattr(controller, "device_id", 0) == slave:
+        if serial is not None:
+            if getattr(controller, "serial_number", None) == serial:
+                return controller
+            continue
+        modbus = getattr(controller, "modbus", None)
+        if modbus is not None:
+            configured_host = getattr(modbus, "host", None)
+            configured_slave = getattr(modbus, "device_id", None)
+        else:
+            configured_host = getattr(controller, "host", None)
+            configured_slave = getattr(controller, "device_id", None)
+        if configured_host == host and configured_slave == slave:
             return controller
     return None
 

custom_components/solisconnect/services.yaml

@@ -10,9 +10,17 @@ solis_write_holding_register:
       default: ''
       example: 500
     host:
-      description: IP of the inverter, only required when running multiple inverters
+      description: Configured Modbus IP of the target inverter; only required when running multiple inverters. Prefer 'serial' for dual-protocol/cloud entries, since 'host' matches the inverter's configured Modbus link regardless of which protocol is currently active.
       default: ''
       example: 192.168.1.75
+    slave:
+      description: Modbus slave/unit ID of the target inverter, when disambiguating by host on a shared link
+      default: 1
+      example: 1
+    serial:
+      description: Inverter serial number of the target inverter; protocol-stable, works for Modbus, cloud, and dual-protocol entries
+      default: ''
+      example: '1234567890123456'
 solis_write_time:
   description: Writes a time string to an entity
   fields:

tests/test_services.py

@@ -2,15 +2,18 @@
 
 import pytest
 from homeassistant.core import HomeAssistant
+from homeassistant.exceptions import HomeAssistantError, ServiceValidationError
 
-from custom_components.solisconnect.const import DOMAIN
+from custom_components.solisconnect.const import CONTROLLER, DOMAIN
+from custom_components.solisconnect.helpers import get_controller
 
 
 @pytest.fixture
 def mock_controller():
     controller = MagicMock()
     controller.host = "1.2.3.4"
     controller.device_id = 1
+    controller.modbus = None  # simulates a cloud-only/bare controller; hubs set this to their sub-controller
     controller.async_write_holding_register = AsyncMock(return_value=None)
     return controller
 
@@ -53,6 +56,105 @@ async def test_service_write_holding_register_no_host(hass: HomeAssistant, mock_
     mock_controller.async_write_holding_register.assert_called_with(123, 456)
 
 
+@pytest.mark.asyncio
+async def test_service_write_holding_register_no_target_with_multiple_controllers_raises(hass: HomeAssistant, mock_controller):
+    """Bug #12: silently broadcasting a write to every configured inverter is dangerous; require a target."""
+    other_controller = MagicMock()
+    other_controller.host = "5.6.7.8"
+    other_controller.device_id = 1
+    other_controller.async_write_holding_register = AsyncMock(return_value=None)
+
+    hass.data.setdefault(DOMAIN, {})
+    hass.data[DOMAIN][CONTROLLER] = {"1.2.3.4_1": mock_controller, "5.6.7.8_1": other_controller}
+
+    from custom_components.solisconnect import async_setup
+
+    await async_setup(hass, {})
+
+    with pytest.raises(ServiceValidationError, match="Multiple"):
+        await hass.services.async_call(DOMAIN, "solis_write_holding_register", {"address": 123, "value": 456}, blocking=True)
+
+    mock_controller.async_write_holding_register.assert_not_called()
+    other_controller.async_write_holding_register.assert_not_called()
+
+
+@pytest.mark.asyncio
+async def test_service_write_holding_register_unmatched_target_raises(hass: HomeAssistant, mock_controller):
+    hass.data.setdefault(DOMAIN, {})
+    hass.data[DOMAIN][CONTROLLER] = {"1.2.3.4_1": mock_controller}
+
+    from custom_components.solisconnect import async_setup
+
+    await async_setup(hass, {})
+
+    with pytest.raises(ServiceValidationError, match="No configured"):
+        await hass.services.async_call(DOMAIN, "solis_write_holding_register", {"address": 123, "value": 456, "host": "9.9.9.9"}, blocking=True)
+
+    mock_controller.async_write_holding_register.assert_not_called()
+
+
+@pytest.mark.asyncio
+async def test_service_write_holding_register_matches_by_serial(hass: HomeAssistant, mock_controller):
+    """Serial matching must work even while the hub's own `host` property is protocol-dependent."""
+    mock_controller.host = "soliscloud"  # simulates a dual-protocol hub currently active on cloud
+    mock_controller.serial_number = "SN12345"
+
+    hass.data.setdefault(DOMAIN, {})
+    hass.data[DOMAIN][CONTROLLER] = {"1.2.3.4_1": mock_controller}
+
+    from custom_components.solisconnect import async_setup
+
+    await async_setup(hass, {})
+
+    await hass.services.async_call(DOMAIN, "solis_write_holding_register", {"address": 123, "value": 456, "serial": "SN12345"}, blocking=True)
+
+    mock_controller.async_write_holding_register.assert_called_with(123, 456)
+
+
+@pytest.mark.asyncio
+async def test_service_write_holding_register_write_failure_raises(hass: HomeAssistant, mock_controller):
+    """Bug #12: a write failure must surface as a service error, not vanish into a fire-and-forget task."""
+    mock_controller.async_write_holding_register = AsyncMock(side_effect=RuntimeError("boom"))
+
+    hass.data.setdefault(DOMAIN, {})
+    hass.data[DOMAIN][CONTROLLER] = {"1.2.3.4_1": mock_controller}
+
+    from custom_components.solisconnect import async_setup
+
+    await async_setup(hass, {})
+
+    with pytest.raises(HomeAssistantError, match="boom"):
+        await hass.services.async_call(DOMAIN, "solis_write_holding_register", {"address": 123, "value": 456, "host": "1.2.3.4"}, blocking=True)
+
+
+def test_get_controller_matches_host_via_modbus_sub_controller_when_hub_is_cloud_active():
+    """Bug #12: a hub's `.host` delegates to the active protocol (e.g. "soliscloud"); host matching
+    must read the hub's `.modbus` sub-controller instead, so it still works after failover."""
+    hub = MagicMock()
+    hub.host = "soliscloud"  # currently active protocol is cloud
+    hub.device_id = 1  # cloud controller's hardcoded device_id, not the real Modbus slave
+    hub.modbus.host = "1.2.3.4"  # the hub's real, protocol-independent Modbus link
+    hub.modbus.device_id = 3
+
+    hass = MagicMock()
+    hass.data = {DOMAIN: {CONTROLLER: {"entry1": hub}}}
+
+    assert get_controller(hass, host="1.2.3.4", slave=3) is hub
+    assert get_controller(hass, host="soliscloud", slave=1) is None  # the active-protocol host must not match
+
+
+def test_get_controller_matches_serial_regardless_of_host():
+    hub = MagicMock()
+    hub.serial_number = "SN999"
+    hub.host = "soliscloud"
+
+    hass = MagicMock()
+    hass.data = {DOMAIN: {CONTROLLER: {"entry1": hub}}}
+
+    assert get_controller(hass, serial="SN999") is hub
+    assert get_controller(hass, serial="SNOTHER") is None
+
+
 @pytest.mark.asyncio
 async def test_service_set_time(hass: HomeAssistant):
     """Test solis_write_time service."""