diff --git a/esphome/components/growatt_solar/growatt_solar.cpp b/esphome/components/growatt_solar/growatt_solar.cpp index fc35271017a..6485e90c25a 100644 --- a/esphome/components/growatt_solar/growatt_solar.cpp +++ b/esphome/components/growatt_solar/growatt_solar.cpp @@ -35,7 +35,8 @@ void GrowattSolar::update() { this->last_send_ = millis(); } -void GrowattSolar::on_modbus_data(const std::vector &data) { +void GrowattSolar::on_response(std::span request_pdu, std::span response_pdu) { + auto data = modbus::helpers::server_pdu_payload(response_pdu); // Other components might be sending commands to our device. But we don't get called with enough // context to know what is what. So if we didn't do a send, we ignore the data. if (!this->last_send_) diff --git a/esphome/components/growatt_solar/growatt_solar.h b/esphome/components/growatt_solar/growatt_solar.h index 18a7c917d5d..a172f49001e 100644 --- a/esphome/components/growatt_solar/growatt_solar.h +++ b/esphome/components/growatt_solar/growatt_solar.h @@ -4,7 +4,7 @@ #include "esphome/components/sensor/sensor.h" #include "esphome/components/modbus/modbus.h" -#include +#include namespace esphome::growatt_solar { @@ -69,7 +69,7 @@ class GrowattSolar final : public PollingComponent, public modbus::ModbusClientD public: void loop() override; void update() override; - void on_modbus_data(const std::vector &data) override; + void on_response(std::span request_pdu, std::span response_pdu) override; void dump_config() override; void set_protocol_version(GrowattProtocolVersion protocol_version) { this->protocol_version_ = protocol_version; } diff --git a/esphome/components/havells_solar/havells_solar.cpp b/esphome/components/havells_solar/havells_solar.cpp index 9257a37fd92..45e57544db0 100644 --- a/esphome/components/havells_solar/havells_solar.cpp +++ b/esphome/components/havells_solar/havells_solar.cpp @@ -10,7 +10,8 @@ static const char *const TAG = "havells_solar"; static const uint8_t MODBUS_CMD_READ_IN_REGISTERS = 0x03; static const uint8_t MODBUS_REGISTER_COUNT = 48; // 48 x 16-bit registers -void HavellsSolar::on_modbus_data(const std::vector &data) { +void HavellsSolar::on_response(std::span request_pdu, std::span response_pdu) { + auto data = modbus::helpers::server_pdu_payload(response_pdu); if (data.size() < MODBUS_REGISTER_COUNT * 2) { ESP_LOGW(TAG, "Invalid size for HavellsSolar!"); return; diff --git a/esphome/components/havells_solar/havells_solar.h b/esphome/components/havells_solar/havells_solar.h index 02e999c56ce..ed5d13b8b61 100644 --- a/esphome/components/havells_solar/havells_solar.h +++ b/esphome/components/havells_solar/havells_solar.h @@ -4,7 +4,7 @@ #include "esphome/components/sensor/sensor.h" #include "esphome/components/modbus/modbus.h" -#include +#include namespace esphome::havells_solar { @@ -77,7 +77,7 @@ class HavellsSolar final : public PollingComponent, public modbus::ModbusClientD void update() override; - void on_modbus_data(const std::vector &data) override; + void on_response(std::span request_pdu, std::span response_pdu) override; void dump_config() override; diff --git a/esphome/components/kuntze/kuntze.cpp b/esphome/components/kuntze/kuntze.cpp index 6df114e93c0..1475ca61ae7 100644 --- a/esphome/components/kuntze/kuntze.cpp +++ b/esphome/components/kuntze/kuntze.cpp @@ -13,7 +13,8 @@ static const uint16_t REGISTER[] = {4136, 4160, 4680, 6000, 4688, 4728, 5832}; // Maximum bytes to log for Modbus responses (2 registers = 4, plus count = 5) static constexpr size_t KUNTZE_MAX_LOG_BYTES = 8; -void Kuntze::on_modbus_data(const std::vector &data) { +void Kuntze::on_response(std::span request_pdu, std::span response_pdu) { + auto data = modbus::helpers::server_pdu_payload(response_pdu); auto get_16bit = [&](int i) -> uint16_t { return (uint16_t(data[i * 2]) << 8) | uint16_t(data[i * 2 + 1]); }; this->waiting_ = false; diff --git a/esphome/components/kuntze/kuntze.h b/esphome/components/kuntze/kuntze.h index 46681843d2a..28c80897483 100644 --- a/esphome/components/kuntze/kuntze.h +++ b/esphome/components/kuntze/kuntze.h @@ -4,6 +4,8 @@ #include "esphome/components/sensor/sensor.h" #include "esphome/components/modbus/modbus.h" +#include + namespace esphome::kuntze { class Kuntze final : public PollingComponent, public modbus::ModbusClientDevice { @@ -19,7 +21,7 @@ class Kuntze final : public PollingComponent, public modbus::ModbusClientDevice void loop() override; void update() override; - void on_modbus_data(const std::vector &data) override; + void on_response(std::span request_pdu, std::span response_pdu) override; void dump_config() override; diff --git a/esphome/components/modbus/modbus.cpp b/esphome/components/modbus/modbus.cpp index ecb2e4461cb..7f90dcd738e 100644 --- a/esphome/components/modbus/modbus.cpp +++ b/esphome/components/modbus/modbus.cpp @@ -214,11 +214,10 @@ bool Modbus::parse_modbus_server_frame_() { // Process before clearing: process_modbus_server_frame (receiving a response or peer message) never sends a reply // synchronously. We can safely point directly into rx_buffer_ and avoid a copy. - uint8_t data_offset = helpers::server_frame_data_offset(this->rx_buffer_.data(), this->rx_buffer_.size()); - const uint8_t *data = this->rx_buffer_.data() + data_offset; - uint16_t data_len = frame_length - 2 - data_offset; + // The PDU is the frame without the leading address and the trailing CRC. + std::span pdu(this->rx_buffer_.data() + 1, frame_length - 3); - this->process_modbus_server_frame(address, function_code, data, data_len); + this->process_modbus_server_frame(address, pdu); this->clear_rx_buffer_(LOG_STR("parse succeeded"), false, frame_length); return true; @@ -258,8 +257,16 @@ bool ModbusServerHub::parse_modbus_client_frame_() { return true; } -void ModbusClientHub::process_modbus_server_frame(uint8_t address, uint8_t function_code, const uint8_t *data, - uint16_t len) { +// Bounds contract, enforced by the parser (parse_modbus_server_frame_) rather than locally: +// - pdu is never empty: helpers::server_frame_length() returns at least MIN_FRAME_SIZE (4) on every +// branch, and find_custom_frame_end_() only ever lengthens that, so the PDU (frame minus address +// and CRC) always holds at least the function code. +// - When the exception bit is set, pdu has at least 2 bytes: server_frame_length() checks the +// exception bit before anything else and pins those frames to 5 bytes, so the exception code +// read below is always present. +// Keep those guarantees in mind when changing server_frame_length() or adding callers. +void ModbusClientHub::process_modbus_server_frame(uint8_t address, std::span pdu) { + const uint8_t function_code = pdu[0]; if (!this->waiting_for_response_.has_value()) { ESP_LOGW(TAG, "Received unexpected frame from address %" PRIu8 ", function code 0x%X, %" PRIu32 "ms after last send", @@ -292,20 +299,23 @@ void ModbusClientHub::process_modbus_server_frame(uint8_t address, uint8_t funct return; } else { // We have a valid device waiting for this response - ModbusClientDevice *device = wfr.device; + // Move the command out of the waiting slot so the request PDU stays alive for the callback. + ModbusDeviceCommand command = std::move(this->waiting_for_response_.value()); this->waiting_for_response_.reset(); + ModbusClientDevice *device = command.device; + // The request PDU is the sent frame without the leading address and the trailing CRC. + std::span request_pdu(command.frame.data.data() + 1, command.frame.size() - 3); // Is it an error response? if (helpers::is_function_code_exception(function_code)) { - uint8_t exception = len > 0 ? data[0] : 0; + uint8_t exception = pdu[1]; // exception frames are fixed-length, so the code is always present ESP_LOGW(TAG, "Error function code: 0x%X exception: %" PRIu8 ", address: %" PRIu8 ", %" PRIu32 "ms after last send", function_code, exception, address, this->last_modbus_byte_ - this->last_send_); if (device) - device->on_modbus_error(function_code & FUNCTION_CODE_MASK, exception); + device->on_error(request_pdu, static_cast(exception)); } else if (device) { // Not an error response - // on_modbus_data is existing public API taking const std::vector& - device->on_modbus_data(std::vector(data, data + len)); + device->on_response(request_pdu, pdu); } else { // Not an error response, but no device to respond to ESP_LOGV(TAG, "Ignoring response from %" PRIu8 " - no callback device set, %" PRIu32 "ms after last send", address, this->last_modbus_byte_ - this->last_send_); @@ -314,7 +324,7 @@ void ModbusClientHub::process_modbus_server_frame(uint8_t address, uint8_t funct } } -void ModbusServerHub::process_modbus_server_frame(uint8_t address, uint8_t function_code, const uint8_t *, uint16_t) { +void ModbusServerHub::process_modbus_server_frame(uint8_t address, std::span) { if (this->find_device_(address) != nullptr) { ESP_LOGE(TAG, "Unexpected response from address %" PRIu8 ", which is mapped to this device.", address); } @@ -503,7 +513,7 @@ void ModbusClientHub::send_next_frame_() { this->waiting_for_response_ = std::move(command); } else { if (command.device) - command.device->on_modbus_not_sent(); + command.device->on_not_sent(); } this->tx_buffer_.pop_front(); @@ -561,11 +571,10 @@ void ModbusServerHub::send_exception_(uint8_t address, uint8_t function_code, Mo this->send_raw_(raw_frame, 3); } -// Raw send for client: pushes to tx queue. Everything except the CRC must be contained in payload. void ModbusClientHub::notify_no_response_(ModbusDeviceCommand &wfr) { if (wfr.device == nullptr) return; - const bool retry = wfr.device->on_modbus_no_response(); + const bool retry = wfr.device->on_no_response(); // The callback may have detached the device (e.g. clear_tx_queue_for_device()); honor the detach // over the retry request rather than re-queueing a frame that can no longer be routed. if (retry && wfr.device != nullptr) @@ -579,17 +588,18 @@ void ModbusClientHub::requeue_waiting_frame_(ModbusDeviceCommand &wfr) { if (this->tx_buffer_.size() >= MODBUS_TX_BUFFER_SIZE) { ESP_LOGE(TAG, "Write buffer full, dropped retry for address %" PRIu8, frame.data.data()[0]); if (wfr.device != nullptr) - wfr.device->on_modbus_not_sent(); + wfr.device->on_not_sent(); return; } // Re-queue a copy (not a move): the waiting entry may have to survive as an interrupted shell. this->tx_buffer_.emplace_back(wfr.device, frame.data.data()[0], frame.data.data() + 1, frame.size() - 3); } +// Raw send for client: pushes to tx queue. Everything except the CRC must be contained in payload. void ModbusClientHub::queue_raw_(uint8_t address, const uint8_t *pdu, uint16_t pdu_len, ModbusClientDevice *device) { if (pdu_len == 0) { if (device) - device->on_modbus_not_sent(); + device->on_not_sent(); return; } @@ -605,7 +615,7 @@ void ModbusClientHub::queue_raw_(uint8_t address, const uint8_t *pdu, uint16_t p #endif ESP_LOGE(TAG, "Write buffer full, dropped: %" PRIu8 ":%s", address, format_hex_pretty_to(hex_buf, pdu, pdu_len)); if (device) - device->on_modbus_not_sent(); + device->on_not_sent(); } } @@ -644,7 +654,7 @@ void ModbusClientHub::clear_tx_queue_for_device(ModbusClientDevice *device) { void ModbusClientHub::send_raw(const std::vector &payload, ModbusClientDevice *device) { if (payload.size() < 2) { if (device) - device->on_modbus_not_sent(); + device->on_not_sent(); return; } this->queue_raw_(payload[0], payload.data() + 1, static_cast(payload.size() - 1), device); diff --git a/esphome/components/modbus/modbus.h b/esphome/components/modbus/modbus.h index eeba00f6b12..ee448245a49 100644 --- a/esphome/components/modbus/modbus.h +++ b/esphome/components/modbus/modbus.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -59,8 +60,8 @@ class Modbus : public uart::UARTDevice, public Component { virtual int32_t tx_delay_remaining(); virtual void parse_modbus_frames() = 0; bool parse_modbus_server_frame_(); - virtual void process_modbus_server_frame(uint8_t address, uint8_t function_code, const uint8_t *data, - uint16_t len) = 0; + // pdu is the whole PDU (function code + payload, no address/CRC); pdu[0] is the (standard or custom) function code. + virtual void process_modbus_server_frame(uint8_t address, std::span pdu) = 0; void clear_rx_buffer_(const LogString *reason, bool warn = false, size_t bytes_to_clear = 0); bool send_frame_(const ModbusFrame &frame); // Scans forward from min_length to find a frame boundary by CRC match for custom function codes. @@ -118,10 +119,9 @@ class ModbusClientHub : public Modbus { protected: int32_t tx_delay_remaining() override; void parse_modbus_frames() override; - // Parsers need to handle standard (ModbusFunctionCode) and custom (uint8_t) function codes, so we use uint8_t here. - void process_modbus_server_frame(uint8_t address, uint8_t function_code, const uint8_t *data, uint16_t len) override; + void process_modbus_server_frame(uint8_t address, std::span pdu) override; void send_next_frame_(); - // Notify the waiting device of no response; re-queues the frame if on_modbus_no_response() returns true. + // Notify the waiting device of no response; re-queues the frame if on_no_response() returns true. // wfr is the caller's checked reference to waiting_for_response_. void notify_no_response_(ModbusDeviceCommand &wfr); void requeue_waiting_frame_(ModbusDeviceCommand &wfr); @@ -146,8 +146,7 @@ class ModbusServerHub : public Modbus { protected: void parse_modbus_frames() override; bool parse_modbus_client_frame_(); - // Parsers need to handle standard (ModbusFunctionCode) and custom (uint8_t) function codes, so we use uint8_t here. - void process_modbus_server_frame(uint8_t address, uint8_t function_code, const uint8_t *data, uint16_t len) override; + void process_modbus_server_frame(uint8_t address, std::span pdu) override; void process_modbus_client_frame_(uint8_t address, uint8_t function_code, const uint8_t *data); ModbusServerDevice *find_device_(uint8_t address); // Returns true if [start_address, start_address + number_of_registers) fits in the 16-bit address space. @@ -180,12 +179,35 @@ class ModbusClientDevice { ModbusClientDevice &operator=(ModbusClientDevice &&) = delete; void set_parent(ModbusClientHub *parent) { this->parent_ = parent; } void set_address(uint8_t address) { this->address_ = address; } - virtual void on_modbus_data(const std::vector &data) {} - virtual void on_modbus_error(uint8_t function_code, uint8_t exception_code) {} - virtual void on_modbus_not_sent() {} + /// Called with the request PDU this device sent and the response PDU received (both: function code + + /// data, no address, no CRC). The spans are only valid for the duration of the call - copy the bytes + /// if they must outlive it. Slice the payload out of the response with helpers::server_pdu_payload(). + virtual void on_response(std::span request_pdu, std::span response_pdu) {} + /// Called with the request PDU and the modbus exception code decoded from the error response. + virtual void on_error(std::span request_pdu, ModbusExceptionCode exception_code) {} + // The on_modbus_* names are signature-identical renames, so the new defaults forward to the old + // virtuals: external devices overriding the old names keep working through the deprecation window. + // Remove the forwards together with the deprecated names. + virtual void on_not_sent() { +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" + this->on_modbus_not_sent(); +#pragma GCC diagnostic pop + } /// Called when no (valid) response arrived; return true to have the hub re-queue the frame for a retry. /// The hub does not bound retries: the device is responsible for limiting them (e.g. track a counter and /// return false when exhausted), or an unresponsive peer will starve other traffic on the bus. + virtual bool on_no_response() { +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" + return this->on_modbus_no_response(); +#pragma GCC diagnostic pop + } + // Remove before 2027.2.0 + ESPDEPRECATED("Override on_not_sent() instead. Removed in 2027.2.0", "2026.8.0") + virtual void on_modbus_not_sent() {} + // Remove before 2027.2.0 + ESPDEPRECATED("Override on_no_response() instead. Removed in 2027.2.0", "2026.8.0") virtual bool on_modbus_no_response() { return false; } void send(uint8_t function, uint16_t start_address, uint16_t number_of_entities, uint8_t payload_len = 0, const uint8_t *payload = nullptr) { diff --git a/esphome/components/modbus/modbus_helpers.h b/esphome/components/modbus/modbus_helpers.h index 45a13f75826..587f6838ee8 100644 --- a/esphome/components/modbus/modbus_helpers.h +++ b/esphome/components/modbus/modbus_helpers.h @@ -47,6 +47,8 @@ uint16_t server_frame_length(const uint8_t *frame, size_t size); // If the frame is too short to determine the length, returns the minimum length uint16_t client_frame_length(const uint8_t *frame, size_t size); +// Remove before 2027.2.0 +ESPDEPRECATED("Use server_pdu_payload() on the response PDU instead. Removed in 2027.2.0", "2026.8.0") inline uint8_t server_frame_data_offset(const uint8_t *frame, size_t size) { if (size < 2) return 0; @@ -61,6 +63,20 @@ inline uint8_t server_frame_data_offset(const uint8_t *frame, size_t size) { } } +/** Returns the payload portion of a server response PDU: the bytes after the function code, and for the + * standard read responses (0x01-0x04) also after the byte-count byte. Responses to 0x14/0x17 also carry a + * byte-count byte, but those codes are not implemented and their count byte is left in the payload. For + * an exception PDU the payload is the exception code byte (the read check must not see the masked + * function code, or an exception-of-read would classify as a read and return an empty span). Returns an + * empty span if the PDU is too short. + */ +inline std::span server_pdu_payload(std::span pdu) { + if (pdu.empty()) + return {}; + const size_t offset = (!is_function_code_exception(pdu[0]) && is_function_code_read(pdu[0])) ? 2 : 1; + return pdu.size() > offset ? pdu.subspan(offset) : std::span(); +} + inline uint8_t client_frame_data_offset(const uint8_t *, size_t) { return 2; } enum class SensorValueType : uint8_t { diff --git a/esphome/components/modbus_controller/modbus_controller.cpp b/esphome/components/modbus_controller/modbus_controller.cpp index 9246239ef95..84f8fc16b81 100644 --- a/esphome/components/modbus_controller/modbus_controller.cpp +++ b/esphome/components/modbus_controller/modbus_controller.cpp @@ -57,7 +57,7 @@ bool ModbusController::send_next_command_() { } // Queue incoming response -void ModbusController::on_modbus_data(const std::vector &data) { +void ModbusController::on_response(std::span request_pdu, std::span response_pdu) { if (this->command_queue_.empty()) { ESP_LOGW(TAG, "Received modbus data but command queue is empty"); return; @@ -78,8 +78,10 @@ void ModbusController::on_modbus_data(const std::vector &data) { this->online_callback_.call((int) current_command->function_code, current_command->register_address); } - // Move the commandItem to the response queue - current_command->payload = data; + // Move the commandItem to the response queue. The span points into the hub's receive buffer, so + // copy the payload into the command for deferred processing in loop(). + auto data = modbus::helpers::server_pdu_payload(response_pdu); + current_command->payload.assign(data.begin(), data.end()); this->incoming_queue_.push(std::move(current_command)); ESP_LOGV(TAG, "Modbus response queued"); this->command_queue_.pop_front(); @@ -93,8 +95,11 @@ void ModbusController::process_modbus_data_(const ModbusCommandItem *response) { response->on_data_func(response->register_type, response->register_address, response->payload); } -void ModbusController::on_modbus_error(uint8_t function_code, uint8_t exception_code) { - ESP_LOGE(TAG, "Modbus error function code: 0x%X exception: %d ", function_code, exception_code); +void ModbusController::on_error(std::span request_pdu, modbus::ModbusExceptionCode exception_code) { + // The request function code (request_pdu[0]) already carries what the log needs; the exception bit only + // ever appears on the response, so no masking is needed here. + const uint8_t function_code = request_pdu.empty() ? 0 : request_pdu[0]; + ESP_LOGE(TAG, "Modbus error function code: 0x%X exception: %d ", function_code, static_cast(exception_code)); if (this->command_queue_.empty()) { return; } diff --git a/esphome/components/modbus_controller/modbus_controller.h b/esphome/components/modbus_controller/modbus_controller.h index 484b59ede30..23e93057b83 100644 --- a/esphome/components/modbus_controller/modbus_controller.h +++ b/esphome/components/modbus_controller/modbus_controller.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -293,9 +294,9 @@ class ModbusController final : public PollingComponent, public modbus::ModbusCli /// Registers a sensor with the controller. Called by esphomes code generator void add_sensor_item(SensorItem *item) { sensorset_.insert(item); } /// called when a modbus response was parsed without errors - void on_modbus_data(const std::vector &data) override; + void on_response(std::span request_pdu, std::span response_pdu) override; /// called when a modbus error response was received - void on_modbus_error(uint8_t function_code, uint8_t exception_code) override; + void on_error(std::span request_pdu, modbus::ModbusExceptionCode exception_code) override; /// default delegate called by process_modbus_data when a response has retrieved from the incoming queue void on_register_data(ModbusRegisterType register_type, uint16_t start_address, const std::vector &data); /// default delegate called by process_modbus_data when a response for a write response has retrieved from the diff --git a/esphome/components/pzemac/pzemac.cpp b/esphome/components/pzemac/pzemac.cpp index d36e5d02504..0f22092f34a 100644 --- a/esphome/components/pzemac/pzemac.cpp +++ b/esphome/components/pzemac/pzemac.cpp @@ -9,7 +9,8 @@ static const uint8_t PZEM_CMD_READ_IN_REGISTERS = 0x04; static const uint8_t PZEM_CMD_RESET_ENERGY = 0x42; static const uint8_t PZEM_REGISTER_COUNT = 10; // 10x 16-bit registers -void PZEMAC::on_modbus_data(const std::vector &data) { +void PZEMAC::on_response(std::span request_pdu, std::span response_pdu) { + auto data = modbus::helpers::server_pdu_payload(response_pdu); if (data.size() < 20) { ESP_LOGW(TAG, "Invalid size for PZEM AC!"); return; diff --git a/esphome/components/pzemac/pzemac.h b/esphome/components/pzemac/pzemac.h index a3ad7e11673..171212d3ee6 100644 --- a/esphome/components/pzemac/pzemac.h +++ b/esphome/components/pzemac/pzemac.h @@ -5,7 +5,7 @@ #include "esphome/components/sensor/sensor.h" #include "esphome/components/modbus/modbus.h" -#include +#include namespace esphome::pzemac { @@ -22,7 +22,7 @@ class PZEMAC final : public PollingComponent, public modbus::ModbusClientDevice void update() override; - void on_modbus_data(const std::vector &data) override; + void on_response(std::span request_pdu, std::span response_pdu) override; void dump_config() override; diff --git a/esphome/components/pzemdc/pzemdc.cpp b/esphome/components/pzemdc/pzemdc.cpp index 6ded9b3a34c..31d1a7dac19 100644 --- a/esphome/components/pzemdc/pzemdc.cpp +++ b/esphome/components/pzemdc/pzemdc.cpp @@ -9,7 +9,8 @@ static const uint8_t PZEM_CMD_READ_IN_REGISTERS = 0x04; static const uint8_t PZEM_CMD_RESET_ENERGY = 0x42; static const uint8_t PZEM_REGISTER_COUNT = 10; // 10x 16-bit registers -void PZEMDC::on_modbus_data(const std::vector &data) { +void PZEMDC::on_response(std::span request_pdu, std::span response_pdu) { + auto data = modbus::helpers::server_pdu_payload(response_pdu); if (data.size() < 16) { ESP_LOGW(TAG, "Invalid size for PZEM DC!"); return; diff --git a/esphome/components/pzemdc/pzemdc.h b/esphome/components/pzemdc/pzemdc.h index 7d14a5ed4be..b7657608e64 100644 --- a/esphome/components/pzemdc/pzemdc.h +++ b/esphome/components/pzemdc/pzemdc.h @@ -5,7 +5,7 @@ #include "esphome/components/sensor/sensor.h" #include "esphome/components/modbus/modbus.h" -#include +#include namespace esphome::pzemdc { @@ -18,7 +18,7 @@ class PZEMDC final : public PollingComponent, public modbus::ModbusClientDevice void update() override; - void on_modbus_data(const std::vector &data) override; + void on_response(std::span request_pdu, std::span response_pdu) override; void dump_config() override; diff --git a/esphome/components/sdm_meter/sdm_meter.cpp b/esphome/components/sdm_meter/sdm_meter.cpp index a4fe6e7d356..989f22dd2a2 100644 --- a/esphome/components/sdm_meter/sdm_meter.cpp +++ b/esphome/components/sdm_meter/sdm_meter.cpp @@ -10,7 +10,8 @@ static const char *const TAG = "sdm_meter"; static const uint8_t MODBUS_CMD_READ_IN_REGISTERS = 0x04; static const uint8_t MODBUS_REGISTER_COUNT = 80; // 74 x 16-bit registers -void SDMMeter::on_modbus_data(const std::vector &data) { +void SDMMeter::on_response(std::span request_pdu, std::span response_pdu) { + auto data = modbus::helpers::server_pdu_payload(response_pdu); if (data.size() < MODBUS_REGISTER_COUNT * 2) { ESP_LOGW(TAG, "Invalid size for SDMMeter!"); return; diff --git a/esphome/components/sdm_meter/sdm_meter.h b/esphome/components/sdm_meter/sdm_meter.h index aa71fcaa47e..e09b74bbc0a 100644 --- a/esphome/components/sdm_meter/sdm_meter.h +++ b/esphome/components/sdm_meter/sdm_meter.h @@ -4,7 +4,7 @@ #include "esphome/components/sensor/sensor.h" #include "esphome/components/modbus/modbus.h" -#include +#include namespace esphome::sdm_meter { @@ -55,7 +55,7 @@ class SDMMeter final : public PollingComponent, public modbus::ModbusClientDevic void update() override; - void on_modbus_data(const std::vector &data) override; + void on_response(std::span request_pdu, std::span response_pdu) override; void dump_config() override; diff --git a/esphome/components/selec_meter/selec_meter.cpp b/esphome/components/selec_meter/selec_meter.cpp index f612b89934c..f5f0fdf40df 100644 --- a/esphome/components/selec_meter/selec_meter.cpp +++ b/esphome/components/selec_meter/selec_meter.cpp @@ -10,7 +10,8 @@ static const char *const TAG = "selec_meter"; static const uint8_t MODBUS_CMD_READ_IN_REGISTERS = 0x04; static const uint8_t MODBUS_REGISTER_COUNT = 34; // 34 x 16-bit registers -void SelecMeter::on_modbus_data(const std::vector &data) { +void SelecMeter::on_response(std::span request_pdu, std::span response_pdu) { + auto data = modbus::helpers::server_pdu_payload(response_pdu); if (data.size() < MODBUS_REGISTER_COUNT * 2) { ESP_LOGW(TAG, "Invalid size for SelecMeter!"); return; diff --git a/esphome/components/selec_meter/selec_meter.h b/esphome/components/selec_meter/selec_meter.h index c367d1d15d1..5ae1f9bf99a 100644 --- a/esphome/components/selec_meter/selec_meter.h +++ b/esphome/components/selec_meter/selec_meter.h @@ -4,7 +4,7 @@ #include "esphome/components/sensor/sensor.h" #include "esphome/components/modbus/modbus.h" -#include +#include namespace esphome::selec_meter { @@ -37,7 +37,7 @@ class SelecMeter final : public PollingComponent, public modbus::ModbusClientDev void update() override; - void on_modbus_data(const std::vector &data) override; + void on_response(std::span request_pdu, std::span response_pdu) override; void dump_config() override; }; diff --git a/tests/components/modbus/heap_probe_test.cpp b/tests/components/modbus/heap_probe_test.cpp index af43c6e5e38..2c7d9747bd2 100644 --- a/tests/components/modbus/heap_probe_test.cpp +++ b/tests/components/modbus/heap_probe_test.cpp @@ -39,6 +39,50 @@ namespace esphome::modbus::testing { namespace { +// A UART the test can inject received bytes into; sent bytes are discarded. +class InjectableUART : public uart::UARTComponent { + public: + void write_array(const uint8_t *data, size_t len) override {} + bool peek_byte(uint8_t *data) override { + if (this->rx_.empty()) + return false; + *data = this->rx_.front(); + return true; + } + bool read_array(uint8_t *data, size_t len) override { + if (len > this->rx_.size()) + return false; + memcpy(data, this->rx_.data(), len); + this->rx_.erase(this->rx_.begin(), this->rx_.begin() + len); + return true; + } + size_t available() override { return this->rx_.size(); } + uart::UARTFlushResult flush() override { return uart::UARTFlushResult::UART_FLUSH_RESULT_ASSUMED_SUCCESS; } + void check_logger_conflict() override {} + + void inject_frame(uint8_t address, std::span pdu) { + // Wire frame: address + PDU + CRC16(low, high) + size_t start = this->rx_.size(); + this->rx_.push_back(address); + this->rx_.insert(this->rx_.end(), pdu.begin(), pdu.end()); + uint16_t crc = crc16(this->rx_.data() + start, this->rx_.size() - start); + this->rx_.push_back(crc & 0xFF); + this->rx_.push_back(crc >> 8); + } + + private: + std::vector rx_; +}; + +class NullDevice : public ModbusClientDevice { + public: + using ModbusClientDevice::ModbusClientDevice; + void on_response(std::span request_pdu, std::span response_pdu) override { + this->responses++; + } + int responses{0}; +}; + struct Sample { size_t count; size_t bytes; @@ -93,14 +137,61 @@ TEST(HeapProbe, QueueingTypicalCommandsIsAllocationFree) { EXPECT_EQ(total, 0u); } +// End to end: bytes injected at the UART travel through receive, frame parsing, response matching and +// device dispatch. The first response may grow the hub's rx buffer once; after that warm-up, handling a +// response performs zero heap allocations all the way to the device callback. +TEST(HeapProbe, ResponseHandlingIsAllocationFreeAfterWarmup) { + InjectableUART uart; + uart.set_baud_rate(115200); // tx timing math divides by the baud rate + ModbusClientHub hub; + hub.set_uart_parent(&uart); + hub.setup(); // computes frame timing from the baud rate + NullDevice device(&hub, 0x02); + + StaticVector req; + const uint8_t read_pdu[] = {0x03, 0x01, 0x00, 0x00, 0x02}; + req.assign(read_pdu, read_pdu + sizeof(read_pdu)); + + // Largest possible read response first, so the rx buffer warm-up covers every later size. + uint8_t large_resp[252] = {0x03, 250}; + const uint8_t small_resp[] = {0x03, 0x04, 0x00, 0x2A, 0x01, 0x00}; + + auto round_trip = [&](std::span response_pdu) { + device.send_pdu(req); + hub.loop(); // transmit; the tx queue is empty during the measured receive below + uart.inject_frame(0x02, response_pdu); + return sample([&] { hub.loop(); }); // receive + parse + match + dispatch + }; + + Sample warmup = round_trip(std::span(large_resp, sizeof(large_resp))); + Sample steady_large = round_trip(std::span(large_resp, sizeof(large_resp))); + Sample steady_small = round_trip(small_resp); + + printf("HEAPPROBE warmup count=%zu bytes=%zu\n", warmup.count, warmup.bytes); + printf("HEAPPROBE steady_large count=%zu bytes=%zu\n", steady_large.count, steady_large.bytes); + printf("HEAPPROBE steady_small count=%zu bytes=%zu\n", steady_small.count, steady_small.bytes); + + EXPECT_EQ(device.responses, 3); + EXPECT_LE(warmup.count, 1u); // at most the one-time rx buffer growth + EXPECT_EQ(steady_large.count, 0u); + EXPECT_EQ(steady_small.count, 0u); +} + } // namespace esphome::modbus::testing #else // !HEAP_PROBE_HAS_ASAN +// Stub every ASan-gated test name, so the suite's test list is identical in every build configuration. namespace esphome::modbus::testing { TEST(HeapProbe, TypicalFrameConstructionIsAllocationFree) { GTEST_SKIP() << "allocation counting requires an AddressSanitizer build"; } +TEST(HeapProbe, QueueingTypicalCommandsIsAllocationFree) { + GTEST_SKIP() << "allocation counting requires an AddressSanitizer build"; +} +TEST(HeapProbe, ResponseHandlingIsAllocationFreeAfterWarmup) { + GTEST_SKIP() << "allocation counting requires an AddressSanitizer build"; +} } // namespace esphome::modbus::testing #endif // HEAP_PROBE_HAS_ASAN diff --git a/tests/components/modbus/modbus_client_hub_test.cpp b/tests/components/modbus/modbus_client_hub_test.cpp index d04c4fe10c8..4447ca93442 100644 --- a/tests/components/modbus/modbus_client_hub_test.cpp +++ b/tests/components/modbus/modbus_client_hub_test.cpp @@ -27,8 +27,8 @@ class NoResponseProbeHub : public ModbusClientHub { this->tx_buffer_.pop_front(); } // Drives the real unexpected-frame branch in process_modbus_server_frame(). - void receive_frame_for_test(uint8_t address, uint8_t function_code, const uint8_t *data, uint16_t len) { - this->process_modbus_server_frame(address, function_code, data, len); + void receive_frame_for_test(uint8_t address, std::span pdu) { + this->process_modbus_server_frame(address, pdu); } void timeout_waiting() { if (this->waiting_for_response_.has_value()) @@ -37,11 +37,11 @@ class NoResponseProbeHub : public ModbusClientHub { } }; -// A device with a scripted answer to on_modbus_no_response(). +// A device with a scripted answer to on_no_response(). class RetryingDevice : public ModbusClientDevice { public: RetryingDevice(ModbusClientHub *hub, uint8_t address, bool retry) : ModbusClientDevice(hub, address), retry_(retry) {} - bool on_modbus_no_response() override { + bool on_no_response() override { this->no_response_count_++; return this->retry_; } @@ -55,7 +55,7 @@ class RetryingDevice : public ModbusClientDevice { class ClearingRetryDevice : public ModbusClientDevice { public: ClearingRetryDevice(ModbusClientHub *hub, uint8_t address) : ModbusClientDevice(hub, address) {} - bool on_modbus_no_response() override { + bool on_no_response() override { this->no_response_count_++; this->clear_tx_queue_for_device(); // detaches this device from the waiting slot mid-callback return true; // and still requests a retry @@ -143,8 +143,8 @@ TEST(ModbusClientHubNoResponse, RetryBehindInterruptedShell) { hub.force_send_front(); // A frame from the wrong address (0x07, expected 0x02) hits the unexpected-frame branch. - const uint8_t stray_payload[] = {0x04, 0x00, 0x2A, 0x01, 0x00}; - hub.receive_frame_for_test(0x07, 0x03, stray_payload, sizeof(stray_payload)); + const uint8_t stray_pdu[] = {0x03, 0x04, 0x00, 0x2A, 0x01, 0x00}; + hub.receive_frame_for_test(0x07, stray_pdu); EXPECT_EQ(device.no_response_count_, 1); ASSERT_EQ(hub.queued_frames(), 1u); // exactly one requeue... @@ -175,4 +175,37 @@ TEST(ModbusClientHubNoResponse, MidCallbackClearCancelsRetry) { EXPECT_FALSE(hub.waiting()); } +namespace { +// Overrides only the DEPRECATED on_modbus_* names: the new-name default implementations must forward, so +// external devices written against the old names keep working through the deprecation window. +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" +class LegacyNameDevice : public ModbusClientDevice { + public: + LegacyNameDevice(ModbusClientHub *hub, uint8_t address) : ModbusClientDevice(hub, address) {} + void on_modbus_not_sent() override { this->legacy_not_sent_++; } + bool on_modbus_no_response() override { + this->legacy_no_response_++; + return false; + } + int legacy_not_sent_{0}; + int legacy_no_response_{0}; +}; +#pragma GCC diagnostic pop +} // namespace + +TEST(ModbusClientHubCompat, LegacyCallbackNamesStillForward) { + NoResponseProbeHub hub; + LegacyNameDevice device(&hub, 0x02); + + const uint8_t read[] = {0x03, 0x00, 0x10, 0x00, 0x01}; + device.send_pdu(read); + hub.force_send_front(); + hub.timeout_waiting(); // no reply -> on_no_response -> forwards to on_modbus_no_response + EXPECT_EQ(device.legacy_no_response_, 1); + + device.send_pdu(std::span()); // empty PDU refused -> on_not_sent -> forwards + EXPECT_EQ(device.legacy_not_sent_, 1); +} + } // namespace esphome::modbus::testing diff --git a/tests/components/modbus/modbus_helpers_test.cpp b/tests/components/modbus/modbus_helpers_test.cpp index 1c57a81e6f7..30ba12b16bc 100644 --- a/tests/components/modbus/modbus_helpers_test.cpp +++ b/tests/components/modbus/modbus_helpers_test.cpp @@ -229,4 +229,21 @@ TEST(ModbusHelpersTest, RegistersToNumberRejectsTruncatedMultiRegisterValue) { EXPECT_FALSE(registers_to_number(registers, 1, SensorValueType::U_DWORD).has_value()); } +// server_pdu_payload() must never classify an exception PDU as a read: [fc|0x80, code] is 2 bytes, and a +// read-offset of 2 would return an empty span, losing the exception code. The payload of an exception PDU +// is the exception code byte, for reads and writes alike. +TEST(ModbusServerPduPayload, ExceptionOfReadYieldsExceptionCode) { + const uint8_t pdu[] = {0x83, 0x02}; // exception response to READ_HOLDING_REGISTERS + auto payload = server_pdu_payload(pdu); + ASSERT_EQ(payload.size(), 1u); + EXPECT_EQ(payload[0], 0x02); +} + +TEST(ModbusServerPduPayload, ExceptionOfWriteYieldsExceptionCode) { + const uint8_t pdu[] = {0x86, 0x03}; // exception response to WRITE_SINGLE_REGISTER + auto payload = server_pdu_payload(pdu); + ASSERT_EQ(payload.size(), 1u); + EXPECT_EQ(payload[0], 0x03); +} + } // namespace esphome::modbus::helpers