Skip to content

feat: extended custom effects, segment colors, and scribble (per-LED) for 0xB6#540

Closed
asxzy wants to merge 12 commits into
lightinglibs:masterfrom
asxzy:feat/add-0xb6-effects-api
Closed

feat: extended custom effects, segment colors, and scribble (per-LED) for 0xB6#540
asxzy wants to merge 12 commits into
lightinglibs:masterfrom
asxzy:feat/add-0xb6-effects-api

Conversation

@asxzy

@asxzy asxzy commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Adds 0xB6's lighting capability on top of the merged protocol PR (#539): ~2,425 lines across 8 files (~1,550 of which are tests).

This is the bulk of the device's lighting capability. Three command families, all wrapped in the standard B0 B1 B2 B3 envelope with version=0x02 (reusing the existing construct_wrapped_message), all reverse-engineered and verified on hardware.

1) Extended custom effects — E1 21

Whole-strip dynamic effects. Inner layout:

e1 21 00 64 PP OO DD NN SS 00 00 00 00 00 00 CC [H/2 S V 00 00] × CC
            |  |  |  |  |                       └ up to 8 colors (HSV)
            |  |  |  |  └ speed 0-100
            |  |  |  └ density 0-100
            |  |  └ direction (0x01 L→R, 0x02 R→L)
            |  └ option/variant (0x00 default, 0x01 color-change, 0x02 breathe)
            └ pattern id
  • 24 patterns: ids 1–22 (Wave, Meteor, Streamer, Building Blocks, Flowing Water, Chase, Horse Racing, Cycle, Breathe, Jump, Strobe, Twinkling Stars, Stars Wink, Warning, Collision, Fireworks, Comet, Gradient Meteor, Volcano, Superluminal, Rainbow Bridge, Gradient Overlay) + 101 Static Gradient, 102 Static Fill.
  • The id→name table (EXTENDED_CUSTOM_EFFECT_ID_NAME, pattern.py) was verified against the app's own labels across a 70-command capture (every id 1–22/101/102, plus direction/density/speed and all three option variants, decoded and matched).

2) Segment colors — E1 22

Static colors for up to 20 fixed segments (each ≈ N/20 LEDs). Inner:
e1 22 00 00 00 00 14 [H/2 S V 00 00] × 20. Fewer than 20 supplied → remaining segments off; no auto-repeat (caller supplies up to 20). Verified on-device.

3) Scribble (per-LED) — E1 23 + E1 26

The per-bulb "scribble" feature. Verified on hardware: E1 26 is the render path; the device keeps a stateful per-LED framebuffer.
E1 26: e1 26 EFFECT DIR DENSITY SPEED BLINK H/2 S V 00 WLVL BLINKSPD <bitmap ceil(N/8)B>
0 1 2 3 4 5 6 7 8 9 10 11 12 13 …

  • BLINK: 0x00 none / 0x08 slow / 0x10 fast; color brightness = V, white brightness = WLVL (mutually exclusive); off = paint (0,0,0). Bitmap is MSB-first, bit set = "apply this command to LED i".
  • EFFECT (byte 2) is the scribble effect id; device accepts 0x00–0x08 (9 effects; named: 0 static, 1 flowing, 2 twinkling, 5 stars-wink, 8 accumulate — ids 3/4/6/7 are valid but unnamed by the app).
  • E1 23 (e1 23 01 00 01 50 64 00 N + N×7B records) is a non-rendering mode-init the app sends on entry; the library sends it (all-zero) only to guarantee scribble-mode entry.

Public API & code

File What
const.py (+97) Enums ExtendedCustomEffectPattern, ExtendedCustomEffectDirection, ExtendedCustomEffectOption, ScribbleEffect, ScribbleBlinkMode; ScribbleLED NamedTuple (rgb | white, blink mode/speed).
protocol.py (+326) construct_extended_custom_effect, construct_custom_segment_colors, construct_scribble_init, construct_scribble_paint, _scribble_bitmap, HSV helpers — all reuse construct_wrapped_message(..., version=0x02).
base_device.py (+239) _generate_* validation + _scribble_paint_groups (the shared render path: groups LEDs by identical color/blink, one E1 26 per group covering all N incl. an off group; coerces+validates the effect id 0–8); supports_scribble.
aiodevice.py (+82) / device.py (+96) async_set_extended_custom_effect / setExtendedCustomEffect, async_set_custom_segment_colors / setCustomSegmentColors, async_set_scribble / setScribble (camelCase sync to match existing methods). effect: ScribbleEffect | int.
pattern.py (+34) EXTENDED_CUSTOM_EFFECT_ID_NAME (verified).

Design decisions

  • Reuse the wrapper/HSV helpers rather than reinventing checksum/framing.

  • Grouped E1 26 is the render path (not E1 23), because on-device testing showed E1 23 alone does not light the strip. A full per-LED config = one E1 26 per color/blink group, covering all N LEDs deterministically.

  • effect accepts a raw int (not just the named enum) so the 4 valid-but-unnamed device effects (ids 3/4/6/7) are reachable without fabricating names; the 0–8 range is validated centrally in _scribble_paint_groups.

  • Public methods guard on capabilityset* / async_set_* raise ValueError on devices lacking the feature (supports_extended_custom_effects / supports_scribble), rather than failing deep in construction (previously a bare AssertionError, or an AttributeError under python -O).

Per-effect capability map (reverse-engineered from the vendor app)

The library enforces only general wire-byte bounds (speed/density 0–100,
direction 0x01/0x02, option 0–2). Per-effect applicability below is documented
for reference; it is intentionally not enforced (deferred to a future PR).
direction is not surfaced per-effect in the app; density is a 0–100 byte
(some effects expose it as discrete steps, e.g. Meteor low/middle/high).
option values are 0-based indices into each effect's option list.

ID Effect Options (index → label) Density Speed (default)
1 Wave 0–100 (80)
2 Meteor low/middle/high 0–100 (80)
3 Streamer 0–100 (80)
4 Building Blocks 0 Not-change-color, 1 Change-color 0–100 (80)
5 Flowing Water 0–100 (80)
6 Chase 0–100 (80)
7 Horse Racing 0–100 (80)
8 Cycle 0–100 (40)
9 Breathe 0–100 (100) 0–100 (40)
10 Jump 0–100 (100) 0–100 (50)
11 Strobe 0 Twinkle, 1 Strobe 0–100 (100) 0–100 (80)
12 Twinkling Stars 0 Twinkle, 1 Strobe, 2 Breathe 0–100 (100) 0–100 (60)
13 Stars Wink 0 Twinkle, 1 Strobe, 2 Breathe 0–100 (60)
14 Warning 0 Collision, 1 Two-Color-Collision 0–100 (50)
15 Collision 0–100 (80)
16 Fireworks 0–100 (100) 0–100 (80)
17 Comet 0 Low, 1 Segmentation 0–100 (80)
18 Gradient Meteor 0 Low, 1 Segmentation 0–100 (80)
19 Volcano 0–100 (80)
20 Superluminal 0–100 (65)
21 Rainbow Bridge 0–100 (80)
22 Gradient Overlay 0–100 (80)
101 Static Gradient
102 Static Fill

NOTE Per-effect validation will add complexity to the code and CLI. The device still function on invalid param per effect. Not a hard block. Can be added in the future in a separate PR if needed.

Testing

  • ~1,550 lines of tests. Construct methods have golden-packet unit tests asserting the exact inner bytes against real captures (e.g. green all-on, blink fast/slow, off-bulb, flowing R→L, the E1 23 init, etc.). Plus grouping, validation, and async/sync sequence tests.
  • pytest green, ruff clean, no new mypy errors.

On-device verification (device 192.168.1.78, 80 LEDs):

Feature Command(s) Result
Solid color (name/hex/rgb) -c red / -c "#0000FF" / -c 0,255,0 ✅ steady
Warm white -w 80
Volatile -c red -v ✅ sets color
Cold white / CCT (no HW) -d 80 / -k 3500 80 ⚠️ unsupported by test device— strip goes dark (no cold-white LED)
Combined RGBW -c -c 255,0,0,128 ⚠️ unsupported by test device (non-RGB&CW device) — use -w
Extended effects: all 24 names app capture, 70 cmds ✅ ids 1–22 + 101/102 decode to the correct names (verified vs the app's labels); direction/density/speed/option all decode
Extended effects (visual) -C wave …, -C meteor … ✅ animate
Segments -C segments 0 "<≤20 colors>" ✅ 20 fixed zones, unspecified→off, no auto-repeat
Scribble color/white/off -C scribble static "…" ✅ per-LED color, white full/dim, per-bulb off
Scribble blink (per-LED) fast/slow ✅ independent per group
Scribble effects static/flowing(L→R,R→L)/twinkling/stars_wink/accumulate
Scribble density twinkling density 10 vs 95 ✅ visibly differs
Scribble effect-id range probe sweep ✅ ids 0x00–0x08 valid (9 effects); 9+ rejected
Count reconcile 40xblue (pad) / 100xred (truncate) ✅ + warning

Brightness/PWM note: at reduced brightness the device shows a sub-visual PWM flicker visible only on a security camera. Confirmed device/hardware behavior — the official app flickers identically at low brightness; full-brightness output is steady. Not introduced by this code (the library faithfully sends the requested brightness).


Review round: hardware-verified state & color-set fidelity

Follow-up to review feedback, resolved by capturing the vendor app's traffic on the physical device (192.168.1.78) and matching it byte-for-byte. Established the device's two top-level modes from real EA 81 state frames:

  • Colorful (solid / white / multi-segment) → preset_pattern=0x24 → a plain color (set via uniform E1 22).
  • Scenes (Wave, Static Fill, …) → preset_pattern=0x25, mode = pattern id → an effect (set via E1 21).

Fixes (all hardware-verified):

  • Solid color no longer reported as an effect. preset 0x24 now maps to effect=None with the real brightness, instead of the phantom name "Segments"/brightness=255. Dropped the unreachable 0x00:"Segments" and 0x6E:"Solid Color" table entries (the app never emits either — confirmed by capture).
  • setRgb / setWarmWhite now match the app. They send a uniform E1 22 (Colorful → preset 0x24), so a plain color-set round-trips as a color — previously they sent E1 21 Static Fill, landing in preset 0x25 and reporting as the effect "Static Fill".
  • White no longer double-counted. The single white LED's value was mirrored into warm+cool then summed (setWarmWhite(50%) shipped full brightness); now combined by max. Verified linear: 20/50/100% → device white 20/49/100.
  • E1 21 header byte fidelity: byte[2] 0x000x01, matching every committed app command.
  • Boundary validation: speed/density 0–100, direction ∈ {0x01,0x02}, option 0–2, pattern id 1–22/101/102; public set* methods raise ValueError on unsupported devices.

Unit tests for these paths are anchored to the real captured device packets (exact EA 81 state frames and the app's E1 22 "Colorful → red" command).

Note: CCT devices are unaffected — the color/white changes live in the 0xB6-only ProtocolLEDENETExtendedCustom override; CCT/multi-white devices use their own protocol classes (warm/cool sent as separate bytes) and are byte-for-byte unchanged.

Follow-up polish (review round 2):

  • The public ExtendedCustomEffectPattern/Direction/Option enums are now accepted by setExtendedCustomEffect/async_set_extended_custom_effect (coerced to .value before validation), consistent with setScribble.
  • Scribble validates its inputs before sending the E1 23 init, so an invalid call raises without leaving the device in a half-applied init state.
  • An unmapped preset 0x25 scene mode now reports "unknown (0x…)" instead of None, so an active-but-unknown effect isn't conflated with idle.

Follow-up polish (review round 3):

  • Extended-effect enums are accepted at the public API; scribble validates before the E1 23 init send; unmapped 0x25 scene modes report "unknown (0x…)" instead of None; extended_custom_effect_pattern_list is Title-Case aligned with effect; non-blinking scribble LEDs coalesce into one E1 26; effect-enum coercion is narrowed to the specific enum types.
  • Scoped deferral (TODO in code): the 0xB6 extended custom effects are reported via effect and set via the dedicated setExtendedCustomEffect API (discovery via extended_custom_effect_pattern_list). Wiring them into Home Assistant's generic effect_list/set_effect is intentionally deferred to a follow-up PR — that path takes only (name, speed) and cannot supply the color list these effects require. Consequently a running scene may report an effect name not present in effect_list; this is a documented, intentional limitation of this first-introduction PR, not an oversight.

@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.01493% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
flux_led/base_device.py 93.27% 4 Missing and 4 partials ⚠️
Files with missing lines Coverage Δ
flux_led/aiodevice.py 96.42% <100.00%> (+0.13%) ⬆️
flux_led/const.py 100.00% <100.00%> (ø)
flux_led/device.py 78.09% <100.00%> (+2.05%) ⬆️
flux_led/pattern.py 100.00% <100.00%> (ø)
flux_led/protocol.py 99.22% <100.00%> (+0.05%) ⬆️
flux_led/base_device.py 95.95% <93.27%> (-0.43%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@asxzy asxzy force-pushed the feat/add-0xb6-effects-api branch 3 times, most recently from 134fd15 to 2546623 Compare June 30, 2026 16:46
asxzy added 3 commits June 30, 2026 20:07
- const.py: ExtendedCustomEffectPattern / Direction / Option enums
- pattern.py: EXTENDED_CUSTOM_EFFECT_ID_NAME / NAME_ID maps
- protocol.py: construct_extended_custom_effect (0xE1 0x21),
  construct_custom_segment_colors (0xE1 0x22), construct_levels_change and the
  HSV(+W) byte helpers on ProtocolLEDENETExtendedCustom
- base_device.py: extended_custom_effect_pattern_list, _generate_* helpers and
  pattern-name lookup in _named_effect
- aiodevice.py / device.py: async_set_* and set* entry points

Stacked on the dedicated-protocol PR.
…test, and dedup HSV helper for 0xB6

- Guard setExtendedCustomEffect/setCustomSegmentColors/setScribble (sync)
  and async_set_extended_custom_effect/async_set_custom_segment_colors/
  async_set_scribble (async) with a clear ValueError on unsupported devices,
  instead of hitting a bare assert (AssertionError, or AttributeError under -O).
- Add golden inner-byte test for construct_custom_segment_colors pinning the
  E1 22 header, 0x14 count, per-segment HSV, and 20-segment padding.
- Add sync test_0xB6_setScribble exercising the E1 23 init + E1 26 paint loop.
- Collapse _rgb_to_hsv_bytes to delegate to _rgb_to_hsv_bytes_rgbw(r,g,b,0).
@asxzy asxzy force-pushed the feat/add-0xb6-effects-api branch from 2546623 to e81e97b Compare July 1, 2026 02:23
@asxzy asxzy marked this pull request as ready for review July 1, 2026 02:31
@bluetoothbot

bluetoothbot commented Jul 1, 2026

Copy link
Copy Markdown

Previous review — superseded by a newer review below.

@bluetoothbot bluetoothbot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found.

  • Pattern ID validation is over-permissive: accepts 23–24 which have no defined pattern

…e direction, guard led_count==0, and cover validation guards (0xB6 review)
@asxzy asxzy requested a review from bluetoothbot July 1, 2026 03:00
@bluetoothbot

bluetoothbot commented Jul 1, 2026

Copy link
Copy Markdown

Previous review — superseded by a newer review below.

@bluetoothbot bluetoothbot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found.

  • Animation params (speed/density/direction/option) bypass range validation

@asxzy

asxzy commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Will add parameter validation to the animations tomorrow.

… and surface unmapped effect modes (0xB6 review)
@asxzy asxzy requested a review from bluetoothbot July 1, 2026 17:45
@bluetoothbot

Copy link
Copy Markdown

@asxzy: Will add parameter validation to the animations tomorrow.

Confirmed present at this head. _generate_extended_custom_effect now checks speed/density 0-100, direction 0x01/0x02, option 0-2. _scribble_paint_groups validates the scribble path too. Prior blocking finding resolved. Two items remain: mode 0x6E reported as effect "Solid Color", and the new enums not accepted by setExtendedCustomEffect.

@bluetoothbot

bluetoothbot commented Jul 1, 2026

Copy link
Copy Markdown

Previous review — superseded by a newer review below.

@bluetoothbot bluetoothbot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found.

  • Solid color (mode 0x6E) is reported as an active effect, distorting effect/brightness

asxzy added 4 commits July 1, 2026 19:05
…fect, drop phantom effect-name entries, and match app E1 21 header byte (0xB6 review, hardware-verified)
…stead of E1 21 Static Fill, so a plain color-set reports as a color not an effect (hardware-verified)
Use the exact EA81 frames captured from the device for the Colorful/Scenes
state-reporting tests (Wave and Static Fill now use the real speed bytes; solid
full/dim were already real), and note that the construct_levels_change color
golden is byte-identical to the app's captured 'Colorful -> red' E1 22 packet.
_generate_levels_change mirrors a single white value into both warm and cool
for non-CCT devices; the 0xB6 override summed them, doubling the white level
(setWarmWhite(50%) shipped full brightness). Combine with max instead of sum
(the device has one white LED). Verified on hardware: white now tracks the
requested level linearly. Repurpose the old clamping test as a double-count
regression test.
@asxzy asxzy requested a review from bluetoothbot July 2, 2026 01:36
@bluetoothbot

bluetoothbot commented Jul 2, 2026

Copy link
Copy Markdown

Previous review — superseded by a newer review below.

@bluetoothbot bluetoothbot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found.

  • Extended-effect API rejects the enums this PR ships (pattern_id/direction/option are int-only)

…init send, and report unmapped 0xB6 scene modes distinctly (review)
@asxzy

asxzy commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — addressed in 0421e43:

  • 🟡 Enums rejected at the API boundarysetExtendedCustomEffect / async_set_extended_custom_effect now accept Enum | int for pattern_id/direction/option and coerce to .value before validation, matching setScribble.

  • 🟢 Scribble init before validation — the paint groups are now built/validated before the E1 23 init is sent, so an invalid call raises without leaving the device in a half-applied init state (test asserts nothing is sent).

  • 🟢 Unmapped 0x25 mode — now returns "unknown (0x…)" instead of None, so an active-but-unknown scene isn't reported as idle.

  • 🟢 "New ValueError guards lack boundary tests" — these already exist (the review noted the test files weren't in the visible diff): test_generate_extended_custom_effect_param_bounds (speed/density/direction/option), test_generate_extended_custom_effect_rejects_too_many_colors, test_generate_custom_segment_colors_rejects_too_many_segments, and the test_scribble_paint_groups_* family (num_leds bounds, rgb/white mutual exclusion, rgb/white ranges, blink_speed, effect-id, count/led_count mismatch). No change needed there.

@asxzy asxzy requested a review from bluetoothbot July 2, 2026 16:53
@bluetoothbot

Copy link
Copy Markdown

@asxzy: Thanks — addressed in 0421e43:

Verified all three at head. Enum coercion present. Init now after validation. Unknown placeholder returned.

One gap remains: effect_list unchanged. 0xB6 hits the RGB branch, returns legacy preset effects. effect now returns "Wave"/"Meteor", absent from effect_list — HA flags effect-not-in-list. Also casing splits: extended_custom_effect_pattern_list gives "wave", effect gives "Wave". Drive both off EXTENDED_CUSTOM_EFFECT_ID_NAME.

@bluetoothbot

bluetoothbot commented Jul 2, 2026

Copy link
Copy Markdown

Previous review — superseded by a newer review below.

@bluetoothbot bluetoothbot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found.

  • effect_list omits the extended custom effects that effect now reports (0xB6)
  • Two new public naming APIs disagree on case: extended_custom_effect_pattern_list vs reported effect

asxzy added 2 commits July 2, 2026 11:11
…coalesce non-blinking scribble groups, and narrow effect enum coercion (review)
…integration)

The prior change advertised extended custom effect names in effect_list, but
set_effect(name, speed) can't set them (they require a color list), so every
listed name was unsettable -- and the unmapped-mode placeholder also isn't in
the list. Per a minimum-change approach, revert the effect_list expansion and
defer wiring extended effects into the generic effect_list/set_effect path to a
follow-up PR (TODO added). Extended effects remain reported via `effect` and
settable via the dedicated setExtendedCustomEffect API; discovery stays on
`extended_custom_effect_pattern_list`.
@asxzy asxzy requested a review from bluetoothbot July 2, 2026 17:35
@asxzy

asxzy commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in 9331008 + c76b374:

  • 🟡 Enums accepted at the APIsetExtendedCustomEffect/async_set_extended_custom_effect take Enum | int and coerce via .value (narrowed to the specific enum types, so a mismatched enum raises rather than being silently reinterpreted).
  • 🟡 Naming round-tripextended_custom_effect_pattern_list now derives from EXTENDED_CUSTOM_EFFECT_ID_NAME (Title Case), matching effect and the NAME_ID reverse map.
  • 🟢 Redundant scribble packets — non-blinking LEDs of the same color now coalesce (blink_speed dropped from the group key when blink is off); the emitted wire byte is unchanged.

On effect vs effect_list: rather than advertise the extended custom effects in the generic effect_list (which would let HA call set_effect(name, speed) on them — but they require a color list, so every such call would fail), I've scoped this to report + dedicated-API only for now. Extended effects are reported via effect and set via setExtendedCustomEffect, with discovery on extended_custom_effect_pattern_list. Wiring them into HA's effect_list/set_effect is deferred to a follow-up PR (TODO in effect_list). So a running scene may report an effect name not in effect_list — an intentional, documented limitation of this first PR, not an oversight.

Separately, the earlier "new ValueError guards lack boundary tests" note was a visibility artifact (the test files weren't in the diff) — those boundary tests already exist (test_generate_extended_custom_effect_param_bounds, ..._rejects_too_many_colors/segments, and the test_scribble_paint_groups_* family).

@bluetoothbot

Copy link
Copy Markdown

@asxzy: Addressed in 9331008 + c76b374:

Confirmed resolved at this head. Enum coercion narrowed to specific types — mismatched enum now raises. extended_custom_effect_pattern_list and effect share EXTENDED_CUSTOM_EFFECT_ID_NAME; round-trip holds. blink_speed dropped from group key when blink off.

On effect vs effect_list: deferral is reasonable. Failing set_effect beats a broken color-less call. One residual: HA validates active effect against effect_list, so a running extended scene logs a warning. Cheap follow-up — list the names in effect_list before wiring set_effect.

Note: protocol.py and tests not in working tree, so golden-packet bytes unverified here.

@bluetoothbot

Copy link
Copy Markdown

PR Review — feat: extended custom effects, segment colors, and scribble (per-LED) for 0xB6

Solid, well-validated 0xB6 lighting API. Prior blocking findings are resolved; one documented, non-blocking effect/effect_list inconsistency remains.

Specific strengths:

  • Enum coercion is now narrowed to the exact expected types (ExtendedCustomEffectPattern/Direction/Option), so a mismatched enum (e.g. ScribbleEffect.STATIC) raises instead of silently unwrapping to an in-range int — the prior MEDIUM silent-failure finding is closed.
  • Scribble paints are built and validated before the E1 23 init is sent (both sync and async), so an invalid call can't leave the device half-initialized.
  • Discovery (extended_custom_effect_pattern_list) and reporting (effect) now share one source of truth (EXTENDED_CUSTOM_EFFECT_ID_NAME), so the two new public APIs round-trip through EXTENDED_CUSTOM_EFFECT_NAME_ID.
  • Validation is comprehensive at the boundary (pattern-id 1-22/101-102, ≤8 colors ≥1, speed/density 0-100, direction 0x01/0x02, option 0-2, per-LED rgb/white/blink/effect-id/num_leds), and public set* methods guard on capability with ValueError rather than failing deep in construction.
  • 0x24 → effect=None (solid color) and 0x25 → name/"unknown (0x…)" reporting confirmed collision-free against PATTERN_CODE_TO_EFFECT (only music/custom codes).

Needs attention:

  • warning (non-blocking) — effect can report an extended name absent from effect_list; author-deferred with an in-code TODO, defensible for a first PR but ships a real HA-facing inconsistency.

Unverified: protocol.py (golden-packet construction, 101/102→0x65/0x66 mapping) and the test files were not present in the working tree (branch not checked out) and were truncated from the diff, so the byte-level construction and boundary tests could not be independently confirmed here.


🟡 Important

1. effect_list omits the extended custom effects that `effect` now reports (0xB6)
flux_led/base_device.py:633-649

_named_effect now returns extended-custom names ("Wave", "Meteor", … via EXTENDED_CUSTOM_EFFECT_ID_NAME) when a 0xB6 device runs a scene (pattern_code == 0x25), but effect_list is unchanged. PROTOCOL_LEDENET_EXTENDED_CUSTOM isn't in any of the OLD_/NEW_/CHRISTMAS_EFFECTS_PROTOCOLS sets, and 0xB6's color_modes intersects COLOR_MODES_RGB, so effect_list falls into the RGB branch (line 643-644) and returns the legacy 8-byte preset names — never the extended names.

Why it matters: effect can return "Wave", which is absent from effect_list. Home Assistant's LightEntity validates the active effect against effect_list; a mismatch logs a warning and can render the light as having no active/selectable effect — the same failure class the 0x24 → None fix in this PR set out to avoid.

Acknowledged deferral: the author has scoped this to report-only + the dedicated setExtendedCustomEffect API, with a TODO in effect_list, because wiring these into the generic set_effect(name, speed) path can't supply the required color list. That's a defensible first-PR boundary, so this is non-blocking — but the inconsistency does ship. A cheap middle ground for the follow-up: include the extended names in effect_list (sourced from EXTENDED_CUSTOM_EFFECT_ID_NAME.values()) even before set_effect supports them, so effect ∈ effect_list holds and the HA warning is avoided.

elif COLOR_MODES_RGB.intersection(self.color_modes):
    effects = EFFECT_LIST_DIMMABLE if self.dimmable_effects else EFFECT_LIST

Checklist

  • Input validation at boundaries
  • No partial device-state on invalid input
  • Capability guarding before construction
  • Public API consistency (enums usable, names round-trip)
  • Correct effect/state reporting (effect ∈ effect_list) — warning #1
  • Backward-compatible public API (additive only)
  • No mutable default arguments
  • No hardcoded secrets

Automated review by Kōan (Claude) HEAD=c76b374 3 min 16s

@bluetoothbot bluetoothbot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

Comment thread flux_led/protocol.py
Comment on lines +1816 to +1822
# Convert Enum to value if needed
if hasattr(pattern_id, "value"):
pattern_id = pattern_id.value
if hasattr(option, "value"):
option = option.value
if hasattr(direction, "value"):
direction = direction.value

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasattr is a bit of a codesmell here

@bdraco bdraco left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split this one up. We are back in the too big for human review space again.

@asxzy

asxzy commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Please split this one up. We are back in the too big for human review space again.

Agree. I can break it down to one feature per PR.

I will defer the set_effect support for this device which is used by HA integration as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants