Merge branch 'ring-buffer-bulk' into usb-uart-fixed-ring-buffer

esphome/components/usb_uart/usb_uart.cpp

@@ -108,35 +108,6 @@ std::vector<CdcEps> USBUartTypeCdcAcm::parse_descriptors(usb_device_handle_t dev
   return cdc_devs;
 }
 
-void RingBuffer::push(uint8_t item) {
-  if (this->get_free_space() == 0)
-    return;
-  this->buffer_[this->insert_pos_] = item;
-  this->insert_pos_ = (this->insert_pos_ + 1) % this->buffer_size_;
-}
-void RingBuffer::push(const uint8_t *data, size_t len) {
-  size_t free = this->get_free_space();
-  if (len > free)
-    len = free;
-  for (size_t i = 0; i != len; i++) {
-    this->buffer_[this->insert_pos_] = *data++;
-    this->insert_pos_ = (this->insert_pos_ + 1) % this->buffer_size_;
-  }
-}
-
-uint8_t RingBuffer::pop() {
-  uint8_t item = this->buffer_[this->read_pos_];
-  this->read_pos_ = (this->read_pos_ + 1) % this->buffer_size_;
-  return item;
-}
-size_t RingBuffer::pop(uint8_t *data, size_t len) {
-  len = std::min(len, this->get_available());
-  for (size_t i = 0; i != len; i++) {
-    *data++ = this->buffer_[this->read_pos_];
-    this->read_pos_ = (this->read_pos_ + 1) % this->buffer_size_;
-  }
-  return len;
-}
 void USBUartChannelBase::write_array(const uint8_t *data, size_t len) {
   if (!this->initialised_.load()) {
     ESP_LOGD(TAG, "Channel not initialised - write ignored");
@@ -188,10 +159,10 @@ uart::UARTFlushResult USBUartChannelBase::flush() {
 }
 
 bool USBUartChannelBase::peek_byte(uint8_t *data) {
-  if (this->input_buffer_.is_empty()) {
+  if (this->input_buffer_.empty()) {
     return false;
   }
-  *data = this->input_buffer_.peek();
+  *data = this->input_buffer_.front();
   return true;
 }
 bool USBUartChannelBase::read_array(uint8_t *data, size_t len) {
@@ -199,18 +170,12 @@ bool USBUartChannelBase::read_array(uint8_t *data, size_t len) {
     ESP_LOGV(TAG, "Channel not initialised - read ignored");
     return false;
   }
-  auto available = this->available();
-  bool status = true;
-  if (len > available) {
-    ESP_LOGV(TAG, "underflow: requested %zu but returned %d, bytes", len, available);
-    len = available;
-    status = false;
-  }
-  for (size_t i = 0; i != len; i++) {
-    *data++ = this->input_buffer_.pop();
+  const size_t got = this->input_buffer_.pop(data, len);
+  if (got < len) {
+    ESP_LOGV(TAG, "underflow: requested %zu but returned %zu bytes", len, got);
   }
   this->parent_->start_input(this);
-  return status;
+  return got == len;
 }
 void USBUartComponent::setup() { USBClient::setup(); }
 void USBUartComponent::loop() {
@@ -233,7 +198,7 @@ void USBUartComponent::loop() {
 
     // If there is not enough space for the full chunk, let the device subclass
     // handle it (e.g. FTDI clears the buffer to prevent mid-telegram corruption).
-    if (channel->input_buffer_.get_free_space() < chunk->length) {
+    if (channel->input_buffer_.free_space() < chunk->length) {
       this->on_rx_overflow(channel);
     }
     // Push data to ring buffer (now safe in main loop)

esphome/components/usb_uart/usb_uart.h

@@ -112,28 +112,6 @@ enum class ConfigMode : uint8_t {
   CONFIG_MODE_MODEM,   ///< Apply the DTR/RTS state of one already-open channel
 };
 
-class RingBuffer {
- public:
-  RingBuffer(uint16_t buffer_size) : buffer_size_(buffer_size), buffer_(new uint8_t[buffer_size]) {}
-  bool is_empty() const { return this->read_pos_ == this->insert_pos_; }
-  size_t get_available() const {
-    return (this->insert_pos_ + this->buffer_size_ - this->read_pos_) % this->buffer_size_;
-  };
-  size_t get_free_space() const { return this->buffer_size_ - 1 - this->get_available(); }
-  uint8_t peek() const { return this->buffer_[this->read_pos_]; }
-  void push(uint8_t item);
-  void push(const uint8_t *data, size_t len);
-  uint8_t pop();
-  size_t pop(uint8_t *data, size_t len);
-  void clear() { this->read_pos_ = this->insert_pos_ = 0; }
-
- protected:
-  uint16_t insert_pos_ = 0;
-  uint16_t read_pos_ = 0;
-  uint16_t buffer_size_;
-  uint8_t *buffer_;
-};
-
 // Structure for queuing received USB data chunks
 struct UsbDataChunk {
   uint8_t data[usb_host::USB_MAX_PACKET_SIZE];
@@ -174,7 +152,7 @@ class USBUartChannelBase : public uart::UARTComponent, public Parented<USBUartCo
   void write_array(const uint8_t *data, size_t len) override;
   bool peek_byte(uint8_t *data) override;
   bool read_array(uint8_t *data, size_t len) override;
-  size_t available() override { return this->input_buffer_.get_available(); }
+  size_t available() override { return this->input_buffer_.size(); }
   bool is_connected() override { return this->initialised_.load(); }
   uart::UARTFlushResult flush() override;
   // Re-apply the current line settings (baud, parity, etc) to this already-open channel.
@@ -215,12 +193,13 @@ class USBUartChannelBase : public uart::UARTComponent, public Parented<USBUartCo
 
  protected:
   // Not directly instantiable; construct a concrete channel type instead.
-  USBUartChannelBase(uint8_t index, uint16_t buffer_size) : input_buffer_(RingBuffer(buffer_size)), index_(index) {}
+  USBUartChannelBase(uint8_t index, uint16_t buffer_size) : index_(index) { this->input_buffer_.init(buffer_size); }
   void check_logger_conflict() override {}
   LineStopBits line_stop_bits_() const;
   LineParity line_parity_() const;
   // Larger structures first (8+ bytes)
-  RingBuffer input_buffer_;
+  // Only the main loop touches it; the USB task hands data over through usb_data_queue_
+  FixedRingBuffer<uint8_t> input_buffer_;
   LockFreeQueue<UsbOutputChunk, USB_OUTPUT_CHUNK_COUNT> output_queue_;
   // Pool sized to queue capacity (SIZE-1) because LockFreeQueue<T,N> is a ring
   // buffer that holds N-1 elements. This guarantees allocate() returns nullptr

esphome/core/helpers.h

@@ -475,7 +475,7 @@ template<typename T, size_t MAX_CAPACITY = std::numeric_limits<uint16_t>::max()>
   class Iterator {
    public:
     Iterator(FixedRingBuffer *buf, index_type pos) : buf_(buf), pos_(pos) {}
-    T &operator*() { return buf_->data_[(buf_->head_ + pos_) % buf_->capacity_]; }
+    T &operator*() { return buf_->data_[buf_->wrap_(static_cast<size_t>(buf_->head_) + pos_)]; }
     Iterator &operator++() {
       ++pos_;
       return *this;
@@ -490,7 +490,7 @@ template<typename T, size_t MAX_CAPACITY = std::numeric_limits<uint16_t>::max()>
   class ConstIterator {
    public:
     ConstIterator(const FixedRingBuffer *buf, index_type pos) : buf_(buf), pos_(pos) {}
-    const T &operator*() const { return buf_->data_[(buf_->head_ + pos_) % buf_->capacity_]; }
+    const T &operator*() const { return buf_->data_[buf_->wrap_(static_cast<size_t>(buf_->head_) + pos_)]; }
     ConstIterator &operator++() {
       ++pos_;
       return *this;
@@ -532,7 +532,7 @@ template<typename T, size_t MAX_CAPACITY = std::numeric_limits<uint16_t>::max()>
     if (this->count_ >= this->capacity_)
       return false;
     this->data_[this->tail_] = value;
-    this->tail_ = (this->tail_ + 1) % this->capacity_;
+    this->tail_ = this->wrap_(static_cast<size_t>(this->tail_) + 1);
     ++this->count_;
     return true;
   }
@@ -540,7 +540,7 @@ template<typename T, size_t MAX_CAPACITY = std::numeric_limits<uint16_t>::max()>
   /// Push a value, overwriting the oldest if full.
   void push_overwrite(const T &value) {
     this->data_[this->tail_] = value;
-    this->tail_ = (this->tail_ + 1) % this->capacity_;
+    this->tail_ = this->wrap_(static_cast<size_t>(this->tail_) + 1);
     if (this->count_ >= this->capacity_) {
       // Buffer full - advance head to drop oldest, count stays at capacity
       this->head_ = this->tail_;
@@ -549,19 +549,48 @@ template<typename T, size_t MAX_CAPACITY = std::numeric_limits<uint16_t>::max()>
     }
   }
 
+  /// Push up to `len` values without overwriting. Returns how many fit.
+  size_t push(const T *data, size_t len) {
+    static_assert(std::is_trivially_copyable<T>::value, "bulk push copies with memcpy");
+    len = std::min<size_t>(len, this->free_space());
+    if (len == 0)
+      return 0;  // data_ may be null, and memcpy with a null pointer is undefined even for 0 bytes
+    const size_t first = std::min<size_t>(len, this->capacity_ - this->tail_);
+    std::memcpy(this->data_ + this->tail_, data, first * sizeof(T));
+    std::memcpy(this->data_, data + first, (len - first) * sizeof(T));
+    this->tail_ = this->wrap_(static_cast<size_t>(this->tail_) + len);
+    this->count_ += len;
+    return len;
+  }
+
   /// Remove the oldest element.
   void pop() {
     if (this->count_ > 0) {
-      this->head_ = (this->head_ + 1) % this->capacity_;
+      this->head_ = this->wrap_(static_cast<size_t>(this->head_) + 1);
       --this->count_;
     }
   }
 
+  /// Move up to `len` of the oldest values into `out`. Returns how many were copied.
+  size_t pop(T *out, size_t len) {
+    static_assert(std::is_trivially_copyable<T>::value, "bulk pop copies with memcpy");
+    len = std::min<size_t>(len, this->count_);
+    if (len == 0)
+      return 0;
+    const size_t first = std::min<size_t>(len, this->capacity_ - this->head_);
+    std::memcpy(out, this->data_ + this->head_, first * sizeof(T));
+    std::memcpy(out + first, this->data_, (len - first) * sizeof(T));
+    this->head_ = this->wrap_(static_cast<size_t>(this->head_) + len);
+    this->count_ -= len;
+    return len;
+  }
+
   T &front() { return this->data_[this->head_]; }
   const T &front() const { return this->data_[this->head_]; }
   index_type size() const { return this->count_; }
   bool empty() const { return this->count_ == 0; }
   index_type capacity() const { return this->capacity_; }
+  index_type free_space() const { return this->capacity_ - this->count_; }
   bool full() const { return this->count_ == this->capacity_; }
 
   /// Clear all elements (reset to empty, keep capacity)
@@ -577,14 +606,12 @@ template<typename T, size_t MAX_CAPACITY = std::numeric_limits<uint16_t>::max()>
   ConstIterator end() const { return ConstIterator(this, this->count_); }
 
   /// The element `pos` places after the oldest one; the sum is widened so a small index type cannot truncate it
-  const T &operator[](index_type pos) const {
-    size_t index = static_cast<size_t>(this->head_) + pos;
-    if (index >= this->capacity_)
-      index -= this->capacity_;
-    return this->data_[index];
-  }
+  const T &operator[](index_type pos) const { return this->data_[this->wrap_(static_cast<size_t>(this->head_) + pos)]; }
 
  protected:
+  /// Fold an index below 2 * capacity back into the buffer
+  index_type wrap_(size_t index) const { return index >= this->capacity_ ? index - this->capacity_ : index; }
+
   T *data_{nullptr};
   index_type head_{0};
   index_type tail_{0};

tests/components/core/test_fixed_ring_buffer.cpp

@@ -0,0 +1,137 @@
+#include <gtest/gtest.h>
+
+#include <array>
+#include <cstdint>
+
+#include "esphome/core/helpers.h"
+
+namespace esphome::core::testing {
+
+TEST(FixedRingBufferBulk, PushAndPopInOrder) {
+  FixedRingBuffer<uint8_t, 255> buf;
+  buf.init(8);
+  const uint8_t in[] = {1, 2, 3, 4, 5};
+  EXPECT_EQ(buf.push(in, sizeof(in)), 5u);
+  EXPECT_EQ(buf.size(), 5);
+  EXPECT_EQ(buf.front(), 1);
+
+  std::array<uint8_t, 5> out{};
+  EXPECT_EQ(buf.pop(out.data(), out.size()), 5u);
+  EXPECT_EQ(out, (std::array<uint8_t, 5>{1, 2, 3, 4, 5}));
+  EXPECT_TRUE(buf.empty());
+}
+
+TEST(FixedRingBufferBulk, PushStopsWhenFullWithoutOverwriting) {
+  FixedRingBuffer<uint8_t, 255> buf;
+  buf.init(4);
+  const uint8_t first[] = {1, 2, 3};
+  const uint8_t second[] = {4, 5, 6};
+  EXPECT_EQ(buf.push(first, sizeof(first)), 3u);
+  EXPECT_EQ(buf.free_space(), 1);
+  EXPECT_EQ(buf.push(second, sizeof(second)), 1u);
+  EXPECT_TRUE(buf.full());
+  EXPECT_EQ(buf.free_space(), 0);
+  EXPECT_EQ(buf.push(second, sizeof(second)), 0u);
+
+  std::array<uint8_t, 4> out{};
+  EXPECT_EQ(buf.pop(out.data(), out.size()), 4u);
+  EXPECT_EQ(out, (std::array<uint8_t, 4>{1, 2, 3, 4}));
+}
+
+TEST(FixedRingBufferBulk, PopFromEmptyReturnsZero) {
+  FixedRingBuffer<uint8_t, 255> buf;
+  buf.init(4);
+  uint8_t out[4] = {9, 9, 9, 9};
+  EXPECT_EQ(buf.pop(out, sizeof(out)), 0u);
+  EXPECT_EQ(out[0], 9);
+}
+
+TEST(FixedRingBufferBulk, PartialPopLeavesTheRest) {
+  FixedRingBuffer<uint8_t, 255> buf;
+  buf.init(6);
+  const uint8_t in[] = {1, 2, 3, 4, 5};
+  buf.push(in, sizeof(in));
+  uint8_t out[2];
+  EXPECT_EQ(buf.pop(out, sizeof(out)), 2u);
+  EXPECT_EQ(out[0], 1);
+  EXPECT_EQ(out[1], 2);
+  EXPECT_EQ(buf.size(), 3);
+  EXPECT_EQ(buf.front(), 3);
+}
+
+TEST(FixedRingBufferBulk, WrapsAroundTheEnd) {
+  FixedRingBuffer<uint8_t, 255> buf;
+  buf.init(5);
+  const uint8_t lead[] = {10, 11, 12, 13};
+  buf.push(lead, sizeof(lead));
+  uint8_t drop[3];
+  buf.pop(drop, sizeof(drop));  // head 3, tail 4, one element
+
+  // Fills the last slot, then wraps to the start
+  const uint8_t in[] = {20, 21, 22, 23};
+  EXPECT_EQ(buf.push(in, sizeof(in)), 4u);
+  EXPECT_TRUE(buf.full());
+  EXPECT_EQ(buf[0], 13);
+  EXPECT_EQ(buf[4], 23);
+
+  // Pop crosses the end in two runs
+  std::array<uint8_t, 5> out{};
+  EXPECT_EQ(buf.pop(out.data(), out.size()), 5u);
+  EXPECT_EQ(out, (std::array<uint8_t, 5>{13, 20, 21, 22, 23}));
+  EXPECT_TRUE(buf.empty());
+
+  // Indices stay consistent after wrapping, including an exact landing on the end
+  EXPECT_EQ(buf.push(in, 3), 3u);
+  EXPECT_EQ(buf.pop(out.data(), 3), 3u);
+  EXPECT_EQ(out[0], 20);
+  EXPECT_EQ(out[2], 22);
+  EXPECT_EQ(buf.push(lead, sizeof(lead)), 4u);
+  buf.push(lead[0]);
+  EXPECT_TRUE(buf.full());
+  EXPECT_EQ(buf.pop(out.data(), out.size()), 5u);
+  EXPECT_EQ(out, (std::array<uint8_t, 5>{10, 11, 12, 13, 10}));
+}
+
+TEST(FixedRingBufferBulk, MixesWithSingleElementCalls) {
+  FixedRingBuffer<uint16_t, 1000> buf;
+  buf.init(3);
+  buf.push(uint16_t{1});
+  const uint16_t in[] = {2, 3};
+  EXPECT_EQ(buf.push(in, 2), 2u);
+  buf.pop();
+  EXPECT_TRUE(buf.push(uint16_t{4}));
+  uint16_t out[3];
+  EXPECT_EQ(buf.pop(out, 3), 3u);
+  EXPECT_EQ(out[0], 2);
+  EXPECT_EQ(out[1], 3);
+  EXPECT_EQ(out[2], 4);
+}
+
+TEST(FixedRingBufferBulk, UninitialisedHoldsNothing) {
+  FixedRingBuffer<uint8_t, 255> buf;
+  const uint8_t in[] = {1};
+  uint8_t out[1];
+  EXPECT_EQ(buf.push(in, 1), 0u);
+  EXPECT_EQ(buf.pop(out, 1), 0u);
+}
+
+// A uint8_t index at its limit: head and tail plus a full-buffer length go past 255
+TEST(FixedRingBufferBulk, WrapsAtTheIndexTypeLimit) {
+  FixedRingBuffer<uint8_t, 255> buf;
+  buf.init(255);
+  std::array<uint8_t, 255> data{};
+  ASSERT_EQ(buf.push(data.data(), 250), 250u);
+  ASSERT_EQ(buf.pop(data.data(), 250), 250u);  // head and tail now at 250
+  for (size_t i = 0; i < data.size(); i++)
+    data[i] = static_cast<uint8_t>(i);
+  EXPECT_EQ(buf.push(data.data(), data.size()), 255u);
+  EXPECT_TRUE(buf.full());
+  EXPECT_EQ(buf[0], 0);
+  EXPECT_EQ(buf[254], 254);
+  std::array<uint8_t, 255> out{};
+  EXPECT_EQ(buf.pop(out.data(), out.size()), 255u);
+  EXPECT_EQ(out, data);
+  EXPECT_TRUE(buf.empty());
+}
+
+}  // namespace esphome::core::testing