Fetch effects from /json/effects to handle ESP8266 buffer truncation#2083
Draft
mik-laj wants to merge 8 commits into
Draft
Fetch effects from /json/effects to handle ESP8266 buffer truncation#2083mik-laj wants to merge 8 commits into
mik-laj wants to merge 8 commits into
Conversation
On ESP8266 devices the /json response truncates the effects array when the output buffer is too small — the device may report fxcount=220 but return only 54 entries. This causes KeyError when a segment references an effect ID that is missing from the truncated list (WLED issue #5674). Fix this by always fetching the complete list from /json/effects on initial load, which is a dedicated endpoint not affected by the buffer limit. On subsequent updates, re-fetch only when fxcount changes or a device restart is detected (boot_time shift > 2 s, derived as now - uptime to avoid false positives from the ever-incrementing uptime counter). Add tests covering: skip when unchanged, re-fetch on fxcount change, re-fetch after device restart, and full list from /json/effects despite truncated /json response. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2083 +/- ##
===========================================
+ Coverage 58.61% 96.63% +38.02%
===========================================
Files 6 8 +2
Lines 662 1159 +497
Branches 143 119 -24
===========================================
+ Hits 388 1120 +732
+ Misses 270 25 -245
- Partials 4 14 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mik-laj
added
the
bugfix
Contributor
There was a problem hiding this comment.
Pull request overview
Adds robust effects fetching to avoid ESP8266 /json truncation by sourcing the full effects list from /json/effects and only re-fetching when the effect count changes or a restart is detected (boot_time shift). This aligns the effects refresh logic with the existing presets “versioning” pattern to reduce unnecessary network calls while preventing KeyError from missing effect IDs.
Changes:
- Introduce
_EffectsVersiontracking and_check_effects_changed()to decide when/json/effectsmust be fetched. - Update
WLED.update()to fetch effects from/json/effectson initial load and when changes/restart are detected. - Extend test helpers and add new tests covering effects refetch behavior and truncated
/jsonresponses.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/wled/wled.py |
Adds effects version tracking + conditional fetch from /json/effects during update(). |
tests/conftest.py |
Updates shared mocking helper to include /json/effects. |
tests/test_wled.py |
Adds tests for effects refetch rules and ESP8266 truncation scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix WLEDEmptyResponseError messages constructed as 1-tuples due to a trailing comma; the exception now receives a plain string in all four call sites (listen() and update()) - Treat uptime=0 as a valid state in _check_effects_changed by using an explicit `is None` guard instead of `not uptime`; previously a device polled in its first second of uptime would never write _effects_version and would re-fetch /json/effects on every subsequent call - Freeze time.time() in test_update_skips_effects_when_unchanged to prevent flakiness when CI is slow enough that the boot_time delta exceeds the 2 s tolerance between update() calls - Update mock_json_and_presets docstring to reflect three endpoints Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When _check_effects_changed returns False (no re-fetch needed), the effects array from /json was still passed into update_from_dict(), which would silently overwrite the full list previously fetched from /json/effects with the truncated ESP8266 payload on every subsequent poll. Fix: pop "effects" from the /json payload when not re-fetching, so update_from_dict() leaves the cached effects untouched. Extend test_update_uses_effects_endpoint_for_full_list with a second update() where /json is still truncated and no /json/effects stub is registered, asserting the full effects list survives. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
mik-laj
force-pushed
the
fix/effects-boot-time-comparison
branch
from
5b2b3fd to
3498651
Compare
Treat fxcount=0 as a valid value rather than "missing": change `not fxcount` to `fxcount is None` in _check_effects_changed, consistent with the earlier uptime fix. A device reporting zero effects would otherwise be forced into a safe-refetch loop on every poll. Update the inline comment in test_update_skips_presets_when_unchanged to reflect that the first update now fetches three endpoints (/json, /json/effects, /presets.json) after mock_json_and_presets was extended to also stub /json/effects. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/wled/wled.py`:
- Around line 330-336: The current logic treats a valid empty effects list as an
error because it uses a falsey check on the result of
self.request("/json/effects"); change the guard to only raise when the request
returned None (or an explicitly invalid response) instead of any falsey
value—e.g., replace the condition with an explicit check like "if effects is
None:" (or validate with isinstance(effects, list) and raise only if the result
is None or not a list) before assigning data["effects"] = effects; keep the
WLEDEmptyResponseError raised only for truly missing/invalid responses.
In `@tests/test_wled.py`:
- Around line 401-423: The test
test_update_refetches_effects_after_device_restart mutates info["fxcount"] which
lets the code detect an effect change by count rather than by restart detection;
revert that change so fxcount remains the same between wled_data and
restarted_data, add the new effect only to restarted_data["effects"], then
update assertions to verify the new effect appears by name/content (e.g. check
that "Post Restart Effect" is present in device2.effects or that a specific
effect name changed) instead of asserting effects length increased based on
fxcount; keep using the existing variables and calls (wled.update(), device1,
device2, initial_effect_count) but replace the count-based assertion with a
presence/name check.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ca0b655-ee1a-4c86-b8bb-36a4fe7a7ebd
📒 Files selected for processing (3)
src/wled/wled.pytests/conftest.pytests/test_wled.py
Comment on lines
+330
to
+336
| if not (effects := await self.request("/json/effects")): | ||
| msg = ( | ||
| f"WLED device at {self.host} returned an empty API" | ||
| " response on effects update" | ||
| ) | ||
| raise WLEDEmptyResponseError(msg) | ||
| data["effects"] = effects |
There was a problem hiding this comment.
Handle empty effects lists without treating them as an error.
At Line 330, if not (effects := await self.request("/json/effects")): will raise on a valid empty list ([]). That makes fxcount=0 scenarios fail despite receiving a valid JSON response.
💡 Suggested fix
- if changed_effects:
- if not (effects := await self.request("/json/effects")):
+ if changed_effects:
+ effects = await self.request("/json/effects")
+ if effects is None:
msg = (
f"WLED device at {self.host} returned an empty API"
" response on effects update"
)
raise WLEDEmptyResponseError(msg)
data["effects"] = effects🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/wled/wled.py` around lines 330 - 336, The current logic treats a valid
empty effects list as an error because it uses a falsey check on the result of
self.request("/json/effects"); change the guard to only raise when the request
returned None (or an explicitly invalid response) instead of any falsey
value—e.g., replace the condition with an explicit check like "if effects is
None:" (or validate with isinstance(effects, list) and raise only if the result
is None or not a list) before assigning data["effects"] = effects; keep the
WLEDEmptyResponseError raised only for truly missing/invalid responses.
Replace `not effects` with `not isinstance(effects, list)` so that a valid empty effects list from /json/effects no longer raises WLEDEmptyResponseError (an empty list is falsy but not an error). In test_update_refetches_effects_after_device_restart, remove the fxcount increment from restarted_data so the re-fetch is driven solely by the boot_time shift (restart detection), not by a count change. Replace the length assertion with a name-presence check so the test validates the actual content rather than an artifact of fxcount. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The WLEDEmptyResponseError for /json/effects said "empty API response" but the guard is isinstance(effects, list), which also fires for a non-empty but wrong-shaped payload. Change to "invalid response" so the message matches the actual check. Device.effects is typed as dict with default_factory=dict and is always initialised in from_dict/update_from_dict, so it can never be None. Remove the dead `or self._device.effects is None` branch from _check_effects_changed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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
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.
Summary
On ESP8266 devices the
/jsonresponse truncates theeffectsarray when the output buffer is too small — the device may reportfxcount=220but return only 54 entries. This causesKeyErrorwhen a segment references an effect ID that is missing from the truncated list (WLED issue #5674).Fix this by always fetching the complete list from
/json/effectson initial load, which is a dedicated endpoint not affected by the buffer limit. On subsequent updates, re-fetch only whenfxcountchanges or a device restart is detected (boot_time shift > 2 s, derived asnow - uptimeto avoid false positives from the ever-incrementing uptime counter).The approach mirrors the existing
_check_presets_changedpattern already used for preset re-fetching.Test plan
test_update_skips_effects_when_unchanged— no re-fetch when fxcount and boot_time unchangedtest_update_refetches_effects_after_device_restart— re-fetch triggered when uptime resetstest_update_uses_effects_endpoint_for_full_list— full list from/json/effectsdespite truncated/jsonresponse (ESP8266 scenario)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes