Correctly handle manufacturer_code=None for attributes and commands#1756
Correctly handle manufacturer_code=None for attributes and commands#1756
manufacturer_code=None for attributes and commands#1756Conversation
There was a problem hiding this comment.
Pull request overview
This PR refines how manufacturer_code is modeled and propagated for ZCL attributes and commands so that None consistently means “do not send a manufacturer code” and UNDEFINED means “auto-detect based on definition/cluster context”.
Changes:
- Update
ZCLAttributeDefandZCLCommandDefto treatmanufacturer_code=UNDEFINEDas the auto-detection sentinel andmanufacturer_code=Noneas an explicit “no manufacturer code”, and adjustCluster.find_attributeto resolve attributes using bothis_manufacturer_specificand manufacturer code. - Normalize attribute cache keys and all ZCL cluster operations (read/write/report, events, unsupported tracking, reporting, and commands) to use effective manufacturer codes consistently, including on manufacturer-specific clusters and in quirks.
- Extend and adjust tests and fixtures to cover the new
manufacturer_code=Nonesemantics, complex attribute read grouping, manufacturer overrides for commands, and quirks behavior, including manufacturer handling in mocked reads/reports.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
zigpy/zcl/helpers.py |
Introduces _cache_key to normalize attribute cache keys by resolving UNDEFINED manufacturer codes to None, ensuring cache lookups and unsupported tracking align with effective manufacturer usage. |
zigpy/zcl/foundation.py |
Changes ZCLCommandDef/ZCLAttributeDef.manufacturer_code to use UNDEFINED as the default sentinel, adds auto-setting of is_manufacturer_specific when a non-sentinel manufacturer code is supplied, and ensures manufacturer-specific definitions without explicit codes normalize to UNDEFINED. |
zigpy/zcl/__init__.py |
Reworks attribute indexing into _attributes_by_id grouped by is_manufacturer_specific and manufacturer code, updates find_attribute logic for the new None vs UNDEFINED semantics, centralizes manufacturer-code resolution in _get_effective_manufacturer_code, wires that through reads/writes/reporting/unsupported/events, and adjusts command/client_command to respect explicit manufacturer overrides and quirks. |
tests/test_zcl.py |
Adds unit tests for attribute lookup and read grouping across manufacturer codes, ensures explicit manufacturer_code=None on manufacturer-specific clusters suppresses the manufacturer header, verifies explicit manufacturer overrides for commands, and tightens tests around quirk-driven attribute report/update behavior. |
tests/conftest.py |
Updates the attribute report helper to use _get_effective_manufacturer_code with the new default/UNDEFINED semantics when computing manufacturer codes for mocked reports. |
💡 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 #1756 +/- ##
==========================================
- Coverage 99.52% 99.51% -0.01%
==========================================
Files 63 63
Lines 12654 12667 +13
==========================================
+ Hits 12594 12606 +12
- Misses 60 61 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@TheJulianJES We had a small hole in the database migration that prevented some manufacturer-specific attributes from being migrated. This has been fixed in this PR as well. You can probably re-trigger this by downgrading your database version with |
|
Do you think it matters for those who already upgraded to the HA beta? |
|
It's all cached data so it probably has been updated by now, probably not. |
TheJulianJES
left a comment
There was a problem hiding this comment.
Changes make sense to me
See zigpy/zha-device-handlers#4694 for more context.
manufacturer_codein the ZCL attribute definitions should haveNonemean "do not provide a manufacturer code". A specific value should be used otherwise.UNDEFINED, the default, allows for auto-detection from the cluster ID and other context (e.g. justis_manufacturer_specific=True).