Skip to content

Conversation

@jniebuhr
Copy link
Owner

@jniebuhr jniebuhr commented Nov 5, 2025

Summary by CodeRabbit

  • Refactor

    • Reworked display infrastructure to support multiple AMOLED variants with configurable hardware profiles and improved detection/fallback between drivers.
    • Migrated panel implementation to use configuration-driven dimensions and initialization.
  • New Features

    • Added an AMOLED driver with SD card support, brightness control, touch handling, rotation, and sleep/wakeup behavior.
    • UI now auto-applies the AMOLED black theme when an AMOLED driver is active.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 5, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Replaces the LilyGoTDisplayDriver and LilyGo_TDisplayPanel with a new AmoledDisplayDriver and Amoled_DisplayPanel backed by an AmoledHwConfig-driven pin/profile system; legacy per-board pin_config.h for LilyGo T-Display was removed and DefaultUI/Controller now prefer the Amoled driver.

Changes

Cohort / File(s) Summary
Driver removal
src/display/drivers/LilyGoTDisplayDriver.h, src/display/drivers/LilyGoTDisplayDriver.cpp
Deleted the LilyGoTDisplayDriver class, singleton, I2C detection helpers, compatibility and init logic.
New driver
src/display/drivers/AmoledDisplayDriver.h, src/display/drivers/AmoledDisplayDriver.cpp
Added AmoledDisplayDriver (singleton) with isCompatible(), init(), setBrightness(), SD card support, hw-config compatibility tests and I2C device detection.
Panel refactor/rename
src/display/drivers/AmoledDisplay/Amoled_DisplayPanel.h, src/display/drivers/AmoledDisplay/Amoled_DisplayPanel.cpp
Renamed LilyGo_TDisplayPanel -> Amoled_DisplayPanel; constructor now takes AmoledHwConfig; many APIs and enums renamed; panel logic (touch, display, SD, brightness, wake/sleep, rotation, battery) updated to use hwConfig.
Hardware profiles
src/display/drivers/AmoledDisplay/pin_config.h
New AmoledHwConfig struct and constexpr profiles (LILYGO_T_DISPLAY_S3_DS_HW_CONFIG, WAVESHARE_S3_AMOLED_HW_CONFIG) plus device address constants.
Removed legacy pin config
src/display/drivers/LilyGo-T-Display-S3-DS/pin_config.h
Deleted legacy per-board pin macros and device address defines.
Controller & UI updates
src/display/core/Controller.cpp, src/display/ui/default/DefaultUI.cpp
Controller and DefaultUI now prefer/use AmoledDisplayDriver::getInstance() and include AmoledDisplayDriver header instead of LilyGoTDisplayDriver.

Sequence Diagram(s)

sequenceDiagram
    participant App as Application
    participant Ctrl as Controller
    participant AMD as AmoledDisplayDriver
    participant Panel as Amoled_DisplayPanel
    participant HW as Hardware (I2C/SPI)

    App->>Ctrl: setupPanel()
    Ctrl->>AMD: getInstance()
    AMD->>AMD: isCompatible()
    AMD->>HW: probe profiles (I2C: PCF8563, touch)
    HW-->>AMD: probe results
    alt compatible
        AMD->>AMD: select hwConfig
        AMD->>Panel: new Amoled_DisplayPanel(hwConfig)
        AMD->>Panel: begin()
        Panel->>HW: init display, touch, SD via hwConfig pins
        HW-->>Panel: ready
        Panel-->>AMD: success
        AMD->>Panel: setBrightness(16)
        AMD-->>Ctrl: init complete
    else incompatible
        AMD-->>Ctrl: not compatible
    end
    Ctrl-->>App: panel status
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Areas needing extra attention:
    • Consistency of renamed enums/types and their uses across callers.
    • Correct initialization and use of AmoledHwConfig fields (pins, offsets, dimensions).
    • I2C/device detection and retry logic in the new driver (testHw / detectI2CDevice).
    • Removal of legacy pin_config.h: ensure no remaining references to deleted macros.

Possibly related PRs

Poem

🐰
New configs snug in pocket fur,
Amoled wakes with gentle purr,
Pins now travel in a tidy map,
Old driver hopped away — clap clap clap! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main objective: adding support for Waveshare 1.75 AMOLED display, which is the primary driver through extensive refactoring of display driver architecture.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7371124 and 89274e7.

📒 Files selected for processing (4)
  • src/display/drivers/AmoledDisplay/Amoled_DisplayPanel.cpp (1 hunks)
  • src/display/drivers/AmoledDisplay/Amoled_DisplayPanel.h (5 hunks)
  • src/display/drivers/AmoledDisplay/pin_config.h (1 hunks)
  • src/display/drivers/AmoledDisplayDriver.h (1 hunks)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/display/drivers/AmoledDisplayDriver.h (1)

1-32: Update the include guard to match this header.

The guard name still references LILYGOTDISPLAYDRIVER_H, which is misleading and risks collisions if any legacy header with the same guard remains. Please rename the guard to something like AMOLEDDISPLAYDRIVER_H so it accurately reflects this file.

-#ifndef LILYGOTDISPLAYDRIVER_H
-#define LILYGOTDISPLAYDRIVER_H
+#ifndef AMOLEDDISPLAYDRIVER_H
+#define AMOLEDDISPLAYDRIVER_H
 ...
-#endif // LILYGOTDISPLAYDRIVER_H
+#endif // AMOLEDDISPLAYDRIVER_H
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 462db4d and 7371124.

📒 Files selected for processing (10)
  • src/display/core/Controller.cpp (2 hunks)
  • src/display/drivers/AmoledDisplay/Amoled_DisplayPanel.cpp (9 hunks)
  • src/display/drivers/AmoledDisplay/Amoled_DisplayPanel.h (4 hunks)
  • src/display/drivers/AmoledDisplay/pin_config.h (1 hunks)
  • src/display/drivers/AmoledDisplayDriver.cpp (1 hunks)
  • src/display/drivers/AmoledDisplayDriver.h (1 hunks)
  • src/display/drivers/LilyGo-T-Display-S3-DS/pin_config.h (0 hunks)
  • src/display/drivers/LilyGoTDisplayDriver.cpp (0 hunks)
  • src/display/drivers/LilyGoTDisplayDriver.h (0 hunks)
  • src/display/ui/default/DefaultUI.cpp (2 hunks)
💤 Files with no reviewable changes (3)
  • src/display/drivers/LilyGoTDisplayDriver.cpp
  • src/display/drivers/LilyGo-T-Display-S3-DS/pin_config.h
  • src/display/drivers/LilyGoTDisplayDriver.h
🧰 Additional context used
🧬 Code graph analysis (4)
src/display/drivers/AmoledDisplayDriver.cpp (1)
src/display/drivers/AmoledDisplayDriver.h (3)
  • hwConfig (27-27)
  • init (9-10)
  • supportsSDCard (11-19)
src/display/drivers/AmoledDisplay/Amoled_DisplayPanel.cpp (3)
src/display/drivers/AmoledDisplay/Amoled_DisplayPanel.h (3)
  • Amoled_DisplayPanel (50-112)
  • width (79-79)
  • height (81-81)
src/display/drivers/AmoledDisplayDriver.h (1)
  • hwConfig (27-27)
src/display/drivers/AmoledDisplay/CO5300.cpp (2)
  • setRotation (7-26)
  • setRotation (7-7)
src/display/drivers/AmoledDisplay/Amoled_DisplayPanel.h (3)
src/display/drivers/AmoledDisplay/Amoled_DisplayPanel.cpp (8)
  • Amoled_DisplayPanel (6-10)
  • Amoled_DisplayPanel (12-29)
  • begin (31-38)
  • begin (31-31)
  • getModel (77-77)
  • getModel (77-77)
  • initDisplay (260-296)
  • initDisplay (260-260)
src/display/drivers/common/Display.h (1)
  • Display (5-18)
src/display/drivers/AmoledDisplayDriver.h (1)
  • hwConfig (27-27)
src/display/drivers/AmoledDisplayDriver.h (3)
src/display/drivers/Driver.h (1)
  • Driver (5-14)
src/display/drivers/AmoledDisplayDriver.cpp (10)
  • isCompatible (24-36)
  • isCompatible (24-24)
  • init (38-53)
  • init (38-38)
  • supportsSDCard (55-55)
  • supportsSDCard (55-55)
  • installSDCard (57-57)
  • installSDCard (57-57)
  • testHw (59-73)
  • testHw (59-59)
src/display/drivers/AmoledDisplay/Amoled_DisplayPanel.cpp (2)
  • setBrightness (55-73)
  • setBrightness (55-55)
🪛 Clang (14.0.6)
src/display/drivers/AmoledDisplay/pin_config.h

[error] 10-10: unknown type name 'int8_t'

(clang-diagnostic-error)


[error] 11-11: unknown type name 'int8_t'

(clang-diagnostic-error)


[error] 12-12: unknown type name 'int8_t'

(clang-diagnostic-error)


[error] 13-13: unknown type name 'int8_t'

(clang-diagnostic-error)


[error] 14-14: unknown type name 'int8_t'

(clang-diagnostic-error)


[error] 15-15: unknown type name 'int8_t'

(clang-diagnostic-error)


[error] 16-16: unknown type name 'int8_t'

(clang-diagnostic-error)


[error] 17-17: unknown type name 'uint16_t'

(clang-diagnostic-error)


[error] 18-18: unknown type name 'uint16_t'

(clang-diagnostic-error)


[error] 19-19: unknown type name 'int8_t'

(clang-diagnostic-error)


[error] 20-20: unknown type name 'int8_t'

(clang-diagnostic-error)


[error] 21-21: unknown type name 'int8_t'

(clang-diagnostic-error)


[error] 22-22: unknown type name 'int8_t'

(clang-diagnostic-error)


[error] 23-23: unknown type name 'int8_t'

(clang-diagnostic-error)


[error] 24-24: unknown type name 'int8_t'

(clang-diagnostic-error)


[error] 25-25: unknown type name 'int8_t'

(clang-diagnostic-error)


[error] 26-26: unknown type name 'int8_t'

(clang-diagnostic-error)


[error] 27-27: unknown type name 'int8_t'

(clang-diagnostic-error)


[error] 28-28: unknown type name 'int8_t'

(clang-diagnostic-error)

src/display/drivers/AmoledDisplayDriver.h

[error] 4-4: 'display/drivers/AmoledDisplay/Amoled_DisplayPanel.h' file not found

(clang-diagnostic-error)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: deploy
  • GitHub Check: test
  • GitHub Check: test

@jniebuhr jniebuhr merged commit ecb0937 into master Nov 6, 2025
4 of 5 checks passed
@jniebuhr jniebuhr deleted the feature/waveshare-amoled branch November 6, 2025 19:21
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 6, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants