[modbus] Fix parsing & split out server mode (#11969)

tests/components/modbus/modbus_helpers_test.cpp

@@ -4,6 +4,181 @@
 
 namespace esphome::modbus::helpers {
 
+using FC = ModbusFunctionCode;
+
+// --- server_frame_length ---------------------------------------------------
+// Frame layout: address(1) + function(1) + ... + CRC(2). Fixtures borrowed from
+// tests/integration/fixtures/uart_mock_modbus.yaml.
+
+TEST(ModbusServerFrameLength, TooShortReturnsMinimum) {
+  const uint8_t frame[] = {0x01};
+  EXPECT_EQ(server_frame_length(frame, 1), MIN_FRAME_SIZE);
+}
+
+TEST(ModbusServerFrameLength, ReadHoldingUsesByteCount) {
+  // inject_rx for basic_register: 2 data bytes -> 5 + 2 = 7
+  const uint8_t frame[] = {0x01, 0x03, 0x02, 0x01, 0x03, 0xF9, 0xD5};
+  EXPECT_EQ(server_frame_length(frame, sizeof(frame)), 7);
+}
+
+TEST(ModbusServerFrameLength, ReadByteCountCappedAtMax) {
+  const uint8_t frame[] = {0x01, 0x03, 0xFF};  // claim 255 bytes
+  EXPECT_EQ(server_frame_length(frame, sizeof(frame)), 5 + MAX_NUM_OF_REGISTERS_TO_READ * 2);
+}
+
+TEST(ModbusServerFrameLength, ReadMissingByteCountReturnsHeaderOnly) {
+  const uint8_t frame[] = {0x01, 0x03};
+  EXPECT_EQ(server_frame_length(frame, sizeof(frame)), 5);
+}
+
+TEST(ModbusServerFrameLength, ExceptionResponse) {
+  // exception_response fixture: function code 0x83 has the exception bit set
+  const uint8_t frame[] = {0x01, 0x83, 0x02, 0xC0, 0xF1};
+  EXPECT_EQ(server_frame_length(frame, sizeof(frame)), 5);
+}
+
+TEST(ModbusServerFrameLength, WriteResponsesAreFixed) {
+  for (FC fc :
+       {FC::WRITE_SINGLE_COIL, FC::WRITE_SINGLE_REGISTER, FC::WRITE_MULTIPLE_COILS, FC::WRITE_MULTIPLE_REGISTERS}) {
+    const uint8_t frame[] = {0x01, static_cast<uint8_t>(fc)};
+    EXPECT_EQ(server_frame_length(frame, sizeof(frame)), 8) << "fc=" << static_cast<int>(fc);
+  }
+}
+
+TEST(ModbusServerFrameLength, MiscFixedAndUnknown) {
+  const uint8_t mask[] = {0x01, static_cast<uint8_t>(FC::MASK_WRITE_REGISTER)};
+  const uint8_t fifo[] = {0x01, static_cast<uint8_t>(FC::READ_FIFO_QUEUE)};
+  const uint8_t unknown[] = {0x01, 0x42};
+  EXPECT_EQ(server_frame_length(mask, sizeof(mask)), 10);
+  EXPECT_EQ(server_frame_length(fifo, sizeof(fifo)), 6);
+  EXPECT_EQ(server_frame_length(unknown, sizeof(unknown)), MIN_FRAME_SIZE);
+}
+
+// --- client_frame_length ---------------------------------------------------
+
+TEST(ModbusClientFrameLength, TooShortReturnsMinimum) {
+  const uint8_t frame[] = {0x01};
+  EXPECT_EQ(client_frame_length(frame, 1), MIN_FRAME_SIZE);
+}
+
+TEST(ModbusClientFrameLength, ReadAndWriteSingleAreFixed) {
+  // basic_register request fixture is a read-holding request -> 8 bytes
+  const uint8_t read[] = {0x01, 0x03, 0x00, 0x03, 0x00, 0x01, 0x74, 0x0A};
+  EXPECT_EQ(client_frame_length(read, sizeof(read)), 8);
+  for (FC fc : {FC::READ_COILS, FC::READ_DISCRETE_INPUTS, FC::READ_INPUT_REGISTERS, FC::WRITE_SINGLE_COIL,
+                FC::WRITE_SINGLE_REGISTER}) {
+    const uint8_t frame[] = {0x01, static_cast<uint8_t>(fc)};
+    EXPECT_EQ(client_frame_length(frame, sizeof(frame)), 8) << "fc=" << static_cast<int>(fc);
+  }
+}
+
+TEST(ModbusClientFrameLength, WriteMultipleUsesByteCount) {
+  // write 2 registers (4 data bytes): addr(2)+qty(2)+count(1) then data; count is frame[6]
+  const uint8_t frame[] = {0x01, 0x10, 0x00, 0x00, 0x00, 0x02, 0x04, 0x00, 0x0B, 0x00, 0x16};
+  EXPECT_EQ(client_frame_length(frame, sizeof(frame)), 9 + 4);
+}
+
+TEST(ModbusClientFrameLength, WriteMultipleByteCountCapped) {
+  const uint8_t frame[] = {0x01, 0x0F, 0x00, 0x00, 0x00, 0x02, 0xFF};
+  EXPECT_EQ(client_frame_length(frame, sizeof(frame)), 9 + MAX_NUM_OF_REGISTERS_TO_WRITE * 2);
+}
+
+TEST(ModbusClientFrameLength, WriteMultipleMissingByteCount) {
+  const uint8_t frame[] = {0x01, 0x10, 0x00, 0x00, 0x00, 0x02};
+  EXPECT_EQ(client_frame_length(frame, sizeof(frame)), 9);
+}
+
+TEST(ModbusClientFrameLength, MiscFixedAndUnknown) {
+  const uint8_t mask[] = {0x01, static_cast<uint8_t>(FC::MASK_WRITE_REGISTER)};
+  const uint8_t fifo[] = {0x01, static_cast<uint8_t>(FC::READ_FIFO_QUEUE)};
+  const uint8_t unknown[] = {0x01, 0x42};
+  EXPECT_EQ(client_frame_length(mask, sizeof(mask)), 10);
+  EXPECT_EQ(client_frame_length(fifo, sizeof(fifo)), 6);
+  EXPECT_EQ(client_frame_length(unknown, sizeof(unknown)), MIN_FRAME_SIZE);
+}
+
+// --- create_client_pdu -----------------------------------------------------
+// PDU = function code + data (no address, no CRC).
+
+TEST(ModbusCreateClientPdu, ReadHolding) {
+  auto pdu = create_client_pdu(FC::READ_HOLDING_REGISTERS, 0x0003, 1);
+  const std::vector<uint8_t> expected{0x03, 0x00, 0x03, 0x00, 0x01};
+  EXPECT_EQ(std::vector<uint8_t>(pdu.begin(), pdu.end()), expected);
+}
+
+TEST(ModbusCreateClientPdu, WriteSingleOmitsQuantity) {
+  const uint8_t values[] = {0x00, 0x0B};
+  auto pdu = create_client_pdu(FC::WRITE_SINGLE_REGISTER, 0x0003, 1, values, sizeof(values));
+  const std::vector<uint8_t> expected{0x06, 0x00, 0x03, 0x00, 0x0B};
+  EXPECT_EQ(std::vector<uint8_t>(pdu.begin(), pdu.end()), expected);
+}
+
+TEST(ModbusCreateClientPdu, WriteSingleTooFewValuesReturnsEmpty) {
+  const uint8_t values[] = {0x00};
+  auto pdu = create_client_pdu(FC::WRITE_SINGLE_COIL, 0x0003, 1, values, sizeof(values));
+  EXPECT_TRUE(pdu.empty());
+}
+
+TEST(ModbusCreateClientPdu, WriteMultipleIncludesByteCount) {
+  const uint8_t values[] = {0x00, 0x0B, 0x00, 0x16};
+  auto pdu = create_client_pdu(FC::WRITE_MULTIPLE_REGISTERS, 0x0000, 2, values, sizeof(values));
+  const std::vector<uint8_t> expected{0x10, 0x00, 0x00, 0x00, 0x02, 0x04, 0x00, 0x0B, 0x00, 0x16};
+  EXPECT_EQ(std::vector<uint8_t>(pdu.begin(), pdu.end()), expected);
+}
+
+TEST(ModbusCreateClientPdu, WriteMultipleOverCapacityReturnsEmpty) {
+  std::vector<uint8_t> values(MAX_PDU_SIZE - 6 + 1, 0xAA);
+  auto pdu = create_client_pdu(FC::WRITE_MULTIPLE_REGISTERS, 0x0000, 1, values.data(), values.size());
+  EXPECT_TRUE(pdu.empty());
+}
+
+TEST(ModbusCreateClientPdu, UnsupportedFunctionCodeReturnsEmpty) {
+  auto pdu = create_client_pdu(FC::READ_FIFO_QUEUE, 0x0000, 1);
+  EXPECT_TRUE(pdu.empty());
+}
+
+TEST(ModbusCreateClientPdu, ZeroEntitiesReturnsEmpty) {
+  auto pdu = create_client_pdu(FC::READ_HOLDING_REGISTERS, 0x0000, 0);
+  EXPECT_TRUE(pdu.empty());
+}
+
+TEST(ModbusCreateClientPdu, WriteWithoutValuesReturnsEmpty) {
+  auto pdu = create_client_pdu(FC::WRITE_MULTIPLE_REGISTERS, 0x0000, 1, nullptr, 0);
+  EXPECT_TRUE(pdu.empty());
+}
+
+TEST(ModbusCreateClientPdu, ReadHoldingOverMaxReturnsEmpty) {
+  auto pdu = create_client_pdu(FC::READ_HOLDING_REGISTERS, 0x0000, MAX_NUM_OF_REGISTERS_TO_READ + 1);
+  EXPECT_TRUE(pdu.empty());
+}
+
+// Regression: coils allow up to 2000 entities, well above the 125 register limit.
+// A switch fall-through previously subjected coil/discrete reads to the register limit.
+TEST(ModbusCreateClientPdu, ReadCoilsAboveRegisterLimitIsValid) {
+  const uint16_t quantity = MAX_NUM_OF_REGISTERS_TO_READ + 1;  // 126: valid for coils, too many for registers
+  auto pdu = create_client_pdu(FC::READ_COILS, 0x0000, quantity);
+  const std::vector<uint8_t> expected{0x01, 0x00, 0x00, static_cast<uint8_t>(quantity >> 8),
+                                      static_cast<uint8_t>(quantity & 0xFF)};
+  EXPECT_EQ(std::vector<uint8_t>(pdu.begin(), pdu.end()), expected);
+}
+
+TEST(ModbusCreateClientPdu, ReadCoilsOverMaxReturnsEmpty) {
+  auto pdu = create_client_pdu(FC::READ_COILS, 0x0000, MAX_NUM_OF_COILS_TO_READ + 1);
+  EXPECT_TRUE(pdu.empty());
+}
+
+TEST(ModbusCreateClientPdu, ReadDiscreteInputsOverMaxReturnsEmpty) {
+  auto pdu = create_client_pdu(FC::READ_DISCRETE_INPUTS, 0x0000, MAX_NUM_OF_DISCRETE_INPUTS_TO_READ + 1);
+  EXPECT_TRUE(pdu.empty());
+}
+
+TEST(ModbusCreateClientPdu, WriteMultipleOverEntityLimitReturnsEmpty) {
+  const uint8_t values[] = {0x00, 0x0B};
+  auto pdu = create_client_pdu(FC::WRITE_MULTIPLE_REGISTERS, 0x0000, MAX_NUM_OF_REGISTERS_TO_WRITE + 1, values,
+                               sizeof(values));
+  EXPECT_TRUE(pdu.empty());
+}
+
 TEST(ModbusHelpersTest, PayloadToNumberRejectsOffsetAtEndOfBuffer) {
   const std::vector<uint8_t> data{0x12, 0x34};
   EXPECT_EQ(payload_to_number(data, SensorValueType::U_WORD, 2, 0xFFFFFFFF), 0);

tests/components/modbus/modbus_test.cpp

@@ -1,59 +0,0 @@
-#include <gtest/gtest.h>
-#include "esphome/components/modbus/modbus.h"
-#include "esphome/core/helpers.h"
-
-namespace esphome::modbus {
-
-// Exposes protected methods for testing.
-class TestModbus : public Modbus {
- public:
-  bool test_parse_modbus_byte(uint8_t byte) { return this->parse_modbus_byte_(byte); }
-  void test_clear_rx_buffer() { this->rx_buffer_.clear(); }
-  void set_waiting(uint8_t addr) { this->waiting_for_response_ = addr; }
-};
-
-class MockDevice : public ModbusDevice {
- public:
-  void on_modbus_data(const std::vector<uint8_t> &data) override { this->data_received = true; }
-  bool data_received{false};
-};
-
-TEST(ModbusTest, TwoByteRegressionTest) {
-  TestModbus modbus;
-  modbus.set_role(ModbusRole::CLIENT);
-  // First byte (at=0)
-  EXPECT_TRUE(modbus.test_parse_modbus_byte(0x01));
-  // Second byte (at=1)
-  // This used to reach raw[2] because it skipped the if(at==2) check, causing a
-  // buffer overflow.
-  EXPECT_TRUE(modbus.test_parse_modbus_byte(0x03));
-}
-
-TEST(ModbusTest, TestValidFrame) {
-  TestModbus modbus;
-  modbus.set_role(ModbusRole::CLIENT);
-
-  MockDevice device;
-  device.set_parent(&modbus);
-  device.set_address(0x01);
-  modbus.register_device(&device);
-  modbus.set_waiting(0x01);
-
-  // Address 1, Function 3, Length 2, Data 0x1234
-  uint8_t frame_data[] = {0x01, 0x03, 0x02, 0x12, 0x34};
-  uint16_t crc = esphome::crc16(frame_data, sizeof(frame_data));
-
-  std::vector<uint8_t> frame;
-  for (uint8_t b : frame_data)
-    frame.push_back(b);
-  frame.push_back(crc & 0xFF);
-  frame.push_back((crc >> 8) & 0xFF);
-
-  for (size_t i = 0; i < frame.size(); i++) {
-    bool result = modbus.test_parse_modbus_byte(frame[i]);
-    EXPECT_TRUE(result) << "Failed at byte " << i << " (0x" << std::hex << (int) frame[i] << ")";
-  }
-  EXPECT_TRUE(device.data_received);
-}
-
-}  // namespace esphome::modbus

tests/integration/fixtures/uart_mock_modbus.yaml

@@ -49,15 +49,16 @@ modbus_controller:
   - address: 1
     id: modbus_controller_ok
     max_cmd_retries: 2
-    update_interval: 1s
+    # Update interval is set to never to prevent automatic polling: the test will trigger requests by pressing the "Start Scenario" button
+    update_interval: never
   - address: 2
     id: modbus_controller_slow
     max_cmd_retries: 0
-    update_interval: 1s
+    update_interval: never
   - address: 3
     id: modbus_controller_offline
     max_cmd_retries: 0
-    update_interval: 1s
+    update_interval: never
 
 sensor:
   - platform: modbus_controller
@@ -91,4 +92,11 @@ button:
     name: "Start Scenario"
     id: start_scenario_btn
     on_press:
-      - lambda: "id(virtual_uart_dev).start_scenario();"
+      - lambda: |-
+          id(virtual_uart_dev).start_scenario();
+          id(modbus_controller_ok).set_update_interval(1000);
+          id(modbus_controller_ok).start_poller();
+          id(modbus_controller_slow).set_update_interval(1000);
+          id(modbus_controller_slow).start_poller();
+          id(modbus_controller_offline).set_update_interval(1000);
+          id(modbus_controller_offline).start_poller();

tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml

@@ -54,7 +54,11 @@ modbus:
 sensor:
   - platform: sdm_meter
     address: 2
-    update_interval: 1s
+    id: sdm_meter_1
+    # update_interval is set to never to avoid automatic polling before the test starts the scenario.
+    # The test will manually start the poller after subscribing to states, to ensure no state changes are missed.
+    # This also allows us to assert there are no modbus errors/warnings during the initial request/response.
+    update_interval: never
     phase_a:
       voltage:
         name: sdm_voltage
@@ -64,4 +68,7 @@ button:
     name: "Start Scenario"
     id: start_scenario_btn
     on_press:
-      - lambda: "id(virtual_uart_dev).start_scenario();"
+      - lambda: |-
+          id(virtual_uart_dev).start_scenario();
+          id(sdm_meter_1).set_update_interval(1000);
+          id(sdm_meter_1).start_poller();

tests/integration/fixtures/uart_mock_modbus_server_controller.yaml

@@ -53,8 +53,8 @@ modbus:
 modbus_controller:
   - address: 1
     modbus_id: virtual_modbus_controller
-    update_interval: 1s
     id: modbus_controller_1
+    update_interval: 1s
 
 modbus_server:
   - address: 1
@@ -176,6 +176,4 @@ button:
   - platform: template
     name: "Start Scenario"
     id: start_scenario_btn
-    on_press:
-      - lambda: "id(virtual_uart_server).start_scenario();"
-      - lambda: "id(virtual_uart_controller).start_scenario();"
+    # This test does not have anything to start (mock is autostart)

tests/integration/fixtures/uart_mock_modbus_server_controller_multiple.yaml

@@ -113,7 +113,4 @@ button:
   - platform: template
     name: "Start Scenario"
     id: start_scenario_btn
-    on_press:
-      - lambda: "id(virtual_uart_server).start_scenario();"
-      - lambda: "id(virtual_uart_server_2).start_scenario();"
-      - lambda: "id(virtual_uart_controller).start_scenario();"
+    # This test does not have anything to start (mock is autostart)

tests/integration/fixtures/uart_mock_modbus_server_controller_write.yaml

@@ -326,6 +326,4 @@ button:
   - platform: template
     name: "Start Scenario"
     id: start_scenario_btn
-    on_press:
-      - lambda: "id(virtual_uart_server).start_scenario();"
-      - lambda: "id(virtual_uart_controller).start_scenario();"
+    # This test does not have anything to start (mock is autostart)

tests/integration/fixtures/uart_mock_modbus_timing.yaml

@@ -53,7 +53,11 @@ modbus:
 sensor:
   - platform: sdm_meter
     address: 2
-    update_interval: 1s
+    id: sdm_meter_1
+    # update_interval is set to never to avoid automatic polling before the test starts the scenario.
+    # The test will manually start the poller after subscribing to states, to ensure no state changes are missed.
+    # This also allows us to assert there are no modbus errors/warnings during the initial request/response.
+    update_interval: never
     phase_a:
       voltage:
         name: sdm_voltage
@@ -63,4 +67,7 @@ button:
     name: "Start Scenario"
     id: start_scenario_btn
     on_press:
-      - lambda: "id(virtual_uart_dev).start_scenario();"
+      - lambda: |-
+          id(virtual_uart_dev).start_scenario();
+          id(sdm_meter_1).set_update_interval(1000);
+          id(sdm_meter_1).start_poller();

tests/integration/test_uart_mock_modbus.py

@@ -127,15 +127,18 @@ async def test_uart_mock_modbus_timing(
 ) -> None:
     """Test modbus timing with multi-register SDM meter response."""
 
+    line_callback, error_log_lines, warning_log_lines = _make_modbus_line_callback()
+
     tracker = SensorTracker(["sdm_voltage"])
     voltage_changed = tracker.expect_any("sdm_voltage")
 
     async with (
-        run_compiled(yaml_config),
+        run_compiled(yaml_config, line_callback=line_callback),
         api_client_connected() as client,
     ):
         await tracker.setup_and_start_scenario(client)
         await tracker.await_change(voltage_changed, "sdm_voltage")
+        _assert_no_modbus_errors(error_log_lines, warning_log_lines)
 
 
 @pytest.mark.asyncio
@@ -148,26 +151,25 @@ async def test_uart_mock_modbus_no_threshold(
 
     Without the 50ms fallback timeout, the chunked response with a 40ms gap
     between USB packets would cause a false timeout and CRC failure cascade.
-    Bus-level warnings (CRC failures, buffer clears) are expected during
-    chunked reassembly — the test only verifies the final value arrives.
+    Bus-level warnings (CRC/parse failures, buffer clears) are NOT expected during
+    chunked reassembly, if timeouts are set properly — these warnings indicate undersized timeouts.
     """
 
+    line_callback, error_log_lines, warning_log_lines = _make_modbus_line_callback()
+
     tracker = SensorTracker(["sdm_voltage"])
     voltage_changed = tracker.expect_any("sdm_voltage")
 
     async with (
-        run_compiled(yaml_config),
+        run_compiled(yaml_config, line_callback=line_callback),
         api_client_connected() as client,
     ):
         await tracker.setup_and_start_scenario(client)
         await tracker.await_change(voltage_changed, "sdm_voltage")
+        _assert_no_modbus_errors(error_log_lines, warning_log_lines)
 
 
 @pytest.mark.asyncio
-@pytest.mark.xfail(
-    reason="Modbus parser cannot handle server responses from other devices on the bus. Fix tracked in PR #11969.",
-    strict=True,
-)
 async def test_uart_mock_modbus_server(
     yaml_config: str,
     run_compiled: RunCompiledFunction,
@@ -308,10 +310,6 @@ async def test_uart_mock_modbus_server_controller_write(
 
 
 @pytest.mark.asyncio
-@pytest.mark.xfail(
-    reason="Modbus parser cannot handle server responses from other devices on the bus. Fix tracked in PR #11969.",
-    strict=True,
-)
 async def test_uart_mock_modbus_server_controller_multiple(
     yaml_config: str,
     run_compiled: RunCompiledFunction,