Make global attributes be part of IM instead of part of DataModel::Provider #37345
+329
−330
Conversation
This file contains 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
|
Review changes with |
andy31415
commented
Jan 31, 2025
|
|
||
| for (auto entry : buffer) | ||
| { | ||
| if (!addedExtraGlobals && entry.attributeId > lastGlobalId) |
There was a problem hiding this comment.
I am somewhat unclear why we sort these...this was taken from the existing implementation, but it does seem to make the code more complex. Is the spec requiring sorting?
There was a problem hiding this comment.
Spec does not require sorting. ember attribute metadata is sorted.
When implementing attributes not in metadata we kept the "act as if they are still in metadata" behavior. But yes, we can probably nix this part and just put these things at the end, always.
|
PR #37345: Size comparison from 435583e to 94b4cf5 Full report (3 builds for cc32xx, stm32)
|
|
PR #37345: Size comparison from 435583e to 1fe7070 Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #37345: Size comparison from 26341b4 to 058fcf2 Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| // if we get here, the path was NOT valid | ||
| if (err == CHIP_ERROR_NOT_FOUND) | ||
| { | ||
| return DataModel::ValidateClusterPath(provider, path, Status::Failure); |
There was a problem hiding this comment.
Worth commenting that the Status::Failure is arbitrary, because in practice we clearly failed on the endpoint-or-cluster level above?
| @@ -80,6 +80,13 @@ class EndpointFinder | |||
| ReadOnlyBuffer<EndpointEntry> mEndpoints; | |||
| }; | |||
|
|
|||
| /// Validates that the cluster identified by `path` exists within the given provider. | |||
| /// If the endpoint exists but does not have the cluster identified by the path, will return Status::UnsupportedCluster. | |||
There was a problem hiding this comment.
Suggested change
| /// If the endpoint exists but does not have the cluster identified by the path, will return Status::UnsupportedCluster. | |
| /// If the endpoint does not exist, will return Status::UnsupportedEndpoint. | |
| /// If the endpoint exists but does not have the cluster identified by the path, will return Status::UnsupportedCluster. |
| /// Validates that the cluster identified by `path` exists within the given provider. | ||
| /// If the endpoint exists but does not have the cluster identified by the path, will return Status::UnsupportedCluster. | ||
| /// | ||
| /// otherwise, it will return successStatus. |
There was a problem hiding this comment.
Suggested change
| /// otherwise, it will return successStatus. | |
| /// Otherwise, will return successStatus. |
| @@ -37,6 +37,7 @@ | |||
| #include <lib/support/CodeUtils.h> | |||
| #include <optional> | |||
| #include <protocols/interaction_model/StatusCode.h> | |||
| #include <regex.h> | |||
There was a problem hiding this comment.
Why is regex.h needed? That seems very odd.
There was a problem hiding this comment.
auto-include (probably by a typo) ... will fix
| else | ||
| { | ||
|
|
| @@ -177,8 +178,15 @@ DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataMode | |||
| { | |||
| status = *access_status; | |||
| } | |||
| else if (IsSupportedGlobalAttributeNotInMetadata(readRequest.path.mAttributeId)) | |||
| { | |||
| // Global attributes are NOT directly handled by datamodel providers, instead | |||
There was a problem hiding this comment.
Suggested change
| // Global attributes are NOT directly handled by datamodel providers, instead | |
| // Global attributes are NOT directly handled by data model providers, instead |
| @@ -116,27 +114,13 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::ReadAttribute(const Data | |||
| } | |||
|
|
|||
| // Read via AAI | |||
| std::optional<CHIP_ERROR> aai_result; | |||
| if (const EmberAfCluster ** cluster = std::get_if<const EmberAfCluster *>(&metadata)) | |||
There was a problem hiding this comment.
Is the "what is the right error if we have no metadata" thing FindAttributeMetadata does still relevant here? Or is that all handled by callers now?
Also, the comments above this method no longer match the actual method behavior. Those should have been adjusted in previous PRs, but we should at least do it now.
| const EmberAfAttributeMetadata * attributeMetadata = std::get<const EmberAfAttributeMetadata *>(metadata); | ||
| // We can only get a status or metadata. |
There was a problem hiding this comment.
What guarantees that? What if someone in fact does a read for a global attribute on the provider?
| @@ -102,6 +103,10 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat | |||
| // and tests and implementation | |||
| // | |||
| // Open issue that needs fixing: https://github.com/project-chip/connectedhomeip/issues/33735 | |||
|
|
|||
| // First check things that are not even in metadata. These are read.only. | |||
| VerifyOrReturnValue(!IsSupportedGlobalAttributeNotInMetadata(request.path.mAttributeId), Status::UnsupportedWrite); | |||
There was a problem hiding this comment.
This does not match spec, if it's actually the status that will get returned, for cases when the cluster does not exist.
So either the cluster existence check is done before we ever get here (?) or we have no test coverage for this case.
There was a problem hiding this comment.
Oh, one last important thing: Where are the changes to the Provider.h documentation that explain that providers do not handle global attributes not in metadata and callers have to do that themselves?
| @@ -82,6 +82,27 @@ std::optional<EndpointEntry> EndpointFinder::Find(EndpointId endpointId) | |||
| return std::nullopt; | |||
| } | |||
|
|
|||
| Protocols::InteractionModel::Status ValidateClusterPath(ProviderMetadataTree * provider, const ConcreteClusterPath & path, | |||
There was a problem hiding this comment.
This seems to duplicate ValidateClusterPath in EmberMetadata.... Is it worth commoning those up somehow?
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.
Since this part is already in metadata, this makes code cleaner and easier to maintain (and easier to write providers).
Fixes #36486
Testing
Unit tests were updated.
Since PR #37338 merged, the changes in TestReadInteraction should be reasonable readable in what got added (generally the global list attributes).