diff --git a/esphome/components/modbus/modbus_definitions.h b/esphome/components/modbus/modbus_definitions.h index 09f430f703..fd99055ca2 100644 --- a/esphome/components/modbus/modbus_definitions.h +++ b/esphome/components/modbus/modbus_definitions.h @@ -123,6 +123,9 @@ static constexpr uint16_t MAX_FRAME_SIZE = 256; * with any subscript. Writes and forwarding are defensive: set() drops out-of-range bits and * bytes() clamps to the real span, because those paths touch buffers and the wire directly. */ +/// Bits pack 8 per data byte, rounded up to whole bytes. +constexpr size_t packed_bit_bytes(size_t bits) { return (bits + 7) / 8; } + class PackedBits { public: PackedBits(std::span data, uint16_t count) : data_(data), count_(count) {} @@ -134,7 +137,7 @@ class PackedBits { /// over a larger buffer - forwarding this span onto the wire can never leak trailing buffer content. /// Clamped to the actual span so a view over a too-short buffer stays detectable instead of UB. std::span bytes() const { - return this->data_.first(std::min((this->count_ + 7) / 8, this->data_.size())); + return this->data_.first(std::min(packed_bit_bytes(this->count_), this->data_.size())); } private: diff --git a/esphome/components/modbus/modbus_helpers.cpp b/esphome/components/modbus/modbus_helpers.cpp index 88e10287a2..85c5d3e882 100644 --- a/esphome/components/modbus/modbus_helpers.cpp +++ b/esphome/components/modbus/modbus_helpers.cpp @@ -7,6 +7,19 @@ namespace esphome::modbus::helpers { static const char *const TAG = "modbus_helpers"; +// A quantity/address pair is standard when the quantity is non-zero, within the per-table maximum, +// and the range [start_address, start_address + quantity) stays inside the 16-bit address space +// (the 32-bit promotion is the overflow guard - a 16-bit sum could wrap and pass). +static bool quantity_in_range(uint16_t start_address, uint16_t quantity, uint16_t max_quantity) { + return quantity != 0 && quantity <= max_quantity && uint32_t(start_address) + quantity <= 0x10000u; +} + +// The spec allows exactly ON (0xFF00) and OFF (0x0000) for a single-coil value, on the request and +// on its echoed response alike. +static bool is_canonical_coil_value(uint8_t high_byte, uint8_t low_byte) { + return (high_byte == 0xFF || high_byte == 0x00) && low_byte == 0x00; +} + uint16_t server_pdu_length(const uint8_t *frame, size_t size) { if (size < MIN_PDU_SIZE) return MIN_PDU_SIZE; @@ -82,11 +95,12 @@ bool is_server_pdu_standard(const uint8_t *pdu, size_t size) { if (server_pdu_length(pdu, size) != size) return false; - switch (static_cast(pdu[0])) { + const auto function_code = static_cast(pdu[0]); + switch (function_code) { case FunctionCode::READ_COILS: case FunctionCode::READ_DISCRETE_INPUTS: // A conformant bit-read response carries at least one packed byte (up to 2000 bits = 250 bytes). - return pdu[1] != 0 && pdu[1] <= uint8_t((MAX_NUM_OF_COILS_TO_READ + 7) / 8); + return pdu[1] != 0 && pdu[1] <= uint8_t(packed_bit_bytes(MAX_NUM_OF_COILS_TO_READ)); case FunctionCode::READ_HOLDING_REGISTERS: case FunctionCode::READ_INPUT_REGISTERS: // Registers are 2 bytes each: the byte count must be a non-zero even count within the read maximum. @@ -99,15 +113,13 @@ bool is_server_pdu_standard(const uint8_t *pdu, size_t size) { case FunctionCode::WRITE_MULTIPLE_COILS: case FunctionCode::WRITE_MULTIPLE_REGISTERS: { // The response echoes start address and quantity: bound them like the request side does. - const bool bits = static_cast(pdu[0]) == FunctionCode::WRITE_MULTIPLE_COILS; - const uint16_t start_address = get_data(pdu, 1); - const uint16_t quantity = get_data(pdu, 3); + const bool bits = function_code == FunctionCode::WRITE_MULTIPLE_COILS; const uint16_t max_quantity = bits ? MAX_NUM_OF_COILS_TO_WRITE : MAX_NUM_OF_REGISTERS_TO_WRITE; - return quantity != 0 && quantity <= max_quantity && (uint32_t) start_address + quantity <= 0x10000u; + return quantity_in_range(get_data(pdu, 1), get_data(pdu, 3), max_quantity); } case FunctionCode::WRITE_SINGLE_COIL: // The response echoes the request, so the same ON/OFF constraint applies. - return (pdu[3] == 0xFF || pdu[3] == 0x00) && pdu[4] == 0x00; + return is_canonical_coil_value(pdu[3], pdu[4]); default: return true; // All other function codes validated by length alone } @@ -117,45 +129,38 @@ bool is_client_pdu_standard(const uint8_t *pdu, size_t size) { if (client_pdu_length(pdu, size) != size) return false; - switch (static_cast(pdu[0])) { + const auto function_code = static_cast(pdu[0]); + switch (function_code) { case FunctionCode::READ_COILS: case FunctionCode::READ_DISCRETE_INPUTS: case FunctionCode::READ_HOLDING_REGISTERS: case FunctionCode::READ_INPUT_REGISTERS: { - const bool bits = static_cast(pdu[0]) == FunctionCode::READ_COILS || - static_cast(pdu[0]) == FunctionCode::READ_DISCRETE_INPUTS; - const uint16_t start_address = get_data(pdu, 1); - const uint16_t quantity = get_data(pdu, 3); + const bool bits = + function_code == FunctionCode::READ_COILS || function_code == FunctionCode::READ_DISCRETE_INPUTS; const uint16_t max_quantity = bits ? MAX_NUM_OF_COILS_TO_READ : MAX_NUM_OF_REGISTERS_TO_READ; - return quantity != 0 && quantity <= max_quantity && (uint32_t) start_address + quantity <= 0x10000u; + return quantity_in_range(get_data(pdu, 1), get_data(pdu, 3), max_quantity); } case FunctionCode::WRITE_MULTIPLE_COILS: case FunctionCode::WRITE_MULTIPLE_REGISTERS: { - const bool bits = static_cast(pdu[0]) == FunctionCode::WRITE_MULTIPLE_COILS; - const uint16_t start_address = get_data(pdu, 1); + const bool bits = function_code == FunctionCode::WRITE_MULTIPLE_COILS; const uint16_t quantity = get_data(pdu, 3); const uint16_t max_quantity = bits ? MAX_NUM_OF_COILS_TO_WRITE : MAX_NUM_OF_REGISTERS_TO_WRITE; // Coils are packed 8 per data byte; registers are 2 bytes each. - const size_t expected_data_bytes = bits ? (static_cast(quantity) + 7) / 8 : quantity * 2; - return quantity != 0 && quantity <= max_quantity && (uint32_t) start_address + quantity <= 0x10000u && - pdu[5] == expected_data_bytes; + const size_t expected_data_bytes = bits ? packed_bit_bytes(quantity) : quantity * 2; + return quantity_in_range(get_data(pdu, 1), quantity, max_quantity) && pdu[5] == expected_data_bytes; } case FunctionCode::READ_FILE_RECORD: case FunctionCode::WRITE_FILE_RECORD: return pdu[1] <= MAX_PDU_SIZE - 2; case FunctionCode::READ_WRITE_MULTIPLE_REGISTERS: { - const uint16_t start_address_read = get_data(pdu, 1); - const uint16_t quantity_read = get_data(pdu, 3); - const uint16_t start_address_write = get_data(pdu, 5); const uint16_t quantity_write = get_data(pdu, 7); - return quantity_read != 0 && quantity_read <= MAX_NUM_OF_REGISTERS_TO_READ && quantity_write != 0 && - quantity_write <= MAX_NUM_OF_REGISTERS_TO_WRITE_RW && - (uint32_t) start_address_read + quantity_read <= 0x10000u && - (uint32_t) start_address_write + quantity_write <= 0x10000u && pdu[9] == quantity_write * 2; + return quantity_in_range(get_data(pdu, 1), get_data(pdu, 3), MAX_NUM_OF_REGISTERS_TO_READ) && + quantity_in_range(get_data(pdu, 5), quantity_write, MAX_NUM_OF_REGISTERS_TO_WRITE_RW) && + pdu[9] == quantity_write * 2; } case FunctionCode::WRITE_SINGLE_COIL: // The one variable field in an otherwise fixed-shape PDU: the spec allows exactly ON/OFF. - return (pdu[3] == 0xFF || pdu[3] == 0x00) && pdu[4] == 0x00; + return is_canonical_coil_value(pdu[3], pdu[4]); default: return true; // All other function codes validated by length alone } @@ -292,6 +297,14 @@ static void append_pdu_header(StaticVector &pdu, FunctionCode func pdu.push_back(second >> 0); } +// Zero the unused bits of a multi-coil write's final data byte, as the spec requires. Kept in one +// place so the generic and typed coil builders produce identical wire bytes for the same write. +static void mask_trailing_pad_bits(std::span data, uint16_t bit_count) { + if (data.empty() || bit_count % 8 == 0) + return; + data.back() &= static_cast((1 << (bit_count % 8)) - 1); +} + ReadPdu create_read_pdu(FunctionCode function_code, uint16_t start_address, uint16_t number_of_entities) { ReadPdu pdu; // declared before every return so NRVO fires (all paths return the same object) if (number_of_entities == 0) { @@ -394,8 +407,7 @@ PduBuffer create_client_pdu(FunctionCode function_code, uint16_t start_address, } // The spec allows exactly ON (0xFF00) and OFF (0x0000) for a single-coil write - the same rule // is_client_pdu_standard() enforces, so a built frame cannot be misclassified on reply. - if (function_code == FunctionCode::WRITE_SINGLE_COIL && - ((values[0] != 0xFF && values[0] != 0x00) || values[1] != 0x00)) { + if (function_code == FunctionCode::WRITE_SINGLE_COIL && !is_canonical_coil_value(values[0], values[1])) { ESP_LOGE(TAG, "Invalid single-coil value %02X%02X (must be FF00 or 0000), dropping request", values[0], values[1]); return pdu; @@ -409,8 +421,7 @@ PduBuffer create_client_pdu(FunctionCode function_code, uint16_t start_address, // non-standard on reply, and the spec bound keeps the PDU within capacity by construction. // Checked before the header append: a failed check must return an empty PDU, not a 5-byte partial one. const bool bits = function_code == FunctionCode::WRITE_MULTIPLE_COILS; - const size_t expected_len = - bits ? (static_cast(number_of_entities) + 7) / 8 : static_cast(number_of_entities) * 2; + const size_t expected_len = bits ? packed_bit_bytes(number_of_entities) : static_cast(number_of_entities) * 2; if (values_len != expected_len) { ESP_LOGE(TAG, "values_len %zu does not match %u entities (expected %zu) for function code %02X, dropping request", values_len, number_of_entities, expected_len, static_cast(function_code)); @@ -420,11 +431,8 @@ PduBuffer create_client_pdu(FunctionCode function_code, uint16_t start_address, pdu.push_back(values_len); // Byte count is required for write multiple for (size_t i = 0; i < values_len; i++) pdu.push_back(values[i]); - // Zero the unused bits of the final byte as the spec requires, matching the typed coil builder - // so both produce identical wire bytes for the same write. - if (bits && number_of_entities % 8 != 0) { - pdu[pdu.size() - 1] &= static_cast((1 << (number_of_entities % 8)) - 1); - } + if (bits) + mask_trailing_pad_bits(pdu, number_of_entities); return pdu; } @@ -484,7 +492,7 @@ static void build_write_coils_pdu(PduBuffer &pdu, uint16_t start_address, Packed ESP_LOGE(TAG, "Write of %u coils at %u runs past the 16-bit address space, dropping request", count, start_address); return; } - const size_t byte_count = (count + 7) / 8; + const size_t byte_count = packed_bit_bytes(count); if (packed_bits.size() < byte_count) { ESP_LOGE(TAG, "packed_bits (%zu bytes) does not cover %u coils (%zu bytes), dropping request", packed_bits.size(), count, byte_count); @@ -495,10 +503,7 @@ static void build_write_coils_pdu(PduBuffer &pdu, uint16_t start_address, Packed for (size_t i = 0; i != byte_count; i++) { pdu.push_back(packed_bits[i]); } - // Zero the unused bits of the final byte, as the spec requires - if (count % 8 != 0) { - pdu[pdu.size() - 1] &= static_cast((1 << (count % 8)) - 1); - } + mask_trailing_pad_bits(pdu, count); } PduBuffer create_write_coils_pdu(uint16_t start_address, PackedBits bits) { @@ -515,7 +520,7 @@ PduBuffer create_write_coils_pdu(uint16_t start_address, std::span v MAX_NUM_OF_COILS_TO_WRITE); return pdu; } - StaticVector packed; + StaticVector packed; for (size_t i = 0; i != values.size(); i++) { if (i % 8 == 0) packed.push_back(0);