Basic data model structure #1468

kedars · 2020-07-07T14:34:06Z

Problem

Define basic objects for Cluster Server, EndPoint, Cluster and Attributes. Use it from the application. Only a small subset of cluster/attributes is implemented for feedback. The changes to the wifi-echo example provide a quick overview.

CC @bhaskar-apple

So currently with the following call in our echo server:

    ClusterServer server(applicationVersion,
                             HWVersion);
    /* Add a cluster to the primary endpoint of our cluster server */
    server.AddCluster(LEDWidget()); //class LEDWidget() derives from class ClusterOnOff

the following database is created:

Server:
  EndPoint: 1
    ClusterId: 0x0000 (Base)
      Attribute: 0x0000 (ZCLVersion)
              Value: 10
      Attribute: 0x0001 (ApplicationVersion)
              Value: 11
      Attribute: 0x0002 (StackVersion)
              Value: 12
      Attribute: 0x0003 (HWVersion)
              Value: 13
    ClusterId: 0x0006 (OnOff)
      Attribute: 0x0000 (On/Off)
              Value: false
      Attribute: 0x4000 (Global Scene Control)
              Value: false
      Attribute: 0x4001 (OnTime)
              Value: 0
      Attribute: 0x4002 (OffWaitTime)
              Value: 0

and the function

int LEDWidget::Set(AttributeId_t attrId, const CHIPValue &value)

is executed when the attribute is updated over the network

Summary of Changes

Created a structure so that the applications can express the cluster definitions with minimum hassle. The names will be updated to whatever are finalised by the TT. Purposefully there are no 'private' members right now. Other dataTypes, clusters and mandatory attributes within clusters can be defined in subsequent requests.

examples/wifi-echo/server/esp32/main/EchoDeviceCallbacks.cpp

src/lib/core/CHIPEndPoint.h

src/lib/core/CHIPStandardAttributes.h

src/lib/core/CoreLayer.am

CLAassistant · 2020-07-07T20:53:13Z

All committers have signed the CLA.

src/lib/datamodel/CHIPBaseAttribute.h

Also some additional cleanups

This saves us from having to decide when to free it

For now use CHIP_ERROR_INTERNAL for all errors

* move from arrays to deque, no new or delete (yay!) * move GetValue() to be a member function of clusterserver * EndPoint -> Endpoint (endpoint is one word)

mrjerryjohns

Left a bunch of comments around the approach in the CL.

Overall, while I like the direction of application-centric ownership of data, I'm very concerned about the intermix of const and non-const data, as well as the memory cost of the architecture. It's fairly heavy, and I don't think will scale well to more complicated data types.

mrjerryjohns · 2020-07-15T20:14:36Z

src/lib/datamodel/ClusterBasic.h

+ * @brief
+ *   This class implements the Base cluster as defined in the CHIP specification.
+ */
+class ClusterBasic : public Cluster


The class hierarchy designed here (ClusterBasic -> Cluster, which contains Attributes -> Values) is I think much too OO-ified to be performant on embedded targets.

There should be a separation between the specification for a cluster (the set of attributes it has, the type of each attribute, their ranges, etc), from the actual data backing that cluster. The former is typically const type information, since the definition for what a cluster is is immutable, lending itself as well to code-generation. The actual dynamic data backing each attribute is controlled by the application however, and is in RAM.

Here in this CL, the application developer is forced to define clusters by defining themselves what composes that cluster (making it error-prone), as well as placing a lot of pressure on RAM.

Left a bunch of comments around the approach in the CL.

Overall, while I like the direction of application-centric ownership of data, I'm very concerned about the intermix of const and non-const data, as well as the memory cost of the architecture. It's fairly heavy, and I don't think will scale well to more complicated data types.

const can be pulled out with cluster/attribute/value-specific classes, basically putting them in code?

Not sure I follow - could you provide an example?

e.g. for clusterid, instead of a member variable, a class-static const

class Cluster { ... virtual ClusterId Id() const = 0; }; class LevelControl : public Cluster { static const ClusterId kId = 0x0008; // from spec virtual ClusterId Id() const { return kId; } ... };

we'd have to be more clever with the structure of the attributes

That approach starts to break down fairly quickly for other const information - e.g the list of attributes in a given cluster. This is represented by themAttrs private member in the Cluster class, but that clearly cannot be made const.

The only scalable approach IMHO is to actually separately represent the schema-derived information as either code, or better yet, tabular data that is then passed in as an argument to a concrete, mutable, non-const cluster data object.

This is the approach already in place in the existing Silabs code-base - why are we altering it?

@rwalker-apple I think your example will not actually save any code -- last time I ran into examples like the above there needed to be a final keyword on the LevelControl::Id method for the compiler to actually collapse and inline the OO structures. Without the final I believe in most invocations a call to Id will actually involve a virtual call, etc (there may be some cases where compiler can prove to itself that LevelControl is only a LevelControl and not a subclass, but those are incredibly infrequent). And if you are able to specify final, it often means that the class hierarchy is not as deep or does not provide as much utility as one had expected.

This pattern looks great as a design principle; in products we've shipped, we have had better success with other structures (e.g. along the lines of what @mrjerryjohns was getting at). And I have spent more time time than I care to admit ripping out interfaces / pure virtual classes from a codebase to squeeze the code into available space.

That approach starts to break down fairly quickly for other const information - e.g the list of attributes in a given cluster. This is represented by themAttrs private member in the Cluster class, but that clearly cannot be made const.

The only scalable approach IMHO is to actually separately represent the schema-derived information as either code, or better yet, tabular data that is then passed in as an argument to a concrete, mutable, non-const cluster data object.

This is the approach already in place in the existing Silabs code-base - why are we altering it?

In my view, this experiment is aimed at making the SiLabs ZCL implementations easier to compose and unit test and to be able to provide the implementations to customers as libraries instead of source. The aim is more runtime conditionals/composition, less compile-time conditionals/composition.

SiLabs ZCL has no provision for dynamic configuration of a device (except spawning another process), which feels weird to me, but I'm new to this arena.

mrjerryjohns · 2020-07-15T20:22:10Z

src/lib/datamodel/Value.h

+{
+public:
+    ValueTypes mType;
+    union


This value union'ing is RAM in-efficient - every single Value instance is going to have the size of the largest item in this list, even though they may not be using it.

Fixed. Regardless of any command mode we consider (only through cluster-command like ZCL or direct attribute updates like HomeKit), this isn't mandatorily required to be part of the DataModel, we only needed the Set/Get APIs for the attributes read/write. The union is only a placeholder for holding values received by the network-stack.

So the union isn't part of the Attribute/Data-Model any more.

mrjerryjohns · 2020-07-15T20:25:03Z

src/lib/datamodel/Value.h

+    kCHIPValueType_EndOfContainer,
+};
+
+class Value


I don't think this approach scales to handle container/collection types, which can nest other simple value types.

Examples include an array of values, or a struct that contains values.

mrjerryjohns · 2020-07-15T20:26:37Z

src/lib/datamodel/Attribute.h

+public:
+    uint16_t mAttrId;
+    Value mValue;
+    Value mMin;


Attributes that don't have a concept of min/max, or don't even specify such a constraint in schema will pay the cost of storage of these fields.

Fixed, these only will show up for Attributes that are range-bound

kedars · 2020-07-16T08:28:07Z

Left a bunch of comments around the approach in the CL.

Overall, while I like the direction of application-centric ownership of data, I'm very concerned about the intermix of const and non-const data, as well as the memory cost of the architecture. It's fairly heavy, and I don't think will scale well to more complicated data types.

My initial idea behind exposing the attributes and values (which primarily adds to the data model) is direct access to read/write of these.
As I later realised, the primary read/write to these will be through cluster-specific commands. In such a case, creating a base-class for attributes and values, may not be warranted. We could embed those within the cluster itself, thus optimising the corresponding .data bits (pointers & enum), while the mix/max be encoded in the code as rwalker point above.

mrjerryjohns · 2020-07-17T22:10:43Z

the primary read/write to these will be through cluster-specific commands

That is definitely not true. Most reads will happen through the ReadAttribute or likely, the Reporting command.

Most writes will happen WriteAttribute commands. In-direct mutations through command invocations is actually the rarity here.

rwalker-apple · 2020-07-17T23:02:00Z

the primary read/write to these will be through cluster-specific commands

That is definitely not true. Most reads will happen through the ReadAttribute or likely, the Reporting command.

Most writes will happen WriteAttribute commands. In-direct mutations through command invocations is actually the rarity here.

@mrjerryjohns Is that the case for ZCL, or more the case for Weave's data model? When I consider the business logic/attribute dependencies behind LevelControl and OnOff, it seems like command-mutations drive other mutations. Perhaps looking at those 2 clusters first are giving us the wrong impression.

github-actions · 2020-07-21T09:06:11Z

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-21T09:06:15Z

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

github-actions · 2020-07-21T09:15:55Z

Size increase report for "nrf-example-build"

File	Section	File	VM

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

github-actions · 2020-07-21T09:16:01Z

Size increase report for "linux-example-build"

File	Section	File	VM

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

github-actions · 2020-07-21T09:16:08Z

Size increase report for "esp32-example-build"

File	Section	File	VM
chip-wifi-echo.elf	.flash.text	1576	1576
chip-wifi-echo.elf	.dram0.bss	0	240
chip-wifi-echo.elf	.flash.rodata	120	120

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,40392
.debug_line,0,9029
.debug_str,0,8879
.debug_loc,0,3734
.debug_ranges,0,2672
.debug_abbrev,0,2379
[41 Others],0,2372
.shstrtab,0,2210
.flash.text,1576,1576
.strtab,0,1461
.symtab,0,1376
.debug_frame,0,816
.debug_aranges,0,272
.dram0.bss,240,0
.xt.prop._ZN9LEDWidgetD2Ev,0,156
.xt.prop._ZN4chip9DataModel7Cluster3SetEtRKNS0_5ValueE,0,136
.xt.prop._ZN4chip9DataModel7Cluster3GetEtRNS0_5ValueE,0,124
.flash.rodata,120,120
.xt.lit._ZN6Screen8GetTitleB5cxx11Ev,0,120
.xt.lit._ZN4chip9DataModel7ClusterD5Ev,0,88
[Unmapped],0,-120

mrjerryjohns · 2020-07-21T22:47:18Z

src/lib/datamodel/Attribute.h

+     *
+     * @param value  the new value that this attribute should be updated with
+     */
+    virtual CHIP_ERROR Set(const Value & newValue) = 0;


This means a vtable pointer for every attribute, as well as extra vtables for each subclass (2 so far).

This is also type-erased, with Value hiding the actual type of the underlying value element.

With Value being still a union, it means that the caller is still going to pay the penalty of maximal storage when utilizing this API.

This API is no better than just having Set(void *value) - the union inside Value is effectively an opaque data store. In fact, the latter is better since the caller-side allocated storage for the value is exactly as big as the type of the value they're trying to set.

+1

The other thing to note is that for primitive values that fit in a single register passing them by value produces slightly smaller code than the const reference.

mrjerryjohns · 2020-07-21T22:56:19Z

src/lib/datamodel/tests/TestClusterServer.cpp

+    server.AddCluster(switch2);
+
+    /* Validate attributes of the OnOff Cluster on Endpoint 1 */
+    server.GetValue(1, kClusterIdOnOff, kAttributeIdOnOff, value);


There is a lot of type-erasure happening here, making for some heavy/clunky APIs.

I want to just be able to do the following as simple as this:

bool value; server.GetValue(1, kClusterIdOnOff, kAttributeIdOnOff, value);

The validation of the type being boolean should happen as part of GetValue.

mrjerryjohns · 2020-07-21T23:19:32Z

Is that the case for ZCL, or more the case for Weave's data model? When I consider the business logic/attribute dependencies behind LevelControl and OnOff, it seems like command-mutations drive other mutations. Perhaps looking at those 2 clusters first are giving us the wrong impression

It's fairly true for ZCL. Take a look at the Basic cluster, or the Battery Settings cluster (under Power Configuration). Both have 15+ attributes each, with a lot being R/W that don't have custom setters/getter commands on those clusters to read/write those.

mspang · 2020-07-22T04:42:09Z

src/lib/datamodel/tests/BUILD.gn

+  ]
+
+  public_deps = [
+    "${chip_root}/src/inet/tests:tests_common",


Is this dependency needed?

robszewczyk

I would echo @mrjerryjohns concerns about extensive use of polymorphism and is impact on RAM and flash usage.

kedars · 2020-07-27T17:03:37Z

Based on the discussions in the chat channels, closing this for now, until we address the concerns listed there through a proposal first.

Merge in WMN_TOOLS/matter from remove_rps_creation_ci_step to silabs Squashed commit of the following: commit a5167f7ef98d6fa9b9c8813c1feeafb91f1c16d6 Author: Junior Martinez <[email protected]> Date: Mon Jan 8 13:55:45 2024 -0500 Base on discussion, 1 stash command including both s37 and rps file should work and is cleaner commit 2d35889116430c3588834286b3c488611a403378 Author: Junior Martinez <[email protected]> Date: Mon Jan 8 13:30:00 2024 -0500 Fix rps copy condition commit cd213485b39ad19a192e566d5e5a4ad21c6c4dcd Author: Junior Martinez <[email protected]> Date: Mon Jan 8 12:45:15 2024 -0500 remove Generate RPS stage. Store .rps file after build script for 917 soc. Remove the now unsupported wifi SOC boards from wifi test suites and iot reports

restyled-io bot mentioned this pull request Jul 7, 2020

Restyle WIP: Basic data model structure #1469

Closed

bzbarsky-apple reviewed Jul 7, 2020

View reviewed changes

examples/wifi-echo/server/esp32/main/EchoDeviceCallbacks.cpp Outdated Show resolved Hide resolved

rwalker-apple suggested changes Jul 7, 2020

View reviewed changes

rwalker-apple reviewed Jul 8, 2020

View reviewed changes

src/lib/datamodel/CHIPBaseAttribute.h Outdated Show resolved Hide resolved

kedars force-pushed the basic_data_model_structure branch from d649618 to d8c66f4 Compare July 10, 2020 09:30

restyled-io bot mentioned this pull request Jul 10, 2020

Restyle WIP: Basic data model structure #1550

Closed

kedars force-pushed the basic_data_model_structure branch from f2b34a7 to 64f4ec9 Compare July 10, 2020 10:19

kedars and others added 22 commits July 10, 2020 23:09

Prefix simulated datamodel with 'App' to avoid conflicts

10e1df4

Create baseline classes for 'Value' and 'Attribute'

4cca13d

Use enum instead of template

3d409c8

Get a basic Cluster Server structure

eb9c412

Let LEDWidget class in wifi-echo implement the OnOff Cluster

a43a2c9

Also some additional cleanups

Restyled by clang-format

ed27179

Fix incorrect data type

dc2f1d9

Endpoint 0 is reserved

1e4612c

Eliminate CHIP prefix from the datatype/function names

2a33054

Move to lib/datamodel from lib/core

71f575f

Use a static allocation of the Basic Cluster instead of on the heap

825133e

This saves us from having to decide when to free it

add skeleton tests directory, tweaks to make system

8ef76bf

fixup

3424e9e

remove CHIP for files that live in datamodel/ subdir

d4e4f54

more little renames

5fd8f44

Add tests for ClusterServer

16d7861

Use CHIPError.h for errors

19133b5

For now use CHIP_ERROR_INTERNAL for all errors

Move standard clusters in their own files

9b5a74d

Documentation updates

679cc6b

Missed the change in the test file

5eee74d

* add Deque.h

725ddca

* move from arrays to deque, no new or delete (yay!) * move GetValue() to be a member function of clusterserver * EndPoint -> Endpoint (endpoint is one word)

fixups

300cf24

mrjerryjohns suggested changes Jul 15, 2020

View reviewed changes

functor instead of std::function

3e390b8

Use 'g' prefix for globals

4901e06

kedars force-pushed the basic_data_model_structure branch 2 times, most recently from bd120a7 to 08ac64a Compare July 19, 2020 11:15

Introduce ClusterId_t and AttributeId_t

41fe827

kedars force-pushed the basic_data_model_structure branch from 08ac64a to 41fe827 Compare July 19, 2020 11:20

Don't embed 'enum' or min/max in the Attribute class

f42189d

kedars force-pushed the basic_data_model_structure branch from 508ea0e to f42189d Compare July 21, 2020 09:00

mrjerryjohns suggested changes Jul 21, 2020

View reviewed changes

mspang reviewed Jul 22, 2020

View reviewed changes

src/lib/datamodel/tests/BUILD.gn

]

public_deps = [

"${chip_root}/src/inet/tests:tests_common",

Copy link

Contributor

mspang Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this dependency needed?

robszewczyk requested changes Jul 22, 2020

View reviewed changes

woody-apple requested a review from rwalker-apple July 23, 2020 16:40

woody-apple added lib repo labels Jul 23, 2020

kedars closed this Jul 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic data model structure #1468

Basic data model structure #1468

kedars commented Jul 7, 2020 •

edited

Loading

CLAassistant commented Jul 7, 2020 •

edited

Loading

mrjerryjohns left a comment

mrjerryjohns Jul 15, 2020

rwalker-apple Jul 15, 2020

mrjerryjohns Jul 17, 2020

rwalker-apple Jul 17, 2020 •

edited

Loading

rwalker-apple Jul 17, 2020

mrjerryjohns Jul 21, 2020 •

edited

Loading

robszewczyk Jul 22, 2020

rwalker-apple Jul 22, 2020 •

edited

Loading

mrjerryjohns Jul 15, 2020

kedars Jul 21, 2020

mrjerryjohns Jul 15, 2020

mrjerryjohns Jul 15, 2020

kedars Jul 21, 2020

kedars commented Jul 16, 2020 •

edited

Loading

mrjerryjohns commented Jul 17, 2020

rwalker-apple commented Jul 17, 2020 •

edited

Loading

github-actions bot commented Jul 21, 2020

github-actions bot commented Jul 21, 2020

github-actions bot commented Jul 21, 2020

github-actions bot commented Jul 21, 2020

github-actions bot commented Jul 21, 2020

mrjerryjohns Jul 21, 2020

mrjerryjohns Jul 21, 2020

mrjerryjohns Jul 21, 2020

robszewczyk Jul 22, 2020

mrjerryjohns Jul 21, 2020

mrjerryjohns commented Jul 21, 2020

mspang Jul 22, 2020

robszewczyk left a comment

kedars commented Jul 27, 2020

Basic data model structure #1468

Basic data model structure #1468

Conversation

kedars commented Jul 7, 2020 • edited Loading

Problem

Summary of Changes

CLAassistant commented Jul 7, 2020 • edited Loading

mrjerryjohns 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

rwalker-apple Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrjerryjohns Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwalker-apple Jul 22, 2020 • edited Loading

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

kedars commented Jul 16, 2020 • edited Loading

mrjerryjohns commented Jul 17, 2020

rwalker-apple commented Jul 17, 2020 • edited Loading

github-actions bot commented Jul 21, 2020

github-actions bot commented Jul 21, 2020

github-actions bot commented Jul 21, 2020

github-actions bot commented Jul 21, 2020

github-actions bot commented Jul 21, 2020

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

mrjerryjohns commented Jul 21, 2020

Choose a reason for hiding this comment

robszewczyk left a comment

Choose a reason for hiding this comment

kedars commented Jul 27, 2020

kedars commented Jul 7, 2020 •

edited

Loading

CLAassistant commented Jul 7, 2020 •

edited

Loading

rwalker-apple Jul 17, 2020 •

edited

Loading

mrjerryjohns Jul 21, 2020 •

edited

Loading

rwalker-apple Jul 22, 2020 •

edited

Loading

kedars commented Jul 16, 2020 •

edited

Loading

rwalker-apple commented Jul 17, 2020 •

edited

Loading