Migration resource monitoring#71738
Draft
tersal wants to merge 19 commits intoproject-chip:masterfrom
Draft
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request migrates the ResourceMonitoring cluster from SafeAttributePersistenceProvider to AttributePersistenceProvider. It introduces a migration path during cluster startup via CodegenResourceMonitoringCluster and updates ResourceMonitoringCluster to use the new persistence API for the LastChangedTime attribute. Feedback was provided to use the AttributePersistence helper class in the UpdateLastChangedTime method for better consistency and safer handling of nullable storage.
Comment on lines
+227
to
+232
| NumericAttributeTraits<uint32_t>::StorageType storageValue; | ||
| DataModel::NullableToStorage(mLastChangedTime, storageValue); | ||
|
|
||
| ReturnValueOnFailure(mContext->attributeStorage.WriteValue( | ||
| ConcreteAttributePath(mPath.mEndpointId, mPath.mClusterId, kAttributeId), | ||
| { reinterpret_cast<const uint8_t *>(&storageValue), sizeof(storageValue) }), Status::Failure); |
Contributor
There was a problem hiding this comment.
Instead of manually converting the nullable value to its storage representation and writing it via WriteValue, it is more idiomatic and consistent with the rest of the cluster implementation (e.g., WriteImpl and LoadPersistentAttributes) to use the AttributePersistence helper class. This encapsulates the logic for handling native-endian storage and nullable types safely.
AttributePersistence attrPersistence{ mContext->attributeStorage };
ReturnValueOnFailure(attrPersistence.StoreNativeEndianValue(
ConcreteAttributePath(mPath.mEndpointId, mPath.mClusterId, kAttributeId), mLastChangedTime),
Status::Failure);References
- Do not persist Nullable objects by writing their raw sizeof bytes, as this is fragile and depends on compiler-specific memory layout. Instead, use a portable method, such as explicitly handling null/non-null cases or using a stable encoding format (e.g., NullableToStorage).
|
PR #71738: Size comparison from ca60dea to cff95de Full report (28 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, efr32, esp32, nrfconnect, nxp, qpg, realtek, stm32, telink)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #71738 +/- ##
=======================================
Coverage 54.52% 54.53%
=======================================
Files 1588 1590 +2
Lines 108571 108582 +11
Branches 13365 13362 -3
=======================================
+ Hits 59203 59217 +14
+ Misses 49368 49365 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR #71738: Size comparison from ca60dea to d45d17a Full report (32 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #71738: Size comparison from ca60dea to f08825f Full report (4 builds for nrfconnect, realtek, stm32)
|
|
PR #71738: Size comparison from ca60dea to 58d161e Full report (22 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, qpg, realtek, stm32, telink)
|
|
PR #71738: Size comparison from 337f8f5 to 1f4a438 Full report (23 builds for cc32xx, efr32, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #71738: Size comparison from adb9376 to 4f3bfc2 Full report (32 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #71738: Size comparison from adb9376 to 81d6dce Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #71738: Size comparison from d94b291 to a07e0ac Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #71738: Size comparison from 0b4c3a7 to 40766b1 Full report (16 builds for cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32)
|
|
PR #71738: Size comparison from 0b4c3a7 to b46919c Full report (5 builds for cc32xx, realtek, stm32)
|
|
PR #71738: Size comparison from 0b4c3a7 to cf14c05 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
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
2 participants
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.
Summary
This PR add a migration layer to the ResourceMonitoring base cluster, this optional layer is only used when migration from SafeAttributePersistence is expected.
Otherwise, the "non-migration" cluster should be used.
Related issues
Fixes #71737
Testing
Added specific unit tests for this migration and also tested it manually with Matter REPL between versions.