Descriptor cluser's new attribute : EndpointUniqueId is added as per latest spec 0.7-fall2024-ncr(V1.5) #37990
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
…latest spec 0.7-fall2024-ncr
|
PR #37990: Size comparison from 5adee57 to c9431c6 Increases above 0.2%:
Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #37990: Size comparison from f217707 to 4b48c44 Increases above 0.2%:
Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
|
||
| CHIP_ERROR DescriptorAttrAccess::ReadEndpointUniqueId(EndpointId endpoint, AttributeValueEncoder & aEncoder) | ||
| { | ||
| char endpointUniqueId[32 + 1] = { 0 }; |
There was a problem hiding this comment.
can we get a consexpr for this 32 with a name that reflect what it is and why it is 32
Comment on lines
+1087
to
+1090
| if (endpointIndex == 0xFFFF) | ||
| { | ||
| return CHIP_ERROR_NOT_FOUND; | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (endpointIndex == 0xFFFF) | |
| { | |
| return CHIP_ERROR_NOT_FOUND; | |
| } | |
| VerifyOrReturnError(endpointIndex != chip::kInvalidEndpointId, CHIP_ERROR_NOT_FOUND); |
There was a problem hiding this comment.
No, endpoint indices are not the same as endpoint ids. The documented behavior of emberAfIndexFromEndpoint is:
Will return 0xFFFF if this is not a valid endpoint id or if the endpoint is disabled.
and the right thing to check for is 0xFFFF.
Comment on lines
+1122
to
+1125
| if (endpointIndex == 0xFFFF) | ||
| { | ||
| return CHIP_ERROR_INVALID_ARGUMENT; | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (endpointIndex == 0xFFFF) | |
| { | |
| return CHIP_ERROR_INVALID_ARGUMENT; | |
| } | |
| VerifyOrReturnError(endpointIndex != chip::kInvalidEndpointId, CHIP_ERROR_INVALID_ARGUMENT); |
bzbarsky-apple
requested changes
Mar 14, 2025
| @@ -236,6 +237,8 @@ struct EmberAfDefinedEndpoint | |||
| * Span pointing to a list of tags. Lifetime has to outlive usage, and data is owned by callers. | |||
| */ | |||
| chip::Span<const chip::app::Clusters::Descriptor::Structs::SemanticTagStruct::Type> tagList; | |||
|
|
|||
| std::string endpointUniqueId; | |||
There was a problem hiding this comment.
Not OK to use std::string in general in embedded code. This needs a different representation.
Comment on lines
+1087
to
+1090
| if (endpointIndex == 0xFFFF) | ||
| { | ||
| return CHIP_ERROR_NOT_FOUND; | ||
| } |
There was a problem hiding this comment.
No, endpoint indices are not the same as endpoint ids. The documented behavior of emberAfIndexFromEndpoint is:
Will return 0xFFFF if this is not a valid endpoint id or if the endpoint is disabled.
and the right thing to check for is 0xFFFF.
Comment on lines
+1121
to
+1122
| uint16_t endpointIndex = emberAfIndexFromEndpoint(endpoint); | ||
| if (endpointIndex == 0xFFFF) |
There was a problem hiding this comment.
This looks wrong to me. Shouldn't it be possible to set the unique ID of a disabled endpoint before enabling it?
But separately: are unique IDs allowed to change? If not, shouldn't the unique ID be part of the "add the endpoint" machinery, not a separate setter?
I know you copy/pasted this stuff from the existing SetTagList bits and whatnot, but those look wrong too.
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.
Specification for V1.5 (https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/9171) defines a new attribute EndpointUniqueId in Descriptor cluster. The Changes in the request are related to adding this new attribute.
Testing
Testing was done using the bridge-app and the DUT deive by reading the EndpointUniqueId attribute using the chip-tool.