Chop down tft-related loooong ifdefs chains#10803
Conversation
⚡ Try this PR in the Web FlasherWarning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (25)
Build artifacts expire on 2026-07-28. Updated for |
There was a problem hiding this comment.
Pull request overview
This PR introduces a single compile-time feature flag (HAS_SPI_TFT) to represent “this variant uses an SPI-connected TFT,” and refactors several long preprocessor condition chains across the firmware to use that flag. The intent is to make adding new TFT-capable devices primarily a variant-folder-only change.
Changes:
- Add
HAS_SPI_TFTto TFT-capable variants (or via build flags) to standardize feature detection. - Replace multiple TFT-driver-specific
#if defined(...)chains in core init and graphics code with checks forHAS_SPI_TFT. - Simplify TFT-related feature gating for fonts/images/color regions using the new macro.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| variants/nrf52840/rak_wismeshtap/variant.h | Adds HAS_SPI_TFT to mark the variant as SPI TFT-capable. |
| variants/nrf52840/heltec_mesh_solar/platformio.ini | Defines HAS_SPI_TFT via build flags for this TFT variant. |
| variants/nrf52840/heltec_mesh_node_t114/variant.h | Adds HAS_SPI_TFT for the ST7789-based TFT variant. |
| variants/nrf52840/heltec_mesh_node_t1/variant.h | Adds HAS_SPI_TFT for the ST7735-based TFT variant. |
| variants/nrf52840/heltec_mesh_node_t096/variant.h | Adds HAS_SPI_TFT for the ST7735-based TFT variant. |
| variants/esp32s3/unphone/variant.h | Adds HAS_SPI_TFT for HX8357 TFT support. |
| variants/esp32s3/tracksenger/lcd/variant.h | Adds HAS_SPI_TFT for the LCD (TFT) variant configuration. |
| variants/esp32s3/tracksenger/internal/variant.h | Adds HAS_SPI_TFT for the internal TFT configuration. |
| variants/esp32s3/tlora-pager/variant.h | Adds HAS_SPI_TFT for ST7796 TFT support. |
| variants/esp32s3/t-watch-s3/variant.h | Adds HAS_SPI_TFT for ST7789 TFT support. |
| variants/esp32s3/t-deck/variant.h | Adds HAS_SPI_TFT for ST7789 TFT support. |
| variants/esp32s3/seeed-sensecap-indicator/variant.h | Adds HAS_SPI_TFT for the ST7701 TFT configuration. |
| variants/esp32s3/picomputer-s3/variant.h | Adds HAS_SPI_TFT for ST7789 TFT support. |
| variants/esp32s3/m5stack_cardputer_adv/variant.h | Adds HAS_SPI_TFT for ST7789 TFT support. |
| variants/esp32s3/heltec_wireless_tracker/variant.h | Adds HAS_SPI_TFT for ST7735 TFT support. |
| variants/esp32s3/heltec_wireless_tracker_v2/variant.h | Adds HAS_SPI_TFT for ST7735 TFT support. |
| variants/esp32s3/heltec_wireless_tracker_V1_0/variant.h | Adds HAS_SPI_TFT for ST7735 TFT support. |
| variants/esp32s3/heltec_vision_master_t190/variant.h | Adds HAS_SPI_TFT for ST7789 TFT support. |
| variants/esp32s3/heltec_v4/variant.h | Adds HAS_SPI_TFT when HAS_TFT is enabled for this variant. |
| variants/esp32s3/heltec_v4_r8/variant.h | Adds HAS_SPI_TFT when HAS_TFT is enabled for this variant. |
| variants/esp32s3/hackaday-communicator/variant.h | Adds HAS_SPI_TFT to align the variant with the new gating logic. |
| variants/esp32c6/m5stack_unitc6l/variant.h | Simplifies SPI SSD1306 small-font forcing by removing a redundant #ifdef. |
| variants/esp32/wiphone/variant.h | Adds HAS_SPI_TFT for the TFT configuration in this variant. |
| variants/esp32/tbeam/variant.h | Adds HAS_SPI_TFT for the optional ST7796 display shield configuration. |
| variants/esp32/m5stack_core/variant.h | Adds HAS_SPI_TFT for the ILI9341 TFT configuration. |
| variants/esp32/chatter2/variant.h | Adds HAS_SPI_TFT for the ST7735 TFT configuration. |
| src/mesh/NodeDB.cpp | Replaces a long TFT-driver #if defined(...) chain with HAS_SPI_TFT for screen-related defaults. |
| src/main.cpp | Simplifies screen allocation/setup gating to use HAS_SPI_TFT (plus E-Ink/SPI SSD1306). |
| src/graphics/TFTColorRegions.h | Uses HAS_SPI_TFT to enable TFT color region support instead of enumerating drivers. |
| src/graphics/ScreenFonts.h | Uses HAS_SPI_TFT to select larger font sizes where appropriate. |
| src/graphics/Screen.cpp | Refactors TFTDisplay selection and setup behavior to key off USE_TFTDISPLAY. |
| src/graphics/images.h | Uses HAS_SPI_TFT to gate larger bitmap assets (when not forcing small fonts). |
| src/graphics/draw/UIRenderer.cpp | Uses HAS_SPI_TFT for selecting higher-resolution UI assets/layout. |
| src/graphics/draw/DebugRenderer.cpp | Uses HAS_SPI_TFT to gate higher-resolution debug icon assets/layout. |
Firmware Size Report22 targets | vs
Show 17 more target(s)
Updated for 2e0f633 |
📝 WalkthroughWalkthroughIntroduces ChangesHAS_SPI_TFT Macro Consolidation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
variants/esp32/wiphone/variant.h (1)
29-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the WiPhone screen capability flags agree.
Line 29 still marks this variant as screenless, but Line 32 now opts it into the new shared TFT capability. With the refactor switching common guards to
HAS_SPI_TFT, this target will compile as screen-capable in some paths and screenless in others. Please pick one source of truth here—either mark WiPhone as screen-backed everywhere or leaveHAS_SPI_TFTunset until the screen path is actually supported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@variants/esp32/wiphone/variant.h` around lines 29 - 37, The WiPhone capability flags are inconsistent because variant.h still declares HAS_SCREEN as disabled while HAS_SPI_TFT and USE_TFTDISPLAY enable the display path. Update the WiPhone variant so the screen capability is represented by one consistent source of truth: either enable the screen-backed flags together if the TFT path is supported, or remove the shared TFT defines and keep the variant screenless. Use the symbols HAS_SCREEN, HAS_SPI_TFT, and USE_TFTDISPLAY to locate and align the configuration.src/graphics/draw/DebugRenderer.cpp (1)
110-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep Portduino on a single icon-size path.
Line 100 and Line 123 already treat
ARCH_PORTDUINOas a large-icon target, andsrc/graphics/images.hcompilesimgSFL1/imgSFL2under that same capability. This branch alone falls back toimgSF, so the prefix next toscreen->ourIdchanges size with heartbeat state.💡 Proposed fix
-#if (defined(USE_EINK) || defined(HAS_SPI_TFT)) && !defined(DISPLAY_FORCE_SMALL_FONTS) +#if (defined(USE_EINK) || defined(HAS_SPI_TFT) || defined(ARCH_PORTDUINO)) && !defined(DISPLAY_FORCE_SMALL_FONTS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graphics/draw/DebugRenderer.cpp` around lines 110 - 118, The DebugRenderer heartbeat prefix uses two different icon paths, and the fallback branch still draws imgSF for ARCH_PORTDUINO while the larger icon path is already used elsewhere. Update the conditional in DebugRenderer.cpp around display->drawFastImage so ARCH_PORTDUINO follows the same large-icon branch as the other supported targets, reusing imgSFL1/imgSFL2 consistently next to screen->ourId instead of switching sizes based on heartbeat state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/graphics/draw/UIRenderer.cpp`:
- Line 720: The conditional in UIRenderer for the user image placement has
drifted from the asset-selection guard in images.h, causing Portduino builds to
use mismatched icon sizing and positioning. Update the guard around the imgUser
drawing path in UIRenderer.cpp to mirror the same ARCH_PORTDUINO-inclusive
condition used by images.h, so the chosen asset and the y-offset fallback stay
aligned. Refer to the imgUser selection logic and the surrounding display-branch
condition in UIRenderer when making the change.
In `@src/graphics/Screen.cpp`:
- Around line 514-526: The Portduino branch in Screen::setupScreen (the
ARCH_PORTDUINO / display initialization block) now skips assigning dispdev when
displaymode is COLOR, which leaves OLEDDisplayUi construction with an
uninitialized device. Update this branch so it exhaustively selects a display
device for both COLOR and non-COLOR cases, using the existing TFTDisplay and
AutoOLEDWire setup paths as appropriate, and ensure dispdev is always set before
any later screen UI initialization.
In `@variants/nrf52840/heltec_mesh_solar/platformio.ini`:
- Line 110: The TFT environment only defines HAS_SPI_TFT, but main.cpp still
uses `#if` HAS_SCREEN to guard screen setup, so the display path won’t activate.
Update the platformio.ini entry for the TFT env to define HAS_SCREEN=1 as well,
or confirm it is inherited from the base environment; check the configuration
around HAS_SPI_TFT and the screen-gated setup in main.cpp.
---
Outside diff comments:
In `@src/graphics/draw/DebugRenderer.cpp`:
- Around line 110-118: The DebugRenderer heartbeat prefix uses two different
icon paths, and the fallback branch still draws imgSF for ARCH_PORTDUINO while
the larger icon path is already used elsewhere. Update the conditional in
DebugRenderer.cpp around display->drawFastImage so ARCH_PORTDUINO follows the
same large-icon branch as the other supported targets, reusing imgSFL1/imgSFL2
consistently next to screen->ourId instead of switching sizes based on heartbeat
state.
In `@variants/esp32/wiphone/variant.h`:
- Around line 29-37: The WiPhone capability flags are inconsistent because
variant.h still declares HAS_SCREEN as disabled while HAS_SPI_TFT and
USE_TFTDISPLAY enable the display path. Update the WiPhone variant so the screen
capability is represented by one consistent source of truth: either enable the
screen-backed flags together if the TFT path is supported, or remove the shared
TFT defines and keep the variant screenless. Use the symbols HAS_SCREEN,
HAS_SPI_TFT, and USE_TFTDISPLAY to locate and align the configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b81803f-a9c5-4220-b00b-06ec313f6307
📒 Files selected for processing (34)
src/graphics/Screen.cppsrc/graphics/ScreenFonts.hsrc/graphics/TFTColorRegions.hsrc/graphics/draw/DebugRenderer.cppsrc/graphics/draw/UIRenderer.cppsrc/graphics/images.hsrc/main.cppsrc/mesh/NodeDB.cppvariants/esp32/chatter2/variant.hvariants/esp32/m5stack_core/variant.hvariants/esp32/tbeam/variant.hvariants/esp32/wiphone/variant.hvariants/esp32c6/m5stack_unitc6l/variant.hvariants/esp32s3/hackaday-communicator/variant.hvariants/esp32s3/heltec_v4/variant.hvariants/esp32s3/heltec_v4_r8/variant.hvariants/esp32s3/heltec_vision_master_t190/variant.hvariants/esp32s3/heltec_wireless_tracker/variant.hvariants/esp32s3/heltec_wireless_tracker_V1_0/variant.hvariants/esp32s3/heltec_wireless_tracker_v2/variant.hvariants/esp32s3/m5stack_cardputer_adv/variant.hvariants/esp32s3/picomputer-s3/variant.hvariants/esp32s3/seeed-sensecap-indicator/variant.hvariants/esp32s3/t-deck/variant.hvariants/esp32s3/t-watch-s3/variant.hvariants/esp32s3/tlora-pager/variant.hvariants/esp32s3/tracksenger/internal/variant.hvariants/esp32s3/tracksenger/lcd/variant.hvariants/esp32s3/unphone/variant.hvariants/nrf52840/heltec_mesh_node_t096/variant.hvariants/nrf52840/heltec_mesh_node_t1/variant.hvariants/nrf52840/heltec_mesh_node_t114/variant.hvariants/nrf52840/heltec_mesh_solar/platformio.inivariants/nrf52840/rak_wismeshtap/variant.h
While this makes the source look a bit better, the real win is that devices can be added, only making changes to the variant folder.
Summary by CodeRabbit
New Features
Bug Fixes