[modbus_controller] Apply the write offset byte-accurately on switch and output (#18787)

Co-authored-by: J. Nick Koston <nick@koston.org>

esphome/components/modbus_controller/__init__.py

@@ -278,6 +278,21 @@ def _final_validate(config: ConfigType) -> None:
 FINAL_VALIDATE_SCHEMA = _final_validate
 
 
+def reject_odd_holding_write_offset(config: ConfigType) -> ConfigType:
+    """Reject an odd byte offset on a holding-register write entity.
+
+    A 16-bit register write cannot target half a register, so the residual byte is inexpressible.
+    """
+    key = CONF_BYTE_OFFSET if CONF_BYTE_OFFSET in config else CONF_OFFSET
+    if config.get(key, 0) % 2:
+        raise cv.Invalid(
+            f"An odd '{key}' cannot be used with holding-register writes: a 16-bit register "
+            "write cannot target half a register. Use an even offset, or fold it into 'address'",
+            path=[key],
+        )
+    return config
+
+
 def modbus_calc_properties(config: ConfigType) -> tuple[int, int]:
     byte_offset = 0
     reg_count = 0

esphome/components/modbus_controller/output/__init__.py

@@ -14,6 +14,7 @@ from .. import (
     SensorItem,
     modbus_calc_properties,
     modbus_controller_ns,
+    reject_odd_holding_write_offset,
 )
 from ..const import (
     CONF_CUSTOM_COMMAND,
@@ -53,23 +54,26 @@ CONFIG_SCHEMA = cv.typed_schema(
                 cv.Optional(CONF_USE_WRITE_MULTIPLE, default=False): cv.boolean,
             }
         ),
-        "holding": output.FLOAT_OUTPUT_SCHEMA.extend(ModbusItemBaseSchema).extend(
-            {
-                cv.GenerateID(): cv.declare_id(ModbusFloatOutput),
-                cv.Required(CONF_ADDRESS): cv.positive_int,
-                cv.Optional(CONF_CUSTOM_PDU): cv.invalid(
-                    "custom_pdu is not supported for outputs; use a write_lambda instead"
-                ),
-                cv.Optional(CONF_CUSTOM_COMMAND): cv.invalid(
-                    "custom_command is not supported for outputs; use a write_lambda instead"
-                ),
-                cv.Optional(CONF_VALUE_TYPE, default="U_WORD"): cv.enum(
-                    SENSOR_VALUE_TYPE
-                ),
-                cv.Optional(CONF_WRITE_LAMBDA): cv.returning_lambda,
-                cv.Optional(CONF_MULTIPLY, default=1.0): cv.float_,
-                cv.Optional(CONF_USE_WRITE_MULTIPLE, default=False): cv.boolean,
-            }
+        "holding": cv.All(
+            output.FLOAT_OUTPUT_SCHEMA.extend(ModbusItemBaseSchema).extend(
+                {
+                    cv.GenerateID(): cv.declare_id(ModbusFloatOutput),
+                    cv.Required(CONF_ADDRESS): cv.positive_int,
+                    cv.Optional(CONF_CUSTOM_PDU): cv.invalid(
+                        "custom_pdu is not supported for outputs; use a write_lambda instead"
+                    ),
+                    cv.Optional(CONF_CUSTOM_COMMAND): cv.invalid(
+                        "custom_command is not supported for outputs; use a write_lambda instead"
+                    ),
+                    cv.Optional(CONF_VALUE_TYPE, default="U_WORD"): cv.enum(
+                        SENSOR_VALUE_TYPE
+                    ),
+                    cv.Optional(CONF_WRITE_LAMBDA): cv.returning_lambda,
+                    cv.Optional(CONF_MULTIPLY, default=1.0): cv.float_,
+                    cv.Optional(CONF_USE_WRITE_MULTIPLE, default=False): cv.boolean,
+                }
+            ),
+            reject_odd_holding_write_offset,
         ),
     },
     lower=True,

esphome/components/modbus_controller/output/modbus_output.h

@@ -12,7 +12,8 @@ class ModbusFloatOutput final : public output::FloatOutput, public Component, pu
  public:
   ModbusFloatOutput(uint16_t start_address, uint8_t offset, SensorValueType value_type, int register_count) {
     this->register_type = modbus::EntityType::HOLDING;
-    this->set_address(start_address + offset);
+    // A byte offset folds into the address as whole registers; odd offsets are rejected at validation.
+    this->set_address(start_address + offset / 2);
     this->set_offset_from_start_address(0);
     this->bitmask = 0xFFFFFFFF;
     this->register_count = register_count;

esphome/components/modbus_controller/switch/__init__.py

@@ -11,6 +11,7 @@ from .. import (
     add_modbus_base_properties,
     modbus_calc_properties,
     modbus_controller_ns,
+    reject_odd_holding_write_offset,
     validate_custom_pdu_item,
     validate_modbus_register,
 )
@@ -31,6 +32,14 @@ ModbusSwitch = modbus_controller_ns.class_(
     "ModbusSwitch", cg.Component, switch.Switch, SensorItem
 )
 
+
+def _validate_holding_offset(config: ConfigType) -> ConfigType:
+    # Only a holding-register switch folds the byte offset into a 16-bit register write.
+    if config.get(CONF_REGISTER_TYPE) == "holding":
+        reject_odd_holding_write_offset(config)
+    return config
+
+
 CONFIG_SCHEMA = cv.All(
     switch.switch_schema(ModbusSwitch, default_restore_mode="DISABLED")
     .extend(cv.COMPONENT_SCHEMA)
@@ -44,6 +53,7 @@ CONFIG_SCHEMA = cv.All(
         }
     ),
     validate_modbus_register,
+    _validate_holding_offset,
 )
 
 FINAL_VALIDATE_SCHEMA = validate_custom_pdu_item

esphome/components/modbus_controller/switch/modbus_switch.h

@@ -18,8 +18,13 @@ class ModbusSwitch final : public Component, public switch_::Switch, public Sens
     this->bitmask = bitmask;
     this->sensor_value_type = SensorValueType::BIT;
     this->register_count = 1;
-    if (register_type == modbus::EntityType::HOLDING || register_type == modbus::EntityType::COIL) {
-      this->set_address(this->start_address + offset);
+    // A holding byte offset folds into the address as whole registers (odd offsets are rejected at
+    // validation: a 16-bit register write cannot target half a register); a coil offset is a coil count.
+    if (register_type == modbus::EntityType::HOLDING) {
+      this->set_address(start_address + offset / 2);
+      this->set_offset_from_start_address(0);
+    } else if (register_type == modbus::EntityType::COIL) {
+      this->set_address(start_address + offset);
       this->set_offset_from_start_address(0);
     }
     this->force_new_range = force_new_range;

tests/component_tests/modbus_controller/test_write_offset.py

@@ -0,0 +1,74 @@
+"""Config validation for the byte offset on holding-register write entities.
+
+A 16-bit register write cannot target half a register, so an odd offset (or byte_offset) is
+rejected for holding-register switches and outputs; even offsets and coil offsets pass.
+"""
+
+import pytest
+from voluptuous import Invalid, MultipleInvalid
+
+from esphome.components.modbus_controller.output import (
+    CONFIG_SCHEMA as OUTPUT_CONFIG_SCHEMA,
+)
+from esphome.components.modbus_controller.switch import (
+    CONFIG_SCHEMA as SWITCH_CONFIG_SCHEMA,
+)
+from esphome.const import CONF_ADDRESS, CONF_ID, CONF_NAME, CONF_OFFSET
+
+
+def _switch_config(register_type: str, offset: int) -> dict:
+    return {
+        CONF_NAME: "test switch",
+        CONF_ADDRESS: 0x10,
+        "register_type": register_type,
+        CONF_OFFSET: offset,
+    }
+
+
+def _output_config(register_type: str, offset: int) -> dict:
+    return {
+        CONF_ID: "test_output",
+        CONF_ADDRESS: 0x10,
+        "register_type": register_type,
+        CONF_OFFSET: offset,
+    }
+
+
+def test_odd_offset_on_holding_switch_rejected() -> None:
+    with pytest.raises((Invalid, MultipleInvalid), match="odd"):
+        SWITCH_CONFIG_SCHEMA(_switch_config("holding", 3))
+
+
+def test_even_offset_on_holding_switch_accepted() -> None:
+    config = SWITCH_CONFIG_SCHEMA(_switch_config("holding", 2))
+    assert config[CONF_OFFSET] == 2
+
+
+def test_odd_offset_on_coil_switch_accepted() -> None:
+    """A coil offset is a coil count, so odd values are fine."""
+    config = SWITCH_CONFIG_SCHEMA(_switch_config("coil", 3))
+    assert config[CONF_OFFSET] == 3
+
+
+def test_odd_byte_offset_on_holding_switch_rejected() -> None:
+    """byte_offset is the alias the validator must also catch."""
+    config = _switch_config("holding", 0)
+    del config[CONF_OFFSET]
+    config["byte_offset"] = 3
+    with pytest.raises((Invalid, MultipleInvalid), match="byte_offset"):
+        SWITCH_CONFIG_SCHEMA(config)
+
+
+def test_odd_offset_on_holding_output_rejected() -> None:
+    with pytest.raises((Invalid, MultipleInvalid), match="odd"):
+        OUTPUT_CONFIG_SCHEMA(_output_config("holding", 3))
+
+
+def test_even_offset_on_holding_output_accepted() -> None:
+    config = OUTPUT_CONFIG_SCHEMA(_output_config("holding", 2))
+    assert config[CONF_OFFSET] == 2
+
+
+def test_odd_offset_on_coil_output_accepted() -> None:
+    config = OUTPUT_CONFIG_SCHEMA(_output_config("coil", 3))
+    assert config[CONF_OFFSET] == 3

tests/integration/fixtures/uart_mock_modbus_register_offset.yaml

@@ -100,7 +100,7 @@ switch:
     offset: 2
     assumed_state: true
   # A holding-register switch that READS its state. Byte offset 6 -> register 0x10 + 6/2 = 0x13. Post-fix
-  # the switch itself resolves to 0x13 (whole registers fold into the address, residual byte stays) and
+  # the switch itself resolves to 0x13 (the even byte offset folds into the address as whole registers) and
   # joins the 0x10..0x13 range, so no separate 0x13 sensor is needed. Pre-fix the whole byte offset folds
   # into the address (0x16), where the server answers ILLEGAL_DATA_ADDRESS and the switch never publishes.
   - platform: modbus_controller

tests/integration/test_uart_mock_modbus.py

@@ -967,13 +967,6 @@ async def test_uart_mock_modbus_client_read_write(
         _assert_no_modbus_errors(error_log_lines, warning_log_lines)
 
 
-@pytest.mark.xfail(
-    strict=True,
-    reason="Byte-accurate register-offset writes land in the follow-up offset fix; "
-    "until then the byte offset is folded into the address (writes 0x12 instead of "
-    "0x11). The write and read assertions both flip via the same switch-constructor "
-    "fold. Remove this marker when that change merges.",
-)
 @pytest.mark.asyncio
 async def test_uart_mock_modbus_register_offset(
     yaml_config: str,