From 32151bfc43121d9f7e8de4269878ab02a190f62e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 24 Mar 2026 13:11:38 -1000 Subject: [PATCH] Address review feedback - Remove dead DEFAULT_SLOT_COUNT constant from host header - Fix misleading BSS comments (Logger is heap-allocated via new_Pvariable) - Add comment explaining Zephyr BUF_WORD_COUNT formula --- esphome/components/logger/__init__.py | 2 +- esphome/components/logger/logger.h | 2 +- esphome/components/logger/task_log_buffer_esp32.cpp | 2 +- esphome/components/logger/task_log_buffer_esp32.h | 2 +- esphome/components/logger/task_log_buffer_host.h | 5 +---- esphome/components/logger/task_log_buffer_libretiny.cpp | 2 +- esphome/components/logger/task_log_buffer_libretiny.h | 2 +- esphome/components/logger/task_log_buffer_zephyr.cpp | 2 +- esphome/components/logger/task_log_buffer_zephyr.h | 3 ++- 9 files changed, 10 insertions(+), 12 deletions(-) diff --git a/esphome/components/logger/__init__.py b/esphome/components/logger/__init__.py index bb271ce38f..4144543b89 100644 --- a/esphome/components/logger/__init__.py +++ b/esphome/components/logger/__init__.py @@ -332,7 +332,7 @@ async def to_code(config: ConfigType) -> None: tx_buffer_size = config[CONF_TX_BUFFER_SIZE] cg.add_define("ESPHOME_LOGGER_TX_BUFFER_SIZE", tx_buffer_size) # Determine task log buffer size. The buffer is a direct member of Logger - # (lives in BSS), so it's available from program start with no heap allocation. + # (no separate heap allocation). task_log_buffer_size = 0 if CORE.is_esp32 or CORE.is_libretiny or CORE.is_nrf52: task_log_buffer_size = config[CONF_TASK_LOG_BUFFER_SIZE] diff --git a/esphome/components/logger/logger.h b/esphome/components/logger/logger.h index 6ca8c5e6fd..784cbea67e 100644 --- a/esphome/components/logger/logger.h +++ b/esphome/components/logger/logger.h @@ -369,7 +369,7 @@ class Logger final : public Component { // Large buffers placed last to keep frequently-accessed member offsets small char tx_buffer_[ESPHOME_LOGGER_TX_BUFFER_SIZE + 1]; // +1 for null terminator #ifdef USE_ESPHOME_TASK_LOG_BUFFER - logger::TaskLogBuffer log_buffer_; // Task log buffer storage in BSS (no heap allocation) + logger::TaskLogBuffer log_buffer_; // Embedded in Logger (no separate heap allocation) #endif // --- get_thread_name_ overloads (per-platform) --- diff --git a/esphome/components/logger/task_log_buffer_esp32.cpp b/esphome/components/logger/task_log_buffer_esp32.cpp index 0747de659a..cb97f5504f 100644 --- a/esphome/components/logger/task_log_buffer_esp32.cpp +++ b/esphome/components/logger/task_log_buffer_esp32.cpp @@ -9,7 +9,7 @@ namespace esphome::logger { TaskLogBuffer::TaskLogBuffer() { // Create a static ring buffer with RINGBUF_TYPE_NOSPLIT for message integrity - // Storage is a member array (lives in BSS), no heap allocation needed + // Storage is a member array (embedded in Logger), no heap allocation needed this->ring_buffer_ = xRingbufferCreateStatic(sizeof(this->storage_), RINGBUF_TYPE_NOSPLIT, this->storage_, &this->structure_); } diff --git a/esphome/components/logger/task_log_buffer_esp32.h b/esphome/components/logger/task_log_buffer_esp32.h index d4ed616a66..e819766795 100644 --- a/esphome/components/logger/task_log_buffer_esp32.h +++ b/esphome/components/logger/task_log_buffer_esp32.h @@ -70,7 +70,7 @@ class TaskLogBuffer { private: RingbufHandle_t ring_buffer_{nullptr}; // FreeRTOS ring buffer handle StaticRingbuffer_t structure_; // Static structure for the ring buffer - uint8_t storage_[ESPHOME_TASK_LOG_BUFFER_SIZE]; // Buffer storage in BSS + uint8_t storage_[ESPHOME_TASK_LOG_BUFFER_SIZE]; // Embedded in Logger (no separate heap allocation) // Atomic counter for message tracking (only differences matter) std::atomic message_counter_{0}; // Incremented when messages are committed diff --git a/esphome/components/logger/task_log_buffer_host.h b/esphome/components/logger/task_log_buffer_host.h index 667080649f..25e9c4da58 100644 --- a/esphome/components/logger/task_log_buffer_host.h +++ b/esphome/components/logger/task_log_buffer_host.h @@ -49,9 +49,6 @@ namespace esphome::logger { */ class TaskLogBuffer { public: - // Default number of message slots - host has plenty of memory - static constexpr size_t DEFAULT_SLOT_COUNT = 64; - // Structure for a log message (fixed size for lock-free operation) struct LogMessage { // Size constants @@ -103,7 +100,7 @@ class TaskLogBuffer { // Commit a slot after writing (thread-safe) void commit_write_slot_(int slot_index); - LogMessage slots_[ESPHOME_TASK_LOG_BUFFER_SIZE]; // Pre-allocated message slots in BSS + LogMessage slots_[ESPHOME_TASK_LOG_BUFFER_SIZE]; // Embedded in Logger (no separate heap allocation) // Lock-free indices using atomics // - reserve_index_: Next slot to reserve (producers CAS this to claim slots) diff --git a/esphome/components/logger/task_log_buffer_libretiny.cpp b/esphome/components/logger/task_log_buffer_libretiny.cpp index 42e9e5e398..b6d6b22ab5 100644 --- a/esphome/components/logger/task_log_buffer_libretiny.cpp +++ b/esphome/components/logger/task_log_buffer_libretiny.cpp @@ -9,7 +9,7 @@ namespace esphome::logger { TaskLogBuffer::TaskLogBuffer() { // Create mutex for thread-safe access - // Storage is a member array (lives in BSS), no heap allocation needed + // Storage is a member array (embedded in Logger), no heap allocation needed this->mutex_ = xSemaphoreCreateMutex(); } diff --git a/esphome/components/logger/task_log_buffer_libretiny.h b/esphome/components/logger/task_log_buffer_libretiny.h index bfc02cb593..b42894502a 100644 --- a/esphome/components/logger/task_log_buffer_libretiny.h +++ b/esphome/components/logger/task_log_buffer_libretiny.h @@ -86,7 +86,7 @@ class TaskLogBuffer { // Calculate available contiguous space at write position size_t available_contiguous_space() const; - uint8_t storage_[ESPHOME_TASK_LOG_BUFFER_SIZE]; // Buffer storage in BSS + uint8_t storage_[ESPHOME_TASK_LOG_BUFFER_SIZE]; // Embedded in Logger (no separate heap allocation) size_t head_{0}; // Write position size_t tail_{0}; // Read position diff --git a/esphome/components/logger/task_log_buffer_zephyr.cpp b/esphome/components/logger/task_log_buffer_zephyr.cpp index 10898065b2..a994925a54 100644 --- a/esphome/components/logger/task_log_buffer_zephyr.cpp +++ b/esphome/components/logger/task_log_buffer_zephyr.cpp @@ -18,7 +18,7 @@ static inline uint32_t get_wlen(const mpsc_pbuf_generic *item) { } TaskLogBuffer::TaskLogBuffer() { - // Storage is a member array (lives in BSS), no heap allocation needed + // Storage is a member array (embedded in Logger), no heap allocation needed this->mpsc_config_.buf = this->buf_storage_; this->mpsc_config_.size = BUF_WORD_COUNT; this->mpsc_config_.flags = MPSC_PBUF_MODE_OVERWRITE; diff --git a/esphome/components/logger/task_log_buffer_zephyr.h b/esphome/components/logger/task_log_buffer_zephyr.h index 46c1139157..7dfe7ac1db 100644 --- a/esphome/components/logger/task_log_buffer_zephyr.h +++ b/esphome/components/logger/task_log_buffer_zephyr.h @@ -53,8 +53,9 @@ class TaskLogBuffer { const char *format, va_list args); protected: + // Round up byte size to 32-bit word count for mpsc_pbuf alignment requirement static constexpr size_t BUF_WORD_COUNT = (ESPHOME_TASK_LOG_BUFFER_SIZE + 3) / sizeof(uint32_t); - uint32_t buf_storage_[BUF_WORD_COUNT]; // Buffer storage in BSS + uint32_t buf_storage_[BUF_WORD_COUNT]; // Embedded in Logger (no separate heap allocation) mpsc_pbuf_buffer_config mpsc_config_{}; mpsc_pbuf_buffer log_buffer_{}; const mpsc_pbuf_generic *current_token_{};