feat: runtime screen rotation for the color UI (opt-in) - #359
Conversation
The six data columns use a fixed 36 px width unless the horizontal resolution exceeds 320, so a 240 px screen asks for 57 + 6*36 = 273 px and the table overflows its panel. Derive the column width from the available space instead.
set(GENERATED_VIEW ${VIEW} "ui_320x240") produces a two-element list as soon
as VIEW is set, and VIEW_320x240 was defined unconditionally, so the CMake
build could only ever produce ui_320x240. Give VIEW a default and derive the
macro from it; behaviour is unchanged when VIEW is unset.
Linking both generated UI trees into one binary requires renaming the secondary tree's 198 colliding globals and republishing its widget pointers into the symbols the C++ view addresses. tools/gen_dual_ui.py derives that glue from the trees themselves and asserts the invariants it depends on: identical objects_t member sets, symmetric global symbols, byte-identical shared assets, and an identical non-asset source inventory. The output is orientation-neutral, so one set of artifacts serves either tree as the primary one. --check regenerates every artifact and byte-compares it, and rejects unexpected files, so the committed glue cannot drift from the trees unnoticed.
The stored value is the quarter-turn count applied on top of the board's compile-time LGFX_ROTATION offset, so value 0 reproduces each board's current behaviour. Even values keep the compile-time layout, odd values need the perpendicular one. Applied at boot only: an LVGL layout tree cannot be rebuilt in place once the view has cached its object pointers. The mapping helpers are pure so they can be tested without touching the loaded state.
Off by default. When enabled, the primary tree supplies every shared asset and the secondary tree is compiled with the generated rename header forced in, so both layouts are linked exactly once. Both build systems read the source inventory from the generator's manifest rather than globbing, and both run --check so stale glue fails the build. The CMake path matters because that is what CI builds: without it the feature would be invisible to CI and effectively untested. X11 is not LGFX, so the new job covers compiling, linking and the state tests, not the panel rotation itself. The link test references a symbol from each tree because a static library would otherwise link green without ever pulling in the secondary one.
The panel is rotated before any layout exists and LVGL is then told the rotated panel's own resolution, rather than the layout's nominal size: substituting the latter would misreport panels larger than the layout, such as 320x480 ones. The view dispatches the secondary tree's ui_init* and publishes the bridge before any objects.* dereference and before apply_hotfix caches pointers.
Adds a settings entry and a dropdown dialog copied from the generated dropdown-panel idiom. The option order is the stored value order, and the labels follow from which layout the build compiled as primary, so an even value always reads as the built-in aspect. The choice is persisted through the existing storeUIConfig path and followed by a reboot request: the value is only read at boot, and the firmware keeps serving the pre-change config until it reloads. A copy is submitted so a failed store cannot leave the new value in memory for an unrelated save to persist, and the dialog stays open unless both submissions succeed. Comparing against the persisted value rather than the active one keeps a saved-but-not-yet-applied change retryable.
|
|
📝 WalkthroughWalkthroughThe change adds dual-layout generation and build support for runtime screen rotation. It adds rotation state management, display integration, orientation controls, localized messages, CI coverage, and tests for layout selection and dimension handling. ChangesRuntime UI rotation
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
cmake/MuiRuntimeRotation.cmake (1)
59-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the primary-tree glob to source files.
file(GLOB_RECURSE ... /generated/${_primary}/*)matches every file in the tree, including headers and any non-source file the UI generator writes. The manifest-driven secondary list uses explicit names, so the two sides are inconsistent. Match*.cexplicitly, and addCONFIGURE_DEPENDSso a regenerated tree retriggers configure.♻️ Proposed fix
- file(GLOB_RECURSE _primary_sources ${CMAKE_CURRENT_SOURCE_DIR}/generated/${_primary}/*) + file(GLOB_RECURSE _primary_sources CONFIGURE_DEPENDS + ${CMAKE_CURRENT_SOURCE_DIR}/generated/${_primary}/*.c) list(APPEND _sources ${_primary_sources})🤖 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 `@cmake/MuiRuntimeRotation.cmake` around lines 59 - 60, Update the primary-source glob in the CMake logic around _primary_sources to match only generated C files using an explicit *.c pattern, and add CONFIGURE_DEPENDS so regenerated files retrigger configuration. Leave the secondary manifest-driven source handling unchanged.tools/gen_dual_ui.py (2)
388-391: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrite the artifacts with an explicit encoding.
readopens files withencoding="utf-8", but the write path uses the platform default encoding. On a machine with a non-UTF-8 default locale, the generated bytes and the--checkcomparison can disagree. Pin the encoding and the newline translation so the committed output is reproducible on every platform.♻️ Proposed fix
for name, content in sorted(artifacts.items()): - with open(os.path.join(OUT, name), "w") as f: + with open(os.path.join(OUT, name), "w", encoding="utf-8", newline="\n") as f: f.write(content)🤖 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 `@tools/gen_dual_ui.py` around lines 388 - 391, Update the artifact-writing loop in the generator to open each output file with explicit UTF-8 encoding and disabled newline translation, matching the read path so generated output is reproducible across platforms.
120-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport stale
SECONDARY_ONLY_ASSETSentries.
asset_inventorynever validates that each name inSECONDARY_ONLY_ASSETSstill exists as a unique asset. If the UI generator removes such a file, or starts emitting it in both trees, the entry becomes dead and the fingerprint silently changes meaning. A short assertion keeps the allowlist honest.♻️ Proposed check
listed = { v: sorted(n for n in unique[v] if n in SECONDARY_ONLY_ASSETS) for v in VIEWS } + covered = {n for v in VIEWS for n in listed[v]} + stale = sorted(set(SECONDARY_ONLY_ASSETS) - covered) + if stale: + sys.exit( + "ERROR: SECONDARY_ONLY_ASSETS lists asset(s) that are no longer unique to one " + "tree: %s; update the list" % ", ".join(stale) + )🤖 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 `@tools/gen_dual_ui.py` around lines 120 - 174, Update asset_inventory to validate SECONDARY_ONLY_ASSETS after computing unique, ensuring every allowlisted name exists as a unique asset in the corresponding view and is not shared or absent. Fail with a clear error identifying any stale view/name entry before building listed or updating the fingerprint..github/workflows/ci.yml (1)
78-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a matrix instead of duplicated steps.
The three new steps repeat the configure, build, and test flow with one option changed. A job matrix over the
MUI_RUNTIME_ROTATIONvalue runs both configurations in parallel and removes the duplication, which keeps total CI wall time close to the current single-configuration time.🤖 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 @.github/workflows/ci.yml around lines 78 - 92, Replace the duplicated runtime-rotation configure, build, and test steps in the CI job with a matrix over the MUI_RUNTIME_ROTATION setting, covering both disabled and enabled values. Use the matrix value in the CMake configuration and ensure each matrix variant builds and runs its tests in a distinct build directory.extra_script.py (1)
88-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStyle routing depends on unverified basenames.
STYLE_ROUTED_CXXmatches only the file basename, andcmake/MuiRuntimeRotation.cmakelines 90-94 hardcodes the same two paths. If either file is renamed or moved, both build paths silently stop force-includingstyle_routing.h, and the secondary layout renders unthemed at runtime instead of failing the build. Track whether each expected translation unit was matched, and fail the build when one is missing.♻️ Proposed guard
routing_flags = ["-include", join(generated, "style_routing.h")] STYLE_ROUTED_CXX = ("Themes.cpp", "TFTView_320x240.cpp") + for _name in STYLE_ROUTED_CXX: + if not os.path.exists(join(lib_root, "source", "graphics", "TFT", _name)): + raise Exception("MUI_RUNTIME_ROTATION: style-routed source %s not found; " + "update STYLE_ROUTED_CXX" % _name)🤖 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 `@extra_script.py` around lines 88 - 102, Update apply_dual_ui_flags and the associated style-routing configuration to track whether each expected translation unit in STYLE_ROUTED_CXX was matched, including the corresponding CMake paths, and fail the build when either expected file is absent or renamed. Preserve the existing routing_flags application for valid matches while preventing a silent unthemed secondary build.CMakeLists.txt (1)
28-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate
VIEWbefore deriving the macro.Line 38 derives the compile definition by stripping a leading
ui_. If a user passes-DVIEW=320x240or any value without the prefix, the regex does not match andadd_compile_definitionsreceives the raw value, so noVIEW_*macro is defined and the failure appears later as missing UI symbols. A guard turns that into a clear configure-time error.♻️ Proposed guard
add_compile_definitions(ARCH_PORTDUINO) add_compile_definitions(ARDUINO) +if(NOT GENERATED_VIEW MATCHES "^ui_[0-9]+x[0-9]+$") + message(FATAL_ERROR "VIEW must name a generated view, e.g. ui_320x240 (got '${GENERATED_VIEW}')") +endif() string(REGEX REPLACE "^ui_" "VIEW_" GENERATED_VIEW_MACRO ${GENERATED_VIEW}) add_compile_definitions(${GENERATED_VIEW_MACRO})🤖 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 `@CMakeLists.txt` around lines 28 - 39, Validate the VIEW value after its default is assigned and before deriving GENERATED_VIEW_MACRO, requiring the expected ui_ prefix. Emit a clear configure-time error and stop configuration for invalid values, while preserving the existing regex conversion and compile-definition behavior for valid views.
🤖 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 `@cmake/MuiRuntimeRotation.cmake`:
- Around line 84-94: Update the project’s cmake_minimum_required declaration in
CMakeLists.txt from version 3.15 to 3.18 so the DIRECTORY option used by
set_source_files_properties in MuiRuntimeRotation.cmake is supported. Preserve
the existing minimum-version configuration otherwise.
In `@locale/cs.yml`:
- Around line 147-154: Replace the seven ~ values for Orientation, Landscape,
Portrait, Landscape flipped, Portrait flipped, Restarting to apply, Save failed,
and Restart required with localized translations in locale/cs.yml lines 147-154,
locale/da.yml lines 168-175, locale/de.yml lines 157-164, locale/el.yml lines
170-177, locale/es.yml lines 165-172, locale/fi.yml lines 153-160, locale/fr.yml
lines 160-167, and locale/hu.yml lines 170-177; preserve the existing keys and
YAML structure.
In `@tests/test_ScreenRotation.cpp`:
- Around line 15-19: Update the “both generated layouts are linked” test to
retain and observe mui_bridge_publish_secondary through a volatile
function-pointer variable or an equivalent non-optimizable link anchor,
replacing the ineffective direct non-null address check while preserving the
existing objects linkage assertion.
In `@tools/gen_dual_ui.py`:
- Around line 349-359: Update main to validate all arguments beginning with "--"
before selecting the view, accepting only "--check" and rejecting unknown flags
with an error exit. Preserve the existing positional view handling and
check_only behavior for valid arguments.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 78-92: Replace the duplicated runtime-rotation configure, build,
and test steps in the CI job with a matrix over the MUI_RUNTIME_ROTATION
setting, covering both disabled and enabled values. Use the matrix value in the
CMake configuration and ensure each matrix variant builds and runs its tests in
a distinct build directory.
In `@cmake/MuiRuntimeRotation.cmake`:
- Around line 59-60: Update the primary-source glob in the CMake logic around
_primary_sources to match only generated C files using an explicit *.c pattern,
and add CONFIGURE_DEPENDS so regenerated files retrigger configuration. Leave
the secondary manifest-driven source handling unchanged.
In `@CMakeLists.txt`:
- Around line 28-39: Validate the VIEW value after its default is assigned and
before deriving GENERATED_VIEW_MACRO, requiring the expected ui_ prefix. Emit a
clear configure-time error and stop configuration for invalid values, while
preserving the existing regex conversion and compile-definition behavior for
valid views.
In `@extra_script.py`:
- Around line 88-102: Update apply_dual_ui_flags and the associated
style-routing configuration to track whether each expected translation unit in
STYLE_ROUTED_CXX was matched, including the corresponding CMake paths, and fail
the build when either expected file is absent or renamed. Preserve the existing
routing_flags application for valid matches while preventing a silent unthemed
secondary build.
In `@tools/gen_dual_ui.py`:
- Around line 388-391: Update the artifact-writing loop in the generator to open
each output file with explicit UTF-8 encoding and disabled newline translation,
matching the read path so generated output is reproducible across platforms.
- Around line 120-174: Update asset_inventory to validate SECONDARY_ONLY_ASSETS
after computing unique, ensuring every allowlisted name exists as a unique asset
in the corresponding view and is not shared or absent. Fail with a clear error
identifying any stale view/name entry before building listed or updating the
fingerprint.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a38695f0-a239-40f3-924d-12da30b32f6b
⛔ Files ignored due to path filters (5)
tools/generated/dual/bridge_primary.cis excluded by!**/generated/**tools/generated/dual/bridge_secondary.cis excluded by!**/generated/**tools/generated/dual/manifest.txtis excluded by!**/generated/**tools/generated/dual/style_routing.his excluded by!**/generated/**tools/generated/dual/ui_secondary_rename.his excluded by!**/generated/**
📒 Files selected for processing (34)
.github/workflows/ci.ymlCMakeLists.txtcmake/MuiRuntimeRotation.cmakeextra_script.pyinclude/graphics/ScreenRotation.hinclude/graphics/driver/LGFXDriver.hinclude/graphics/view/TFT/TFTView_320x240.hlocale/bg.ymllocale/cs.ymllocale/da.ymllocale/de.ymllocale/el.ymllocale/en.ymllocale/es.ymllocale/fi.ymllocale/fr.ymllocale/hu.ymllocale/it.ymllocale/nl.ymllocale/no.ymllocale/pl.ymllocale/pt.ymllocale/ro.ymllocale/ru.ymllocale/se.ymllocale/sl.ymllocale/sr.ymllocale/tr.ymllocale/uk.ymllocale/zh-CN.ymlsource/graphics/ScreenRotation.cppsource/graphics/TFT/TFTView_320x240.cpptests/test_ScreenRotation.cpptools/gen_dual_ui.py
| set_source_files_properties( | ||
| ${_secondary_sources} ${MUI_GLUE_DIR}/bridge_secondary.c | ||
| DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} | ||
| PROPERTIES COMPILE_OPTIONS "${_rename_flags}") | ||
|
|
||
| # C++ sources referencing generated styles dispatch them to the active tree. | ||
| set_source_files_properties( | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/source/graphics/TFT/Themes.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/source/graphics/TFT/TFTView_320x240.cpp | ||
| DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} | ||
| PROPERTIES COMPILE_OPTIONS "-include;${MUI_GLUE_DIR}/style_routing.h") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the declared minimum CMake version.
set -euo pipefail
rg -n 'cmake_minimum_required' --glob '*.txt' --glob '*.cmake'Repository: meshtastic/device-ui
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "CMake files:"
git ls-files | rg '(^|/)(CMakeLists\.txt|.*\.cmake)$' || true
echo
echo "cmake_minimum_required occurrences:"
rg -n --fixed-strings 'cmake_minimum_required' . --glob '*.txt' --glob '*.cmake' || true
echo
echo "Directory option usages:"
rg -n 'set_source_files_properties' --glob '*.cmake' --glob '*.txt' . || true
echo
echo "Relevant file excerpt:"
if [ -f cmake/MuiRuntimeRotation.cmake ]; then
sed -n '1,130p' cmake/MuiRuntimeRotation.cmake | cat -n
fiRepository: meshtastic/device-ui
Length of output: 5946
🌐 Web query:
CMake set_source_files_properties DIRECTORY option version documentation
💡 Result:
The DIRECTORY option for the set_source_files_properties command in CMake was introduced in version 3.18 [1][2]. It allows you to set source file properties in specific directory scopes beyond the current CMakeLists.txt file [1][3]. By default, source file properties are only visible to targets defined within the same directory scope [1][2]. The DIRECTORY option overrides this behavior by applying the specified properties to the source files within each of the provided directory scopes [1][3]. Key requirements and details for using the DIRECTORY option include: 1. Prerequisite Knowledge: CMake must already be aware of the specified directories [1][3]. This means they must have been added previously via the add_subdirectory command or be the top-level source directory [1][2]. 2. Path Handling: Relative paths provided to the DIRECTORY option are interpreted as relative to the current source directory [1][3]. 3. Alternative Scope: The command also supports a TARGET_DIRECTORY option, which allows setting properties in the directory scopes where specific targets were created [1][2]. You can retrieve these properties using the get_source_file_property command [1][3]. Note that the GENERATED property is an exception to these scoping rules and may be globally visible [1][4].
Citations:
- 1: https://cmake.org/cmake/help/latest/command/set_source_files_properties.html
- 2: https://cmake.org/cmake/help/v3.30/command/set_source_files_properties.html
- 3: https://cmake.org/cmake/help/v3.31/command/set_source_files_properties.html
- 4: https://cmake.org/cmake/help/git-stage/command/set_source_files_properties.html
Require CMake 3.18 for the CMakeLists.txt minimum.
CMakeLists.txt declares cmake_minimum_required(VERSION 3.15), but cmake/MuiRuntimeRotation.cmake uses set_source_files_properties(... DIRECTORY ...) from lines 84-94. That call is invalid before CMake 3.18, so older compatible CMake releases fail at configure time and the rotation rename or style-routing flags are not applied.
🤖 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 `@cmake/MuiRuntimeRotation.cmake` around lines 84 - 94, Update the project’s
cmake_minimum_required declaration in CMakeLists.txt from version 3.15 to 3.18
so the DIRECTORY option used by set_source_files_properties in
MuiRuntimeRotation.cmake is supported. Preserve the existing minimum-version
configuration otherwise.
| Orientation: ~ | ||
| Landscape: ~ | ||
| Portrait: ~ | ||
| Landscape flipped: ~ | ||
| Portrait flipped: ~ | ||
| Restarting to apply: ~ | ||
| Save failed: ~ | ||
| Restart required: ~ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Translate the new orientation messages in all non-English locales.
The same seven keys use ~ in each locale. Add localized values so orientation labels and restart-status messages are not shown untranslated.
locale/cs.yml#L147-L154: Add Czech translations.locale/da.yml#L168-L175: Add Danish translations.locale/de.yml#L157-L164: Add German translations.locale/el.yml#L170-L177: Add Greek translations.locale/es.yml#L165-L172: Add Spanish translations.locale/fi.yml#L153-L160: Add Finnish translations.locale/fr.yml#L160-L167: Add French translations.locale/hu.yml#L170-L177: Add Hungarian translations.
📍 Affects 8 files
locale/cs.yml#L147-L154(this comment)locale/da.yml#L168-L175locale/de.yml#L157-L164locale/el.yml#L170-L177locale/es.yml#L165-L172locale/fi.yml#L153-L160locale/fr.yml#L160-L167locale/hu.yml#L170-L177
🤖 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 `@locale/cs.yml` around lines 147 - 154, Replace the seven ~ values for
Orientation, Landscape, Portrait, Landscape flipped, Portrait flipped,
Restarting to apply, Save failed, and Restart required with localized
translations in locale/cs.yml lines 147-154, locale/da.yml lines 168-175,
locale/de.yml lines 157-164, locale/el.yml lines 170-177, locale/es.yml lines
165-172, locale/fi.yml lines 153-160, locale/fr.yml lines 160-167, and
locale/hu.yml lines 170-177; preserve the existing keys and YAML structure.
| TEST_CASE("both generated layouts are linked") | ||
| { | ||
| CHECK(&objects != &MUI2_objects); | ||
| CHECK(mui_bridge_publish_secondary != nullptr); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the bridge declaration and its generated definition.
rg -n -C 3 'mui_bridge_publish_secondary|MUI2_objects' \
tests/test_ScreenRotation.cpp \
.Repository: meshtastic/device-ui
Length of output: 5397
🌐 Web query:
C++ standard address-of function pointer compare with nullptr can be true even if function deleted
💡 Result:
In C++, it is not possible to obtain a pointer to a deleted function. The C++ standard mandates that any attempt to form a pointer to a deleted function is ill-formed [1]. Because this operation is a compile-time error, you cannot evaluate such a pointer, let alone compare it with nullptr [1]. Confusion regarding this topic often stems from a misunderstanding of what a "deleted function" is in C++ versus a "deleted pointer" or an object that has been deallocated: 1. Deleted Functions (=delete): When a function is declared as deleted, the compiler removes its definition [2]. Any code that attempts to refer to this function—including calling it or taking its address—will result in a compilation error [1][3]. It is not a runtime state, so it cannot "compare" to anything at runtime [1]. 2. Deallocated Memory (delete operator): If you are referring to a pointer that held the address of an object that has since been deallocated (e.g., using the delete keyword on a heap-allocated object), that pointer becomes an invalid pointer [4][5]. Using an invalid pointer in any way, including comparing it to nullptr, results in undefined behavior [5]. While an invalid pointer might happen to compare equal to nullptr on some specific implementations or under specific circumstances, this is not standard-compliant behavior and should never be relied upon [6][5]. In summary, a deleted function cannot have an address and cannot be compared to nullptr because the code will not compile [1]. A pointer to a deallocated object is invalid, and its comparison to nullptr is undefined behavior [5].
Citations:
- 1: https://timsong-cpp.github.io/cppwp/dcl.fct.def.delete
- 2: https://stackoverflow.com/questions/19265920/whats-the-exact-semantics-of-deleted-member-functions-in-c11
- 3: https://learn.microsoft.com/en-us/cpp/cpp/explicitly-defaulted-and-deleted-functions?view=msvc-170
- 4: https://cppreference.net/cpp/memory/new/operator_delete.html
- 5: https://stackoverflow.com/questions/20725030/address-held-by-pointer-changes-after-pointer-is-deleted
- 6: https://lwn.net/Articles/894076/
🌐 Web query:
libtool archive member symbol extraction test CHECK(function != nullptr) compiler optimization linker
💡 Result:
The issue you are encountering likely stems from a conflict between libtool’s legacy mechanisms for symbol management and modern compiler optimizations that assume Undefined Behavior (UB) never occurs [1][2]. Libtool historically supports "fake static preloaded modules" by extracting symbols from objects using tools like nm, then synthesizing a source file that declares these symbols to build a symbol list (lt_dlsymlist) [1]. This generated code often uses pointer-to-void casting and potentially mismatched prototypes, which modern compilers—particularly under Link Time Optimization (LTO) or aggressive optimization levels—may interpret as UB [1]. Specifically, regarding your mention of CHECK(function!= nullptr): 1. Compiler Optimization of Null Checks: Modern compilers (GCC, Clang) often assume that dereferencing a pointer or calling a member function implies the pointer is non-null [2][3]. If the compiler determines through inlining or value range propagation that a pointer must be non-null to avoid UB, it may eliminate "redundant" null checks (e.g., if(function!= nullptr)) [2][3]. If your code relies on this check to handle a scenario that the compiler deems impossible, the optimization effectively removes the safety logic, leading to crashes [4][2]. 2. LTO and Symbol Visibility: Libtool's symbol extraction sometimes struggles when LTO is involved, as LTO may rightfully complain about inconsistent declarations or unexpected visibility changes [1]. Furthermore, if libtool is not correctly handling --whole-archive or symbol visibility, the linker might discard object code containing those functions, rendering the pointers null or invalid when the runtime attempts to access them [5]. 3. Debugging/Resolution: - Verify if LTO flags (-flto) are being passed to the compilation of the generated libtool symbol files; these should generally be avoided for such synthesized code [1]. - Consider if the issue is related to the compiler assuming that symbols cannot be null. In high-performance contexts, developers sometimes use -fno-delete-null-pointer-checks to prevent the compiler from optimizing out these specific safety checks [6][3]. - Ensure that the archive member containing the function is not being garbage-collected by the linker. If the symbol is not explicitly referenced by the final executable, the linker may drop the object file entirely unless --whole-archive is used [5]. If you are seeing a crash where the null check is "optimized out," the compiler is likely treating your code as UB. You can test this by disabling optimizations for that specific unit or using the aforementioned compiler flag to confirm if the behavior is caused by aggressive null-check elimination [4][3].
Citations:
- 1: https://lists.nongnu.org/archive/html/libtool/2010-03/msg00025.html
- 2: https://stackoverflow.com/questions/36893251/why-does-the-enhanced-gcc-6-optimizer-break-practical-c-code
- 3: https://mina86.com/2025/this-is-null/
- 4: https://stackoverflow.com/questions/66494107/g-optimizes-out-nullpointer-checks
- 5: https://www.mail-archive.com/libtool-patches@gnu.org/msg07443.html
- 6: [Inline] Allow inlining with null_pointer_is_valid mismatch llvm/llvm-project#190510
Force a retained reference to mui_bridge_publish_secondary.
A direct function pointer address cannot be null, so this test does not prove the secondary bridge object is linked. Store the address in a volatile function-pointer variable, or use an observable link anchor that cannot be optimized away.
🤖 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 `@tests/test_ScreenRotation.cpp` around lines 15 - 19, Update the “both
generated layouts are linked” test to retain and observe
mui_bridge_publish_secondary through a volatile function-pointer variable or an
equivalent non-optimizable link anchor, replacing the ineffective direct
non-null address check while preserving the existing objects linkage assertion.
| def main(): | ||
| check_only = "--check" in sys.argv[1:] | ||
| args = [a for a in sys.argv[1:] if not a.startswith("--")] | ||
| # accepted for symmetry with the build invocation; the output is the same | ||
| # for either primary, so it only has to be a supported view | ||
| view = args[0] if args else DEFAULT_PRIMARY | ||
| if view not in VIEWS: | ||
| sys.exit( | ||
| "ERROR: MUI_RUNTIME_ROTATION supports only %s; view %r is not one of them" | ||
| % (" and ".join(VIEWS), view) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject unknown flags in main.
check_only matches only --check, and line 351 discards every other -- argument. A typo such as --chek therefore runs a full regeneration instead of a verification, and a CI gate would overwrite the committed glue rather than fail. Reject any flag that is not --check.
🐛 Proposed fix
- check_only = "--check" in sys.argv[1:]
- args = [a for a in sys.argv[1:] if not a.startswith("--")]
+ flags = [a for a in sys.argv[1:] if a.startswith("--")]
+ unknown = [a for a in flags if a != "--check"]
+ if unknown:
+ sys.exit("ERROR: unknown option(s): %s" % ", ".join(unknown))
+ check_only = "--check" in flags
+ args = [a for a in sys.argv[1:] if not a.startswith("--")]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def main(): | |
| check_only = "--check" in sys.argv[1:] | |
| args = [a for a in sys.argv[1:] if not a.startswith("--")] | |
| # accepted for symmetry with the build invocation; the output is the same | |
| # for either primary, so it only has to be a supported view | |
| view = args[0] if args else DEFAULT_PRIMARY | |
| if view not in VIEWS: | |
| sys.exit( | |
| "ERROR: MUI_RUNTIME_ROTATION supports only %s; view %r is not one of them" | |
| % (" and ".join(VIEWS), view) | |
| ) | |
| def main(): | |
| flags = [a for a in sys.argv[1:] if a.startswith("--")] | |
| unknown = [a for a in flags if a != "--check"] | |
| if unknown: | |
| sys.exit("ERROR: unknown option(s): %s" % ", ".join(unknown)) | |
| check_only = "--check" in flags | |
| args = [a for a in sys.argv[1:] if not a.startswith("--")] | |
| # accepted for symmetry with the build invocation; the output is the same | |
| # for either primary, so it only has to be a supported view | |
| view = args[0] if args else DEFAULT_PRIMARY | |
| if view not in VIEWS: | |
| sys.exit( | |
| "ERROR: MUI_RUNTIME_ROTATION supports only %s; view %r is not one of them" | |
| % (" and ".join(VIEWS), view) | |
| ) |
🤖 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 `@tools/gen_dual_ui.py` around lines 349 - 359, Update main to validate all
arguments beginning with "--" before selecting the view, accepting only
"--check" and rejecting unknown flags with an error exit. Preserve the existing
positional view handling and check_only behavior for valid arguments.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Looking at the code (see gen_dual_ui.py, bridge_primary.c / bridge_secondary.c, ...) it looks like it is purely vibe coded without any steering into direction of minimal code changes and reusable design. This code as it is will never be maintainable and may break easily when adding new features. Also this doesn't look like a generic solution and will probably work only for a single device so I don't think it is worth to further take into consideration. |
3 participants
What this does
Lets the color UI switch between the landscape and portrait layouts at runtime,
as a settings entry, instead of committing to one at build time via
VIEW_320x240/VIEW_240x320.Off by default.
MUI_RUNTIME_ROTATIONis opt-in; a build without it isunchanged (evidence below).
Implements the runtime half of what #288 asked for. #294 solved it by adding the
perpendicular layout; this makes choosing between them a user setting.
How
Both generated layouts are linked into one binary. The primary tree (the build's
VIEW_*) keeps its symbols and supplies every shared asset; the secondary treeis compiled with its colliding globals renamed through a generated forced
include, and a generated bridge republishes its widget pointers into the symbols
the C++ view addresses.
tools/gen_dual_ui.pyderives that glue from the trees themselves and assertswhat it depends on: identical
objects_tmember sets, symmetric global symbols,byte-identical shared assets, identical non-asset source inventory.
--checkregenerates every artifact and byte-compares it, and rejects unexpected files, so
the committed glue cannot drift from the trees unnoticed — it runs on every
build and in CI.
The stored value is a quarter-turn count composed with each board's existing
LGFX_ROTATIONoffset, so value 0 reproduces the current behaviour on any board.Even values keep the compile-time layout, odd values select the perpendicular
one. Persistence uses the existing
storeUIConfigpath, followed by a rebootrequest, since the value is only read at boot.
Cost, measured: +66.7 KB flash (1.04 % of a 6.25 MB app partition), +4.5 KB
static RAM. The 93 byte-identical asset translation units compile once, which is
why this is far cheaper than linking two full trees.
Scope limits, stated up front
VIEW_320x240andVIEW_240x320. They are the only perpendicularpair; any other
VIEW_*fails the build with a message saying so.CUSTOM_TOUCH_DRIVERboards are excluded at compile time. Those driversapply a fixed rotation to touch coordinates and would desynchronize from a
rotated panel. That unfortunately includes the Heltec V4 of [Feature Request]: Custom / rotated UI for Heltec V4 Expansion Kit #288. Making it
work needs a rotation-aware custom-touch contract, which I left out of this PR
deliberately — happy to follow up if you want it.
Two independent fixes, first in the series
These stand alone, apply with the feature off, and can be taken without the rest:
fix(graphics): the statistics table uses a fixed 36 px column width unlessthe horizontal resolution exceeds 320, so a 240 px screen asks for
57 + 6*36 = 273 px and overflows. Affects existing compile-time
VIEW_240x320builds too.fix(build):set(GENERATED_VIEW ${VIEW} "ui_320x240")produces atwo-element list as soon as
VIEWis set, andVIEW_320x240was definedunconditionally — so the CMake build could only ever produce
ui_320x240.No change when
VIEWis unset.Verification
Hardware — all four rotations, on device. ESP32-S3 with an ST7789 320x240
panel. Each value was driven through the on-device settings entry, so the whole
chain is covered: store, reboot request, and the value being applied on the next
boot. (Attaky Mesh Series)
This caught a real bug that was not derivable on paper: quarter turns had to run
opposite to the panel's own numbering, because upright portrait is one step
back from upright landscape on this panel, not one step on.
Feature-off equivalence. Built the branch with the macro undefined and the
same branch minus the feature commits, through the same PlatformIO environment
sequentially — necessary because LTO stamps a per-TU hash derived from the source
path, so two differently-named environments differ everywhere for no reason.
Result: identical length; after masking the ELF-hash at
0xB0..0xCFand the33-byte trailing digest, 2 bytes differ. Both come from
source/graphics/ScreenRotation.cppexisting at all — with the macro off itcompiles to a genuinely empty TU (
sizereports text/data/bss all 0), but oneextra TU perturbs LTO's internal numbering. The decisive evidence is
symbol-level: the same 20,934 defined symbols, zero additions or removals,
with
textandbssidentical.Which hardware I do not have. Everything above is on one ESP32-S3 + ST7789
board. I have no Heltec V3, T-Deck, T-Beam, RAK4631 or T-1000E to test against,
per the template. The equivalence result above is the reason I think that is
acceptable here: with
MUI_RUNTIME_ROTATIONundefined those builds get the samebinary they get today, so there is nothing for the feature to regress. Testing
is only needed for boards that actually turn it on.
Attestations
regressions on the listed devices — I do not have them; see above for why
feature-off builds are unaffected.
screen.orintation.-.small.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Localization