[modbus] CRC scan all unknown function codes (#18483)

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

esphome/components/modbus/modbus.cpp

@@ -219,14 +219,25 @@ void ModbusServerHub::parse_modbus_frames() {
     this->clear_rx_buffer_(LOG_STR("timeout after partial response"), true);
 }
 
-uint16_t Modbus::find_custom_frame_end_(uint16_t min_length) const {
+uint16_t Modbus::find_frame_end_by_crc_(uint16_t min_length) const {
-  // Custom functions could be any length - we have to rely on the CRC to determine completeness.
+  // Unknown-length functions (user-defined codes, unimplemented management codes, unassigned values)
+  // could be any length - we have to rely on the CRC to determine completeness.
   // If a CRC match is never found, the buffer will eventually overflow and be cleared.
   const uint8_t *raw = &this->rx_buffer_[0];
   const size_t size = this->rx_buffer_.size();
-  for (uint16_t len = min_length; len <= std::min(size, size_t(MAX_FRAME_SIZE)); len++) {
+  const auto max_len = static_cast<uint16_t>(std::min(size, size_t(MAX_FRAME_SIZE)));
-    if (crc16(raw, len) == 0)
+  if (min_length > max_len)
-      return len;
+    return 0;
+  // The Modbus CRC (poly 0xa001, refin/refout false) keeps its running state in the returned value,
+  // so we seed once over the first min_length bytes and extend one byte at a time instead of
+  // recomputing the whole prefix for every candidate length.
+  uint16_t crc = crc16(raw, min_length);
+  if (crc == 0)
+    return min_length;
+  for (uint16_t len = min_length; len < max_len; len++) {
+    crc = crc16(&raw[len], 1, crc);
+    if (crc == 0)
+      return len + 1;
   }
   return 0;
 }
@@ -241,11 +252,11 @@ bool Modbus::parse_modbus_server_frame_() {
   uint8_t address = this->rx_buffer_[0];
   uint8_t function_code = this->rx_buffer_[1];
 
-  if (helpers::is_function_code_custom(function_code)) {
+  if (helpers::is_function_code_unknown_length(function_code)) {
-    frame_length = this->find_custom_frame_end_(frame_length);
+    frame_length = this->find_frame_end_by_crc_(frame_length);
     if (frame_length == 0)
       return size < MAX_FRAME_SIZE;  // Continue to parse until we hit max size
-    ESP_LOGD(TAG, "User-defined function %02X found", function_code);
+    ESP_LOGD(TAG, "Unknown-length function %02X found", function_code);
   } else {
     if (crc16(&this->rx_buffer_[0], frame_length) != 0)
       return false;
@@ -272,11 +283,11 @@ bool ModbusServerHub::parse_modbus_client_frame_() {
   uint8_t address = this->rx_buffer_[0];
   uint8_t function_code = this->rx_buffer_[1];
 
-  if (helpers::is_function_code_custom(function_code)) {
+  if (helpers::is_function_code_unknown_length(function_code)) {
-    frame_length = this->find_custom_frame_end_(frame_length);
+    frame_length = this->find_frame_end_by_crc_(frame_length);
     if (frame_length == 0)
       return size < MAX_FRAME_SIZE;  // Continue to parse until we hit max size
-    ESP_LOGD(TAG, "User-defined function %02X found", function_code);
+    ESP_LOGD(TAG, "Unknown-length function %02X found", function_code);
   } else {
     if (crc16(&this->rx_buffer_[0], frame_length) != 0)
       return false;

esphome/components/modbus/modbus.h

@@ -82,7 +82,7 @@ class Modbus : public uart::UARTDevice, public Component {
   bool send_frame_(const ModbusFrame &frame);
   // Scans forward from min_length to find a frame boundary by CRC match for custom function codes.
   // Returns the matched frame length, or 0 if no valid CRC was found within MAX_FRAME_SIZE.
-  uint16_t find_custom_frame_end_(uint16_t min_length) const;
+  uint16_t find_frame_end_by_crc_(uint16_t min_length) const;
 
   uint32_t last_modbus_byte_{0};
   uint32_t last_receive_check_{0};

esphome/components/modbus/modbus_helpers.h

@@ -55,6 +55,38 @@ inline bool is_function_code_custom(uint8_t function_code) {
           masked_function_code <= FUNCTION_CODE_USER_DEFINED_SPACE_2_END);
 }
 
+/// True for any function code whose frame length the parsers cannot predict - everything the
+/// server_pdu_length()/client_pdu_length() switches fall through to `default` on (keep the case list
+/// in step with those switches). Deliberately wider than is_function_code_custom(): the user-defined
+/// ranges are unknown to the parser too, but so are the assigned-but-unimplemented codes
+/// (READ_EXCEPTION_STATUS, DIAGNOSTICS, GET_COMM_EVENT_*, REPORT_SERVER_ID) and every unassigned value.
+/// The 0x80 exception flag is masked off first, so a frame with it set classifies by its base code -
+/// even though a spec exception reply has a known 2-byte PDU. That is deliberate, matching what
+/// is_function_code_custom() has always done: some vendors use codes with the 0x80 bit set as ordinary
+/// codes with longer payloads, so the response parser CRC-scans these rather than assuming the spec
+/// length. For an intact spec exception the scan matches at its first candidate, so only a corrupt one
+/// pays (recovery by timeout instead of an immediate CRC failure).
+inline bool is_function_code_unknown_length(uint8_t function_code) {
+  switch (static_cast<FunctionCode>(function_code & FUNCTION_CODE_MASK)) {
+    case FunctionCode::READ_COILS:
+    case FunctionCode::READ_DISCRETE_INPUTS:
+    case FunctionCode::READ_HOLDING_REGISTERS:
+    case FunctionCode::READ_INPUT_REGISTERS:
+    case FunctionCode::WRITE_SINGLE_COIL:
+    case FunctionCode::WRITE_SINGLE_REGISTER:
+    case FunctionCode::WRITE_MULTIPLE_COILS:
+    case FunctionCode::WRITE_MULTIPLE_REGISTERS:
+    case FunctionCode::READ_FILE_RECORD:
+    case FunctionCode::WRITE_FILE_RECORD:
+    case FunctionCode::MASK_WRITE_REGISTER:
+    case FunctionCode::READ_WRITE_MULTIPLE_REGISTERS:
+    case FunctionCode::READ_FIFO_QUEUE:
+      return false;
+    default:
+      return true;
+  }
+}
+
 // Returns the expected length of a server response PDU based on the function code.
 // If too few bytes have arrived to determine the length, returns the minimum length. `size` is the
 // number of bytes available so far, which may exceed the eventual PDU (e.g. include the frame's CRC

tests/components/modbus/common.h

@@ -1,7 +1,10 @@
 #pragma once
 #include <cstdint>
+#include <cstring>
+#include <span>
 #include <vector>
 #include "esphome/components/uart/uart_component.h"
+#include "esphome/core/helpers.h"
 
 namespace esphome::modbus::testing {
 
@@ -30,4 +33,37 @@ class RecordingUART : public NullUART {
   std::vector<uint8_t> written;
 };
 
+// A UART the test can inject received bytes into, so frames travel the full receive path
+// (receive_modbus_frames -> parse -> dispatch) through hub.loop(). Writes are recorded.
+class InjectableUART : public RecordingUART {
+ public:
+  bool peek_byte(uint8_t *data) override {
+    if (this->rx_.empty())
+      return false;
+    *data = this->rx_.front();
+    return true;
+  }
+  bool read_array(uint8_t *data, size_t len) override {
+    if (len > this->rx_.size())
+      return false;
+    memcpy(data, this->rx_.data(), len);
+    this->rx_.erase(this->rx_.begin(), this->rx_.begin() + len);
+    return true;
+  }
+  size_t available() override { return this->rx_.size(); }
+
+  // Queues a complete wire frame: address + PDU + CRC16 (low byte first).
+  void inject_frame(uint8_t address, std::span<const uint8_t> pdu) {
+    size_t start = this->rx_.size();
+    this->rx_.push_back(address);
+    this->rx_.insert(this->rx_.end(), pdu.begin(), pdu.end());
+    uint16_t crc = crc16(this->rx_.data() + start, this->rx_.size() - start);
+    this->rx_.push_back(crc & 0xFF);
+    this->rx_.push_back(crc >> 8);
+  }
+
+ private:
+  std::vector<uint8_t> rx_;
+};
+
 }  // namespace esphome::modbus::testing

tests/components/modbus/modbus_unknown_function_test.cpp

@@ -0,0 +1,141 @@
+#include <gtest/gtest.h>
+
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "common.h"
+#include "esphome/components/modbus/modbus.h"
+
+namespace esphome::modbus::testing {
+
+namespace {
+
+// Records custom-response dispatches so tests can assert an unknown-length frame reached the device.
+class CustomRecordingDevice : public ModbusClientDevice {
+ public:
+  using ModbusClientDevice::ModbusClientDevice;
+  void on_custom_response(std::span<const uint8_t> request_pdu, std::span<const uint8_t> response_pdu,
+                          ResponseStatus status) override {
+    this->requests.emplace_back(request_pdu.begin(), request_pdu.end());
+    this->responses.emplace_back(response_pdu.begin(), response_pdu.end());
+    this->statuses.push_back(status);
+  }
+  std::vector<std::vector<uint8_t>> requests;
+  std::vector<std::vector<uint8_t>> responses;
+  std::vector<ResponseStatus> statuses;
+};
+
+// Every handler keeps its ILLEGAL_FUNCTION default; the hub's dispatch is what is under test.
+class SilentServerDevice : public ModbusServerDevice {};
+
+// Drives full client frames through the server hub's receive path (same shape as the broadcast tests).
+class TestServerHub : public ModbusServerHub {
+ public:
+  bool tx_blocked() override { return false; }
+
+  // Builds a complete client frame (address + FC + data + CRC) and runs the full receive-side parser.
+  // Returns true once the buffer has fully drained.
+  bool run_receive_parser_for_test(uint8_t address, uint8_t function_code, std::span<const uint8_t> data) {
+    this->rx_buffer_.clear();
+    this->rx_buffer_.reserve(data.size() + 4);
+    this->rx_buffer_.push_back(address);
+    this->rx_buffer_.push_back(function_code);
+    this->rx_buffer_.insert(this->rx_buffer_.end(), data.begin(), data.end());
+    uint16_t crc = crc16(this->rx_buffer_.data(), this->rx_buffer_.size());
+    this->rx_buffer_.push_back(crc & 0xFF);
+    this->rx_buffer_.push_back(crc >> 8);
+    this->parse_modbus_frames();
+    return this->rx_buffer_.empty();
+  }
+};
+
+}  // namespace
+
+// The frame-length parsers have explicit cases for exactly these 13 codes; every other value - the
+// assigned-but-unimplemented management codes, both user-defined ranges, and all unassigned codes -
+// must classify as unknown length. The exception flag masks off first.
+TEST(ModbusUnknownFunction, HelperMatchesParserCoverage) {
+  for (uint8_t fc : {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x0F, 0x10, 0x14, 0x15, 0x16, 0x17, 0x18}) {
+    EXPECT_FALSE(helpers::is_function_code_unknown_length(fc)) << "fc 0x" << std::hex << int(fc);
+  }
+  for (uint8_t fc : {0x07, 0x08, 0x0B, 0x0C, 0x11, 0x2A, 0x41, 0x48, 0x49, 0x64, 0x6E, 0x00, 0x7F}) {
+    EXPECT_TRUE(helpers::is_function_code_unknown_length(fc)) << "fc 0x" << std::hex << int(fc);
+  }
+  // Exception replies classify by their base code.
+  EXPECT_FALSE(helpers::is_function_code_unknown_length(0x83));
+  EXPECT_TRUE(helpers::is_function_code_unknown_length(0x87));
+  // Strictly wider than the user-defined ranges: every custom code is unknown-length, but not vice versa.
+  for (int fc = 0; fc <= 0xFF; fc++) {
+    if (helpers::is_function_code_custom(fc))
+      EXPECT_TRUE(helpers::is_function_code_unknown_length(fc)) << "fc 0x" << std::hex << fc;
+  }
+  EXPECT_FALSE(helpers::is_function_code_custom(0x49));
+
+  // Derived contract check: the helper must say "unknown" exactly when both length parsers fall
+  // through to default. With a zero-filled max-size PDU every explicit case returns at least 2
+  // (file records bottom out at 2, FIFO at 3) and only default returns MIN_PDU_SIZE, so comparing
+  // against MIN_PDU_SIZE detects a case added to either switch without updating the helper. The
+  // loop stops at 0x7F: above it the helper masks the exception flag off while client_pdu_length()
+  // switches on the unmasked byte and server_pdu_length() early-returns the exception length.
+  for (int fc = 0; fc <= 0x7F; fc++) {
+    const uint8_t pdu[MAX_PDU_SIZE] = {static_cast<uint8_t>(fc)};  // zero header fields
+    EXPECT_EQ(helpers::is_function_code_unknown_length(fc),
+              helpers::client_pdu_length(pdu, sizeof(pdu)) == MIN_PDU_SIZE)
+        << "client_pdu_length disagrees for fc 0x" << std::hex << fc;
+    EXPECT_EQ(helpers::is_function_code_unknown_length(fc),
+              helpers::server_pdu_length(pdu, sizeof(pdu)) == MIN_PDU_SIZE)
+        << "server_pdu_length disagrees for fc 0x" << std::hex << fc;
+  }
+}
+
+// A response with a function code outside the user-defined ranges (0x49) has no length case in
+// server_pdu_length(), so the parser must find the frame end by CRC scan - the same way it already
+// handles user-defined codes. Frame: address + FC 0x49 + 3 data bytes + CRC = 7 bytes. Without the
+// scan the parser assumes a 4-byte frame, fails the CRC, and the response never reaches the device.
+TEST(ModbusUnknownFunction, ClientParsesUnknownLengthResponse) {
+  InjectableUART uart;
+  ModbusClientHub hub;
+  hub.set_uart_parent(&uart);
+  hub.setup();  // computes frame timing from the baud rate
+  CustomRecordingDevice device(&hub, 0x02);
+
+  const uint8_t request[] = {0x49, 0x01};
+  ASSERT_TRUE(device.queue_pdu(request));
+  hub.loop();  // transmit
+  ASSERT_FALSE(uart.written.empty());
+
+  const uint8_t response_pdu[] = {0x49, 0x02, 0xAA, 0xBB};
+  uart.inject_frame(0x02, response_pdu);
+  hub.loop();  // receive + parse + match + dispatch
+
+  ASSERT_EQ(device.responses.size(), 1u);
+  EXPECT_EQ(device.requests[0], std::vector<uint8_t>(request, request + sizeof(request)));
+  EXPECT_EQ(device.responses[0], std::vector<uint8_t>(response_pdu, response_pdu + sizeof(response_pdu)));
+  EXPECT_FALSE(device.statuses[0].has_value());
+}
+
+// The server side of the same gap: a request with FC 0x49 for a registered device must parse (CRC
+// scan again) so the hub can answer ILLEGAL_FUNCTION per the spec. Without the scan the frame fails
+// to parse and the client gets silence instead of the exception.
+TEST(ModbusUnknownFunction, ServerRepliesIllegalFunctionToUnknownLengthRequest) {
+  TestServerHub hub;
+  RecordingUART uart;
+  hub.set_uart_parent(&uart);
+
+  SilentServerDevice device;
+  device.set_address(0x02);
+  hub.register_device(&device);
+
+  const uint8_t data[] = {0x02, 0xAA, 0xBB};
+  ASSERT_TRUE(hub.run_receive_parser_for_test(0x02, 0x49, data));
+
+  // Expected reply: address + FC with exception flag + ILLEGAL_FUNCTION + CRC.
+  std::vector<uint8_t> expected = {0x02, 0xC9, 0x01};
+  uint16_t crc = crc16(expected.data(), expected.size());
+  expected.push_back(crc & 0xFF);
+  expected.push_back(crc >> 8);
+  EXPECT_EQ(uart.written, expected);
+}
+
+}  // namespace esphome::modbus::testing