diff --git a/esphome/components/modbus/modbus.cpp b/esphome/components/modbus/modbus.cpp index 9f2527d9fb..901bfcc52e 100644 --- a/esphome/components/modbus/modbus.cpp +++ b/esphome/components/modbus/modbus.cpp @@ -1,4 +1,7 @@ #include "modbus.h" + +#include + #include "esphome/core/application.h" #include "esphome/core/helpers.h" #include "esphome/core/log.h" @@ -378,10 +381,9 @@ ModbusServerDevice *ModbusServerHub::find_device_(uint8_t address) { return nullptr; } -ResponseStatus ModbusServerHub::check_register_range_(uint16_t start_address, uint16_t number_of_registers) { - if ((uint32_t) start_address + number_of_registers > 0x10000u) { - ESP_LOGW(TAG, "Register address out of range - start: %" PRIu16 " num: %" PRIu16, start_address, - number_of_registers); +ResponseStatus ModbusServerHub::check_address_range_(uint16_t start_address, uint16_t count) { + if ((uint32_t) start_address + count > 0x10000u) { + ESP_LOGW(TAG, "Address out of range - start: %" PRIu16 " num: %" PRIu16, start_address, count); return ExceptionCode::ILLEGAL_DATA_ADDRESS; } return std::nullopt; @@ -394,6 +396,11 @@ static constexpr size_t WRITE_SINGLE_VALUES_OFFSET = 2; static constexpr size_t WRITE_MULTIPLE_VALUES_OFFSET = 5; // FC 0x17 writes follow read start(2) + read quantity(2) + write start(2) + write quantity(2) + byte count(1). static constexpr size_t READ_WRITE_VALUES_OFFSET = 9; +// A coil write (FC 0x0F) is function(1) + start(2) + quantity(2) + byte count(1) + packed bits. The largest +// one (MAX_NUM_OF_COILS_TO_WRITE coils) must fit the received request PDU, so the value subspan taken at +// WRITE_MULTIPLE_VALUES_OFFSET can never run past it. +static_assert(1 + WRITE_MULTIPLE_VALUES_OFFSET + packed_bit_bytes(MAX_NUM_OF_COILS_TO_WRITE) <= MAX_PDU_SIZE, + "the largest FC 0x0F coil write must fit within MAX_PDU_SIZE"); ResponseStatus ModbusServerHub::parse_write_single_(std::span data, uint16_t &start_address, RegisterValues ®isters) { @@ -413,13 +420,59 @@ ResponseStatus ModbusServerHub::parse_write_multiple_(std::span d ESP_LOGW(TAG, "Invalid number of registers %" PRIu16 " or bytes %" PRIu8, number_of_registers, number_of_bytes); return ExceptionCode::ILLEGAL_DATA_VALUE; } - if (ResponseStatus status = this->check_register_range_(start_address, number_of_registers); status.has_value()) { + if (ResponseStatus status = this->check_address_range_(start_address, number_of_registers); status.has_value()) { return status; } this->assemble_registers_(data.subspan(WRITE_MULTIPLE_VALUES_OFFSET, number_of_bytes), registers); return std::nullopt; } +ResponseStatus ModbusServerHub::parse_read_request_(std::span data, uint16_t max_entities, + const LogString *entity_name, uint16_t &start_address, + uint16_t &count) { + // Every read request is start address(2) + quantity(2); only the protocol ceiling differs per function + // code, so registers and coils/discrete inputs validate through here and cannot drift apart. + start_address = helpers::get_data(data.data(), 0); + count = helpers::get_data(data.data(), 2); + if (count == 0 || count > max_entities) { + ESP_LOGW(TAG, "Invalid number of %s %" PRIu16, LOG_STR_ARG(entity_name), count); + return ExceptionCode::ILLEGAL_DATA_VALUE; + } + return this->check_address_range_(start_address, count); +} + +ResponseStatus ModbusServerHub::parse_write_single_coil_(std::span data, uint16_t &start_address, + bool &value) { + start_address = helpers::get_data(data.data(), 0); + const uint16_t raw_value = helpers::get_data(data.data(), WRITE_SINGLE_VALUES_OFFSET); + if (raw_value != 0xFF00 && raw_value != 0x0000) { + ESP_LOGW(TAG, "Invalid coil value 0x%04X", raw_value); + return ExceptionCode::ILLEGAL_DATA_VALUE; + } + // No range check needed: one coil can never push start_address + 1 past the address space. + value = raw_value == 0xFF00; + return std::nullopt; +} + +ResponseStatus ModbusServerHub::parse_write_multiple_coils_(std::span data, uint16_t &start_address, + uint16_t &count, std::span &packed_bytes) { + start_address = helpers::get_data(data.data(), 0); + const uint16_t number_of_bits = helpers::get_data(data.data(), 2); + const uint8_t number_of_bytes = helpers::get_data(data.data(), 4); + if (number_of_bits == 0 || number_of_bits > MAX_NUM_OF_COILS_TO_WRITE || + packed_bit_bytes(number_of_bits) != number_of_bytes) { + ESP_LOGW(TAG, "Invalid number of coils %" PRIu16 " or bytes %" PRIu8, number_of_bits, number_of_bytes); + return ExceptionCode::ILLEGAL_DATA_VALUE; + } + if (ResponseStatus status = this->check_address_range_(start_address, number_of_bits); status.has_value()) { + return status; + } + count = number_of_bits; + // coil values follow start(2) + quantity(2) + byte count(1) + packed_bytes = data.subspan(WRITE_MULTIPLE_VALUES_OFFSET, number_of_bytes); + return std::nullopt; +} + void ModbusServerHub::assemble_registers_(std::span values, RegisterValues ®isters) { for (size_t offset = 0; offset + 1 < values.size(); offset += 2) { registers.push_back(helpers::get_data(values.data(), offset)); @@ -427,11 +480,16 @@ void ModbusServerHub::assemble_registers_(std::span values, Regis } void ModbusServerHub::process_broadcast_frame_(uint8_t function_code, std::span data) { - // Broadcasts are only meaningful for register writes and are never answered (Modbus 4.1 / 6.12), so an - // unsupported function code or a validation failure is silently dropped instead of replying with an exception. - // Coil writes (FC 0x05/0x0F) are also broadcastable by spec, but server coil handlers are not implemented yet. + // Broadcasts are only meaningful for writes and are never answered (Modbus 4.1 / 6.12), so an unsupported + // function code or a validation failure is silently dropped instead of replying with an exception. Both + // register writes (FC 0x06/0x10) and coil writes (FC 0x05/0x0F) are broadcastable by spec, and each shares + // its parser with the addressed path so a broadcast is validated exactly as the unicast form would be. uint16_t start_address; RegisterValues registers; + uint16_t coil_count = 0; + std::span packed_bytes; + uint8_t single_bit = 0; // backs packed_bytes for a single-coil write, so it must outlive the loop below + bool coils = false; ResponseStatus status; switch (static_cast(function_code)) { case FunctionCode::WRITE_SINGLE_REGISTER: @@ -440,6 +498,19 @@ void ModbusServerHub::process_broadcast_frame_(uint8_t function_code, std::span< case FunctionCode::WRITE_MULTIPLE_REGISTERS: status = this->parse_write_multiple_(data, start_address, registers); break; + case FunctionCode::WRITE_SINGLE_COIL: { + coils = true; + bool value = false; + status = this->parse_write_single_coil_(data, start_address, value); + single_bit = value ? 0x01 : 0x00; + coil_count = 1; + packed_bytes = std::span(&single_bit, 1); + break; + } + case FunctionCode::WRITE_MULTIPLE_COILS: + coils = true; + status = this->parse_write_multiple_coils_(data, start_address, coil_count, packed_bytes); + break; default: // Reads and read/write require a reply, so they are not valid as broadcasts. ESP_LOGV(TAG, "Ignoring broadcast with unsupported function code %" PRIu8, function_code); @@ -452,8 +523,12 @@ void ModbusServerHub::process_broadcast_frame_(uint8_t function_code, std::span< // per-device outcome at V, and warn if the write reached nobody at all. bool accepted = false; for (auto *device : this->devices_) { - if (ResponseStatus device_status = device->on_broadcast_write_registers(start_address, registers); - device_status.has_value()) { + // Same handlers as an addressed write - a device cannot tell a broadcast apart, and does not need + // to: the hub owns the difference, which is only that no reply is ever sent. + const ResponseStatus device_status = + coils ? device->on_write_coils(start_address, PackedBits(packed_bytes, coil_count)) + : device->on_write_registers(start_address, registers); + if (device_status.has_value()) { ESP_LOGV(TAG, "Device %" PRIu8 " rejected broadcast write with exception %" PRIu8, device->get_address(), static_cast(device_status.value())); } else { @@ -461,15 +536,19 @@ void ModbusServerHub::process_broadcast_frame_(uint8_t function_code, std::span< } } if (!accepted && !this->devices_.empty()) { + const uint16_t entity_count = coils ? coil_count : static_cast(registers.size()); + const LogString *const entity_name = coils ? LOG_STR("coils") : LOG_STR("registers"); // Warn at most once per interval, then drop to VERBOSE: on a shared bus a broadcast aimed at other nodes // repeats forever, so warning per frame would flood the log. const uint32_t now = millis(); if (this->last_unaccepted_broadcast_warn_ == 0 || now - this->last_unaccepted_broadcast_warn_ > UNACCEPTED_BROADCAST_WARN_INTERVAL_MS) { this->last_unaccepted_broadcast_warn_ = now; - ESP_LOGW(TAG, "No device accepted broadcast write of %zu registers at 0x%04X", registers.size(), start_address); + ESP_LOGW(TAG, "No device accepted broadcast write of %" PRIu16 " %s at 0x%04X", entity_count, + LOG_STR_ARG(entity_name), start_address); } else { - ESP_LOGV(TAG, "No device accepted broadcast write of %zu registers at 0x%04X", registers.size(), start_address); + ESP_LOGV(TAG, "No device accepted broadcast write of %" PRIu16 " %s at 0x%04X", entity_count, + LOG_STR_ARG(entity_name), start_address); } } } @@ -479,8 +558,7 @@ bool ModbusServerHub::build_or_reject_read_response_(uint8_t address, uint8_t fu std::span response_buffer, uint16_t &response_len) { // A handler that returns an exception leaves registers partially filled, so check the exception // first and forward it before validating the register count on the success path. - if (status.has_value()) { - this->send_exception_(address, function_code, status.value()); + if (this->rejected_(address, function_code, status)) { return false; } @@ -535,17 +613,11 @@ void ModbusServerHub::process_modbus_client_frame_(uint8_t address, uint8_t func switch (static_cast(function_code)) { case FunctionCode::READ_HOLDING_REGISTERS: case FunctionCode::READ_INPUT_REGISTERS: { - // PDU data: start address(2) + quantity(2). - uint16_t start_address = helpers::get_data(data.data(), 0); - uint16_t number_of_registers = helpers::get_data(data.data(), 2); - if (number_of_registers == 0 || number_of_registers > MAX_NUM_OF_REGISTERS_TO_READ) { - ESP_LOGW(TAG, "Invalid number of registers %" PRIu16, number_of_registers); - this->send_exception_(address, function_code, ExceptionCode::ILLEGAL_DATA_VALUE); - return; - } - status = this->check_register_range_(start_address, number_of_registers); - if (status.has_value()) { - this->send_exception_(address, function_code, status.value()); + uint16_t start_address; + uint16_t number_of_registers; + status = this->parse_read_request_(data, MAX_NUM_OF_REGISTERS_TO_READ, LOG_STR("registers"), start_address, + number_of_registers); + if (this->rejected_(address, function_code, status)) { return; } RegisterValues registers; @@ -571,8 +643,7 @@ void ModbusServerHub::process_modbus_client_frame_(uint8_t address, uint8_t func } else { status = this->parse_write_multiple_(data, start_address, registers); } - if (status.has_value()) { - this->send_exception_(address, function_code, status.value()); + if (this->rejected_(address, function_code, status)) { return; } status = device->on_write_registers(start_address, registers); @@ -580,6 +651,64 @@ void ModbusServerHub::process_modbus_client_frame_(uint8_t address, uint8_t func response_len = 4; break; } + case FunctionCode::READ_COILS: + case FunctionCode::READ_DISCRETE_INPUTS: { + uint16_t start_address; + uint16_t number_of_bits; + status = + this->parse_read_request_(data, MAX_NUM_OF_COILS_TO_READ, LOG_STR("bits"), start_address, number_of_bits); + if (this->rejected_(address, function_code, status)) { + return; + } + // Response: byte count(1) + packed bytes, written straight into the pre-zeroed response buffer. It + // always fits: the parse above caps the count, and a static_assert bounds that against MAX_RAW_SIZE. + const uint8_t byte_count = static_cast(packed_bit_bytes(number_of_bits)); + response_buffer[response_len++] = byte_count; + // Take the packed-bytes span off a span that knows response_buffer's real size, so a future non-zero + // response_len (e.g. a prefix written before the packed data) is a bounds error, not a silent overrun. + std::span packed_out = std::span(response_buffer).subspan(response_len, byte_count); + std::fill(packed_out.begin(), packed_out.end(), 0); + MutablePackedBits bits(packed_out, number_of_bits); + if (static_cast(function_code) == FunctionCode::READ_COILS) { + status = device->on_read_coils(start_address, bits); + } else { + status = device->on_read_discrete_inputs(start_address, bits); + } + if (this->rejected_(address, function_code, status)) { + return; + } + response_len += byte_count; + break; + } + case FunctionCode::WRITE_SINGLE_COIL: { + // A single coil is handed to the device as a one-bit packed view, the same form a multiple-coil + // write takes, so a device only ever implements one coil write handler. + uint16_t start_address; + bool value = false; + status = this->parse_write_single_coil_(data, start_address, value); + if (this->rejected_(address, function_code, status)) { + return; + } + const uint8_t single_bit = value ? 0x01 : 0x00; + status = device->on_write_coils(start_address, PackedBits(std::span(&single_bit, 1), 1)); + response_data = data.data(); // echo the request header per Modbus 6.5, 6.11 + response_len = 4; + break; + } + case FunctionCode::WRITE_MULTIPLE_COILS: { + // Parse and validate the coil write PDU into a packed-bit view; reply with an exception on failure. + uint16_t start_address; + uint16_t count; + std::span packed_bytes; + status = this->parse_write_multiple_coils_(data, start_address, count, packed_bytes); + if (this->rejected_(address, function_code, status)) { + return; + } + status = device->on_write_coils(start_address, PackedBits(packed_bytes, count)); + response_data = data.data(); // echo the request header per Modbus 6.5, 6.11 + response_len = 4; + break; + } case FunctionCode::READ_WRITE_MULTIPLE_REGISTERS: { // PDU data: read start address(2) + read quantity(2) + write start address(2) + write quantity(2) + // write byte count(1) + write register values. Per Modbus 6.17 the write is performed before the read. @@ -596,12 +725,11 @@ void ModbusServerHub::process_modbus_client_frame_(uint8_t address, uint8_t func this->send_exception_(address, function_code, ExceptionCode::ILLEGAL_DATA_VALUE); return; } - status = this->check_register_range_(read_start_address, number_of_registers); + status = this->check_address_range_(read_start_address, number_of_registers); if (!status.has_value()) { - status = this->check_register_range_(write_start_address, number_of_write_registers); + status = this->check_address_range_(write_start_address, number_of_write_registers); } - if (status.has_value()) { - this->send_exception_(address, function_code, status.value()); + if (this->rejected_(address, function_code, status)) { return; } // Perform the write first (Modbus 6.17). Scoped so the write values are off the stack before the read @@ -614,8 +742,7 @@ void ModbusServerHub::process_modbus_client_frame_(uint8_t address, uint8_t func // from the values it just stored. status = device->on_write_registers(write_start_address, write_registers); } - if (status.has_value()) { - this->send_exception_(address, function_code, status.value()); + if (this->rejected_(address, function_code, status)) { return; } RegisterValues registers; @@ -632,9 +759,7 @@ void ModbusServerHub::process_modbus_client_frame_(uint8_t address, uint8_t func this->send_exception_(address, function_code, ExceptionCode::ILLEGAL_FUNCTION); return; } - if (status.has_value()) { - this->send_exception_(address, function_code, status.value()); - } else { + if (!this->rejected_(address, function_code, status)) { this->send_response_(address, function_code, response_data, response_len); } } @@ -733,6 +858,19 @@ void ModbusServerHub::send_response_(uint8_t address, uint8_t function_code, con this->send_raw_(raw_frame, payload_len + 2); } +bool ModbusServerHub::rejected_(uint8_t address, uint8_t function_code, ResponseStatus status) { + if (!status.has_value()) + return false; + // The one place a rejection becomes an exception reply, so the log carries the transaction context a + // device handler never has: which client-facing address and function code drew which exception. DEBUG + // rather than WARN because an exception reply is a normal protocol outcome and arrives per frame - a + // probing or broken client would otherwise flood the log. The parse helpers still WARN with specifics. + ESP_LOGD(TAG, "Exception %" PRIu8 " replied to function 0x%02X for address %" PRIu8, + static_cast(status.value()), function_code, address); + this->send_exception_(address, function_code, status.value()); + return true; +} + void ModbusServerHub::send_exception_(uint8_t address, uint8_t function_code, ExceptionCode exception_code) { uint8_t raw_frame[3]; raw_frame[0] = address; diff --git a/esphome/components/modbus/modbus.h b/esphome/components/modbus/modbus.h index 3b6028e90a..6331f23f99 100644 --- a/esphome/components/modbus/modbus.h +++ b/esphome/components/modbus/modbus.h @@ -361,9 +361,27 @@ class ModbusServerHub : public Modbus { // Appends the big-endian register values in values to registers, in host byte order. void assemble_registers_(std::span values, RegisterValues ®isters); ModbusServerDevice *find_device_(uint8_t address); - // Returns std::nullopt if [start_address, start_address + number_of_registers) fits in the 16-bit address space, - // otherwise ILLEGAL_DATA_ADDRESS. The caller sends the exception reply if one is required. - ResponseStatus check_register_range_(uint16_t start_address, uint16_t number_of_registers); + // Returns std::nullopt if [start_address, start_address + count) fits in the 16-bit address space, + // otherwise ILLEGAL_DATA_ADDRESS. The caller sends the exception reply if one is required - a broadcast + // write is never answered, so the check cannot send it itself. Shared by the register and + // coil/discrete-input handlers, which all address the same 16-bit space. + ResponseStatus check_address_range_(uint16_t start_address, uint16_t count); + + // Parses a read request PDU (start address(2) + quantity(2)), shared by the register and + // coil/discrete-input reads so the two cannot drift apart. max_entities is the protocol ceiling for the + // function code; entity_name only labels the rejection log. + ResponseStatus parse_read_request_(std::span data, uint16_t max_entities, const LogString *entity_name, + uint16_t &start_address, uint16_t &count); + + // Parses a single-coil write PDU (FC 0x05), which carries a 2-byte on/off value rather than packed + // bytes. The caller packs value into a byte it owns to build the PackedBits view the handlers take. + ResponseStatus parse_write_single_coil_(std::span data, uint16_t &start_address, bool &value); + + // Parses a multiple-coil write PDU (FC 0x0F) into a packed-bit view pointing straight into the receive + // buffer, so the coil values are never copied. Both coil parsers are shared by the addressed and + // broadcast paths so the two validate identically. + ResponseStatus parse_write_multiple_coils_(std::span data, uint16_t &start_address, uint16_t &count, + std::span &packed_bytes); // Builds the body of a register read response (byte count followed by the big-endian register values) into // response_buffer. Shared by every function code that answers with register values, so the read reply stays @@ -374,6 +392,9 @@ class ModbusServerHub : public Modbus { uint16_t number_of_registers, const RegisterValues ®isters, std::span response_buffer, uint16_t &response_len); void send_raw_(const uint8_t *payload, uint16_t len); + // Sends and logs the exception reply when status holds one; returns true if the request was rejected. + // Every parse and handler rejection funnels through here, so the reply and its log cannot drift apart. + bool rejected_(uint8_t address, uint8_t function_code, ResponseStatus status); void send_exception_(uint8_t address, uint8_t function_code, ExceptionCode exception_code); void send_response_(uint8_t address, uint8_t function_code, const uint8_t *payload, uint16_t payload_len); uint8_t expecting_peer_response_{0}; @@ -644,18 +665,26 @@ class ModbusServerDevice { virtual ResponseStatus on_write_registers(uint16_t start_address, const RegisterValues ®isters) { return ExceptionCode::ILLEGAL_FUNCTION; }; - // Hub entry point for broadcast (address 0) writes, which are never answered. - ResponseStatus on_broadcast_write_registers(uint16_t start_address, const RegisterValues ®isters) { - this->broadcast_write_ = true; - ResponseStatus status = this->on_write_registers(start_address, registers); - this->broadcast_write_ = false; - return status; - } + /// Coil/discrete-input reads: set the requested bits (bit 0 = the coil at start_address) with + /// bits.set(). The view covers bits.size() pre-zeroed bits and writes land directly in the hub's + /// response buffer (no copy); it is only valid during the call. + virtual ResponseStatus on_read_bits(uint16_t start_address, MutablePackedBits bits) { + return ExceptionCode::ILLEGAL_FUNCTION; + }; + virtual ResponseStatus on_read_coils(uint16_t start_address, MutablePackedBits bits) { + return this->on_read_bits(start_address, bits); + }; + virtual ResponseStatus on_read_discrete_inputs(uint16_t start_address, MutablePackedBits bits) { + return this->on_read_bits(start_address, bits); + }; + /// Coil writes deliver the values as a PackedBits view over the hub's receive buffer (only valid + /// during the call). A single-coil write (FC 0x05) arrives as bits.size() == 1. + virtual ResponseStatus on_write_coils(uint16_t start_address, PackedBits bits) { + return ExceptionCode::ILLEGAL_FUNCTION; + }; protected: uint8_t address_{0}; - // Set while handling a broadcast write: the caller sends no reply, so a rejection has no wire consequence. - bool broadcast_write_{false}; }; } // namespace esphome::modbus diff --git a/esphome/components/modbus/modbus_definitions.h b/esphome/components/modbus/modbus_definitions.h index 9ec776b67a..64f7210585 100644 --- a/esphome/components/modbus/modbus_definitions.h +++ b/esphome/components/modbus/modbus_definitions.h @@ -128,6 +128,19 @@ static_assert(MAX_RAW_SIZE + 2 == MAX_FRAME_SIZE, "a framed raw server payload m /// Bits pack 8 per data byte, rounded up to whole bytes. constexpr size_t packed_bit_bytes(size_t bits) { return (bits + 7) / 8; } +// A coil/discrete-input read answers with byte count(1) + packed_bit_bytes(count) bytes, which has to fit +// the raw frame body. The runtime check on that path catches a caller entering with bytes already written; +// this catches the other way in, raising the ceiling past what a frame can carry. +static_assert(1 + packed_bit_bytes(MAX_NUM_OF_COILS_TO_READ) <= MAX_RAW_SIZE, + "MAX_NUM_OF_COILS_TO_READ yields a read response larger than MAX_RAW_SIZE"); +static_assert(1 + packed_bit_bytes(MAX_NUM_OF_DISCRETE_INPUTS_TO_READ) <= MAX_RAW_SIZE, + "MAX_NUM_OF_DISCRETE_INPUTS_TO_READ yields a read response larger than MAX_RAW_SIZE"); + +// The coil and discrete-input ceilings are separate limits in the spec but hold the same value, so the +// read paths validate both against MAX_NUM_OF_COILS_TO_READ. Should the spec ever split them, this fires. +static_assert(MAX_NUM_OF_COILS_TO_READ == MAX_NUM_OF_DISCRETE_INPUTS_TO_READ, + "the coil and discrete-input read ceilings must match"); + /** Read-only view of Modbus-packed bits: bit 0 of byte 0 is the first bit (LSB first), the layout * coil/discrete-input values use on the wire. Bundles the bit count with the packed bytes so the * two cannot desynchronize. The view does not own the bytes - it is only valid while they are. diff --git a/esphome/components/modbus/modbus_helpers.cpp b/esphome/components/modbus/modbus_helpers.cpp index a0c8440c79..4287256101 100644 --- a/esphome/components/modbus/modbus_helpers.cpp +++ b/esphome/components/modbus/modbus_helpers.cpp @@ -69,8 +69,10 @@ uint16_t client_pdu_length(const uint8_t *frame, size_t size) { case FunctionCode::WRITE_SINGLE_REGISTER: return 5; // function(1) + output/register address(2) + value(2) case FunctionCode::WRITE_MULTIPLE_COILS: + // function(1) + start address(2) + quantity(2) + byte count(1) + packed coil data (8 coils per byte). + return 6 + (size > 5 ? std::min(frame[5], uint8_t(packed_bit_bytes(MAX_NUM_OF_COILS_TO_WRITE))) : 0); case FunctionCode::WRITE_MULTIPLE_REGISTERS: - // function(1) + start address(2) + quantity(2) + byte count(1) + data + // function(1) + start address(2) + quantity(2) + byte count(1) + register data (2 bytes per register). return 6 + (size > 5 ? std::min(frame[5], uint8_t(MAX_NUM_OF_REGISTERS_TO_WRITE * 2)) : 0); // Unsupported function codes. Included here to prevent parser failures. Excluding Serial Line specific functions. case FunctionCode::READ_FILE_RECORD: @@ -546,12 +548,7 @@ static PduBuffer create_write_coils_pdu_from_bools(uint16_t start_address, const return pdu; } CoilPackBuffer packed; - for (size_t i = 0; i != count; i++) { - if (i % 8 == 0) - packed.push_back(0); - if (values[i]) - packed[i / 8] |= (1 << (i % 8)); - } + pack_bits(packed, values); build_write_coils_pdu(pdu, start_address, PackedBits(std::span(packed.data(), packed.size()), count)); return pdu; } diff --git a/esphome/components/modbus/modbus_helpers.h b/esphome/components/modbus/modbus_helpers.h index 2c312b8a61..e47a6835cd 100644 --- a/esphome/components/modbus/modbus_helpers.h +++ b/esphome/components/modbus/modbus_helpers.h @@ -257,6 +257,29 @@ inline bool bit_from_packed(int bit, std::span data) { ESPDEPRECATED("Use bit_from_packed() instead. Removed in 2027.2.0", "2026.8.0") inline bool coil_from_vector(int coil, std::span data) { return bit_from_packed(coil, data); } +/** Append packed bytes (LSB first) for the given bits onto a growable byte container. + * push_back-based so callers can build a payload incrementally (e.g. a std::vector + * with no fixed upper bound). A non-byte-aligned count appends n+1 bytes, the last holding + * the remaining bits in its low positions. + * @param out destination byte container exposing push_back(uint8_t) + * @param bits container of bool exposing range-based iteration + */ +template void pack_bits(Out &out, const Bits &bits) { + uint8_t byte = 0; + uint8_t bit = 0; + for (bool b : bits) { + if (b) + byte |= (1 << bit); + if (++bit == 8) { + out.push_back(byte); + byte = 0; + bit = 0; + } + } + if (bit != 0) // flush the final partial byte + out.push_back(byte); +} + /** Extract bits from value and shift right according to the bitmask * if the bitmask is 0x00F0 we want the values frrom bit 5 - 8. * the result is then shifted right by the position if the first right set bit in the mask diff --git a/esphome/components/modbus_controller/modbus_controller.cpp b/esphome/components/modbus_controller/modbus_controller.cpp index da9d29887e..35f21fd0af 100644 --- a/esphome/components/modbus_controller/modbus_controller.cpp +++ b/esphome/components/modbus_controller/modbus_controller.cpp @@ -2,6 +2,8 @@ #include "esphome/core/application.h" #include "esphome/core/log.h" +#include + namespace esphome::modbus_controller { static const char *const TAG = "modbus_controller"; @@ -427,14 +429,15 @@ ModbusCommandItem ModbusCommandItem::create_write_multiple_coils(ModbusControlle modbusdevice->on_write_register_response(register_type, start_address, data); }; - uint8_t *p = cmd.payload.init((values.size() + 7) / 8); - memset(p, 0, (values.size() + 7) / 8); - size_t bit = 0; - for (auto coil : values) { - if (coil) { - p[bit / 8] |= (1 << (bit % 8)); - } - bit++; + // Pack through the shared bit view (MutablePackedBits) so the coil wire layout lives in one place + // instead of an open-coded loop. + const size_t byte_count = modbus::packed_bit_bytes(values.size()); + uint8_t *p = cmd.payload.init(byte_count); + memset(p, 0, byte_count); + modbus::MutablePackedBits bits(std::span(p, byte_count), static_cast(values.size())); + for (size_t i = 0; i != values.size(); i++) { + if (values[i]) + bits.set(i, true); } return cmd; } diff --git a/esphome/components/modbus_server/modbus_server.cpp b/esphome/components/modbus_server/modbus_server.cpp index bf39efbd54..e63495cb25 100644 --- a/esphome/components/modbus_server/modbus_server.cpp +++ b/esphome/components/modbus_server/modbus_server.cpp @@ -145,12 +145,9 @@ modbus::ResponseStatus ModbusServer::on_write_registers(uint16_t start_address, } return true; })) { - // On a broadcast every device that does not map these registers rejects them, which is the normal case. - if (this->broadcast_write_) { - ESP_LOGV(TAG, "Write request rejected before applying any register."); - } else { - ESP_LOGW(TAG, "Write request rejected before applying any register."); - } + // Only VERBOSE: one handler serves both addressed and broadcast writes, and rejecting a broadcast for + // registers this device does not map is routine. The hub logs the outcome with the context it has. + ESP_LOGV(TAG, "Write request rejected before applying any register."); return precheck; } diff --git a/tests/components/modbus/common.h b/tests/components/modbus/common.h index 659b72014c..d03ccf8ec3 100644 --- a/tests/components/modbus/common.h +++ b/tests/components/modbus/common.h @@ -1,5 +1,6 @@ #pragma once #include +#include #include "esphome/components/uart/uart_component.h" namespace esphome::modbus::testing { @@ -19,4 +20,14 @@ class NullUART : public uart::UARTComponent { void check_logger_conflict() override {} }; +// A UART that records every byte written so tests can assert on the exact wire response. +class RecordingUART : public NullUART { + public: + void write_array(const uint8_t *data, size_t len) override { + this->written.insert(this->written.end(), data, data + len); + } + + std::vector written; +}; + } // namespace esphome::modbus::testing diff --git a/tests/components/modbus/modbus_broadcast_test.cpp b/tests/components/modbus/modbus_broadcast_test.cpp index 5840259021..6f088f4888 100644 --- a/tests/components/modbus/modbus_broadcast_test.cpp +++ b/tests/components/modbus/modbus_broadcast_test.cpp @@ -28,6 +28,27 @@ class RecordingDevice : public ModbusServerDevice { std::vector last_values; }; +// A server device that records the coil writes the hub routes to it. Coils arrive as a PackedBits view +// over the hub's buffers, so the bits are copied out here rather than the view retained. +class RecordingCoilDevice : public ModbusServerDevice { + public: + explicit RecordingCoilDevice(uint8_t address) { this->set_address(address); } + + ResponseStatus on_write_coils(uint16_t start_address, PackedBits bits) override { + this->write_count++; + this->last_start_address = start_address; + this->last_bits.clear(); + for (uint16_t i = 0; i != bits.size(); i++) { + this->last_bits.push_back(bits[i]); + } + return std::nullopt; // return value is ignored for broadcasts, which are never answered + } + + int write_count{0}; + uint16_t last_start_address{0}; + std::vector last_bits; +}; + // A server device that rejects every write, to exercise the broadcast dispatch loop's rejection branch. class RejectingDevice : public ModbusServerDevice { public: @@ -41,15 +62,6 @@ class RejectingDevice : public ModbusServerDevice { int write_count{0}; }; -// A UART that records every byte written so the test can assert the hub sends no reply. -class RecordingUART : public testing::NullUART { - public: - void write_array(const uint8_t *data, size_t len) override { - this->written.insert(this->written.end(), data, data + len); - } - std::vector written; -}; - // Drives full frames through the server hub's receive path in tests. class TestServerHub : public ModbusServerHub { public: @@ -75,6 +87,8 @@ class TestServerHub : public ModbusServerHub { } // namespace +using testing::RecordingUART; + // A broadcast (address 0) single-register write reaches every registered device and is not answered. // Driven through the full receive parser (parse_modbus_frames) so the address-0 routing -- frame length, // CRC, and client-vs-broadcast dispatch -- is exercised, not just the handler below it. @@ -273,4 +287,83 @@ TEST(ModbusBroadcast, UnicastOutOfRangeWriteSendsSingleExceptionFrame) { EXPECT_EQ(uart.written[2], static_cast(ExceptionCode::ILLEGAL_DATA_ADDRESS)); } +// A broadcast single-coil write (FC 0x05) reaches every device and is not answered. The 2-byte ON value +// is normalized to a one-bit view, so the handler sees the same shape as a multiple-coil write of one. +TEST(ModbusBroadcast, SingleCoilWriteReachesAllDevicesWithoutReply) { + TestServerHub hub; + RecordingUART uart; + hub.set_uart_parent(&uart); + + RecordingCoilDevice device_a(0x02); + RecordingCoilDevice device_b(0x03); + hub.register_device(&device_a); + hub.register_device(&device_b); + + // FC 0x05 payload: coil 0x00AC, value 0xFF00 (ON). + const uint8_t pdu_data[] = {0x00, 0xAC, 0xFF, 0x00}; + ASSERT_TRUE(hub.run_receive_parser_for_test(BROADCAST_ADDRESS, static_cast(FunctionCode::WRITE_SINGLE_COIL), + pdu_data, sizeof(pdu_data))); + + for (RecordingCoilDevice *device : {&device_a, &device_b}) { + EXPECT_EQ(device->write_count, 1); + EXPECT_EQ(device->last_start_address, 0x00AC); + ASSERT_EQ(device->last_bits.size(), 1u); + EXPECT_TRUE(device->last_bits[0]); + } + EXPECT_TRUE(uart.written.empty()); // broadcasts are never answered +} + +// A broadcast multiple-coil write (FC 0x0F) delivers the packed bits to every device, LSB first. +TEST(ModbusBroadcast, MultipleCoilWriteReachesAllDevicesWithoutReply) { + TestServerHub hub; + RecordingUART uart; + hub.set_uart_parent(&uart); + + RecordingCoilDevice device_a(0x02); + RecordingCoilDevice device_b(0x03); + hub.register_device(&device_a); + hub.register_device(&device_b); + + // FC 0x0F payload: start 0x0013, 10 coils, 2 bytes, 0xCD 0x01 -> bit 0 set, bit 8 set. + const uint8_t pdu_data[] = {0x00, 0x13, 0x00, 0x0A, 0x02, 0xCD, 0x01}; + ASSERT_TRUE(hub.run_receive_parser_for_test( + BROADCAST_ADDRESS, static_cast(FunctionCode::WRITE_MULTIPLE_COILS), pdu_data, sizeof(pdu_data))); + + for (RecordingCoilDevice *device : {&device_a, &device_b}) { + EXPECT_EQ(device->write_count, 1); + EXPECT_EQ(device->last_start_address, 0x0013); + ASSERT_EQ(device->last_bits.size(), 10u); + EXPECT_TRUE(device->last_bits[0]); // 0xCD bit 0 + EXPECT_FALSE(device->last_bits[1]); // 0xCD bit 1 + EXPECT_TRUE(device->last_bits[8]); // 0x01 bit 0 + EXPECT_FALSE(device->last_bits[9]); // padding bit + } + EXPECT_TRUE(uart.written.empty()); +} + +// A coil broadcast that fails validation is dropped exactly like a bad register broadcast: no handler +// call and, because broadcasts are never answered, no exception frame either. +TEST(ModbusBroadcast, InvalidCoilBroadcastProducesNoWriteAndNoReply) { + TestServerHub hub; + RecordingUART uart; + hub.set_uart_parent(&uart); + + RecordingCoilDevice device(0x02); + hub.register_device(&device); + + // Byte count disagrees with the coil quantity: 10 coils need 2 bytes, not 1. + const uint8_t bad_count[] = {0x00, 0x13, 0x00, 0x0A, 0x01, 0xCD}; + ASSERT_TRUE(hub.run_receive_parser_for_test( + BROADCAST_ADDRESS, static_cast(FunctionCode::WRITE_MULTIPLE_COILS), bad_count, sizeof(bad_count))); + EXPECT_EQ(device.write_count, 0); + + // A single-coil value must be 0x0000 or 0xFF00; anything else is out of spec. + const uint8_t bad_value[] = {0x00, 0xAC, 0x12, 0x34}; + ASSERT_TRUE(hub.run_receive_parser_for_test(BROADCAST_ADDRESS, static_cast(FunctionCode::WRITE_SINGLE_COIL), + bad_value, sizeof(bad_value))); + EXPECT_EQ(device.write_count, 0); + + EXPECT_TRUE(uart.written.empty()); +} + } // namespace esphome::modbus diff --git a/tests/components/modbus/modbus_helpers_test.cpp b/tests/components/modbus/modbus_helpers_test.cpp index 768c23c33c..6a65c3bf68 100644 --- a/tests/components/modbus/modbus_helpers_test.cpp +++ b/tests/components/modbus/modbus_helpers_test.cpp @@ -426,6 +426,20 @@ TEST(ModbusHelpersTest, RegistersToNumberRejectsTruncatedMultiRegisterValue) { EXPECT_FALSE(registers_to_number(registers, 1, SensorValueType::U_DWORD).has_value()); } +// --- packed bit helpers ------------------------------------------------------ + +TEST(ModbusHelpersTest, PackBitsAppendsToContainer) { + // Bits are packed LSB first: the first value is bit 0 of the first byte, and the push_back + // overload appends packed bytes onto a growable container preserving existing content. + std::vector bits{true, false, true, true, false, false, false, false, true, true}; + std::vector out{0x55}; // pre-existing content must be preserved + pack_bits(out, bits); + ASSERT_EQ(out.size(), 3u); // leading byte + 2 packed bytes (10 bits) + EXPECT_EQ(out[0], 0x55); + EXPECT_EQ(out[1], 0x0D); // 0b00001101 + EXPECT_EQ(out[2], 0x03); // bits 8 and 9 -> bits 0,1 of second byte +} + // --- typed builders ---------------------------------------------------------- TEST(ModbusTypedBuilders, ReadPduWireBytes) { diff --git a/tests/components/modbus/modbus_server_coils_test.cpp b/tests/components/modbus/modbus_server_coils_test.cpp new file mode 100644 index 0000000000..e4bf3f6b14 --- /dev/null +++ b/tests/components/modbus/modbus_server_coils_test.cpp @@ -0,0 +1,398 @@ +#include + +#include +#include +#include + +#include "common.h" +#include "esphome/components/modbus/modbus.h" +#include "esphome/core/hal.h" + +namespace esphome::modbus { + +namespace { + +// A server device backed by a small coil array: reads deliver the stored bits, writes apply them. +class CoilDevice : public ModbusServerDevice { + public: + explicit CoilDevice(uint8_t address) { this->set_address(address); } + + ResponseStatus on_read_coils(uint16_t start_address, MutablePackedBits bits) override { + this->read_count++; + for (uint16_t i = 0; i < bits.size(); i++) + bits.set(i, this->coils[start_address + i]); + return std::nullopt; + } + + ResponseStatus on_write_coils(uint16_t start_address, PackedBits bits) override { + this->write_count++; + this->last_write_count = bits.size(); + for (uint16_t i = 0; i < bits.size(); i++) + this->coils[start_address + i] = bits[i]; + return std::nullopt; + } + + bool coils[32] = {}; + int read_count{0}; + int write_count{0}; + uint16_t last_write_count{0}; +}; + +// A device with no bit handlers, to exercise the ILLEGAL_FUNCTION defaults. +class NoBitsDevice : public ModbusServerDevice { + public: + explicit NoBitsDevice(uint8_t address) { this->set_address(address); } +}; + +// Distinguishes the two bit-read entry points: each fills a different pattern and counts its calls, so a +// test can prove FC 0x01 vs 0x02 dispatch routes to the right handler (and not merely that bits came back). +class DualReadDevice : public ModbusServerDevice { + public: + explicit DualReadDevice(uint8_t address) { this->set_address(address); } + + ResponseStatus on_read_coils(uint16_t start_address, MutablePackedBits bits) override { + this->coil_reads++; + bits.set(0, true); // pattern 0x01 + return std::nullopt; + } + ResponseStatus on_read_discrete_inputs(uint16_t start_address, MutablePackedBits bits) override { + this->discrete_reads++; + bits.set(1, true); // pattern 0x02 + return std::nullopt; + } + + int coil_reads{0}; + int discrete_reads{0}; +}; + +// Overrides only on_read_bits() - the shared fallback the header documents that on_read_coils() and +// on_read_discrete_inputs() default to. Both FC 0x01 and FC 0x02 must reach it. +class BitsOnlyDevice : public ModbusServerDevice { + public: + explicit BitsOnlyDevice(uint8_t address) { this->set_address(address); } + ResponseStatus on_read_bits(uint16_t start_address, MutablePackedBits bits) override { + this->calls++; + bits.set(0, true); // set bit 0 so the response proves the fallback ran + return std::nullopt; + } + int calls{0}; +}; + +using testing::RecordingUART; + +// Exposes the client-frame parser so a fully CRC-framed request can be pushed through the hub. +class TestServerHub : public ModbusServerHub { + public: + bool tx_blocked() override { return false; } + + void prime_send_timestamps_for_test() { + uint32_t now = millis(); + this->last_modbus_byte_ = now; + this->last_send_ = now; + } + + bool process_full_client_frame_for_test(uint8_t address, uint8_t function_code, const uint8_t *pdu_data, + size_t pdu_data_len) { + this->rx_buffer_.clear(); + this->rx_buffer_.reserve(pdu_data_len + 4); + this->rx_buffer_.push_back(address); + this->rx_buffer_.push_back(function_code); + this->rx_buffer_.insert(this->rx_buffer_.end(), pdu_data, pdu_data + pdu_data_len); + 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); + return this->parse_modbus_client_frame_(); + } +}; + +struct CoilFixture { + CoilFixture() { + hub.set_uart_parent(&uart); + hub.prime_send_timestamps_for_test(); + hub.register_device(&device); + } + TestServerHub hub; + RecordingUART uart; + CoilDevice device{0x02}; +}; + +} // namespace + +// A coil read returns byte count + packed bits, set by the handler directly in the response buffer. +TEST(ModbusServerCoils, ReadCoilsReturnsPackedBits) { + CoilFixture f; + f.device.coils[0] = true; + f.device.coils[2] = true; + f.device.coils[3] = true; + f.device.coils[9] = true; + + // FC 0x01: start 0x0000, quantity 10 -> 2 packed bytes + const uint8_t pdu_data[] = {0x00, 0x00, 0x00, 0x0A}; + ASSERT_TRUE(f.hub.process_full_client_frame_for_test(0x02, static_cast(FunctionCode::READ_COILS), pdu_data, + sizeof(pdu_data))); + + EXPECT_EQ(f.device.read_count, 1); + // Response: address(1) + fc(1) + byte count(1) + packed(2) + CRC(2) + ASSERT_EQ(f.uart.written.size(), 7u); + EXPECT_EQ(f.uart.written[0], 0x02); + EXPECT_EQ(f.uart.written[1], static_cast(FunctionCode::READ_COILS)); + EXPECT_EQ(f.uart.written[2], 2u); // byte count + EXPECT_EQ(f.uart.written[3], 0x0D); // coils 0,2,3 + EXPECT_EQ(f.uart.written[4], 0x02); // coil 9 -> bit 1 of byte 1 +} + +// A device overriding only on_read_bits() - the documented fallback - still serves both FC 0x01 (coils) +// and FC 0x02 (discrete inputs), since on_read_coils()/on_read_discrete_inputs() default to it. +TEST(ModbusServerCoils, ReadBitsFallbackServesBothCoilsAndDiscreteInputs) { + TestServerHub hub; + RecordingUART uart; + hub.set_uart_parent(&uart); + hub.prime_send_timestamps_for_test(); + BitsOnlyDevice device{0x05}; + hub.register_device(&device); + + const uint8_t pdu_data[] = {0x00, 0x00, 0x00, 0x01}; // start 0x0000, quantity 1 + + ASSERT_TRUE(hub.process_full_client_frame_for_test(0x05, static_cast(FunctionCode::READ_COILS), pdu_data, + sizeof(pdu_data))); + EXPECT_EQ(device.calls, 1); + // address(1) + fc(1) + byte count(1) + packed(1) + CRC(2); bit 0 set -> 0x01 + ASSERT_EQ(uart.written.size(), 6u); + EXPECT_EQ(uart.written[1], static_cast(FunctionCode::READ_COILS)); + EXPECT_EQ(uart.written[3], 0x01); + + uart.written.clear(); + ASSERT_TRUE(hub.process_full_client_frame_for_test(0x05, static_cast(FunctionCode::READ_DISCRETE_INPUTS), + pdu_data, sizeof(pdu_data))); + EXPECT_EQ(device.calls, 2); + ASSERT_EQ(uart.written.size(), 6u); + EXPECT_EQ(uart.written[1], static_cast(FunctionCode::READ_DISCRETE_INPUTS)); + EXPECT_EQ(uart.written[3], 0x01); +} + +// A multiple-coil write hands the handler the packed wire bytes and echoes the request header. +TEST(ModbusServerCoils, WriteMultipleCoilsAppliesPackedBits) { + CoilFixture f; + + // FC 0x0F: start 0x0000, quantity 10, byte count 2, packed values 0x0D 0x02 + const uint8_t pdu_data[] = {0x00, 0x00, 0x00, 0x0A, 0x02, 0x0D, 0x02}; + ASSERT_TRUE(f.hub.process_full_client_frame_for_test(0x02, static_cast(FunctionCode::WRITE_MULTIPLE_COILS), + pdu_data, sizeof(pdu_data))); + + EXPECT_EQ(f.device.write_count, 1); + EXPECT_EQ(f.device.last_write_count, 10u); + EXPECT_TRUE(f.device.coils[0]); + EXPECT_FALSE(f.device.coils[1]); + EXPECT_TRUE(f.device.coils[2]); + EXPECT_TRUE(f.device.coils[3]); + EXPECT_TRUE(f.device.coils[9]); + EXPECT_FALSE(f.device.coils[10]); + // Response echoes start address + quantity: address(1) + fc(1) + start(2) + quantity(2) + CRC(2) + ASSERT_EQ(f.uart.written.size(), 8u); + EXPECT_EQ(f.uart.written[1], static_cast(FunctionCode::WRITE_MULTIPLE_COILS)); +} + +// A single-coil write (FC 0x05) is normalized to a one-bit packed buffer. +TEST(ModbusServerCoils, WriteSingleCoilNormalizedToOneBit) { + CoilFixture f; + + const uint8_t pdu_on[] = {0x00, 0x03, 0xFF, 0x00}; // coil 3 ON + ASSERT_TRUE(f.hub.process_full_client_frame_for_test(0x02, static_cast(FunctionCode::WRITE_SINGLE_COIL), + pdu_on, sizeof(pdu_on))); + EXPECT_EQ(f.device.last_write_count, 1u); + EXPECT_TRUE(f.device.coils[3]); + + f.uart.written.clear(); + f.hub.prime_send_timestamps_for_test(); + const uint8_t pdu_off[] = {0x00, 0x03, 0x00, 0x00}; // coil 3 OFF + ASSERT_TRUE(f.hub.process_full_client_frame_for_test(0x02, static_cast(FunctionCode::WRITE_SINGLE_COIL), + pdu_off, sizeof(pdu_off))); + EXPECT_FALSE(f.device.coils[3]); + EXPECT_EQ(f.device.write_count, 2); +} + +// An invalid single-coil value (not 0xFF00/0x0000) is rejected with ILLEGAL_DATA_VALUE, no write. +TEST(ModbusServerCoils, InvalidSingleCoilValueRejected) { + CoilFixture f; + + const uint8_t pdu_data[] = {0x00, 0x03, 0x12, 0x34}; + ASSERT_TRUE(f.hub.process_full_client_frame_for_test(0x02, static_cast(FunctionCode::WRITE_SINGLE_COIL), + pdu_data, sizeof(pdu_data))); + + EXPECT_EQ(f.device.write_count, 0); + ASSERT_EQ(f.uart.written.size(), 5u); // one exception frame + EXPECT_EQ(f.uart.written[1], static_cast(FunctionCode::WRITE_SINGLE_COIL) | 0x80); + EXPECT_EQ(f.uart.written[2], static_cast(ExceptionCode::ILLEGAL_DATA_VALUE)); +} + +// Read quantity validation lives in the shared read-request parser, so the register and bit reads cannot +// drift apart. These pin both ends of the range for coils; the register case below pins that the same +// parser is on that path too. +TEST(ModbusServerCoils, ZeroCoilReadQuantityRejected) { + CoilFixture f; + + // FC 0x01: start 0x0000, quantity 0 - a read of nothing is out of spec. + const uint8_t pdu_data[] = {0x00, 0x00, 0x00, 0x00}; + ASSERT_TRUE(f.hub.process_full_client_frame_for_test(0x02, static_cast(FunctionCode::READ_COILS), pdu_data, + sizeof(pdu_data))); + + EXPECT_EQ(f.device.read_count, 0); + ASSERT_EQ(f.uart.written.size(), 5u); // one exception frame + EXPECT_EQ(f.uart.written[1], static_cast(FunctionCode::READ_COILS) | 0x80); + EXPECT_EQ(f.uart.written[2], static_cast(ExceptionCode::ILLEGAL_DATA_VALUE)); +} + +TEST(ModbusServerCoils, OverLimitCoilReadQuantityRejected) { + CoilFixture f; + + // One past MAX_NUM_OF_COILS_TO_READ (2000 = 0x07D0), which no frame could carry anyway. + const uint8_t pdu_data[] = {0x00, 0x00, 0x07, 0xD1}; + ASSERT_TRUE(f.hub.process_full_client_frame_for_test(0x02, static_cast(FunctionCode::READ_COILS), pdu_data, + sizeof(pdu_data))); + + EXPECT_EQ(f.device.read_count, 0); + ASSERT_EQ(f.uart.written.size(), 5u); + EXPECT_EQ(f.uart.written[2], static_cast(ExceptionCode::ILLEGAL_DATA_VALUE)); +} + +// The register read path shares that parser, so a zero quantity is rejected there identically. Lives +// beside the coil cases deliberately: together they are what stops the shared parser being bypassed on +// one side without the other noticing. +TEST(ModbusServerCoils, ZeroRegisterReadQuantityRejectedByTheSameParser) { + CoilFixture f; + + const uint8_t pdu_data[] = {0x00, 0x00, 0x00, 0x00}; + ASSERT_TRUE(f.hub.process_full_client_frame_for_test(0x02, static_cast(FunctionCode::READ_HOLDING_REGISTERS), + pdu_data, sizeof(pdu_data))); + + ASSERT_EQ(f.uart.written.size(), 5u); + EXPECT_EQ(f.uart.written[1], static_cast(FunctionCode::READ_HOLDING_REGISTERS) | 0x80); + EXPECT_EQ(f.uart.written[2], static_cast(ExceptionCode::ILLEGAL_DATA_VALUE)); +} + +// A device without bit handlers rejects coil requests with ILLEGAL_FUNCTION via the defaults. +TEST(ModbusServerCoils, UnhandledCoilReadIsIllegalFunction) { + TestServerHub hub; + RecordingUART uart; + hub.set_uart_parent(&uart); + hub.prime_send_timestamps_for_test(); + NoBitsDevice device(0x02); + hub.register_device(&device); + + const uint8_t pdu_data[] = {0x00, 0x00, 0x00, 0x08}; + ASSERT_TRUE(hub.process_full_client_frame_for_test(0x02, static_cast(FunctionCode::READ_COILS), pdu_data, + sizeof(pdu_data))); + + ASSERT_EQ(uart.written.size(), 5u); + EXPECT_EQ(uart.written[1], static_cast(FunctionCode::READ_COILS) | 0x80); + EXPECT_EQ(uart.written[2], static_cast(ExceptionCode::ILLEGAL_FUNCTION)); +} + +// The view contracts are enforced, not merely documented: bytes() returns exactly ceil(size()/8) bytes +// even over a larger buffer (forwarding it can never leak trailing buffer content), and set() drops +// out-of-range bits instead of writing past the span (on the server read path that span wraps a stack +// response buffer). +TEST(ModbusServerCoils, PackedBitsViewContractsEnforced) { + uint8_t buf[8] = {}; + PackedBits view(buf, 10); // 10 bits -> 2 bytes, over an 8-byte buffer + EXPECT_EQ(view.bytes().size(), 2u); + + PackedBits short_view(std::span(buf, 1), 10); // contract-violating: 10 bits over 1 byte + EXPECT_EQ(short_view.bytes().size(), 1u); // clamped to the real span, not a fabricated 2-byte span + + MutablePackedBits bits(std::span(buf, 2), 10); + bits.set(9, true); // in range: lands in byte 1 + bits.set(10, true); // out of range: dropped + bits.set(300, true); // far out of range: dropped, no write past the span + EXPECT_EQ(buf[1], 0x02); + for (size_t i = 2; i < sizeof(buf); i++) + EXPECT_EQ(buf[i], 0) << "byte " << i; +} + +// FC 0x02 must dispatch to on_read_discrete_inputs, not on_read_coils: the two handlers fill different +// patterns, so a swapped dispatch would fail on both the counters and the wire bytes. +TEST(ModbusServerCoils, ReadDiscreteInputsDispatchesToItsOwnHandler) { + TestServerHub hub; + RecordingUART uart; + hub.set_uart_parent(&uart); + hub.prime_send_timestamps_for_test(); + DualReadDevice device(0x02); + hub.register_device(&device); + + const uint8_t pdu_data[] = {0x00, 0x00, 0x00, 0x08}; + ASSERT_TRUE(hub.process_full_client_frame_for_test(0x02, static_cast(FunctionCode::READ_DISCRETE_INPUTS), + pdu_data, sizeof(pdu_data))); + + EXPECT_EQ(device.discrete_reads, 1); + EXPECT_EQ(device.coil_reads, 0); + ASSERT_GE(uart.written.size(), 4u); + EXPECT_EQ(uart.written[1], static_cast(FunctionCode::READ_DISCRETE_INPUTS)); + EXPECT_EQ(uart.written[3], 0x02); // the discrete handler's pattern, not the coil handler's + + uart.written.clear(); + ASSERT_TRUE(hub.process_full_client_frame_for_test(0x02, static_cast(FunctionCode::READ_COILS), pdu_data, + sizeof(pdu_data))); + EXPECT_EQ(device.coil_reads, 1); + EXPECT_EQ(device.discrete_reads, 1); + ASSERT_GE(uart.written.size(), 4u); + EXPECT_EQ(uart.written[3], 0x01); +} + +// The write-side ILLEGAL_FUNCTION defaults: a device without bit handlers rejects coil writes too +// (single and multiple), mirroring the read-side default already covered above. +TEST(ModbusServerCoils, UnhandledCoilWriteIsIllegalFunction) { + TestServerHub hub; + RecordingUART uart; + hub.set_uart_parent(&uart); + hub.prime_send_timestamps_for_test(); + NoBitsDevice device(0x02); + hub.register_device(&device); + + const uint8_t single[] = {0x00, 0x03, 0xFF, 0x00}; + ASSERT_TRUE(hub.process_full_client_frame_for_test(0x02, static_cast(FunctionCode::WRITE_SINGLE_COIL), + single, sizeof(single))); + ASSERT_EQ(uart.written.size(), 5u); + EXPECT_EQ(uart.written[1], static_cast(FunctionCode::WRITE_SINGLE_COIL) | 0x80); + EXPECT_EQ(uart.written[2], static_cast(ExceptionCode::ILLEGAL_FUNCTION)); + + uart.written.clear(); + const uint8_t multiple[] = {0x00, 0x00, 0x00, 0x08, 0x01, 0xAA}; + ASSERT_TRUE(hub.process_full_client_frame_for_test(0x02, static_cast(FunctionCode::WRITE_MULTIPLE_COILS), + multiple, sizeof(multiple))); + ASSERT_EQ(uart.written.size(), 5u); + EXPECT_EQ(uart.written[1], static_cast(FunctionCode::WRITE_MULTIPLE_COILS) | 0x80); + EXPECT_EQ(uart.written[2], static_cast(ExceptionCode::ILLEGAL_FUNCTION)); +} + +// FC 0x0F with a byte count that does not match ceil(quantity / 8) is ILLEGAL_DATA_VALUE and never +// reaches the handler. +TEST(ModbusServerCoils, WriteCoilsByteCountMismatchRejected) { + CoilFixture f; + + // quantity 10 needs 2 bytes; claim 1 + const uint8_t pdu_data[] = {0x00, 0x00, 0x00, 0x0A, 0x01, 0xFF}; + ASSERT_TRUE(f.hub.process_full_client_frame_for_test(0x02, static_cast(FunctionCode::WRITE_MULTIPLE_COILS), + pdu_data, sizeof(pdu_data))); + + ASSERT_EQ(f.uart.written.size(), 5u); + EXPECT_EQ(f.uart.written[1], static_cast(FunctionCode::WRITE_MULTIPLE_COILS) | 0x80); + EXPECT_EQ(f.uart.written[2], static_cast(ExceptionCode::ILLEGAL_DATA_VALUE)); + EXPECT_EQ(f.device.write_count, 0); +} + +// A coil range that runs past address 0xFFFF is ILLEGAL_DATA_ADDRESS and never reaches the handler. +TEST(ModbusServerCoils, CoilAddressRangeOverflowRejected) { + CoilFixture f; + + // start 0xFFF8, quantity 16 -> 0x10008 > 0x10000 + const uint8_t pdu_data[] = {0xFF, 0xF8, 0x00, 0x10}; + ASSERT_TRUE(f.hub.process_full_client_frame_for_test(0x02, static_cast(FunctionCode::READ_COILS), pdu_data, + sizeof(pdu_data))); + + ASSERT_EQ(f.uart.written.size(), 5u); + EXPECT_EQ(f.uart.written[1], static_cast(FunctionCode::READ_COILS) | 0x80); + EXPECT_EQ(f.uart.written[2], static_cast(ExceptionCode::ILLEGAL_DATA_ADDRESS)); + EXPECT_EQ(f.device.read_count, 0); +} + +} // namespace esphome::modbus diff --git a/tests/components/modbus_controller/command_payload_test.cpp b/tests/components/modbus_controller/command_payload_test.cpp new file mode 100644 index 0000000000..c125a44da5 --- /dev/null +++ b/tests/components/modbus_controller/command_payload_test.cpp @@ -0,0 +1,31 @@ +#include + +#include +#include + +#include "esphome/components/modbus_controller/modbus_controller.h" + +namespace esphome::modbus_controller::testing { + +// The coil write factory packs into an exact-size payload. Pinned at one past the protocol maximum +// because a fixed pack buffer sized for the maximum would silently truncate there while the quantity +// field still claimed every coil - and the truncated frame would fit the RTU limit and go on the wire +// malformed. Built at its true byte count, the oversize frame is refused by the hub's size check with +// a log instead. +TEST(ModbusCommandPayload, CoilWritePayloadIsExactSizedNotTruncated) { + ModbusController controller; + std::vector coils(modbus::MAX_NUM_OF_COILS_TO_WRITE + 1, true); + auto cmd = ModbusCommandItem::create_write_multiple_coils(&controller, 0x10, coils); + EXPECT_EQ(cmd.payload.size(), modbus::packed_bit_bytes(coils.size())); +} + +// LSB-first packing with zeroed pad bits, matching the wire layout the PDU builders produce. +TEST(ModbusCommandPayload, CoilWritePacksLsbFirstWithZeroPad) { + ModbusController controller; + const std::vector coils{true, false, true, true}; + auto cmd = ModbusCommandItem::create_write_multiple_coils(&controller, 0x10, coils); + ASSERT_EQ(cmd.payload.size(), 1u); + EXPECT_EQ(cmd.payload.data()[0], 0b00001101); +} + +} // namespace esphome::modbus_controller::testing