API to subscribe to multicast groups - #737
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for zigpy’s new coordinator multicast group subscription APIs by wiring bellows’ multicast table controller into the ControllerApplication hooks, and updating tests/dependencies accordingly.
Changes:
- Implement
_subscribe_to_multicast_group()/_unsubscribe_from_multicast_group()inbellows.zigbee.application.ControllerApplication. - Add application-level tests covering subscribe/unsubscribe behavior with and without XNCP extensions.
- Update multicast table “missing entry” logging and bump the zigpy minimum version.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
bellows/zigbee/application.py |
Adds multicast subscribe/unsubscribe hook implementations that delegate to bellows.multicast.Multicast when enabled. |
bellows/multicast.py |
Lowers log severity when unsubscribing a group that is not in the multicast table. |
tests/test_application.py |
Adds tests for the new multicast group subscription APIs under different firmware feature sets. |
pyproject.toml |
Raises the minimum zigpy dependency version to pick up the new APIs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #737 +/- ##
=======================================
Coverage 99.54% 99.54%
=======================================
Files 64 64
Lines 4203 4213 +10
=======================================
+ Hits 4184 4194 +10
Misses 19 19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Comment-only — one small real bug (a broken debug log), the rest nits. This is the thin adapter over Multicast anticipated in zigpy/zigpy#1864: the hook signatures match the merged zigpy API exactly (including the optional endpoint_id=1), keying the table by (group_id, endpoint_id) keeps this path and the existing EZSPGroupEndpoint.add_to_group path deduplicated in one table, and with XNCP MEMBER_OF_ALL_GROUPS the hooks correctly no-op since _multicast stays None. Demoting the unsubscribe missing-entry log from error to debug also makes sense now that unsubscribing an unknown group is a normal API outcome.
- zigpy 2.1.0 was released today, so the
zigpy>=2.1.0pin now resolves — CI should pass on a re-run. The full suite is green locally against zigpy 2.1.0: 430 passed. - The success-path debug log in
_set_multicast_entryhas five placeholders but four arguments, so with debug logging enabled every successful table write logs a--- Logging error ---traceback instead of the message — inline. That's exactly the log a #708 user would be asked to capture, so worth fixing before quirks start driving this path. - Carried over from the #1864 thread, not a defect of this diff: subscriptions made through this hook still don't survive a restart — the NCP table is wiped on reset and
Multicast.startup()replays only the DB-backedep.member_of, while zigpy/zha-device-handlers#5209 still calls only fromasync_configure. If replay ends up being the fix,startup()here is the natural place.
Verification
- Full suite in a clean worktree venv against zigpy 2.1.0: 430 passed in 66s.
- Log bug reproduced end-to-end:
Multicast.subscribe()withlevel=DEBUG→TypeError: not enough arguments for format stringvia the logging error handler. typing.get_type_hints(ControllerApplication._subscribe_to_multicast_group)→AttributeError: module 'bellows.types' has no attribute 'Group'.- Hook signatures diffed against merged zigpy
devzigpy/application.py:1677-1704— exact match, public wrappers defaultendpoint_id=1. - Copilot's two comments checked: the skip-if-API-missing suggestion is moot with zigpy 2.1.0 released and pinned; the "XNCP test asserts nothing" claim is incorrect — it asserts
setMulticastTableEntry.mock_callsis empty. - No reactive auto-subscribe on
send_multicast(unlike zigbee-herdsman) — unchanged by this PR, consistent with the explicit-API design.
Implements zigpy/zigpy#1864. CI will fail until a new version of zigpy is released.