fix: split song scripts between the 2 firmware types#20
Conversation
WalkthroughThe pull request refactors ESPHome automation scripts across three configuration files: removing song and color-effect definitions from Core.yaml, replacing extended play_song_X scripts with explicit conditional implementations in H-2.yaml, and introducing new play_song_1 through play_song_x scripts in H-2D.yaml. Changes center on conditional light activation patterns before RTTTL playback. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 1
🧹 Nitpick comments (1)
Integrations/ESPHome/H-2D.yaml (1)
117-280: Consider extracting repeated conditional light activation logic.All five
play_song_*scripts repeat the same conditional pattern for turning onrgb_lightandlogo_light. While ESPHome YAML has limitations, you could potentially use a helper script with parameters (if supported by your ESPHome version) or accept the duplication for clarity.This is a minor maintainability concern; the current implementation is functionally correct.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Integrations/ESPHome/Core.yaml(0 hunks)Integrations/ESPHome/H-2.yaml(4 hunks)Integrations/ESPHome/H-2D.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- Integrations/ESPHome/Core.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Building Integrations/ESPHome/H-2.yaml / ESPHome stable / ESPHome stable
- GitHub Check: Building Integrations/ESPHome/H-2D.yaml / ESPHome dev / ESPHome dev
- GitHub Check: Building Integrations/ESPHome/H-2.yaml / ESPHome beta / ESPHome beta
- GitHub Check: Building Integrations/ESPHome/H-2D.yaml / ESPHome stable / ESPHome stable
- GitHub Check: Building Integrations/ESPHome/H-2.yaml / ESPHome dev / ESPHome dev
- GitHub Check: Building Integrations/ESPHome/H-2D.yaml / ESPHome beta / ESPHome beta
🔇 Additional comments (4)
Integrations/ESPHome/H-2D.yaml (1)
117-148: Verify wifi handling is intentionally omitted for H-2D.The H-2D scripts don't include
wifi.disable/wifi.enablearoundrtttl.play, unlike the H-2.yaml counterparts. If this is intentional due to hardware differences, consider adding a brief comment explaining why. If omitted by mistake, the buzzer audio may not be as loud as intended (based on the PR objective).Integrations/ESPHome/H-2.yaml (3)
127-138: Verify ifplay_song_xneeds an API service.API services are defined for
play_song_1throughplay_song_4, but not forplay_song_x. Ifplay_song_xis an internal/easter-egg script, this is fine. If it should be callable via Home Assistant, add a corresponding service.
279-282: Good use of wifi disable/enable around audio playback.Disabling wifi during RTTTL playback helps ensure cleaner audio by reducing RF interference. This aligns well with the PR objective for louder buzzer audio.
253-286: LGTM - Well-structured conditional light activation.The pattern of checking
remote_values.is_on()before setting brightness ensures user-set brightness is preserved when lights are already on, while providing sensible defaults when waking from sleep. Good implementation.
| - id: play_song_x | ||
| then: | ||
| - if: | ||
| condition: | ||
| lambda: 'return !id(rgb_light).remote_values.is_on();' | ||
| then: | ||
| - light.turn_on: | ||
| id: rgb_light | ||
| brightness: 80% | ||
| effect: "Sandstorm" | ||
| else: | ||
| - light.turn_on: | ||
| id: rgb_light | ||
| effect: "Sandstorm" | ||
| - if: | ||
| condition: | ||
| lambda: 'return !id(logo_light).remote_values.is_on();' | ||
| then: | ||
| - light.turn_on: | ||
| id: logo_light | ||
| brightness: 60% | ||
| effect: "Sandstorm" | ||
| else: | ||
| - light.turn_on: | ||
| id: logo_light | ||
| effect: "Sandstorm" | ||
| - wifi.disable: | ||
| - rtttl.play: | ||
| rtttl: Sandstorm:d=16,o=6,b=85:e,e,e,e,8e,e,e,a,a,a,a,g,g,g,d,e,e,e,e, 8e,e,a,e,e,e,e,8e,e,d,e,e,e,e,e,8e,e,e,a,a,a,a,g,g,g,d,e,e,e,e,8e,e, a,e,e,e,e,8e,e,d,e,e,e,e,e,8e,e,e,a,a,a,a,g,g,g,d,e,e,e,e,8e,e,a,e, e,e,e,8e,e,d,e | ||
| - wifi.enable: | ||
| - delay: 30s | ||
| - light.turn_off: rgb_light | ||
| - light.turn_off: logo_light | ||
| - script.execute: ShouldSleep |
There was a problem hiding this comment.
Brightness inconsistency in play_song_x.
play_song_x uses 80% brightness for rgb_light (line 401), while play_song_1 through play_song_4 use 90%. If this is intentional, please disregard. Otherwise, apply this diff for consistency:
- if:
condition:
lambda: 'return !id(rgb_light).remote_values.is_on();'
then:
- light.turn_on:
id: rgb_light
- brightness: 80%
+ brightness: 90%
effect: "Sandstorm"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - id: play_song_x | |
| then: | |
| - if: | |
| condition: | |
| lambda: 'return !id(rgb_light).remote_values.is_on();' | |
| then: | |
| - light.turn_on: | |
| id: rgb_light | |
| brightness: 80% | |
| effect: "Sandstorm" | |
| else: | |
| - light.turn_on: | |
| id: rgb_light | |
| effect: "Sandstorm" | |
| - if: | |
| condition: | |
| lambda: 'return !id(logo_light).remote_values.is_on();' | |
| then: | |
| - light.turn_on: | |
| id: logo_light | |
| brightness: 60% | |
| effect: "Sandstorm" | |
| else: | |
| - light.turn_on: | |
| id: logo_light | |
| effect: "Sandstorm" | |
| - wifi.disable: | |
| - rtttl.play: | |
| rtttl: Sandstorm:d=16,o=6,b=85:e,e,e,e,8e,e,e,a,a,a,a,g,g,g,d,e,e,e,e, 8e,e,a,e,e,e,e,8e,e,d,e,e,e,e,e,8e,e,e,a,a,a,a,g,g,g,d,e,e,e,e,8e,e, a,e,e,e,e,8e,e,d,e,e,e,e,e,8e,e,e,a,a,a,a,g,g,g,d,e,e,e,e,8e,e,a,e, e,e,e,8e,e,d,e | |
| - wifi.enable: | |
| - delay: 30s | |
| - light.turn_off: rgb_light | |
| - light.turn_off: logo_light | |
| - script.execute: ShouldSleep | |
| - id: play_song_x | |
| then: | |
| - if: | |
| condition: | |
| lambda: 'return !id(rgb_light).remote_values.is_on();' | |
| then: | |
| - light.turn_on: | |
| id: rgb_light | |
| brightness: 90% | |
| effect: "Sandstorm" | |
| else: | |
| - light.turn_on: | |
| id: rgb_light | |
| effect: "Sandstorm" | |
| - if: | |
| condition: | |
| lambda: 'return !id(logo_light).remote_values.is_on();' | |
| then: | |
| - light.turn_on: | |
| id: logo_light | |
| brightness: 60% | |
| effect: "Sandstorm" | |
| else: | |
| - light.turn_on: | |
| id: logo_light | |
| effect: "Sandstorm" | |
| - wifi.disable: | |
| - rtttl.play: | |
| rtttl: Sandstorm:d=16,o=6,b=85:e,e,e,e,8e,e,e,a,a,a,a,g,g,g,d,e,e,e,e, 8e,e,a,e,e,e,e,8e,e,d,e,e,e,e,e,8e,e,e,a,a,a,a,g,g,g,d,e,e,e,e,8e,e, a,e,e,e,e,8e,e,d,e,e,e,e,e,8e,e,e,a,a,a,a,g,g,g,d,e,e,e,e,8e,e,a,e, e,e,e,8e,e,d,e | |
| - wifi.enable: | |
| - delay: 30s | |
| - light.turn_off: rgb_light | |
| - light.turn_off: logo_light | |
| - script.execute: ShouldSleep |
🤖 Prompt for AI Agents
In Integrations/ESPHome/H-2.yaml around lines 393 to 426, the play_song_x
sequence sets rgb_light brightness to 80% while play_song_1 through play_song_4
use 90%; update the rgb_light brightness in the play_song_x then block from 80%
to 90% so it matches the other play_song handlers for consistent lighting
behavior.
2 participants
Version:
What does this implement/fix?
Splits song scripts between the 2 firmware types to allow for brighter LEDs and louder buzzer audio.
Do not adjust LED brightness levels if the LEDs are already on.
Types of changes
Checklist / Checklijst:
If user-visible functionality or configuration variables are added/modified:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.