Listen for preset changes #1931
Conversation
📝 WalkthroughWalkthroughThe changes introduce a preset change detection mechanism to the WLED client. A new Changes
Sequence DiagramsequenceDiagram
participant Device as WLED Device
participant Client as WLED Client
participant API as /json & /presets.json
participant CB as Callback/Handler
Device->>Client: WebSocket Update (TEXT)
rect rgb(200, 220, 255)
Note over Client: _check_presets_version()
Client->>Client: Parse device info<br/>(uptime, time, pmt)
Client->>Client: Compare with<br/>cached version
end
alt Presets Changed
Client->>API: GET /presets.json
API-->>Client: Preset Data
Client->>Client: Store new version
Client->>Client: Update message data
else Presets Unchanged
Client->>Client: Skip preset fetch
end
Client->>CB: Invoke callback<br/>with data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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: 2
Fix all issues with AI Agents 🤖
In @src/wled/wled.py:
- Around line 131-144: The err_msg is mistakenly a one-element tuple due to a
trailing comma in the presets update block (inside the presets_changed branch
that calls request("/presets.json")); remove the trailing comma and construct
err_msg as a single string (e.g., a single f-string combining the host and
message) before passing it into WLEDEmptyResponseError and WLEDConnectionError
so WLEDEmptyResponseError receives a string, not a tuple.
- Around line 281-296: The error message assigned to msg inside the presets
update block is accidentally created as a singleton tuple due to a trailing
comma, which causes WLEDEmptyResponseError to receive a tuple instead of a
string; remove the trailing comma and construct a proper string (e.g., build msg
with f"WLED device at {self.host} returned an empty API response on presets
update") before raising WLEDEmptyResponseError in the branch where
presets_changed is true and request("/presets.json") returns falsy.
🧹 Nitpick comments (1)
tests/test_wled.py (1)
171-185: Consider using f-strings for better readability.The
string.Templateapproach works, but f-strings are more idiomatic in modern Python and would simplify this helper.🔎 Optional refactor to f-string
- def make_json(uptime: int, time: str, pmt: int) -> str: - return string.Template( - """ - {"info": {"uptime":$uptime,"time":"$time","fs":{"pmt": $pmt}}, + def make_json(uptime: int, time_str: str, pmt: int) -> str: + return f""" + {{"info": {{"uptime":{uptime},"time":"{time_str}","fs":{{"pmt": {pmt}}}}}, "state":{"bri":127,"on":true,"transition":7,"ps":-1,"pl":-1, "nl":{"on":false,"dur":60,"mode":1,"tbri":0,"rem":-1}, "udpn":{"send":false,"recv":true,"sgrp":1,"rgrp":1},"lor":0, "seg":[{"id":0,"start":0,"stop":48,"startY":0,"stopY":19,"len":48, "grp":1,"spc":0,"of":0,"on":true,"frz":false,"bri":255,"cct":127, "set":0,"lc":1,"col":[[18,22,255],[0,0,0],[0,0,0]],"fx":174,"sx":0, "ix":91,"pal":0,"c1":128,"c2":128,"c3":16,"sel":true,"rev":false, "mi":false,"rY":false,"mY":false,"tp":false,"o1":false,"o2":false, - "o3":false,"si":0,"m12":0,"bm":0}]}} - """ - ).substitute({"uptime": uptime, "time": time, "pmt": pmt}) + "o3":false,"si":0,"m12":0,"bm":0}]}}}}" + """
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/wled/wled.pytests/test_wled.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_wled.py (1)
src/wled/wled.py (2)
WLED(41-782)update(259-297)
src/wled/wled.py (2)
src/wled/exceptions.py (2)
WLEDConnectionError(12-13)WLEDEmptyResponseError(8-9)src/wled/models.py (2)
update_from_dict(783-841)Device(716-841)
🔇 Additional comments (6)
src/wled/wled.py (4)
7-7: LGTM!The
timeimport is correctly added to support boot time estimation in_check_presets_version.
34-38: LGTM!The
_PresetsVersiondataclass appropriately encapsulates the state needed for preset change detection.
51-51: LGTM!The
_presets_versionfield is correctly added to track preset state across calls.
731-782: The code already documents the design trade-offs and constraints this implementation addresses. Lines 770-772 explicitly explain whyinfo.timeis not used ("The device time is however unreliable, especially around boot"), and lines 767-768 document the 2-second tolerance rationale ("allow 2 seconds changes for rounding errors and network delay"). The integer truncation is intentional, and safe fallbacks exist for parse failures. These decisions reflect reasonable pragmatic choices for a client-side implementation without access to a reliable centralized time source.Likely an incorrect or invalid review comment.
tests/test_wled.py (2)
4-4: LGTM!The
stringimport is added for theTemplateusage in the test helper function.
167-293: Well-designed test, but consider adding listen() coverage.The test comprehensively validates preset caching across multiple scenarios (no change, pmt change, restart). However, the
listen()method has identical preset caching logic (lines 131-144 in src/wled/wled.py) but lacks test coverage.Consider adding a similar test for the
listen()method to ensure both code paths are covered. The listen method uses WebSocket messages which would require mocking WebSocket responses rather than HTTP responses.
| presets_changed, new_presets_version = self._check_presets_version( | ||
| message_data | ||
| ) | ||
| if presets_changed: | ||
| if not (presets := await self.request("/presets.json")): | ||
| err_msg = ( | ||
| f"WLED device at {self.host} returned an empty API" | ||
| " response on presets update", | ||
| ) | ||
| raise WLEDConnectionError(WLEDEmptyResponseError(err_msg)) | ||
| message_data["presets"] = presets | ||
|
|
||
| device = self._device.update_from_dict(data=message_data) | ||
| self._presets_version = new_presets_version |
There was a problem hiding this comment.
Fix error message construction.
The error message at lines 136-140 has a trailing comma that creates a tuple instead of a string:
err_msg = (
f"WLED device at {self.host} returned an empty API"
" response on presets update",
)This results in err_msg being a tuple ("message",) rather than a string, which is then incorrectly passed to WLEDEmptyResponseError.
🔎 Proposed fix
if presets_changed:
if not (presets := await self.request("/presets.json")):
- err_msg = (
- f"WLED device at {self.host} returned an empty API"
- " response on presets update",
- )
+ err_msg = (
+ f"WLED device at {self.host} returned an empty API"
+ " response on presets update"
+ )
raise WLEDConnectionError(WLEDEmptyResponseError(err_msg))🤖 Prompt for AI Agents
In @src/wled/wled.py around lines 131-144, The err_msg is mistakenly a
one-element tuple due to a trailing comma in the presets update block (inside
the presets_changed branch that calls request("/presets.json")); remove the
trailing comma and construct err_msg as a single string (e.g., a single f-string
combining the host and message) before passing it into WLEDEmptyResponseError
and WLEDConnectionError so WLEDEmptyResponseError receives a string, not a
tuple.
| presets_changed, new_presets_version = self._check_presets_version(data) | ||
| if presets_changed: | ||
| if not (presets := await self.request("/presets.json")): | ||
| msg = ( | ||
| f"WLED device at {self.host} returned an empty API" | ||
| " response on presets update", | ||
| ) | ||
| raise WLEDEmptyResponseError(msg) | ||
| data["presets"] = presets | ||
|
|
||
| if not self._device: | ||
| self._device = Device.from_dict(data) | ||
| else: | ||
| self._device.update_from_dict(data) | ||
|
|
||
| self._presets_version = new_presets_version |
There was a problem hiding this comment.
Fix error message construction.
The error message at lines 284-288 has the same trailing comma issue as in the listen() method, creating a tuple instead of a string.
🔎 Proposed fix
presets_changed, new_presets_version = self._check_presets_version(data)
if presets_changed:
if not (presets := await self.request("/presets.json")):
- msg = (
- f"WLED device at {self.host} returned an empty API"
- " response on presets update",
- )
+ msg = (
+ f"WLED device at {self.host} returned an empty API"
+ " response on presets update"
+ )
raise WLEDEmptyResponseError(msg)📝 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.
| presets_changed, new_presets_version = self._check_presets_version(data) | |
| if presets_changed: | |
| if not (presets := await self.request("/presets.json")): | |
| msg = ( | |
| f"WLED device at {self.host} returned an empty API" | |
| " response on presets update", | |
| ) | |
| raise WLEDEmptyResponseError(msg) | |
| data["presets"] = presets | |
| if not self._device: | |
| self._device = Device.from_dict(data) | |
| else: | |
| self._device.update_from_dict(data) | |
| self._presets_version = new_presets_version | |
| presets_changed, new_presets_version = self._check_presets_version(data) | |
| if presets_changed: | |
| if not (presets := await self.request("/presets.json")): | |
| msg = ( | |
| f"WLED device at {self.host} returned an empty API" | |
| " response on presets update" | |
| ) | |
| raise WLEDEmptyResponseError(msg) | |
| data["presets"] = presets | |
| if not self._device: | |
| self._device = Device.from_dict(data) | |
| else: | |
| self._device.update_from_dict(data) | |
| self._presets_version = new_presets_version |
🤖 Prompt for AI Agents
In @src/wled/wled.py around lines 281-296, The error message assigned to msg
inside the presets update block is accidentally created as a singleton tuple due
to a trailing comma, which causes WLEDEmptyResponseError to receive a tuple
instead of a string; remove the trailing comma and construct a proper string
(e.g., build msg with f"WLED device at {self.host} returned an empty API
response on presets update") before raising WLEDEmptyResponseError in the branch
where presets_changed is true and request("/presets.json") returns falsy.
1 participant
Proposed Changes
This PR makes
listen()detect changes to Presets. It also avoids fetchingpresets.jsoninupdate()when presets haven't changed.Potential changes to presets are detected two ways: If WLED's announces a change in
pmtaka presetsModifiedTime or if the WLED device is restarted. Restart detection is needed since presetsModifiedTime is only stored in the volatile memory.Home Assistant uses
listen()but also pollsupdate()every 10 seconds. My understanding the polling is needed to update the Presets. The intent of this PR is to allow removing the polling in the future by making thelisten()return all needed information, and immediately reduce the amount of polling topresets.json.Polling
presets.jsonis problematic as it causes visual glitching on WLED devices. On many ESP devices, fetching the file results in reads from the flash storage on the chip. For reasons beoynd my undestanding, this apparently breaks some interrupts on some firmware versions and led output gets corrupted. On better firmware, it only causes the led animations to stutter.WLED community is very aware of the HASS polling behavior, and has attempted to work around it with some caching tricks. These only reduce the issues and require special hardware (PSRAM) to be installed. Some users have switched to MQTT based HASS integration to avoid the this default HASS integration's issues. To me, it seems more sensible to fix the unnecessary polling than try to work around the issues it causes.
The accessed fields
pmtanduptimeare supported since WLED 0.11.0 (November 2020 release).Related Issues
MoonModules/WLED-MM#307
wled/WLED#2480
home-assistant/core#90676
home-assistant/core#134863
A related potential future improvement has been described in the first issue MoonModules/WLED-MM#307 . By adding ETag on WLED's output, client could avoid fetching /presets.json contents when it hasn't changed. This does not however solve the use-case of using
listen()to detect Preset changes nor does it fix the issue for the existing fleet, which are the goals of this PR.For completeness, an ETag implementation is provided in: gunjambi#1
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.