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

Reduce Fabrics memory consumption; provisions for UpdateNOC #208

Merged
merged 5 commits into from
Sep 25, 2024

Conversation

ivmarkov
Copy link
Contributor

(This is the last big PR in my series (forthcoming ones are much smaller), so please bear with me one more time.)

The PR builds upon the previous two (in-place-init and tlv-rework) and implements the following changes, in order of importance and magnitude:

Matter object memory reduced by ~ 16 sessions x 480B = 7KB

Reason: Session::noc_data (= Option<NocData>) is now removed. The fabric initial/updated key-pair as well as the Root CA are now stored in the Failsafe.

(In all fairness, before the project became no_std, storing the NocData inside the session was OK, because it was Option<Arc<NocData>> and as such did not occupy space until allocated. And usually, there was just one session with allocated NocData.)

This is possible because of the observation that - at any point in time - only ONE fabric can be in the process of being created (AddNOC) or updated (UpdateNOC). This is so because NOC updates do require the Failsafe to be locked first.

@kedars Would appreciate if you could confirm that my understanding ^^^ is correct.

Hence, the "in-transit" data preceding AddNOC / UpdateNOC is in the failsafe now. It is automatically invalidated when the failsafe timeout expires/it is disarmed/commissioning is complete.

AclMgr is gone

All AclEntrys belonging to a fabric are now simply kept in their corresponding Fabric object. ACL tests are updated to work with FabricMgr instead of AclMgr.

While not strictly necessary, it makes dealing with these entries a tad easier. Including their persistence to NVS. If a Fabric object is persisted, so are its ACLs as they are contained inside it.

Also, this change is symmetric to the removal of ExchangeMgr one year ago, where I merged all Exchanges into their corresponding Session object.

Failsafe reworked

It resembles the Matter C++ logic much more closely now, and I hope it would be relatively easy to add UpdateNOC now, which - while not part of this PR, is already handled in Failsafe.

Smaller items

  • Maybe had a bug where its destructor was not called. Now fixed and covered with unit tests

Copy link

semanticdiff-com bot commented Sep 17, 2024

Review changes with SemanticDiff.

Analyzed 28 of 30 files.

Overall, the semantic diff is 20% smaller than the GitHub diff.

File Information
Filename Status
rs-matter/Cargo.toml Unsupported file format
✔️ rs-matter/tests/data_model/acl_and_dataver.rs 79.85% smaller
✔️ rs-matter/tests/common/e2e.rs 51.66% smaller
✔️ rs-matter/src/acl.rs 21.64% smaller
✔️ rs-matter/src/core.rs 36.14% smaller
✔️ rs-matter/src/error.rs 9.89% smaller
✔️ rs-matter/src/fabric.rs 16.76% smaller
rs-matter/src/persist.rs Unsupported file format
✔️ rs-matter/src/utils/init.rs Analyzed
✔️ rs-matter/src/utils/maybe.rs Analyzed
✔️ rs-matter/src/utils/rand.rs 35.08% smaller
✔️ rs-matter/src/transport/core.rs 53.54% smaller
✔️ rs-matter/src/transport/exchange.rs Analyzed
✔️ rs-matter/src/transport/session.rs 19.6% smaller
✔️ rs-matter/src/tlv/read.rs 16.67% smaller
✔️ rs-matter/src/secure_channel/case.rs 52.78% smaller
✔️ rs-matter/src/secure_channel/core.rs 52.46% smaller
✔️ rs-matter/src/secure_channel/pake.rs 25.06% smaller
✔️ rs-matter/src/secure_channel/spake2p.rs 12.76% smaller
✔️ rs-matter/src/interaction_model/core.rs Analyzed
✔️ rs-matter/src/interaction_model/messages.rs Analyzed
✔️ rs-matter/src/data_model/core.rs Analyzed
✔️ rs-matter/src/data_model/system_model/access_control.rs 18.94% smaller
✔️ rs-matter/src/data_model/sdm/admin_commissioning.rs 16.67% smaller
✔️ rs-matter/src/data_model/sdm/failsafe.rs 4.67% smaller
✔️ rs-matter/src/data_model/sdm/general_commissioning.rs 49.25% smaller
✔️ rs-matter/src/data_model/sdm/noc.rs 18.42% smaller
✔️ rs-matter/src/data_model/objects/cluster.rs Analyzed
✔️ rs-matter/src/data_model/objects/node.rs Analyzed
✔️ examples/onoff_light/src/main.rs 7.26% smaller

@ivmarkov
Copy link
Contributor Author

Still in Draft as I noticed I have the tests in access_control.rs commented out. Will restore them shortly.

@ivmarkov ivmarkov marked this pull request as ready for review September 18, 2024 14:04
@ivmarkov
Copy link
Contributor Author

Ready.

@kedars
Copy link
Collaborator

kedars commented Sep 23, 2024

@kedars Would appreciate if you could confirm that my understanding ^^^ is correct.

Yes, that understanding is aligned with mine 👍

@ivmarkov ivmarkov merged commit 470a7be into project-chip:main Sep 25, 2024
12 checks passed
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