diff --git a/esphome/components/modbus_controller/__init__.py b/esphome/components/modbus_controller/__init__.py index e87eccb32c..c390d8ab79 100644 --- a/esphome/components/modbus_controller/__init__.py +++ b/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 diff --git a/esphome/components/modbus_controller/output/__init__.py b/esphome/components/modbus_controller/output/__init__.py index 34a0f488ec..c2055fa690 100644 --- a/esphome/components/modbus_controller/output/__init__.py +++ b/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, diff --git a/esphome/components/modbus_controller/output/modbus_output.h b/esphome/components/modbus_controller/output/modbus_output.h index b942dcea62..48153dc0b7 100644 --- a/esphome/components/modbus_controller/output/modbus_output.h +++ b/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; diff --git a/esphome/components/modbus_controller/switch/__init__.py b/esphome/components/modbus_controller/switch/__init__.py index c52067f941..49dc0bb222 100644 --- a/esphome/components/modbus_controller/switch/__init__.py +++ b/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 diff --git a/esphome/components/modbus_controller/switch/modbus_switch.h b/esphome/components/modbus_controller/switch/modbus_switch.h index 1d3d03919f..bd1c837080 100644 --- a/esphome/components/modbus_controller/switch/modbus_switch.h +++ b/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; diff --git a/tests/component_tests/modbus_controller/test_write_offset.py b/tests/component_tests/modbus_controller/test_write_offset.py new file mode 100644 index 0000000000..72bd3a933e --- /dev/null +++ b/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 diff --git a/tests/integration/fixtures/uart_mock_modbus_register_offset.yaml b/tests/integration/fixtures/uart_mock_modbus_register_offset.yaml index e93e78d5a3..21c451aa99 100644 --- a/tests/integration/fixtures/uart_mock_modbus_register_offset.yaml +++ b/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 diff --git a/tests/integration/test_uart_mock_modbus.py b/tests/integration/test_uart_mock_modbus.py index 3dfeda9b37..ae9a481630 100644 --- a/tests/integration/test_uart_mock_modbus.py +++ b/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,