fix(m5stack-tab5): rotation audit, experimental opt-in ST7121 panel rotation (default off), and DSI underrun fixes by finger563 · Pull Request #738 · esp-cpp/espp

finger563 · 2026-08-28T16:18:09Z

Summary

Audit of how the M5Stack Tab5's three display variants (ILI9881 / ST7123 / ST7121) handle rotation, plus fixes for DSI scan-out underrun (display streaking) found while testing on hardware.

The branch originally routed 0°/180° rotation on the ST7121 TDDI variant to the panel itself (MADCTL GS/SS scan-direction flip) instead of the ESP32-P4 PPA. Hardware testing showed the 180° MADCTL flip renders corrupted on (at least some) ST7121 units — the init sequence programs full TDDI gate/source mux tables (the 0xAC block) matched to the normal scan direction, and a MADCTL GS flip alone reorders gate scanning without swapping them. Panel-side rotation is therefore experimental and opt-in, gated behind CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION (default off). The default build keeps the pre-branch known-good behavior: PPA rotation for all orientations and zero runtime MADCTL writes.

What landed

Rotation audit + plumbing (default behavior unchanged)

Audit conclusions documented in code: these are MIPI-DSI video-mode (DPI) panels — a fixed 720x1280 raster with no axis swap — so panel-side rotation could only ever cover 0/180; 90/270 must stay on the PPA. And on the ST7121 specifically, the init-time gate/source mux tables make even runtime 0/180 MADCTL flips unsafe.
Wired Display::LvglConfig::rotation_callback to a new M5StackTab5::on_display_rotation(); the rotation decision is centralized in a single predicate (panel_handles_rotation()) shared by the flush path and the callback.
Enum unification (DisplayRotation <-> lv_display_rotation_t mapping pinned down with explicit converters) and init-order hardening: the current rotation is (re)applied when the display driver comes up, correct in either initialize_lcd() / initialize_display() order, with the thread-safety of the init-path call documented at the call site.
Experimental opt-in: CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION (default n) with the corruption warning in its Kconfig help. When enabled, the ST7121 applies 0/180 via the display driver's set_rotation() (MADCTL) and flush() skips the PPA for those orientations.
ILI9881 / ST7123 behavior unchanged; touch mapping (touchpad_convert) is invariant to the transform mechanism; camera PPA pass unaffected.

DSI underrun / streaking fixes (active by default)

Streaking traced to DSI bridge FIFO underrun from PSRAM read contention against the DPI panel's continuous ~140 MB/s framebuffer scan-out. Three traffic reductions:

DMA2D re-enabled for draw_bitmap: IDF 6.0 removed flags.use_dma2d in favor of esp_lcd_dpi_panel_enable_dma2d(), which was never called — every flush had silently regressed to a CPU memcpy (~5.4 MB of PSRAM/cache traffic per full frame).
PPA rotates directly into the DPI framebuffer (output block placement) instead of PPA -> scratch buffer -> draw_bitmap copy, halving the rotation path's PSRAM traffic.
PPA burst length 128 -> 64 on both PPA clients (display + camera); full-length PPA bursts are known to starve the DSI scan-out even at 200 MHz hex PSRAM ((draw/ppa) cause DSI underrun under heavy load on ESP32-P4 lvgl/lvgl#9590).

Hardware testing

ST7121 unit: 180° MADCTL scan flip renders corrupted -> panel-side rotation gated off by default (see PR discussion for the full root-cause analysis and the streaking diagnosis / escalation ladder).

Testing

Tab5 example builds clean in both configurations (CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION off and on) with ESP-IDF 6.0, esp32p4.

🤖 Generated with Claude Code

…21 variant All three Tab5 display variants previously rotated every non-zero LVGL orientation at flush time via the ESP32-P4 PPA (with an lv_draw_sw_rotate fallback); the espp::Display rotation_callback was left null, so the display driver's set_rotation() (MADCTL) support was never used. Route rotation through the espp::Display machinery for the ST7121: - Wire the Display rotation_callback to a new on_display_rotation() which, for the ST7121, forwards 0/180 to the driver's set_rotation() (MADCTL GS/SS scan-direction flip done by the panel itself) and restores the natural scan direction for 90/270. - Skip the PPA/software buffer rotation in flush() when the panel handles the current rotation (new panel_handles_rotation()); the logical frame is then written unrotated and the panel flips it at scan-out, which lands partial areas exactly where LVGL's rotated mapping expects them. - 90/270 keep the PPA path: the Tab5 panels are MIPI-DSI DPI (video mode) panels streaming a fixed 720x1280 raster, so the panel cannot swap axes. The ILI9881 and ST7123 variants keep their existing PPA rotation path, and the touch transform is unchanged: touchpad_convert() maps physical to logical coordinates from the LVGL rotation, which is independent of how the display rotation is implemented. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

github-actions · 2026-08-28T16:18:59Z

✅Static analysis result - no issues found! ✅

Copilot

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR updates M5Stack Tab5 rotation handling so the ST7121 (TDDI) panel uses the panel’s MADCTL scan-direction flip for 0°/180° rotation, avoiding PPA/software rotation artifacts with partial flush areas.

Changes:

Wire Display::LvglConfig::rotation_callback to M5StackTab5::on_display_rotation() to forward 0°/180° rotation to the ST7121 driver.
Add panel_handles_rotation() to decide when to skip PPA/software rotation.
Update flush() to bypass rotation when the panel applies it (ST7121, 180°).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
components/m5stack-tab5/src/video.cpp	Adds rotation callback handling, panel-rotation decision logic, and adjusts flush rotation path accordingly.
components/m5stack-tab5/include/m5stack-tab5.hpp	Declares the new rotation callback handler and helper used by the updated flush logic.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…ply initial rotation, document MADCTL ordering - Pin the espp::DisplayRotation <-> lv_display_rotation_t value correspondence with a static_assert and route all conversions through explicit to_lv_rotation()/to_display_rotation() helpers; the panel-vs-PPA decision in on_display_rotation() now uses the exact same panel_handles_rotation() predicate flush() uses. - Explicitly (re)apply the current rotation at the end of initialize_lcd() so the panel MADCTL matches flush()'s expectation from the first frame even if initialize_display() was called before initialize_lcd() (the normal order is already covered: LVGL sends LV_EVENT_RESOLUTION_CHANGED synchronously from lv_display_set_rotation() in Display's constructor, before any flush can run). - Document why the runtime MADCTL write is safe w.r.t. the DPI pipeline: it travels on the DSI generic/DBI command channel, hardware-arbitrated against the video stream and independent of the framebuffer/DMA path; worst case is a single transient frame during a rotation change, which is stated and accepted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…ntal Kconfig (default off) Hardware testing showed the MADCTL GS/SS 180-degree scan flip renders corrupted on (at least some) ST7121 units. Root-cause analysis: the init sequence programs full TDDI gate/source mux tables (the 0xAC block), which vendor inits pair to a specific scan direction - a MADCTL GS flip alone reorders gate scanning without swapping those tables, producing interleaved garbage. (The bit positions themselves match Espressif's esp_lcd_st7121 mirror implementation, whose generic template was likely never validated on this glass either; ESPHome's mipi_dsi driver documents the same 0/180-only constraint for DSI panels generally, so the design was sound - this panel's init tables are what make runtime flips unsafe.) Default behavior is now EXACTLY the pre-branch known-good path: PPA/software rotation for all orientations and ZERO runtime MADCTL writes (the rotation callback is a full no-op), which also makes A/B comparison for unrelated display artifacts clean. The panel-side path remains available for experimentation via CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION, and the rest of the branch (init-order hardening, single-predicate rotation decision, enum unification, documented DSI command-channel safety) stands. Both configurations build clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

finger563 · 2026-08-28T20:59:31Z

Hardware test result (ST7121 unit): the MADCTL 180° scan flip renders corrupted — addressed in 33eb080.

Root cause analysis: the ST7121 init sequence programs full TDDI gate/source mux tables (the 0xAC block), which vendor inits pair to a specific scan direction. A MADCTL GS flip alone reorders gate scanning without swapping those tables → interleaved garbage. Corroborating evidence: espp's GS/SS bit positions match Espressif's official esp_lcd_st7121 mirror implementation (generic template code, likely never validated on this glass), and ESPHome's mipi_dsi driver documents the same 0/180-only constraint for DSI panels generally — the design was sound; this panel's init tables are what make runtime flips unsafe.

Resolution: panel-side rotation is now gated behind CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION (default off). The default build is exactly the pre-branch known-good behavior — PPA rotation for all orientations and zero runtime MADCTL writes. The branch's remaining value stands: the audit conclusion (documented in code), init-order hardening, the single-predicate rotation decision, and enum unification. Both configurations build clean.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

…n-out (display streaking) The Tab5's DPI (video mode) panel continuously scans its PSRAM framebuffer at ~140 MB/s via DW-GDMA; when concurrent PSRAM/AXI traffic starves that read stream the DSI bridge FIFO underruns and the panel shows streaks along the physical scan-line axis. The flush path was generating far more PSRAM traffic than needed: - On ESP-IDF 6.0 the esp_lcd_dpi_panel_config_t::flags.use_dma2d flag no longer exists (replaced by esp_lcd_dpi_panel_enable_dma2d()), so every esp_lcd_panel_draw_bitmap silently regressed to a CPU memcpy through the cache (~1.8 MB read + ~1.8 MB write + ~1.8 MB writeback per full frame). Call esp_lcd_dpi_panel_enable_dma2d() so copies run on the 2D-DMA engine again (matches M5Stack's UserDemo and esp-bsp's m5stack_tab5). - The PPA rotation path rotated into a PSRAM scratch buffer and then draw_bitmap-copied that into the framebuffer. The PPA output supports placing a block inside a larger picture, so rotate directly into the DPI framebuffer at the rotated offset instead, halving that path's PSRAM traffic. The CPU software-rotate fallback keeps the scratch buffer. - Register both PPA clients (display rotation and camera preview) with data_burst_length = 64 instead of the default 128: full-length PPA bursts are known to starve the DSI scan-out DMA even with 200 MHz hex PSRAM (lvgl/lvgl#9590; LVGL exposes the same knob as LV_PPA_BURST_LENGTH). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

finger563 · 2026-08-28T21:32:54Z

Display streaking: root cause + fixes (`513f565`)

The streaking is consistent with DSI bridge FIFO underrun from PSRAM read contention: the DPI (video-mode) panel scans its PSRAM framebuffer continuously at ~140 MB/s via DW-GDMA, and when competing PSRAM/AXI traffic starves that stream the bridge discards pixels, which renders as streaks along the physical scan-line axis in any orientation. This matches "worse with HW_ROTATION off" (that config runs the PPA + an extra full-frame copy on every flush). Note: on this board's pre-v3.0 P4 silicon the driver's underrun happens error log may never fire (the underrun interrupt is only supported on rev v3.0 per IDF v5.5.2 release notes), so absence of that log does not rule underrun out.

Three traffic reductions landed in 513f565:

DMA2D re-enabled for draw_bitmap — IDF 6.0 removed flags.use_dma2d in favor of esp_lcd_dpi_panel_enable_dma2d(), which we never called, so every flush had silently regressed to a CPU memcpy (~5.4 MB of cache/PSRAM traffic per full frame). M5Stack's UserDemo and esp-bsp's m5stack_tab5 both use DMA2D here.
PPA rotates directly into the DPI framebuffer (output block placement) instead of PPA -> scratch buffer -> draw_bitmap copy — halves the rotation path's PSRAM traffic.
PPA burst length 128 -> 64 on both PPA clients (display + camera) — full-length PPA bursts are known to starve the DSI scan-out even at 200 MHz hex PSRAM ((draw/ppa) cause DSI underrun under heavy load on ESP32-P4 lvgl/lvgl#9590).

If streaks persist, please try in this order

Rule out the camera: comment out initialize_camera() in the example. The CSI/ISP pipeline writes ~100+ MB/s into PSRAM; camera-induced DSI underrun at max PSRAM config is documented ((draw/ppa) cause DSI underrun under heavy load on ESP32-P4 lvgl/lvgl#9590). If this fixes it, the remaining fix is camera-side (lower preview rate/resolution).
PPA burst 32: change PPA_DATA_BURST_LENGTH_64 -> PPA_DATA_BURST_LENGTH_32 in video.cpp and camera.cpp.
Larger L2 cache: in example/sdkconfig set CONFIG_CACHE_L2_CACHE_512KB=y (keep CONFIG_CACHE_L2_CACHE_LINE_128B=y) — fewer CPU misses to PSRAM.
Disable power management: set CONFIG_PM_ENABLE=n (M5Stack's UserDemo ships without PM; the DPI driver pins CPU freq, but PM still adds latency/jitter elsewhere).
XIP experiment: toggle CONFIG_SPIRAM_XIP_FROM_PSRAM (currently y). It removes flash-cache stalls but adds instruction-fetch traffic to PSRAM; Espressif's own P4 external-RAM guide says to profile both ways.
Lower DPI clock: dpi_clock_freq_mhz 70 -> 65/60 in video.cpp (ST7121/ST7123 branches). Caveat: the TDDI touch scan is timed against the pixel clock, so verify touch still works.
AXI-ICM QoS (last resort, experimental): IDF ships hal/axi_icm_ll.h with per-master token-bucket regulators (axi_icm_ll_set_qos_burstiness(AXI_ICM_MASTER_CPU/DMA2D, ..., AXI_ICM_ACCESS_READ)) that could throttle the CPU/DMA2D masters so the DW-GDMA scan-out wins arbitration — nothing in IDF programs it yet, so this would be a hand-rolled experiment.

If a serial console is attached during streaking, any lcd.dsi.dpi: ... underrun happens message would confirm the diagnosis outright (only possible on rev v3 silicon).

…_rotation() call on_display_rotation()'s ordering comment claimed it runs on the LVGL thread, but initialize_lcd() also calls it directly on the init thread. Scope the LVGL-thread claim to the rotation-callback invocation and spell out at the call site why the direct init-path call is safe in either init order: in the documented order display_ is still null (no LVGL display, flush callback, or handler task exists, and the Display constructor re-fires the callback synchronously before any flush); in the reversed order the MADCTL write rides the DSI command channel, which never touches the DPI framebuffer or its DMA, so at worst one transient frame — and with CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION off (default) the call is a no-op entirely. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

…rently flushing LVGL thread initialize_lcd() writes lcd_handles_.panel early (before panel->init(), DMA2D enable, and caching the dpi_framebuffer_/dpi_framebuffer_bytes_ pair), while flush() gated only on a plain null-check of that pointer. With the reversed init order (initialize_display() first, the app already pumping LVGL on another thread), flush() could race those plain writes: undefined behavior, and observably a not-yet-initialized panel or a torn framebuffer pointer/size pair. Publish the LCD state through a single std::atomic<bool> lcd_initialized_ gate: initialize_lcd() clears it on entry, writes every field (lcd_handles_, dpi_framebuffer_ + bytes, display_driver_, display_controller_), and store-releases it true as its final step; flush(), write_lcd_lines() and on_display_rotation() load-acquire it and no-op until it is true, so every plain write happens-before any read that observes the gate open. The init-path rotation call runs after the release store, and a rotation event that arrives while the gate is closed is harmless: initialize_lcd() re-applies the current LVGL rotation once the driver is up. Also restructure on_display_rotation()'s Kconfig-disabled early-return into #if/#else/#endif, fixing the cppcheck unreachableCode finding from static analysis. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

- flush(): rotate a local copy of the LVGL area instead of mutating the const area pointer via const_cast (lv_display_rotate_area on a copy) - Tie the direct-to-framebuffer PPA rotation to the single-framebuffer panel configuration: name the num_fbs value (kNumDpiFramebuffers) and static_assert it is 1 where the framebuffer pointer is cached, since with multiple DPI framebuffers the cached pointer could target a non-scanned buffer - Replace the hard-coded 128-byte cache-line alignment with the value the PPA driver itself validates against, queried via esp_cache_get_alignment(MALLOC_CAP_SPIRAM | MALLOC_CAP_DMA) (used for both the framebuffer alignment check and the scratch-buffer allocation); add esp_mm to the component requirements for the header Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

components/m5stack-tab5/src/video.cpp:498

Publishing true here exposes the panel before its initial MADCTL state is applied below. In the supported reversed-init order, an already-running LVGL thread can flush in this window; with experimental 180° rotation, panel_handles_rotation() skips PPA even though the panel is still in its old scan direction. Apply the initial panel rotation before opening the gate (using an init-only helper that does not require the gate).

  lcd_initialized_.store(true, std::memory_order_release);

components/m5stack-tab5/src/video.cpp:154

Clearing an atomic flag does not wait for readers that already observed the previous true. A concurrent flush() or write_lcd_lines() can pass its acquire-load, then race the re-init writes to display_driver_, display_controller_, or framebuffer state despite the stated guarantee. Either prohibit re-initialization while readers run, or use reader/writer synchronization that waits for in-flight operations before rebuilding this state.

  lcd_initialized_.store(false, std::memory_order_release);

…ublication-gate lifecycle - Skip esp_lcd_dpi_panel_enable_dma2d() on the ILI9881 variant: the repo documents (esp32-p4-function-ev-board, esp32-p4-nano) that the DMA2D draw_bitmap path corrupts RGB565 channel order on ILI9881C-family DSI panels; keep it for the ST7121/ST7123 TDDI variants (no corruption reported there, the underrun fix targets them, and M5Stack's own demo ships DMA2D-enabled on mostly-ST71xx units). Update the stale cross-reference in the function-ev-board note. - Apply the initial panel rotation BEFORE store-releasing lcd_initialized_: with the reversed init order an already-running LVGL thread could flush in the window between the gate opening and the MADCTL apply, skipping PPA rotation with the panel still in its old scan direction. The init path now uses a gate-free apply_panel_rotation() helper (same thread as the state writes) and the gate opens last. - Refuse LCD re-initialization once the gate has opened: clearing the atomic would not wait for readers that already observed true, so a concurrent flush()/write_lcd_lines() could race the re-init writes; the gate is now monotonic (false -> true, never back). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

finger563 · 2026-08-29T20:00:22Z

Addressed the two suppressed comments from the latest Copilot review in b85dd72 (same commit as the DMA2D gating fix).

1. video.cpp:498 — gate published before the initial MADCTL apply

Valid: in the reversed init order (initialize_display() first), an already-running LVGL thread could flush in the window between lcd_initialized_.store(true) and the initial on_display_rotation() call that followed it — panel_handles_rotation() would skip PPA rotation while the panel's MADCTL still held its old scan direction.

Fixed by restructuring so the gate opens last:

The MADCTL/rotation core moved into a gate-free private helper, apply_panel_rotation(). on_display_rotation() (the LVGL rotation callback) is now just the lcd_initialized_ acquire-load check followed by the helper.
initialize_lcd() calls apply_panel_rotation() directly before the store-release. This bypasses the gate, which is safe: it runs on the same init thread that just wrote display_driver_/display_controller_ (no synchronization needed against itself), and any concurrent flush() still no-ops on the still-closed gate. So by the time any flush can observe the gate open, the panel scan direction already matches panel_handles_rotation()'s decision.
The per-init-order safety analysis at the call site (documented order: display_ null, nothing can race, redundant-but-harmless vs the espp::Display ctor's synchronous rotation event; reversed order: MADCTL rides the DSI command channel, arbitrated in hardware against the video stream) was kept and updated for the new ordering.

2. video.cpp:154 — clearing the gate does not wait for in-flight readers

Also valid: lcd_initialized_.store(false) on re-entry cannot un-publish state from a flush()/write_lcd_lines() that already passed its acquire-load of true; rebuilding lcd_handles_/display_driver_/the framebuffer pair underneath such a reader would be a race despite the gate.

Of the two options offered, I took "prohibit re-initialization": initialize_lcd() now refuses to run once the gate has opened (warn + early-return true), rather than adding reader/writer synchronization (e.g. an rwlock or RCU-style quiescence wait) to the per-frame flush hot path for a re-init this BSP never performs — the panel hardware is fixed at boot, and no espp BSP supports LCD re-init. Consequences, now documented on the flag:

The gate is monotonic: false -> true, never back. The store(false) is gone; on entry the gate is provably closed (first call, or retry after a failed attempt that never reached the final store-release), so no reader can have observed true while the state is being built — retry-after-failure stays supported.
The stale "clears this flag on entry" / "mid re-init" wording in the header and flush() comments was updated to match.

Both CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION states build cleanly (default off, and =y).

🤖 Generated with Claude Code

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

…c Kconfig cache-line constant The PPA output-buffer alignment was queried at runtime via esp_cache_get_alignment(), which is only declared in the private esp_private/esp_cache_private.h header and can move or break across IDF releases. On the ESP32-P4 that call just reads back the L2 cache line size that cpu_start.c programs into the cache HAL from CONFIG_CACHE_L2_CACHE_LINE_SIZE (64 or 128), so use that public compile-time constant directly (with a conservative 128-byte fallback if the symbol is ever absent). This also removes the component's only use of esp_mm, so drop the esp_mm REQUIRES again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

components/m5stack-tab5/src/video.cpp:1

The rotation path is hard-coded to PPA_SRM_COLOR_MODE_RGB565 (and elsewhere the scratch-buffer sizing uses sizeof(uint16_t)), while initialize_lcd() configures dpi_cfg.pixel_format via #if/#else. If this component can be built in a non-RGB565 mode (e.g., RGB888), these PPA operations and buffer sizing will produce incorrect results or memory corruption. Suggested fix: either enforce RGB565 for Tab5 at compile time (assert/panic early if not), or branch the PPA + buffer-size logic based on the active pixel format / sizeof(Pixel) and use the correct PPA color mode.

#include "m5stack-tab5.hpp"

The static-analysis CI flagged the new cache-line alignment checks (video.cpp:514): cppcheck's --force explores a preprocessor configuration where CONFIG_CACHE_L2_CACHE_LINE_SIZE folds to 1, trivializing the modulo checks (knownConditionTrueFalse/moduloofone); on real configurations the constant is 64 or 128 and both checks are meaningful. Also suppressed the flush()-path knownConditionTrueFalse where panel_handles_rotation() is a compile-time false in the (default) HW-rotation-disabled configuration. Inline suppressions with justifications; both build configurations still compile clean and local cppcheck reports no findings at either site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… video path Address the remaining PR #738 review feedback on video.cpp: - Reset dpi_framebuffer_/dpi_framebuffer_bytes_ at the start of every initialize_lcd() attempt: a retry after a failed attempt that then cannot re-cache the framebuffer (query or alignment failure) could otherwise open the publication gate with a stale pointer from the previous attempt still cached, and flush()'s direct-to-framebuffer PPA path would write through it. - Make the kNumDpiFramebuffers commentary match the static_assert: the single-framebuffer assumption is an intentional compile-time guard (raising the count requires teaching the PPA path to track the active framebuffer), not a runtime fallback as the old comment claimed. - Enforce the RGB565-only video path at compile time: the DPI panel config, the PPA rotation color modes, and the buffer sizing all hard-code RGB565, so static_assert LV_COLOR_DEPTH == 16 (and a 2-byte Pixel) instead of corrupting output if LVGL were configured with a different color depth, and size the rotation scratch buffer with sizeof(Pixel) rather than a bare sizeof(uint16_t). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

finger563 · 2026-08-30T03:35:53Z

Addressing Copilot's suppressed comment (video.cpp: the PPA rotation hard-codes PPA_SRM_COLOR_MODE_RGB565 and the scratch-buffer sizing used sizeof(uint16_t) while the DPI pixel format is configured separately — a non-RGB565 build would corrupt output or overrun buffers):

Fixed in 2d69b61, taking the suggested enforce-at-compile-time option. The Tab5 video path is RGB565-only by design (the DPI panel config, both PPA SRM color modes, and all buffer sizing assume 2-byte pixels), so video.cpp now carries a file-level static_assert(LV_COLOR_DEPTH == 16 && sizeof(M5StackTab5::Pixel) == sizeof(uint16_t), ...) that refuses to build any other LVGL color depth instead of failing at runtime, with a comment noting that supporting another depth means plumbing the active format through the DPI config, the PPA color modes, and the buffer sizing together. The rotation scratch-buffer sizing was also switched from the bare sizeof(uint16_t) to sizeof(Pixel) so it stays tied to the asserted pixel type. Branching the PPA path on the runtime lv_display_get_color_format() was considered and rejected: no Tab5 configuration can produce a non-RGB565 LVGL buffer today, so it would be dead, untestable code.

Both example flag states (CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION off and on) build cleanly with the change.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

… config site, Pixel-typed buffer Round-8 review fixes + static-analysis repair: - The rotation scratch buffer is allocated with MALLOC_CAP_SPIRAM | MALLOC_CAP_DMA - it is a PPA (DMA) output target and these are exactly the caps kPpaOutBufferAlignment is derived for (was SPIRAM|8BIT). - The DPI pixel-format selection is consolidated to ONE shared site for all three controller variants, with a comment noting the preprocessor branch only picks IDF-version field names for the single supported RGB565 format (the file-level static_assert already enforces RGB565-only). - third_buffer is typed as M5StackTab5::Pixel* to match the sizeof(Pixel) sizing introduced earlier. - Removed the now-stale knownConditionTrueFalse inline suppression on the flush() rotation gate: CI's cppcheck no longer emits that finding there, so the suppression itself failed the static-analysis run as unmatchedSuppression (cppcheck version divergence; same repair as the coredump component). Both Kconfig states build clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

components/m5stack-tab5/Kconfig:32

This adds a user-facing opt-in behavior, but neither the component README nor the example README documents the option, its default, or the known corruption risk. The repository guidance requires both READMEs to be updated when an existing component's behavior changes; please add the same enablement and warning information there.

  config M5STACK_TAB5_ST7121_HW_ROTATION
    bool "ST7121: apply 0/180 rotation in the panel (EXPERIMENTAL)"
    default n

…tial panel rotation initialize_lcd()'s init-path apply_panel_rotation() called lv_display_get_rotation() from the init thread when display_ already existed (reversed init order) - reading LVGL state without the app's LVGL lock while the GUI task may be running (LV_USE_OS == LV_OS_NONE). Use the configured initial 'rotation' instead; runtime rotation changes are still applied by on_display_rotation() (the LVGL rotation callback, on the GUI task). Both Kconfig states build clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

components/m5stack-tab5/src/video.cpp:794

With the experimental option enabled, this drops every rotation event that occurs after initialize_display() but before the LCD gate opens. initialize_lcd() then applies the compile-time initial rotation, not the current/pending value; for example, if LVGL changes to 180° during the documented reversed-init window, the panel is programmed to 0°, while the first flush observes 180° and panel_handles_rotation() skips PPA rotation. Preserve pending rotations and synchronize the final apply/publication step so an event cannot be lost between applying MADCTL and opening the gate.

  if (!lcd_initialized_.load(std::memory_order_acquire)) {
    return;
  }

…e global LVGL default flush() and rotated_display_width()/height() read the rotation from lv_display_get_default() rather than the display actually being used (the flush callback's disp argument / the BSP's own display). Correct in a single-display setup but wrong if another LVGL display is the default; matches the fix already applied to the sibling P4 BSPs. Builds clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…s single-flight-safe With the IDF6 DMA2D hook, esp_lcd_panel_draw_bitmap() is asynchronous and single-flight (a second call while one is in flight returns ESP_ERR_INVALID_STATE). Both flush() and the public cross-thread write_lcd_lines() issued draws without serialization or checking the return, so a concurrent direct write could make an LVGL flush's draw fail with no on_color_trans_done -> LVGL waits forever; and a direct write's completion could be taken for an LVGL flush completion. Now every panel draw goes through draw_and_wait() under panel_op_mutex_: the ISR only gives a completion semaphore (no longer signals LVGL); each draw holds the mutex across the draw and its completion wait (only one transfer in flight); draw_and_wait() checks the return and skips the wait on failure, and flush() ALWAYS calls lv_display_flush_ready() afterwards so a failed draw never hangs LVGL (frame dropped); the PPA direct-to-FB path takes the same mutex around its framebuffer write. Both Kconfig states build clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI lite review requested due to automatic review settings August 28, 2026 16:18

Copilot started reviewing on behalf of finger563 August 28, 2026 16:34 View session

Copilot AI reviewed Aug 28, 2026

View reviewed changes

Comment thread components/m5stack-tab5/src/video.cpp

Comment thread components/m5stack-tab5/src/video.cpp

Comment thread components/m5stack-tab5/src/video.cpp

finger563 and others added 2 commits August 28, 2026 14:35

finger563 requested a balanced review from Copilot August 28, 2026 21:02

Copilot AI reviewed Aug 28, 2026

View reviewed changes

Comment thread components/m5stack-tab5/Kconfig

Comment thread components/m5stack-tab5/src/video.cpp Outdated

finger563 changed the title ~~fix(m5stack-tab5): use ST7121 driver (MADCTL) rotation for 0/180 instead of PPA/software rotation~~ fix(m5stack-tab5): rotation audit, experimental opt-in ST7121 panel rotation (default off), and DSI underrun fixes Aug 28, 2026

Copilot started reviewing on behalf of finger563 August 28, 2026 21:43 View session

finger563 requested a balanced review from Copilot August 29, 2026 00:47

Copilot started reviewing on behalf of finger563 August 29, 2026 00:48 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

Comment thread components/m5stack-tab5/src/video.cpp

finger563 requested a balanced review from Copilot August 29, 2026 02:52

Copilot AI reviewed Aug 29, 2026

View reviewed changes

Copilot started reviewing on behalf of finger563 August 29, 2026 03:50 View session

finger563 requested a balanced review from Copilot August 29, 2026 14:30

Copilot started reviewing on behalf of finger563 August 29, 2026 14:30 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

Comment thread components/m5stack-tab5/src/video.cpp Outdated

finger563 requested a balanced review from Copilot August 29, 2026 20:11

Copilot AI reviewed Aug 29, 2026

View reviewed changes

Comment thread components/m5stack-tab5/src/video.cpp Outdated

Comment thread components/m5stack-tab5/src/video.cpp Outdated

Comment thread components/m5stack-tab5/CMakeLists.txt Outdated

Copilot started reviewing on behalf of finger563 August 29, 2026 22:41 View session

finger563 requested a balanced review from Copilot August 30, 2026 01:30

Copilot AI reviewed Aug 30, 2026

View reviewed changes

Comment thread components/m5stack-tab5/src/video.cpp

Comment thread components/m5stack-tab5/src/video.cpp

Copilot started reviewing on behalf of finger563 August 30, 2026 03:24 View session

finger563 requested a balanced review from Copilot August 30, 2026 04:16

Copilot AI reviewed Aug 30, 2026

View reviewed changes

Copilot stopped reviewing on behalf of finger563 due to an error August 30, 2026 06:36
Failed to download runtime after multiple attempts.

finger563 requested a balanced review from Copilot August 31, 2026 00:44

Copilot started reviewing on behalf of finger563 August 31, 2026 00:45 View session

Copilot AI reviewed Aug 31, 2026

View reviewed changes

Comment thread components/m5stack-tab5/src/video.cpp Outdated

finger563 requested a balanced review from Copilot August 31, 2026 14:08

Copilot started reviewing on behalf of finger563 August 31, 2026 14:08 View session

Copilot AI reviewed Aug 31, 2026

View reviewed changes

Comment thread components/m5stack-tab5/src/video.cpp

finger563 and others added 2 commits August 31, 2026 09:18

Uh oh!

Conversation

finger563 commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What landed

Rotation audit + plumbing (default behavior unchanged)

DSI underrun / streaking fixes (active by default)

Hardware testing

Testing

Uh oh!

github-actions Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅Static analysis result - no issues found! ✅

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

finger563 commented Aug 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

finger563 commented Aug 28, 2026

Display streaking: root cause + fixes (513f565)

If streaks persist, please try in this order

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

finger563 commented Aug 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

finger563 commented Aug 30, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

finger563 commented Aug 28, 2026 •

edited

Loading

github-actions Bot commented Aug 28, 2026 •

edited

Loading

Display streaking: root cause + fixes (`513f565`)