Remove duplicate state from system clusters #197
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(
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
.Exchange
object - of course - is bound to a certainMatter
instance, which is available viaExchange::matter(&self)
Matter
instance 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
Matter
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:
Dataver
- see below for that one) is removedMatter
object of the suppliedExchange
referenceread
/write
did not have a reference toExchange
(invoke
did (?)) which was an omission I addressed now'a
which was present in theread/write/invoke
methods ofAsyncHandler
and which is unnecessary (as I was extending the signatures of theHandler
/AsyncHandler
traits' methods anyway)What is NOT part of the changes
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:Dataver
. It is also related to [IM/DM] More intelligent reporting on subscriptions #166 which is not solved yetDataver
s for system cluster should probably be part of theMatter
object 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
CommissioningComplete
and not before that. Currently, we do it onAddNoc
and 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
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 theFabric
object (tracking them in theAclMgr
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 correspondingSession
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.