[Feature request] Add a way to tag default setup values for an attribute about zap HOT 17 CLOSED

@cecille #1140

from zap.

@bzbarsky-apple is this issue a duplicate of #543?

@cecille my interpretation of this is that there is code which changes the value (as seen in the link) but the global attributes are not properly changed

Or am I misinterpreting the ask?

from zap.

These issues are different as far as I can tell, unless I'm misunderstanding 543.

The problem here is that people implementing a new configuration for a device won't necessarily know what the implementation of the cluster logic looks like. Hence, they have no idea how to properly set the storage column. It defaults to RAM, which is a safe assumption given no other information, but I'd like a way to supply additional information and adjust the default if the SDK implementation does not use ember storage, so devices aren't wasting RAM space unnecessarily

I think that possibly applies to other fields as well.

from zap.

Matter already has a way to force attributes to be external, if they are always handled via AttributeAccessInterface,, by adding them to the attributeAccessInterfaceAttributes lists in src/app/zap-templates/zcl/zcl.json and src/app/zap-templates/zcl/zcl-with-test-extensions.json. What that will do is ignore what the ZAP UI says and just make the attribute external.

Of course it would be better if the ZAP UI reflected reality for such attributes by (1) saying the attribute is external and (2) making the storage field not editable in the UI.

from zap.

@cecille Note that the mechanism I describe is an override, not a hint. I agree that it might be good to have, for every UI element ZAP presents, a generic ability for an SDK to provide a hint or an override for that element.

Looking at attributes, for Matter, the editable fields seem to be the following:

1. Enabled. I think this is hinted via the XML, maybe, by making attributes mandatory or optional, with ZAP enabling the mandatory ones by default?
2. Storage. This currently has a way to override, kinda, but not hint, and only override to External.
3. Singleton. Should not exist for Matter at all; this is not a concept Matter has.
4. Bounded. I have no idea what this is, but I expect Matter does not use this.
5. Default. This can be hinted via the XML, cannot be overridden.

from zap.

Interesting. I wasn't aware of the override, but I probably should have been. It's not super obvious, but at least it exists. I definitely have reviewed zap files and made comments about incorrect storage options, which apparently didn't matter.

I'm also not sure what all the other columns are in zap. Or what the xml format schema is, for that matter. Or how they hook together.

from zap.

@bzbarsky-apple so ZAP should set storage policy to external by default and not allow the user to change the value? if so, I can do that, but I want to make sure I am understanding correctly

from zap.

sorry,

for all clusters?

from zap.

Only for attributes that have an override in the matter SDK. Basically, this list: https://github.com/project-chip/connectedhomeip/blob/master/src/app/zap-templates/zcl/zcl.json#L128. But probably not populated from that list because that's weirdly circular.

from zap.

@paulr34 Matter has a list of things for which ZAP already always treats the storage policy as External in practice (i.e. External ends up coming out of the DB if you query it). But that's not reflected in the UI by default and the UI lets you set a different policy... which is then ignored.

There is no blanked "everything is external" going on here.

from zap.

Understood, thank you. I'm debugging. Will get back to you asap.

from zap.

@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 globalAttribute list generated by the XML tag and our list of clusters/attributes pairs that are supposed to be external storage.

from zap.

right now the global attributes are tagged as "attribute" in the XML and its hard to scrape out the globals

bottom line, we have some easily fixable XML issues which I will fix and should make our lives much easier and at least fix the problem for now until some glorious redesign

will talk with the team and try and fix it tomorrow

from zap.

This issue isn't specific to global attributes though. The global attributes are actually less concerning to get right in the zap IMO because they all work the same way.

from zap.

Agree, sorry my comment was more for the other issue.

I'm aware that this issue is about how when you add an endpoint it is not aware of what cluster/attribute pairs should be forced external.

Sorry I should've commented these comments on the other ticket originally.

The issues are similar in the sense that I need to cross reference the cluster/attribute list of pairs that should be forced external.

I'll try and make progress on this issue tomorrow and get back to you.

Apologies for the confusion. You're right.

from zap.

ah, yeah - makes sense. I suppose they are related then since the globals are also handled at runtime.

from zap.

@cecille sorry Ive been busy with another unrelated issue but found time to make some progress.

So there are two issues as discussed:
1.) when opening ZAP without a .zap file, the data in ENDPOINT_TYPE represents STORAGE_OPTION as RAM for all attributes
2.) when you load a .zap file into ZAP the non global attributes have STORAGE_OPTION set correctly but the global attributes are incorrectly set

One thing that didn't make sense is why in scenario 2 the attributes would be set correctly but in scenario 1 those same attributes are always set to RAM no matter what.

I found out it is because of this https://github.com/project-chip/zap/blob/master/src-electron/db/query-config.js#L248 as you can see at the initial insert to the table "ram" is hardcoded. When the .zap file is loaded this code is "IGNORE" so that hardcoded line is never hit.

So, in order to address this specific issue, I am going to fix this hardcoded problem which will hopefully solve problem 1 and this ticket.

from zap.