[modbus_controller] Switch write_lambda return value is the wire value only (#18788)

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

esphome/components/modbus_controller/switch/modbus_switch.cpp

@@ -65,6 +65,7 @@ void ModbusSwitch::write_state(bool state) {
   // so a rapidly-changing value writes the latest, not every intermediate.
   this->clear_tx_queue_for_device();
   modbus::helpers::PduBuffer data;
+  bool write_value = state;
   if (this->write_transform_func_.has_value()) {
     // The lambda may drive the write itself via item->write_*/queue_pdu(), override the written value (return a
     // value), or (deprecated) fill `data` with a custom PDU.
@@ -92,26 +93,29 @@ void ModbusSwitch::write_state(bool state) {
       ESP_LOGV(TAG, "Communication handled by lambda - exiting control");
       return;
     }
+    // The returned bool is the wire value only; the entity still reports the requested state. A polled
+    // entity needs the read lambda inverted to match, or the next poll flips the display back.
     ESP_LOGV(TAG, "Value overwritten by lambda");
-    state = val.value();
+    write_value = val.value();
   }
-  ESP_LOGV(TAG, "write_state '%s': new value = %s type = %d address = %X offset = %x", this->get_name().c_str(),
-           ONOFF(state), (int) this->register_type, this->start_address, this->offset);
+  ESP_LOGV(TAG, "write_state '%s': new value = %s (wire = %s) type = %d address = %X offset = %x",
+           this->get_name().c_str(), ONOFF(state), ONOFF(write_value), (int) this->register_type, this->start_address,
+           this->offset);
   bool queued;
   if (this->register_type == EntityType::COIL) {
     // offset for coil and discrete inputs is the coil/register number not bytes
     if (this->use_write_multiple_) {
-      std::array<bool, 1> states{state};
+      std::array<bool, 1> states{write_value};
       queued = this->write_multiple_coils(this->write_address(), states);
     } else {
-      queued = this->write_single_coil(this->write_address(), state);
+      queued = this->write_single_coil(this->write_address(), write_value);
     }
   } else {
     if (this->use_write_multiple_) {
-      std::array<uint16_t, 1> states{static_cast<uint16_t>(state ? (0xFFFF & this->bitmask) : 0)};
+      std::array<uint16_t, 1> states{static_cast<uint16_t>(write_value ? (0xFFFF & this->bitmask) : 0)};
       queued = this->write_multiple_registers(this->write_address(), states);
     } else {
-      queued = this->write_single_register(this->write_address(), state ? 0xFFFF & this->bitmask : 0u);
+      queued = this->write_single_register(this->write_address(), write_value ? 0xFFFF & this->bitmask : 0u);
     }
   }
   if (!queued) {

tests/integration/fixtures/uart_mock_modbus_lambda_invert.yaml

@@ -0,0 +1,95 @@
+esphome:
+  name: uart-mock-modbus-lambda-invert
+
+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: reg40
+    type: uint16_t
+    initial_value: "5"
+
+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: 0x40
+        value_type: U_WORD
+        read_lambda: return id(reg40);
+        write_lambda: id(reg40) = x; return true;
+
+# An active-low holding switch: the write_lambda inverts the wire value, but the entity must still
+# report the REQUESTED state. assumed_state keeps the register unpolled, so the published state comes
+# only from write_state() - turning ON writes 0x0000 yet the switch shows ON.
+switch:
+  - platform: modbus_controller
+    modbus_controller_id: modbus_controller_1
+    name: "invert_switch"
+    register_type: holding
+    address: 0x40
+    assumed_state: true
+    write_lambda: |-
+      return !x;
+
+sensor:
+  - platform: modbus_controller
+    modbus_controller_id: modbus_controller_1
+    name: "reg_40"
+    address: 0x40
+    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)

tests/integration/test_uart_mock_modbus.py

@@ -1058,6 +1058,61 @@ async def test_uart_mock_modbus_lambda_write(
         await tracker.await_change(wrote_30, "reg_30", timeout=4.0)
 
 
+@pytest.mark.asyncio
+async def test_uart_mock_modbus_lambda_invert(
+    yaml_config: str,
+    run_compiled: RunCompiledFunction,
+    api_client_connected: APIClientConnectedFactory,
+) -> None:
+    """Test that a write_lambda's return value is the wire value only.
+
+    `invert_switch` is an active-low holding switch whose write_lambda returns !x. Turning it ON must
+    write 0x0000 to the register (observed through the independent reg_40 sensor) while the entity
+    reports ON - the requested state, not the inverted wire value. Turning it OFF writes 0xFFFF and
+    reports OFF. The switch is assumed_state, so the published state comes only from write_state().
+    """
+
+    tracker = SensorTracker(["reg_40"])
+    initial = tracker.expect("reg_40", 5)
+    wrote_on = tracker.expect("reg_40", 0)
+    wrote_off = tracker.expect("reg_40", 65535)
+
+    async with (
+        run_compiled(yaml_config),
+        api_client_connected() as client,
+    ):
+        entities = await tracker.setup_and_start_scenario(client)
+        await tracker.await_change(initial, "reg_40", timeout=4.0)
+
+        switch = find_entity(entities, "invert_switch", SwitchInfo)
+        assert switch is not None, "invert_switch not found"
+
+        client.switch_command(switch.key, True)
+        # The wire byte carries the inverted value...
+        await tracker.await_change(wrote_on, "reg_40", timeout=4.0)
+        # ...while the entity reports the requested state. 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) == switch.key
+                and getattr(s, "state", None) is True
+            ),
+            timeout=6.0,
+        )
+
+        client.switch_command(switch.key, False)
+        await tracker.await_change(wrote_off, "reg_40", timeout=4.0)
+        await wait_for_state(
+            client,
+            lambda s: (
+                getattr(s, "key", None) == switch.key
+                and getattr(s, "state", None) is False
+            ),
+            timeout=6.0,
+        )
+
+
 @pytest.mark.asyncio
 async def test_uart_mock_modbus_deprecated_write_buffer(
     yaml_config: str,