Stop LocalDataCluster reads from re-applying quirk value conversions - #5287
Open
rasmusbe wants to merge 1 commit into
Open
Stop LocalDataCluster reads from re-applying quirk value conversions#5287rasmusbe wants to merge 1 commit into
rasmusbe wants to merge 1 commit into
Conversation
`LocalDataCluster.read_attributes_raw` serves values straight from the
attribute cache, and zigpy feeds every successful read result back through
`_update_attribute`. On a cluster that converts values there, that converts an
already converted value a second time and stores the result, so every read
corrupts the value further.
`LocalIlluminanceMeasurementCluster` applies `10000 * log10(v) + 1`, so a
reading of 100 lx walks towards the fixed point of that function on each read:
100 -> 20001 -> 43011 -> 46336 -> 46660
46660 decodes to roughly 46 300 lx. Observed on an Aqara Curtain Driver E1
after four reads of `measured_value` in the Manage Zigbee device UI, but this
affects every quirk combining `LocalDataCluster` with a converting
`_update_attribute`.
The cache is the source of truth for a local cluster, so an echo of a value it
just returned carries no new information. Record the attribute ids handed back
by `read_attributes_raw` and drop the matching `_update_attribute` call. Only
the id is compared, not the value: a subclass overriding `_update_attribute`
sits ahead of `LocalDataCluster` in the MRO and has already transformed the
value by the time it gets there.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #5287 +/- ##
=======================================
Coverage 92.60% 92.60%
=======================================
Files 424 424
Lines 14667 14676 +9
=======================================
+ Hits 13582 13591 +9
Misses 1085 1085 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
LocalDataCluster.read_attributes_rawserves values straight from the attributecache, and zigpy feeds every successful read result back through
_update_attribute(via_legacy_apply_quirk_attribute_updateinCluster.read_attributes). On a cluster that converts values in_update_attribute, that converts an already converted value a second time andstores the result, so every read corrupts the value further.
LocalIlluminanceMeasurementClusterapplies10000 * log10(v) + 1, so a readingof 100 lx walks towards the fixed point of that function on each read:
20001 is the correct stored value for 100 lx. 46660 decodes to roughly
46 300 lx — direct sunlight. I observed this on an Aqara Curtain Driver E1 after
four reads of
measured_valuein the "Manage Zigbee device" UI, but nothingabout it is specific to that device: it affects every quirk pairing
LocalDataClusterwith a converting_update_attribute, which includes allXiaomi quirks using
LocalIlluminanceMeasurementCluster.The cache is the source of truth for a local cluster, so an echo of a value it
just returned carries no new information. This records the attribute ids handed
back by
read_attributes_rawand drops the matching_update_attributecall.Only the attribute id is compared, not the value. A subclass overriding
_update_attributesits ahead ofLocalDataClusterin the MRO and has alreadytransformed the value by the time it gets there — comparing values works for
LocalIlluminanceMeasurementClusterbut not forIlluminanceMeasurementClusterP1, which overrides_update_attributeitself.Additional information
Reproduction: on any device using
LocalIlluminanceMeasurementCluster, readmeasured_valuerepeatedly on the local cluster and watch the value climb.Two regression tests, both verified to fail without the change:
test_local_data_cluster_read_does_not_reconvertcovers the pattern generallywith a synthetic converting local cluster
test_xiaomi_local_illuminance_survives_repeated_readscovers the realLocalIlluminanceMeasurementClusterviaLumiMotionAC02Side effects worth flagging for review: reads of
_CONSTANT_ATTRIBUTESand_DEFAULT_VALUESno longer populate the attribute cache as a side effect of theecho. The read itself still returns those values, and
get()still falls back to_DEFAULT_VALUES, so I could not find behaviour that depends on the oldcaching — but it is the one observable difference beyond the fix itself.
There is a narrow window between
read_attributes_rawand the echo in which agenuine update to the same attribute id would be dropped.
read_attributes_rawon a
LocalDataClusterperforms no I/O, and updates to a local cluster comefrom quirk code running synchronously inside a report handler, so I do not
believe this is reachable in practice.
No separate issue was filed; this PR is the report.
Device diagnostics
Not applicable — this is a fix in
LocalDataCluster, not a new or changed quirk.The behaviour is covered by the two tests above rather than by diagnostics data.
Checklist
pre-commitchecks pass / the code has been formatted using Black