-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix(nrf52): drive the PWM RTTTL sequencer from a FreeRTOS timer #11613
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 4 commits
8b2d197
75f9e49
38d845b
6408bcd
a72ae0b
f6a54c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,9 @@ bool ascending = true; | |
| #if defined(HAS_I2S_SPEAKER_NRF52) | ||
| #include "platform/nrf52/NRF52RtttlPlayer.h" | ||
| #endif | ||
| #ifdef ARCH_NRF52 | ||
| #include "platform/nrf52/NRF52RtttlTicker.h" | ||
| #endif | ||
|
|
||
| /* | ||
| Documentation: | ||
|
|
@@ -60,6 +63,35 @@ bool ascending = true; | |
|
|
||
| #define EXT_NOTIFICATION_FAST_THREAD_MS 25 | ||
|
|
||
| // The PWM buzzer sequencer is normally polled from this cooperative thread, so a slow display refresh | ||
| // delays the next note. nRF52 runs it from a FreeRTOS timer instead (NRF52RtttlTicker). | ||
| static void pwmRtttlBegin(uint8_t pin, const char *song) | ||
| { | ||
| #ifdef ARCH_NRF52 | ||
| NRF52RtttlTicker::begin(pin, song); | ||
| #else | ||
| rtttl::begin(pin, song); | ||
| #endif | ||
| } | ||
|
|
||
| static void pwmRtttlPump() | ||
| { | ||
| #ifdef ARCH_NRF52 | ||
| NRF52RtttlTicker::pump(); | ||
| #else | ||
| rtttl::play(); | ||
| #endif | ||
| } | ||
|
|
||
| static void pwmRtttlStop() | ||
| { | ||
| #ifdef ARCH_NRF52 | ||
| NRF52RtttlTicker::stop(); | ||
| #else | ||
| rtttl::stop(); | ||
| #endif | ||
| } | ||
|
Comment on lines
+66
to
+93
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect the sequencer implementation and all direct state queries.
fd -a -t f '^NonBlockingRtttl\.(h|hpp|cpp|cc)$' . -x sh -c '
echo "== $1 =="
rg -n -C 5 "isPlaying|play\\s*\\(" "$1"
' sh {}
rg -n -C 3 'rtttl::isPlaying|NRF52RtttlTicker::' \
src/modules/ExternalNotificationModule.cpp src/platform/nrf52/NRF52RtttlTicker.cppRepository: meshtastic/firmware Length of output: 3902 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== repository instructions =="
cat .github/copilot-instructions.md
echo "== available convention scopes =="
for f in /tmp/coderabbit-repo-knowledge/meshtastic-firmware-d6b70ec8/conventions/*.md; do
echo "== $f =="
head -5 "$f"
done
echo "== relevant files =="
fd -a -t f 'NonBlockingRtttl\.(h|hpp|cpp|cc)$|NRF52RtttlTicker\.cpp$|ExternalNotificationModule\.cpp$' .
echo "== sequencer declarations and definitions =="
fd -a -t f 'NonBlockingRtttl\.(h|hpp|cpp|cc)$' . -x sh -c '
echo "== $1 =="
cat -n "$1"
' sh {}
echo "== nRF52 ticker implementation =="
cat -n src/platform/nrf52/NRF52RtttlTicker.cpp
echo "== ExternalNotificationModule control flow =="
sed -n '95,125p;190,215p' src/modules/ExternalNotificationModule.cppRepository: meshtastic/firmware Length of output: 50377 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== RTTTL files =="
fd -a -t f 'NonBlockingRtttl\.(h|hpp|cpp|cc)$|NRF52RtttlTicker\.(h|hpp|cpp|cc)$' .
echo "== sequencer symbols =="
for f in $(fd -a -t f 'NonBlockingRtttl\.(h|hpp|cpp|cc)$'); do
echo "== $f =="
rg -n -C 12 'isPlaying|play\s*\(' "$f"
done
echo "== ticker declarations and implementation =="
for f in $(fd -a -t f 'NRF52RtttlTicker\.(h|hpp|cpp|cc)$'); do
echo "== $f =="
cat -n "$f"
done
echo "== module includes and queried paths =="
sed -n '1,105p;105,125p;195,212p' src/modules/ExternalNotificationModule.cppRepository: meshtastic/firmware Length of output: 7967 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== RTTTL-related files =="
fd -a -t f -i 'rtttl|nonblocking' . | head -100
echo "== isPlaying declarations and definitions =="
rg -n -C 8 --glob '!src/platform/nrf52/NRF52RtttlTicker.cpp' \
'(^|[^[:alnum:]_])isPlaying\s*\(|bool[[:space:]]+isPlaying|volatile.*playing|playing[[:space:]]*=' \
src lib libraries 2>/dev/null || true
echo "== build dependency references =="
rg -n -C 4 'NonBlockingRtttl|NonBlockingRTTTL|rtttl' platformio.ini variants src lib libraries 2>/dev/null | head -240Repository: meshtastic/firmware Length of output: 39908 🌐 Web query:
💡 Result: The NonBlockingRTTTL library (authored by end2endzone) is a non-blocking Arduino library designed to play RTTTL (Ring Tone Text Transfer Protocol) melodies without halting the execution of other code [1][2]. The library enables multitasking, such as monitoring inputs or sensors, while audio is playing [1][3]. In the library header, NonBlockingRtttl.h, the core functionality is accessed through the rtttl namespace [1]. Key functions include: - rtttl::begin(pin, melody): Initializes the library with a specified buzzer pin and RTTTL melody string [1][4]. - rtttl::play: This function must be called repeatedly in the main loop to update the library's state and process note timing [1][4]. - rtttl::isPlaying: Returns true if a melody is currently playing, and false when the song finishes [1][4]. - rtttl::stop: Immediately stops the currently playing melody [1][4]. - rtttl::done: An alternative to isPlaying to check if the playback has finished [1][4]. Version 1.4.0 is a specific release of this library [5]. A typical implementation pattern in the loop function is to check if the library is playing; if not, you may trigger the next melody, otherwise, call rtttl::play to ensure playback continues [1][4]. Example implementation structure: void loop { if (!rtttl::isPlaying) { // Logic to start next song using rtttl::begin } else { rtttl::play; // Essential to call this to progress the melody } } [1][4] Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
tree="$(curl -fsSL 'https://api.github.com/repos/end2endzone/NonBlockingRTTTL/git/trees/1.4.0?recursive=1')"
printf '%s\n' "$tree" | jq -r '.tree[] | select(.path|test("Rtttl|rtttl")) | .path'
for path in $(printf '%s\n' "$tree" | jq -r '.tree[] | select(.path|test("Rtttl|rtttl")) | .path'); do
echo "== $path =="
curl -fsSL "https://raw.githubusercontent.com/end2endzone/NonBlockingRTTTL/1.4.0/$path" |
rg -n -C 12 'isPlaying|play\s*\(|playing|current' || true
doneRepository: meshtastic/firmware Length of output: 9362 Synchronize the PWM playback-state query.
🤖 Prompt for AI Agents |
||
|
|
||
| #define ASCII_BELL 0x07 | ||
|
|
||
| meshtastic_RTTTLConfig rtttlConfig; | ||
|
|
@@ -78,6 +110,8 @@ int32_t ExternalNotificationModule::runOnce() | |
| return INT32_MAX; // we don't need this thread here... | ||
| } else { | ||
| uint32_t delay = EXT_NOTIFICATION_MODULE_OUTPUT_MS; | ||
| // Racy by design: the sequencer's flag is one byte, stale only for a cycle at song end, which | ||
| // just defers stopNow(). Locking it would block this loop on the timer task it hands work to. | ||
| bool isRtttlPlaying = rtttl::isPlaying(); | ||
| #ifdef HAS_I2S | ||
| // audioThread->isPlaying() also handles actually playing the RTTTL, needs to be called in loop | ||
|
|
@@ -172,10 +206,10 @@ int32_t ExternalNotificationModule::runOnce() | |
| // now let the PWM buzzer play | ||
| if (moduleConfig.external_notification.use_pwm && config.device.buzzer_gpio && canBuzz() && buzzerShouldAlert) { | ||
| if (rtttl::isPlaying()) { | ||
| rtttl::play(); | ||
| pwmRtttlPump(); | ||
| } else if (isNagging && !Throttle::deadlinePassed(nagCycleCutoff)) { | ||
| // start the song again if we have time left | ||
| rtttl::begin(config.device.buzzer_gpio, rtttlConfig.ringtone); | ||
| pwmRtttlBegin(config.device.buzzer_gpio, rtttlConfig.ringtone); | ||
| } | ||
| // we need fast updates to play the RTTTL | ||
| delay = EXT_NOTIFICATION_FAST_THREAD_MS; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On nRF52 the module thread no longer sequences anything on this path, it only polls for song end and nag restart, so the 25 ms rate is now dead weight. Not a bug, and fine to leave if you want the diff scoped to one concern.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Left as is -- the 25 ms poll is what bounds the nag restart. At EXT_NOTIFICATION_MODULE_OUTPUT_MS (1000 ms) a nagging ringtone would get up to a second of silence between repeats. Worth revisiting if the nag path gets restructured, but it'd need its own timing rather than falling back to the slow rate. |
||
|
|
@@ -283,7 +317,7 @@ void ExternalNotificationModule::stopNow() | |
| { | ||
| LOG_INFO("Turning off external notification: "); | ||
| LOG_INFO("Stop RTTTL playback"); | ||
| rtttl::stop(); | ||
| pwmRtttlStop(); | ||
| #ifdef HAS_I2S | ||
| LOG_INFO("Stop audioThread playback"); | ||
| audioThread->stop(); | ||
|
|
@@ -488,7 +522,7 @@ void ExternalNotificationModule::triggerBuzzerOutput() | |
| audioThread->beginRttl(rtttlConfig.ringtone, strlen_P(rtttlConfig.ringtone)); | ||
| #endif | ||
| } else if (moduleConfig.external_notification.use_pwm) { | ||
| rtttl::begin(config.device.buzzer_gpio, rtttlConfig.ringtone); | ||
| pwmRtttlBegin(config.device.buzzer_gpio, rtttlConfig.ringtone); | ||
| } else { | ||
| setExternalState(2, true); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| #include "NRF52RtttlTicker.h" | ||
|
|
||
| #ifdef ARCH_NRF52 | ||
|
|
||
| #include "DebugConfiguration.h" | ||
| #include "freertosinc.h" | ||
| #include <NonBlockingRtttl.h> | ||
| #include <timers.h> | ||
|
|
||
| namespace NRF52RtttlTicker | ||
| { | ||
| namespace | ||
| { | ||
| // The stall this works around is tens of milliseconds, so 5 ms is inaudible and keeps the timer | ||
| // daemon at 200 wakeups/s. Must stay above one tick (configTICK_RATE_HZ is 1024 here) or | ||
| // pdMS_TO_TICKS() rounds to zero, xTimerCreate() fails, and we silently drop back to polling. | ||
| constexpr uint32_t kTickMs = 5; | ||
| static_assert(kTickMs * configTICK_RATE_HZ >= 1000, "kTickMs rounds to zero ticks"); | ||
|
|
||
| TimerHandle_t timer = nullptr; | ||
| SemaphoreHandle_t lock = nullptr; | ||
| // True only while the timer is servicing a song; pump() polls from the main loop whenever it is not. | ||
| bool timerRunning = false; | ||
| bool reportedInitFailure = false; | ||
|
|
||
| void onTick(TimerHandle_t) | ||
| { | ||
| // The main thread holds the lock only across begin()/stop(); skip this tick rather than block the timer task. | ||
| if (xSemaphoreTake(lock, 0) != pdTRUE) | ||
| return; | ||
| // onTick -> rtttl::play -> tone -> applyConfiguration is the deepest chain on the timer daemon's | ||
| // 256 word stack, shared with Bluefruit; measured 61 words peak on a T-Echo Plus. | ||
| if (rtttl::isPlaying()) | ||
| rtttl::play(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The mutex covers NonBlockingRTTTL's state, not Tone.cpp's.
Failure mode: the main thread is preempted between One line in if (rtttl::isPlaying())
return; // a notification ringtone owns the buzzer
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Took your one-liner (38d845b), scoped to the tone() loop in a72ae0b. rtttl::isPlaying() only tracks the PWM sequencer, and runOnce() starts the I2S and PWM ringtone paths from independent conditions — only triggerBuzzerOutput() treats them as exclusive — so above the I2S branches it could silence I2S system tones on a board where PWM rtttl happened to be running. NonBlockingRtttl.h is only a lib_dep under arduino_base, and ExternalNotificationModule.h carries a stub rtttl class for Portduino/STM32WL. Rather than pull that header into buzz.cpp I mirrored the guard, so the include and the check are both behind !ARCH_PORTDUINO && !ARCH_STM32WL. |
||
| else | ||
| xTimerStop(timer, 0); // song finished on its own | ||
| xSemaphoreGive(lock); | ||
| } | ||
|
|
||
| bool ensureInit() | ||
| { | ||
| if (timer) | ||
| return true; | ||
| if (!lock) | ||
| lock = xSemaphoreCreateMutex(); | ||
| if (lock) | ||
| timer = xTimerCreate("rtttl", pdMS_TO_TICKS(kTickMs), pdTRUE, nullptr, onTick); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Measured on a T-Echo Plus: uxTaskGetStackHighWaterMark(xTimerGetTimerDaemonTaskHandle()) logged once after a ringtone finishes reports 195 of 256 words free, so onTick -> rtttl::play -> nextnote -> tone -> applyConfiguration peaks at 61 words / 244 bytes — with Bluefruit's timer callbacks sharing the stack. Dropped the probe and left the figure as a comment on onTick (6408bcd) so anything that deepens that chain later has a baseline. |
||
| if (!timer && !reportedInitFailure) { | ||
| // begin() runs again on every nag restart, so only complain the first time. | ||
| reportedInitFailure = true; | ||
| LOG_ERROR("RTTTL timer unavailable, falling back to main-loop playback"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once creation fails this logs on every
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One-shot flag in ensureInit() (38d845b). Kept it there rather than at the caller so the fallback path in begin() stays a single branch. |
||
| } | ||
| return timer != nullptr; | ||
| } | ||
| } // namespace | ||
|
|
||
| void begin(uint8_t pin, const char *song) | ||
| { | ||
| if (!ensureInit()) { | ||
| rtttl::begin(pin, song); | ||
| return; | ||
| } | ||
| xSemaphoreTake(lock, portMAX_DELAY); | ||
| rtttl::begin(pin, song); | ||
| xSemaphoreGive(lock); | ||
| // A start rejected by a full timer command queue must fall back to polling, or the song never advances. | ||
| timerRunning = xTimerStart(timer, pdMS_TO_TICKS(10)) == pdPASS; | ||
| if (!timerRunning) | ||
| LOG_WARN("RTTTL timer start rejected, falling back to main-loop playback"); | ||
| } | ||
|
|
||
| void pump() | ||
| { | ||
| if (!timerRunning && rtttl::isPlaying()) | ||
| rtttl::play(); | ||
| } | ||
|
|
||
| void stop() | ||
| { | ||
| if (!timer) { | ||
| rtttl::stop(); | ||
| return; | ||
| } | ||
| // Unlike the start, a rejected stop is harmless: rtttl::stop() below clears the playing flag, | ||
| // so the next onTick() stops the timer itself. | ||
| (void)xTimerStop(timer, pdMS_TO_TICKS(10)); | ||
| timerRunning = false; | ||
| xSemaphoreTake(lock, portMAX_DELAY); | ||
| rtttl::stop(); | ||
| xSemaphoreGive(lock); | ||
| } | ||
| } // namespace NRF52RtttlTicker | ||
|
|
||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| #pragma once | ||
|
|
||
| #include "configuration.h" | ||
|
|
||
| #ifdef ARCH_NRF52 | ||
|
|
||
| #include <stdint.h> | ||
|
|
||
| // Advances the NonBlockingRTTTL sequencer from a FreeRTOS timer, so the next note does not wait | ||
| // behind the cooperative main loop. tone() is hardware timed, so only note starts need servicing. | ||
| namespace NRF52RtttlTicker | ||
| { | ||
| void begin(uint8_t pin, const char *song); | ||
|
|
||
| // Only advances the song if the timer could not be created or started; otherwise a no-op. | ||
| void pump(); | ||
|
|
||
| void stop(); | ||
| } // namespace NRF52RtttlTicker | ||
|
|
||
| #endif |
Uh oh!
There was an error while loading. Please reload this page.