feat(baseui): tier the emote set by flash class - #11257
Merged
Merged
Conversation
The BaseUI emote table is 146 entries over 103 unique 16x16 bitmaps, and it was all-or-nothing: EXCLUDE_EMOJI or the lot. On the tightest targets that made it a 6.8 KB cliff, which is how nrf52_promicro_diy_tcxo ended up giving up emoji entirely to fit under the warm-store region. Split it into four ordinal levels - NONE, CORE, STANDARD, FULL - gated per section so picker order is unchanged as tiers drop out. Measured on rak4631: FULL 0xE46D8 (todays behavior) STANDARD 0xE3BC0 -2,840 CORE 0xE30E0 -5,624 NONE 0xE2C40 -6,808 Only the table is gated. Dropped entries leave their bitmaps unreferenced and -fdata-sections/--gc-sections discards them, so a tier costs one #if per section rather than a gate per bitmap. Verified at symbol level: an EMOTE_SET_CORE image keeps graphics::thumbup/question/heart/check_mark and has dropped smiling_eyes, sun, new_moon, jack_o_lantern and the rest. The level is pegged to a new MESHTASTIC_FLASH_CLASS (memory/FlashClass.h), deliberately NOT to MESHTASTIC_MEM_CLASS. Emote bitmaps are PROGMEM and cost zero RAM, and the two resources genuinely disagree: classic ESP32 and nRF52840 are both MEM_CLASS_SMALL, but the ESP32 has over twice the app flash, so a RAM-keyed default would trim the wrong boards. FlashClass is the sibling ladder MemClass.h already gestures at when it calls MAX_NUM_NODES "deliberately unclassed (flash-shaped)". Defaults: FLASH_CLASS_SMALL (nRF52840) gets STANDARD, everything MEDIUM and above keeps FULL, TINY gets NONE. That is a real behavior change for nRF52 - it trades moon phases, objects, arrows and the seasonal sets for 2.8 KB on a class where rak4631 already sits at 97% of its region on develop. Variants override with -D MESHTASTIC_EMOTE_SET, as promicro now does. EXCLUDE_EMOJI still works and maps to EMOTE_SET_NONE. InkHUD is unaffected: it excludes graphics/emotes.cpp from its build.
Contributor
📝 WalkthroughWalkthroughThe change introduces flash-class-based emote tiers, applies those tiers to bitmap declarations and emote-table entries, adds a non-empty-table assertion, and configures the nRF52 Pro Micro TCXO variant to build with no emotes. ChangesEmote tier selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/memory/FlashClass.h (1)
3-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim oversized C++ comment blocks.
Both headers add multi-line narrative comments where the macro names and concise contract comments are sufficient.
src/memory/FlashClass.h#L3-L24: reduce the block to the flash-vs-RAM distinction and override behavior.src/graphics/emotes.h#L6-L19: reduce the block to flash-based tier selection and independent section gating.As per coding guidelines: “Keep code comments minimal—one or two lines maximum.”
🤖 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/memory/FlashClass.h` around lines 3 - 24, Trim the narrative comment block in src/memory/FlashClass.h lines 3-24 to one or two lines covering only the flash-versus-RAM distinction and that variants may override the default. Also shorten the comment block in src/graphics/emotes.h lines 6-19 to one or two lines describing flash-based tier selection and independent section gating; do not change the macro logic.Source: Coding guidelines
🤖 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/emotes.cpp`:
- Around line 10-14: Update the emotes[] definition in src/graphics/emotes.cpp
to provide a one-element placeholder array when MESHTASTIC_EMOTE_SET is
EMOTE_SET_NONE, while keeping numEmotes equal to 0 for that branch. Preserve the
existing emote table for all non-empty tiers so the static assertion and normal
counts remain unchanged.
---
Nitpick comments:
In `@src/memory/FlashClass.h`:
- Around line 3-24: Trim the narrative comment block in src/memory/FlashClass.h
lines 3-24 to one or two lines covering only the flash-versus-RAM distinction
and that variants may override the default. Also shorten the comment block in
src/graphics/emotes.h lines 6-19 to one or two lines describing flash-based tier
selection and independent section gating; do not change the macro logic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cf4fc9f9-cdad-4041-be06-73a33ac3a8bf
📒 Files selected for processing (4)
src/graphics/emotes.cppsrc/graphics/emotes.hsrc/memory/FlashClass.hvariants/nrf52840/diy/nrf52_promicro_diy_tcxo/platformio.ini
Comment on lines
+10
to
+14
| #if MESHTASTIC_EMOTE_SET >= EMOTE_SET_CORE | ||
| // --- Thumbs --- | ||
| {"\U0001F44D", thumbup, thumbs_width, thumbs_height}, // 👍 Thumbs Up | ||
| {"\U0001F44E", thumbdown, thumbs_width, thumbs_height}, // 👎 Thumbs Down | ||
| #endif // EMOTE_SET_CORE |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -eu
tmp="$(mktemp --suffix=.cpp)"
trap 'rm -f "$tmp"' EXIT
cat >"$tmp" <<'EOF'
struct Emote {};
const Emote emotes[] = {};
EOF
"${CXX:-c++}" -std=c++17 -pedantic-errors -fsyntax-only "$tmp"Repository: meshtastic/firmware
Length of output: 199
🏁 Script executed:
#!/usr/bin/env bash
set -eu
echo "CXX availability:"
command -v c++ || true
command -v clang++ || true
command -v g++ || true
echo
echo "Relevant files:"
fd -a 'emotes.cpp|platformio.ini' . | sed 's#^\./##' | head -100
echo
echo "Search for MESHTASTIC_EMOTE_SET definitions/usages:"
rg -n "MESHTASTIC_EMOTE_SET|EMOTE_SET_NONE|enum.*Emote|enum class Emote|struct Emote|const Emote emotes" .Repository: meshtastic/firmware
Length of output: 9453
🏁 Script executed:
#!/usr/bin/env bash
set -eu
echo "emotes.cpp first 35 lines:"
sed -n '1,35p' src/graphics/emotes.cpp | cat -n
echo
echo "emotes.cpp lines 217-235:"
sed -n '217,235p' src/graphics/emotes.cpp | cat -n
echo
echo "emotes.h relevant section:"
sed -n '40,55p' src/graphics/emotes.h | cat -n
echo
echo "clang C++17 compile test for unknown-bound empty aggregate initializer:"
tmp="$(mktemp --suffix=.cpp)"
trap 'rm -f "$tmp"' EXIT
cat >"$tmp" <<'EOF'
struct Emote {
const char* label;
};
const Emote emotes[] = {};
EOF
if command -v clang++ >/dev/null 2>&1; then
clang++ -std=c++17 -pedantic-errors -fsyntax-only "$tmp" || true
fi
echo
echo "clang C++17 compile test for bounded empty aggregate initializer:"
tmp2="$(mktemp --suffix=.cpp)"
trap 'rm -f "$tmp2"' EXIT
cat >"$tmp2" <<'EOF'
struct Emote {
const char* label;
};
const Emote emotes[1] = {};
EOF
if command -v clang++ >/dev/null 2>&1; then
clang++ -std=c++17 -pedantic-errors -fsyntax-only "$tmp2" || true
fiRepository: meshtastic/firmware
Length of output: 4519
Fix the empty emotes[] definition for EMOTE_SET_NONE.
With MESHTASTIC_EMOTE_SET=EMOTE_SET_NONE, every initializer is removed, leaving an unknown-bound empty array declaration that fails before numEmotes or the static assertion are reached. Use a placeholder ONE-element array for the NONE branch and keep numEmotes at 0; use the current table for non-empty tiers.
🤖 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/emotes.cpp` around lines 10 - 14, Update the emotes[] definition
in src/graphics/emotes.cpp to provide a one-element placeholder array when
MESHTASTIC_EMOTE_SET is EMOTE_SET_NONE, while keeping numEmotes equal to 0 for
that branch. Preserve the existing emote table for all non-empty tiers so the
static assertion and normal counts remain unchanged.
Contributor
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
thebentern
added
bugfix
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The BaseUI emote table is 146 entries over 103 unique 16x16 bitmaps, and today it's all-or-nothing:⚠️ , drop the moon phases."
EXCLUDE_EMOJI, or the lot. On tight targets that's a 6.8 KB cliff — which is exactly why #11256 hasnrf52_promicro_diy_tcxogiving up emoji entirely to fit under the warm-store region. There was no way to say "keep 👍 andWhat
Four ordinal levels, gated per section so picker order doesn't shuffle as tiers drop out:
EMOTE_SET_NONEEMOTE_SET_COREEMOTE_SET_STANDARDEMOTE_SET_FULLMeasured on rak4631:
FULL(today)0xE46D8STANDARD0xE3BC0CORE0xE30E0NONE0xE2C40Only the table is gated, not the ~103 bitmap definitions. Dropped entries leave their bitmaps unreferenced and
-fdata-sections/--gc-sectionsdiscards them, so a tier costs one#ifper section instead of a gate per bitmap. Verified at symbol level — anEMOTE_SET_COREimage keepsgraphics::thumbup,question,heart,check_markand has droppedsmiling_eyes,sun,new_moon,jack_o_lantern,turkey,bowling,christmas_tree.Why a new flash ladder instead of MESHTASTIC_MEM_CLASS
This is the part worth arguing with me about. Emote bitmaps are
PROGMEM— they cost zero RAM — so keying them off the RAM ladder would be measuring the wrong resource, and the two genuinely disagree: classic ESP32 and nRF52840 are bothMEM_CLASS_SMALL, but the ESP32 has well over twice the app flash. A RAM-keyed default would trim the wrong boards.So this adds
MESHTASTIC_FLASH_CLASS(src/memory/FlashClass.h) as the sibling ladder thatMemClass.halready gestures at when it callsMAX_NUM_NODES"deliberately unclassed (flash-shaped)". It's written to be reusable for other flash-shaped decisions later.A class is a default, not a verdict — rak4631 and promicro are the same chip and the same
FLASH_CLASS_SMALL, yet one has ~22 KB of headroom and the other doesn't fit at all. Variants override with-D MESHTASTIC_EMOTE_SET, as promicro does here.Behavior change to weigh
FLASH_CLASS_SMALL(nRF52840) defaults toSTANDARD, so every nRF52 board loses moon phases, objects, arrows and the seasonal sets for 2,840 bytes. I picked that because nRF52840 is the tightest screen-capable region we ship and rak4631 already sits at ~97% of it on develop — but it's a one-line change to leave nRF52 onFULLif you'd rather only promicro pay. ESP32/RP2040 and above are unchanged atFULL.EXCLUDE_EMOJIstill works and maps toEMOTE_SET_NONE. InkHUD is unaffected — it already excludesgraphics/emotes.cppfrom its build.Verification
nrf52_promicro_diy_tcxolands at0xE8618, byte-identical to theEXCLUDE_EMOJIbuild it replaces.static_assertcatches a mis-gated section that would otherwise silently ship an empty picker.test_B11_normal_unicast_still_uses_pki,test_B12_licensed_receiver_does_not_decrypt_pki) reproduce identically on pristinedevelop@ 1e982fa and are unrelated to this change — worth a separate look, they came in with Sign plaintext packets in licensed mode #10969.Summary by CodeRabbit
New Features
Improvements