handling global attributes per cluster #1142

paulr34 · 2023-09-17T16:10:48Z

handling global attributes per cluster

paulr34 · 2023-09-17T16:12:22Z

@tecimovic @bzbarsky-apple @brdandu proof of concept for changing global attributes on a per cluster basis. I am aware that I need to clean it up but It works.

src-electron/db/query-config.js

paulr34 · 2023-09-19T21:57:02Z

ready for review.

bzbarsky-apple

Just tested, and this does not seem to fix #543 in any of the contexts I looked at:

We are still ending up with accessor codegen for global attributes when we should not.
We are not getting the relevant attributes flagged as external in endpoint_config.h
zap convert is not marking them external in the .zap file.

paulr34 · 2023-09-20T01:14:04Z

Just tested, and this does not seem to fix #543 in any of the contexts I looked at:

We are still ending up with accessor codegen for global attributes when we should not.

We are not getting the relevant attributes flagged as external in endpoint_config.h

zap convert is not marking them external in the .zap file.

Thank you. I will work on those cases tomorrow.

codecov-commenter · 2023-09-20T12:35:56Z

Codecov Report

Merging #1142 (a73c55a) into master (bc2ca3c) will increase coverage by 0.08%.
The diff coverage is 96.49%.

@@            Coverage Diff             @@
##           master    #1142      +/-   ##
==========================================
+ Coverage   65.16%   65.25%   +0.08%     
==========================================
  Files         170      171       +1     
  Lines       18232    18284      +52     
  Branches     3927     3936       +9     
==========================================
+ Hits        11881    11931      +50     
- Misses       6351     6353       +2

Files	Coverage Δ
src-electron/db/query-cluster.js	`92.85% <100.00%> (+1.19%)`	⬆️
src-electron/db/query-config.js	`80.67% <100.00%> (-0.06%)`	⬇️
src-electron/db/query-impexp.js	`98.13% <100.00%> (+0.07%)`	⬆️
src-electron/db/query-package.js	`88.94% <100.00%> (+0.21%)`	⬆️
src-electron/importexport/import-json.js	`82.45% <ø> (ø)`
src-electron/zcl/zcl-loader-silabs.js	`94.43% <ø> (-0.01%)`	⬇️
src-electron/upgrade/upgrade.js	`95.00% <95.00%> (ø)`

package-lock.json

src-electron/db/query-attribute.js

src-electron/db/query-cluster.js

src-electron/db/query-config.js

src-electron/db/query-impexp.js

brdandu · 2023-09-20T14:09:45Z

src-electron/db/query-upgrade.js

+const fs = require('fs')
+const fsp = fs.promises
+
+async function selectClusterName(db, clusterRef) {


If you need this move this to query-cluster.js

originally I had it there but will probably just do what you suggested above, thanks

src-electron/db/query-upgrade.js

brdandu · 2023-09-20T14:12:15Z

src-electron/db/query-upgrade.js

+  return attributeName[0].NAME
+}
+
+async function queryMetaFile(db, packageId) {


Use query-package#getPackageByPackageId instead and get rid of this

unfortunately that query does not retrieve the PATH which I need

Hmm are you sure: I see
const querySelectFromPackage =
SELECT
PACKAGE_ID,
PATH,
TYPE,
CRC,
VERSION,
CATEGORY,
DESCRIPTION
FROM PACKAGE ``

src-electron/db/query-upgrade.js

brdandu · 2023-09-20T14:22:57Z

Also fix Zigbee Code generation Issue in the CI

bzbarsky-apple · 2023-09-20T22:25:11Z

src-electron/db/query-upgrade.js

+ * @returns An array of objects
+ */
+
+async function checkGlobals(db, attributeId) {


This name is way more generic than the documented behavior of the function. And how does the attributeId fit in with the documented behavior bit? It does not seem to be used below....

I suspect this function just needs to be renamed to reflect what it actually does.

agree, will fix, thanks

attributeId is used to get the associated package

bzbarsky-apple · 2023-09-20T22:25:33Z

src-electron/db/query-upgrade.js

+ * @returns A flag.
+ */
+
+async function checkStorage(


Perhaps computeStorageOption?

sounds good to me, will fix, thanks

bzbarsky-apple

@paulr34 Actually, there is one more thing still broken here. This template:

{{#zcl_clusters}}
{{#zcl_attributes_server}}
{{#unless (isStrEqual storagePolicy "attributeAccessInterface")}}

does not behave correctly, because for global attributes this query is not producing the right storagePolicy.....

paulr34 · 2023-09-21T00:28:53Z

@paulr34 Actually, there is one more thing still broken here. This template:
{{#zcl_clusters}}

{{#zcl_attributes_server}}

{{#unless (isStrEqual storagePolicy "attributeAccessInterface")}}
does not behave correctly, because for global attributes this query is not producing the right storagePolicy.....

Thank you for catching this. I'm on it.

paulr34 · 2023-09-21T17:48:17Z

@paulr34 Actually, there is one more thing still broken here. This template:
{{#zcl_clusters}}
{{#zcl_attributes_server}}
{{#unless (isStrEqual storagePolicy "attributeAccessInterface")}}
does not behave correctly, because for global attributes this query is not producing the right storagePolicy.....

    "Access Control": [
      "ClusterRevision",
      "SubjectsPerAccessControlEntry",
      "TargetsPerAccessControlEntry",
      "AccessControlEntriesPerFabric"
    ],
    "Administrator Commissioning": [
      "WindowStatus",
      "AdminFabricIndex",
      "AdminVendorId"
    ],
    "Basic Information": [
      "DataModelRevision",
      "VendorName",
      "VendorID",
      "ProductName",
      "ProductID",
      "Location",
      "HardwareVersion",
      "HardwareVersionString",
      "SoftwareVersion",
      "SoftwareVersionString",
      "ManufacturingDate",
      "PartNumber",
      "ProductURL",
      "ProductLabel",
      "SerialNumber",
      "UniqueID",
      "CapabilityMinima"
    ],
    "Descriptor": ["ClusterRevision"],
    "Ethernet Network Diagnostics": [
      "PHYRate",
      "FullDuplex",
      "CarrierDetect",
      "TimeSinceReset",
      "PacketRxCount",
      "PacketTxCount",
      "TxErrCount",
      "CollisionCount",
      "OverrunCount"
    ],
    "General Commissioning": [
      "RegulatoryConfig",
      "LocationCapability",
      "BasicCommissioningInfo",
      "SupportsConcurrentConnection"
    ],
    "General Diagnostics": [
      "NetworkInterfaces",
      "ActiveHardwareFaults",
      "ActiveRadioFaults",
      "ActiveNetworkFaults",
      "RebootCount",
      "UpTime",
      "TotalOperationalHours",
      "BootReason"
    ],
    "Group Key Management": [
      "ClusterRevision",
      "MaxGroupsPerFabric",
      "MaxGroupKeysPerFabric",
      "FeatureMap"
    ],
    "Operational Credentials": [
      "SupportedFabrics",
      "CommissionedFabrics",
      "CurrentFabricIndex"
    ],
    "Software Diagnostics": [
      "CurrentHeapFree",
      "CurrentHeapHighWatermark",
      "CurrentHeapUsed",
      "FeatureMap",
      "ThreadMetrics"
    ],
    "Unit Testing": [
      "struct_attr",
      "nullable_struct",
      "general_error_boolean",
      "cluster_error_boolean"
    ],
    "Thread Network Diagnostics": [
      "Channel",
      "RoutingRole",
      "NetworkName",
      "PanId",
      "ExtendedPanId",
      "MeshLocalPrefix",
      "OverrunCount",
      "PartitionId",
      "Weighting",
      "DataVersion",
      "StableDataVersion",
      "LeaderRouterId",
      "DetachedRoleCount",
      "ChildRoleCount",
      "RouterRoleCount",
      "LeaderRoleCount",
      "AttachAttemptCount",
      "PartitionIdChangeCount",
      "BetterPartitionAttachAttemptCount",
      "ParentChangeCount",
      "TxTotalCount",
      "TxUnicastCount",
      "TxBroadcastCount",
      "TxAckRequestedCount",
      "TxAckedCount",
      "TxNoAckRequestedCount",
      "TxDataCount",
      "TxDataPollCount",
      "TxBeaconCount",
      "TxBeaconRequestCount",
      "TxOtherCount",
      "TxRetryCount",
      "TxDirectMaxRetryExpiryCount",
      "TxIndirectMaxRetryExpiryCount",
      "TxErrCcaCount",
      "TxErrAbortCount",
      "TxErrBusyChannelCount",
      "RxTotalCount",
      "RxUnicastCount",
      "RxBroadcastCount",
      "RxDataCount",
      "RxDataPollCount",
      "RxBeaconCount",
      "RxBeaconRequestCount",
      "RxOtherCount",
      "RxAddressFilteredCount",
      "RxDestAddrFilteredCount",
      "RxDuplicatedCount",
      "RxErrNoFrameCount",
      "RxErrUnknownNeighborCount",
      "RxErrInvalidSrcAddrCount",
      "RxErrSecCount",
      "RxErrFcsCount",
      "RxErrOtherCount",
      "ActiveTimestamp",
      "PendingTimestamp",
      "Delay",
      "SecurityPolicy",
      "ChannelPage0Mask",
      "OperationalDatasetComponents"
    ],
    "WiFi Network Diagnostics": [
      "BSSID",
      "SecurityType",
      "WiFiVersion",
      "ChannelNumber",
      "RSSI",
      "BeaconLostCount",
      "BeaconRxCount",
      "PacketMulticastRxCount",
      "PacketMulticastTxCount",
      "PacketUnicastRxCount",
      "PacketUnicastTxCount",
      "CurrentMaxRate",
      "OverrunCount"
    ]
  },```

paulr34 · 2023-09-21T17:55:29Z

@paulr34 Actually, there is one more thing still broken here. This template:
{{#zcl_clusters}}
{{#zcl_attributes_server}}
{{#unless (isStrEqual storagePolicy "attributeAccessInterface")}}
does not behave correctly, because for global attributes this query is not producing the right storagePolicy.....

    "Access Control": [
      "ClusterRevision",
      "SubjectsPerAccessControlEntry",
      "TargetsPerAccessControlEntry",
      "AccessControlEntriesPerFabric"
    ],
    "Administrator Commissioning": [
      "WindowStatus",
      "AdminFabricIndex",
      "AdminVendorId"
    ],
    "Basic Information": [
      "DataModelRevision",
      "VendorName",
      "VendorID",
      "ProductName",
      "ProductID",
      "Location",
      "HardwareVersion",
      "HardwareVersionString",
      "SoftwareVersion",
      "SoftwareVersionString",
      "ManufacturingDate",
      "PartNumber",
      "ProductURL",
      "ProductLabel",
      "SerialNumber",
      "UniqueID",
      "CapabilityMinima"
    ],
    "Descriptor": ["ClusterRevision"],
    "Ethernet Network Diagnostics": [
      "PHYRate",
      "FullDuplex",
      "CarrierDetect",
      "TimeSinceReset",
      "PacketRxCount",
      "PacketTxCount",
      "TxErrCount",
      "CollisionCount",
      "OverrunCount"
    ],
    "General Commissioning": [
      "RegulatoryConfig",
      "LocationCapability",
      "BasicCommissioningInfo",
      "SupportsConcurrentConnection"
    ],
    "General Diagnostics": [
      "NetworkInterfaces",
      "ActiveHardwareFaults",
      "ActiveRadioFaults",
      "ActiveNetworkFaults",
      "RebootCount",
      "UpTime",
      "TotalOperationalHours",
      "BootReason"
    ],
    "Group Key Management": [
      "ClusterRevision",
      "MaxGroupsPerFabric",
      "MaxGroupKeysPerFabric",
      "FeatureMap"
    ],
    "Operational Credentials": [
      "SupportedFabrics",
      "CommissionedFabrics",
      "CurrentFabricIndex"
    ],
    "Software Diagnostics": [
      "CurrentHeapFree",
      "CurrentHeapHighWatermark",
      "CurrentHeapUsed",
      "FeatureMap",
      "ThreadMetrics"
    ],
    "Unit Testing": [
      "struct_attr",
      "nullable_struct",
      "general_error_boolean",
      "cluster_error_boolean"
    ],
    "Thread Network Diagnostics": [
      "Channel",
      "RoutingRole",
      "NetworkName",
      "PanId",
      "ExtendedPanId",
      "MeshLocalPrefix",
      "OverrunCount",
      "PartitionId",
      "Weighting",
      "DataVersion",
      "StableDataVersion",
      "LeaderRouterId",
      "DetachedRoleCount",
      "ChildRoleCount",
      "RouterRoleCount",
      "LeaderRoleCount",
      "AttachAttemptCount",
      "PartitionIdChangeCount",
      "BetterPartitionAttachAttemptCount",
      "ParentChangeCount",
      "TxTotalCount",
      "TxUnicastCount",
      "TxBroadcastCount",
      "TxAckRequestedCount",
      "TxAckedCount",
      "TxNoAckRequestedCount",
      "TxDataCount",
      "TxDataPollCount",
      "TxBeaconCount",
      "TxBeaconRequestCount",
      "TxOtherCount",
      "TxRetryCount",
      "TxDirectMaxRetryExpiryCount",
      "TxIndirectMaxRetryExpiryCount",
      "TxErrCcaCount",
      "TxErrAbortCount",
      "TxErrBusyChannelCount",
      "RxTotalCount",
      "RxUnicastCount",
      "RxBroadcastCount",
      "RxDataCount",
      "RxDataPollCount",
      "RxBeaconCount",
      "RxBeaconRequestCount",
      "RxOtherCount",
      "RxAddressFilteredCount",
      "RxDestAddrFilteredCount",
      "RxDuplicatedCount",
      "RxErrNoFrameCount",
      "RxErrUnknownNeighborCount",
      "RxErrInvalidSrcAddrCount",
      "RxErrSecCount",
      "RxErrFcsCount",
      "RxErrOtherCount",
      "ActiveTimestamp",
      "PendingTimestamp",
      "Delay",
      "SecurityPolicy",
      "ChannelPage0Mask",
      "OperationalDatasetComponents"
    ],
    "WiFi Network Diagnostics": [
      "BSSID",
      "SecurityType",
      "WiFiVersion",
      "ChannelNumber",
      "RSSI",
      "BeaconLostCount",
      "BeaconRxCount",
      "PacketMulticastRxCount",
      "PacketMulticastTxCount",
      "PacketUnicastRxCount",
      "PacketUnicastTxCount",
      "CurrentMaxRate",
      "OverrunCount"
    ]
  },```

@bzbarsky-apple is the above list correct? or out of date?

paulr34 · 2023-09-28T12:53:49Z

src-electron/db/query-upgrade.js

+    if (forcedExternal.lists == true && attribute.type == 'array') {
+      storageOption = dbEnum.storageOption.external
+    }
+    if (


@bzbarsky-apple this code which presumably changes global attributes to external if they are in the list seems to cause CI and codeine failures. Any idea what the failures mean?

That code changes all attributes to be external if they are of array type. And that code is very much pre-existing, the right behavior, and not the problem here.

okay thank you

jingteng25742 · 2023-09-28T15:57:09Z

src-electron/db/query-upgrade.js

+  let zcl = await queryPackage.getMetaFile(db, pkgs)
+  let obj = await fsp.readFile(zcl)
+  let data = JSON.parse(obj)
+  let externals = data.attributeAccessInterfaceAttributes


Is attributeAccessInterfaceAttributes always present in the zcl meta file?
Optional Chaining (data?.fieldName) can be used here when it's not guaranteed.

Is attributeAccessInterfaceAttributes always present in the zcl meta file?

No.

good idea @jingteng25742 will fix, thanks

jingteng25742 · 2023-09-28T15:58:44Z

src-electron/db/query-upgrade.js

+) {
+  let storageOption
+  let attributeName
+  if (!clusterName) {


Did you meant to test clusterName or clusterRef?

clusterName

I believe you are passing null values to the cluster name and then extracting the cluster name regardless. See let storageOption = await queryUpgrade.computeStorage( db, null,

I think I only retrieve clusterName if it is null

if (!clusterName) {
clusterName = await queryCluster.selectClusterName(db, clusterRef)

bzbarsky-apple

So this is a partial fix, in that it does not fix item 1 from #1142 (review), right?

bzbarsky-apple · 2023-09-28T16:05:02Z

src-electron/db/query-config.js

+    staticAttribute.storagePolicy,
+    forcedExternal,


I guess my architectural question is this: if we are doing the "getForcedExternalStorage" thing here, what is the point of having storagePolicy as a concept at all?

That is, why are we bothering to store a storage policy in the db?

All of this could really benefit from some architecture documentation.

Going into this I had no idea what the history behind storagePolicy was. After debugging, I think we have to do getForcedExternalStorage here because it is the only way to reference the cluster.

Note, we are still using storage policy when creating endpoints from scratch. As in, when we have a "new Configuration". But point taken.

src-electron/db/query-impexp.js

bzbarsky-apple · 2023-09-28T16:08:19Z

src-electron/db/query-upgrade.js

+  let zcl = await queryPackage.getMetaFile(db, pkgs)
+  let obj = await fsp.readFile(zcl)
+  let data = JSON.parse(obj)
+  let externals = data.attributeAccessInterfaceAttributes


Is attributeAccessInterfaceAttributes always present in the zcl meta file?

No.

src-electron/db/query-upgrade.js

paulr34 · 2023-09-28T17:17:18Z

So this is a partial fix, in that it does not fix item 1 from #1142 (review), right?

right

src-electron/db/query-cluster.js

src-electron/db/query-impexp.js

src-electron/db/query-upgrade.js

brdandu · 2023-09-28T18:24:59Z

src-electron/db/query-upgrade.js

+  return attributeName[0].NAME
+}
+
+async function queryMetaFile(db, packageId) {


Hmm are you sure: I see
const querySelectFromPackage =
SELECT
PACKAGE_ID,
PATH,
TYPE,
CRC,
VERSION,
CATEGORY,
DESCRIPTION
FROM PACKAGE ``

src-electron/db/query-package.js

brdandu · 2023-09-28T18:32:35Z

src-electron/db/query-config.js

+  )
+  let storageOption = await queryUpgrade.computeStorage(
+    db,
+    null,


Why is this null instead of the cluster name?

because clusterName is not needed when upgrading a new configuration so I used it as a differentiator

never mind, you're right. will fix.

src-electron/db/query-impexp.js

brdandu · 2023-09-28T18:37:24Z

src-electron/db/query-upgrade.js

+) {
+  let storageOption
+  let attributeName
+  if (!clusterName) {


I believe you are passing null values to the cluster name and then extracting the cluster name regardless. See let storageOption = await queryUpgrade.computeStorage( db, null,

test/gen-matter-4.test.js

src-electron/db/query-config.js

src-electron/upgrade/upgrade.js

src-electron/db/query-impexp.js

src-electron/upgrade/upgrade.js

paulr34 requested review from tecimovic, brdandu, jingteng25742 and bzbarsky-apple September 17, 2023 16:10

bzbarsky-apple reviewed Sep 17, 2023

View reviewed changes

src-electron/db/query-config.js Outdated Show resolved Hide resolved

src-electron/db/query-config.js Outdated Show resolved Hide resolved

paulr34 marked this pull request as ready for review September 19, 2023 20:36

paulr34 force-pushed the globalAttributeHandling branch from 71f91b8 to 2a22341 Compare September 19, 2023 21:53

bzbarsky-apple requested changes Sep 20, 2023

View reviewed changes

paulr34 force-pushed the globalAttributeHandling branch from 098292d to 2a22341 Compare September 20, 2023 12:23

brdandu requested changes Sep 20, 2023

View reviewed changes

bzbarsky-apple reviewed Sep 20, 2023

View reviewed changes

bzbarsky-apple reviewed Sep 21, 2023

View reviewed changes

paulr34 force-pushed the globalAttributeHandling branch 2 times, most recently from e8b944b to 9f6b561 Compare September 26, 2023 22:49

paulr34 commented Sep 28, 2023

View reviewed changes

jingteng25742 reviewed Sep 28, 2023

View reviewed changes

bzbarsky-apple approved these changes Sep 28, 2023

View reviewed changes

paulr34 force-pushed the globalAttributeHandling branch from f01d09b to 64a5de8 Compare September 28, 2023 17:37

brdandu requested changes Sep 28, 2023

View reviewed changes

brdandu requested changes Sep 29, 2023

View reviewed changes

src-electron/db/query-config.js Show resolved Hide resolved

src-electron/upgrade/upgrade.js Show resolved Hide resolved

src-electron/db/query-impexp.js Show resolved Hide resolved

src-electron/upgrade/upgrade.js Outdated Show resolved Hide resolved

paulr34 mentioned this pull request Oct 2, 2023

zcl_attributes does not produce the correct data #1156

Closed

paulr34 force-pushed the globalAttributeHandling branch 2 times, most recently from 8cb8ed6 to 5b65d3c Compare October 2, 2023 22:00

paulr34 requested review from jingteng25742 and brdandu October 3, 2023 17:53

brdandu approved these changes Oct 4, 2023

View reviewed changes

paulr34 force-pushed the globalAttributeHandling branch from a73c55a to 879713a Compare October 4, 2023 14:23

handling global attributes per cluster

98d91d4

paulr34 force-pushed the globalAttributeHandling branch from 3905ebd to 98d91d4 Compare October 4, 2023 15:42

brdandu merged commit 331ce9d into project-chip:master Oct 4, 2023

paulr34 mentioned this pull request Oct 16, 2023

Allow global attributes to have forced storage option #543

Closed

handling global attributes per cluster #1142

handling global attributes per cluster #1142

Conversation

paulr34 commented Sep 17, 2023

paulr34 commented Sep 17, 2023 • edited Loading

paulr34 commented Sep 19, 2023

bzbarsky-apple left a comment

Choose a reason for hiding this comment

paulr34 commented Sep 20, 2023

codecov-commenter commented Sep 20, 2023 • edited Loading

Codecov Report

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

brdandu commented Sep 20, 2023

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

bzbarsky-apple left a comment

Choose a reason for hiding this comment

paulr34 commented Sep 21, 2023

paulr34 commented Sep 21, 2023 • edited Loading

paulr34 commented Sep 21, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzbarsky-apple 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

paulr34 commented Sep 28, 2023

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

paulr34 commented Sep 17, 2023 •

edited

Loading

codecov-commenter commented Sep 20, 2023 •

edited

Loading

paulr34 commented Sep 21, 2023 •

edited

Loading