From 2d6a7dae547f0940f0c1deb76c6d201d435c5a20 Mon Sep 17 00:00:00 2001 From: Clyde Stubbs <2366188+clydebarrow@users.noreply.github.com> Date: Tue, 29 Sep 2026 09:40:39 +1000 Subject: [PATCH] [mipi_rgb] Fix SWRESET delay for st7701s (#17646) --- esphome/components/mipi/__init__.py | 10 ++- esphome/components/mipi_rgb/display.py | 2 +- esphome/components/mipi_rgb/mipi_rgb.cpp | 4 +- esphome/components/mipi_rgb/models/st7701s.py | 5 +- .../mipi_rgb/test_mipi_rgb_config.py | 51 ++++++++++++ .../mipi_rgb/test_reset_sequence.py | 81 +++++++++++++++++++ .../mipi_spi/test_get_sequence.py | 64 +++++++++++++++ 7 files changed, 211 insertions(+), 6 deletions(-) create mode 100644 tests/component_tests/mipi_rgb/test_reset_sequence.py create mode 100644 tests/component_tests/mipi_spi/test_get_sequence.py diff --git a/esphome/components/mipi/__init__.py b/esphome/components/mipi/__init__.py index 3f73f96327..50b84b630d 100644 --- a/esphome/components/mipi/__init__.py +++ b/esphome/components/mipi/__init__.py @@ -606,11 +606,11 @@ class DriverChip: """ Create the init sequence for the display. Use the default sequence from the model, if any, and append any custom sequence provided in the config. - Append SLPOUT (if not already in the sequence) and DISPON to the end of the sequence + Append SLPOUT (if not suppressed by the model) and DISPON to the end of the sequence MADCTL will be set if add_madctl is True If add_reset is True, a reset is prepended: a software reset when no reset pin is configured (and the model doesn't skip it), followed by a settling delay that - both a software and a hardware reset require. + both a software and a hardware reset require. The delay length is set via reset_delay, and defaults to 10ms. Returns the init sequence """ sequence = list(self.initsequence or ()) @@ -620,12 +620,16 @@ class DriverChip: sequence = [x if isinstance(x, tuple) else (x,) for x in sequence] if add_reset: + # Matches the 1-255ms range map_sequence() already allows for a "delay N" entry. + reset_delay = self.get_default("reset_delay", 10) + if reset_delay < 1 or reset_delay > 255: + raise ValueError("reset_delay must be between 1 and 255ms") reset: list = [] # A software reset is only needed when there is no hardware reset pin. if CONF_RESET_PIN not in config and not self.skip_command("SWRESET"): reset.append((SWRESET,)) # Both a software and a hardware reset need a settling delay before further commands. - reset.append(delay(10)) + reset.append(delay(reset_delay)) sequence = reset + sequence # Set pixel format if not already in the custom sequence diff --git a/esphome/components/mipi_rgb/display.py b/esphome/components/mipi_rgb/display.py index b91528160e..9cba56a976 100644 --- a/esphome/components/mipi_rgb/display.py +++ b/esphome/components/mipi_rgb/display.py @@ -285,7 +285,7 @@ async def to_code(config: ConfigType) -> None: if CONF_SPI_ID in config: await spi.register_spi_device(var, config, write_only=True) - sequence = model.get_sequence(config) + sequence = model.get_sequence(config, add_reset=True) cg.add(var.set_init_sequence(sequence)) cg.add(var.set_color_mode(COLOR_ORDERS[config[CONF_COLOR_ORDER]])) diff --git a/esphome/components/mipi_rgb/mipi_rgb.cpp b/esphome/components/mipi_rgb/mipi_rgb.cpp index 3f83da7f80..034efb9c92 100644 --- a/esphome/components/mipi_rgb/mipi_rgb.cpp +++ b/esphome/components/mipi_rgb/mipi_rgb.cpp @@ -44,8 +44,10 @@ void MipiRgb::setup_enables_() { void MipiRgbSpi::setup() { this->setup_enables_(); this->spi_setup(); - this->write_init_sequence_(); this->common_setup_(); + if (this->is_failed()) + return; + this->write_init_sequence_(); } void MipiRgbSpi::write_command_(uint8_t value) { this->enable(); diff --git a/esphome/components/mipi_rgb/models/st7701s.py b/esphome/components/mipi_rgb/models/st7701s.py index cad5dc8e20..b51e7447ad 100644 --- a/esphome/components/mipi_rgb/models/st7701s.py +++ b/esphome/components/mipi_rgb/models/st7701s.py @@ -7,6 +7,10 @@ SDIR_CMD = 0xC7 class ST7701S(RgbDriverChip): + def __init__(self, *args, reset_delay=50, **kwargs): + kwargs["reset_delay"] = reset_delay + super().__init__(*args, **kwargs) + # The ST7701s does not use the standard MADCTL bits for x/y mirroring def add_madctl(self, sequence: list, config: dict) -> int: transform = self.get_transform(config) @@ -49,7 +53,6 @@ st7701s = ST7701S( pclk_frequency="16MHz", pclk_inverted=True, initsequence=( - (0x01,), # Software Reset (0xFF, 0x77, 0x01, 0x00, 0x00, 0x10), # Page 0 (0xC0, 0x3B, 0x00), (0xC1, 0x0D, 0x02), (0xC2, 0x31, 0x05), (0xB0, 0x00, 0x11, 0x18, 0x0E, 0x11, 0x06, 0x07, 0x08, 0x07, 0x22, 0x04, 0x12, 0x0F, 0xAA, 0x31, 0x18,), diff --git a/tests/component_tests/mipi_rgb/test_mipi_rgb_config.py b/tests/component_tests/mipi_rgb/test_mipi_rgb_config.py index ac8e111ddb..e677577ec0 100644 --- a/tests/component_tests/mipi_rgb/test_mipi_rgb_config.py +++ b/tests/component_tests/mipi_rgb/test_mipi_rgb_config.py @@ -1,5 +1,7 @@ """Tests for mipi_rgb configuration validation.""" +from collections.abc import Generator + import pytest from esphome import config_validation as cv @@ -17,6 +19,7 @@ from esphome.components.esp32 import ( VARIANT_ESP32S3, VARIANT_ESP32S31, ) +from esphome.components.mipi import DriverChip import esphome.components.pca9554 # noqa: F401 import esphome.components.xl9535 # noqa: F401 from esphome.const import ( @@ -44,6 +47,19 @@ DATA_PINS = { } +@pytest.fixture(autouse=True) +def _remove_test_models() -> Generator[None]: + """Unregister chips created by a test. + + display.py modules drain DriverChip.models when first imported, so a + leftover TEST-* chip could become a selectable model there. + """ + existing = set(DriverChip.models) + yield + for name in set(DriverChip.models) - existing: + del DriverChip.models[name] + + def _set_s3(set_core_config: SetCoreConfigCallable) -> None: set_core_config( PlatformFramework.ESP32_IDF, @@ -176,6 +192,41 @@ def test_configuration_succeeds_on_supported_variants( CONFIG_SCHEMA(config) +def test_st7701s_default_reset_delay() -> None: + """ST7701S instances default to a 50ms reset delay. + + The datasheet's stated 5ms is too short in practice; ST7701S overrides the + DriverChip default of 10ms with its own default of 50ms. + """ + from esphome.components.mipi_rgb.models.st7701s import st7701s + + assert st7701s.get_default("reset_delay") == 50 + + +def test_st7701s_reset_delay_can_be_overridden() -> None: + """An explicit reset_delay overrides the ST7701S default of 50ms.""" + from esphome.components.mipi_rgb.models.st7701s import ST7701S + + chip = ST7701S("TEST-ST7701S-RESET-DELAY", width=480, height=480, reset_delay=99) + + assert chip.get_default("reset_delay") == 99 + + +def test_st7701s_extend_inherits_reset_delay_default() -> None: + """extend() carries the 50ms default forward to derived board models. + + Every shipped ST7701S variant is built via ``st7701s.extend(...)`` rather + than direct construction, so the override in ``ST7701S.__init__`` must + survive that path (see DriverChip.extend, which re-passes the copied + defaults as kwargs to the constructor). + """ + from esphome.components.mipi_rgb.models.st7701s import st7701s + + extended = st7701s.extend("TEST-ST7701S-EXTEND", width=480, height=480) + + assert extended.get_default("reset_delay") == 50 + + def test_only_on_variant_rejects_unsupported_variant( set_core_config: SetCoreConfigCallable, ) -> None: diff --git a/tests/component_tests/mipi_rgb/test_reset_sequence.py b/tests/component_tests/mipi_rgb/test_reset_sequence.py new file mode 100644 index 0000000000..50478361b1 --- /dev/null +++ b/tests/component_tests/mipi_rgb/test_reset_sequence.py @@ -0,0 +1,81 @@ +"""End-to-end tests for the mipi_rgb SPI reset sequence. + +These exercise the actual codegen path (mipi_rgb/display.py's +``model.get_sequence(config, add_reset=True)`` call) rather than calling +DriverChip.get_sequence directly, so a regression that drops add_reset or +reintroduces a hardcoded SWRESET into a model's initsequence would be caught +here. +""" + +from collections.abc import Callable +from pathlib import Path + +# A model with no reset_pin default: SWRESET ({1, 0}) is prepended ahead of the +# inherited ST7701S reset_delay ({50, 255}). +_NO_RESET_PIN_YAML = """ +esphome: + name: mipi-rgb-reset-test +esp32: + board: esp32-s3-devkitc-1 + framework: + type: esp-idf +psram: + mode: octal +spi: + id: spi_bus + clk_pin: 10 + mosi_pin: 11 +display: + - platform: mipi_rgb + id: no_reset_display + spi_id: spi_bus + model: MAKERFABS-4 +""" + +# A model with a reset_pin default: no SWRESET, just the settling delay. +_RESET_PIN_YAML = """ +esphome: + name: mipi-rgb-reset-test +esp32: + board: esp32-s3-devkitc-1 + framework: + type: esp-idf +psram: + mode: octal +spi: + id: spi_bus + clk_pin: 6 + mosi_pin: 7 +display: + - platform: mipi_rgb + id: has_reset_display + spi_id: spi_bus + model: WAVESHARE-3.16-320X820 +""" + + +def test_swreset_and_reset_delay_without_reset_pin( + generate_main: Callable[[str | Path], str], + tmp_path: Path, +) -> None: + """A model with no reset_pin gets SWRESET plus the ST7701S 50ms delay.""" + yaml_file = tmp_path / "no_reset.yaml" + yaml_file.write_text(_NO_RESET_PIN_YAML) + + main_cpp = generate_main(yaml_file) + + assert "no_reset_display->set_init_sequence({1, 0, 50, 255," in main_cpp + + +def test_reset_delay_only_with_reset_pin( + generate_main: Callable[[str | Path], str], + tmp_path: Path, +) -> None: + """A model with a reset_pin default skips SWRESET but keeps the settling delay.""" + yaml_file = tmp_path / "has_reset.yaml" + yaml_file.write_text(_RESET_PIN_YAML) + + main_cpp = generate_main(yaml_file) + + assert "has_reset_display->set_init_sequence({50, 255," in main_cpp + assert "has_reset_display->set_init_sequence({1, 0," not in main_cpp diff --git a/tests/component_tests/mipi_spi/test_get_sequence.py b/tests/component_tests/mipi_spi/test_get_sequence.py new file mode 100644 index 0000000000..263b567b9c --- /dev/null +++ b/tests/component_tests/mipi_spi/test_get_sequence.py @@ -0,0 +1,64 @@ +"""Tests for DriverChip.get_sequence's reset-delay handling.""" + +from collections.abc import Generator + +import pytest + +from esphome.components.mipi import CONF_INVERT_COLORS, CONF_PIXEL_MODE, DriverChip + +# A minimal config with no reset pin: enough for get_sequence(add_madctl=False) to run +# without needing a full display configuration. +_BASE_CONFIG = {CONF_PIXEL_MODE: "16bit", CONF_INVERT_COLORS: False} + + +@pytest.fixture(autouse=True) +def _remove_test_models() -> Generator[None]: + """Unregister chips created by a test.""" + existing = set(DriverChip.models) + yield + for name in set(DriverChip.models) - existing: + del DriverChip.models[name] + + +def test_get_sequence_defaults_to_10ms_reset_delay() -> None: + """A model with no reset_delay default falls back to a 10ms settling delay.""" + chip = DriverChip("TEST-GET-SEQUENCE-DEFAULT") + + sequence = chip.get_sequence(_BASE_CONFIG, add_madctl=False, add_reset=True) + + # SWRESET ({1, 0}) is prepended (no reset pin configured), followed by the + # 10ms settling delay, flattened to {10, 255}. + assert sequence[:4] == (1, 0, 10, 255) + + +def test_get_sequence_uses_model_reset_delay_default() -> None: + """A model's own reset_delay default overrides the base 10ms default.""" + chip = DriverChip("TEST-GET-SEQUENCE-CUSTOM-DELAY", reset_delay=99) + + sequence = chip.get_sequence(_BASE_CONFIG, add_madctl=False, add_reset=True) + + assert sequence[:4] == (1, 0, 99, 255) + + +@pytest.mark.parametrize("reset_delay", [0, 256]) +def test_get_sequence_rejects_out_of_range_reset_delay(reset_delay: int) -> None: + """reset_delay outside 1-255ms is rejected. + + This matches the 1-255ms range map_sequence() already allows for a + "delay N" entry in a custom init sequence. + """ + chip = DriverChip("TEST-GET-SEQUENCE-BAD-DELAY", reset_delay=reset_delay) + + with pytest.raises(ValueError, match="reset_delay must be between 1 and 255ms"): + chip.get_sequence(_BASE_CONFIG, add_madctl=False, add_reset=True) + + +def test_get_sequence_skips_reset_delay_validation_without_add_reset() -> None: + """An out-of-range reset_delay is only checked when add_reset is requested. + + mipi_dsi calls get_sequence with add_reset=False and never uses + reset_delay, so an invalid default there should not raise. + """ + chip = DriverChip("TEST-GET-SEQUENCE-NO-RESET", reset_delay=999) + + chip.get_sequence(_BASE_CONFIG, add_madctl=False, add_reset=False)