Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1848 +/- ##
==========================================
- Coverage 99.51% 99.50% -0.01%
==========================================
Files 61 62 +1
Lines 12777 13061 +284
==========================================
+ Hits 12715 12997 +282
- 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.
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.
|
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 |
# Conflicts: # tests/test_appdb.py # tests/zgp/test_device.py # zigpy/appdb.py # zigpy/zgp/__init__.py
|
I've rewritten a large portion of this PR on top of the ZGP constructs we've been slowly merging into zigpy and have been able to commission and receive events from my test devices (EnOcean PTM 215Z, EnOcean PTM 216Z, and a Moes switch) alongside zigpy/bellows#767. Once this is merged, we'll need to wire it up on the ZHA side and then on the Core side. For quirks, zigpy/zha-device-handlers#5273 + zigpy/zha-device-handlers#5273 will integrate the events into ZHA now that we support event entities. |
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
Related: #341