From 10e592fa3a51b301644b5a742c75088cc3ab286a Mon Sep 17 00:00:00 2001 From: Bonne Eggleston Date: Tue, 18 Aug 2026 09:09:12 -0700 Subject: [PATCH] [modbus] CRC scan all unknown function codes (#18483) Co-authored-by: Claude Opus 4.8 (1M context) --- esphome/components/modbus/modbus.cpp | 33 ++-- esphome/components/modbus/modbus.h | 2 +- esphome/components/modbus/modbus_helpers.h | 32 ++++ tests/components/modbus/common.h | 36 +++++ .../modbus/modbus_unknown_function_test.cpp | 141 ++++++++++++++++++ 5 files changed, 232 insertions(+), 12 deletions(-) create mode 100644 tests/components/modbus/modbus_unknown_function_test.cpp diff --git a/esphome/components/modbus/modbus.cpp b/esphome/components/modbus/modbus.cpp index 5305f6313f..e4bd51ad5a 100644 --- a/esphome/components/modbus/modbus.cpp +++ b/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 { - // Custom functions could be any length - we have to rely on the CRC to determine completeness. +uint16_t Modbus::find_frame_end_by_crc_(uint16_t min_length) const { + // 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++) { - if (crc16(raw, len) == 0) - return len; + const auto max_len = static_cast(std::min(size, size_t(MAX_FRAME_SIZE))); + if (min_length > max_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)) { - frame_length = this->find_custom_frame_end_(frame_length); + if (helpers::is_function_code_unknown_length(function_code)) { + 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)) { - frame_length = this->find_custom_frame_end_(frame_length); + if (helpers::is_function_code_unknown_length(function_code)) { + 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; diff --git a/esphome/components/modbus/modbus.h b/esphome/components/modbus/modbus.h index dfe4a4872d..bb303c43a8 100644 --- a/esphome/components/modbus/modbus.h +++ b/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}; diff --git a/esphome/components/modbus/modbus_helpers.h b/esphome/components/modbus/modbus_helpers.h index c737e206c0..b2454e6f14 100644 --- a/esphome/components/modbus/modbus_helpers.h +++ b/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(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 diff --git a/tests/components/modbus/common.h b/tests/components/modbus/common.h index d03ccf8ec3..e6c37b0e6d 100644 --- a/tests/components/modbus/common.h +++ b/tests/components/modbus/common.h @@ -1,7 +1,10 @@ #pragma once #include +#include +#include #include #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 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 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 rx_; +}; + } // namespace esphome::modbus::testing diff --git a/tests/components/modbus/modbus_unknown_function_test.cpp b/tests/components/modbus/modbus_unknown_function_test.cpp new file mode 100644 index 0000000000..8b91d088b8 --- /dev/null +++ b/tests/components/modbus/modbus_unknown_function_test.cpp @@ -0,0 +1,141 @@ +#include + +#include +#include +#include + +#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 request_pdu, std::span 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> requests; + std::vector> responses; + std::vector 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 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(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(request, request + sizeof(request))); + EXPECT_EQ(device.responses[0], std::vector(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 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