Convert 'echo' command to a manufacturer specific 'ping' command #4127

vivien-apple · 2020-12-08T17:57:51Z

Problem

The echo command is an hand rolled command. It has been super useful but currently, in its current shape, it is a pain as it make some part of the code harder to write while it will never really be used as it is in an end product.

This PR replace echo by a ping manufacturer specific command. This is nice because if add supports for some part of the code that where not used until then, notably the manufacturer specific side of ZCL clusters, and it removes the need for some special handling support in various places.

The changes in examples/all-clusters-app/all-clusters-common/gen/endpoint_config.h has been done using AppBuilder while the rest of the gen/ changes has been done using ZAP.

Summary of Changes

Add a manufacturer specific command to the all-clusters-app configuration
Update src/app/zap-templates/call-command-handler-src.zapt to support manufacturer specific command
Remove echo command support in examples/common/chip-app-server
Simplify chip-tool by killing the EchoCommand class
Simplify chip-tool by mergin the NetworkCommand and ModelCommand classes together

rwalker-apple · 2020-12-08T20:21:23Z

examples/all-clusters-app/all-clusters-common/all-clusters-app.zap

@@ -2822,4 +2830,4 @@
      "networkId": 1
    }
  ]
-}
+}


nit: can we keep newlines at EOF to eliminate this kind of noise in PRs?

Does restyler-whitespace fix this, so we could just run that on .zap files?

vivien-apple · 2020-12-10T17:02:17Z

Pushed an update to resolve merge conflicts.

github-actions · 2020-12-10T17:30:57Z

Size increase report for "esp32-example-build" from 97a07fb

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

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,2132
.xt.lit._ZNK4chip8OptionalIyE5ValueEv,0,40
.flash.rodata,8,8
.xt.prop._ZTVN4chip11DeviceLayer37DeviceNetworkProvisioningDelegateImplE,0,2
[Unmapped],0,-8
.debug_aranges,0,-16
.symtab,0,-16
.xt.prop._ZNK4chip20RendezvousParameters12IsControllerEv,0,-40
.debug_frame,0,-48
.debug_ranges,0,-72
.strtab,0,-208
.debug_str,0,-254
.debug_abbrev,0,-261
.flash.text,-272,-272
.debug_loc,0,-2055
.debug_line,0,-3140

mspang · 2020-12-10T21:54:53Z

examples/all-clusters-app/all-clusters-common/all-clusters-app.zap

@@ -2822,4 +2830,4 @@
      "networkId": 1
    }
  ]
-}
+}


Does restyler-whitespace fix this, so we could just run that on .zap files?

bzbarsky-apple · 2020-12-11T05:55:55Z

@vivien-apple Please rebase to tip to retrigger CI on top of the pigweed fixes?

bzbarsky-apple

This just sends back a status response, unlike the existing echo command, which responds with the incoming data, right? Why that change?

examples/chip-tool/commands/clusters/ModelCommand.cpp

vivien-apple · 2020-12-11T10:18:51Z

This just sends back a status response, unlike the existing echo command, which responds with the incoming data, right? Why that change?

You are right.
There are 2 reasons why I did that:

The first one is that there is already an echo protocol now. I'm not saying I'm happy with that but since it is there it would not make sense to duplicate it.
The second is because I felt like the goal of echo at the beginning was to make sure the data is encoded/decoded properly. Since ping will be encrypted/decrypted already, and since the command will be parse on both ends, it just seems redundant.

github-actions · 2020-12-11T15:54:52Z

Size increase report for "nrfconnect-example-build" from 27d3ea8

File	Section	File	VM
chip-lighting.elf	shell_root_cmds_sections	4	4
chip-lighting.elf	rodata	-64	-68
chip-lighting.elf	text	-340	-340
chip-lock.elf	shell_root_cmds_sections	4	4
chip-lock.elf	rodata	-64	-68
chip-lock.elf	text	-340	-340

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_loc,0,832
.debug_info,0,172
shell_root_cmds_sections,4,4
.shstrtab,0,2
.debug_aranges,0,-24
.debug_ranges,0,-40
.debug_line,0,-56
rodata,-68,-64
.debug_frame,0,-96
.symtab,0,-176
.strtab,0,-286
text,-340,-340
.debug_str,0,-364
.debug_abbrev,0,-376

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,840
.debug_info,0,172
shell_root_cmds_sections,4,4
.shstrtab,0,-2
.debug_aranges,0,-24
.debug_ranges,0,-40
.debug_line,0,-56
rodata,-68,-64
.debug_frame,0,-96
.symtab,0,-176
.strtab,0,-286
text,-340,-340
.debug_str,0,-364
.debug_abbrev,0,-376

This PR introduces a folder inside `src/app/zap-templates` to host custom ZCL extensions such as the `MfgSpecificPing` command introduced in project-chip#4127 as well as a `Binding Cluster` from project-chip#4156. The `Binding Cluster` exposes 2 methods, `Bind` and `Unbind`. The current spec defines `bindings` has an `RW` attribute but an issue has been opened since it does not look very convenient (see CHIP-Specifications/connectedhomeip-spec#627) The `gen/` folders are also updated to reflect the addition of the `Binding Cluster`. Futhermore, the 2 scripts used to generates the `gen/` folders have also been updated to points to the `src/app/zap-templates/zcl/zcl.json` instead of `third_party/zap/repo/zcl-builtin/silabs/zcl.json` since the former is a superset of the later.

This PR introduces a folder inside `src/app/zap-templates` to host custom ZCL extensions such as the `MfgSpecificPing` command introduced in #4127 as well as a `Binding Cluster` from #4156. The `Binding Cluster` exposes 2 methods, `Bind` and `Unbind`. The current spec defines `bindings` has an `RW` attribute but an issue has been opened since it does not look very convenient (see CHIP-Specifications/connectedhomeip-spec#627) The `gen/` folders are also updated to reflect the addition of the `Binding Cluster`. Futhermore, the 2 scripts used to generates the `gen/` folders have also been updated to points to the `src/app/zap-templates/zcl/zcl.json` instead of `third_party/zap/repo/zcl-builtin/silabs/zcl.json` since the former is a superset of the later.

…4192) This PR introduces a folder inside `src/app/zap-templates` to host custom ZCL extensions such as the `MfgSpecificPing` command introduced in project-chip#4127 as well as a `Binding Cluster` from project-chip#4156. The `Binding Cluster` exposes 2 methods, `Bind` and `Unbind`. The current spec defines `bindings` has an `RW` attribute but an issue has been opened since it does not look very convenient (see CHIP-Specifications/connectedhomeip-spec#627) The `gen/` folders are also updated to reflect the addition of the `Binding Cluster`. Futhermore, the 2 scripts used to generates the `gen/` folders have also been updated to points to the `src/app/zap-templates/zcl/zcl.json` instead of `third_party/zap/repo/zcl-builtin/silabs/zcl.json` since the former is a superset of the later.

vivien-apple self-assigned this Dec 8, 2020

boring-cyborg bot added app examples labels Dec 8, 2020

pullapprove bot requested review from andy31415, BroderickCarlin, bzbarsky-apple, chrisdecenzo, hawk248, jelderton, mspang, rwalker-apple and saurabhst December 8, 2020 17:58

vivien-apple mentioned this pull request Dec 8, 2020

Add C++ wrappers for ZCL Clusters #4126

Merged

vivien-apple force-pushed the Cluster_Ping branch from 35dc063 to 8e65a91 Compare December 8, 2020 18:13

rwalker-apple approved these changes Dec 8, 2020

View reviewed changes

rwalker-apple reviewed Dec 8, 2020

View reviewed changes

andy31415 approved these changes Dec 10, 2020

View reviewed changes

saurabhst approved these changes Dec 10, 2020

View reviewed changes

vivien-apple force-pushed the Cluster_Ping branch from 8e65a91 to 82bb1d7 Compare December 10, 2020 17:01

mspang approved these changes Dec 10, 2020

View reviewed changes

bzbarsky-apple reviewed Dec 11, 2020

View reviewed changes

examples/chip-tool/commands/clusters/ModelCommand.cpp Outdated Show resolved Hide resolved

vivien-apple force-pushed the Cluster_Ping branch from 82bb1d7 to d12551f Compare December 11, 2020 10:15

Convert 'echo' command to a manufacturer specific 'ping' command

d1054f3

vivien-apple force-pushed the Cluster_Ping branch from d12551f to d1054f3 Compare December 11, 2020 15:31

andy31415 merged commit fb65a31 into project-chip:master Dec 11, 2020

vivien-apple mentioned this pull request Dec 14, 2020

Add custom ZCL extensions inside src/app/zap-templates #4192

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert 'echo' command to a manufacturer specific 'ping' command #4127

Convert 'echo' command to a manufacturer specific 'ping' command #4127

vivien-apple commented Dec 8, 2020

rwalker-apple Dec 8, 2020

mspang Dec 10, 2020

vivien-apple commented Dec 10, 2020

github-actions bot commented Dec 10, 2020

mspang Dec 10, 2020

bzbarsky-apple commented Dec 11, 2020

bzbarsky-apple left a comment

vivien-apple commented Dec 11, 2020

github-actions bot commented Dec 11, 2020

Convert 'echo' command to a manufacturer specific 'ping' command #4127

Convert 'echo' command to a manufacturer specific 'ping' command #4127

Conversation

vivien-apple commented Dec 8, 2020

Problem

Summary of Changes

rwalker-apple Dec 8, 2020

Choose a reason for hiding this comment

mspang Dec 10, 2020

Choose a reason for hiding this comment

vivien-apple commented Dec 10, 2020

github-actions bot commented Dec 10, 2020

mspang Dec 10, 2020

Choose a reason for hiding this comment

bzbarsky-apple commented Dec 11, 2020

bzbarsky-apple left a comment

Choose a reason for hiding this comment

vivien-apple commented Dec 11, 2020

github-actions bot commented Dec 11, 2020