Repository navigation
Conversation
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
|
Next on my list is your point 3, splitting 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
2. Do Green Power devices belong in The case for keeping them out: If you would rather have them in |
puddly
left a comment
There was a problem hiding this comment.
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.
| self.groups.add_listener(self._dblistener) | ||
| self.backups.add_listener(self._dblistener) | ||
| self.topology.add_listener(self._dblistener) | ||
| if hasattr(self, "green_power"): |
There was a problem hiding this comment.
Why check for the attribute? I think we can drop this check.
| break | ||
|
|
||
| async def _load_gp_devices(self) -> None: | ||
| if not hasattr(self._application, "green_power"): |
| ) | ||
|
|
||
| async def _save_gp_device(self, device) -> None: | ||
| d = device.as_dict() |
There was a problem hiding this comment.
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.
| # 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]) |
There was a problem hiding this comment.
To avoid hardcoding manually-serialized byte sequences, can GPCommissioningReplyPayload be added to zgp/frame.py?
| """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. |
There was a problem hiding this comment.
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.
| await app.shutdown() | ||
|
|
||
|
|
||
| async def test_no_sidecar_file_created(tmp_path): |
There was a problem hiding this comment.
Where would this zgp_devices.json come from?
|
|
||
| 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 |
There was a problem hiding this comment.
Please move all imports out of function bodies and to the top of the testing module.
| assert rows == [(-1,)] | ||
|
|
||
|
|
||
| async def test_gp_device_tables_fresh_db(tmp_path): |
There was a problem hiding this comment.
These tests can be dropped as their functionality is already covered elsewhere.
| mac_seq_num_capability INTEGER NOT NULL, | ||
| rx_on_capability INTEGER NOT NULL, | ||
| fixed_location INTEGER NOT NULL, | ||
| last_seen TEXT |
There was a problem hiding this comment.
Why is this a string? We use a numerical type for the "normal" Zigbee device table.
|
|
||
| event_type: Final[str] = "gp_device_joined" | ||
|
|
||
| device: GPDevice |
There was a problem hiding this comment.
We should have these events emit IEEE addresses, as serializing a GPDevice object may not be possible in the future.
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.
|
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 One of them I could not replace with a citation, and that is worth flagging rather than papering over. The Structured events. appdb owning serialization. The schema column. Fixed, and I took two more in the same table while I was there: 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 The rest are done as asked: both Kenn Herman's persistence commits are still in here with their authorship intact. |
|
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:
Results.
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 I have the captures as a fixture in the same shape as |
Adds the Green Power (GP / GPEP) runtime on top of the types/frame/crypto parsing core: the
GreenPowerManager(commissioning, pairing, packet dispatch), theGPDevicemodel, the proxy table, and a typed event bus, plus theControllerApplicationwiring — 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_devicestable): 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
application.pywiring, and SQLite persistence (schema v16 + v15→v16 migration).CustomGreenPowerDevice/ GP quirk registry) — a quirks refactor is in flight upstream, so that work is tracked separately.Credits