diff --git a/esphome/components/epaper_spi/epaper_spi_t133a01.cpp b/esphome/components/epaper_spi/epaper_spi_t133a01.cpp index 5735333761..95d1fcb484 100644 --- a/esphome/components/epaper_spi/epaper_spi_t133a01.cpp +++ b/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 diff --git a/tests/components/epaper_spi/common.h b/tests/components/epaper_spi/common.h new file mode 100644 index 0000000000..5ac12afa6a --- /dev/null +++ b/tests/components/epaper_spi/common.h @@ -0,0 +1,50 @@ +#pragma once + +#include + +#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 diff --git a/tests/components/epaper_spi/display/test_t133a01_transfer.cpp b/tests/components/epaper_spi/display/test_t133a01_transfer.cpp new file mode 100644 index 0000000000..5c7abdc022 --- /dev/null +++ b/tests/components/epaper_spi/display/test_t133a01_transfer.cpp @@ -0,0 +1,77 @@ +#include + +#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