Skip to content

Chop down tft-related loooong ifdefs chains#10803

Merged
jp-bennett merged 4 commits into
developfrom
remove-tft-ifdefs
Jun 30, 2026
Merged

Chop down tft-related loooong ifdefs chains#10803
jp-bennett merged 4 commits into
developfrom
remove-tft-ifdefs

Conversation

@jp-bennett

@jp-bennett jp-bennett commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator

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

    • Expanded display support across more device variants, including SPI TFT and e-ink configurations.
    • Improved screen handling so the right display layout, orientation, and font sizing are chosen more consistently.
    • Updated icon and UI rendering to better match supported display types.
  • Bug Fixes

    • Fixed screen initialization paths on supported hardware so displays are detected and set up more reliably.
    • Improved default behavior for devices with built-in screens, reducing mismatches in startup and pairing flows.

@jp-bennett jp-bennett requested a review from HarukiToreda June 28, 2026 05:08
@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

⚡ Try this PR in the Web Flasher

Flash this PR in the Web Flasher

firmware commit boards expires

Warning

This is an automated, unreviewed CI test build. Back up your device configuration
before flashing, and only flash devices you are able to recover.

Supported boards built by this PR (25)
Device Board Platform
Crowpanel Adv 3.5 TFT elecrow-adv-35-tft esp32-s3
Heltec HT62 heltec-ht62-esp32c3-sx1262 esp32-c3
Heltec Mesh Node 096 heltec-mesh-node-t096 nrf52840
Heltec Mesh Node T1 heltec-mesh-node-t1 nrf52840
Heltec Mesh Node T114 heltec-mesh-node-t114 nrf52840
Heltec V3 heltec-v3 esp32-s3
Heltec V4 heltec-v4 esp32-s3
Raspberry Pi Pico pico rp2040
Raspberry Pi Pico W picow rp2040
RAK WisMesh Tag rak_wismeshtag nrf52840
RAK WisBlock 11200 rak11200 esp32
RAK WisBlock 11310 rak11310 rp2040
RAK3312 rak3312 esp32-s3
RAK WisBlock 4631 rak4631 nrf52840
Seeed Wio Tracker L1 seeed_wio_tracker_L1 nrf52840
Seeed Xiao NRF52840 Kit seeed_xiao_nrf52840_kit nrf52840
Seeed Xiao ESP32-S3 seeed-xiao-s3 esp32-s3
Station G2 station-g2 esp32-s3
Station G3 station-g3 esp32-s3
LILYGO T-Deck t-deck-tft esp32-s3
LILYGO T-Echo t-echo nrf52840
LILYGO T-Echo Plus t-echo-plus nrf52840
LILYGO T-Impulse Plus t-impulse-plus nrf52840
LilyGo T3-C6 tlora-c6 esp32-c6
Seeed SenseCAP T1000-E tracker-t1000-e nrf52840

Build artifacts expire on 2026-07-28. Updated for a8f0460.

@github-actions github-actions Bot added needs-review Needs human review enhancement New feature or request labels Jun 28, 2026
@jp-bennett jp-bennett requested review from Copilot and thebentern June 28, 2026 05:08
@jp-bennett jp-bennett added the cleanup Code cleanup or refactor label Jun 28, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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_TFT to 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 for HAS_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.

Comment thread src/graphics/Screen.cpp
Comment thread src/graphics/Screen.cpp Outdated
@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Firmware Size Report

22 targets | vs develop: 22 increased, net +11,144 (+10.9 KB)

Target Size vs develop
heltec-vision-master-e213-inkhud 2,230,240 📈 +2,880 (+2.8 KB)
t-deck-tft 3,811,184 📈 +512
station-g3 2,266,848 📈 +496
rak11200 1,861,088 📈 +480
station-g2 2,266,832 📈 +480
Show 17 more target(s)
Target Size vs develop
heltec-v3 2,264,544 📈 +464
heltec-v4 2,277,760 📈 +464
rak3312 2,272,704 📈 +464
heltec-ht62-esp32c3-sx1262 2,135,440 📈 +448
seeed-xiao-s3 2,276,592 📈 +448
tlora-c6 2,368,752 📈 +448
t-eth-elite 2,491,920 📈 +432
elecrow-adv-35-tft 3,417,040 📈 +416
pico2w 1,220,932 📈 +416
picow 1,245,240 📈 +360
seeed_xiao_rp2350 768,296 📈 +360
pico2 770,128 📈 +352
pico 783,016 📈 +328
rak11310 805,752 📈 +328
seeed_xiao_rp2040 781,232 📈 +328
wio-e5 238,756 📈 +128
rak3172 186,408 📈 +112

Updated for 2e0f633

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Introduces HAS_SPI_TFT as a unified compile-time capability macro, defined across 27+ board variant headers and one platformio.ini. Replaces long per-driver macro lists in graphics headers (ScreenFonts.h, TFTColorRegions.h, images.h), renderers (DebugRenderer.cpp, UIRenderer.cpp), Screen.cpp, main.cpp, and NodeDB.cpp with simplified checks against HAS_SPI_TFT, USE_EINK, and USE_SPISSD1306.

Changes

HAS_SPI_TFT Macro Consolidation

Layer / File(s) Summary
HAS_SPI_TFT definition across board variants
variants/esp32/chatter2/variant.h, variants/esp32/m5stack_core/variant.h, variants/esp32/tbeam/variant.h, variants/esp32/wiphone/variant.h, variants/esp32c6/m5stack_unitc6l/variant.h, variants/esp32s3/hackaday-communicator/variant.h, variants/esp32s3/heltec_v4/variant.h, variants/esp32s3/heltec_v4_r8/variant.h, variants/esp32s3/heltec_vision_master_t190/variant.h, variants/esp32s3/heltec_wireless_tracker*/variant.h, variants/esp32s3/m5stack_cardputer_adv/variant.h, variants/esp32s3/picomputer-s3/variant.h, variants/esp32s3/seeed-sensecap-indicator/variant.h, variants/esp32s3/t-deck/variant.h, variants/esp32s3/t-watch-s3/variant.h, variants/esp32s3/tlora-pager/variant.h, variants/esp32s3/tracksenger/*/variant.h, variants/esp32s3/unphone/variant.h, variants/nrf52840/heltec_mesh_node_t*/variant.h, variants/nrf52840/heltec_mesh_solar/platformio.ini, variants/nrf52840/rak_wismeshtap/variant.h
Each board variant adds #define HAS_SPI_TFT 1. m5stack_unitc6l also adds DISPLAY_FORCE_SMALL_FONTS and unconditionalizes SSD1306 pin defines outside the USE_SPISSD1306 guard.
Graphics subsystem macro simplification
src/graphics/ScreenFonts.h, src/graphics/TFTColorRegions.h, src/graphics/images.h, src/graphics/draw/DebugRenderer.cpp, src/graphics/draw/UIRenderer.cpp
Font size selection, TFT color-region enablement, large icon bitmap selection, and node-rendering branch guards all replace long per-driver macro enumerations with HAS_SPI_TFT/USE_EINK checks.
Screen init, setup, and NodeDB guards
src/graphics/Screen.cpp, src/main.cpp, src/mesh/NodeDB.cpp
Screen constructor restructures ARCH_PORTDUINO display selection and narrows flipScreenVertically guard to USE_TFTDISPLAY && !ARCH_PORTDUINO. main.cpp screen init and setup guards, and NodeDB::installDefaultConfig() hasScreen branch, switch to `HAS_SPI_TFT

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Poem

🐇 Hopping through headers, one flag to unite,
No more long lists of drivers in sight!
HAS_SPI_TFT hops into each .h,
Replacing the clutter, clearing the way.
The screen blinks to life with a unified cheer—
One macro to rule them all, crystal clear! ✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is only one sentence and omits the required template sections and attestations. Replace the placeholder text with the repo template, including testing/attestation checkboxes and any hardware/regression notes.
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the main change: simplifying long TFT-related ifdef chains.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch remove-tft-ifdefs

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 win

Make 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 leave HAS_SPI_TFT unset 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 win

Keep Portduino on a single icon-size path.

Line 100 and Line 123 already treat ARCH_PORTDUINO as a large-icon target, and src/graphics/images.h compiles imgSFL1/imgSFL2 under that same capability. This branch alone falls back to imgSF, so the prefix next to screen->ourId changes 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8345c2b and c331d51.

📒 Files selected for processing (34)
  • src/graphics/Screen.cpp
  • src/graphics/ScreenFonts.h
  • src/graphics/TFTColorRegions.h
  • src/graphics/draw/DebugRenderer.cpp
  • src/graphics/draw/UIRenderer.cpp
  • src/graphics/images.h
  • src/main.cpp
  • src/mesh/NodeDB.cpp
  • variants/esp32/chatter2/variant.h
  • variants/esp32/m5stack_core/variant.h
  • variants/esp32/tbeam/variant.h
  • variants/esp32/wiphone/variant.h
  • variants/esp32c6/m5stack_unitc6l/variant.h
  • variants/esp32s3/hackaday-communicator/variant.h
  • variants/esp32s3/heltec_v4/variant.h
  • variants/esp32s3/heltec_v4_r8/variant.h
  • variants/esp32s3/heltec_vision_master_t190/variant.h
  • variants/esp32s3/heltec_wireless_tracker/variant.h
  • variants/esp32s3/heltec_wireless_tracker_V1_0/variant.h
  • variants/esp32s3/heltec_wireless_tracker_v2/variant.h
  • variants/esp32s3/m5stack_cardputer_adv/variant.h
  • variants/esp32s3/picomputer-s3/variant.h
  • variants/esp32s3/seeed-sensecap-indicator/variant.h
  • variants/esp32s3/t-deck/variant.h
  • variants/esp32s3/t-watch-s3/variant.h
  • variants/esp32s3/tlora-pager/variant.h
  • variants/esp32s3/tracksenger/internal/variant.h
  • variants/esp32s3/tracksenger/lcd/variant.h
  • variants/esp32s3/unphone/variant.h
  • variants/nrf52840/heltec_mesh_node_t096/variant.h
  • variants/nrf52840/heltec_mesh_node_t1/variant.h
  • variants/nrf52840/heltec_mesh_node_t114/variant.h
  • variants/nrf52840/heltec_mesh_solar/platformio.ini
  • variants/nrf52840/rak_wismeshtap/variant.h

Comment thread src/graphics/draw/UIRenderer.cpp
Comment thread src/graphics/Screen.cpp
Comment thread variants/nrf52840/heltec_mesh_solar/platformio.ini
@jp-bennett jp-bennett merged commit fd8ba4f into develop Jun 30, 2026
93 checks passed
@jp-bennett jp-bennett deleted the remove-tft-ifdefs branch June 30, 2026 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup Code cleanup or refactor enhancement New feature or request needs-review Needs human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants