diff --git a/esphome/components/modbus/modbus.cpp b/esphome/components/modbus/modbus.cpp index eefab7967f..527d57fcd7 100644 --- a/esphome/components/modbus/modbus.cpp +++ b/esphome/components/modbus/modbus.cpp @@ -51,7 +51,7 @@ void ModbusClientHub::loop() { // If we're past the send_wait_time timeout and response buffer doesn't have the start of the expected response if (this->waiting_for_response_.has_value()) { ModbusDeviceCommand &wfr = this->waiting_for_response_.value(); - uint8_t expected_address = wfr.frame.data.get()[0]; + uint8_t expected_address = wfr.frame.data.data()[0]; if (this->last_receive_check_ - this->last_send_ > this->last_send_tx_offset_ + this->send_wait_time_ && (this->rx_buffer_.empty() || this->rx_buffer_[0] != expected_address)) { ESP_LOGW(TAG, "Stop waiting for response from %" PRIu8 " %" PRIu32 "ms after last send", expected_address, @@ -270,8 +270,8 @@ void ModbusClientHub::process_modbus_server_frame(uint8_t address, uint8_t funct // Check if the response matches the expected address and function code ModbusDeviceCommand &wfr = this->waiting_for_response_.value(); - uint8_t expected_address = wfr.frame.data.get()[0]; - uint8_t expected_function_code = wfr.frame.data.get()[1]; + uint8_t expected_address = wfr.frame.data.data()[0]; + uint8_t expected_function_code = wfr.frame.data.data()[1]; if (expected_address != address || expected_function_code != (function_code & FUNCTION_CODE_MASK)) { ESP_LOGW(TAG, "Received incorrect frame address %" PRIu8 " <> %" PRIu8 " or function code 0x%X <> 0x%X, %" PRIu32 @@ -458,7 +458,7 @@ bool Modbus::send_frame_(const ModbusFrame &frame) { ESP_LOGE(TAG, "Attempted to send while transmission blocked"); return false; } - if (frame.size > MAX_FRAME_SIZE) { + if (frame.size() > MAX_FRAME_SIZE) { ESP_LOGE(TAG, "Attempted to send frame larger than max frame size of %" PRIu16 " bytes", MAX_FRAME_SIZE); return false; } @@ -470,13 +470,13 @@ bool Modbus::send_frame_(const ModbusFrame &frame) { if (this->flow_control_pin_ != nullptr) { this->flow_control_pin_->digital_write(true); - this->write_array(frame.data.get(), frame.size); + this->write_array(frame.data.data(), frame.size()); this->flush(); this->flow_control_pin_->digital_write(false); this->last_send_tx_offset_ = 0; } else { - this->write_array(frame.data.get(), frame.size); - this->last_send_tx_offset_ = frame.size * MODBUS_BITS_PER_CHAR * MS_PER_SEC / this->parent_->get_baud_rate() + 1; + this->write_array(frame.data.data(), frame.size()); + this->last_send_tx_offset_ = frame.size() * MODBUS_BITS_PER_CHAR * MS_PER_SEC / this->parent_->get_baud_rate() + 1; } uint32_t now = millis(); @@ -484,7 +484,7 @@ bool Modbus::send_frame_(const ModbusFrame &frame) { char hex_buf[format_hex_pretty_size(MODBUS_MAX_LOG_BYTES)]; #endif ESP_LOGV(TAG, "Write: %s %" PRIu32 "ms after last send, %" PRIu32 "ms after last receive", - format_hex_pretty_to(hex_buf, frame.data.get(), frame.size), now - this->last_send_, + format_hex_pretty_to(hex_buf, frame.data.data(), frame.size()), now - this->last_send_, now - this->last_modbus_byte_); this->last_send_ = now; return true; @@ -590,12 +590,13 @@ void ModbusClientHub::queue_raw_(uint8_t address, const uint8_t *pdu, uint16_t p void ModbusClientHub::clear_tx_queue_for_address(uint8_t address, bool clear_sent) { // Remove any pending commands for this address from the tx buffer auto &tx_buffer = this->tx_buffer_; - tx_buffer.erase(std::remove_if(tx_buffer.begin(), tx_buffer.end(), - [address](const ModbusDeviceCommand &cmd) { return cmd.frame.data[0] == address; }), - tx_buffer.end()); + tx_buffer.erase( + std::remove_if(tx_buffer.begin(), tx_buffer.end(), + [address](const ModbusDeviceCommand &cmd) { return cmd.frame.data.data()[0] == address; }), + tx_buffer.end()); if (clear_sent && this->waiting_for_response_.has_value() && this->waiting_for_response_.value().device) { - if (this->waiting_for_response_.value().frame.data[0] == address) { + if (this->waiting_for_response_.value().frame.data.data()[0] == address) { ESP_LOGV(TAG, "Clearing waiting for response for address %" PRIu8, address); // Invalidate the waiting device so it won't process a response. this->waiting_for_response_.value().device = nullptr; diff --git a/esphome/components/modbus/modbus.h b/esphome/components/modbus/modbus.h index d995c441ad..e48c8c298a 100644 --- a/esphome/components/modbus/modbus.h +++ b/esphome/components/modbus/modbus.h @@ -18,19 +18,27 @@ namespace esphome::modbus { static constexpr uint16_t MODBUS_TX_BUFFER_SIZE = 15; static constexpr uint16_t MODBUS_TX_MAX_DELAY_MS = 5; -struct ModbusFrame { - // Frame with exact-size allocation to avoid std::vector overhead - std::unique_ptr data; - uint16_t size; // Modbus RTU max is 256 bytes +// Typical frames -- reads and single-register/coil writes -- are exactly 8 bytes +// (address + 5-byte PDU + 2-byte CRC) and fit inline with no heap allocation. +static constexpr uint16_t MODBUS_FRAME_INLINE_SIZE = 8; - ModbusFrame(uint8_t address, const uint8_t *pdu, uint16_t pdu_len) - : data(std::make_unique(pdu_len + 3)), size(pdu_len + 3) { - data[0] = address; - memcpy(data.get() + 1, pdu, pdu_len); - auto crc = crc16(data.get(), pdu_len + 1); - data[pdu_len + 1] = crc >> 0; - data[pdu_len + 2] = crc >> 8; +struct ModbusFrame { + // Frame held in a small-buffer-optimized buffer. Typical frames fit inline; only larger + // multi-register or custom frames spill to a single heap allocation. This keeps the common, + // high-frequency tx traffic off the heap entirely, avoiding per-frame alloc/free churn. + // The buffer tracks its own length, so no separate size field is needed. + SmallInlineBuffer data; // Modbus RTU max is 256 bytes + + ModbusFrame(uint8_t address, const uint8_t *pdu, uint16_t pdu_len) { + uint8_t *buf = this->data.init(pdu_len + 3); + buf[0] = address; + memcpy(buf + 1, pdu, pdu_len); + auto crc = crc16(buf, pdu_len + 1); + buf[pdu_len + 1] = crc >> 0; + buf[pdu_len + 2] = crc >> 8; } + + uint16_t size() const { return static_cast(this->data.size()); } }; class Modbus : public uart::UARTDevice, public Component { diff --git a/esphome/core/helpers.h b/esphome/core/helpers.h index f39b5aa4d0..e862d015da 100644 --- a/esphome/core/helpers.h +++ b/esphome/core/helpers.h @@ -184,8 +184,10 @@ template class SmallInlineBuffer { SmallInlineBuffer(const SmallInlineBuffer &) = delete; SmallInlineBuffer &operator=(const SmallInlineBuffer &) = delete; - /// Set buffer contents, allocating heap if needed - void set(const uint8_t *src, size_t size) { + /// Resize to `size` bytes of (uninitialized) storage and return a writable pointer to fill. + /// Allocates heap only when `size` exceeds the inline capacity. Use this when the contents are + /// built in place (e.g. assembling a frame and appending a checksum) to avoid a staging copy. + uint8_t *init(size_t size) { // Free existing heap allocation if switching from heap to inline or different heap size if (!this->is_inline_() && (size <= InlineSize || size != this->len_)) { delete[] this->heap_; @@ -196,9 +198,12 @@ template class SmallInlineBuffer { this->heap_ = new uint8_t[size]; // NOLINT(cppcoreguidelines-owning-memory) } this->len_ = size; - memcpy(this->data(), src, size); + return this->data(); } + /// Set buffer contents, allocating heap if needed + void set(const uint8_t *src, size_t size) { memcpy(this->init(size), src, size); } + uint8_t *data() { return this->is_inline_() ? this->inline_ : this->heap_; } const uint8_t *data() const { return this->is_inline_() ? this->inline_ : this->heap_; } size_t size() const { return this->len_; } diff --git a/tests/components/modbus/heap_probe_test.cpp b/tests/components/modbus/heap_probe_test.cpp new file mode 100644 index 0000000000..af43c6e5e3 --- /dev/null +++ b/tests/components/modbus/heap_probe_test.cpp @@ -0,0 +1,106 @@ +#include + +#include +#include +#include +#include +#include +#include + +#include "esphome/components/modbus/modbus.h" + +// The allocation counters rely on AddressSanitizer's malloc hooks. The cpp_unit_test harness always +// builds with ASan, so this is exercised in CI; the fallback only applies to out-of-harness builds. +#ifndef __has_feature +#define __has_feature(x) 0 +#endif +#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer) +#define HEAP_PROBE_HAS_ASAN +#endif + +#ifdef HEAP_PROBE_HAS_ASAN + +// Allocation counters fed by ASan's malloc hooks; sampled tightly around the calls under test. +static std::atomic g_alloc_count{0}; +static std::atomic g_alloc_bytes{0}; + +static void malloc_hook(const volatile void *, size_t size) { + g_alloc_count++; + g_alloc_bytes += size; +} +static void free_hook(const volatile void *) {} + +extern "C" int __sanitizer_install_malloc_and_free_hooks(void (*malloc_hook)(const volatile void *, size_t), + void (*free_hook)(const volatile void *)); + +[[maybe_unused]] static const int g_hooks_installed = __sanitizer_install_malloc_and_free_hooks(malloc_hook, free_hook); + +namespace esphome::modbus::testing { + +namespace { + +struct Sample { + size_t count; + size_t bytes; +}; + +template Sample sample(F &&f) { + size_t c0 = g_alloc_count.load(), b0 = g_alloc_bytes.load(); + f(); + return {g_alloc_count.load() - c0, g_alloc_bytes.load() - b0}; +} + +} // namespace + +// Typical frames (reads and single-register/coil writes are exactly address + 5-byte PDU + CRC = 8 +// bytes) fit the SmallInlineBuffer and are built with zero heap allocations; only larger frames spill +// to a single allocation. +TEST(HeapProbe, TypicalFrameConstructionIsAllocationFree) { + const uint8_t read_pdu[] = {0x03, 0x01, 0x00, 0x00, 0x02}; // 5 bytes -> 8-byte frame, inline + Sample typical = sample([&] { + ModbusFrame frame(0x02, read_pdu, sizeof(read_pdu)); + (void) frame; + }); + printf("HEAPPROBE frame_typical count=%zu bytes=%zu\n", typical.count, typical.bytes); + EXPECT_EQ(typical.count, 0u); + + uint8_t large_pdu[250] = {0x10}; // multi-register write -> 253-byte frame, spills once + Sample large = sample([&] { + ModbusFrame frame(0x02, large_pdu, sizeof(large_pdu)); + (void) frame; + }); + printf("HEAPPROBE frame_large count=%zu bytes=%zu\n", large.count, large.bytes); + EXPECT_EQ(large.count, 1u); +} + +// Queueing typical commands is fully allocation-free: the frame fits the inline buffer and the tx +// deque's first block is already allocated when the hub is constructed. (A queue deeper than one +// deque block - roughly a dozen commands - would allocate further blocks.) +TEST(HeapProbe, QueueingTypicalCommandsIsAllocationFree) { + ModbusClientHub hub; + ModbusClientDevice device(&hub, 0x02); + + StaticVector req; + const uint8_t read_pdu[] = {0x03, 0x01, 0x00, 0x00, 0x02}; + req.assign(read_pdu, read_pdu + sizeof(read_pdu)); + + constexpr int n = 12; + size_t total = 0; + for (int i = 0; i != n; i++) { + total += sample([&] { device.send_pdu(req); }).count; + } + printf("HEAPPROBE queue_%d_typical_commands total_allocs=%zu\n", n, total); + EXPECT_EQ(total, 0u); +} + +} // namespace esphome::modbus::testing + +#else // !HEAP_PROBE_HAS_ASAN + +namespace esphome::modbus::testing { +TEST(HeapProbe, TypicalFrameConstructionIsAllocationFree) { + GTEST_SKIP() << "allocation counting requires an AddressSanitizer build"; +} +} // namespace esphome::modbus::testing + +#endif // HEAP_PROBE_HAS_ASAN