From d25d1606867972060c22d2c7dea9118ae89a408d Mon Sep 17 00:00:00 2001 From: Bonne Eggleston Date: Wed, 1 Jul 2026 13:01:48 -0700 Subject: [PATCH] [modbus_server] Fix register range issues and allow partial reads (#17205) Co-authored-by: Claude Opus 4.8 Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com> Co-authored-by: J. Nick Koston --- esphome/components/modbus_server/__init__.py | 55 +++++- esphome/components/modbus_server/const.py | 1 + .../modbus_server/modbus_server.cpp | 107 ++++++++---- .../components/modbus_server/modbus_server.h | 6 + .../modbus_server/test_modbus_server.py | 84 +++++++++ tests/components/modbus_server/common.yaml | 1 + .../modbus_server/modbus_server_test.cpp | 161 ++++++++++++++++++ 7 files changed, 382 insertions(+), 33 deletions(-) create mode 100644 tests/component_tests/modbus_server/test_modbus_server.py diff --git a/esphome/components/modbus_server/__init__.py b/esphome/components/modbus_server/__init__.py index 2ba7f41b832..14f4ca8a4d7 100644 --- a/esphome/components/modbus_server/__init__.py +++ b/esphome/components/modbus_server/__init__.py @@ -8,8 +8,10 @@ from esphome.components.modbus.helpers import ( ) import esphome.config_validation as cv from esphome.const import CONF_ADDRESS, CONF_ID +from esphome.types import ConfigType from .const import ( + CONF_ALLOW_PARTIAL_READ, CONF_COURTESY_RESPONSE, CONF_READ_LAMBDA, CONF_REGISTER_LAST_ADDRESS, @@ -41,17 +43,62 @@ SERVER_COURTESY_RESPONSE_SCHEMA = cv.Schema( } ) +# RAW has no numeric encoding, so it is not a valid server register type: a server value is produced by a +# lambda and encoded into registers, and on the server a RAW register would just be a single 16-bit word -- +# use U_WORD for that. Restrict the choices to the encodable types. +SERVER_SENSOR_VALUE_TYPE = { + key: value for key, value in SENSOR_VALUE_TYPE.items() if key != "RAW" +} + ModbusServerRegisterSchema = cv.Schema( { cv.GenerateID(): cv.declare_id(ServerRegister), cv.Required(CONF_ADDRESS): cv.hex_uint16_t, - cv.Optional(CONF_VALUE_TYPE, default="U_WORD"): cv.enum(SENSOR_VALUE_TYPE), + cv.Optional(CONF_VALUE_TYPE, default="U_WORD"): cv.enum( + SERVER_SENSOR_VALUE_TYPE + ), cv.Required(CONF_READ_LAMBDA): cv.returning_lambda, cv.Optional(CONF_WRITE_LAMBDA): cv.returning_lambda, + cv.Optional(CONF_ALLOW_PARTIAL_READ, default=False): cv.boolean, } ) +def _validate_register_ranges(config: ConfigType) -> ConfigType: + # Each register occupies [address, address + register_count); the whole span must fit inside the 16-bit + # Modbus address space (0x0000-0xFFFF). + for register in config.get(CONF_REGISTERS, []): + address = register[CONF_ADDRESS] + register_count = TYPE_REGISTER_MAP[register[CONF_VALUE_TYPE]] + if address + register_count > 0x10000: + raise cv.Invalid( + f"Register at 0x{address:04X} spans {register_count} register(s) and runs past " + "the end of the 16-bit address space (0xFFFF)", + path=[CONF_REGISTERS], + ) + return config + + +def _validate_no_overlapping_registers(config: ConfigType) -> ConfigType: + # Each register occupies [address, address + register_count). Reject configs where any two ranges + # overlap -- the same address twice, or a multi-register value straddling a neighbour -- since the + # server resolves a request by the value containing an address and overlaps are ambiguous. + spans = sorted( + (register[CONF_ADDRESS], TYPE_REGISTER_MAP[register[CONF_VALUE_TYPE]]) + for register in config.get(CONF_REGISTERS, []) + ) + for (address, register_count), (next_address, _) in zip( + spans, spans[1:], strict=False + ): + if next_address < address + register_count: + raise cv.Invalid( + f"Register address 0x{next_address:04X} overlaps the register at 0x{address:04X}, " + f"which spans {register_count} register(s); each register's address range must be unique", + path=[CONF_REGISTERS], + ) + return config + + CONFIG_SCHEMA = cv.All( cv.Schema( { @@ -62,10 +109,12 @@ CONFIG_SCHEMA = cv.All( ): cv.ensure_list(ModbusServerRegisterSchema), } ).extend(modbus.modbus_device_schema(0x01, role="server")), + _validate_register_ranges, + _validate_no_overlapping_registers, ) -def _final_validate(config): +def _final_validate(config: ConfigType) -> ConfigType: return modbus.final_validate_modbus_device("modbus_server", role="server")(config) @@ -118,6 +167,8 @@ async def to_code(config): ), ) ) + if server_register[CONF_ALLOW_PARTIAL_READ]: + cg.add(server_register_var.set_allow_partial_read(True)) cg.add(var.add_server_register(server_register_var)) await cg.register_component(var, config) return await modbus.register_modbus_server_device(var, config) diff --git a/esphome/components/modbus_server/const.py b/esphome/components/modbus_server/const.py index f83211c207b..f2a8c53f45c 100644 --- a/esphome/components/modbus_server/const.py +++ b/esphome/components/modbus_server/const.py @@ -5,3 +5,4 @@ CONF_COURTESY_RESPONSE = "courtesy_response" CONF_READ_LAMBDA = "read_lambda" CONF_WRITE_LAMBDA = "write_lambda" CONF_REGISTERS = "registers" +CONF_ALLOW_PARTIAL_READ = "allow_partial_read" diff --git a/esphome/components/modbus_server/modbus_server.cpp b/esphome/components/modbus_server/modbus_server.cpp index bb264eb9933..44b1b160a5d 100644 --- a/esphome/components/modbus_server/modbus_server.cpp +++ b/esphome/components/modbus_server/modbus_server.cpp @@ -8,6 +8,25 @@ using modbus::helpers::registers_to_number; static const char *const TAG = "modbus_server"; +// The widest Modbus value type (QWORD) spans four registers. +static constexpr uint8_t MAX_REGISTERS_PER_VALUE = 4; +// number_to_payload() encodes the 64-bit value returned by read_lambda() into 16-bit registers, so the +// widest possible value spans exactly sizeof(int64_t) / sizeof(uint16_t) registers. Tie the bound to that +// source so a future wider value type -- which would require widening the encoded value itself -- can't +// silently overflow the value_words buffer below (StaticVector::push_back drops words past capacity). +static_assert(MAX_REGISTERS_PER_VALUE == sizeof(int64_t) / sizeof(uint16_t), + "MAX_REGISTERS_PER_VALUE must match the register span of the widest encodable value"); + +ServerRegister *ModbusServer::find_containing_register_(uint32_t address) const { + for (auto *server_register : this->server_registers_) { + if (address >= server_register->address && + address < static_cast(server_register->address) + server_register->register_count) { + return server_register; + } + } + return nullptr; +} + modbus::ServerResponseStatus ModbusServer::on_modbus_read_registers(uint16_t start_address, uint16_t number_of_registers, modbus::RegisterValues ®isters) { @@ -15,42 +34,68 @@ modbus::ServerResponseStatus ModbusServer::on_modbus_read_registers(uint16_t sta "Received read holding/input registers for device 0x%X. Start address: 0x%X. Number of registers: 0x%X.", this->address_, start_address, number_of_registers); - for (uint16_t current_address = start_address; current_address < start_address + number_of_registers;) { - bool found = false; - for (auto *server_register : this->server_registers_) { - if (server_register->address == current_address) { - if (!server_register->read_lambda) { - break; - } - int64_t value = server_register->read_lambda(); - char value_buf[ServerRegister::FORMAT_VALUE_BUF_SIZE]; - ESP_LOGV(TAG, "Matched register. Address: 0x%02X. Value type: %zu. Register count: %u. Value: %s.", - server_register->address, static_cast(server_register->value_type), - server_register->register_count, server_register->format_value(value, value_buf, sizeof(value_buf))); + const uint32_t end_address = static_cast(start_address) + number_of_registers; + uint32_t current_address = start_address; + while (current_address < end_address) { + ServerRegister *server_register = this->find_containing_register_(current_address); - modbus::helpers::number_to_payload(registers, value, server_register->value_type); - current_address += server_register->register_count; - found = true; - break; - } - } - - if (!found) { + if (server_register == nullptr) { + // Unregistered address: optionally answer with the courtesy default, otherwise reject. if (this->server_courtesy_response_.enabled && - (current_address <= this->server_courtesy_response_.register_last_address)) { - ESP_LOGV(TAG, - "Could not match any register to address 0x%02X, but default allowed. " - "Returning default value: %" PRIu16 ".", - current_address, this->server_courtesy_response_.register_value); + current_address <= this->server_courtesy_response_.register_last_address) { + ESP_LOGV(TAG, "No register at 0x%04X; returning courtesy default %" PRIu16 ".", + static_cast(current_address), this->server_courtesy_response_.register_value); registers.push_back(this->server_courtesy_response_.register_value); - current_address += 1; // Just increment by 1, as the default response is a single register - } else { - ESP_LOGW(TAG, - "Could not match any register to address 0x%02X and default not allowed. Sending exception response.", - current_address); - return ModbusExceptionCode::ILLEGAL_DATA_ADDRESS; + current_address += 1; // the courtesy default is always a single register + continue; } + ESP_LOGW(TAG, "No register at 0x%04X and courtesy default not allowed. Sending exception response.", + static_cast(current_address)); + return ModbusExceptionCode::ILLEGAL_DATA_ADDRESS; } + + if (!server_register->read_lambda) { + // Registered but not readable (write-only); don't mask it with the courtesy default. + ESP_LOGW(TAG, "Register at 0x%04X is not readable. Sending exception response.", server_register->address); + return ModbusExceptionCode::ILLEGAL_DATA_ADDRESS; + } + + // A multi-register value is normally atomic: the request must start at its first register and cover all of + // it. A value may opt in to partial reads, in which case the request may start inside it or stop short of + // its end and we return only the covered words. + const uint16_t value_offset = static_cast(current_address - server_register->address); + const uint16_t words_available = static_cast(server_register->register_count - value_offset); + const uint16_t words_wanted = static_cast(end_address - current_address); + const uint16_t take = words_available < words_wanted ? words_available : words_wanted; + const bool clipped = value_offset != 0 || take != server_register->register_count; + if (clipped && !server_register->allow_partial_read) { + ESP_LOGW(TAG, + "Read clips the multi-register value at 0x%04X, which does not allow partial reads. " + "Sending exception response.", + server_register->address); + return ModbusExceptionCode::ILLEGAL_DATA_ADDRESS; + } + + int64_t value = server_register->read_lambda(); + char value_buf[ServerRegister::FORMAT_VALUE_BUF_SIZE]; + ESP_LOGV(TAG, "Matched register. Address: 0x%02X. Value type: %zu. Register count: %u. Value: %s.", + server_register->address, static_cast(server_register->value_type), + server_register->register_count, server_register->format_value(value, value_buf, sizeof(value_buf))); + + // Encode the whole value once (wire word order) and emit only the covered words. Slicing the encoded words + // handles the reversed value types for free, since number_to_payload already emits in wire order. + StaticVector value_words; + modbus::helpers::number_to_payload(value_words, value, server_register->value_type); + if (value_offset + take > value_words.size()) { + // The value encoded to fewer words than its register span (e.g. a RAW register); treat as a device fault. + ESP_LOGE(TAG, "Register at 0x%04X did not encode to %u registers", server_register->address, + server_register->register_count); + return ModbusExceptionCode::SERVICE_DEVICE_FAILURE; + } + for (uint16_t i = 0; i < take; i++) { + registers.push_back(value_words[value_offset + i]); + } + current_address += take; } return {}; diff --git a/esphome/components/modbus_server/modbus_server.h b/esphome/components/modbus_server/modbus_server.h index 0c224545286..f68d1c4a30f 100644 --- a/esphome/components/modbus_server/modbus_server.h +++ b/esphome/components/modbus_server/modbus_server.h @@ -84,9 +84,13 @@ class ServerRegister { } } + void set_allow_partial_read(bool allow_partial_read) { this->allow_partial_read = allow_partial_read; } + uint16_t address{0}; SensorValueType value_type{SensorValueType::RAW}; uint8_t register_count{0}; + // When true, a read may cover only part of this multi-register value; otherwise it must read the whole value. + bool allow_partial_read{false}; ReadLambda read_lambda; WriteLambda write_lambda; }; @@ -111,6 +115,8 @@ class ModbusServer : public Component, public modbus::ModbusServerDevice { ServerCourtesyResponse get_server_courtesy_response() const { return this->server_courtesy_response_; } protected: + /// Find the registered value whose register span contains address, or nullptr if none does. + ServerRegister *find_containing_register_(uint32_t address) const; /// Collection of all server registers for this component std::vector server_registers_{}; /// Server courtesy response diff --git a/tests/component_tests/modbus_server/test_modbus_server.py b/tests/component_tests/modbus_server/test_modbus_server.py new file mode 100644 index 00000000000..7c978a5cd54 --- /dev/null +++ b/tests/component_tests/modbus_server/test_modbus_server.py @@ -0,0 +1,84 @@ +"""Tests for modbus_server configuration validation.""" + +import pytest + +from esphome import config_validation as cv +from esphome.components.modbus_server import ( + SERVER_SENSOR_VALUE_TYPE, + _validate_no_overlapping_registers, + _validate_register_ranges, +) +from esphome.components.modbus_server.const import CONF_REGISTERS, CONF_VALUE_TYPE +from esphome.const import CONF_ADDRESS + + +def _config(registers: list[tuple[int, str]]) -> dict: + return { + CONF_REGISTERS: [ + {CONF_ADDRESS: address, CONF_VALUE_TYPE: value_type} + for address, value_type in registers + ] + } + + +def test_non_overlapping_registers_pass() -> None: + # Values that tile the address space without gaps or overlaps are accepted. + config = _config([(0x00, "U_WORD"), (0x01, "U_DWORD"), (0x03, "U_WORD")]) + assert _validate_no_overlapping_registers(config) is config + + +def test_registers_with_gaps_pass() -> None: + config = _config([(0x00, "U_WORD"), (0x05, "U_QWORD"), (0x20, "U_WORD")]) + assert _validate_no_overlapping_registers(config) is config + + +def test_no_registers_pass() -> None: + assert _validate_no_overlapping_registers({}) == {} + + +def test_duplicate_address_rejected() -> None: + config = _config([(0x10, "U_WORD"), (0x10, "U_WORD")]) + with pytest.raises(cv.Invalid, match="overlaps"): + _validate_no_overlapping_registers(config) + + +def test_multi_register_value_overlapping_neighbour_rejected() -> None: + # U_DWORD at 0x10 occupies 0x10 and 0x11; a U_WORD at 0x11 collides with its low word. + config = _config([(0x10, "U_DWORD"), (0x11, "U_WORD")]) + with pytest.raises(cv.Invalid, match="overlaps"): + _validate_no_overlapping_registers(config) + + +def test_overlap_detected_regardless_of_order() -> None: + # The U_DWORD at 0x10 covers 0x10-0x11 and overlaps the U_WORD at 0x11 even when declared after it. + config = _config([(0x11, "U_WORD"), (0x10, "U_DWORD")]) + with pytest.raises(cv.Invalid, match="overlaps"): + _validate_no_overlapping_registers(config) + + +def test_register_span_within_address_space_pass() -> None: + # A value whose span ends exactly at 0xFFFF is fine (U_QWORD at 0xFFFC covers 0xFFFC-0xFFFF). + config = _config([(0xFFFF, "U_WORD"), (0xFFFC, "U_QWORD")]) + assert _validate_register_ranges(config) is config + + +def test_register_span_past_end_rejected() -> None: + # U_QWORD at 0xFFFE would need 0xFFFE-0x10001, running off the 16-bit address space. + config = _config([(0xFFFE, "U_QWORD")]) + with pytest.raises(cv.Invalid, match="past the end"): + _validate_register_ranges(config) + + +def test_multi_register_value_at_last_address_rejected() -> None: + # A U_DWORD at 0xFFFF needs a second register at 0x10000, which does not exist. + config = _config([(0xFFFF, "U_DWORD")]) + with pytest.raises(cv.Invalid, match="past the end"): + _validate_register_ranges(config) + + +def test_raw_value_type_rejected() -> None: + # RAW has no numeric encoding, so it is not offered as a server register type. + validator = cv.enum(SERVER_SENSOR_VALUE_TYPE) + with pytest.raises(cv.Invalid): + validator("RAW") + assert validator("U_WORD") == "U_WORD" diff --git a/tests/components/modbus_server/common.yaml b/tests/components/modbus_server/common.yaml index 2e4a81a1aa5..8b2316b6e3c 100644 --- a/tests/components/modbus_server/common.yaml +++ b/tests/components/modbus_server/common.yaml @@ -18,6 +18,7 @@ modbus_server: registers: - address: 0x9 value_type: S_DWORD + allow_partial_read: true read_lambda: |- return 31; write_lambda: |- diff --git a/tests/components/modbus_server/modbus_server_test.cpp b/tests/components/modbus_server/modbus_server_test.cpp index 0c8f5d04cf0..419bb9cf25d 100644 --- a/tests/components/modbus_server/modbus_server_test.cpp +++ b/tests/components/modbus_server/modbus_server_test.cpp @@ -121,4 +121,165 @@ TEST(ModbusServerWrite, CallbackFailureIsServiceDeviceFailure) { EXPECT_TRUE(first_written); // pre-validation passed, so the first write applied before the failure } +// --- on_modbus_read_registers -------------------------------------------------- + +TEST(ModbusServerRead, SingleWordSucceeds) { + ModbusServer server; + ServerRegister reg(0x0000, SensorValueType::U_WORD, 1); + reg.read_lambda = []() -> int64_t { return 0x1234; }; + server.add_server_register(®); + + RegisterValues out; + auto status = server.on_modbus_read_registers(0x0000, 1, out); + EXPECT_FALSE(status.has_value()); + ASSERT_EQ(out.size(), 1u); + EXPECT_EQ(out[0], 0x1234); +} + +TEST(ModbusServerRead, DwordReturnsTwoWordsHighFirst) { + ModbusServer server; + ServerRegister reg(0x0000, SensorValueType::U_DWORD, 2); + reg.read_lambda = []() -> int64_t { return 0x12345678; }; + server.add_server_register(®); + + RegisterValues out; + auto status = server.on_modbus_read_registers(0x0000, 2, out); + EXPECT_FALSE(status.has_value()); + ASSERT_EQ(out.size(), 2u); + EXPECT_EQ(out[0], 0x1234); + EXPECT_EQ(out[1], 0x5678); +} + +// Starting inside a multi-register value is rejected with ILLEGAL_DATA_ADDRESS -- not masked by the courtesy +// default -- and the read_lambda is never invoked. +TEST(ModbusServerRead, StartInsideValueRejected) { + ModbusServer server; + bool read_called = false; + ServerRegister reg(0x0010, SensorValueType::U_DWORD, 2); // occupies 0x0010 and 0x0011 + reg.read_lambda = [&read_called]() -> int64_t { + read_called = true; + return 0; + }; + server.set_server_courtesy_response( + ServerCourtesyResponse{.enabled = true, .register_last_address = 0xFFFF, .register_value = 0xABCD}); + server.add_server_register(®); + + RegisterValues out; + auto status = server.on_modbus_read_registers(0x0011, 1, out); // the second cell of the DWORD + ASSERT_TRUE(status.has_value()); + if (status.has_value()) + EXPECT_EQ(status.value(), ModbusExceptionCode::ILLEGAL_DATA_ADDRESS); + EXPECT_FALSE(read_called); +} + +// A read that stops short of a value's end clips it -> ILLEGAL_DATA_ADDRESS, and the read_lambda is not invoked. +TEST(ModbusServerRead, ClippedTailRejected) { + ModbusServer server; + bool read_called = false; + ServerRegister reg(0x0000, SensorValueType::U_DWORD, 2); + reg.read_lambda = [&read_called]() -> int64_t { + read_called = true; + return 0; + }; + server.add_server_register(®); + + RegisterValues out; + auto status = server.on_modbus_read_registers(0x0000, 1, out); // only 1 of the DWORD's 2 registers + ASSERT_TRUE(status.has_value()); + if (status.has_value()) + EXPECT_EQ(status.value(), ModbusExceptionCode::ILLEGAL_DATA_ADDRESS); + EXPECT_FALSE(read_called); +} + +// A write-only register (no read_lambda) is not readable -> ILLEGAL_DATA_ADDRESS, not a courtesy default. +TEST(ModbusServerRead, WriteOnlyRegisterRejected) { + ModbusServer server; + ServerRegister reg(0x0000, SensorValueType::U_WORD, 1); // no read_lambda set + server.set_server_courtesy_response( + ServerCourtesyResponse{.enabled = true, .register_last_address = 0xFFFF, .register_value = 0xABCD}); + server.add_server_register(®); + + RegisterValues out; + auto status = server.on_modbus_read_registers(0x0000, 1, out); + ASSERT_TRUE(status.has_value()); + if (status.has_value()) + EXPECT_EQ(status.value(), ModbusExceptionCode::ILLEGAL_DATA_ADDRESS); +} + +// An unregistered address with courtesy enabled returns the default value for each cell. +TEST(ModbusServerRead, CourtesyDefaultForUnregistered) { + ModbusServer server; + server.set_server_courtesy_response( + ServerCourtesyResponse{.enabled = true, .register_last_address = 0xFFFF, .register_value = 0xABCD}); + + RegisterValues out; + auto status = server.on_modbus_read_registers(0x0005, 2, out); + EXPECT_FALSE(status.has_value()); + ASSERT_EQ(out.size(), 2u); + EXPECT_EQ(out[0], 0xABCD); + EXPECT_EQ(out[1], 0xABCD); +} + +// An unregistered address with courtesy disabled is rejected. +TEST(ModbusServerRead, UnregisteredRejectedWithoutCourtesy) { + ModbusServer server; + RegisterValues out; + auto status = server.on_modbus_read_registers(0x0005, 1, out); + ASSERT_TRUE(status.has_value()); + if (status.has_value()) + EXPECT_EQ(status.value(), ModbusExceptionCode::ILLEGAL_DATA_ADDRESS); +} + +// --- partial reads (opt-in) ---------------------------------------------------- + +// With allow_partial_read, reading only the first register of a DWORD returns its high word. +TEST(ModbusServerRead, PartialReadHighWord) { + ModbusServer server; + ServerRegister reg(0x0010, SensorValueType::U_DWORD, 2); + reg.allow_partial_read = true; + reg.read_lambda = []() -> int64_t { return 0x12345678; }; + server.add_server_register(®); + + RegisterValues out; + auto status = server.on_modbus_read_registers(0x0010, 1, out); + EXPECT_FALSE(status.has_value()); + ASSERT_EQ(out.size(), 1u); + EXPECT_EQ(out[0], 0x1234); +} + +// With allow_partial_read, starting at the interior cell returns the low word. +TEST(ModbusServerRead, PartialReadLowWordFromInterior) { + ModbusServer server; + ServerRegister reg(0x0010, SensorValueType::U_DWORD, 2); + reg.allow_partial_read = true; + reg.read_lambda = []() -> int64_t { return 0x12345678; }; + server.add_server_register(®); + + RegisterValues out; + auto status = server.on_modbus_read_registers(0x0011, 1, out); + EXPECT_FALSE(status.has_value()); + ASSERT_EQ(out.size(), 1u); + EXPECT_EQ(out[0], 0x5678); +} + +// Slicing is in wire order, so a reversed value type partials correctly: U_DWORD_R emits the low word +// first, so 0x0010 holds 0x5678 and 0x0011 holds 0x1234. +TEST(ModbusServerRead, PartialReadReversedType) { + ModbusServer server; + ServerRegister reg(0x0010, SensorValueType::U_DWORD_R, 2); + reg.allow_partial_read = true; + reg.read_lambda = []() -> int64_t { return 0x12345678; }; + server.add_server_register(®); + + RegisterValues first; + ASSERT_FALSE(server.on_modbus_read_registers(0x0010, 1, first).has_value()); + ASSERT_EQ(first.size(), 1u); + EXPECT_EQ(first[0], 0x5678); + + RegisterValues second; + ASSERT_FALSE(server.on_modbus_read_registers(0x0011, 1, second).has_value()); + ASSERT_EQ(second.size(), 1u); + EXPECT_EQ(second[0], 0x1234); +} + } // namespace esphome::modbus_server