Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow global attributes to have forced storage option #543

Closed
bzbarsky-apple opened this issue Jun 6, 2022 · 5 comments
Closed

Allow global attributes to have forced storage option #543

bzbarsky-apple opened this issue Jun 6, 2022 · 5 comments
Assignees
Labels
high priority Tag that shows this as higher priority as other issues. matter Important to Matter SDK

Comments

@bzbarsky-apple
Copy link
Contributor

We have code in src-electron/zcl/zcl-loader-silabs.js that sets storage policy like so:

      let storagePolicy = dbEnum.storagePolicy.any
      if (context.listsUseAttributeAccessInterface && attribute.$.entryType) {
        storagePolicy = dbEnum.storagePolicy.attributeAccessInterface
      } else if (
        context.attributeAccessInterfaceAttributes &&
        context.attributeAccessInterfaceAttributes[cluster.name] &&
        context.attributeAccessInterfaceAttributes[cluster.name].includes(name)
      ) {
        storagePolicy = dbEnum.storagePolicy.attributeAccessInterface
      }

but that code does not run for global attributes, as far as I can tell. Then later when we try to use the storage policy in src-electron/db/query-impexp.js in importAttributeForEndpointType I am not sure that code runs for global attributes either.

So in practice the storage policy bits don't work for global attributes.

@bzbarsky-apple bzbarsky-apple added the matter Important to Matter SDK label Jun 6, 2022
@tecimovic tecimovic changed the title attributeAccessInterfaceAttributes and storagePolicy does not work for global attributes Allow global attributes to have forced storage option Jun 6, 2022
@tecimovic
Copy link
Collaborator

There is currently a mechanism that forced storage option to be external for certain attributes, via this block inside the zcl.json file:

  "listsUseAttributeAccessInterface": true,
  "attributeAccessInterfaceAttributes": {
    "Access Control": [
      "ClusterRevision",
      "SubjectsPerAccessControlEntry",
      "TargetsPerAccessControlEntry",
      "AccessControlEntriesPerFabric"
    ],
    "AdministratorCommissioning": [
      "WindowStatus",
      "AdminFabricIndex",
      "AdminVendorId"
    ],
    "Basic": [
      "DataModelRevision",
      "VendorName",
      "VendorID",
      "ProductName",
      "ProductID",
      "Location",
      "HardwareVersion",
    ...

If you put a global attribute into this block, however, this doesn't work per cluster. There isn't also any place in the database to put this data per-cluster for a global attribute. We should add this to GLOBAL_ATTRIBUTE_DEFAULT (possibly renaming the table to GLOBAL_ATTRIBUTE_PER_CLUSTER_POLICY).

@tecimovic
Copy link
Collaborator

tecimovic commented Jun 6, 2022

So what I plan to do, and @bzbarsky-apple , please review:

1.) Rename GLOBAL_ATTRIBUTE_DEFAULT to GLOBAL_ATTRIBUTE_CLUSTER_POLICY.
2.) Add in there a column STORAGE_POLICY.
3.) Concoct a mechanism that will allow for that thing in zcl.json to also refer to global attributes, same way as it refers to non-global ones.
4.) Properly populate this table with per-cluster STORAGE_POLICY.
5.) Fix the things that read this data to make use of this (mostly the thing that creates endpoints, which currently already takes the defaults from this table, but now it should also take storage option from it).

Possibly: why is this called "attributeAccessInterface"?

Why not just call it "forcedExternalStorageOption" ?

@bzbarsky-apple
Copy link
Contributor Author

@tecimovic That plan sounds good, I think, if I understand how these things fit together.

Possibly: why is this called "attributeAccessInterface"?

On the Matter side: because that's why we're forcing these things to be external-storage, and because that's what the storage-policy enup in ZAP got named. But I don't have any particular attachment to the naming here.

@paulr34
Copy link
Collaborator

paulr34 commented Sep 12, 2023

sorry @bzbarsky-apple copying my comment here #1110 so @tecimovic sees it

see: #1110

@bzbarsky-apple I think I figured it out. The global attributes are labeled as "attribute"within general.xml and they should actually be labeled as "globalAttribute".

Maybe there is a better solution but at the moment the code treats global attributes like non global attributes and it doesn't work because the cluster is undefined. The only way to match a referenced cluster is to run a "forcedExternalCheck()" or something like that on each cluster which cross references the globalAttribute list generated by the XML tag and our list of clusters/attributes pairs that are supposed to be external storage.

@paulr34
Copy link
Collaborator

paulr34 commented Oct 16, 2023

completed #1142 will make new issues for follow up work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority Tag that shows this as higher priority as other issues. matter Important to Matter SDK
Projects
Development

No branches or pull requests

4 participants