[im] Fix error code cases in IM[TE3] #7002

erjiaqing · 2021-05-20T16:20:27Z

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
updated by future im command code (see comment in code).

Problem

We don't check if one cluster is enabled on one endpoint, this causes some issues.
The error code endpoint should be the endpoint on server.

Changes

Return error when some cluster is not enabled on some endpoint
Update status code endpoint id and general error code.

Tests

UT included (since "Add UT" commit)
Also Tested with chip-device-ctrl

OnOff cluster is not enabled on endpoint 0

chip-device-ctrl > zcl OnOff On 1 0 0
CHIP:DMG: ICR moving to [Initialize]
CHIP:DMG: ICR moving to [AddCommand]
CHIP:IN: Secure message was encrypted: Msg ID 3
CHIP:IN: Sending msg from 0x000000000001b669 to 0x0000000000000001 at utc time: 354422368 msec
CHIP:IN: Sending secure msg on generic transport
CHIP:IN: Secure msg send status No Error
CHIP:DMG: ICR moving to [   Sending]
CHIP:IN: Secure transport received message destined to node ID (112233)
CHIP:EM: Received message of type 9 and protocolId 327680
CHIP:DMG: InvokeCommand =
CHIP:DMG: {
CHIP:DMG:       CommandList =
CHIP:DMG:       [
CHIP:DMG:               CommandDataElement =
CHIP:DMG:               {
CHIP:DMG:                       CommandPath =
CHIP:DMG:                       {
CHIP:DMG:                               EndpointId = 0x0,
CHIP:DMG:                               ClusterId = 0x6,
CHIP:DMG:                               CommandId = 0x1,
CHIP:DMG:                       },
CHIP:DMG: 
CHIP:DMG:                       StatusElement =
CHIP:DMG:                       {
CHIP:DMG:                               GeneralCode = 0xd,
CHIP:DMG:                               ProtocolId = 0x0,
CHIP:DMG:                               protocolCode = 0xffff,
CHIP:DMG:                       },
CHIP:DMG: 
CHIP:DMG:               },
CHIP:DMG: 
CHIP:DMG:       ],
CHIP:DMG: 
CHIP:DMG: }
SetCommandIndexStatus commandHandle=140534550478280 commandIndex=1
CHIP:ZCL: DefaultResponse:
CHIP:ZCL:   Transaction: 0x7fd0bff5bdc8
CHIP:ZCL:   status: EMBER_ZCL_STATUS_FAILURE (0x01)
CHIP:ZCL: emberAfDefaultResponseCallback: Missing success callback
CHIP:ZCL: emberAfDefaultResponseCallback: Missing failure callback
CHIP:EM: Sending Standalone Ack for MsgId:00000005
CHIP:IN: Secure message was encrypted: Msg ID 4
CHIP:IN: Sending msg from 0x000000000001b669 to 0x0000000000000001 at utc time: 354422371 msec
CHIP:IN: Sending secure msg on generic transport
CHIP:IN: Secure msg send status No Error
CHIP:EM: Flushed pending ack for MsgId:00000005
CHIP:ZCL: DefaultResponse:
CHIP:ZCL:   Transaction: 0x7fd0bff5bdc8
CHIP:ZCL:   status: EMBER_ZCL_STATUS_SUCCESS (0x00)
CHIP:ZCL: emberAfDefaultResponseCallback: Missing success callback
CHIP:ZCL: emberAfDefaultResponseCallback: Missing failure callback
CHIP:DMG: ICR moving to [Uninitiali]
Received command status response:
Container: 
    ProtocolId = 0
    ProtocolCode = 65535
    EndpointId = 0
    ClusterId = 6
    CommandId = 1
    CommandIndex = 1

It will report error when cluster is not found on device.

Tests: Cirque test and local device controller run against all cluster app in two raspberry pi

todo · 2021-05-20T16:20:29Z

Currently, we are using cluster catalog from the ember library, this should be modified or replaced after several

connectedhomeip/src/app/util/ember-compatibility-functions.cpp

Lines 92 to 98 in 3d42285

    
               // TODO: Currently, we are using cluster catalog from the ember library, this should be modified or replaced after several 
        
               // updates to Commands. 
        
               return emberAfContainsServer(aEndPointId, aClusterId) ? CHIP_NO_ERROR : CHIP_ERROR_INVALID_PROFILE_ID; 
        
           } 
        
           } // namespace app 
        
           } // namespace chip

This comment was generated by todo based on a `TODO` comment in `3d42285` in #7002. cc @erjiaqing.

andy31415 · 2021-05-20T17:44:45Z

Could we have unit tests for some of these things?

Cirque is end-to-end and debugging failures at that level is hard.

bzbarsky-apple

Are there tests that run in CI that exercise this?

src/app/util/ember-compatibility-functions.cpp

woody-apple

Thanks for the change! We have updated our PR template, requesting changes to conform to the new PR template:

#### Problem
What is being fixed?  Examples:
* Fix crash on startup
* Fixes #12345 Frobnozzle is leaky (exactly like that, so GitHub will auto-close the issue).

#### Change overview
What's in this PR

#### Testing
How was this tested? (at least one bullet point required)
* If unit tests were added, how do they cover this issue?
* If unit tests existed, how were they fixed/modified to prevent this in future?
* If new unit tests are not added, why not?
* If integration tests were added, how do they verify this change?
* If new integration tests are not added, why not?
* If manually tested, what platforms controller and device platforms were manually tested, and how?
* If no testing is required, why not?

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.

erjiaqing · 2021-05-21T16:48:11Z

@andy31415 @bzbarsky-apple Test added, PTAL
Still using the IM test in cirque, due to we need to verify the content of response, and none of current unit tests implemnted this.
Per design, receiving a invalid endpoint/cluster/command will not result in error of command handler since it is still a valid message, this is a app level error and instead of encoding error.
Added a simple UT to test this logic by checking core call path in this function.

todo · 2021-05-21T17:15:21Z

Need to find a way to get the response instead of only check if a function on key path is called.

connectedhomeip/src/app/tests/TestCommandInteraction.cpp

Lines 328 to 338 in b980d16

    
               // TODO: Need to find a way to get the response instead of only check if a function on key path is called. 
        
               // We should not reach CommandDispatch if requisted command does not exist. 
        
               commandReachedDispatch = false; 
        
               err                    = commandHandler.ProcessCommandMessage(std::move(commandDatabuf), Command::CommandRoleId::HandlerId); 
        
               NL_TEST_ASSERT(apSuite, !commandReachedDispatch); 
        
               commandHandler.Shutdown(); 
        
           } 
        
           } // namespace app 
        
           } // namespace chip

This comment was generated by todo based on a `TODO` comment in `b980d16` in #7002. cc @erjiaqing.

todo · 2021-05-24T03:42:40Z

(Spec#3258): The endpoint id in response command is not clear, so we cannot do "ClientClusterCommandExists" check.

connectedhomeip/src/app/CommandSender.cpp

Lines 164 to 167 in 685f6e7

    
           // TODO(Spec#3258): The endpoint id in response command is not clear, so we cannot do "ClientClusterCommandExists" check. 
        
           err = aCommandElement.GetData(&commandDataReader); 
        
           SuccessOrExit(err); 
        
           // TODO(#4503): Should call callbacks of cluster that sends the command.

This comment was generated by todo based on a `TODO` comment in `685f6e7` in #7002. cc @erjiaqing.

todo · 2021-05-24T03:42:43Z

The endpoint id on response command (client side command) is unclear, so we don't have a ClientClusterCommandExists

connectedhomeip/src/app/InteractionModelEngine.h

Lines 172 to 180 in 685f6e7

    
            *  TODO: The endpoint id on response command (client side command) is unclear, so we don't have a ClientClusterCommandExists 
        
            * function. (Spec#3258) 
        
            * 
        
            *  @retval  If the endpoint contains the cluster and command. 
        
            */ 
        
           bool ServerClusterCommandExists(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId); 
        
           CHIP_ERROR ReadSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVWriter & aWriter); 
        
           CHIP_ERROR WriteSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader); 
        
           } // namespace app

This comment was generated by todo based on a `TODO` comment in `685f6e7` in #7002. cc @erjiaqing.

src/app/CommandHandler.cpp

src/app/InteractionModelEngine.h

todo · 2021-05-25T01:01:31Z

The error code should be updated after #7072 added error codes required by IM.

connectedhomeip/src/app/CommandHandler.cpp

Lines 118 to 128 in 7e4bc2c

    
                   // TODO: The error code should be updated after #7072 added error codes required by IM. 
        
                   AddStatusCode(&returnStatusParam, 
        
                                 err == CHIP_ERROR_INVALID_PROFILE_ID ? GeneralStatusCode::kNotFound : GeneralStatusCode::kInvalidArgument, 
        
                                 Protocols::SecureChannel::Id, Protocols::SecureChannel::kProtocolCodeGeneralFailure); 
        
               } 
        
               // We have handled the error status above and put the error status in response, now return success status so we can process 
        
               // other commands in the invoke request. 
        
               return CHIP_NO_ERROR; 
        
           } 
        
           CHIP_ERROR CommandHandler::AddStatusCode(const CommandPathParams * apCommandPathParams,

This comment was generated by todo based on a `TODO` comment in `7e4bc2c` in #7002. cc @erjiaqing.

todo · 2021-05-25T01:01:34Z

(Spec#3258): The endpoint id in response command is not clear, so we cannot do "ClientClusterCommandExists" check.

connectedhomeip/src/app/CommandSender.cpp

Lines 166 to 169 in 7e4bc2c

    
           // TODO(Spec#3258): The endpoint id in response command is not clear, so we cannot do "ClientClusterCommandExists" check. 
        
           err = aCommandElement.GetData(&commandDataReader); 
        
           SuccessOrExit(err); 
        
           // TODO(#4503): Should call callbacks of cluster that sends the command.

This comment was generated by todo based on a `TODO` comment in `7e4bc2c` in #7002. cc @erjiaqing.

todo · 2021-05-25T01:01:37Z

The endpoint id on response command (client side command) is unclear, so we don't have a ClientClusterCommandExists

connectedhomeip/src/app/InteractionModelEngine.h

Lines 172 to 181 in 7e4bc2c

    
            *  TODO: The endpoint id on response command (client side command) is unclear, so we don't have a ClientClusterCommandExists 
        
            * function. (Spec#3258) 
        
            * 
        
            *  @retval  True if the endpoint contains the server side of the given cluster and that cluster implements the given command, false 
        
            * otherwise. 
        
            */ 
        
           bool ServerClusterCommandExists(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId); 
        
           CHIP_ERROR ReadSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVWriter & aWriter); 
        
           CHIP_ERROR WriteSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader); 
        
           } // namespace app

This comment was generated by todo based on a `TODO` comment in `7e4bc2c` in #7002. cc @erjiaqing.

erjiaqing · 2021-05-25T01:07:32Z

@woody-apple The PR description is updated.
@andy31415 The UT is added, PTAL, thanks.

github-actions · 2021-05-25T01:27:40Z

Size increase report for "esp32-example-build" from 21354c3

File	Section	File	VM
chip-all-clusters-app.elf	.flash.text	100	100

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,919
.debug_line,0,905
.debug_loc,0,195
.flash.text,100,100
.debug_abbrev,0,98
.debug_ranges,0,80
.debug_str,0,72
.strtab,0,45
.debug_frame,0,24
.symtab,0,16
.debug_aranges,0,8
.shstrtab,0,-1
.xt.prop._ZTVN4chip11DeviceLayer37DeviceNetworkProvisioningDelegateImplE,0,-1

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

github-actions · 2021-05-25T01:28:00Z

Size increase report for "gn_qpg6100-example-build" from 21354c3

File	Section	File	VM
chip-qpg6100-lighting-example.out	.text	72	72

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_info,0,899
.debug_line,0,655
.debug_loc,0,243
.debug_abbrev,0,101
.debug_str,0,74
.text,72,72
.debug_ranges,0,48
.strtab,0,45
.symtab,0,32
.debug_frame,0,12
.debug_aranges,0,8
.shstrtab,0,-1
[Unmapped],0,-72

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

github-actions · 2021-05-25T02:50:18Z

Size increase report for "nrfconnect-example-build" from 21354c3

File	Section	File	VM
chip-lock.elf	text	104	104
chip-lock.elf	device_handles	8	8

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,899
.debug_line,0,600
.debug_loc,0,252
text,104,104
.debug_abbrev,0,101
.strtab,0,73
.debug_str,0,72
.symtab,0,64
.debug_ranges,0,48
.debug_frame,0,12
.debug_aranges,0,8
device_handles,8,8
.shstrtab,0,-1

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize

andy31415 · 2021-05-25T14:27:15Z

@woody-apple summary format changed to match template

There are tests in the PR now.

* [im] Return error when some cluster is not enabled on endpoint 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. * Add tests * Add UT * Address comments * Address comments * Address comments * Update comments * Fix test

boring-cyborg bot added the app label May 20, 2021

erjiaqing mentioned this pull request May 20, 2021

[IM] Add empty command Support[TE3] #6989

Closed

yunhanw-google requested review from bzbarsky-apple, andy31415 and yunhanw-google May 20, 2021 16:22

pullapprove bot requested review from chrisdecenzo, jelderton, jmartinez-silabs, LuDuda and saurabhst May 20, 2021 16:24

pullapprove bot added the review - pending label May 20, 2021

yunhanw-google requested a review from msandstedt May 20, 2021 16:25

yunhanw-google changed the title ~~[im] Fix error code cases in IM~~ [im] Fix error code cases in IM[TE3] May 20, 2021

yunhanw-google approved these changes May 20, 2021

View reviewed changes

bzbarsky-apple reviewed May 20, 2021

View reviewed changes

src/app/util/ember-compatibility-functions.cpp Show resolved Hide resolved

jmartinez-silabs approved these changes May 20, 2021

View reviewed changes

woody-apple previously requested changes May 20, 2021

View reviewed changes

erjiaqing force-pushed the im-errorcode-update branch from 9bff0d4 to d9dfb44 Compare May 21, 2021 02:43

erjiaqing force-pushed the im-errorcode-update branch from 0ba84d4 to b57a176 Compare May 21, 2021 16:17

Add tests

60e1022

erjiaqing force-pushed the im-errorcode-update branch from b57a176 to 60e1022 Compare May 21, 2021 16:47

erjiaqing requested review from woody-apple and bzbarsky-apple May 21, 2021 16:48

Add UT

20cf005

erjiaqing force-pushed the im-errorcode-update branch from b980d16 to 20cf005 Compare May 21, 2021 17:18

erjiaqing force-pushed the im-errorcode-update branch from 7506073 to 685f6e7 Compare May 24, 2021 03:42

erjiaqing requested a review from bzbarsky-apple May 24, 2021 03:46

bzbarsky-apple approved these changes May 24, 2021

View reviewed changes

src/app/CommandHandler.cpp Outdated Show resolved Hide resolved

src/app/CommandHandler.cpp Show resolved Hide resolved

src/app/InteractionModelEngine.h Outdated Show resolved Hide resolved

erjiaqing mentioned this pull request May 25, 2021

[IM] Add error code enum for interaction model #7072

Closed

erjiaqing added 3 commits May 25, 2021 08:51

Merge remote-tracking branch 'chip/master' into im-errorcode-update

1ed2e45

Update comments

06e0239

Fix test

7e4bc2c

msandstedt approved these changes May 25, 2021

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels May 25, 2021

bzbarsky-apple merged commit 4389925 into project-chip:master May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[im] Fix error code cases in IM[TE3] #7002

[im] Fix error code cases in IM[TE3] #7002

erjiaqing commented May 20, 2021 •

edited

Loading

todo bot commented May 20, 2021

andy31415 commented May 20, 2021

bzbarsky-apple left a comment

woody-apple left a comment

erjiaqing commented May 21, 2021 •

edited

Loading

todo bot commented May 21, 2021

todo bot commented May 24, 2021

todo bot commented May 24, 2021

todo bot commented May 25, 2021

todo bot commented May 25, 2021

todo bot commented May 25, 2021

erjiaqing commented May 25, 2021

github-actions bot commented May 25, 2021

github-actions bot commented May 25, 2021

github-actions bot commented May 25, 2021

andy31415 commented May 25, 2021

[im] Fix error code cases in IM[TE3] #7002

[im] Fix error code cases in IM[TE3] #7002

Conversation

erjiaqing commented May 20, 2021 • edited Loading

Problem

Changes

Tests

todo bot commented May 20, 2021

Currently, we are using cluster catalog from the ember library, this should be modified or replaced after several

This comment was generated by todo based on a TODO comment in 3d42285 in #7002. cc @erjiaqing.

andy31415 commented May 20, 2021

bzbarsky-apple left a comment

Choose a reason for hiding this comment

woody-apple left a comment

Choose a reason for hiding this comment

erjiaqing commented May 21, 2021 • edited Loading

todo bot commented May 21, 2021

Need to find a way to get the response instead of only check if a function on key path is called.

This comment was generated by todo based on a TODO comment in b980d16 in #7002. cc @erjiaqing.

todo bot commented May 24, 2021

(Spec#3258): The endpoint id in response command is not clear, so we cannot do "ClientClusterCommandExists" check.

This comment was generated by todo based on a TODO comment in 685f6e7 in #7002. cc @erjiaqing.

todo bot commented May 24, 2021

The endpoint id on response command (client side command) is unclear, so we don't have a ClientClusterCommandExists

This comment was generated by todo based on a TODO comment in 685f6e7 in #7002. cc @erjiaqing.

todo bot commented May 25, 2021

The error code should be updated after #7072 added error codes required by IM.

This comment was generated by todo based on a TODO comment in 7e4bc2c in #7002. cc @erjiaqing.

todo bot commented May 25, 2021

(Spec#3258): The endpoint id in response command is not clear, so we cannot do "ClientClusterCommandExists" check.

This comment was generated by todo based on a TODO comment in 7e4bc2c in #7002. cc @erjiaqing.

todo bot commented May 25, 2021

The endpoint id on response command (client side command) is unclear, so we don't have a ClientClusterCommandExists

This comment was generated by todo based on a TODO comment in 7e4bc2c in #7002. cc @erjiaqing.

erjiaqing commented May 25, 2021

github-actions bot commented May 25, 2021

github-actions bot commented May 25, 2021

github-actions bot commented May 25, 2021

andy31415 commented May 25, 2021

erjiaqing commented May 20, 2021 •

edited

Loading

This comment was generated by todo based on a `TODO` comment in `3d42285` in #7002. cc @erjiaqing.

erjiaqing commented May 21, 2021 •

edited

Loading

This comment was generated by todo based on a `TODO` comment in `b980d16` in #7002. cc @erjiaqing.

This comment was generated by todo based on a `TODO` comment in `685f6e7` in #7002. cc @erjiaqing.

This comment was generated by todo based on a `TODO` comment in `685f6e7` in #7002. cc @erjiaqing.

This comment was generated by todo based on a `TODO` comment in `7e4bc2c` in #7002. cc @erjiaqing.

This comment was generated by todo based on a `TODO` comment in `7e4bc2c` in #7002. cc @erjiaqing.

This comment was generated by todo based on a `TODO` comment in `7e4bc2c` in #7002. cc @erjiaqing.