Make `update_attribute` manufacturer code aware by TheJulianJES · Pull Request #1760 · zigpy/zigpy

TheJulianJES · 2026-02-05T11:02:09Z

Proposed change

This removes the legacy fallback in _legacy_apply_quirk_attribute_update to insert values into the legacy cache for attributes that have a shared ID. A ContextVar with the attribute ID and manufacturer code is used to preserve the original manufacturer code sent by the device and later update the typed cache with that.

Additional information

An additional PR will be made later to fix an issue where UnsupportedError is raised too early, before the legacy cache is accessed.

Later, we can also merge something like TheJulianJES/zigpy@tjj/manufacturer_update_fix...TheJulianJES:zigpy:tjj/manufacturer_update_fix_with_mfr_attr_fix_zcl_cache. See the comments for more details on this.

codecov · 2026-02-05T11:07:27Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.51%. Comparing base (b2ee3c3) to head (eedddbf).
⚠️ Report is 7 commits behind head on dev.

Additional details and impacted files

@@           Coverage Diff           @@
##              dev    #1760   +/-   ##
=======================================
  Coverage   99.51%   99.51%           
=======================================
  Files          63       63           
  Lines       12676    12682    +6     
=======================================
+ Hits        12615    12621    +6     
  Misses         61       61

☔ View full report in Codecov by Sentry.
📢 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.

TheJulianJES · 2026-02-12T17:32:38Z

+        # Get the manufacturer code from context if set (set by
+        # _legacy_apply_quirk_attribute_update to preserve manufacturer code).
+        # Only apply when the attribute ID matches the original to avoid affecting
+        # other attributes that quirks may update.
+        ctx = _attribute_update_context.get()
+
        try:
-            attr_def = self.find_attribute(attrid)
+            if ctx is not None and ctx[0] == attrid:
+                attr_def = self.find_attribute(attrid, manufacturer_code=ctx[1])
+            else:
+                attr_def = self.find_attribute(attrid)
        except KeyError:
            if value is not None:
                self._attr_cache.set_legacy_value(attrid, value)


Both with and without this PR, we persist manufacturer manufacturer specific attribute reports into the mfr-keyed attribute cache if there's only a ZCL definition. IMO, we should adjust this behavior to only insert into the new typed cache if there's a proper attribute definition with the same manufacturer code.

If there isn't, we should only insert into legacy cache IMO. But currently, the legacy cache really isn't used, as cluster.get(some_legacy_attr) won't return the legacy cache value.

But right now, we're somewhat polluting the new attribute cache with manufacturer_code=None for manufacturer-specific attributes if there's no definition for them yet.

We can still go ahead with this PR now, as it doesn't change this behavior. But we should address that issue very soon.

A nice fix for this, built on top of this PR, would be the following: TheJulianJES/zigpy@tjj/manufacturer_update_fix...TheJulianJES:zigpy:tjj/manufacturer_update_fix_with_mfr_attr_fix_zcl_cache

It's (probably?) not something we should do for a patch release, but this should work nicely.

But currently, the legacy cache really isn't used, as cluster.get(some_legacy_attr) won't return the legacy cache value.

Do you mean that we don't allow persisting attributes without a corresponding definition? I believe the few quirks that rely on this behavior directly mess with _attr_cache.

Do you mean that we don't allow persisting attributes without a corresponding definition?

They should be persisted into the database.

(But for attribute reports that do not have a corresponding definition, the unmodified value will land in the DB. With the modified one being stored in the runtime legacy cache. Though that seems to never be used really?)

See the last block for what I meant with my original comment.

So, I searched for _attr_cache in the quirks repo and except for one usage in LocalDataCluster, all quirks I looked at had proper attribute definitions when they accessed _attr_cache.get(attr_id) directly.

I'm only aware of the Aqara P1 motion sensor quirk relying on the behavior of _update_attribute being called with an attribute ID with no corresponding definition here. There may be a few other quirks that do this, but I imagine it's only very few. For that one, it's also not needed to persist the attribute into the DB.

Right now, it should also be saved to the DB with the manufacturer code of the attribute (which is correct): zigpy/zcl/init.py#L861-L880
For attributes without definition, only the unmodified value is persisted into the DB. For ones with a definition, the modified value is persisted (depends on value in AttributeReportedEvent). This behavior is a bit weird..
_update_attribute will also (correctly) call _attr_cache.set_legacy_value with the modified value.

What I meant with my original comment was that ZHA sensors cannot access values only stored in the legacy cache at the moment. E.g. zha/application/platforms/sensor/init.py#L321 will call Cluster.get: zigpy/zcl/init.py#L1594-L1600, which calls AttributeCache.get_value directly zigpy/zcl/helpers.py#L71-L73, bypassing AttributeCache.get (which would have called AttributeCache.__getitem__). The latter two would have checked the legacy cache. ZHA entities do not, as they generally use cluster.get.

This does not seem to be an issue, since we shouldn't generally access the attribute cache without a definition. But it would be good to know what actually lands in the legacy cache at all now. Anything, except for the P1 sensor?

#1774 should address the .get regression.

Copilot

Pull request overview

This PR enhances the ZCL attribute update mechanism to properly handle manufacturer-specific attributes that share the same attribute ID but have different manufacturer codes. The implementation uses a ContextVar to preserve the manufacturer code during attribute updates through quirk transformations.

Changes:

Introduces _attribute_update_context ContextVar to track the current attribute ID and manufacturer code during update operations
Adds _set_attribute_update_context() context manager to manage the context lifecycle
Modifies _legacy_apply_quirk_attribute_update() to set the manufacturer code context before calling _update_attribute()
Updates _update_attribute() to check the context and use the manufacturer code when looking up attribute definitions
Removes the legacy fallback that moved values from the legacy cache to the typed cache, as values now go directly to the typed cache with the correct manufacturer code
Adds comprehensive test test_quirk_manufacturer_code_context_isolation() to verify correct behavior

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
zigpy/zcl/init.py	Implements manufacturer code context tracking via ContextVar and updates attribute update logic to use it
tests/test_zcl.py	Adds test coverage for manufacturer code context isolation in attribute updates

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-12T17:41:21Z

+
        try:
-            attr_def = self.find_attribute(attrid)
+            if ctx is not None and ctx[0] == attrid:


The comparison ctx[0] == attrid will fail when attrid is a ZCLAttributeDef object because ctx[0] is an int. This means the manufacturer code context will not be applied when update_attribute is called with a ZCLAttributeDef (which is a valid and commonly used pattern in the codebase, see test_appdb.py lines 865, 872, 1329, etc.). The attribute ID should be extracted from attrid before comparison. Consider adding a helper to extract the ID from any of the three valid types (int, t.uint16_t, ZCLAttributeDef) or use isinstance checks.

It'll never fail because everywhere where _set_attribute_update_context is called, update_attribute is called with the attribute ID (int). It's not optimal though, but we have the same "issue" for _suppress_attribute_update_event.

Copilot · 2026-02-12T17:41:22Z

        suppressed = (self.cluster_id, attrid) in _suppressed_attribute_updates.get()



The suppression check creates a tuple with attrid which can be an int, t.uint16_t, or ZCLAttributeDef. However, the suppression set (populated by _suppress_attribute_update_event on line 476) contains tuples with integer attribute IDs. When attrid is a ZCLAttributeDef, the tuple will be (cluster_id, ZCLAttributeDef) which won't match (cluster_id, int) in the set, causing the suppression mechanism to fail. Extract the integer ID from attrid before creating the tuple.

Suggested change

suppressed = (self.cluster_id, attrid) in _suppressed_attribute_updates.get()

if isinstance(attrid, foundation.ZCLAttributeDef):

attr_id_for_suppression = attrid.id

else:

attr_id_for_suppression = int(attrid)

suppressed = (

self.cluster_id,

attr_id_for_suppression,

) in _suppressed_attribute_updates.get()

Ha, this is what I mentioned in the comment above. Everywhere _suppress_attribute_update_event is used, we're calling update_attribute with the attribute ID (int), so it's not really an issue.

Copilot · 2026-02-12T17:41:22Z

+                attr_def = self.find_attribute(attrid)
        except KeyError:
            if value is not None:
                self._attr_cache.set_legacy_value(attrid, value)


In the KeyError exception path, set_legacy_value is called with attrid which can be a ZCLAttributeDef. The method signature expects an int. While this may work in practice (since dicts accept any hashable key), it violates the type contract and could lead to inconsistent cache behavior. Extract the integer ID from attrid before calling this method. Note that this path can be reached when a ZCLAttributeDef is passed but its name is not found in attributes_by_name.

This isn't a new issue with this PR. It's still something we should address, but it can only happen if a quirk calls update_attribute with a ZCLAttributeDef from another cluster. Otherwise, find_attribute will always return the correct definition and we'll never hit this. IMO, it's not urgent to address.

* Make `update_attribute` manufacturer code aware * Add test for manufacturer code `update_attribute` handling * Remove legacy cache fallback in `_legacy_apply_quirk_attribute_update` * Adjust comments * Use `mock_attribute_report` in test * Mark attribute as unsupported initially

TheJulianJES added 3 commits February 5, 2026 11:04

Make update_attribute manufacturer code aware

b7db14c

Add test for manufacturer code update_attribute handling

93b11d3

Remove legacy cache fallback in _legacy_apply_quirk_attribute_update

b27bc21

TheJulianJES added 2 commits February 5, 2026 14:37

Adjust comments

4f5c72c

Use mock_attribute_report in test

e0bf8c6

puddly reviewed Feb 9, 2026

View reviewed changes

Comment thread tests/test_zcl.py

Mark attribute as unsupported initially

eedddbf

TheJulianJES commented Feb 12, 2026

View reviewed changes

TheJulianJES marked this pull request as ready for review February 12, 2026 17:34

Copilot AI review requested due to automatic review settings February 12, 2026 17:34

Copilot started reviewing on behalf of TheJulianJES February 12, 2026 17:34

View session

This was referenced Feb 12, 2026

Remove Frient EMI Norwegian HAN divisor workaround zigpy/zha-device-handlers#4750

Draft

Fix Frient EMI LED divisor and multiplier zigpy/zha-device-handlers#4485

Closed

Copilot AI reviewed Feb 12, 2026

View reviewed changes

TheJulianJES marked this pull request as draft February 12, 2026 17:42

TheJulianJES marked this pull request as ready for review February 12, 2026 17:47

puddly mentioned this pull request Feb 12, 2026

Allow accessing legacy cache attributes via cluster.get #1774

Closed

puddly approved these changes Feb 12, 2026

View reviewed changes

TheJulianJES mentioned this pull request Feb 12, 2026

Fix Aqara FP1E using incorrect manufacturer code zigpy/zha-device-handlers#4749

Merged

3 of 4 tasks

puddly merged commit 70d15a5 into zigpy:dev Feb 12, 2026
18 checks passed

TheJulianJES mentioned this pull request Feb 13, 2026

Fix manufacturer attribute report updating standard ZCL cache #1775

Merged

zigpy-review-bot mentioned this pull request Oct 2, 2026

Scope quirk attribute update state to the cluster instance #1896

Open

		suppressed = (self.cluster_id, attrid) in _suppressed_attribute_updates.get()

Conversation

TheJulianJES commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change

Additional information

Uh oh!

codecov Bot commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

TheJulianJES Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

TheJulianJES Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheJulianJES Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

puddly Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

TheJulianJES Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

puddly Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

TheJulianJES Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

TheJulianJES Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

TheJulianJES Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TheJulianJES commented Feb 5, 2026 •

edited

Loading

codecov Bot commented Feb 5, 2026 •

edited

Loading

TheJulianJES Feb 12, 2026 •

edited

Loading

TheJulianJES Feb 12, 2026 •

edited

Loading