From 1e7c48e2cfc6eac01ec515b34be57cde768d67d4 Mon Sep 17 00:00:00 2001 From: Bonne Eggleston Date: Wed, 26 Aug 2026 06:35:46 -0700 Subject: [PATCH] [modbus_controller] Add integration tests for register offset, response size and write buffer (#18741) --- ...t_mock_modbus_deprecated_write_buffer.yaml | 106 ++++++++++++++ .../uart_mock_modbus_register_offset.yaml | 138 ++++++++++++++++++ tests/integration/test_uart_mock_modbus.py | 119 ++++++++++++++- 3 files changed, 362 insertions(+), 1 deletion(-) create mode 100644 tests/integration/fixtures/uart_mock_modbus_deprecated_write_buffer.yaml create mode 100644 tests/integration/fixtures/uart_mock_modbus_register_offset.yaml diff --git a/tests/integration/fixtures/uart_mock_modbus_deprecated_write_buffer.yaml b/tests/integration/fixtures/uart_mock_modbus_deprecated_write_buffer.yaml new file mode 100644 index 0000000000..f378e3de43 --- /dev/null +++ b/tests/integration/fixtures/uart_mock_modbus_deprecated_write_buffer.yaml @@ -0,0 +1,106 @@ +esphome: + name: uart-mock-modbus-dep-buffer + +host: +api: +logger: + level: VERBOSE + +external_components: + - source: + type: local + path: EXTERNAL_COMPONENT_PATH + +# Dummy uart entry to satisfy modbus's DEPENDENCIES = ["uart"] +# The actual UART bus used is the uart_mock component below +uart: + baud_rate: 115200 + port: /dev/null + +uart_mock: + - id: virtual_uart_server + baud_rate: 9600 + auto_start: true + debug: + on_tx: + - then: + - uart_mock.inject_rx: + id: virtual_uart_controller + data: !lambda return data; + - id: virtual_uart_controller + baud_rate: 9600 + auto_start: true + debug: + on_tx: + - then: + - uart_mock.inject_rx: + id: virtual_uart_server + data: !lambda return data; + +globals: + - id: reg10 + type: uint16_t + initial_value: "0" + +modbus: + - uart_id: virtual_uart_server + id: virtual_modbus_server + role: server + - uart_id: virtual_uart_controller + id: virtual_modbus_controller + role: client + turnaround_time: 10ms + +modbus_controller: + - address: 1 + modbus_id: virtual_modbus_controller + id: modbus_controller_1 + update_interval: 1s + +modbus_server: + - address: 1 + modbus_id: virtual_modbus_server + id: modbus_server_1 + registers: + - address: 0x10 + value_type: U_WORD + read_lambda: return id(reg10); + write_lambda: |- + id(reg10) = x; + return true; + +# A number whose write_lambda uses the DEPRECATED buffer parameter (fills `payload` with a legacy raw +# frame as words: device address + function code + data) instead of the new item->write_* API. The write +# must still land with its legacy semantics, and the one-time deprecation warning must fire only once per +# entity no matter how many writes happen. +number: + - platform: modbus_controller + modbus_controller_id: modbus_controller_1 + name: "buf_number" + id: buf_number + address: 0x10 + register_type: holding + value_type: U_WORD + min_value: 0 + max_value: 1000 + step: 1 + write_lambda: |- + // Legacy raw frame as words: [addr 0x01 | fc 0x06], register 0x0010, value. + payload.push_back(0x0106); + payload.push_back(0x0010); + payload.push_back((uint16_t) x); + return {}; + +# Reports the server-side register so the test can observe that the deprecated buffer write landed. +sensor: + - platform: template + name: "written_value" + id: written_value + update_interval: 0.5s + lambda: "return id(reg10);" + +button: + - platform: template + name: "Start Scenario" + id: start_scenario_btn + # The test drives the writes via number_command; the mock is autostart. diff --git a/tests/integration/fixtures/uart_mock_modbus_register_offset.yaml b/tests/integration/fixtures/uart_mock_modbus_register_offset.yaml new file mode 100644 index 0000000000..e93e78d5a3 --- /dev/null +++ b/tests/integration/fixtures/uart_mock_modbus_register_offset.yaml @@ -0,0 +1,138 @@ +esphome: + name: uart-mock-modbus-reg-offset + +host: +api: +logger: + level: VERBOSE + +external_components: + - source: + type: local + path: EXTERNAL_COMPONENT_PATH + +# Dummy uart entry to satisfy modbus's DEPENDENCIES = ["uart"] +# The actual UART bus used is the uart_mock component below +uart: + baud_rate: 115200 + port: /dev/null + +uart_mock: + - id: virtual_uart_server + baud_rate: 9600 + auto_start: true + debug: + on_tx: + - then: + - uart_mock.inject_rx: + id: virtual_uart_controller + data: !lambda return data; + - id: virtual_uart_controller + baud_rate: 9600 + auto_start: true + debug: + on_tx: + - then: + - uart_mock.inject_rx: + id: virtual_uart_server + data: !lambda return data; + +globals: + - id: reg10 + type: uint16_t + initial_value: "100" + - id: reg11 + type: uint16_t + initial_value: "200" + - id: reg12 + type: uint16_t + initial_value: "300" + - id: reg13 + type: uint16_t + initial_value: "0xABCD" + +modbus: + - uart_id: virtual_uart_server + id: virtual_modbus_server + role: server + - uart_id: virtual_uart_controller + id: virtual_modbus_controller + role: client + turnaround_time: 10ms + +modbus_controller: + - address: 1 + modbus_id: virtual_modbus_controller + id: modbus_controller_1 + update_interval: 1s + +modbus_server: + - address: 1 + modbus_id: virtual_modbus_server + id: modbus_server_1 + registers: + - address: 0x10 + value_type: U_WORD + read_lambda: return id(reg10); + write_lambda: id(reg10) = x; return true; + - address: 0x11 + value_type: U_WORD + read_lambda: return id(reg11); + write_lambda: id(reg11) = x; return true; + - address: 0x12 + value_type: U_WORD + read_lambda: return id(reg12); + write_lambda: id(reg12) = x; return true; + - address: 0x13 + value_type: U_WORD + read_lambda: return id(reg13); + write_lambda: id(reg13) = x; return true; + +# A holding-register switch at 0x10 with a 2-BYTE offset. offset is byte-based, so the write must target +# register 0x10 + 2/2 = 0x11. The old (pre-fix) behavior folded offset into the address as a register +# count, hitting 0x12 instead. assumed_state keeps the switch write-only so it does not read any register. +switch: + - platform: modbus_controller + modbus_controller_id: modbus_controller_1 + name: "offset_switch" + register_type: holding + address: 0x10 + 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 + # 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 + modbus_controller_id: modbus_controller_1 + name: "read_offset_switch" + register_type: holding + address: 0x10 + offset: 6 + bitmask: 0x1 + +sensor: + - platform: modbus_controller + modbus_controller_id: modbus_controller_1 + name: "reg_10" + address: 0x10 + register_type: holding + value_type: U_WORD + - platform: modbus_controller + modbus_controller_id: modbus_controller_1 + name: "reg_11" + address: 0x11 + register_type: holding + value_type: U_WORD + - platform: modbus_controller + modbus_controller_id: modbus_controller_1 + name: "reg_12" + address: 0x12 + register_type: holding + value_type: U_WORD + +button: + - platform: template + name: "Start Scenario" + id: start_scenario_btn + # This test does not have anything to start (mock is autostart) diff --git a/tests/integration/test_uart_mock_modbus.py b/tests/integration/test_uart_mock_modbus.py index c84fb34e70..707637cfc2 100644 --- a/tests/integration/test_uart_mock_modbus.py +++ b/tests/integration/test_uart_mock_modbus.py @@ -24,7 +24,7 @@ from dataclasses import dataclass from aioesphomeapi import ButtonInfo, NumberInfo, SwitchInfo import pytest -from .state_utils import SensorTracker, find_entity +from .state_utils import SensorTracker, find_entity, wait_for_state from .types import APIClientConnectedFactory, RunCompiledFunction @@ -965,3 +965,120 @@ async def test_uart_mock_modbus_client_read_write( await tracker.setup_and_start_scenario(client) await tracker.await_all(futures) _assert_no_modbus_errors(error_log_lines, warning_log_lines) + + +@pytest.mark.xfail( + strict=True, + reason="Byte-accurate register-offset writes require the modbus_controller " + "entity-device change; on dev 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, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """Test that a byte offset on a holding-register write is byte-accurate. + + `offset` is a byte offset, so a holding-register write at address 0x10 with offset: 2 must target + register 0x10 + 2/2 = 0x11. The pre-fix behavior folded the byte offset into the address as a register + count (0x10 + 2 = 0x12). The switch is assumed_state (write-only), so reg_11 turning 0xFFFF pins the + fix; had the write landed on 0x12 the wait would time out and reg_12 would change instead. + """ + + tracker = SensorTracker(["reg_10", "reg_11", "reg_12"]) + initial = tracker.expect_all({"reg_10": 100, "reg_11": 200, "reg_12": 300}) + wrote_11 = tracker.expect("reg_11", 65535) + + async with ( + run_compiled(yaml_config), + api_client_connected() as client, + ): + entities = await tracker.setup_and_start_scenario(client) + await tracker.await_all(initial, timeout=4.0) + + switch = find_entity(entities, "offset_switch", SwitchInfo) + assert switch is not None, "offset_switch not found" + client.switch_command(switch.key, True) + + # reg_11 (0x10 + offset 2/2) must receive the write; if the write went to 0x12 this times out. + await tracker.await_change(wrote_11, "reg_11", timeout=4.0) + # And 0x12 (the pre-fix register-offset target) must be untouched. + assert tracker.sensor_states["reg_12"][-1] == 300, ( + "reg_12 (0x12) should be untouched - offset is byte-based, so the write targets 0x11; " + f"got {tracker.sensor_states['reg_12']}" + ) + + # Read path: read_offset_switch has byte offset 6. Post-fix the switch folds the whole registers + # into its address (0x10 + 6/2 = 0x13, residual byte 0) and joins the 0x10..0x13 range, so the + # read lands in-bounds on 0xABCD (bit 0 set) -> ON. Pre-fix the whole byte offset folded into the + # address (0x16); the server answers ILLEGAL_DATA_ADDRESS there and the switch never publishes. + read_switch = find_entity(entities, "read_offset_switch", SwitchInfo) + assert read_switch is not None, "read_offset_switch not found" + # The ON transition happened at the first poll and switch states are deduped, so this relies on + # wait_for_state's fresh subscribe_states re-dumping every entity's current state. + await wait_for_state( + client, + lambda s: ( + getattr(s, "key", None) == read_switch.key + and getattr(s, "state", None) is True + ), + timeout=6.0, + ) + + +@pytest.mark.xfail( + strict=True, + reason="The deprecated write buffer requires the modbus_controller " + "entity-device change; on dev a nullopt-returning write_lambda early-returns " + "before the buffer is used, so the write never happens. The warn-once " + "assertion matches the log substring 'write_lambda buffer'. Remove this " + "marker when that change merges.", +) +@pytest.mark.asyncio +async def test_uart_mock_modbus_deprecated_write_buffer( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """Test the deprecated write_lambda buffer path still works, and warns once per entity. + + buf_number's write_lambda fills the old `payload` buffer with a legacy raw frame as words (device + address + function code + data) instead of calling item->write_*. Two writes must both land with the + legacy raw-frame semantics, and the one-time deprecation warning must fire exactly once per entity + regardless of how many writes happen. + """ + + warn_count = 0 + + def line_callback(line: str) -> None: + nonlocal warn_count + if "write_lambda buffer" in line: + warn_count += 1 + + tracker = SensorTracker(["written_value"]) + + async with ( + run_compiled(yaml_config, line_callback=line_callback), + api_client_connected() as client, + ): + entities = await tracker.setup_and_start_scenario(client) + number = find_entity(entities, "buf_number", NumberInfo) + assert number is not None, "buf_number not found" + + # First write via the deprecated buffer path. + client.number_command(number.key, 111) + await tracker.await_change( + tracker.expect("written_value", 111), "written_value", timeout=4.0 + ) + # Second write: lands too, but must not warn again (warn-once per entity). + client.number_command(number.key, 222) + await tracker.await_change( + tracker.expect("written_value", 222), "written_value", timeout=4.0 + ) + + assert warn_count == 1, ( + f"deprecation warning should fire exactly once per entity, got {warn_count}" + )