[epaper_spi] Do not yield on the final row of a T133A01 transfer phase (#17709)

esphome/components/epaper_spi/epaper_spi_t133a01.cpp

@@ -311,9 +311,12 @@ bool HOT EPaperT133A01::transfer_data() {
       this->current_data_index_ = half;
 
       if (millis() - start_time > MAX_TRANSFER_TIME) {
-        return false;
+        break;
       }
     }
+    if (half < total_rows) {
+      return false;
+    }
     ESP_LOGD(TAG, "CS phase done");
     this->disable();
     this->cs_pin_->digital_write(true);  // deselect CS
@@ -346,9 +349,12 @@ bool HOT EPaperT133A01::transfer_data() {
       this->current_data_index_ = half;
 
       if (millis() - start_time > MAX_TRANSFER_TIME) {
-        return false;
+        break;
       }
     }
+    if (half < total_rows * 2) {
+      return false;
+    }
     ESP_LOGD(TAG, "CS1 phase done");
     this->disable();
     this->cs1_pin_->digital_write(true);  // deselect CS1

tests/components/epaper_spi/common.h

@@ -0,0 +1,50 @@
+#pragma once
+
+#include <gtest/gtest.h>
+
+#include "esphome/components/spi/spi.h"
+#include "esphome/core/hal.h"
+
+namespace esphome::epaper_spi::testing {
+
+/// SPI delegate that records transaction boundaries and burns wall-clock time on each
+/// row write, so a transfer can be driven past its MAX_TRANSFER_TIME yield deadline.
+class TimedSPIDelegate : public spi::SPIDelegate {
+ public:
+  explicit TimedSPIDelegate(uint32_t row_transfer_ms) : row_transfer_ms_(row_transfer_ms) {}
+
+  uint8_t transfer(uint8_t data) override { return 0; }
+
+  void write_array(const uint8_t *ptr, size_t length) override {
+    // A row of pixel data is one "slow" write; single-byte writes are commands.
+    if (length > 1) {
+      const uint32_t until = millis() + this->row_transfer_ms_;
+      while (millis() < until) {
+      }
+    }
+  }
+
+  void begin_transaction() override { this->begin_count++; }
+  void end_transaction() override { this->end_count++; }
+
+  int begin_count{0};
+  int end_count{0};
+
+ protected:
+  uint32_t row_transfer_ms_;
+};
+
+/// GPIO pin that just remembers the last level written to it.
+class RecordingPin : public GPIOPin {
+ public:
+  void setup() override {}
+  void pin_mode(gpio::Flags flags) override {}
+  gpio::Flags get_flags() const override { return gpio::Flags::FLAG_NONE; }
+  bool digital_read() override { return false; }
+  void digital_write(bool value) override { this->level = value; }
+  size_t dump_summary(char *buffer, size_t len) const override { return snprintf(buffer, len, "recording"); }
+
+  bool level{true};
+};
+
+}  // namespace esphome::epaper_spi::testing

tests/components/epaper_spi/display/test_t133a01_transfer.cpp

@@ -0,0 +1,77 @@
+#include <gtest/gtest.h>
+
+#include "../common.h"
+#include "esphome/components/epaper_spi/epaper_spi_t133a01.h"
+
+namespace esphome::epaper_spi::testing {
+
+/// Exposes the protected transfer machinery so the yield behaviour can be driven directly.
+class TestableT133A01 : public EPaperT133A01 {
+ public:
+  TestableT133A01(uint16_t width, uint16_t height) : EPaperT133A01("test", width, height, nullptr, 0) {}
+
+  void install(spi::SPIDelegate *delegate) {
+    this->delegate_ = delegate;
+    this->set_dc_pin(&this->dc);
+    this->set_cs_pins(&this->cs, &this->cs1);
+    ASSERT_TRUE(this->init_buffer_(this->buffer_length_));
+  }
+
+  using EPaperT133A01::transfer_data;
+
+  RecordingPin dc, cs, cs1;
+};
+
+/// Regression test for the T133A01 transfer deadlock (issue #17668).
+///
+/// `transfer_data()` evaluates its yield deadline *after* incrementing the row counter, so the
+/// deadline can expire on a phase's final row. The phase is then complete but the function
+/// reports "not done"; on the next call the phase guard is false, so the `disable()` /
+/// CS-deassert epilogue is skipped permanently. The SPI transaction is never closed and the
+/// next `enable()` blocks forever, tripping the task watchdog.
+///
+/// Here the CS phase is two rows and every row write overruns the deadline, so the second call
+/// completes the phase exactly as the deadline expires, which is the failing alignment. The completed
+/// phase must still run its epilogue: end the transaction and deassert CS.
+TEST(EPaperT133A01, CompletedPhaseRunsEpilogueWhenDeadlineExpiresOnFinalRow) {
+  // width 8 -> 4 bytes per row, 2 per half-row; height 2 -> a two-row CS phase
+  TestableT133A01 display(8, 2);
+  TimedSPIDelegate delegate(MAX_TRANSFER_TIME + 5);
+  display.install(&delegate);
+
+  // First call performs the one-off CCSET setup (which opens and closes a transaction of its
+  // own) and then writes row 0 of the CS phase before yielding on the deadline.
+  ASSERT_FALSE(display.transfer_data()) << "transfer should have yielded after the first row";
+  ASSERT_FALSE(display.cs.level) << "CS must stay asserted across a yield mid-phase";
+  const int closed_after_setup = delegate.end_count;
+
+  // Second call writes the final row of the CS phase; the deadline expires as it lands.
+  display.transfer_data();
+
+  EXPECT_EQ(delegate.end_count, closed_after_setup + 1)
+      << "completed CS phase skipped disable() -- SPI transaction left open";
+  EXPECT_TRUE(display.cs.level) << "completed CS phase left CS asserted";
+}
+
+/// The CS1 phase has the same off-by-one, but fails worse: after the skipped epilogue the
+/// function falls through to `return true`, reporting the transfer complete while the SPI
+/// transaction is still open and CS1 is still asserted. The next command's `enable()` then
+/// blocks forever. A transfer that reports done must have released the bus.
+TEST(EPaperT133A01, TransferReportsDoneOnlyAfterReleasingTheBus) {
+  TestableT133A01 display(8, 2);
+  TimedSPIDelegate delegate(MAX_TRANSFER_TIME + 5);
+  display.install(&delegate);
+
+  // Both phases are two rows each and every row overruns the deadline, so the transfer needs
+  // one call per row plus the setup call. Bound the loop so a regression fails rather than hangs.
+  int calls = 0;
+  while (!display.transfer_data()) {
+    ASSERT_LT(++calls, 10) << "transfer never reported completion";
+  }
+
+  EXPECT_TRUE(display.cs1.level) << "transfer reported done with CS1 still asserted";
+  EXPECT_EQ(delegate.begin_count, delegate.end_count)
+      << "transfer reported done with an SPI transaction still open -- the next enable() would deadlock";
+}
+
+}  // namespace esphome::epaper_spi::testing