Add XNCP coordinator LED support#710
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #710 +/- ##
=======================================
Coverage 99.54% 99.54%
=======================================
Files 61 61
Lines 4147 4160 +13
=======================================
+ Hits 4128 4141 +13
Misses 19 19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zigpy-review-bot
left a comment
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Verdict: looks right end-to-end against firmware, no bellows-side blockers. The wire shape and feature-bit value both match the ZBT-2 firmware exactly; raising mostly follow-up notes since the PR is DRAFT.
Firmware verification (from NabuCasa/silabs-firmware-builder src/zigbee_ncp/extension/xncp_zbt2_extension/src/xncp_zbt2_commands.c):
- Opcode
0x0F00->handle_set_led_state-- matchesXncpCommandId.SET_LED_STATE_REQ. - The handler accepts payload lengths of
3(8-bit/ch) or6(16-bit/ch, big-endian), in r/g/b order. The bellows 3-bytet.uint8_t * 3serialization (FF 80 00for r=255,g=128,b=0) hits the 3-byte branch and the firmware promotes each byte to the high byte of a 16-bit register before callingled_manager_set_color(LED_PRIORITY_MANUAL, color). So(255,128,0)from bellows maps to firmware color(0xFF00, 0x8000, 0x0000)-- consistent with whatled_managerexpects. XNCP_FEATURE_LED_CONTROL = 1 << 31inxncp_types.hmatches the newFirmwareFeatures.LED_CONTROL = 1 << 31. Bit 31 round-trips cleanly throught.bitmap32(verified:int(LED_CONTROL) == 0x80000000, serializes to00 00 00 80, deserializes back to a single member).- Empty
SetLedStateRspmatches the firmware handler (writes status only, no reply bytes).
Local checks: full pytest tests/ passes (425/425). Pre-commit black/flake8/ruff/isort/codespell pass on the changed files. Bellows' [tool.mypy] sets ignore_errors = true, so mypy doesn't gate.
Top notes (all non-blocking, mostly about the consumer-side surface):
- Once you un-draft the ZHA companion (zigpy/zha#715), the consumer reaches into
application_controller._ezsp._xncp_featuresand._ezsp.xncp_set_led_state(...)-- both private-prefixed members. Existing precedent (MEMBER_OF_ALL_GROUPS,MANUAL_SOURCE_ROUTE,RESTORE_ROUTE_TABLE) keeps those internal tobellows.zigbee.application, so this would be the first feature consumed from outside bellows. Worth exposing a public surface onControllerApplication(e.g.set_coordinator_led_state(red, green, blue)that no-ops or raises when the feature bit is absent) so ZHA doesn't have to private-poke. - The firmware also exposes a 6-byte (16-bit/ch) payload format. Not adding it now is fine, but a struct-level option (or a separate
SetLedStateReq16payload) is the only way to drive the LED at >8-bit precision later. Worth a TODO comment near the request payload definition. - There's no command exposed for
led_manager_clear_pattern(LED_PRIORITY_MANUAL)-- the ZHA PR body already flags this. Without it, "off" (rgb=0,0,0) holds the manual override and blocks firmware-driven patterns from re-asserting. If you intend to address this on the bellows side eventually, aCLEAR_LED_STATE_REQopcode reservation (or a comment noting the future plan) would be friendly. - The
0x0F00jump from the contiguous0x0000..0x0008block is meaningful (it's the firmware's per-device extension table base, separate from the core XNCP table); a one-line comment on the enum value would save future readers from wondering about the gap. - Optional: a feature-bit roundtrip assertion would catch any future migration to a narrower bitmap type accidentally truncating bit 31. The project doesn't unit-test other feature bits this way, so this is just a suggestion.
Nothing that should block landing once the companion ZHA PR is ready.
| SET_ROUTE_TABLE_ENTRY_REQ = 0x0006 | ||
| GET_ROUTE_TABLE_ENTRY_REQ = 0x0007 | ||
| GET_TX_POWER_INFO_REQ = 0x0008 | ||
| SET_LED_STATE_REQ = 0x0F00 |
There was a problem hiding this comment.
Consider a brief comment here explaining the 0x0F00 opcode jump -- in the firmware, command IDs 0x0000..0x000F are the adapter-core XNCP table and IDs 0x0F00+ are the per-device extension table (xncp_zbt2_commands[], where 0x0F00 = SET_LED_STATE, 0x0F01 = GET_ACCELEROMETER). Future readers (and you, in six months) will appreciate not having to trace the firmware to understand the gap.
Also a future-proofing thought: the firmware handler accepts both 3-byte (8-bit/ch) and 6-byte (16-bit/ch, big-endian) payload formats, but bellows only emits the 3-byte form. If 16-bit precision ever matters for ZBT-2 LED color smoothness, you'd want either a second payload class (SetLedState16Req) or a flag on this one. Not blocking; worth a TODO.
| TX_POWER_INFO = 1 << 7 | ||
|
|
||
| # The firmware exposes adapter LED control | ||
| LED_CONTROL = 1 << 31 |
There was a problem hiding this comment.
Confirmed against firmware: XNCP_FEATURE_LED_CONTROL = 1 << 31 in src/zigbee_ncp/extension/xncp_extension/inc/xncp_types.h. Bit 31 round-trips cleanly through t.bitmap32 -- serializes to 00 00 00 80 (LE) and deserializes back to a single-member flag. No sign-bit weirdness despite hitting the top of a signed-int32 range, since bitmap32 is unsigned-backed.
The gap between bit 7 (TX_POWER_INFO) and bit 31 (LED_CONTROL) mirrors the firmware's separation between adapter-core features (low bits, sequential) and device-specific features (top of the range). A comment along those lines on the LED_CONTROL line would document the intentional gap.
| class SetLedStateReq(XncpCommandPayload): | ||
| red: t.uint8_t | ||
| green: t.uint8_t | ||
| blue: t.uint8_t |
There was a problem hiding this comment.
Payload shape verified against firmware handle_set_led_state -- with payload_length == 3 it reads payload[0..2] as r/g/b and shifts each into the high byte of a 16-bit color register. t.uint8_t * 3 serializes to exactly red green blue (no padding, no length prefix), so xncp_set_led_state(red=255, green=128, blue=0) lands as wire bytes 00 0F 00 FF 80 00 (cmd-id LE, status, payload) -- which the firmware decodes back to RGB (0xFF00, 0x8000, 0x0000) for the LED driver. Correct.
The firmware also has a 6-byte branch for 16-bit-per-channel color. Out of scope for this PR, but worth a # TODO: 16-bit color uses 6-byte payload (big-endian) if you anticipate adding that later -- the 6-byte branch is big-endian per channel, which is unusual for XNCP, so future-you will want the breadcrumb.
|
|
||
| async def xncp_set_led_state( | ||
| self, red: t.uint8_t, green: t.uint8_t, blue: t.uint8_t | ||
| ) -> None: |
There was a problem hiding this comment.
ZHA pull zigpy/zha#715 ends up calling this directly via application_controller._ezsp.xncp_set_led_state(...) and gating on application_controller._ezsp._xncp_features. That's the first time a feature bit is being consumed from outside bellows.zigbee.application -- the existing MEMBER_OF_ALL_GROUPS/MANUAL_SOURCE_ROUTE/RESTORE_ROUTE_TABLE callers all live inside bellows.
Consider exposing a public method on bellows.zigbee.application.ControllerApplication (e.g. async def set_coordinator_led_state(self, red, green, blue) -> None that checks FirmwareFeatures.LED_CONTROL in self._ezsp._xncp_features and raises a typed CoordinatorFeatureNotSupported -- or returns a sentinel -- otherwise). That gives ZHA a stable public surface to call instead of poking _ezsp directly, and you avoid the implicit-API problem the next time someone refactors EZSP._xncp_features into something else.
No-op if you prefer keeping the consumer inside bellows long-term, but the ZHA PR body explicitly raises this question, so worth answering in this PR's review.
| xncp.SetLedStateReq(red=1, green=2, blue=3) | ||
| ).serialize() | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Test is fine and matches the established pattern in this file. Two optional additions worth considering for follow-up:
- A
FirmwareFeatures.LED_CONTROLroundtrip assertion (serialize + deserialize, confirm the bit survives) -- bit 31 sits at the top of a signed-int32 range and a future migration of the underlying type could silently drop it. Verified locally that the current code is fine, but a regression test costs ~3 lines. - A negative-path test for what happens when the firmware doesn't support the command -- i.e.
customFramereturning anUnknownpayload or a non-OK status.send_xncp_frameraisesInvalidCommandErrorin that case (see__init__.py:752,762,767), but neither the bellows side nor the ZHA-side PR has a test pinning that behavior.
Both are nice-to-have, not project-pattern-required.
2 participants
DRAFT.
Proposed change
This adds XNCP support for coordinators implementing LED control, like the ZBT-2.
Additional information
Please see the linked PR for notes (and shortcomings).
Related ZHA PR: