Skip to content

Add XNCP coordinator LED support #710

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
TheJulianJES wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

Add XNCP coordinator LED support #710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions bellows/ezsp/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -854,3 +854,15 @@ async def xncp_get_tx_power_info(self, country_code: str) -> GetTxPowerInfoRsp:
"""Get maximum and recommended TX power for a country (ISO 3166-1 alpha-2)."""
code = country_code.upper().encode("ascii")
return await self.send_xncp_frame(xncp.GetTxPowerInfoReq(country_code=code))

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

@zigpy-review-bot zigpy-review-bot May 23, 2026

Choose a reason for hiding this comment

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

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.

"""Set the adapter LED color."""
await self.send_xncp_frame(
xncp.SetLedStateReq(
red=red,
green=green,
blue=blue,
)
)
17 changes: 17 additions & 0 deletions bellows/ezsp/xncp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ class XncpCommandId(t.enum16):
SET_ROUTE_TABLE_ENTRY_REQ = 0x0006
GET_ROUTE_TABLE_ENTRY_REQ = 0x0007
GET_TX_POWER_INFO_REQ = 0x0008
SET_LED_STATE_REQ = 0x0F00
Copy link
Copy Markdown

@zigpy-review-bot zigpy-review-bot May 23, 2026

Choose a reason for hiding this comment

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

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.


GET_SUPPORTED_FEATURES_RSP = GET_SUPPORTED_FEATURES_REQ | 0x8000
SET_SOURCE_ROUTE_RSP = SET_SOURCE_ROUTE_REQ | 0x8000
Expand All @@ -53,6 +54,7 @@ class XncpCommandId(t.enum16):
SET_ROUTE_TABLE_ENTRY_RSP = SET_ROUTE_TABLE_ENTRY_REQ | 0x8000
GET_ROUTE_TABLE_ENTRY_RSP = GET_ROUTE_TABLE_ENTRY_REQ | 0x8000
GET_TX_POWER_INFO_RSP = GET_TX_POWER_INFO_REQ | 0x8000
SET_LED_STATE_RSP = SET_LED_STATE_REQ | 0x8000

UNKNOWN = 0xFFFF

Expand Down Expand Up @@ -123,6 +125,9 @@ class FirmwareFeatures(t.bitmap32):
# Recommended and maximum TX power can be queried by country code
TX_POWER_INFO = 1 << 7

# The firmware exposes adapter LED control
LED_CONTROL = 1 << 31
Copy link
Copy Markdown

@zigpy-review-bot zigpy-review-bot May 23, 2026

Choose a reason for hiding this comment

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

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 XncpCommandPayload(t.Struct):
pass
Expand Down Expand Up @@ -233,6 +238,18 @@ class GetTxPowerInfoRsp(XncpCommandPayload):
max_power_dbm: t.int8s


@register_command(XncpCommandId.SET_LED_STATE_REQ)
class SetLedStateReq(XncpCommandPayload):
red: t.uint8_t
green: t.uint8_t
blue: t.uint8_t
Copy link
Copy Markdown

@zigpy-review-bot zigpy-review-bot May 23, 2026

Choose a reason for hiding this comment

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

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.



@register_command(XncpCommandId.SET_LED_STATE_RSP)
class SetLedStateRsp(XncpCommandPayload):
pass


@register_command(XncpCommandId.UNKNOWN)
class Unknown(XncpCommandPayload):
pass
20 changes: 20 additions & 0 deletions tests/test_xncp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,3 +314,23 @@ async def test_xncp_get_tx_power_info(ezsp_f: EZSP) -> None:
).serialize()
)
]


async def test_xncp_set_led_state(ezsp_f: EZSP) -> None:
"""Test XNCP set_led_state."""
ezsp_f._mock_commands["customFrame"] = customFrame = AsyncMock(
return_value=[
t.EmberStatus.SUCCESS,
xncp.XncpCommand.from_payload(xncp.SetLedStateRsp()).serialize(),
]
)

await ezsp_f.xncp_set_led_state(red=1, green=2, blue=3)

assert customFrame.mock_calls == [
call(
xncp.XncpCommand.from_payload(
xncp.SetLedStateReq(red=1, green=2, blue=3)
).serialize()
)
]
Copy link
Copy Markdown

@zigpy-review-bot zigpy-review-bot May 23, 2026

Choose a reason for hiding this comment

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

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

  1. 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.
  2. 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.

Loading