Initial version of Joint Fabric implementation #34637
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review changes with SemanticDiff. Analyzed 11 of 73 files. Overall, the semantic diff is 1% smaller than the GitHub diff. File Information
|
github-actions
bot
added
documentation
vijs
force-pushed
the
feature/joint_fabric
branch
from
23027eb to
feef2f1
Compare
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
axelnxp,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chapongatien,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic and
dhrishi
pullapprove
bot
requested review from
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
andy31415
previously requested changes
Jul 30, 2024
There was a problem hiding this comment.
Please split this up to something more manageable. A PR of 300K lines added and 240K removed is not reviewable in any practical sense.
I imagine a lot of this is code generated files, so please start with a PR for those (XML for data first, then maybe some commands individually)
Comment on lines
+500
to
+530
| namespace { | ||
| static constexpr size_t kFabricId = 1; | ||
|
|
||
| ExampleOperationalCredentialsIssuer gOpCredsIssuer(kFabricId); | ||
| PersistentStorage gStorage; | ||
|
|
||
| CHIP_ERROR PrepareJointFabricCluster() | ||
| { | ||
| SetPersistentStorageDelegate(&gStorage); | ||
| SetOperationalCredentialsIssuer(&gOpCredsIssuer); | ||
| SetChipToolKvs(LinuxDeviceOptions::GetInstance().chipToolKvs); | ||
| return CHIP_NO_ERROR; | ||
| } | ||
| } // namespace |
There was a problem hiding this comment.
Linux apps all share this. Having these global setters and fixed fabric ID breaks other assumptions of the Linux examples
| ExampleOperationalCredentialsIssuer gOpCredsIssuer(kFabricId); | ||
| PersistentStorage gStorage; | ||
|
|
||
| CHIP_ERROR PrepareJointFabricCluster() |
There was a problem hiding this comment.
The entire joint fabric feature should be conditional on build,and should be in a different example, not all-clusters-app. Please do like what Fabric Sync did, and make your own special samples for that whole method that doesn't bloat and complexify the already very complex all-clusters-app
| @@ -427,6 +427,7 @@ static_library("app") { | |||
| output_name = "libCHIPDataModel" | |||
|
|
|||
| sources = [ | |||
| "${chip_root}/src/controller/ExamplePersistentStorage.cpp", | |||
There was a problem hiding this comment.
This must be a separate library that is NOT a dependency of libCHIPDataModel since it's common code shared by all. Specific examples have to be split AND injectable. Not baked-in.
| @@ -473,6 +474,7 @@ static_library("app") { | |||
| "${chip_root}/src/messaging", | |||
| "${chip_root}/src/protocols/interaction_model", | |||
| "${chip_root}/src/system", | |||
| "${chip_root}/third_party/inipp", | |||
There was a problem hiding this comment.
Do not add 3rd party deps to libCHIPDataModel without justification
There was a problem hiding this comment.
inipp does not seem to make sense for embedded devices
Comment on lines
-76
to
-79
|
|
||
| [[deprecated("This constant is no longer used by common code and should be replaced by kMIN_CSR_Buffer_Size. Checks that a CSR is " | ||
| "<= kMAX_CSR_Buffer_size must be updated. This remains to keep valid buffers working from previous public API " | ||
| "usage.")]] constexpr size_t kMAX_CSR_Buffer_Size = 255; |
There was a problem hiding this comment.
You cannot change this deprecation without justification. Changing this value will break stuff.
|
@vijs I think the huge delta is due to the kotlin formatter not being run: all kt files changed and all look unformatted (quite ugly) |
vijs
force-pushed
the
feature/joint_fabric
branch
from
feef2f1 to
42f7e5d
Compare
Thank you for your suggestions. I’ve regenerated and restyled the files. Now, only 73 files have changed. |
vijs
dismissed
andy31415’s stale review
Regenerated and restyled the files. Now, only 73 files have changed.
|
PR #34637: Size comparison from b426fde to 42f7e5d Full report (27 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, mbed, nxp, qpg)
|
andy31415
requested changes
Jul 31, 2024
There was a problem hiding this comment.
Please separate out datamodel updates + codegen from implementation in separate PRs. start with xml creation/update and codegen as single stand-alone PR (without any app code).
Review-wise those are very different: on xml we just validate against spec text, for implementation we have to review logic. You can do the first PR and in parallel while it is being reviewed, you can address the comments you received in this PR and fix some compilation issues that seem to exist.
robszewczyk
previously requested changes
Aug 9, 2024
robszewczyk
dismissed
their stale review
Dismissing the CR now that the 1.4 test harness tag has been cut
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Initial version of the Joint Fabric implementation 34635
Added support for JCM (5.6.3. Joint Commissioning Method)
Fixes #34635