WriteHandler/WriteSingleClusterData architecture does not seem like the right approach

[bzbarsky-apple]

Problem

WriteHandler (implemented in #7730) seems to call a generic WriteSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader, WriteHandler * apWriteHandler); callback and expect the app to then parse the TLV and whatnot. This does not seem like the approach we want to take.

The current Ember architecture for writes is that there are callbacks before/after the attribute store is modified that allow the app to react to the change, but the SDK logic modifies the attribute store directly. This eliminates the need for the app to hunt down where the storage lives (something the SDK already knows and has to know for a lot of clusters), eliminates the need for the app to carefully do "will it fit?" checks for the storage, eliminates the need for the app to parse the values being written, eliminates the app needing to manually implement the complicated "modify this list item" semantics from the spec, etc, etc.

Proposed Solution

Implement something closer to the current Ember architecture for handling writes. We should not just be throwing the TLV for the attribute value at the app here.

@yunhanw-google @mrjerryjohns @andy31415

[mrjerryjohns]

I certainly agree that the approach of 'throwing TLV at the app' is not what we want.

However, I don't think a monolithic 'attribute database' store as is currently the case in the Ember logic is what we want either (this is an area of concern as I detailed in the Interaction Model: Deep Dive). This model is highly limiting for the types of devices we are trying to build with Matter.

The model I'd like to see us move to (and one I intend to propose soon) is where cluster servers interact with a code-generated, language-native representation of attributes (simplest version: C++ code-generated struct) into which TLV data is automatically serialized/deserialized from/to by the stack. The servers however, own the memory and representation of this object, and 'register' it with the stack.

This should address the bulk of your concerns, while still permitting a more dynamic model of storage ownership.

The current Ember architecture for writes is that there are callbacks before/after the attribute store is modified that allow the app to react to the change

This question is a key one. Do we really foresee a need to have the application (which is seemingly distinct from the cluster server logic?) to be able to observe cluster mutations and be told of that? If so, wouldn't they either be writing their own cluster server handling logic, OR design specific callbacks on a per cluster basis?

[bzbarsky-apple]

The servers however, own the memory and representation of this object, and 'register' it with the stack.

Sure. The point is the stack knows where the data should live and can just put it there, without requiring the app to manage that process.

Do we really foresee a need to have the application (which is seemingly distinct from the cluster server logic?) to be able to observe cluster mutations and be told of that?

In some cases, absolutely. Simple example: when the "onoff" attribute of the On/Off cluster changes, the app needs to do something with the light. That does not mean we want the app to manually implement all the ways that attribute can change (interactions with level control, optional on/off features like "on with timed off", etc, etc).

It might make more sense to design specific per-cluster callbacks to handle that case, though. So we might not need a generic callback except insofar as we use that to notify the cluster logic.

[mrjerryjohns]

When you say "app", you're presumably not referring to the body of server-side, cluster logic that is in the SDK today, right?

(we really should have a term for that body of logic...).

It might make more sense to design specific per-cluster callbacks to handle that case, though.

This is definitely something we should discuss.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.