Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove duplicate state from system clusters #197

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

ivmarkov
Copy link
Contributor

@ivmarkov ivmarkov commented Jun 26, 2024

(
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:

  • Failsafe
  • ACL mgr
  • Transport mgr
  • Pase mgr
  • ... and quite a few others

The culprit for me is the NOC cluster, which contains a ton of data.

  • 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 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:

  • 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

Why now?

  1. 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

  2. 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.

@ivmarkov ivmarkov merged commit 3054a32 into project-chip:main Jun 27, 2024
12 checks passed
@ivmarkov ivmarkov deleted the sys-clusters-remove-state branch September 14, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants