Add new on/off cluster implementation based on the Silicon Labs ZigbeePro code #1609

bzbarsky-apple · 2020-07-15T05:32:30Z

Problem

We actually want to pull in the ZigbeePro implementation from Silicon Labs, not the Dotdot implementation.

Summary of Changes

Pulls in (into the echo server for now) enough code to stand up the on-off cluster. Modifies our two chip-tool clients to talk the slightly different format this code expects.

Fixes #1216 and #1393

CLAassistant · 2020-07-15T05:32:35Z

All committers have signed the CLA.

bzbarsky-apple · 2020-07-15T05:33:52Z

@wehale, could you or someone else from Silicon Labs please review the commits that pull in Silicon Labs code (the first two commits in this branch) to make sure that there's nothing obviously broken with the licensing bits? This is particularly important for the generated files, since I added the license header to those, and that needs approval from someone at Silicon Labs...

lgtm-com · 2020-07-15T05:48:14Z

This pull request introduces 1 alert when merging ea31e71 into 475b533 - view on LGTM.com

new alerts:

1 for Expression has no effect

bzbarsky-apple · 2020-07-15T06:09:45Z

Hmm. Looks like the nRF example app (examples/lock-app) is using the dotdot data model bits. I guess I need to fix it to link to libCHIPDataModel directly...

I'm not sure yet what the "GN Examples / Linux Standalone" failure is.

wehale · 2020-07-15T09:29:01Z

Sergei, can you look at this? Thanks, Ezra

…

Sent from my iPhone On Jul 15, 2020, at 1:34 AM, Boris Zbarsky <[email protected]> wrote: CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. @wehale<https://github.com/wehale>, could you or someone else from Silicon Labs please review the commits that pull in Silicon Labs code (the first two commits in this branch) to make sure that there's nothing obviously broken with the licensing bits? This is particularly important for the generated files, since I added the license header to those, and that needs approval from someone at Silicon Labs... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#1609 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AD27TVGU2ZYSBV4BXAAX3JLR3U5U3ANCNFSM4O2FTPZA>.

wehale

this all looks good, the licenses look correct.

wehale · 2020-07-15T10:14:31Z

examples/wifi-echo/server/esp32/main/gen/att-storage.h

+ */
+// This file is generated by Simplicity Studio.  Please do not edit manually.
+//
+//


This Simplicity Studio comment can probably be removed, or will be removed in the future

I expect a bunch of cleanup to happen here after initial landing. The goal was to land something minimally different from the "upstream" code as a first cut.

selissia · 2020-07-15T13:24:13Z

@bzbarsky-apple: I'm reviewing it now.

andy31415

We are checking in large files under gen/ . Assuming these are machine generated. Could we make the build system generate them instead?

andy31415 · 2020-07-15T13:23:38Z

examples/wifi-echo/server/esp32/main/af-main.h

+ *    limitations under the License.
+ */
+
+/**


Is this supposed to be dual copyright, since SiLabs is already a CHIP Author? Question applies throughout

IANAL

andy31415 · 2020-07-15T13:24:17Z

examples/wifi-echo/server/esp32/main/af-main.h

+
+#include "af-types.h"
+
+//#include CONFIGURATION_HEADER


Remove commented out code?

This commented out include seems to occur in other places as well.

As I mentioned above, the goal is to land something with as few differences from the original SiLabs code as possible, subject to the "it compiles and runs" constraint. Once this lands, we will definitely be doing a bunch of cleanup, which will include removing the commented-out bits here.

I filed #1611 on this.

andy31415 · 2020-07-15T13:28:03Z

examples/wifi-echo/server/esp32/main/include/CHIPDeviceManager.h

@@ -77,7 +75,7 @@ class DLL_EXPORT CHIPDeviceManagerCallbacks
     * @param size               size of the attribute
     * @param value              pointer to the new value
     */
-    virtual void PostAttributeChangeCallback(uint8_t endpoint, ChipZclClusterId clusterId, ChipZclAttributeId attributeId,
+    virtual void PostAttributeChangeCallback(uint8_t endpoint, EmberAfClusterId clusterId, EmberAfAttributeId attributeId,


Should we rename Ember to CHIP or have a Github issue to do so?

I think it would be best to rename things in a different PR, this way there is a record that would help correlate the code to its Silabs' origin.

The latter. #1610 filed.

wehale · 2020-07-15T13:32:27Z

We can and we will, we’re just not ready to yet. We have not landed on exact generation criteria yet. Once we know exactly what we want to generate, we’ll create generation templates for ZAP and move headless generation into the build system. Thanks, Ezra From: Andrei Litvin <[email protected]> Reply-To: project-chip/connectedhomeip <[email protected]> Date: Wednesday, July 15, 2020 at 9:30 AM To: project-chip/connectedhomeip <[email protected]> Cc: Ezra Hale <[email protected]>, Mention <[email protected]> Subject: Re: [project-chip/connectedhomeip] Add new on/off cluster implementation based on the Silicon Labs ZigbeePro code (#1609) CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. @andy31415 commented on this pull request. We are checking in large files under gen/ . Assuming these are machine generated. Could we make the build system generate them instead?

________________________________ In examples/wifi-echo/server/esp32/main/af-main.h<#1609 (comment)>:

+ * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** Is this supposed to be dual copyright, since SiLabs is already a CHIP Author? Question applies throughout IANAL

________________________________ In examples/wifi-echo/server/esp32/main/af-main.h<#1609 (comment)>:

+ * See the License for the specific language governing permissions and

+ * limitations under the License. + */ +/***************************************************************************/ +/** + * @file + * @brief + ******************************************************************************* + ******************************************************************************/ + +#ifndef SILABS_AF_MAIN_H +#define SILABS_AF_MAIN_H + +#include "af-types.h" + +//#include CONFIGURATION_HEADER Remove commented out code? This commented out include seems to occur in other places as well.

________________________________ In examples/wifi-echo/server/esp32/main/include/CHIPDeviceManager.h<#1609 (comment)>:

@@ -77,7 +75,7 @@ class DLL_EXPORT CHIPDeviceManagerCallbacks

* @param size size of the attribute * @param value pointer to the new value */ - virtual void PostAttributeChangeCallback(uint8_t endpoint, ChipZclClusterId clusterId, ChipZclAttributeId attributeId, + virtual void PostAttributeChangeCallback(uint8_t endpoint, EmberAfClusterId clusterId, EmberAfAttributeId attributeId, Should we rename Ember to CHIP or have a Github issue to do so? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#1609 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AD27TVGTUTADZPB4Q7RF5BTR3WVNRANCNFSM4O2FTPZA>.

selissia · 2020-07-15T13:53:43Z

I have reviewed the first two commits in the PR, everything looks good.

All except type_stubs.h come from project-chip#1164 types_stub.h comes from project-chip#1425 (comment) and project-chip#1425 (comment)

… Hale. The sample app only implements the On/Off cluster. Most of the files have the same exact name as in the sample app, with the following exceptions: * endpoint_config.h was originally ZigbeeMinimalSoc_endpoint_config.h * gen_config.h was originally ZigbeeMinimalSoc.h * gen_tokens.h was originally ZigbeeMinimalSoc_tokens.h

This corrects various obvious problems in the file that prevent files that include it from compiling.

…s ZigbeePro implementation can handle. The actual encoding we are using is not quite the APS encoding, but it's still a throwaway, so that does not matter too much.

We no longer compile the Dotdot-based ZCL data model files into libCHIP, because those have symbols whose names conflict with the new ZigbeePro-based ZCL data model files.

github-actions · 2020-07-17T19:37:57Z

Size increase report for "nrf-example-build"

File	Section	File	VM
chip-nrf52840-lock-example.out	.bss	0	-8
chip-nrf52840-lock-example.out	.text	-12	-12

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-nrf52840-lock-example.out and ./pull_artifact/chip-nrf52840-lock-example.out:

sections,vmsize,filesize
.debug_str,0,79
[Unmapped],0,13
.bss,-8,0
.text,-12,-12

github-actions · 2020-07-17T19:38:01Z

Size increase report for "linux-example-build"

File	Section	File	VM
chip-standalone-demo.out	.rodata	88	88
chip-standalone-demo.out	.bss	0	-32
chip-standalone-demo.out	.eh_frame_hdr	-184	-184
chip-standalone-demo.out	.eh_frame	-600	-600
chip-standalone-demo.out	.text	-1008	-1008

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-standalone-demo.out and ./pull_artifact/chip-standalone-demo.out:

sections,vmsize,filesize
[Unmapped],0,1703
.rodata,88,88
.bss,-32,0
.eh_frame_hdr,-184,-184
.debug_aranges,0,-368
.strtab,0,-595
.eh_frame,-600,-600
.symtab,0,-600
.debug_ranges,0,-800
.text,-1008,-1008
.debug_loc,0,-1061
.debug_abbrev,0,-1433
.debug_macro,0,-2173
.debug_line,0,-3333
.debug_str,0,-6566
.debug_info,0,-7422

github-actions · 2020-07-17T19:38:10Z

Size increase report for "esp32-example-build"

File	Section	File	VM
chip-wifi-echo.elf	.flash.text	1580	1580
chip-wifi-echo.elf	.dram0.bss	0	-208
chip-wifi-echo.elf	.flash.rodata	-1448	-1448

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-wifi-echo.elf and ./pull_artifact/chip-wifi-echo.elf:

sections,vmsize,filesize
.debug_info,0,55269
.debug_line,0,29295
.debug_loc,0,19746
.debug_str,0,18226
.debug_frame,0,5700
.debug_abbrev,0,4318
.debug_ranges,0,1992
.flash.text,1580,1580
[Unmapped],0,1451
.debug_aranges,0,1400
.strtab,0,915
.symtab,0,656
.dram0.bss,-208,0
.flash.rodata,-1448,-1448

github-actions · 2020-07-17T19:38:16Z

Size increase report for "gn_nrf-example-build"

File	Section	File	VM

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

github-actions · 2020-07-17T19:38:21Z

Size increase report for "gn_linux-example-build"

File	Section	File	VM

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

erjiaqing · 2020-07-18T01:27:58Z

examples/wifi-echo/server/esp32/main/af-main.h

@@ -0,0 +1,162 @@
+/**


Are these files common or intended to be generated in real apps? I suggest to move them to common since I did not see esp32 specific codes.

These files are common, but can't compile without the gen bits (which are app-specific) right now. See #1630.

We can move them to examples/common if files in there can include app-specific headers (which is possible if we set up the include paths for the apps correctly).

Once #1630 is fixed, we can move these files somewhere else much more easily, of course.

bhaskar-apple

Looks good to me. Thanks Boris

woody-apple · 2020-07-20T18:12:26Z

@saurabhst @BroderickCarlin @jelderton ?

woody-apple · 2020-07-21T16:18:47Z

@saurabhst @jelderton ?

bzbarsky-apple requested review from wehale, bhaskar-apple and rwalker-apple July 15, 2020 05:32

bzbarsky-apple requested review from andy31415, anush-apple, BroderickCarlin, chrisdecenzo, hawk248, jelderton, robszewczyk, saurabhst and woody-apple as code owners July 15, 2020 05:32

bzbarsky-apple changed the title ~~Add new on off~~ Add new on/off cluster implementation based on the Silicon Labs ZigbeePro code Jul 15, 2020

wehale approved these changes Jul 15, 2020

View reviewed changes

andy31415 reviewed Jul 15, 2020

View reviewed changes

selissia approved these changes Jul 15, 2020

View reviewed changes

This was referenced Jul 15, 2020

Clean up naming in the Silicon Labs ZigbeePro code #1610

Open

Clean up commented-out code in Silicon Labs ZigbeePro files #1611

Closed

bzbarsky-apple added 3 commits July 15, 2020 12:32

Add initial Silicon Labs ZCL data model files.

44fbe1e

All except type_stubs.h come from project-chip#1164 types_stub.h comes from project-chip#1425 (comment) and project-chip#1425 (comment)

Fix compile errors in types_stub.h

8745ea0

This corrects various obvious problems in the file that prevent files that include it from compiling.

Bhaskar Sarma and others added 4 commits July 17, 2020 15:29

Add utilities for encoding and decoding messages that the Silicon Lab…

76aa9b0

…s ZigbeePro implementation can handle. The actual encoding we are using is not quite the APS encoding, but it's still a throwaway, so that does not matter too much.

Switch the echo server to using the new On/Off implementation.

b847240

We no longer compile the Dotdot-based ZCL data model files into libCHIP, because those have symbols whose names conflict with the new ZigbeePro-based ZCL data model files.

Switch command-line chip-tool to using the new On/Off implementation.

9c79b6c

Switch iOS CHIPTool to using the new On/Off implementation.

b83d489

bzbarsky-apple force-pushed the add-new-on-off branch from ae6f93d to b83d489 Compare July 17, 2020 19:29

bzbarsky-apple mentioned this pull request Jul 17, 2020

Change the existing zcl codec bits to match the data format of the new ones we are planning to add. #1645

Merged

erjiaqing reviewed Jul 18, 2020

View reviewed changes

bhaskar-apple reviewed Jul 20, 2020

View reviewed changes

woody-apple approved these changes Jul 20, 2020

View reviewed changes

saurabhst approved these changes Jul 21, 2020

View reviewed changes

woody-apple merged commit 4dd5230 into project-chip:master Jul 21, 2020

bzbarsky-apple deleted the add-new-on-off branch July 21, 2020 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new on/off cluster implementation based on the Silicon Labs ZigbeePro code #1609

Add new on/off cluster implementation based on the Silicon Labs ZigbeePro code #1609

bzbarsky-apple commented Jul 15, 2020

CLAassistant commented Jul 15, 2020 •

edited

Loading

bzbarsky-apple commented Jul 15, 2020

lgtm-com bot commented Jul 15, 2020

bzbarsky-apple commented Jul 15, 2020

wehale commented Jul 15, 2020 via email

wehale left a comment

wehale Jul 15, 2020

bzbarsky-apple Jul 15, 2020

selissia commented Jul 15, 2020

andy31415 left a comment

andy31415 Jul 15, 2020

andy31415 Jul 15, 2020

bzbarsky-apple Jul 15, 2020

bzbarsky-apple Jul 15, 2020

andy31415 Jul 15, 2020

selissia Jul 15, 2020

bzbarsky-apple Jul 15, 2020

wehale commented Jul 15, 2020 via email

selissia commented Jul 15, 2020

github-actions bot commented Jul 17, 2020

github-actions bot commented Jul 17, 2020

github-actions bot commented Jul 17, 2020

github-actions bot commented Jul 17, 2020

github-actions bot commented Jul 17, 2020

erjiaqing Jul 18, 2020

bzbarsky-apple Jul 18, 2020

bhaskar-apple left a comment

woody-apple commented Jul 20, 2020

woody-apple commented Jul 21, 2020


		#include "af-types.h"

		//#include CONFIGURATION_HEADER

Add new on/off cluster implementation based on the Silicon Labs ZigbeePro code #1609

Add new on/off cluster implementation based on the Silicon Labs ZigbeePro code #1609

Conversation

bzbarsky-apple commented Jul 15, 2020

Problem

Summary of Changes

CLAassistant commented Jul 15, 2020 • edited Loading

bzbarsky-apple commented Jul 15, 2020

lgtm-com bot commented Jul 15, 2020

bzbarsky-apple commented Jul 15, 2020

wehale commented Jul 15, 2020 via email

wehale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selissia commented Jul 15, 2020

andy31415 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wehale commented Jul 15, 2020 via email

selissia commented Jul 15, 2020

github-actions bot commented Jul 17, 2020

github-actions bot commented Jul 17, 2020

github-actions bot commented Jul 17, 2020

github-actions bot commented Jul 17, 2020

github-actions bot commented Jul 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhaskar-apple left a comment

Choose a reason for hiding this comment

woody-apple commented Jul 20, 2020

woody-apple commented Jul 21, 2020

CLAassistant commented Jul 15, 2020 •

edited

Loading