refactor: effects to frame-generator pattern, add new effects, rename examples#107
Merged
Djelibeybi merged 6 commits intomainfrom Feb 7, 2026
Merged
refactor: effects to frame-generator pattern, add new effects, rename examples#107Djelibeybi merged 6 commits intomainfrom
Djelibeybi merged 6 commits intomainfrom
Conversation
Introduce FrameEffect base class and refactor EffectColorloop to use the animation module for direct UDP frame delivery instead of imperative set_color() calls. Add EffectRainbow for per-pixel rainbow effects on multizone strips and matrix lights. Key changes: - Add LightPacketGenerator for single-light animation support - Add FrameBuffer.for_light() and Animator.for_light() factories - Add duration_ms parameter to all packet generators and animator factories - New FrameEffect base class with generate_frame() pattern and FrameContext - Refactor EffectColorloop from LIFXEffect to FrameEffect - New EffectRainbow effect spreading 360-degree rainbow across pixels - Conductor creates/manages Animators for FrameEffect lifecycle - Fix participants timing bug in Conductor.start() - Use 1.5x frame interval for duration_ms to prevent flicker - Split example into separate colorloop and rainbow scripts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Avi Miller <me@dje.li>
Add flame, aurora, progress bar, and sunrise/sunset effects with full test coverage. Add an effect registry for dynamic effect lookup. Rename all example scripts from numbered prefixes to functional group prefixes (discovery_, control_, effects_, matrix_, animation_) and add an examples/README.md documenting every script and its parameters. Update all cross-references in docstrings and docs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Avi Miller <me@dje.li>
github-actions
bot
added
documentation
📝 WalkthroughWalkthroughIntroduces a frame-based effects architecture: FrameEffect/FrameContext base, multiple new frame effects, EffectRegistry and state utilities, Conductor methods for dynamic light management and per-device last-frame access, single-light animator/packet support, docs/examples updates and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Conductor as Conductor
participant FrameEffect as FrameEffect
participant Animator as Animator
participant UDP as UDP Network
User->>Conductor: start(effect, lights)
activate Conductor
Conductor->>FrameEffect: async_setup(lights)
FrameEffect-->>Conductor: setup complete
Conductor->>Conductor: create_animators(effect, lights)
Conductor->>Animator: for_light/for_multizone/for_matrix
Animator-->>Conductor: Animator instance(s)
Conductor->>FrameEffect: async_play()
activate FrameEffect
loop Frame loop
FrameEffect->>FrameEffect: build FrameContext
FrameEffect->>FrameEffect: generate_frame(ctx)
FrameEffect->>Animator: send_frame(protocol_frame)
Animator->>UDP: UDP packet
end
FrameEffect-->>Conductor: finished/stop
Conductor->>Animator: close()
deactivate Conductor
sequenceDiagram
participant User as User
participant Conductor as Conductor
participant StateMgr as StateMgr
participant Animator as Animator
User->>Conductor: add_lights(effect, new_lights)
activate Conductor
Conductor->>StateMgr: capture_prestate(new_lights)
StateMgr-->>Conductor: PreState
Conductor->>Animator: for_light/for_multizone/for_matrix
Animator-->>Conductor: Animator instance(s)
Conductor->>Conductor: append participants / update animators
Conductor-->>User: return
deactivate Conductor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Djelibeybi
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #107 +/- ##
==========================================
+ Coverage 92.26% 93.00% +0.73%
==========================================
Files 50 57 +7
Lines 5692 6344 +652
Branches 702 799 +97
==========================================
+ Hits 5252 5900 +648
+ Misses 281 280 -1
- Partials 159 164 +5 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lifx/animation/animator.py (1)
135-291:⚠️ Potential issue | 🟡 MinorAdd explicit validation for
duration_msto raise a clearValueErrorinstead of a low-level struct error.Negative or invalid
duration_msvalues currently pass through tostruct.pack_into()duringAnimatorinitialization (whencreate_templates()is called), raising an unhelpfulstruct.error: argument out of range. Add validation in the factory methods or packet generator constructors so invalid values are rejected early with a semantic error message.All three factory methods—
for_matrix(),for_multizone(), andfor_light()—need this check.docs/api/effects.md (1)
576-702:⚠️ Potential issue | 🟡 MinorFPS formula doesn’t match the implementation.
Line 588 and Line 700 saymax(1.0, …), but the code clamps to a minimum of 20.0. This can mislead users about network load and smoothness.Proposed doc correction
- The FPS is automatically calculated from `period` and `change`: `fps = max(1.0, (360 / change) / period)` + The FPS is automatically calculated from `period` and `change`: `fps = max(20.0, (360 / change) / period)` @@ -- Frame interval: `1 / fps` where `fps = max(1.0, (360 / change) / period)` +- Frame interval: `1 / fps` where `fps = max(20.0, (360 / change) / period)`src/lifx/effects/colorloop.py (1)
76-91:⚠️ Potential issue | 🟡 MinorTransition docstring is now misleading.
The parameter is deprecated/ignored in the frame‑based flow, but the docstring still describes runtime behaviour. Suggest aligning it with the public docs.Docstring tweak
- transition: Color transition time in seconds, or None for - random per device (default None). When synchronized=True - and transition=None, uses iteration_period as transition. + transition: Deprecated. Accepted for backward compatibility but ignored.
🤖 Fix all issues with AI agents
In `@docs/api/effects.md`:
- Around line 1181-1184: The docs currently state the Conductor passes
duration_ms = 1000 / fps to the Animator but the implementation/tests use a 1.5×
interval; update the docs so the fps description and the Conductor->Animator
contract reference duration_ms = 1500 / fps (or "1.5× frame interval") instead
of 1000 / fps; ensure the symbols mentioned are fps, duration_ms, Conductor, and
Animator so readers can reconcile the doc with the code/tests.
In `@docs/user-guide/effects-troubleshooting.md`:
- Around line 336-344: The two adjacent bullets repeat “zone colors/colours”;
update the list to remove the duplicate phrasing by rewording the bullets
mentioning FrameEffect and zone restoration—e.g., keep the point that
FrameEffect subclasses can generate per-zone output (reference: FrameEffect,
generate_frame, pixel_count) and combine or rephrase the restoration point to
something like “zones are restored after effects” (reference: zone restoration
behavior) so you no longer repeat “zone colours”; also ensure EffectPulse and
EffectColorloop lines (reference: EffectPulse, EffectColorloop,
MultiZonePacketGenerator) remain unchanged in meaning.
In `@examples/effects_progress.py`:
- Around line 29-57: The resolve_devices function currently logs and skips
unresolved targets; change it to raise LifxDeviceNotFoundError instead so
callers get immediate failure: in resolve_devices, after calling find_by_ip or
find_by_serial, if device is None raise LifxDeviceNotFoundError(target) (include
a helpful message or the target value) and likewise if the resolved object is
not an instance of Light raise LifxDeviceNotFoundError(target, "resolved object
is not a Light"); ensure you import LifxDeviceNotFoundError and update any
callers/tests that expect exceptions instead of silent skips.
In `@examples/effects_pulse.py`:
- Around line 23-57: The resolve_devices function currently logs and skips when
find_by_ip/find_by_serial returns None; change that behavior to raise
LifxDeviceNotFoundError with a clear message including the target (e.g.,
f"Device not found: {target}") instead of printing and continuing. Update the
code path in resolve_devices to raise LifxDeviceNotFoundError when device is
None, and ensure LifxDeviceNotFoundError is imported or referenced (and keep the
existing isinstance(device, Light) check for successful resolution).
In `@examples/effects_sunrise_sunset.py`:
- Around line 38-65: The resolve_devices function currently logs and skips
missing or invalid targets; change this to raise the project-specific
LifxDeviceNotFoundError so callers can fail fast. Import
LifxDeviceNotFoundError, and in resolve_devices replace the branches that
print-and-skip (when device is None or when device is not an instance of Light)
with raising LifxDeviceNotFoundError containing the target identifier and a
short context message (e.g., "device not found" or "unexpected device type") so
callers receive a clear exception instead of silent skipping.
In `@src/lifx/effects/registry.py`:
- Around line 52-70: The _classify_device function currently only checks
MatrixLight and MultiZoneLight and falls back to DeviceType.LIGHT; add lazy
imports for InfraredLight and HevLight and insert isinstance checks in the
priority order Matrix → Multizone → Infrared → HEV → Light so InfraredLight maps
to DeviceType.INFRARED and HevLight maps to DeviceType.HEV; update the imports
inside _classify_device and add the two conditional checks (isinstance(device,
InfraredLight) and isinstance(device, HevLight)) before returning
DeviceType.LIGHT.
In `@tests/test_effects/test_aurora.py`:
- Around line 14-74: Several top-level test functions in this file (e.g.,
test_aurora_default_parameters, test_aurora_custom_parameters,
test_aurora_invalid_speed, test_aurora_invalid_brightness,
test_aurora_invalid_palette_too_short, test_aurora_invalid_palette_hue,
test_aurora_invalid_spread and other untyped tests referenced around lines
270-323) are missing explicit return type annotations; add "-> None" to each of
these test function signatures so they conform to strict Pyright typing. Locate
each top-level test def (functions named test_*) and update the signature to
include the return type annotation (def test_...(...) -> None:) consistently
across the file.
In `@tests/test_effects/test_integration.py`:
- Around line 638-669: Add precise type hints to the new test helper classes:
import FrameContext from lifx.effects.frame_effect and Any from typing, then
annotate both classes' __init__ signatures as __init__(self, **kwargs: Any) ->
None and annotate generate_frame as generate_frame(self, ctx: FrameContext) ->
list[HSBK]. Update both ConcreteFrameEffectForIntegration and FailingFrameEffect
accordingly so generate_frame returns list[HSBK] and __init__ matches the
recommended typing.
In `@tests/test_effects/test_progress.py`:
- Around line 14-107: Add explicit return type annotations "-> None" to all
module-level test functions that currently lack them; update functions shown in
this diff—test_progress_default_parameters, test_progress_custom_parameters,
test_progress_invalid_start_end, test_progress_invalid_position,
test_progress_gradient_foreground, test_progress_gradient_too_few_stops,
test_progress_invalid_spot_brightness, test_progress_invalid_spot_width, and
test_progress_invalid_spot_speed—to have signatures like "def <name>() ->
None:", and likewise add "-> None" to the other module-level test functions
later in the file referenced in the review (the ones around the later sections)
so the file is Pyright strict-compliant.
🧹 Nitpick comments (5)
docs/user-guide/effects-custom.md (1)
161-171: Minor documentation inconsistency inasync_setup()example.The
async_setup()method receivesparticipantsas a parameter but the example iterates overself.participants. While this may work ifself.participantsis set beforeasync_setup()is called, it could confuse readers. Consider using the parameter directly for clarity:async def async_setup(self, participants: list[Light]) -> None: """Fetch initial colors from all devices.""" - for light in self.participants: + for light in participants: color = await self.fetch_light_color(light) self._base_hues.append(color.hue)src/lifx/effects/aurora.py (1)
192-205: Consider documenting the use of private method_ensure_capabilities.The compatibility check accesses
light._ensure_capabilities()which is a private method. While this is acceptable within the library, it would be helpful to add a brief comment noting this is internal API usage, especially since this pattern appears in documentation examples.tests/test_animation/test_framebuffer.py (1)
383-412: Consider adding copy assertion for consistency.The existing
test_apply_passthroughtest (line 48) verifies thatapply()returns a copy withassert result is not data. For consistency, consider adding the same assertion totest_for_light_apply_passthrough:Suggested addition
def test_for_light_apply_passthrough(self) -> None: """Test for_light apply is passthrough for single pixel.""" device = MagicMock() fb = FrameBuffer.for_light(device) data: list[tuple[int, int, int, int]] = [(65535, 32768, 16384, 3500)] result = fb.apply(data) assert result == data + assert result is not data # Should be a copysrc/lifx/effects/frame_effect.py (1)
164-185: Consider guarding against wrong frame lengths.
A mis‑sized frame will currently flow intosend_frame()without a clear error. A fast explicit check would make debugging easier.Suggested defensive check
# Generate user-friendly HSBK frame hsbk_frame = self.generate_frame(ctx) + if len(hsbk_frame) != ctx.pixel_count: + raise ValueError( + f"generate_frame returned {len(hsbk_frame)} pixels, " + f"expected {ctx.pixel_count}" + )src/lifx/effects/sunrise.py (1)
375-380: Moveasyncioimport to module level.The
asyncioimport insideasync_play()should be moved to the top of the file with other imports. Importing inside a method is less efficient and unconventional.♻️ Proposed fix
Add at the top of the file with other imports:
import asyncio import math from typing import TYPE_CHECKING, LiteralThen remove line 376:
async def async_play(self) -> None: """Run the sunset frame loop, then optionally power off.""" await super().async_play() if self.power_off and self.participants: - import asyncio - await asyncio.gather( *(light.set_power(False) for light in self.participants) )
Cover all uncovered patch lines across four effect files: - aurora: hue wrapping branches in _interpolate_hue - progress: gradient hue wrapping, from_poweroff_hsbk - sunrise: is_light_compatible with None capabilities (both sunrise and sunset) - conductor: get_last_frame for non-FrameEffect, add_lights with no task, remove_lights for non-running light Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Avi Miller <me@dje.li>
- docs: fix duration_ms formula (1000 -> 1500/fps) in effects API docs - docs: deduplicate "zone colors" phrasing in troubleshooting - sunrise: move asyncio import to module level - tests: add -> None return type annotations to all module-level test functions - tests: add type hints to FrameEffect integration test helpers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Avi Miller <me@dje.li>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/test_effects/test_aurora.py`:
- Around line 346-349: The nested async function ensure_caps is missing an
explicit return type annotation; update its signature to include "-> None"
(e.g., async def ensure_caps() -> None:) so it satisfies strict Pyright type
checks while keeping its body that creates a MagicMock and assigns it to
light.capabilities unchanged.
In `@tests/test_effects/test_conductor.py`:
- Around line 64-85: The fixtures conductor, light1, light2, and light3 lack
explicit return type annotations required by strict Pyright; update each fixture
signature to include the correct return type (e.g., Conductor for conductor and
whatever type _make_color_light returns for light1/2/3) so Pyright can validate
them—locate the fixture functions named conductor, light1, light2, light3 and
add explicit return type hints referencing Conductor and the color light type
returned by _make_color_light.
🧹 Nitpick comments (1)
tests/test_effects/test_progress.py (1)
519-524: Replace fixed sleep with polling to reduce flaky async tests.
A fixed 0.1s sleep can be brittle on slower CI; polling forsend_frameis more reliable.♻️ Suggested update
- await asyncio.sleep(0.1) + for _ in range(20): + if animator.send_frame.call_count: + break + await asyncio.sleep(0.01) + else: + pytest.fail("send_frame was not called")
Add `-> None` return type annotations to module-level test functions, fixtures, and nested helper functions across all effect test files to satisfy strict Pyright validation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Avi Miller <me@dje.li>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_effects/test_integration.py (1)
91-108:⚠️ Potential issue | 🟡 MinorAdd a return type for
mock_white_light.Line 91 defines a fixture without a return type, which conflicts with strict Pyright expectations.
Proposed update
`@pytest.fixture` -def mock_white_light(): +def mock_white_light() -> MagicMock: """Create a mock white light (no color support)."""As per coding guidelines: "Use type hints with strict Pyright validation across all Python code".
🧹 Nitpick comments (3)
tests/test_effects/test_state_manager.py (1)
20-24: LGTM! Return type annotation added to fixture.The
-> MagicMockreturn type satisfies strict Pyright validation. However, for consistency, thestate_managerfixture (line 14) andmock_multizone_lightfixture (line 28) should also receive return type annotations.♻️ Suggested additions for consistency
`@pytest.fixture` -def state_manager(): +def state_manager() -> DeviceStateManager: """Create a DeviceStateManager instance.""" return DeviceStateManager()`@pytest.fixture` -def mock_multizone_light(): +def mock_multizone_light() -> MagicMock: """Create a mock multizone light device.""" light = MagicMock(spec=MultiZoneLight)tests/test_effects/test_capability_filtering.py (1)
13-39: Consider adding return type annotations to fixtures.The
color_lightandwhite_lightfixtures are missing return type annotations. For full Pyright strict compliance, consider adding-> MagicMockto these fixtures, consistent with the pattern seen in other test files (e.g.,tests/test_effects/test_state_manager.py).Suggested fix
`@pytest.fixture` -def color_light(): +def color_light() -> MagicMock: """Create a mock color light.""" ... `@pytest.fixture` -def white_light(): +def white_light() -> MagicMock: """Create a mock white-only light (no color capability)."""Also applies to: 42-68
tests/test_effects/test_base.py (1)
26-29: Consider adding return type annotation toeffectfixture.For consistency with the
mock_lightfixture, consider adding a return type annotation to theeffectfixture.Suggested fix
`@pytest.fixture` -def effect(): +def effect() -> ConcreteEffect: """Create a concrete effect instance for testing.""" return ConcreteEffect(power_on=True)
Add return type annotations to remaining fixtures in test_integration, test_state_manager, test_capability_filtering, and test_base. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Avi Miller <me@dje.li>
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
1 participant
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
01_,02_, ...) to functional group prefixes (discovery_,control_,effects_,matrix_,animation_) for better discoverabilityexamples/README.mddocumenting every script with descriptions and parameter tablesTest plan
uv run pytestto verify all existing and new tests passuv run pyrightto verify type checking passesuv run mkdocs build🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests