Repository navigation
Add ZBT-2 coordinator LED support [RFC/DRAFT/WIP] - #715
TheJulianJES wants to merge 7 commits into
Conversation
zigpy-review-bot
left a comment
There was a problem hiding this comment.
RFC engagement — leaving as COMMENT, no asks to block on while the design is still being shaped.
Direction
The overall shape (entity yielded from discover_coordinator_device_entities, modelled as a BaseLight subclass like DeviceCounterSensor is a BaseEntity, gated on FirmwareFeatures.LED_CONTROL) reads naturally next to the existing coordinator-entity path. EntityCategory.CONFIG + _attr_primary = False is the right framing — this is plumbing, not a primary device control. Tests cover the on/off/restore happy paths and the no-feature gate; the testing seam (zigpy_app_controller._ezsp = SimpleNamespace(...)) is workable but is itself a tell about the bellows-internal coupling discussed below.
Open questions worth nailing down before promoting from draft
1. Bellows-internal coupling — pick a venue. Both call sites reach across the API boundary:
discovery.pyimportsbellows.ezsp.xncp.FirmwareFeaturesand dereferencesapp_controller._ezsp._xncp_featuresdirectly.CoordinatorLED._async_set_led_statedoesgetattr(..., "_ezsp", None)+hasattr(ezsp, "xncp_set_led_state").
This works, but every other coordinator-feature check inside zha right now goes through application_controller public surface, not _ezsp. The PR body already lists the two reasonable resolutions (zigpy-as-intermediary like the country-code path, or bellows exposing a public XNCP-features API). My weak preference is the zigpy intermediary: it generalises naturally to non-EZSP coordinators (zigpy-znp, future hardware) without zha needing per-radio branches, and the RadioType.ezsp guard in discovery.py falls out for free. Either path needs the upstream change first; landing this PR without it locks in a coupling that's awkward to refactor through.
2. Restore semantics on first paint vs. firmware default. on_add calls _schedule_restore_turn_on, which only fires if _restore_on_startup was set — i.e. only after restore_external_state_attributes ran. On an install with no prior persisted state, the LED stays whatever the firmware boot pattern produced (off, by the PR body's description). That's defensible, but it's also the path every freshly-paired user is on, so it's worth being explicit: should there be a one-shot "first ever turn on after pairing" that surfaces something? The PR body's note about router firmware using a blue colour while searching for a network points at the same UX question from the other direction.
3. "Turn off" semantics. The PR body already flags this: xncp_set_led_state(0,0,0) isn't "give control back to firmware," it's "force black." If a user disables the LED entity in HA and later the firmware wants to indicate something (joining state, error), it stays dark. Worth confirming with the firmware folks whether led_manager_clear_pattern(LED_PRIORITY_MANUAL) (or equivalent) can be added to the XNCP surface so the off path can hand control back cleanly. Doesn't need to block this PR, but the answer shapes whether async_turn_off stays as-is or grows a mode argument.
4. HA-core wiring. Linked PR home-assistant/core#162307 is about coordinator-device grouping, separate concern. The actual HA-side wiring needed for this entity — coordinator device showing a light entity in the right place, not under "Zigbee group" — is the missing piece. The current behaviour will work (entity discovery yields a BaseLight, HA renders it), but visual placement on the device page is the bit the PR body flags as confusing. Cross-link the HA-core PR when you have one so this PR doesn't merge in isolation.
Smaller things worth a glance
_xy_brightness_to_rgbreimplements the XY→sRGB transform from scratch.homeassistant.util.color.color_xy_brightness_to_RGBexists and matches what most HA-side colour pickers feed in — would also avoid the manual gamma table here. Not a strong preference if you want zha to stay decoupled fromhomeassistant.util, but worth a moment.async_turn_on(xy_color: tuple[int, int] | None = None)keeps the type-hint inherited fromBaseLight.async_turn_on, but the PR's own conversion (float(xy_color[0])) and tests ((0.64, 0.33)) treat it astuple[float, float]. The base-class hint is wrong, but worth being consistent: either fix the override here, or leave a TODO pointing at the base class._get_rgb_colorrewritesself._xy_color/self._brightnessas a side-effect of computing the RGB tuple. Splitting state-update from value-derivation makes the restore path easier to reason about, but it's idiomatic-enough as a private helper if you'd rather leave it.- CI is failing because the bellows PR isn't released yet (
AttributeError: type object 'FirmwareFeatures' has no attribute 'LED_CONTROL') — expected for the cross-repo dependency, not a real failure. Mentionrequires zigpy/bellows#710in the merge plan and gate on a bellows release.
No mypy regression against dev (72 errors in PR vs ~76 baseline — diff is noise from a few unrelated touched-file shifts).
|
@zigpy-review-bot Update this (code + PR body/title, ...), do review rounds, possibly fix stuff. Anything missing then? |
…ucture - CoordinatorLED is now a BaseLight + PlatformEntity (like Light) bound to the coordinator device, so it inherits device_ieee/available/state handling - Discovery moved from discovery.py into CoordinatorDevice.discover_entities - Tests use the dataclass entity state
…uccessful send - Look up FirmwareFeatures.LED_CONTROL with getattr, so released bellows without it no longer raises in coordinator discovery (which also dropped the counter sensors) - Resolve brightness/XY color without mutating state; store them only after xncp_set_led_state succeeds - Add tests for a failed send and for an adapter without the LED feature bit
|
Pushed c775574 to the branch (fast-forward) and updated the PR title and body. What changed
Still missing
|
DRAFT. Just playing around for now.
Proposed change
This adds experimental support for directly controlling the ZBT-2 coordinator LED using firmware extensions.
The changes work (with the linked bellows PR), but this is very experimental and rough. Nothing that should be merged.
Changes
CoordinatorLEDlight entity (BaseLight+PlatformEntity, XY color mode,EntityCategory.CONFIG, not primary), yielded byCoordinatorDevice.discover_entitieswhen the EZSP coordinator reportsFirmwareFeatures.LED_CONTROL.xncp_set_led_state; turning it off sends black.tests/test_coordinator_light.pycover discovery gating (no EZSP adapter, and an adapter without the LED feature bit), on/off, the default dim-white first turn-on, brightness-only updates keeping the XY color, a failed adapter command leaving the stored state untouched, and restore replay.getattr, so bellows releases withoutFirmwareFeatures.LED_CONTROLjust don't get the entity (coordinator discovery keeps working).devafter the device/entity rework, so discovery now lives inCoordinatorDeviceand the entity is aPlatformEntitybound to the coordinator device.Notes:
on_add. That way, you'll only see the ZBT-2 LED turn off for a few seconds when ZHA is reloaded (or HA restarted).DEFAULT_COORDINATOR_LED_XY_COLORcolor.LED_COLOR_WHITE_DIMcolor from firmware (RGB8(75, 75, 75))led_manager_clear_pattern(LED_PRIORITY_MANUAL)is not supported via firmware extensions, so ZHA turns the light "black" to turn it off.Testing
tests/test_coordinator_light.pypasses (7 tests) and the full suite passes (1788 tests), run against the branch of the bellows PR below.dev, so it needs a rebase onto current bellows before this can be re-tested against a real release.FirmwareFeatures.LED_CONTROLdoes not exist: the entity is simply not created, and five of the seven tests (the ones using the ZBT-2 fixture with the LED feature bit) fail.Open items
bellowspin has to be bumped to a release containing the LED support before this can merge.Additional information
Requires bellows PR (not merged or released yet; bellows 1.2.1 does not have
LED_CONTROL):