Correct modbus config validator: slave/swap (#97798)

homeassistant/components/modbus/validators.py

@@ -65,25 +65,14 @@ def struct_validator(config: dict[str, Any]) -> dict[str, Any]:
     name = config[CONF_NAME]
     structure = config.get(CONF_STRUCTURE)
     slave_count = config.get(CONF_SLAVE_COUNT, 0) + 1
-    swap_type = config.get(CONF_SWAP)
+    slave = config.get(CONF_SLAVE, 0)
-    if config[CONF_DATA_TYPE] != DataType.CUSTOM:
+    swap_type = config.get(CONF_SWAP, CONF_SWAP_NONE)
-        if structure:
+    if config[CONF_DATA_TYPE] == DataType.CUSTOM:
-            error = f"{name}  structure: cannot be mixed with {data_type}"
+        if slave or slave_count > 1:
+            error = f"{name}: `{CONF_STRUCTURE}` illegal with `{CONF_SLAVE_COUNT}` / `{CONF_SLAVE}`"
             raise vol.Invalid(error)
-        if data_type not in DEFAULT_STRUCT_FORMAT:
+        if swap_type != CONF_SWAP_NONE:
-            error = f"Error in sensor {name}. data_type `{data_type}` not supported"
+            error = f"{name}: `{CONF_STRUCTURE}` illegal with `{CONF_SWAP}`"
-            raise vol.Invalid(error)
-
-        structure = f">{DEFAULT_STRUCT_FORMAT[data_type].struct_id}"
-        if CONF_COUNT not in config:
-            config[CONF_COUNT] = DEFAULT_STRUCT_FORMAT[data_type].register_count
-        if slave_count > 1:
-            structure = f">{slave_count}{DEFAULT_STRUCT_FORMAT[data_type].struct_id}"
-        else:
-            structure = f">{DEFAULT_STRUCT_FORMAT[data_type].struct_id}"
-    else:
-        if slave_count > 1:
-            error = f"{name}  structure: cannot be mixed with {CONF_SLAVE_COUNT}"
             raise vol.Invalid(error)
         if not structure:
             error = (
@@ -102,19 +91,43 @@ def struct_validator(config: dict[str, Any]) -> dict[str, Any]:
                 f"Structure request {size} bytes, "
                 f"but {count} registers have a size of {bytecount} bytes"
             )
+        return {
+            **config,
+            CONF_STRUCTURE: structure,
+            CONF_SWAP: swap_type,
+        }
 
-        if swap_type != CONF_SWAP_NONE:
+    if structure:
-            if swap_type == CONF_SWAP_BYTE:
+        error = f"{name}  structure: cannot be mixed with {data_type}"
-                regs_needed = 1
+        raise vol.Invalid(error)
-            else:  # CONF_SWAP_WORD_BYTE, CONF_SWAP_WORD
+    if data_type not in DEFAULT_STRUCT_FORMAT:
-                regs_needed = 2
+        error = f"Error in sensor {name}. data_type `{data_type}` not supported"
-            if count < regs_needed or (count % regs_needed) != 0:
+        raise vol.Invalid(error)
-                raise vol.Invalid(
+    if (slave or slave_count > 1) and data_type == DataType.STRING:
-                    f"Error in sensor {name} swap({swap_type}) "
+        error = (
-                    "not possible due to the registers "
+            f"{name}: `{data_type}`  illegal with `{CONF_SLAVE_COUNT}` / `{CONF_SLAVE}`"
-                    f"count: {count}, needed: {regs_needed}"
+        )
-                )
+        raise vol.Invalid(error)
 
+    if CONF_COUNT not in config:
+        config[CONF_COUNT] = DEFAULT_STRUCT_FORMAT[data_type].register_count
+    if swap_type != CONF_SWAP_NONE:
+        if swap_type == CONF_SWAP_BYTE:
+            regs_needed = 1
+        else:  # CONF_SWAP_WORD_BYTE, CONF_SWAP_WORD
+            regs_needed = 2
+        count = config[CONF_COUNT]
+        if count < regs_needed or (count % regs_needed) != 0:
+            raise vol.Invalid(
+                f"Error in sensor {name} swap({swap_type}) "
+                "not possible due to the registers "
+                f"count: {count}, needed: {regs_needed}"
+            )
+    structure = f">{DEFAULT_STRUCT_FORMAT[data_type].struct_id}"
+    if slave_count > 1:
+        structure = f">{slave_count}{DEFAULT_STRUCT_FORMAT[data_type].struct_id}"
+    else:
+        structure = f">{DEFAULT_STRUCT_FORMAT[data_type].struct_id}"
     return {
         **config,
         CONF_STRUCTURE: structure,

tests/components/modbus/test_init.py

@@ -181,7 +181,6 @@ async def test_nan_validator() -> None:
             CONF_COUNT: 2,
             CONF_DATA_TYPE: DataType.CUSTOM,
             CONF_STRUCTURE: ">i",
-            CONF_SWAP: CONF_SWAP_BYTE,
         },
     ],
 )
@@ -239,6 +238,16 @@ async def test_ok_struct_validator(do_config) -> None:
             CONF_STRUCTURE: ">f",
             CONF_SLAVE_COUNT: 5,
         },
+        {
+            CONF_NAME: TEST_ENTITY_NAME,
+            CONF_DATA_TYPE: DataType.STRING,
+            CONF_SLAVE_COUNT: 2,
+        },
+        {
+            CONF_NAME: TEST_ENTITY_NAME,
+            CONF_DATA_TYPE: DataType.INT16,
+            CONF_SWAP: CONF_SWAP_WORD,
+        },
     ],
 )
 async def test_exception_struct_validator(do_config) -> None:

tests/components/modbus/test_sensor.py

@@ -246,7 +246,7 @@ async def test_config_sensor(hass: HomeAssistant, mock_modbus) -> None:
                     },
                 ]
             },
-            f"Error in sensor {TEST_ENTITY_NAME} swap(word) not possible due to the registers count: 1, needed: 2",
+            f"{TEST_ENTITY_NAME}: `structure` illegal with `swap`",
         ),
     ],
 )