Add XNCP coordinator LED support by TheJulianJES · Pull Request #710 · zigpy/bellows

TheJulianJES · 2026-03-21T03:15:46Z

Proposed change

This adds XNCP support for coordinators implementing LED control, like the ZBT-2.

FirmwareFeatures.LED_CONTROL (1 << 31) and the SET_LED_STATE command (0x0F00, response 0x8F00) with an 8-bit red/green/blue payload, matching the ZBT-2 Zigbee NCP firmware (NabuCasa/silabs-firmware-builder, xncp_types.h and xncp_zbt2_commands.c).
EZSP.xncp_set_led_state(red, green, blue) sends it.
The branch was merged with dev (conflict only in bellows/ezsp/xncp.py, next to the new SEND_UNICAST command and COMBINED_SEND feature bit; both kept). The tests pass and pre-commit is clean.

Additional information

Please see the linked PR for notes (and shortcomings).

Related ZHA PR:

Add ZBT-2 coordinator LED support [RFC/DRAFT/WIP] zha#715

codecov · 2026-03-21T03:17:51Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.59%. Comparing base (1374aae) to head (6c73f46).

Additional details and impacted files

@@           Coverage Diff           @@
##              dev     #710   +/-   ##
=======================================
  Coverage   99.59%   99.59%           
=======================================
  Files          64       64           
  Lines        4417     4430   +13     
=======================================
+ Hits         4399     4412   +13     
  Misses         18       18

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

zigpy-review-bot

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 -- matches XncpCommandId.SET_LED_STATE_REQ.
The handler accepts payload lengths of 3 (8-bit/ch) or 6 (16-bit/ch, big-endian), in r/g/b order. The bellows 3-byte t.uint8_t * 3 serialization (FF 80 00 for 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 calling led_manager_set_color(LED_PRIORITY_MANUAL, color). So (255,128,0) from bellows maps to firmware color (0xFF00, 0x8000, 0x0000) -- consistent with what led_manager expects.
XNCP_FEATURE_LED_CONTROL = 1 << 31 in xncp_types.h matches the new FirmwareFeatures.LED_CONTROL = 1 << 31. Bit 31 round-trips cleanly through t.bitmap32 (verified: int(LED_CONTROL) == 0x80000000, serializes to 00 00 00 80, deserializes back to a single member).
Empty SetLedStateRsp matches 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_features and ._ezsp.xncp_set_led_state(...) -- both private-prefixed members. Existing precedent (MEMBER_OF_ALL_GROUPS, MANUAL_SOURCE_ROUTE, RESTORE_ROUTE_TABLE) keeps those internal to bellows.zigbee.application, so this would be the first feature consumed from outside bellows. Worth exposing a public surface on ControllerApplication (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 SetLedStateReq16 payload) 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, a CLEAR_LED_STATE_REQ opcode reservation (or a comment noting the future plan) would be friendly.
The 0x0F00 jump from the contiguous 0x0000..0x0008 block 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.

zigpy-review-bot · 2026-05-23T03:22:18Z

    SET_ROUTE_TABLE_ENTRY_REQ = 0x0006
    GET_ROUTE_TABLE_ENTRY_REQ = 0x0007
    GET_TX_POWER_INFO_REQ = 0x0008
+    SET_LED_STATE_REQ = 0x0F00


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.

zigpy-review-bot · 2026-05-23T03:22:18Z

    TX_POWER_INFO = 1 << 7

+    # The firmware exposes adapter LED control
+    LED_CONTROL = 1 << 31


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.

zigpy-review-bot · 2026-05-23T03:22:18Z

+class SetLedStateReq(XncpCommandPayload):
+    red: t.uint8_t
+    green: t.uint8_t
+    blue: t.uint8_t


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.

zigpy-review-bot · 2026-05-23T03:22:18Z

+
+    async def xncp_set_led_state(
+        self, red: t.uint8_t, green: t.uint8_t, blue: t.uint8_t
+    ) -> None:


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.

I also don't really like ZHA reaching into bellows like this, but I feel like it's acceptable for a "one-off" feature like this. We can always move this to a different level / make it more generic in the future.

The only other radio stack with LED support is Z-Stack and it's usually for the tiny red LED on a PCB.

(So you're saying we should add light entities to zigpy, so we can have radio libraries directly exposing different LED types 😆)

Well 😅 zigpy/zigpy@fc4e805...puddly:zigpy:puddly/rgb-led-api

Haha, that was unexpected 😁.

Honestly though, it's a small/clean change, so we could also go with that now already..? And add a color mode/type for (Z-Stack's) on/off LEDs? (I'd be fine with doing that or not doing it)

A zigpy API won't hurt (I guess we could also expose this in Ziggurat for the ZBT-2) but I feel this might have to stay a bellows-only feature unless this can be turned into a simple firmware version check: https://github.com/zigpy/zigpy-znp/blob/926626c4035b75f943182e7f41ca5f7c97da4ac1/zigpy_znp/zigbee/application.py#L599-L614

zigpy-review-bot · 2026-05-23T03:22:18Z

+                xncp.SetLedStateReq(red=1, green=2, blue=3)
+            ).serialize()
+        )
+    ]


Test is fine and matches the established pattern in this file. Two optional additions worth considering for follow-up:

A FirmwareFeatures.LED_CONTROL roundtrip 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. customFrame returning an Unknown payload or a non-OK status. send_xncp_frame raises InvalidCommandError in 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.

zigpy-review-bot · 2026-11-12T03:09:35Z

Pushed 6c73f46 to tjj/zbt2_led (fast-forward), requested from zigpy/zha#715. The PR was conflicting with dev and is now mergeable.

What changed

Merged dev. The only conflict was in bellows/ezsp/xncp.py, where dev added the SEND_UNICAST command and the COMBINED_SEND feature bit in the same places. Both sides are kept, and the net diff against dev is unchanged apart from a blank line (3 files, +49).
Added a blank line before the LED_CONTROL feature bit, as every other FirmwareFeatures entry has one.
The PR body now lists what the PR adds. The title is unchanged. The PR is still a draft.

Checks

The full suite passes (768 tests), and pre-commit is clean on the changed files.
One review round with an Opus sub-agent found nothing material; it checked the merge resolution and the bit and command ID collisions. Copilot was not run for this 49-line diff.
The command ID 0x0F00, the LED_CONTROL bit 1 << 31 and the 3-byte 8-bit RGB payload match the ZBT-2 NCP firmware (xncp_types.h and xncp_zbt2_commands.c in NabuCasa/silabs-firmware-builder).

zigpy-review-bot

Re-review at 6c73f46 (after the merge with dev): no blockers, nothing new since my earlier review.

The merge kept both sides in bellows/ezsp/xncp.py (SEND_UNICAST_REQ / COMBINED_SEND next to the new SET_LED_STATE_REQ / LED_CONTROL), and the net change is still the 3 files / +49 lines. The new SetLedStateReq / SetLedStateRsp registrations, the 0x0F00 / 0x8F00 IDs and the bit 31 feature flag have no collisions with the existing entries, and tests/test_xncp.py passes in a fresh venv at this head (15 tests).

The five inline threads from my earlier review are all optional follow-ups (a comment on the 0x0F00 opcode gap and the bit 31 gap, a TODO for the 6-byte 16-bit payload, a public ControllerApplication surface for ZHA to call instead of reaching into _ezsp, and two extra tests). None of them was acted on, so I left them open for the author to decide. The public-API question is the only one worth settling before this leaves draft, since zigpy/zha#715 is the first caller outside bellows.zigbee.application.

Copilot

🟢 Approval recommended

No active review issues remain for the XNCP LED-control changes.

No open issues 🎉

What changed in this PR

Adds XNCP LED-control support for compatible Zigbee coordinators, including the ZBT-2, exposing RGB control through the existing EZSP interface.

Changes:

Defines the LED-control feature flag, command IDs, and RGB request payload.
Adds EZSP.xncp_set_led_state(red, green, blue).
Adds a test verifying the helper sends the expected XNCP command.

File	Description
`tests/test_xncp.py`	Tests LED-state command construction and dispatch.
`bellows/ezsp/xncp.py`	Registers the LED-control feature, command IDs, and payload types.
`bellows/ezsp/__init__.py`	Exposes an asynchronous helper for setting the adapter LED color.

_{Review effort: Balanced}

Add XNCP coordinator LED support

0b421e6

TheJulianJES mentioned this pull request Mar 21, 2026

Add ZBT-2 coordinator LED support [RFC/DRAFT/WIP] zigpy/zha#715

Draft

zigpy-review-bot reviewed May 23, 2026

View reviewed changes

zigpy-review-bot added the new feature Adds new feature label Oct 10, 2026

zigpy-review-bot added 2 commits November 12, 2026 04:07

Merge branch 'dev' into tjj/zbt2_led

d7b686f

Add a blank line before LED_CONTROL feature bit

6c73f46

TheJulianJES requested review from zigpy-review-bot and a balanced review from Copilot November 12, 2026 03:10

Copilot started reviewing on behalf of TheJulianJES November 12, 2026 03:11

View session

zigpy-review-bot reviewed Nov 12, 2026

View reviewed changes

Copilot AI reviewed Nov 12, 2026

View reviewed changes

TheJulianJES marked this pull request as ready for review November 12, 2026 03:15

Conversation

TheJulianJES commented Mar 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change

Additional information

Uh oh!

codecov Bot commented Mar 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zigpy-review-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zigpy-review-bot commented Nov 12, 2026

What changed

Checks

Uh oh!

zigpy-review-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TheJulianJES commented Mar 21, 2026 •

edited

Loading

codecov Bot commented Mar 21, 2026 •

edited

Loading