From 962b65a46d9f279ca9187000d7132d97beeafd7a Mon Sep 17 00:00:00 2001 From: Jesse Hills <3060199+jesserockz@users.noreply.github.com> Date: Mon, 15 Jun 2026 20:06:19 +1200 Subject: [PATCH] [pixoo] Fix clang-tidy warnings and address review feedback Move build_packet/pad_unused to file-local helpers, round brightness with lroundf, free the draw buffer on frame-buffer alloc failure, and pad the final DMA chunk when exactly the static packet length remains. --- esphome/components/pixoo/pixoo.cpp | 31 ++++++++++++++++++++++-------- esphome/components/pixoo/pixoo.h | 17 ++-------------- 2 files changed, 25 insertions(+), 23 deletions(-) diff --git a/esphome/components/pixoo/pixoo.cpp b/esphome/components/pixoo/pixoo.cpp index 768d2de056..81962ced03 100644 --- a/esphome/components/pixoo/pixoo.cpp +++ b/esphome/components/pixoo/pixoo.cpp @@ -2,6 +2,7 @@ #include "esphome/core/log.h" +#include #include #include @@ -9,9 +10,19 @@ namespace esphome::pixoo { static const char *const TAG = "pixoo"; -float Pixoo::get_setup_priority() const { return setup_priority::PROCESSOR; } +// Divoom LED-board packet protocol. +static constexpr uint8_t PACKET_HEAD = 0xAA; +static constexpr uint8_t PACKET_TAIL = 0xBB; +static constexpr uint8_t CMD_DATA = 0x00; +static constexpr uint8_t CMD_LIGHT = 0x01; +static constexpr uint8_t CMD_UNUSED = 0x21; +static constexpr uint8_t CMD_SET_RGB_IOUT = 0x22; +static constexpr size_t PACKET_HEADER_LEN = 4; // head + len(2) + cmd +static constexpr size_t PACKET_STATIC_LEN = 5; // header + tail +static constexpr uint8_t DEFAULT_IOUT = 75; // per-channel LED current / white balance default -size_t Pixoo::build_packet_(uint8_t *buf, uint8_t cmd, const uint8_t *data, uint16_t len) { +// Pack a `0xAA len cmd data 0xBB` packet into buf; returns the packet length. +static inline size_t build_packet(uint8_t *buf, uint8_t cmd, const uint8_t *data, uint16_t len) { buf[0] = PACKET_HEAD; buf[1] = static_cast(len & 0xFF); buf[2] = static_cast((len >> 8) & 0xFF); @@ -22,7 +33,8 @@ size_t Pixoo::build_packet_(uint8_t *buf, uint8_t cmd, const uint8_t *data, uint return len + PACKET_STATIC_LEN; } -void Pixoo::pad_unused_(uint8_t *buf, size_t total) { +// Fill `total` bytes at buf with a single UNUSED padding packet. +static inline void pad_unused(uint8_t *buf, size_t total) { const uint16_t len = static_cast(total - PACKET_STATIC_LEN); buf[0] = PACKET_HEAD; buf[1] = static_cast(len & 0xFF); @@ -31,6 +43,8 @@ void Pixoo::pad_unused_(uint8_t *buf, size_t total) { buf[total - 1] = PACKET_TAIL; } +float Pixoo::get_setup_priority() const { return setup_priority::PROCESSOR; } + void Pixoo::setup() { const uint32_t num_pixels = static_cast(this->model_) * this->model_; this->data_size_ = num_pixels * 3; @@ -47,6 +61,7 @@ void Pixoo::setup() { RAMAllocator allocator(RAMAllocator::ALLOC_INTERNAL); this->frame_buffer_ = allocator.allocate(this->frame_size_); if (this->frame_buffer_ == nullptr) { + this->buffer_.free(); this->mark_failed(LOG_STR("Failed to allocate frame buffer")); return; } @@ -57,7 +72,7 @@ void Pixoo::setup() { this->frame_buffer_[2] = static_cast((this->data_size_ >> 8) & 0xFF); this->frame_buffer_[3] = CMD_DATA; this->frame_buffer_[PACKET_HEADER_LEN + this->data_size_] = PACKET_TAIL; - this->pad_unused_(this->frame_buffer_ + this->data_size_ + PACKET_STATIC_LEN, DMA_CHUNK); + pad_unused(this->frame_buffer_ + this->data_size_ + PACKET_STATIC_LEN, DMA_CHUNK); this->spi_setup(); @@ -75,16 +90,16 @@ void Pixoo::setup() { void Pixoo::send_command_(uint8_t cmd, const uint8_t *data, uint16_t len) { std::memset(this->cmd_buffer_, 0, DMA_CHUNK); - const size_t used = build_packet_(this->cmd_buffer_, cmd, data, len); - if (DMA_CHUNK - used > PACKET_STATIC_LEN) - pad_unused_(this->cmd_buffer_ + used, DMA_CHUNK - used); + const size_t used = build_packet(this->cmd_buffer_, cmd, data, len); + if (DMA_CHUNK - used >= PACKET_STATIC_LEN) + pad_unused(this->cmd_buffer_ + used, DMA_CHUNK - used); this->enable(); this->write_array(this->cmd_buffer_, DMA_CHUNK); this->disable(); } void Pixoo::set_panel_brightness(float brightness) { - const uint8_t pct = static_cast(clamp(brightness, 0.0f, 1.0f) * 100.0f + 0.5f); + const uint8_t pct = static_cast(lroundf(clamp(brightness, 0.0f, 1.0f) * 100.0f)); this->send_command_(CMD_LIGHT, &pct, 1); } diff --git a/esphome/components/pixoo/pixoo.h b/esphome/components/pixoo/pixoo.h index a7ec2cb84e..01256cc34f 100644 --- a/esphome/components/pixoo/pixoo.h +++ b/esphome/components/pixoo/pixoo.h @@ -49,22 +49,9 @@ class Pixoo : public Display, void set_pixel_(uint32_t index, Color color); void send_command_(uint8_t cmd, const uint8_t *data, uint16_t len); - // Pack a `0xAA len cmd data 0xBB` packet into buf; returns the packet length. - static size_t build_packet_(uint8_t *buf, uint8_t cmd, const uint8_t *data, uint16_t len); - // Fill `total` bytes at buf with a single UNUSED padding packet. - static void pad_unused_(uint8_t *buf, size_t total); - // Divoom LED-board packet protocol constants. - static constexpr uint8_t PACKET_HEAD = 0xAA; - static constexpr uint8_t PACKET_TAIL = 0xBB; - static constexpr uint8_t CMD_DATA = 0x00; - static constexpr uint8_t CMD_LIGHT = 0x01; - static constexpr uint8_t CMD_UNUSED = 0x21; - static constexpr uint8_t CMD_SET_RGB_IOUT = 0x22; - static constexpr size_t PACKET_HEADER_LEN = 4; // head + len(2) + cmd - static constexpr size_t PACKET_STATIC_LEN = 5; // header + tail - static constexpr size_t DMA_CHUNK = 240; // LED board DMA granularity - static constexpr uint8_t DEFAULT_IOUT = 75; // per-channel LED current / white balance default + // Size of the LED board's SPI DMA chunk; the command scratch buffer is one chunk. + static constexpr size_t DMA_CHUNK = 240; PixooModel model_;