Skip to content

IDL gen improvements 2 - Convert all system clusters to use the import! macro #237

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

Merged
merged 2 commits into from
Jun 3, 2025

Conversation

ivmarkov
Copy link
Contributor

@ivmarkov ivmarkov commented May 19, 2025

NOTE: PR #233 needs to be merged first!

As mentioned in #233, this is the next PR in the sequence which converts ALL rs-matter system clusters and their handlers to use the import! macro.

As part of this conversion, I've renamed the module name of each cluster + handler to a shorter variant, because the content of each cluster module file is anyway almost completely redefined now that it uses most of the import! auto-generated types. This work had already started in the previous PR with e.g. cluster_basic_information renamed to just basic_info. E.g.:

  • admin_commissioning => adm_comm
  • nw_commissioning => net_comm
  • general_diagnostics => gen_diag
  • thread_nw_diagnostics => thread_diag
  • wifi_nw_diagnostics => wifi_diag
  • etc.

Note that I've resisted the inner urge to also rename/shorten upper-level package names. This could wait for a subsequent PR.

Also, what used to be named *Cluster is now named *Handler. We should really keep the Cluster terminology for the "meta-data" aspect of a cluster, and use Handler for the actual code that reads/writes attributes and executes commands. This is also matching the generic Handler and AsyncHandler traits names which rs-matter uses since a long time.

===

This PR also introduces the rs_matter::data_model::networks utilities, which are a simplified and more streamlined variant of what used to be in rs-matter-stack. I think these utilities do deserve to live in rs-matter proper instead, as they are not really opinionated (rs-matter-stack is an opinionated instantiation of rs-matter that decides for you how the stack should be organized and polled), and moreover, user might use none, a few or all of these utilities. But the reality is, if you plan to use rs-matter over a wireless network (Thread or Wifi) and you don't take advantage of those utilities, you are in for a lot of coding, hence another reason they should be in rs-matter.

These utilities include:

  • A trait - Networks as well as a sample implementation (WirelessNetworks) that model the storage for wireless networks
  • A hard-coded unmodifable Ethernet network storage (as per spec)
  • A Wireless network manager (WirelessMgr), which - upon commissioning a wireless operational network - can be used (and is used downstream) to monitor for wireless network disconnection and tries to re-connect the existing (and the other) networks registered in the storage
  • A NetCtl, WifiDiag, ThreadDiag and WirelessDiag traits that need to be implemented downstream and which model the (wireless) network adapter state and its controller, capable of scanning for wireless networks as well as connecting to a wireless network
  • Usable default implementations of the Network Commissioning Cluster Handler as well as the Eth / Thread / Wifi Diagnostics Cluster Handlers relying on the abstractions in the above networks module

===

PC:

  • Changes tested with rs-matter examples, and rs-matter-stack examples.

Baremetal:
These changes are in the meantime tested with rs-matter-stack and rs-matter-embassy downstream. The relevant PRs are:

Due to an unfortunate set of events (embassy-sync 0.7, bt-hci 0.3 and others), I have only tested these changes with RP Pico W and the NRF2040, and can't (yet) test with the ESP (whose BT stack is still at embassy-sync 0.6 and bt-hci 0.2).

But I don't expect any surprises there. Testing might be possible if we switch to latest main of esp-hal in rs-matter-embassy though, as I did for the RP and NRF? This would require patching openthread as well though, as it is still on esp-hal 0.23

@ivmarkov ivmarkov changed the title Idl gen improvements 2 - Convert all system clusters to use the import! macro IDL gen improvements 2 - Convert all system clusters to use the import! macro May 19, 2025
@ivmarkov ivmarkov force-pushed the idl-gen-improvements-2 branch 11 times, most recently from b503b03 to 09e0983 Compare May 20, 2025 12:46
@ivmarkov
Copy link
Contributor Author

This is now also ready for review but #233 has to go in first, as this PR is layered on top of the changes introduced by #233

@ivmarkov
Copy link
Contributor Author

@ivmarkov ivmarkov force-pushed the idl-gen-improvements-2 branch from a436562 to faf9b0a Compare June 3, 2025 05:45
@ivmarkov ivmarkov force-pushed the idl-gen-improvements-2 branch from ff91515 to 13e8426 Compare June 3, 2025 07:25
@ivmarkov
Copy link
Contributor Author

ivmarkov commented Jun 3, 2025

@bjoernQ Let me expand on something which might not be quite clear when you read the concrete changeset.

Since ~ 1y, rs-matter is using something called pinned-init. This is a crate also used in the RfL project.
The basic idea of pinned-init (forget about "pinned" in its name, it is not just about pinning, and we don't use the pinning aspect of it), so the basic idea is to allow safe, in-place initialization of large structures by avoiding temp allocation of the structure on the stack, before it is moved to its final destination which is so typical of Rust (and is a solved problem in C++).

There are multiple attempts at solving the above issue, like Google's moveit crate, and a bunch of RFCs in rust-lang which don't seem to go anywhere.

pinned-init (to be renamed to pin-init in future) is an attempt to do this at the library level, without extensions to the language, and with proc-macro for ergonomic initialization. The author has some blogs about that.

But why the need for in-place initialization in rs-matter?
Certain aspects of the Matter protocol deal with relatively large payloads. Certificates (which ultimately end up in the Fabric object) are one such example. ACLs (albeit to a smaller extent) are another.

What rs-matter strives to do in these cases is to make sure that a minimum amount of stack is used in these cases. Ideally, nothing large should be constructed on-stack. The certificates for example are read directly from the TLV (which in turn is drectly residing and read directly from the RX packet buffer) and are "emplaced" in the Fabric struct which is also never constructed on-stack (hence the need to use rs_matter::utils::storage::Vec instead of just heapless::Vec for fabrics). The latter simply does not support emplacement of its members and they are materialized on stack first, which is a big issue.

Similarly, Sessions which also tend to be a bit large are emplaced with our own version of the heapless Vec.

An alternative to the whole pinned-init approach would've been some variation of the "resources" pattern used in openthread plus some unsafe in-place initialization without the help of pinned-init as used in openthread's SRP code. Or even a separate pooled storage ala heapless::Box. But tbh I'm not convinced the resources pattern is necessarily the better approach. I used it in openthread as I did not want to introduce a dependency to pinned-init there as well, as there, the codebase is much simpler anyway. Also, a pooling approach ala heapless::Box leads to the introduction of an extra invariant lifetime in the Matter struct (in addition to its covariant 'a) which I dislike, as Matter is the object users would see the most. Not to mention that this invariant lifetime spreads down to Exchange and such... I did experiment with different approaches to Matter's internal storages quite a bit 1y ago.

@ivmarkov
Copy link
Contributor Author

ivmarkov commented Jun 3, 2025

Update: blog ^^^ The "placement by return" section is of a particular interest, as it touches on the existing RFC in rust, as well as on the moveit crate.

@bjoernQ
Copy link
Contributor

bjoernQ commented Jun 3, 2025

Thanks for the additional background! I was aware of the in-place-construction problem and your efforts to reduce stack usage but the details help a lot 👍 (also thanks for the link to that blog post)

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the change from Cluster to Handler and I generally like using abbreviations - not sure I exactly like CommPolicy - while it's probably clear in the context of Matter, Comm reads to me like communication but that's pretty much just a matter of taste.

Otherwise, all the changes look good to me. Ran a few tests on a Linux machine.

@ivmarkov
Copy link
Contributor Author

ivmarkov commented Jun 3, 2025

I like the change from Cluster to Handler and I generally like using abbreviations - not sure I exactly like CommPolicy - while it's probably clear in the context of Matter, Comm reads to me like communication but that's pretty much just a matter of taste.

Otherwise, all the changes look good to me. Ran a few tests on a Linux machine.

I'm also a bit torn on this, but the same applies to NetCommHandler / net_comm, AdminCommHandler /adm_comm.

Since I abbreviated Commissioning to Comm everywhere, I also did it for CommissioningPolicy. BTW open to any ideas for a better abbreviation, but for sure I think we should get rid of "commissioning" as it is way too mouthful.

@ivmarkov
Copy link
Contributor Author

ivmarkov commented Jun 3, 2025

Also... unfortunately we cannot abbreviate whatever names come from the IDL, but alas, that is life...

@ivmarkov ivmarkov merged commit f781ce2 into project-chip:main Jun 3, 2025
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