connectedhomeip

Declare a "server cluster interface" that is intended to combine "attribute access interface" and "command handler interface"

#37541

Merged

I recommend adding [[nodiscard]] to this method. When writing #37078 the issues with the FabricSync tests would've been much easier to diagnose if the CHI registry required the caller to do something with the return value.

Don't forget to capitalize.

Suggest mentioning that it has internal intrusive list semantics.

I was torn here ... I want the "intrusive list" to be an implementation detail and not a stable API. For efficiency we could opt for example on https://pigweed.dev/pw_containers/maps.html#pw-intrusivemap (looks neat to get log search guaranteed, unsure about overhead).

Would you prefer a mention of "this has intrusive list" with stating that the API there is NOT stable? I tried to add those to every method, but can also split it in a separate class and have it noted that the dependency there is not stable API.

This interface prevents implementing more than 1 cluster in a given instance. This seems like it's a bit overly limiting. Since most of the methods underneath actually take paths, the cluster ID could be part of the path or of the cluster quality flags.

The reason to possibly support > 1 cluster ID is to allow more ease in making bridges (where there are a lot of cluster versions to host, or variance depending on underlying transport), or to support make tests more easily (e.g. 1 handler for several clusters in a fake)

This makes checking for uniqueness a lot clearer. My thinking was that for implemented clusters we generally need some "state" in some way (since we would not guarantee ember "get data at this path" anymore).

For bridges, you are right one could implement more generic things, however then I may need paths on other places (like qualities or accepted commands and such if they vary) and bridges are likely "large systems" so if I ask them to just register clusters as they need, they could instantiate reasonably small bridge clusters as needed (probably order of 10-20 per endpoint) and that does not sound so onerous.

My current thinking is still to YAGNI this:

• we can add a WildcardClusterId if needed in the future (e.g. make it return invalid and support it) for "any cluster" and "any endpoint"
• we have no concrete need for this right now except ease of testing and bridges and those can be built with the current interface.

This DOES NOT need to handle attributes built out of cluster metadata

Not sure what that means, or that the average developer would understand what that means. Do you mean "this does NOT handle any implicit ZAP-based data access, such as falling-back on "RAM attributes" supported by legacy ZAP-based flows" ?

Will try to rephrase. I meant this method WILL NOT ever be called with attributeID being those list attributes because we handle those inside the interaction model engine. So instead of trying to implement code to build those lists (which takes flash) the interfaces are supposed to implement the metadata calls since we need those anyway (e.g. to check permissions)

InvokeCommand for symmetry with ReadAttribute/WriteAttribute

This is already named Invoke in DataModel::Provider. Should that be renamed too?

Renamed done on both sides.

If these are internal, could they be part of a separate base class?

A bit more difficult since the pointer is Self i.e. ServerClusterInterface *.

I could make it a template base class, however not sure if that helps or hurts readability. Thoughts?

Leaning more towards splitting, because then I can document the "this is NOT a stable interface" only once and also resolve the comment about class documentation.

Yes. I think the bottom line may be that "RegisterableClusterServer" desccends from ClusterServerInterface, and the registration logic is what is hidden in the Registerable part, but the Data Model Provider onlyc alls ClusterServerInterface?

Done with the split, placed the template in Detail. A bit of casting going on, hoping it is ok.

I don't see any methods to associate the Matter stack context, that would give access to:

• EventLogging
• Attribute dirty marking
• PersistentStorageDelegate
• FabricTable
• SceneTable (if it exists)
• etc

Which is all state needed by several clusters. Is it intended that this would be done not by common code? Perhaps a few clusters don't need to log events, but I claim most would need to mark dirty.

I will have to design how the common context looks like and is initialized. it very likely it is a datamodelprovider (provides a context with event logging, eventing, dirty notification) however will see what else. My concern is that i have to balance:

• provide everything via a pointer - minimal RAM, however context will be 100% a god object that will be really hard for tests to provide
• provide a few choices/reduced amount or defer to clusters themselves (so that main can set them up) but that uses more RAM and ember has a hard time.

I am leaning towards followup to refine things.

OK, we can follow-up. This could be partially solved by adding interface methods for those common cases of marking dirty or logging an event, that would then access the right state when it becomes registered. That way the API accounts for it today.

Keeping it as followup for now, so we can have the starter PR in. It is reasonably large already.

Followup I thinkg would be:

• iterators (seems straight-forward now to have listings of clusters and attributes based on the registry)
• context design
• have some clusters converted

Would like to be able to see flash overhead as soon as possible, hence having things in sooner is better.

I think that these interfaces should be free from functionality and data members.

Requiring a single implementation of data version makes this interface less useful.

• We can't precisely reconstruct specific cluster state for test or other purposes
• It adds unnecessary coupling to the crypto library & we can't inject the entropy source
• We can't construct a "fuzzer" server cluster to try to expose bugs with data version handling.

Keeping the interface pure virtual is the best practice.

How about the IntrusiveList bit? I would generally want classes from clusters to be directly usable within registries and mocks will be hard to use if they don't have that intrusiveness.

It seems like any ember compatibility code should be able to be configured out of the build entirely, since we want to make it possible to have a legacy-free build and soon.

I made the base class pure virtual except the intrusive list bit since I could not find a good way to minimize RAM usage and keep pure virtual without intrusiveness. Created a "Standard" class that implements the interface with the implementations it had before. GetDataVersion is now pure virtual.

I think API-compatibility can be maintained if we figure out a better way to decouple the registry from interface, for now however I would rather leave that for future work as it seems splittable.

If ServerClusterInterfaceRegistry is optional ember compatibility code, could the state associated with managing these lists also be optional?

I could not figure out a good way to do this and still highly optmize for RAM.

I think if we do decide to allow for another 100 or so bytes of RAM for tiny apps, we could make this a real map to pointers (we would need endpointid + pointer, so about 8 bytes for every mapped cluster ... a tiny thread sensor with 1 endpoint should have 10 root node clusters and 3 clusters on a endpoint like "contact sensor" or "light sensor") .. so every increase is multiplied by 13 or more as cluster counts increase.

Would be interested in suggestions, although at this time I would prefer them as followups.