Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.52%. Comparing base (0ceeb47) to head (ce21c59).

@@            Coverage Diff             @@
##              dev    #1774      +/-   ##
==========================================
- Coverage   99.52%   99.52%   -0.01%     
==========================================
  Files          63       63              
  Lines       12709    12705       -4     
==========================================
- Hits        12649    12645       -4     
  Misses         60       60              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I do kind of wonder: What (useful) values actually land in the _legacy_cache at all now and where are they accessed?
Currently, ZHA entities all do not use the legacy cache because of this regression and I don't think I've seen a single issue that's related to this?

So, I wonder if we should just try to keep this..? Because if we re-introduce this, even with TheJulianJES/zigpy@tjj/manufacturer_update_fix...TheJulianJES:zigpy:tjj/manufacturer_update_fix_with_mfr_attr_fix_zcl_cache (which fixes the issue of mf-specific reports with ZCL IDs landing in ZCL cache, instead of only in legacy cache), we'll then have ZHA entities use cluster.get and accessing mf-specific attribute values that overlap with ZCL attribute IDs if there's not an attribute definition for them, even though they (correctly) only landed in the legacy cache.
(Maybe we can also just block the legacy cache entirely for attribute reports and only allow it if quirks do updates?..)

But I guess we can rethink this at a later point. The involuntary beta testing by everyone was nice to see we may not rely on this at all though. I think this should be fine, for now. I'll give it a proper look in a minute.

My original idea for the legacy cache was based on the (as it turns out, mostly false) assumption that quirks frequently used patterns like cluster._update_attribute(0xABCD, "some value") to write data to the database and then relied on it being readable via cluster.get(0xABCD) after restart, with there being no corresponding attribute definition. We could then migrate these to a generic "key/value" table separate from the ZCL attribute cache.

It turns out practically all quirks these days actually register their virtual attributes normally and thus "bypass" the legacy cache. They use the attribute system properly, just not with attributes that really exist.

I guess next release we can try to introduce some better logging for truly "legacy" attribute access patterns and then deprecate it entirely? This would really simplify the attribute cache.

If nothing is really broken without this PR, let's leave things as is then.

Yeah, I think the additional logging for everything that still ends up in the legacy cache now, and an eventual key/value migration sound sensible. But with #1769, I can't think of anything "really used" that would still end up in the legacy cache now.

If nothing is really broken without this PR, let's leave things as is then.

I mean, I'm not 100% sure, but all quirks I can think of do use "fake attribute definitions" then, most on LocalDataClusters.
For the Aqara P1 sensor that's missing one attribute definition, we should eventually add one. But even there, it's only used to forward/update attributes on other clusters, so I guess that's why it was never added in the first place — just forgotten. Like, it's not directly used by any entity and everything with that works properly.

So I guess we can skip this PR for now?