From 59397b4e28e2b7a7faec74969dc35bd1a0a0bc79 Mon Sep 17 00:00:00 2001 From: Bonne Eggleston Date: Fri, 28 Aug 2026 11:36:24 -0700 Subject: [PATCH] [modbus] Build single-value register writes on a right-sized stack buffer (#18844) Co-authored-by: J. Nick Koston --- esphome/components/modbus/modbus.h | 3 +++ .../components/modbus/modbus_definitions.h | 3 +++ esphome/components/modbus/modbus_helpers.cpp | 22 ++++++++++++++++--- esphome/components/modbus/modbus_helpers.h | 13 +++++++++++ esphome/core/helpers.h | 1 + .../components/modbus/modbus_helpers_test.cpp | 22 +++++++++++++++++++ 6 files changed, 61 insertions(+), 3 deletions(-) diff --git a/esphome/components/modbus/modbus.h b/esphome/components/modbus/modbus.h index 3ddaafb9fc..69a7eb82e3 100644 --- a/esphome/components/modbus/modbus.h +++ b/esphome/components/modbus/modbus.h @@ -534,6 +534,9 @@ class ModbusClientDevice { return this->queue_pdu(helpers::create_write_single_coil_pdu(address, value)); } bool write_multiple_registers(uint16_t start_address, std::span values) { + // Empty goes to the full-size builder so the rejection log names this method's limit, not the small one's. + if (!values.empty() && values.size() <= helpers::MAX_FEW_REGISTERS) + return this->queue_pdu(helpers::create_write_few_registers_pdu(start_address, values)); return this->queue_pdu(helpers::create_write_registers_pdu(start_address, values)); } /// Note: std::vector cannot bind to std::span; use a contiguous bool container or the packed diff --git a/esphome/components/modbus/modbus_definitions.h b/esphome/components/modbus/modbus_definitions.h index 089fc3d1ae..0939f9e76c 100644 --- a/esphome/components/modbus/modbus_definitions.h +++ b/esphome/components/modbus/modbus_definitions.h @@ -116,6 +116,9 @@ static constexpr uint16_t MAX_RAW_SIZE = 254; // Max RAW size is 256 - CRC(2) = static constexpr uint16_t READ_PDU_SIZE = 5; // A single-write PDU is always function code(1) + address(2) + value(2) static constexpr uint16_t WRITE_SINGLE_PDU_SIZE = 5; +// A multiple-write PDU starts with function code(1) + start address(2) + quantity(2) + byte count(1), +// followed by two bytes per register. +static constexpr uint16_t WRITE_MULTIPLE_HEADER_SIZE = 6; static constexpr uint16_t MAX_FRAME_SIZE = 256; // 4.1 Address 0 is the broadcast address: the request is processed by every device and never answered. diff --git a/esphome/components/modbus/modbus_helpers.cpp b/esphome/components/modbus/modbus_helpers.cpp index 836d9b2d38..92bd06cdf5 100644 --- a/esphome/components/modbus/modbus_helpers.cpp +++ b/esphome/components/modbus/modbus_helpers.cpp @@ -485,9 +485,12 @@ static bool register_block_in_range(const LogString *role, uint16_t start_addres return true; } -PduBuffer create_write_registers_pdu(uint16_t start_address, std::span values) { - PduBuffer pdu; // declared before every return so NRVO fires (all paths return the same object) - if (!register_block_in_range(LOG_STR("Write"), start_address, values.size(), MAX_NUM_OF_REGISTERS_TO_WRITE)) { +// The ceiling comes from the buffer itself: push_back() drops silently, so a bound wider than the buffer +// would put a truncated frame on the wire. +template static Pdu build_write_registers_pdu(uint16_t start_address, std::span values) { + constexpr auto max_registers = static_cast((Pdu::capacity() - WRITE_MULTIPLE_HEADER_SIZE) / 2); + Pdu pdu; // declared before every return so NRVO fires (all paths return the same object) + if (!register_block_in_range(LOG_STR("Write"), start_address, values.size(), max_registers)) { return pdu; } append_pdu_header(pdu, FunctionCode::WRITE_MULTIPLE_REGISTERS, start_address, values.size()); @@ -498,6 +501,19 @@ PduBuffer create_write_registers_pdu(uint16_t start_address, std::span values) { + return build_write_registers_pdu(start_address, values); +} + +WriteFewRegistersPdu create_write_few_registers_pdu(uint16_t start_address, std::span values) { + return build_write_registers_pdu(start_address, values); +} + PduBuffer create_read_write_multiple_registers_pdu(uint16_t read_start_address, uint16_t read_count, uint16_t write_start_address, std::span write_values) { diff --git a/esphome/components/modbus/modbus_helpers.h b/esphome/components/modbus/modbus_helpers.h index c3bccc4cba..a070ce250c 100644 --- a/esphome/components/modbus/modbus_helpers.h +++ b/esphome/components/modbus/modbus_helpers.h @@ -473,11 +473,15 @@ inline int64_t payload_to_number(const std::vector &data, SensorValueTy */ std::optional registers_to_number(const uint16_t *registers, size_t count, SensorValueType sensor_value_type); +/// The widest standard numeric value (a QWORD) spans 4 registers, so one entity value never writes more. +static constexpr uint16_t MAX_FEW_REGISTERS = 4; + // Named PDU buffer types: the builders' storage strategy (currently stack-allocated StaticVector, // right-sized per shape) can be swapped in one place without touching every signature. using PduBuffer = StaticVector; using ReadPdu = StaticVector; using WriteSinglePdu = StaticVector; +using WriteFewRegistersPdu = StaticVector; /// Scratch space for packing coils into wire layout: one bit per coil, sized for the spec maximum. using CoilPackBuffer = StaticVector; @@ -521,6 +525,15 @@ PduBuffer create_client_pdu(FunctionCode function_code, uint16_t start_address, */ PduBuffer create_write_registers_pdu(uint16_t start_address, std::span values); +/** Create modbus write multiple registers command (function 0x10) on a right-sized stack buffer. + * Identical wire bytes to create_write_registers_pdu() for any accepted input. + * @param start_address modbus address of the first register to write + * @param values register values to write, at most MAX_FEW_REGISTERS (an over-long or empty set is + * rejected and an empty PDU is returned) + * @return PDU (function code + data, no address, no CRC) + */ +WriteFewRegistersPdu create_write_few_registers_pdu(uint16_t start_address, std::span values); + /** Create modbus read/write multiple registers command * Function 0x17 Read/Write Multiple Registers * Writes write_values then reads read_count registers in one transaction (write first, per Modbus 6.17); diff --git a/esphome/core/helpers.h b/esphome/core/helpers.h index 1ccc833048..a0afb03124 100644 --- a/esphome/core/helpers.h +++ b/esphome/core/helpers.h @@ -290,6 +290,7 @@ template class StaticVector { } size_t size() const { return count_; } + static constexpr size_t capacity() { return N; } bool empty() const { return count_ == 0; } // Direct access to underlying data diff --git a/tests/components/modbus/modbus_helpers_test.cpp b/tests/components/modbus/modbus_helpers_test.cpp index 28573dc8a6..87af49710f 100644 --- a/tests/components/modbus/modbus_helpers_test.cpp +++ b/tests/components/modbus/modbus_helpers_test.cpp @@ -489,6 +489,28 @@ TEST(ModbusTypedBuilders, WriteRegistersPduRejectsOverLimit) { EXPECT_FALSE(create_write_registers_pdu(0x0000, values).empty()); } +TEST(ModbusTypedBuilders, WriteFewRegistersPduMatchesFullSizeBuilder) { + static_assert(sizeof(WriteFewRegistersPdu) < sizeof(PduBuffer) / 4, + "WriteFewRegistersPdu must be meaningfully smaller"); + const uint16_t values[] = {0x000B, 0x0016, 0xABCD, 0xFF00}; + for (size_t count = 1; count <= MAX_FEW_REGISTERS; count++) { + auto small = create_write_few_registers_pdu(0x0102, std::span(values, count)); + auto full = create_write_registers_pdu(0x0102, std::span(values, count)); + EXPECT_EQ(std::vector(small.begin(), small.end()), std::vector(full.begin(), full.end())) + << count << " registers"; + EXPECT_EQ(small.size(), 6u + 2 * count); + EXPECT_TRUE(is_client_pdu_standard(small.data(), small.size())); + } +} + +TEST(ModbusTypedBuilders, WriteFewRegistersPduRejectsInvalidInput) { + const uint16_t values[MAX_FEW_REGISTERS + 1] = {0xAAAA, 0xAAAA, 0xAAAA, 0xAAAA, 0xAAAA}; + EXPECT_TRUE(create_write_few_registers_pdu(0x0000, values).empty()); + EXPECT_FALSE(create_write_few_registers_pdu(0x0000, std::span(values, MAX_FEW_REGISTERS)).empty()); + EXPECT_TRUE(create_write_few_registers_pdu(0x0000, std::span()).empty()); + EXPECT_TRUE(create_write_few_registers_pdu(0xFFFF, std::span(values, 2)).empty()); +} + TEST(ModbusTypedBuilders, ReadWriteMultipleRegistersPduWireBytes) { const uint16_t write_values[] = {0x000B, 0x0016}; // Read 2 registers at 0x0010, write 2 registers at 0x0020.