(
This is a mechanical change that I had to apply laboriously everywhere, hence the large number of changed files.
But otherwise, no big semantic changes.
)
What
Currently, a lot of the system clusters contain state, in the form of various Matter objects that are borrowed upon the system cluster creation. For example:
The thing is, caching this data is completely unnecessary, because every cluster command (read/write/invoke) operates on a provided Exchange.
The Exchange object - of course - is bound to a certain Matter instance, which is available via Exchange::matter(&self)
The Matter instance in turn contains all of that data which is needlessly cached in the cluster and takes space
Moreover, I would argue that caching this data in the cluster is even incorrect! What happens if the user has created the cluster on oneMatter instance, but then erroneously uses the cluster for another Matter instance? Havoc.
(This problem by the way was fixed in the Pase / Case SC handlers with the transport rework, but I did not touch the DM clusters back then.)
Scope of changes
The changes are completely mechanical and should NOT introduce any semantic changes:
All state from all system clusters (except Dataver - see below for that one) is removed
All previously cached Matter state is now retrieved on-the-fly from the supplied Matter object of the supplied Exchange reference
Cluster methods read/write did not have a reference to Exchange (invoke did (?)) which was an omission I addressed now
I also removed the lifetime 'a which was present in the read/write/invoke methods of AsyncHandler and which is unnecessary (as I was extending the signatures of the Handler / AsyncHandler traits' methods anyway)
What is NOT part of the changes
Addressing the header issue (we have #190 for that)
Commenting old code (we have #191 for that)
Treatment of Dataver. Dataver got two very minor changes, but is otherwise unchanged, and is still part of the cluster handler state (in fact, it is the only state of most system clusters now) which I think is wrong! BUT:
I haven't figured out in my head how to handle Dataver. It is also related to #166 which is not solved yet
(Datavers for system cluster should probably be part of the Matter object itself. As these clusters are changing the state of that object, and not state they they themselves own)
Anything else we feel is pressing but might de-focus the purpose of this PR
Why now?
Our handling of commissioning completion is currently wrong. We need to register the new Fabric ONLY when we get CommissioningComplete and not before that. Currently, we do it on AddNoc and that's not OK. The C++ SDK does it properly
To fix (1) from above, without introducing even more temporary space on the program stack I need to rework a bit how fabrics are handled - in that each fabric needs to have a pending/non-commissioned flag. This would also allow me to get rid of the NocData thing which is a looping 400 bytes and is stored in each session. If I do this, ideally I need to merge the ACLs which are per-fabric really into the Fabric object (tracking them in the AclMgr is simply not convenient - for the same reasons why having separate sessions from exchanges was not convenient and now all exchanges for a session are aggregated within its corresponding Session struct)
... but then (2) means I had to introduce even more cached state to the NOC and General Commissioning handlers, hence this PR to just stop this.
( This is a mechanical change that I had to apply laboriously everywhere, hence the large number of changed files. But otherwise, no big semantic changes. )
What
Currently, a lot of the system clusters contain state, in the form of various Matter objects that are borrowed upon the system cluster creation. For example:
The culprit for me is the NOC cluster, which contains a ton of data.
Exchange.Exchangeobject - of course - is bound to a certainMatterinstance, which is available viaExchange::matter(&self)Matterinstance in turn contains all of that data which is needlessly cached in the cluster and takes spaceMoreover, I would argue that caching this data in the cluster is even incorrect! What happens if the user has created the cluster on one
Matterinstance, but then erroneously uses the cluster for another Matter instance? Havoc.(This problem by the way was fixed in the Pase / Case SC handlers with the transport rework, but I did not touch the DM clusters back then.)
Scope of changes
The changes are completely mechanical and should NOT introduce any semantic changes:
Dataver- see below for that one) is removedMatterobject of the suppliedExchangereferenceread/writedid not have a reference toExchange(invokedid (?)) which was an omission I addressed now'awhich was present in theread/write/invokemethods ofAsyncHandlerand which is unnecessary (as I was extending the signatures of theHandler/AsyncHandlertraits' methods anyway)What is NOT part of the changes
Dataver.Datavergot two very minor changes, but is otherwise unchanged, and is still part of the cluster handler state (in fact, it is the only state of most system clusters now) which I think is wrong! BUT:Dataver. It is also related to #166 which is not solved yetDatavers for system cluster should probably be part of theMatterobject itself. As these clusters are changing the state of that object, and not state they they themselves own)Why now?
Our handling of commissioning completion is currently wrong. We need to register the new Fabric ONLY when we get
CommissioningCompleteand not before that. Currently, we do it onAddNocand that's not OK. The C++ SDK does it properlyTo fix (1) from above, without introducing even more temporary space on the program stack I need to rework a bit how fabrics are handled - in that each fabric needs to have a pending/non-commissioned flag. This would also allow me to get rid of the
NocDatathing which is a looping 400 bytes and is stored in each session. If I do this, ideally I need to merge the ACLs which are per-fabric really into theFabricobject (tracking them in theAclMgris simply not convenient - for the same reasons why having separate sessions from exchanges was not convenient and now all exchanges for a session are aggregated within its correspondingSessionstruct)... but then (2) means I had to introduce even more cached state to the NOC and General Commissioning handlers, hence this PR to just stop this.