Fix StatusCode Handling for command data with only path in invoke request[TE3]

[yunhanw-google]

Problem

When receiving empty data element in invoke request(it has path, no data inside), Status Code needs to be handled by
SDK Consumer, instead of setting success status code.

Change overview

-- If this is empty command data element in invoke request, skip the CHIP_END_OF_TLV error, the
sdk consumer would handle this status code further.
-- Add new unit test

Testing

-- Add new unit test to check if DispatchSingleClusterCommand would be trigged when empty command data in invoke request is processed in handler, without this change, this new test will fail.

[bzbarsky-apple]

I tried applying just the test change, not the CommandHandler.cpp change, and the test still seems to pass (or at least ./gn_build.sh does not fail). That seems a little odd.

Also, I tried testing a command with no data (the toggle command for the on/off cluster) and it seems to work without this change. Why is that? Based on the code I would have expected it to not work due to not calling DispatchSingleClusterCommand.

[bzbarsky-apple]

Summarizing my question from Slack:

1. If I apply just the test change, not the code change, I would expect tests to fail, but they pass. This suggests the test is not usefully testing the code change.
2. What situations result in CHIP_END_OF_TLV on the code side? I tried the toggle command (On/Off cluster) and it's taking the CHIP_NO_ERROR codepath, reaching DispatchSingleClusterCommand, and toggling the light.
3. Should toggle end up with CHIP_END_OF_TLV?

[yunhanw-google]

I tried applying just the test change, not the CommandHandler.cpp change, and the test still seems to pass (or at least ./gn_build.sh does not fail). That seems a little odd.

Also, I tried testing a command with no data (the toggle command for the on/off cluster) and it seems to work without this change. Why is that? Based on the code I would have expected it to not work due to not calling DispatchSingleClusterCommand.

thanks for checking, I just add the additional check in test to check if DispatchSingleClusterCommand would be called when the command data has only path in invoke request. Without the CommandHandler's change, the new test would fail, with the commandHandler's change, the new test would pass.

Per chat with @erjiaqing, there is bug in OnOffCluster, where it is having CommandData container inside,
-- WITH previous code, it would go to DispatchSingleClusterCommand when the GetData(&commandDataReader) return CHIP_NO_ERROR because of the existing CommandData empty container, so you light can be on/off, so test result is right, after song fix #7022, the test result would not be right.
-- After applying the fix in current PR, after song fix #7022, the test result would become right, which means when GetData return END_OF_TLV, it would reset the error to CHIP_NO_ERROR, and call DispatchSingleClusterCommand. Thanks

The below is current problematic InvokeCommandRequest for InvokeCommand for on/off cluster, we should not keep CommandData.
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: CommandData =
CHIP:DMG: {
CHIP:DMG: },
CHIP:DMG: },
CHIP:DMG:
CHIP:DMG: ],
CHIP:DMG:
CHIP:DMG: }

[yunhanw-google]

Summarizing my question from Slack:

1. If I apply just the test change, not the code change, I would expect tests to fail, but they pass. This suggests the test is not usefully testing the code change.
2. What situations result in CHIP_END_OF_TLV on the code side? I tried the toggle command (On/Off cluster) and it's taking the CHIP_NO_ERROR codepath, reaching DispatchSingleClusterCommand, and toggling the light.
3. Should toggle end up with CHIP_END_OF_TLV?

Yep, just reply the question above.

1. fix the test
2. OnOff cluster should not have CommandData inside, but currently it has one CommandData(Empty here), which means GetData return CHIP_NO_ERROR, after song fix Remove CommandData for OnOffCluster's invoke command request #7022, it would return END_OF_TLV, finally go to DispatchSingleClusterCommand.

[github-actions]

Size increase report for "gn_qpg6100-example-build" from 82a8544

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

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
[Unmapped],0,16
.debug_str,0,1
.text,-16,-16
.debug_loc,0,-27
.debug_line,0,-34
.debug_info,0,-36

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]

Size increase report for "nrfconnect-example-build" from 82a8544

File	Section	File	VM
chip-lock.elf	device_handles	4	4
chip-lock.elf	rodata	-8	-8
chip-lock.elf	text	-20	-20

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize

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

sections,vmsize,filesize
.debug_loc,0,4
device_handles,4,4
.debug_frame,0,-4
rodata,-8,-8
text,-20,-20
.debug_line,0,-23
.debug_abbrev,0,-29
.debug_info,0,-36

[github-actions]

Size increase report for "esp32-example-build" from 82a8544

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

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
.xt.lit._ZN12QRCodeScreenD5Ev,0,88
.xt.prop._ZN12QRCodeScreenD5Ev,0,52
.xt.prop._ZN4chip9Transport16AdminPairingInfo5ResetEv,0,40
.debug_info,0,18
.flash.rodata,4,4
.debug_loc,0,3
.debug_abbrev,0,-1
.flash.text,-4,-4
[Unmapped],0,-4
.xt.prop._ZN4chip8Platform3NewINS_9Transport16AdminPairingInfo24StorableAdminPairingInfoEJEEEPT_DpOT0_,0,-12
.xt.prop._ZNSt6vectorIhSaIhEE17_M_default_appendEj,0,-40
.debug_line,0,-48
.xt.lit._ZN4chip8Platform3NewINS_9Transport16AdminPairingInfo24StorableAdminPairingInfoEJEEEPT_DpOT0_,0,-48
.xt.lit._ZNSt6vectorIhSaIhEE17_M_default_appendEj,0,-80

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

sections,vmsize,filesize

[bzbarsky-apple]

In the commit message, "reques(it" should be "request (it" (i.e. add missing 't' and space).