sound: a switch that gives the memory back, and a level for the board that has one - #118
Merged
Merged
Conversation
… that has one The sounder was a build-time fact. On the board whose sounder is a speaker that made it an awkward one: its task and DMA ring are about 5.4 KB of internal RAM, which on that board is enough to stop the portal serving, so choosing between a chime and a web interface meant reflashing. sound.enabled is a setting now, and switching it off releases the hardware rather than merely silencing it — which is the only version of the switch worth having. Measured on the T-Deck, live, without a restart: 18972 bytes free with it off, 11672 with it on, 18940 when switched off again. The audio task stands down rather than being killed. A sentinel note reaches it only between notes, and it deletes itself there; deleting it by handle could land inside an I2S write and leave the driver's state to chance. If it does not answer within a second the sounder is left up and says so, because a wedged task holding a queue that has just been freed is a worse outcome than a kilobyte unreclaimed. sound.volume scales the speaker's amplitude. On a piezo it is accepted and ignored, deliberately: a PWM element has one loudness, duty cycle changes the timbre rather than the level, and a fleet should be able to carry one configuration across boards that answer it differently. Defaults follow the cost. A piezo is a channel and a timer, so those boards ship on and behave as they always did; the speaker ships off, and turning it on is a choice made knowing what it spends. Both controls appear on the device's settings screen without that file being touched, which is the point of its being generated from the key table — the volume as a slider, beside the display's brightness. Verified on both boards: the T-Deck for the memory going and coming back, the M9 for the piezo path and for the defaults being what they claim. The device screen was verified by somebody moving the slider on the glass while this was being written, which is a better test than the one that was planned.
There was a problem hiding this comment.
🟡 Changes recommended
The PWM take-up path can mark the sounder “up” even when the timer creation fails, leaving sound enabled but non-functional and preventing apply() from retrying initialization.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds runtime-configurable sound settings so speaker-based boards can reclaim internal RAM by fully releasing the audio task/I2S resources when sound is disabled, while also introducing a volume control (effective on I2S speaker boards and intentionally ignored on piezo/PWM boards).
Changes:
- Introduces
sound.enabledandsound.volumesettings fields and persists them in NVS. - Updates the buzzer subsystem to (idempotently) apply settings at runtime, including taking up/releasing hardware and scaling I2S amplitude.
- Exposes
sound.volumeas an LVGL slider (alongside existing brightness slider behavior).
File summaries
| File | Description |
|---|---|
| src/ui/lvgl/UiSettings.cpp | Renders sound.volume as a slider control in the settings UI. |
| src/sys/SettingsFields.cpp | Adds runtime-configurable sound.enabled and sound.volume settings handlers and applies them immediately via Buzzer::apply(). |
| src/sys/Settings.h | Adds SoundSettings to the settings model and public accessor/save API. |
| src/sys/Settings.cpp | Loads/saves sound settings to NVS keys snd_en / snd_vol. |
| src/sys/Buzzer.h | Adds Buzzer::apply() and Buzzer::present() APIs for runtime hardware management. |
| src/sys/Buzzer.cpp | Implements runtime apply/release behavior and volume scaling (I2S), plus safe stand-down of the audio task. |
| src/Config.h | Adds defaults for sound enabled/volume based on buzzer kind (I2S vs PWM). |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
293
to
296
| if (esp_timer_create(&args, &sTimer) != ESP_OK) sTimer = nullptr; | ||
| #endif | ||
| sUp = true; | ||
| } |
Comment on lines
+608
to
+610
| // Percent. Only the speaker can act on it: a piezo is driven at the one | ||
| // amplitude its PWM channel produces, so the setting is offered where it means | ||
| // something and refused where it does not. |
…reated Every failure in taking up the hardware returned early except one. Without the timer a note starts and never ends, so start() refuses and the sounder is silent — but the flag saying it was up got set anyway, three lines further down and past the #endif that hid it from the branch it belonged to. Two things follow from that flag, and both are wrong in this state. present() reports a sounder that cannot make a sound. And apply() compares the setting against it to decide whether anything needs doing, so a sounder that failed to start would never be tried again — a transient failure made permanent until the next restart. It gives the pin back and stays down now, and says so. The I2S path was already right: each of its failures returns before the flag. This was the one branch where the early return had been left out and the shared line at the end covered for it. From Copilot's review of #118. Its second comment quotes the volume comment back rather than finding anything, so there is nothing to answer there. Verified on the M9, which is the board with the piezo: the sounder comes up, goes off and comes back with no refusal on either transition.
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
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.
The sounder was a build-time fact. On the board whose sounder is a speaker that made it an awkward one: its task and DMA ring are about 5.4 KB of internal RAM, which on that board is enough to stop the portal serving, so choosing between a chime and a web interface meant reflashing.
sound.enabled is a setting now, and switching it off releases the hardware rather than merely silencing it — which is the only version of the switch worth having. Measured on the T-Deck, live, without a restart: 18972 bytes free with it off, 11672 with it on, 18940 when switched off again.
The audio task stands down rather than being killed. A sentinel note reaches it only between notes, and it deletes itself there; deleting it by handle could land inside an I2S write and leave the driver's state to chance. If it does not answer within a second the sounder is left up and says so, because a wedged task holding a queue that has just been freed is a worse outcome than a kilobyte unreclaimed.
sound.volume scales the speaker's amplitude. On a piezo it is accepted and ignored, deliberately: a PWM element has one loudness, duty cycle changes the timbre rather than the level, and a fleet should be able to carry one configuration across boards that answer it differently.
Defaults follow the cost. A piezo is a channel and a timer, so those boards ship on and behave as they always did; the speaker ships off, and turning it on is a choice made knowing what it spends.
Both controls appear on the device's settings screen without that file being touched, which is the point of its being generated from the key table — the volume as a slider, beside the display's brightness.
Verified on both boards: the T-Deck for the memory going and coming back, the M9 for the piezo path and for the defaults being what they claim. The device screen was verified by somebody moving the slider on the glass while this was being written, which is a better test than the one that was planned.