-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[IM] Add empty command Support[TE3] #6989
[IM] Add empty command Support[TE3] #6989
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just run the technical review, it looks good to me, thanks to add this support
cbf47fb
to
5b47041
Compare
@@ -84,5 +86,11 @@ void ResetEmberAfObjects() | |||
} | |||
|
|||
} // namespace Compatibility | |||
|
|||
CHIP_ERROR CheckIfClusterCommandExists(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method doesn't seem to do what it claims to do. Are you going to change it in the future? If so, please add some "TODO" comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a TODO comment.
@mrjerryjohns I remembered that your design for Command dispatch / Encoding covered this part (Endpoint - Cluster - Command catalog)?
1350bee
to
61c3244
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit message for the first commit doesn't make sense. What does "cluster not enabled on cluster" mean?
More generally, I don't understand what this is trying to implement. CommandStatusIB
absolutely contains a path in the spec. Is this trying to implement a StatusResponse response instead of an InvokeResponse? Or something else? Please clearly explain in the commit message what is actually being done here, in terms of the spec.
And this needs tests.
chip::app::CommandPathParams returnStatusParam = { imCompatibilityEmberApsFrame.sourceEndpoint, | ||
chip::app::CommandPathParams returnStatusParam = { imCompatibilityEmberApsFrame.destinationEndpoint, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't look right... Why is it being made?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for this change: the imCompatibilityEmberApsFrame
refers to the ApsFrame struct we built for ember command handlers.
In zigbee, the commands have "Source Endpoint", and in CHIP we no longer have it (see above, the source endpoint is fixed to 1). So the destinationEndpoint
here refers to the endpoint on the server, which matches the spec -- the endpoint where this status comes from.
|
||
CHIP_ERROR CheckIfClusterCommandExists(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId) | ||
{ | ||
return emberAfContainsServerWithMfgCode(aEndPointId, aClusterId, 0x0) ? CHIP_NO_ERROR : CHIP_ERROR_INVALID_PROFILE_ID; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return emberAfContainsServerWithMfgCode(aEndPointId, aClusterId, 0x0) ? CHIP_NO_ERROR : CHIP_ERROR_INVALID_PROFILE_ID; | |
return emberAfContainsServer(aEndPointId, aClusterId) ? CHIP_NO_ERROR : CHIP_ERROR_INVALID_PROFILE_ID; |
One other thing: The encoding spec currently has For InvokeResponse, there can be a status, but that status includes a path in the IM spec (see CommandStatusIB), if we get far enough that we are actually ending up with an InvokeResponse at all. |
61c3244
to
27b8b5c
Compare
The current code may return wrong success or unexpected error code when one cluster is not enabled on endpoint, this commit added a function thay will query cluster from ember cluster catalog, the function will be replaced by cluster catalog.
27b8b5c
to
0a70d55
Compare
considering there is spec conflict between IM data model spec and IM encoding spec, we would not merge this PR, and would file the ticket in spec side |
As the first PR is bug fix only and not related to spec, it is submitted as seperate PR: #7002 |
Size increase report for "nrfconnect-example-build" from dfb2285
Full report output
|
Size increase report for "esp32-example-build" from dfb2285
Full report output
|
Size increase report for "gn_qpg6100-example-build" from dfb2285
Full report output
|
Problem
We don't have support for empty command body / status code only in IM.
Summary of Changes
Tests
Note command specified response command still contains path
Note this command element no longer contains path
Note this command element no longer contains path
Note this command element no longer contains path
Note this command element no longer contains path
Device side:
Note it does not contains command body
Fixes #6988