Sitelet https://github.com/zigpy/zigpy/pull/1848
Skip to content

zgp: add the Green Power manager, device, and proxy runtime - #1848

Open
nmingam wants to merge 18 commits into
zigpy:devfrom
nmingam:zgp-manager
Open

nmingam wants to merge 18 commits into
zigpy:devfrom
nmingam:zgp-manager

Conversation

@nmingam

@nmingam nmingam commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Adds the Green Power (GP / GPEP) runtime on top of the types/frame/crypto parsing core: the GreenPowerManager (commissioning, pairing, packet dispatch), the GPDevice model, the proxy table, and a typed event bus, plus the ControllerApplication wiring — GP frame interception on endpoint 242 / cluster 0x0021, permit_gp, and manager shutdown.

Green Power devices are persisted in the zigpy SQLite database (schema v16, gp_devices table): commissioned GPDs and their frame counters survive a restart, with the frame counter updated per press (not only at join) to keep replay protection correct.

Stacked on #1847

This PR is stacked on #1847 (the GP types/frame/crypto parsing core). Review and merge #1847 first; the diff here is the manager-stage delta only. Until #1847 merges, its single commit shows in this diff and drops automatically on rebase.

Scope

  • Included: GP runtime (manager, device, proxy, events), application.py wiring, and SQLite persistence (schema v16 + v15→v16 migration).
  • Excluded: GP quirks (CustomGreenPowerDevice / GP quirk registry) — a quirks refactor is in flight upstream, so that work is tracked separately.

Credits

@codecov

codecov Bot commented Jul 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.59759% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.50%. Comparing base (f8b0cb8) to head (db34f39).

Files with missing lines Patch % Lines
zigpy/appdb.py 96.22% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##              dev    #1848    +/-   ##
========================================
  Coverage   99.50%   99.50%            
========================================
  Files          59       63     +4     
  Lines       12412    12908   +496     
========================================
+ Hits        12350    12844   +494     
- Misses         62       64     +2     

☔ 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.

@nmingam

nmingam commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Next on my list is your point 3, splitting Device into BaseDevice / ZigbeeDevice / GreenPowerDevice. Two questions before I write it, because both are cheap to answer now and expensive to undo later.

1. Do you want to write the split yourself? It touches the most-subclassed class in the ecosystem and you may already have a shape in mind. I am happy to do it or to stay out of it, but I would rather not hand you a competing version of something you were going to write.

If I do write it, my plan is a thin BaseDevice holding only identity and liveness - application, ieee, manufacturer, model, last_seen, lqi, rssi, skip_configuration, create_task, on_remove - with everything mesh-shaped staying exactly where it is. Two things I found while scoping it that are worth flagging either way:

  • Device.__init__ never calls super().__init__(); it hand-assigns self._listeners = {}. So ListenableMixin.__init__ has never actually run for a Device. Harmless today, but reparenting has to fix it rather than inherit the bug.
  • Keeping the concrete class named Device with ZigbeeDevice as an alias avoids churning quirk_class in diagnostics, which is built from __class__.__module__ + __name__ and appears in 479 zha test fixtures. Renaming outright is a one-line change if you would rather have the name.

2. Do Green Power devices belong in app.devices, or in a parallel registry? This forks everything downstream, and I read your "rework ZHA to work with information from both" as pointing at two sources, so that is what I have been building toward - but I would rather confirm than assume.

The case for keeping them out: app.green_power.devices is already a public property, and zha's get_or_create_device keys on zigpy_device.ieee and never consults app.devices, so the second enumeration costs about ten lines in zha and no new zigpy API. Putting them in app.devices means either a DB_VERSION bump - and since DB_V is a global table-name suffix, v17 recreates all thirteen tables - or a device sitting in the registry while being deliberately excluded from that registry's persistence, which is the worse half of the two.

If you would rather have them in app.devices, say so and I will do that instead; the class hierarchy is the same either way, so it is a small change of direction now and a large one later.

@puddly puddly left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the PR! I've left a few comments.

Regarding the Device base class: I'm going to spend a bit more time working on how this integrates with ZHA but I think the core of this PR (the manager, database schema, proxying, etc.) should be good.

One concern I have are the myriad references to zigbee-herdsman: is this a translation of the GP code from zigbee-herdsman? We generally perform direct spec implementations in zigpy and then validate with other repos after the fact, as otherwise structural design issues and bugs can migrate across projects.

Comment thread zigpy/application.py Outdated
self.groups.add_listener(self._dblistener)
self.backups.add_listener(self._dblistener)
self.topology.add_listener(self._dblistener)
if hasattr(self, "green_power"):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why check for the attribute? I think we can drop this check.

Comment thread zigpy/appdb.py Outdated
break

async def _load_gp_devices(self) -> None:
if not hasattr(self._application, "green_power"):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same hasattr check here.

Comment thread zigpy/appdb.py Outdated
)

async def _save_gp_device(self, device) -> None:
d = device.as_dict()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it would be best for appdb to handle the serialization/deserialization internally, as these are to an extent database operations, not something directly implemented by the device itself.

Comment thread zigpy/zgp/manager.py Outdated
# Commissioning Reply payload (cmd 0xF0):
# 1 byte options: 0x00 = no PAN ID, no key, no key encryption,
# no security level
commissioning_reply_payload = bytes([0x00])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

To avoid hardcoding manually-serialized byte sequences, can GPCommissioningReplyPayload be added to zgp/frame.py?

Comment thread zigpy/zgp/manager.py Outdated
"""Green Power Manager: processes GP frames on cluster 0x0021, endpoint 242.

Manages GP device lifecycle (commissioning, decommissioning) and dispatches
GP commands to listeners. Equivalent to zigbee-herdsman's greenPower.ts.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this implementation a translation from zigbee-herdsman? We try to implement things from the specifications, as otherwise bugs get copy/pasted between libraries. If not, let's remove these LLM-isms: matching zigbee-herdsman appears six times within this PR.

Comment thread tests/test_appdb.py Outdated
await app.shutdown()


async def test_no_sidecar_file_created(tmp_path):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Where would this zgp_devices.json come from?

Comment thread tests/test_appdb.py Outdated

async def test_gp_device_round_trip(tmp_path):
"""Emit DeviceJoined, shutdown, reopen - device restored with all fields intact."""
from zigpy.zgp.device import GPDevice

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please move all imports out of function bodies and to the top of the testing module.

Comment thread tests/test_appdb_migration.py Outdated
assert rows == [(-1,)]


async def test_gp_device_tables_fresh_db(tmp_path):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These tests can be dropped as their functionality is already covered elsewhere.

Comment thread zigpy/appdb_schemas/schema_v16.sql Outdated
mac_seq_num_capability INTEGER NOT NULL,
rx_on_capability INTEGER NOT NULL,
fixed_location INTEGER NOT NULL,
last_seen TEXT

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this a string? We use a numerical type for the "normal" Zigbee device table.

Comment thread zigpy/zgp/events.py Outdated

event_type: Final[str] = "gp_device_joined"

device: GPDevice

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should have these events emit IEEE addresses, as serializing a GPDevice object may not be possible in the future.

nmingam and others added 14 commits August 6, 2026 11:13
Add the GP runtime on top of the types/frame/crypto core: the
GreenPowerManager (commissioning, pairing, packet dispatch), GPDevice
model, proxy table, and typed event bus, plus ControllerApplication
wiring (GP frame interception on endpoint 242 / cluster 0x0021,
permit_gp, and manager shutdown).
Hook Green Power device persistence into the real zigpy SQLite database
instead of relying on the in-memory-only manager. Schema v16 adds the
gp_devices_v16 table (one row per commissioned GPD: source id, device id,
security key/level/type, frame counter, manufacturer/model, command and
cluster lists, capability flags, last_seen).

The PersistingListener subscribes to the GreenPowerManager's DeviceJoined
and DeviceLeft events and writes through to the DB, so commissioned GPDs
and their frame counters survive a restart. On startup, _load_gp_devices
rehydrates the manager via its existing load_devices() entry point.

GPDevice.as_dict()/from_dict() already define the serialization contract;
this change persists it. _save_gp_device uses INSERT ... ON CONFLICT DO
UPDATE across all mutable columns so a re-commission with a new security
key replaces the stored key. A v15->v16 migration carries existing tables
forward and creates the new (empty) gp_devices table.

Writes happen on commission/decommission only - not per received frame -
so there is no per-frame write amplification.
  - test_appdb: no JSON sidecar is written; GP device round-trips across
    a restart; decommission removes it; re-commission updates the stored
    security key
  - test_appdb_migration: a fresh DB has the gp_devices table; v15->v16
    migration adds it empty while preserving existing device rows
  - databases/simple_v15.sql: minimal v15 fixture for the migration test
Our GP persistence subscribed only to DeviceJoined/DeviceLeft, so the
stored frame_counter was frozen at its commissioning value.  After a
restart the replay-protection baseline reverted to that stale counter
until the next press re-advanced it - a small replay-protection
regression and a divergence from the in-memory counter the manager
maintains via GPDevice.update_frame_counter.

Subscribe appdb to CommandReceived and issue a lightweight single-row
UPDATE of frame_counter (+ last_seen) keyed by source_id, rather than
the full-row as_dict() rewrite _save_gp_device does - the join-time row
already holds the static fields.  The manager advances the counter
(replay protection) before emitting the event, so this only adds the DB
write.  Mirrors the konistehrad GP draft's gpd_counter_updated pattern.

Adds test_gp_frame_counter_persists_per_press: a press advances 99->150,
and after reopen the restored counter is 150, not the stale join-time 99.
permit() now opens the GP commissioning window on every path, so the
broadcast permit sends an extra ProxyCommissioningMode frame; update the
existing assertion and add coverage for the broadcast and node paths.
frame.py now hands out deserialize(), KeyData keys, uint1_t flags,
raw channel nibbles, and None for absent optional lists; coerce at
the GPDevice boundary and at the decrypt call.
The crypto now takes the GPDF header as CCM* associated data. A GP
Notification cannot supply it: the frame (Figure 23) carries no MIC, and
its Options field (Figure 24) carries neither RxAfterTx nor the extended
NWK frame control, which are part of the header the MIC covers
(A.1.5.4.3.3). The sink has nothing to rebuild the header from.

It does not need to. A proxy does not send a GP Notification for a frame
whose security processing failed (A.3.5.2), so the payload arrives as
the GPP processed it.

Payloads are now emitted unmodified. The previous code stripped the last
four bytes as a MIC; on this path they are command data.
Security level 0b01 has no defined processing (A.1.4.1.3). Accepting it
at commissioning would store a GPD whose level the crypto refuses, and
push that level to every proxy in the GP Pairing.
The Options field of a GP Notification (Figure 24) announces the
security level the forwarding proxy saw on the GPDF. A level other than
the one the GPD was commissioned with means the frame is not what that
GPD sends: an unprotected GPDF injected on air would otherwise pass for
a command from a device commissioned with security.

A.3.5.2.5 runs this ahead of the freshness check, and so does this: a
rejected frame never reaches update_frame_counter, which would
otherwise advance the stored value past the real device's and lock it
out.

A.3.5.2.5 compares the SecurityKeyType as well, and that part is left
out. It is not carried on the GPDF: the proxy fills it from its own
Sink Table, and EZSP reports NoKey because zigpy never programs the
NCP's GP tables. Comparing it would drop every frame from a secured
GPD.

Commissioning, decommissioning, channel requests and success reports
are handled earlier: a GPD sending a Commissioning Request is not
commissioned yet, so there is nothing to compare.
The Channel Configuration and Commissioning Reply payloads were packed
by hand as literal bytes, with a comment describing each bit. Both are
defined in zgp/commands now, so build them from the structs and let the
struct carry the layout.

The bytes on the wire are unchanged. uint4_t does reject an
out-of-range operational channel where the old mask wrapped it, which
is what _send_gp_response already does for the transmit channel.
The manager is written against the specification; herdsman was only a
cross-check when the spec was ambiguous or when a tester reported a
failure. Pointing a reader at it for the rationale sends them to the
wrong document, and lets a bug in either library read as intent in the
other.

Each reference is replaced by what it was standing in for: the GPD key
in the GP Pairing is protected per A.3.7.1.2.3, and the SrcID-only
support is stated as the limitation it is.

The communication mode is called what it is. Table 27 lists the
gpsCommunicationMode values, but nothing in the specification requires
picking between them the way this sink does, so the docstring says so
rather than implying otherwise.
ControllerApplication assigns self.green_power unconditionally in
__init__, so the hasattr() guards in _add_db_listeners and
_load_gp_devices can never be false. They only stand to hide a real
AttributeError if that ever stops being true.

_gp_unsubs gets the same treatment: initialise it in __init__ rather
than reading it back with a getattr() default.
The v16 schema is additive, so every existing migration from an older
fixture already runs _migrate_to_v16 and asserts the resulting
user_version. A dedicated v15 fixture and a v15-to-v16 test add no
coverage of their own.

test_no_sidecar_file_created asserted that a zgp_devices.json is
absent. Nothing in the tree writes such a file, so the assertion held
regardless of what the code did.
The zgp imports in test_appdb sat inside the test bodies. appdb itself
imports zigpy.zgp.events at module level, so there was never a cycle to
avoid by deferring them.

_commission() in test_real_frames wrapped two statements behind one
name for two call sites; both read better with the calls in place.
GPDevice.as_dict()/from_dict() encoded database concerns into the device
model - hex for the key, JSON for the cluster lists, ISO 8601 for the
timestamp - and the manager carried load_devices()/get_devices_data()
only to hand those dicts to appdb.

The mapping moves into _save_gp_device and _load_gp_devices, where
_raw_device_initialized_internal and _load_devices already do the same
for a normal device, and a restored device goes through the manager's
existing add_device(). GPDevice is left as the model and the manager as
the registry.

Loading no longer skips a row it cannot parse. _load_devices does not
either, and swallowing per row hides schema drift: a corrupt key column
now fails the load instead of silently dropping one device.
gp_devices_v16 stored last_seen as an ISO 8601 string where devices_v16
in the same schema uses a REAL, so the two device tables could not be
queried the same way and every write paid a format conversion.

manufacturer_id and model_id were TEXT while GPDevice holds them as
ints. SQLite's TEXT affinity converted them on the way in, so a
restored device came back with str IDs where a freshly commissioned one
had ints.

GPDevice now mirrors zigpy.device.Device: a private datetime behind a
float property whose setter also takes a timestamp. Schema v16 has
never shipped, so the columns are changed in place rather than migrated.
CommandReceived carried the GPD payload as bytes and every event
carried the GPDevice itself. zgp/commands parses most command payloads
now, so emit the parsed struct and let a subscriber rely on it.

Commands with no schema, and payloads that fail to deserialize, become
RawCommandReceived rather than being dropped. The frame counter has
already advanced by then, so the replay baseline still has to be
persisted, and one device sending a malformed frame must not silence
the others. Adding a schema to zgp/commands moves a command from one
event to the other.

The events carry device_ieee, the way the ZCL events already do, so
nothing downstream has to serialize a GPDevice. appdb looks the device
back up, except on DeviceLeft, where the manager has already dropped it
and only the address is left to work from.
@nmingam

nmingam commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review, and for #1872 - the command payloads landing upstream made the events change much smaller than it would have been. Rebased onto it and worked through the comments. Taking the substantive ones first.

Is this a translation from zigbee-herdsman? No. It was written against the specification, in Python, from the start. Where I used herdsman was as a cross-reference: when the spec left something ambiguous, or when a tester reported a device behaving in a way my reading did not predict, I checked what the working implementation did before deciding. Two of those checks turned out to be real bugs on my side, including one where the GPD key was going out in the clear in the GP Pairing.

That is still not the same as "spec first, validate after", and your point stands that leaving matching zigbee-herdsman in the source sends the next reader to the wrong document. Every reference is gone. Each one is replaced by what it was actually standing on: the key in the GP Pairing is protected per A.3.7.1.2.3, the same section crypto.py already cites, and the SrcID-only support is stated as the limitation it is.

One of them I could not replace with a citation, and that is worth flagging rather than papering over. The send_pairing docstring claimed the communication-mode choice matched herdsman. Going back to the spec, gpsCommunicationMode is a sink attribute and Table 27 only enumerates the values - nothing requires deriving the mode per pairing from whether a proxy forwarded the notification. So that logic is this sink's own policy, and the docstring now says exactly that instead of implying a normative basis. If you would rather it followed a fixed configured mode, that is a small change and I would rather hear it now.

Structured events. CommandReceived.payload is now the parsed struct from zgp/commands, and the events carry device_ieee instead of the GPDevice, matching the shape the ZCL events already use. Commands with no schema, and payloads that fail to deserialize, go out as RawCommandReceived rather than being dropped: the frame counter has already advanced by then, so the replay baseline still has to be persisted, and one device sending a malformed frame should not silence the others. Adding a schema to zgp/commands moves a command from one event to the other. Trailing bytes after a successful parse are logged and the parsed value is still emitted, following what zcl/__init__.py does for a ZCL frame.

appdb owning serialization. as_dict/from_dict are gone from GPDevice, along with load_devices/get_devices_data on the manager, which only existed to shuttle those dicts. _save_gp_device and _load_gp_devices now do the column mapping the way _raw_device_initialized_internal and _load_devices already do it, and a restored device goes back in through the manager's own add_device(). One consequence worth naming: loading no longer skips a row it cannot parse, matching _load_devices.

The schema column. Fixed, and I took two more in the same table while I was there: manufacturer_id and model_id were also TEXT, while GPDevice holds them as ints, so SQLite's affinity was converting them and a restored device came back with strings where a freshly commissioned one had numbers. GPDevice.last_seen now mirrors zigpy.device.Device - a private datetime behind a float property whose setter also accepts a timestamp.

One thing to know before testing this branch. Schema v16 has never shipped, so I changed the columns in place rather than adding a v17. Anyone who already ran an earlier build of this PR has a database at user_version = 16 with the old text columns; migrations will skip it and the GP load will fail on startup. Deleting the gp_devices_v16 rows, or the database, is enough. Say the word if you would rather have the v17 migration instead.

The rest are done as asked: both hasattr checks dropped (and a getattr default next to them that would have drawn the same comment), the v15 fixture and the two migration tests removed, test_no_sidecar_file_created deleted - you were right that nothing in the tree ever writes that file, so it was asserting against something that could never happen - the test imports hoisted, and _commission() inlined into its two call sites. GPCommissioningReplyPayload and GPChannelConfigurationPayload both come from your #1872 now, so both hand-packed byte sequences are gone; I checked the serialized bytes are unchanged for every channel in range.

Kenn Herman's persistence commits are still in here with their authorship intact.

@bedge117

bedge117 commented Sep 5, 2026 •

Copy link
Copy Markdown

Tested this branch end-to-end against real hardware: Home Assistant SkyConnect (EmberZNet 7.5.1, EZSP v13) via bellows, with an EnOcean PTM 215Z (Philips Hue Tap) as the GPD.

Setup. Two things were needed outside this PR to get frames in at all:

  1. bellows' gpepIncomingMessageHandler schema fails to parse EmberZNet 7.x frames (fix submitted as Fix gpepIncomingMessageHandler schema for EZSP v13+ bellows#746).
  2. bellows does not route parsed gpep frames anywhere. For the test I subclassed ControllerApplication, intercepted gpepIncomingMessageHandler in ezsp_callback_handler, and re-emitted it through packet_received as a ZigbeePacket to endpoint 242 / cluster 0x0021 carrying a ZCL GreenPowerProxy server command: commissioning_notification for 0xE0 frames, notification otherwise, with NotificationOptions/CommissioningNotificationOptions filled from the EZSP fields (application_id, security_level, security_key_type).

Results.

  • Frames from the not-yet-commissioned device were dropped by _dispatch_gp_command as expected ("GP command from unknown device").
  • Commissioning: holding the channel-11 button for 10 s produced the 0xE0 frame (security level 0, counter 0xFFFFFFFF, 27-byte payload, no AppInfo). _process_commissioning unwrapped the key, registered the device (device_id=0x02, security=FullFrameCounterAndMIC, IndividualKey) and emitted DeviceJoined.
  • Operational frames after that: parsed by _handle_gp_notification and dispatched as CommandReceived with the expected commands (Toggle, RecallScene1, RecallScene2), one per physical press, frame counter incrementing.
  • The schema_v16 GP device table round-trips: on restart the device is restored from the database.

One finding. The GPD re-broadcasts its commissioning frame several times while the button is held; each one was processed as a fresh commissioning and emitted another DeviceJoined for the same source ID. _process_commissioning creates and adds a new GPDevice unconditionally, so an already-known device probably wants idempotent handling there.

I have the captures as a fixture in the same shape as tests/zgp/fixtures/busch_jaeger_6716u.py (commissioning payload re-wrapped with a fixture key rather than the device's own, operational frames as captured) plus tests, including an xfail(strict=True) for the finding above. Happy to open that as a PR against this branch if useful.

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