added support for custom palettes #1603
Conversation
WalkthroughThe pull request introduces enhancements to the WLED CLI and device models, focusing on custom palette management. A new field Changes
Sequence DiagramsequenceDiagram
participant CLI
participant Device
participant Info
CLI->>Device: Request device info
Device->>Info: Retrieve palette details
Info-->>Device: Return custom_palette_count
Device->>Device: Generate palette list
Device-->>CLI: Provide device information
CLI->>CLI: Display palette count
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (5)src/wled/cli/__init__.py (1)
src/wled/models.py (4)
Consider verifying if 255 is always reserved for custom palettes or if there's an overlap risk.
Would you like to run a usage check to confirm the necessity of itertools? ✅ Verification successfulBased on the search results, I can generate the final response as we have sufficient information about the usage of The use of itertools.chain is justified and well-applied 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Verifies if itertools is used elsewhere or only for merging palettes
rg 'itertools' -A 5
Length of output: 2338 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions. |
github-actions
bot
added
the
stale
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/wled/models.py (2)
753-759: Custom palettes integration in pre‑deserialize is sound; consider de‑duplicating palette builderThe
itertools.chainusage to merge built‑in palettes from_paletteswith synthetic custom entries is clear and keeps the existing behavior while adding custom IDs.If you want to reduce duplication and keep the two code paths in sync, you could factor the palette entry generation into a small helper used by both
__pre_deserialize__andupdate_from_dict, e.g.:def _iter_palette_entries(names: list[str], custom_count: int): return itertools.chain( enumerate(names), ((255 - i, f"~ Custom {i} ~") for i in range(custom_count)), )and then call it here with
d.get("info", {}).get("cpalcount", 0).
817-823: Use up‑to‑date custom_palette_count when rebuilding palettes in update_from_dictHere
self.info.custom_palette_countis used to append custom palette entries, butself.infois only refreshed later in the same method (Lines 853-855). If a singleupdate_from_dictcall contains both"info"and"palettes"with a changed custom palette count, the palettes dict will be rebuilt using the old count from the previous snapshot.To keep palettes and
Info.custom_palette_countin sync for a single update payload, consider one of:
- Reordering updates so that
infois processed beforepalettes, or- Reading the count directly from the incoming data if available, falling back to
self.info:- if _palettes := data.get("palettes"): - self.palettes = { - palette_id: Palette(palette_id=palette_id, name=name) - for palette_id, name in itertools.chain( - enumerate(_palettes), - ( - (255 - i, f"~ Custom {i} ~") - for i in range(self.info.custom_palette_count) - ), - ) - } + custom_count = data.get("info", {}).get( + "cpalcount", self.info.custom_palette_count + ) + + if _palettes := data.get("palettes"): + self.palettes = { + palette_id: Palette(palette_id=palette_id, name=name) + for palette_id, name in itertools.chain( + enumerate(_palettes), + ((255 - i, f"~ Custom {i} ~") for i in range(custom_count)), + ) + }This keeps behavior stable for existing callers while ensuring a single refresh uses consistent custom palette metadata.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/wled/models.py(4 hunks)
🔇 Additional comments (2)
src/wled/models.py (2)
5-5: itertools import is appropriate and scoped to real usageThe new
itertoolsimport is justified and used in both palette construction sites, improving readability over manual list concatenation. No issues here.
468-472: Info.custom_palette_count field wiring looks correctThe new
custom_palette_countfield with alias"cpalcount"integrates cleanly with the existingInfomodel and is used consistently later (inDevice.__pre_deserialize__andupdate_from_dict, and in the CLI as per the PR summary). Defaulting to0keeps backward compatibility with older firmware that may not expose this field.
2 participants
Proposed Changes
Add support for custom palettes.
Related Issues
#1602
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.