fix: Harden media_player page against unavailable entities and missing features#166
Merged
fix: Harden media_player page against unavailable entities and missing features#166
media_player page against unavailable entities and missing features#166Conversation
spacing should take care of that, otherwise we should work on case by case.
…ing features - Handle unavailable/unknown states explicitly; hide all controls and stop progress timer rather than falling through feature checks. - Relax bt_on_off visibility to any of TURN_ON, TURN_OFF, PLAY, PAUSE, STOP since the HMI touch event fires "media_player,toggle" (fixes the case reported as Blackymas/NSPanel_HA_Blueprint#2768). - Hide bt_play_pause when paused on a player exposing only PAUSE (no PLAY), to avoid a dead-resume control. - Show bt_prev/bt_next on idle state, not just playing. - Unify all visibility calls to set_component_visibility. - Cache state and feature flags into locals; drop repeated string compares and bitmask ops. - Replace per-field ESP_LOGV dump with a single ESP_LOGD line. - Replace hardcoded MDI escape sequences with Icons:: constants. - Remove all feed_wdt_delay calls; lambda runtime is well under the watchdog window and command_spacing handles UART pacing. Drop the now-unused PAGE_MEDIA_PLAYER_DELAY_* substitutions. - Delete unused substitutions (COMPONENT_ID_*, MDI_ICON_*) and dead round()/int() casts. - Use std::min explicitly; add explicit bool conversions for clang-tidy. Probably closes Blackymas/NSPanel_HA_Blueprint#2768
…encies ## What Three small TFT-side cleanups to the `media_player` page on both landscape and portrait layouts. ## Why Discovered while investigating upstream [Blackymas/NSPanel_HA_Blueprint#2768](Blackymas/NSPanel_HA_Blueprint#2768). The ESPHome-side fixes for that issue landed in the previous PR (#TODO link); this PR cleans up the HMI side of the same area. ## Changes ### 1. Preinitialize event no longer overwrites Slider / Progress Bar colors The previous Preinitialize iterated every component with `for(sys0=1; sys0<=last_component.id; sys0++) { b[sys0].picc=...; b[sys0].pco=... }`. Two problems: - `vol_slider` (Slider, id 4) and `time_progress` (Progress Bar, id 9) had their `pco` (foreground / fill color) forced to white on every page load, overwriting whatever was set at design time. The progress bar should show its design-time blue when filled. - `picc` was sent to components that don't have that attribute (Progress Bar, Slider, Variables). Nextion silently ignored these but they still consumed UART bandwidth on every page load. New Preinitialize uses two `for` loops over the contiguous text-component ID ranges (`5..8` and `10..17`) plus two named exceptions for `page_label` and `icon_state`. Slider and Progress Bar are intentionally not touched — their colors are owned by design-time (Slider) and the ESPHome runtime (Progress Bar, after the companion ESPHome PR lands). This matches the existing convention from the weather pages (`weather01..weather05`), which use the same hybrid named/loop approach. ### 2. `bt_vol_up` Send Component ID setting `bt_vol_up` (id 7) had `Send Component ID : on press`, while sibling buttons (`bt_vol_down` id 6, `bt_mute` id 8) used `on press and release`. Cosmetic copy-paste inconsistency. Fixed to `on press and release` to match siblings. ### 3. Landscape `artist` / `bt_on_off` 1-pixel overlap On landscape: - `artist` (id 13): x=15, y=126, w=430, **h=43** → bottom edge at y=168 - `bt_on_off` (id 17): x=91, **y=168**, w=50, h=50 → top edge at y=168 Overlapped by exactly one pixel row (y=168). Reduced `artist` height from 43 to 42 to remove the overlap. Single lost pixel is below the text baseline of font 2 — no visible effect on text rendering. Portrait does not have this issue. ## Out of scope (separate PR) - The `time_progress` empty-state policy (Change 4 in the spec): keeping the bar visible always with a theme-aware dim color when no track is loaded. That requires an ESPHome patch to drive the color per playback state and will ship in a follow-up PR with `min_tft_version` pinned to this release. ## Release coordination TFT-only — no ESPHome or Blueprint changes in this PR. Bumps TFT version. The follow-up ESPHome PR for Change 4 will pin `min_tft_version` to whatever this PR ships.
## Summary Four related cleanups in the media_player refresh sequence and its input documentation, surfaced while investigating upstream issue Blackymas/NSPanel_HA_Blueprint#2768. ### Fixes - **`media_position_delta` formula**: removed a dead `default(now() | as_timestamp)` that never fires (the outer `if/else` already guards the missing case), added explicit parentheses around the `as_timestamp` subtraction to make precedence obvious, and cast the result to `int` to match the ESPHome action signature (previously passed a float and relied on HA coercion). Also switched the guard to the `is defined` idiom used elsewhere. - **`media_title` / `media_artist` guards**: dropped two predicates that were either dead (`is defined` never catches `None`) or practically unreachable (`not in enum.states.unknown` for title text). Kept the two meaningful checks: `is string` and `| length > 0`. ### Documentation - **Music Assistant group `supported_features` mask**: added a comment naming every bit in the hand-maintained `1+4+8+16+32+128+256+512+4096+16384` constant, so future maintainers don't have to decode it when MA adds a new capability. - **`media_player_update_interval` input description**: clarified that each poll triggers a real upstream API call for polled integrations (Spotify, Sonos, etc.), not just an internal refresh. Users setting a very low interval should know they're hitting the upstream service at that rate. ## Scope ESPHome and TFT components are untouched — this is Blueprint-only. No breaking change, no `min_*_version` bump needed. ## Testing - Blueprint still parses as valid YAML (with `!input` tag handled). - `media_position_delta` math verified: `((now - updated_at) | round(0) | int)` produces the same integer ESPHome expected from the previous float-through-coercion path. - Title/artist guards tested against the documented edge cases (None, empty string, "unknown"/"unavailable" literals).
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 32 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughRemoved many explicit delay/watchdog-feed calls across lambdas/scripts, refactored boot and media-player lambdas (control flow/visibility/feature-flag logic), bumped TFT/version strings to 17.8, and updated blueprint metadata to v9999.99.9. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
esphome/nspanel_esphome_page_media_player.yaml (1)
176-183: Progress refresh guard ignoresmedia_position_delta.The change-detection compares only
media_durationandmedia_positionagainst last-seen values;media_position_deltais folded intocapped_posbut never influences whether an update is pushed. If HA emits successive calls with identicalmedia_positionbut growingdelta(common when the integration polls at a coarser cadence than this action fires), the displayed value will not advance from ESPHome. The Nextion-sideprg_timer.en=1animation masks this while playing, but the initial value right after a paused→playing transition (or any time the timer is briefly off) can lag by up to one poll cycle.Consider including delta in the comparison, or simply recomputing
capped_posevery call since the writes are cheap:♻️ Suggested tightening
- if (media_duration != last_media_duration || media_position != last_media_position) { + const int capped_pos = std::min(std::max(media_position + media_position_delta, 0), media_duration); + if (media_duration != last_media_duration || capped_pos != last_media_position) { last_media_duration = media_duration; - last_media_position = media_position; - const int capped_pos = std::min(media_position + media_position_delta, media_duration); + last_media_position = capped_pos; disp1->set_component_value("prg_current", capped_pos); disp1->set_component_value("prg_total", media_duration); }The
std::max(..., 0)also defends against a negative delta pushing the bar below zero if HA ever back-reports jitter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esphome/nspanel_esphome_page_media_player.yaml` around lines 176 - 183, The progress refresh guard currently only compares media_duration and media_position (last_media_duration/last_media_position) so changes in media_position_delta are ignored; update the if-block that computes capped_pos to either include media_position_delta in the change-detection or skip the guard and always recompute capped_pos each call, then always call disp1->set_component_value("prg_current", capped_pos) and disp1->set_component_value("prg_total", media_duration); when computing capped_pos use a safe clamp (e.g., capped_pos = std::min(media_duration, std::max(0, media_position + media_position_delta))) to prevent negative 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 `@esphome/nspanel_esphome_page_media_player.yaml`:
- Around line 77-93: The early-return when state_unavailable hides all controls
but omits clearing or hiding the track/artist text, leaving stale metadata
visible; update the unavailable branch in the same block (where
state_unavailable is checked and disp1 is used) to also clear or hide the
metadata components (e.g., call disp1->set_component_visibility("track", false)
and disp1->set_component_visibility("artist", false) or send commands to set
those text components to empty strings) so title/artist are not shown for
unavailable/unknown entities.
In `@nspanel_easy_blueprint.yaml`:
- Around line 8122-8127: The comment above the supported_features mask is out of
sync with the actual sum: the expression in supported_features currently
evaluates to 21437 but the comment says 21053; either update the comment to list
the correct total (21437) to match the expression, or if the intended mask was
21053 remove the TURN_ON(128) and TURN_OFF(256) terms from both the comment and
the supported_features expression so the calculation becomes
(1+4+8+16+32+512+4096+16384)=21053; locate the supported_features block and
adjust the comment and/or the arithmetic accordingly.
---
Nitpick comments:
In `@esphome/nspanel_esphome_page_media_player.yaml`:
- Around line 176-183: The progress refresh guard currently only compares
media_duration and media_position (last_media_duration/last_media_position) so
changes in media_position_delta are ignored; update the if-block that computes
capped_pos to either include media_position_delta in the change-detection or
skip the guard and always recompute capped_pos each call, then always call
disp1->set_component_value("prg_current", capped_pos) and
disp1->set_component_value("prg_total", media_duration); when computing
capped_pos use a safe clamp (e.g., capped_pos = std::min(media_duration,
std::max(0, media_position + media_position_delta))) to prevent negative values.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: aa34d7d6-de7d-4087-b273-0954a7d0e6df
📒 Files selected for processing (25)
esphome/nspanel_esphome_addon_climate_base.yamlesphome/nspanel_esphome_addon_upload_tft.yamlesphome/nspanel_esphome_api.yamlesphome/nspanel_esphome_boot.yamlesphome/nspanel_esphome_chips.yamlesphome/nspanel_esphome_core.yamlesphome/nspanel_esphome_page_alarm.yamlesphome/nspanel_esphome_page_boot.yamlesphome/nspanel_esphome_page_climate.yamlesphome/nspanel_esphome_page_home.yamlesphome/nspanel_esphome_page_media_player.yamlesphome/nspanel_esphome_page_utilities.yamlesphome/nspanel_esphome_version.yamlhmi/dev/nextion2text/nspanel_easy_landscape/boot.txthmi/dev/nextion2text/nspanel_landscape/boot.txthmi/dev/nextion2text/nspanel_landscape/media_player.txthmi/dev/nextion2text/nspanel_portrait/boot.txthmi/dev/nextion2text/nspanel_portrait/media_player.txthmi/nspanel_easy_landscape.hmihmi/nspanel_easy_landscape.tfthmi/nspanel_landscape.hmihmi/nspanel_landscape.tfthmi/nspanel_portrait.hmihmi/nspanel_portrait.tftnspanel_easy_blueprint.yaml
💤 Files with no reviewable changes (6)
- esphome/nspanel_esphome_chips.yaml
- esphome/nspanel_esphome_core.yaml
- esphome/nspanel_esphome_page_utilities.yaml
- esphome/nspanel_esphome_api.yaml
- esphome/nspanel_esphome_addon_upload_tft.yaml
- esphome/nspanel_esphome_page_alarm.yaml
The workflow failed on PRs from forks because `actions/checkout` was pointing at the base repo while using the head branch name, which only exists in the contributor's fork (see #167). - Set `repository` and `ref` from the PR head so checkout targets the fork when applicable, falling back to the current repo for `workflow_dispatch`. - Add a "Verify formatting (fork PRs)" step that fails the check with a clear message when a fork contribution is not properly formatted, since the auto-commit step cannot push back to forks.
style: Fix icon mapping for curtain
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
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 `@esphome/nspanel_esphome_page_media_player.yaml`:
- Line 184: The type mismatch comes from using a long literal (0L) with int32_t
variables; update the expression that computes capped_pos (using media_position,
media_position_delta, media_duration) to use matching integer types by removing
the L suffix from the zero literal or explicitly casting operands (e.g., ensure
std::max is called with int types or provide template arguments) so
std::max(media_position_delta, 0) and the subsequent std::min operate on the
same integral type.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: 877be5c8-42f0-4303-85e7-bf59d3b21f6f
📒 Files selected for processing (3)
.github/workflows/validate_clang_format.ymlesphome/nspanel_esphome_page_media_player.yamlnspanel_easy_blueprint.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- nspanel_easy_blueprint.yaml
Contributor
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 `@esphome/nspanel_esphome_page_media_player.yaml`:
- Around line 177-190: The current calculation adds media_position +
std::max(media_position_delta, 0) before clamping which can overflow or
propagate negative positions; change the logic in the block that computes
capped_pos (the variables media_position, media_position_delta, media_duration,
last_media_position, last_media_duration, and capped_pos) to first clamp
media_position to >=0, clamp media_position_delta to >=0, perform the addition
in a wider integer type (e.g., int64_t) to avoid overflow, then clamp the sum to
at most media_duration (and to at least 0) before assigning last_media_position
and calling disp1->set_component_value.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: 317f63a8-438a-4277-bc39-6a6789bfce53
📒 Files selected for processing (1)
esphome/nspanel_esphome_page_media_player.yaml
Contributor
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 `@esphome/nspanel_esphome_page_media_player.yaml`:
- Around line 195-205: The empty-bar branch zeros prg_current but doesn't clear
the visible Nextion time_progress value, so the bar can retain previous fill
when prg_timer is later disabled; in the branch that sets
disp1->set_component_value("prg_current", 0) and
time_progress.pco=color_pco_unsupported, also explicitly reset the visible
progress value (e.g., send a command to set time_progress.val or call
disp1->set_component_value for "time_progress" to 0) and keep
last_media_duration and last_media_position cleared; ensure this happens before
the prg_timer enable/disable command (the send_command_printf call) so the UI
shows a cleared bar immediately.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: 0959f51b-756d-4919-bae7-56df074fdf62
📒 Files selected for processing (1)
esphome/nspanel_esphome_page_media_player.yaml
This should resolve some of the `Invalid variable name` seen on #163
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
2 participants
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.
Fixes
BugMedia player page is losing the power button Blackymas/NSPanel_HA_Blueprint#2768).unavailableorunknown, instead of showing stale state.bt_play_pauseno longer hides itself on paused players that only support PAUSE (no PLAY).media_position_deltais now passed to ESPHome as a clean integer with stronger fallback handling for pathologicalmedia_position_updated_atvalues.media_titleandmedia_artist(behavior unchanged).Internal improvements
Documentation
supported_featuresoverride is now self-documenting: every bit in the bitmask is named in an inline comment.media_player_update_intervaldescription now makes clear that each poll triggers a real upstream API call for polled integrations (Spotify, Sonos, etc.), not just an internal HA refresh.Summary by CodeRabbit
New Features
Bug Fixes
Chores