Reduce ExternalNotificationModule flash usage (RTTTL + InputBroker) by ndoo · Pull Request #10989 · meshtastic/firmware

ndoo · 2026-07-11T16:44:28Z

Summary

Adds MESHTASTIC_EXCLUDE_RTTTL, set in each architecture's *_base build_flags (stm32_base, esp32c6_base, portduino_base), replacing the inline ARCH_STM32WL/ARCH_PORTDUINO/CONFIG_IDF_TARGET_ESP32C6 check in ExternalNotificationModule.h that already stubbed out RTTTL playback on these platforms. Also gates the ringtone config/persistence/admin-message plumbing around that already-stubbed playback path behind the same flag.
Gates ExternalNotificationModule's inputObserver (CallbackObserver) member and handleInputEvent() behind MESHTASTIC_EXCLUDE_INPUTBROKER, matching the flag its only use site already checked.
wio-e5: 656 bytes flash saved (368 + 288), ~236 bytes RAM saved. No behavior change on any platform.

What changed

variants/stm32/stm32.ini (stm32_base), variants/esp32c6/esp32c6.ini (esp32c6_base), variants/native/portduino.ini (portduino_base): set MESHTASTIC_EXCLUDE_RTTTL for the three platforms that previously got the inline RTTTL stub. This matches how every other MESHTASTIC_EXCLUDE_* flag in the tree is set (stm32_base already sets ten directly; esp32c6_base already excludes PAXCOUNTER for an analogous platform-limitation reason) rather than introducing a new per-architecture header mechanism.
src/modules/ExternalNotificationModule.h / .cpp: consume MESHTASTIC_EXCLUDE_RTTTL instead of the inline ARCH check; gate ringtone get/set AdminMessage handlers, /prefs/ringtone.proto persistence, and the PWM-playback call site behind it. Gate inputObserver / handleInputEvent() behind MESHTASTIC_EXCLUDE_INPUTBROKER.

Testing

pio run -e wio-e5: confirmed flash/RAM deltas in isolation and combined; used nm --size-sort -S on the linked ELF to confirm removed code was unreachable, not live functionality.
pio run -e native-macos -v: confirmed -DMESHTASTIC_EXCLUDE_RTTTL reaches the compiled build-flag set for portduino (full link not verifiable on this machine due to an unrelated missing argp-standalone dependency).
No physical hardware tested. Affected targets: wio-e5, rak3172, russell, CDEBYTE_E77-MBL, milesight_gs301 (STM32WL), plus portduino and ESP32C6 builds.

🤝 Attestations

I have tested that my proposed changes behave as described. (build-level only, see Testing above)
I have tested that my proposed changes do not cause any obvious regressions on the following devices:
- Heltec (Lora32) V3
- LilyGo T-Deck
- LilyGo T-Beam
- RAK WisBlock 4631
- Seeed Studio T-1000E tracker card
- Other (please specify below): none of these are affected — see Testing above for the platforms this does touch

Summary by CodeRabbit

Bug Fixes
- Improved compatibility for platforms that don’t support ringtone (RTTTL/PWM) playback.
- Removed ringtone controls and buzzer alert behavior from unsupported builds.
- Improved compatibility for builds without input-event broker support.
Platform Support
- Updated ESP32-C6, native (Portduino), and STM32 build configurations to disable RTTTL/PWM ringtone support via compile-time flags.

coderabbitai · 2026-07-11T16:44:35Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e74b1a6a-3e19-4017-bb47-9e297e3ee8df

📥 Commits

Reviewing files that changed from the base of the PR and between aff3a3b and b417267.

📒 Files selected for processing (1)

variants/stm32/stm32.ini

🚧 Files skipped from review as they are similar to previous changes (1)

variants/stm32/stm32.ini

📝 Walkthrough

Walkthrough

The change adds platform-specific RTTTL exclusion flags and applies RTTTL and input-broker conditional compilation throughout ExternalNotificationModule.

Changes

Conditional Notification Features

Layer / File(s)	Summary
Platform RTTTL exclusions `variants/esp32c6/esp32c6.ini`, `variants/native/portduino.ini`, `variants/stm32/stm32.ini`	The ESP32-C6, Portduino, and STM32 builds define `MESHTASTIC_EXCLUDE_RTTTL`.
Notification module conditional declarations `src/modules/ExternalNotificationModule.h`	RTTTL handlers and input-broker members are declared only when their corresponding features are enabled.
Notification module conditional behavior `src/modules/ExternalNotificationModule.cpp`	RTTTL state, playback, configuration, ringtone administration, input observer registration, and input event handling are conditionally compiled.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: reducing ExternalNotificationModule flash usage by excluding RTTTL and InputBroker code.
Description check	✅ Passed	The description covers summary, changed files, testing, and attestations, matching the repository template well.
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 unit tests (beta)

Create PR with unit tests

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.}

github-actions · 2026-07-11T16:44:36Z

⚡ Try this PR in the Web Flasher

Note

Building this pull request… the flash button, badges and supported-board
list will appear here automatically once CI finishes.

ndoo · 2026-07-11T17:04:19Z

@coderabbitai review

coderabbitai · 2026-07-11T17:04:25Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 1

🤖 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/modules/ExternalNotificationModule.cpp`:
- Around line 60-63: Guard the I2S playback paths that reference rtttlConfig,
including the logic near the I2S handlers around lines 144 and 468, with
!MESHTASTIC_EXCLUDE_RTTTL alongside HAS_I2S. Ensure builds with
MESHTASTIC_EXCLUDE_RTTTL enabled do not compile any rtttlConfig references while
preserving the existing I2S ringtone behavior when RTTTL is enabled.

🪄 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: 9f6e8150-7ab7-45c7-96d4-cd27c6f7ba32

📥 Commits

Reviewing files that changed from the base of the PR and between c535564 and 05f8ac6.

📒 Files selected for processing (5)

src/modules/ExternalNotificationModule.cpp
src/modules/ExternalNotificationModule.h
src/platform/esp32/architecture.h
src/platform/portduino/architecture.h
src/platform/stm32wl/architecture.h

ndoo · 2026-07-11T17:28:00Z

@coderabbitai review

Fixed: the two HAS_I2S ringtone-playback call sites (audioThread->beginRttl in runOnce() and handleReceived()) referenced rtttlConfig without also checking !MESHTASTIC_EXCLUDE_RTTTL, even though rtttlConfig is now only declared under that guard. No platform currently defines both HAS_I2S and MESHTASTIC_EXCLUDE_RTTTL, so this had no effect on any build today, but it would have broken compilation for a future HAS_I2S platform that also excludes RTTTL. Both sites now check defined(HAS_I2S) && !MESHTASTIC_EXCLUDE_RTTTL.

coderabbitai · 2026-07-11T17:28:32Z

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n "HAS_I2S" src/modules/ExternalNotificationModule.cpp -B2 -A2

Length of output: 1512

Confirms the fix is in place at both call sites (Line 139 and Line 467) with defined(HAS_I2S) && !MESHTASTIC_EXCLUDE_RTTTL.

@ndoo Confirmed — both audioThread->beginRttl call sites in runOnce() (Line 139) and handleReceived() (Line 467) are now correctly guarded with #if defined(HAS_I2S) && !MESHTASTIC_EXCLUDE_RTTTL, matching the guard around the rtttlConfig declaration. This closes the compile-time gap for any future HAS_I2S platform that also sets MESHTASTIC_EXCLUDE_RTTTL. Thanks for the fix.

✏️ Learnings added

Learnt from: ndoo
URL: https://github.com/meshtastic/firmware/pull/10989

Timestamp: 2026-07-11T17:28:30.722Z
Learning: In src/modules/ExternalNotificationModule.cpp (meshtastic/firmware), the `rtttlConfig` global is only declared under `#if !MESHTASTIC_EXCLUDE_RTTTL`. Any call site referencing `rtttlConfig` or RTTTL playback (e.g., `audioThread->beginRttl`) guarded by `#ifdef HAS_I2S` must also check `!MESHTASTIC_EXCLUDE_RTTTL` (i.e., `#if defined(HAS_I2S) && !MESHTASTIC_EXCLUDE_RTTTL`) to avoid compile breakage on future HAS_I2S platforms that exclude RTTTL.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

ExternalNotificationModule already stubbed out RTTTL playback for STM32WL/portduino/ESP32C6 via a raw ARCH/CONFIG_IDF check, but the ringtone config plumbing around it (protobuf message, encode/decode tables, /prefs/ringtone.proto persistence, admin get/set-ringtone handlers) still compiled in even though it can never do anything on those platforms. Introduce MESHTASTIC_EXCLUDE_RTTTL and gate the dead ringtone plumbing behind it too. The flag is set in each architecture's *_base build_flags (stm32_base, esp32c6_base, portduino_base) rather than in the module itself - this matches how every other MESHTASTIC_EXCLUDE_* flag in the tree is set (e.g. stm32_base already sets ten of them directly, and esp32c6_base already excludes PAXCOUNTER for an analogous platform-can't-support-this reason), rather than introducing a new per-architecture C header pattern. Behavior is unchanged on all three platforms; overridable via -D like every other MESHTASTIC_EXCLUDE_* flag. Also guard the two HAS_I2S ringtone-playback call sites with !MESHTASTIC_EXCLUDE_RTTTL alongside HAS_I2S, since rtttlConfig itself is now only declared when RTTTL is not excluded. No current platform defines both HAS_I2S and MESHTASTIC_EXCLUDE_RTTTL simultaneously, so this has no effect today, but prevents a future HAS_I2S platform that also excludes RTTTL from failing to compile. Saves 368 bytes flash / 236 bytes RAM on wio-e5 with no loss to the GPIO on/off notification toggle itself, which does not depend on RTTTL. Signed-off-by: Andrew Yong <me@ndoo.sg> Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>

The inputObserver CallbackObserver member was declared unconditionally, even though its only use site was already gated behind MESHTASTIC_EXCLUDE_INPUTBROKER (set for all of stm32 in stm32.ini). Because it's a non-trivial member, the compiler still generated its constructor/destructor as part of ExternalNotificationModule's own lifecycle even when InputBroker is compiled out entirely. Gate the member and its only consumer, handleInputEvent(), behind the same flag as their use site, and match the codebase's dominant !MESHTASTIC_EXCLUDE_X style (used ~330 times) rather than !defined(MESHTASTIC_EXCLUDE_X) (used ~20 times) while touching this flag's other call site. Saves an additional 288 bytes flash on wio-e5, no RAM change, no functional impact since InputBroker was already unused on this platform. Signed-off-by: Andrew Yong <me@ndoo.sg> Assisted-by: Claude Sonnet 5 <noreply@anthropic.com>

ndoo · 2026-07-11T18:00:04Z

Moved MESHTASTIC_EXCLUDE_RTTTL's definition from src/platform/{stm32wl,portduino,esp32}/architecture.h into each architecture's *_base build_flags (stm32_base, esp32c6_base, portduino_base) instead.

thebentern · 2026-07-16T13:56:23Z

@ndoo can you fix the conflicts

meshtastic#10992 landed on develop after this branch forked, inserting new MESHTASTIC_EXCLUDE_* flags at the same point this PR adds MESHTASTIC_EXCLUDE_RTTTL. Combine both sets and alphabetize the whole exclude-flag block in stm32_base's build_flags. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg>

ndoo · 2026-07-16T15:25:28Z

@ndoo can you fix the conflicts

Done and sorted the excludes list while we're touching the file

ndoo force-pushed the stm32wl-externalnotification-flash-savings branch 2 times, most recently from b4b69ab to 05f8ac6 Compare July 11, 2026 17:03

ndoo changed the title ~~stm32wl: reduce ExternalNotificationModule flash usage (RTTTL + InputBroker)~~ Reduce ExternalNotificationModule flash usage (RTTTL + InputBroker) Jul 11, 2026

coderabbitai Bot reviewed Jul 11, 2026

View reviewed changes

Comment thread src/modules/ExternalNotificationModule.cpp

ndoo force-pushed the stm32wl-externalnotification-flash-savings branch from 05f8ac6 to 2cfd6a9 Compare July 11, 2026 17:27

ndoo marked this pull request as ready for review July 11, 2026 17:37

ndoo added 2 commits July 12, 2026 01:57

ndoo force-pushed the stm32wl-externalnotification-flash-savings branch from 2cfd6a9 to aff3a3b Compare July 11, 2026 17:59

Uh oh!

Uh oh!

Conversation

ndoo commented Jul 11, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What changed

Testing

🤝 Attestations

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Jul 11, 2026

⚡ Try this PR in the Web Flasher

Uh oh!

ndoo commented Jul 11, 2026

Uh oh!

coderabbitai Bot commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ndoo commented Jul 11, 2026

Uh oh!

coderabbitai Bot commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ndoo commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thebentern commented Jul 16, 2026

Uh oh!

ndoo commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ndoo commented Jul 11, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 11, 2026 •

edited

Loading

coderabbitai Bot commented Jul 11, 2026 •

edited

Loading

coderabbitai Bot commented Jul 11, 2026 •

edited

Loading

ndoo commented Jul 11, 2026 •

edited

Loading