Add configurable startup notification sounds and UI controls#24
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two notification sound path settings (success/failure) across configs, C++ types, service logic, web API, and frontend; ChimeService now evaluates WiFi/MQTT at startup and may play a one-time success or failure notification within a startup timeout. Changes
Sequence DiagramsequenceDiagram
participant Service as ChimeService
participant WiFi as WiFiMonitor
participant MQTT as MQTTMonitor
participant Audio as AudioSystem
Service->>WiFi: Read initial WiFi state
WiFi-->>Service: Return WifiState (optional)
Service->>MQTT: Query MQTT connection status
MQTT-->>Service: Return MQTT status
Service->>Service: WifiStateIsConnected(state)?
Service->>Service: Evaluate MQTT + WiFi + elapsed startup time
alt decide success
Service->>Audio: PlayNotification(kSuccess)
else decide failure
Service->>Audio: PlayNotification(kFailure)
else wait / poll
Service->>Service: Continue polling until timeout
end
Audio-->>Service: Playback result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
webui/src/App.svelte (1)
577-590: Add client-sidemaxlengthto match backend validation.This avoids avoidable save failures and gives faster feedback in the form.
🧩 Suggested UI tweak
<input id="notification_success_sound_path" bind:value={notificationSuccessSoundPath} + maxlength="256" placeholder="/usr/local/share/chime/test.wav" /> @@ <input id="notification_failure_sound_path" bind:value={notificationFailureSoundPath} + maxlength="256" placeholder="/usr/local/share/chime/ring.wav" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webui/src/App.svelte` around lines 577 - 590, Add maxlength attributes to the success and failure sound path inputs to mirror backend validation so users get immediate feedback and avoid save errors; locate the two input elements bound to notificationSuccessSoundPath and notificationFailureSoundPath in App.svelte and add the same maxlength value used by the server-side validation for sound path fields (apply the identical numeric limit to both inputs).chime/src/webd/web_server.cpp (1)
1043-1046: Consider optional parsing for backward compatibility of/api/v1/config/core.Requiring these new fields can reject older clients unnecessarily. Using optional parsing keeps existing clients functional while still accepting new values.
♻️ Suggested refactor
- const auto notification_success_sound_path = - ReadRequiredString(parsed.value, "notification_success_sound_path", &parse_errors); - const auto notification_failure_sound_path = - ReadRequiredString(parsed.value, "notification_failure_sound_path", &parse_errors); + const auto notification_success_sound_path = + ReadOptionalString(parsed.value, "notification_success_sound_path", &parse_errors); + const auto notification_failure_sound_path = + ReadOptionalString(parsed.value, "notification_failure_sound_path", &parse_errors); @@ - save_request.config.notification_success_sound_path = *notification_success_sound_path; - save_request.config.notification_failure_sound_path = *notification_failure_sound_path; + if (notification_success_sound_path.has_value()) { + save_request.config.notification_success_sound_path = *notification_success_sound_path; + } + if (notification_failure_sound_path.has_value()) { + save_request.config.notification_failure_sound_path = *notification_failure_sound_path; + }Also applies to: 1075-1076
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chime/src/webd/web_server.cpp` around lines 1043 - 1046, The parser currently calls ReadRequiredString for notification_success_sound_path and notification_failure_sound_path (and the similar calls at the later block around lines 1075-1076), which forces these new fields and breaks older clients; change those calls to the optional variant (e.g., ReadOptionalString or equivalent) and update any downstream code that assumes these values are present to handle absence (use optional/empty string/default path) so backward compatibility is preserved while still accepting new values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@chime/include/chime/chime_service.h`:
- Around line 37-43: The header has clang-format style violations; run the
project’s clang-format configuration against this header (or reformat the
declarations manually) so the enum NotificationSoundType and the method
declarations LogWifiState, LogHealth, RingTopicMatches, WifiStateIsConnected,
and PlayNotification follow the repo’s formatting rules (spacing, alignment, and
trailing semicolons consistent with other headers); ensure the file is saved
after formatting and re-run CI to confirm the violations are resolved.
In `@chime/include/chime/webd_types.h`:
- Around line 29-30: Run clang-format on chime/include/chime/webd_types.h to fix
formatting violations reported by CI; reformat the header (including the lines
defining notification_success_sound_path and notification_failure_sound_path) so
it conforms to the project’s clang-format rules and commit the updated file.
In `@chime/src/service/chime_service.cpp`:
- Around line 209-210: CI flagged a clang-format violation in chime_service.cpp
around the creation of startup_elapsed; run clang-format (or apply the project's
formatting rules) to reformat the expression that computes startup_elapsed using
std::chrono::duration_cast from now and startup_begin so it matches the
repository style. Locate the occurrence of "startup_elapsed", "startup_begin",
"now" and the chrono::duration_cast expression in chime_service.cpp and reformat
the lines (or run the formatter tool) until clang-format no longer reports an
error.
- Around line 144-150: Startup can incorrectly report WiFi failure because
last_wifi_state may be empty/stale when deriving wifi_connected; update the
startup/state-check logic (the wifi_monitor_.ReadState call, LogWifiState usage,
and any code that derives wifi_connected from last_wifi_state) to treat
absent/unknown state as "unknown" rather than "disconnected": when ReadState
returns no value or the monitor is unavailable, do not set last_wifi_state to a
failing state or flip wifi_connected to false—leave last_wifi_state as
std::nullopt (or explicit Unknown) and change any boolean derivation of
wifi_connected to require last_wifi_state.has_value() &&
last_wifi_state->connected; ensure startup notification logic only reports
success/failure when a definite state exists (otherwise emit an "unknown" or
defer notification), and apply the same fix pattern to the other occurrences
mentioned (the blocks around where LogWifiState/last_wifi_state and
wifi_connected are used).
- Around line 395-400: The WifiStateIsConnected predicate incorrectly treats
carrier == -1 (unknown) as connected because it checks carrier != 0; update
ChimeService::WifiStateIsConnected to treat only carrier == 1 as indicating a
live carrier. Specifically, in WifiStateIsConnected(const
std::optional<WifiState>& state) replace the carrier check (state->carrier != 0)
with an explicit equality check (state->carrier == 1) so only a known "up"
carrier reports connected while -1 (unknown) and 0 (down) return false.
In `@chime/src/webd/config_store.cpp`:
- Around line 451-460: The current validation in config_store.cpp for
request.config.notification_success_sound_path and
notification_failure_sound_path only checks length and emptiness; update it to
reject values that are whitespace-only or contain CR/LF characters: trim the
value (or check that it contains at least one non-whitespace char), verify the
trimmed length is between 1 and 256, and explicitly check for '\n' or '\r' (or
any control newlines) and push an error (same field name) if found so these
invalid values are not persisted.
---
Nitpick comments:
In `@chime/src/webd/web_server.cpp`:
- Around line 1043-1046: The parser currently calls ReadRequiredString for
notification_success_sound_path and notification_failure_sound_path (and the
similar calls at the later block around lines 1075-1076), which forces these new
fields and breaks older clients; change those calls to the optional variant
(e.g., ReadOptionalString or equivalent) and update any downstream code that
assumes these values are present to handle absence (use optional/empty
string/default path) so backward compatibility is preserved while still
accepting new values.
In `@webui/src/App.svelte`:
- Around line 577-590: Add maxlength attributes to the success and failure sound
path inputs to mirror backend validation so users get immediate feedback and
avoid save errors; locate the two input elements bound to
notificationSuccessSoundPath and notificationFailureSoundPath in App.svelte and
add the same maxlength value used by the server-side validation for sound path
fields (apply the identical numeric limit to both inputs).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
buildroot/board/raspberrypi0w/rootfs_overlay/etc/chime.confchime/include/chime/chime_config.hchime/include/chime/chime_service.hchime/include/chime/webd_types.hchime/src/config/chime_config.cppchime/src/service/chime_service.cppchime/src/webd/config_store.cppchime/src/webd/web_server.cppwebui/src/App.svelte
Comment on lines
+37
to
+43
| enum class NotificationSoundType { kSuccess, kFailure }; | ||
|
|
||
| void LogWifiState(const WifiState& state) const; | ||
| void LogHealth(bool clock_sane); | ||
| bool RingTopicMatches(const std::string& message_topic) const; | ||
| bool WifiStateIsConnected(const std::optional<WifiState>& state) const; | ||
| void PlayNotification(NotificationSoundType type); |
There was a problem hiding this comment.
Run clang-format on this header to fix CI.
chime CI reports clang-format violations for this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@chime/include/chime/chime_service.h` around lines 37 - 43, The header has
clang-format style violations; run the project’s clang-format configuration
against this header (or reformat the declarations manually) so the enum
NotificationSoundType and the method declarations LogWifiState, LogHealth,
RingTopicMatches, WifiStateIsConnected, and PlayNotification follow the repo’s
formatting rules (spacing, alignment, and trailing semicolons consistent with
other headers); ensure the file is saved after formatting and re-run CI to
confirm the violations are resolved.
Comment on lines
+29
to
+30
| std::string notification_success_sound_path = "/usr/local/share/chime/test.wav"; | ||
| std::string notification_failure_sound_path = "/usr/local/share/chime/ring.wav"; |
There was a problem hiding this comment.
Run clang-format on this header to unblock CI.
chime CI reports clang-format violations for this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@chime/include/chime/webd_types.h` around lines 29 - 30, Run clang-format on
chime/include/chime/webd_types.h to fix formatting violations reported by CI;
reformat the header (including the lines defining
notification_success_sound_path and notification_failure_sound_path) so it
conforms to the project’s clang-format rules and commit the updated file.
…r-mention-tw-14-beep-sounds-after-bootup
…r-mention-tw-14-beep-sounds-after-bootup
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@chime/src/service/chime_service.cpp`:
- Around line 212-236: The startup notification defer branch can hang forever if
last_wifi_state remains unset; modify the logic in the block that checks
startup_elapsed and wifi_state_known so that when wifi_state_known is false you
either (a) start a secondary timeout counter (e.g., record a new timestamp or
compare now against a new constant) and when that secondary timeout expires call
PlayNotification(NotificationSoundType::kFailure) and set
startup_notification_played = true, or (b) simply set
startup_notification_played = true after a configured grace period to stop
repeated deferrals; update startup_notification_unknown_logged usage to keep
one-time logging but ensure either PlayNotification or
startup_notification_played is set once the secondary timeout/grace period
passes so the loop cannot defer indefinitely (changes touch the logic around
last_wifi_state, WifiStateIsConnected, startup_notification_unknown_logged,
startup_notification_played, kStartupNotificationTimeoutSeconds,
PlayNotification and NotificationSoundType::kFailure).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d49cbf0-1955-469a-b3d1-73fcd9ceedbe
📒 Files selected for processing (4)
chime/src/service/chime_service.cppchime/src/webd/config_store.cppchime/src/webd/web_server.cppwebui/src/App.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- chime/src/webd/web_server.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (1)
chime/src/service/chime_service.cpp (1)
145-149:⚠️ Potential issue | 🟠 MajorRefresh WiFi state during startup checks to avoid false failure from stale state.
At Line 216,
wifi_connectedis computed fromlast_wifi_state, but that value may be stale until the next periodic poll. Ifwifi_check_intervalis greater than 10s (or effectively disabled), startup can emit failure at Lines 245-249 even when WiFi recovered before timeout.💡 Proposed fix
- if (!startup_notification_played) { + if (!startup_notification_played) { + if (const auto refreshed_state = wifi_monitor_.ReadState(config_.wifi_interface); + refreshed_state.has_value()) { + if (WifiStateChanged(last_wifi_state, *refreshed_state)) { + LogWifiState(*refreshed_state); + } + last_wifi_state = refreshed_state; + } + const bool wifi_state_known = last_wifi_state.has_value(); const bool wifi_connected = wifi_state_known && WifiStateIsConnected(last_wifi_state); const bool mqtt_connected = mqtt_connected_.load();Also applies to: 197-201, 214-218, 224-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chime/src/service/chime_service.cpp` around lines 145 - 149, The startup path uses last_wifi_state which can be stale; before computing wifi_connected during startup checks (where wifi_check_interval may be large/disabled) call wifi_monitor_.ReadState(config_.wifi_interface) again, and if it has_value() pass it to LogWifiState and assign last_wifi_state so wifi_connected is derived from the freshly-read state; apply same update pattern around the startup/initial-check logic that computes wifi_connected (references: startup_wifi_state, last_wifi_state, wifi_monitor_.ReadState, LogWifiState, wifi_check_interval, wifi_connected) so the code won't report a false failure when WiFi recovered before timeout.
🧹 Nitpick comments (1)
chime/src/service/chime_service.cpp (1)
110-112: Add startup validation for notification sound files (parity with bell sound).The new success/failure paths are logged and used, but unlike
config_.sound_path, they are not pre-validated for existence. A startup warning here would make misconfiguration obvious before first playback attempt.♻️ Proposed refactor
- if (config_.audio_enabled && vc::util::IsLinux() && !vc::util::FileExists(config_.sound_path)) { - logger_.Warn("audio", "configured sound file does not exist: " + config_.sound_path); - } + if (config_.audio_enabled && vc::util::IsLinux()) { + if (!vc::util::FileExists(config_.sound_path)) { + logger_.Warn("audio", "configured sound file does not exist: " + config_.sound_path); + } + if (!vc::util::FileExists(config_.notification_success_sound_path)) { + logger_.Warn("audio", "configured success notification file does not exist: " + + config_.notification_success_sound_path); + } + if (!vc::util::FileExists(config_.notification_failure_sound_path)) { + logger_.Warn("audio", "configured failure notification file does not exist: " + + config_.notification_failure_sound_path); + } + }Also applies to: 116-118, 431-442
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chime/src/service/chime_service.cpp` around lines 110 - 112, The notification_success_sound_path and notification_failure_sound_path are logged but not validated at startup like config_.sound_path; add existence checks during initialization (where logger_.Info is called) to verify both config_.notification_success_sound_path and config_.notification_failure_sound_path point to accessible files and emit logger_.Warn (or logger_.Error) if they are missing or unreadable, mirroring the validation used for config_.sound_path (reuse the same helper/logic used for sound_path validation) so misconfiguration is detected before first playback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@chime/src/service/chime_service.cpp`:
- Around line 145-149: The startup path uses last_wifi_state which can be stale;
before computing wifi_connected during startup checks (where wifi_check_interval
may be large/disabled) call wifi_monitor_.ReadState(config_.wifi_interface)
again, and if it has_value() pass it to LogWifiState and assign last_wifi_state
so wifi_connected is derived from the freshly-read state; apply same update
pattern around the startup/initial-check logic that computes wifi_connected
(references: startup_wifi_state, last_wifi_state, wifi_monitor_.ReadState,
LogWifiState, wifi_check_interval, wifi_connected) so the code won't report a
false failure when WiFi recovered before timeout.
---
Nitpick comments:
In `@chime/src/service/chime_service.cpp`:
- Around line 110-112: The notification_success_sound_path and
notification_failure_sound_path are logged but not validated at startup like
config_.sound_path; add existence checks during initialization (where
logger_.Info is called) to verify both config_.notification_success_sound_path
and config_.notification_failure_sound_path point to accessible files and emit
logger_.Warn (or logger_.Error) if they are missing or unreadable, mirroring the
validation used for config_.sound_path (reuse the same helper/logic used for
sound_path validation) so misconfiguration is detected before first playback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57c10b8f-4321-404a-bcb3-174b9b0db295
📒 Files selected for processing (1)
chime/src/service/chime_service.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@chime/src/service/chime_service.cpp`:
- Around line 164-167: The startup notification logic should be skipped when
audio is disabled: detect config_.audio_enabled early in the startup flow and
short-circuit the startup notification loop (or set startup_notification_played
= true) so it does not poll WiFi or emit "playing ... notification" logs while
PlayNotification is a no-op; update the code around startup_notification_played,
startup_notification_unknown_logged, startup_unknown_wifi_begin, startup_begin
and any loop that calls PlayNotification to either wrap that loop with if
(config_.audio_enabled) or mark the notification as already played when
config_.audio_enabled is false, and ensure any logging about playing
notifications is suppressed in that branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fe190e5-7efa-4b31-a547-263eaa8138bd
📒 Files selected for processing (1)
chime/src/service/chime_service.cpp
chime/src/service/chime_service.cpp
Outdated
Comment on lines
+164
to
+167
| bool startup_notification_played = false; | ||
| bool startup_notification_unknown_logged = false; | ||
| std::optional<std::chrono::steady_clock::time_point> startup_unknown_wifi_begin; | ||
| const auto startup_begin = std::chrono::steady_clock::now(); |
There was a problem hiding this comment.
Bypass startup notification checks when audio notifications are disabled.
At Line 164 and Line 226, the startup notification loop still runs even when config_.audio_enabled is false. That adds unnecessary wifi polling and emits misleading “playing ... notification” logs, while PlayNotification is a no-op.
💡 Suggested fix
- bool startup_notification_played = false;
+ bool startup_notification_played = !config_.audio_enabled;Also applies to: 226-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@chime/src/service/chime_service.cpp` around lines 164 - 167, The startup
notification logic should be skipped when audio is disabled: detect
config_.audio_enabled early in the startup flow and short-circuit the startup
notification loop (or set startup_notification_played = true) so it does not
poll WiFi or emit "playing ... notification" logs while PlayNotification is a
no-op; update the code around startup_notification_played,
startup_notification_unknown_logged, startup_unknown_wifi_begin, startup_begin
and any loop that calls PlayNotification to either wrap that loop with if
(config_.audio_enabled) or mark the notification as already played when
config_.audio_enabled is false, and ensure any logging about playing
notifications is suppressed in that branch.
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.
Motivation
Description
notification_success_sound_pathandnotification_failure_sound_pathwith defaults toChimeConfig, and wired them into the config parser inchime/src/config/chime_config.cppand the board overlayetc/chime.conf.ChimeService::Run: addedNotificationSoundTypeenum,WifiStateIsConnectedhelper, andPlayNotificationmethod to play a success/failure sound when startup checks pass or a timeout (kStartupNotificationTimeoutSeconds) elapses; added logging for notification paths.chime/src/webd/config_store.cppandchime/src/webd/web_server.cppto load, validate (1-256 chars), save, and return the new fields, and updated the replacements map when writingchime.conf.webui/src/App.svelte) to expose two inputs for notification sound paths, include them in GET/POST payload handling, and add a smallNotificationscard to the settings UI.Testing
Codex Task
Summary by CodeRabbit
New Features
Bug Fixes
Chores