[E2E][JF] Add skeleton for steps 6-7 of JCM #38749
Open
+475
−64
Conversation
This file contains hidden or 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
This avoids code duplication and the new certicate API may be used by JF. Signed-off-by: Doru Gucea <doru-cristian.gucea@nxp.com>
If this option is set during the pairing command, then JCM flow is triggered. Signed-off-by: Doru Gucea <doru-cristian.gucea@nxp.com>
…t in core-SDK Enable JF suppport by default when jf-control-app is being compiled. Signed-off-by: Doru Gucea <doru-cristian.gucea@nxp.com>
… 6-7 of JCM Signed-off-by: Doru Gucea <doru-cristian.gucea@nxp.com>
|
PR #38749: Size comparison from cff103b to a788222 Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Comment on lines
+768
to
+776
| Optional<ByteSpan> mJFAdminNOC; | ||
| Optional<uint16_t> mJFAdminNOCLen; | ||
|
|
||
| Optional<ByteSpan> mJFAdminICAC; | ||
| Optional<uint16_t> mJFAdminICACLen; | ||
|
|
||
| Optional<ByteSpan> mJFAdminRCAC; | ||
| Optional<uint16_t> mJFAdminRCACLen; |
There was a problem hiding this comment.
Use size_t instead of uint16_t everywhere for sizes, even if they are small. It's more standard and less wondering about the why and less casts.
Comment on lines
+878
to
+880
| VendorId vendorId = VendorId::Common; | ||
| FabricId fabricId = kUndefinedFabricId; |
There was a problem hiding this comment.
FabricID is not unique across roots. Did you mean fabricIndex here?
| @@ -117,6 +117,11 @@ declare_args() { | |||
| } | |||
| } | |||
|
|
|||
| declare_args() { | |||
| # Enable Joint Fabric features | |||
| chip_device_config_enable_joint_fabric = false | |||
There was a problem hiding this comment.
There are no tests for the new code. Is that intended?
a788222 to
efd914c
Compare
* check that the commissioned Admin has an Admin CAT inside its NOC. Signed-off-by: Doru Gucea <doru-cristian.gucea@nxp.com>
efd914c to
22bd237
Compare
|
PR #38749: Size comparison from 039b430 to 22bd237 Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Comment on lines
+69
to
+70
| CHIP_ERROR SaveCertificate(ByteSpan inCertSpan, uint8_t ** outCert, uint16_t * outCertSize); | ||
| void ReleaseCertificate(uint8_t ** cert, uint16_t * certSize); |
There was a problem hiding this comment.
These APIs need a bunch of documentation, since they are making some assumptions about what the caller will do with those pointers and require some specific actions on the caller to avoid memory leaks, right?
Might want to do something in terms of the naming too to make it clearer that one of these things allocates and the other releases.
But also, since this is all private to this class: is there a reason to not just use a ScopedMemoryBufferWithSize for the outparam? That will also ensure that the destructor properly deallocates, which would be good, because I am not seeing ReleaseCertificate calls in this PR to match the SaveCertificate calls it adds.
Comment on lines
+2816
to
+2817
| info.JFAdministratorFabricIndex = administratorFabricIndex.Value(); | ||
| info.JFAdminEndpointId = path.mEndpointId; |
There was a problem hiding this comment.
Can there be more than one JointFabricAdministrator cluster on the node? Or is there at most one, but we just don't know which endpoint it will be on?
| break; | ||
| } | ||
| } | ||
| return CHIP_NO_ERROR; |
There was a problem hiding this comment.
This is never checking whether we got a value but it's not well-formed TLV (in which case Next() will return false at some point and the status of the iterator will be a failure).
| ChipLogProgress(Controller, "JCM: Successfully parsed the Administrator NOC and ICAC"); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Again, this is never checking for iteration failure.
| ReturnErrorOnFailure(this->mAttributeCache->Get<TrustedRootCertificates::TypeInfo>(path, trustedCAs)); | ||
|
|
||
| auto iter = trustedCAs.begin(); | ||
| while (iter.Next()) |
There was a problem hiding this comment.
All these iterators need status checks at end of iteration.
| @@ -629,6 +701,11 @@ class CommissioningParameters | |||
| mDefaultNTP.ClearValue(); | |||
| mICDSymmetricKey.ClearValue(); | |||
| mExtraReadPaths = decltype(mExtraReadPaths)(); | |||
| #if CHIP_DEVICE_CONFIG_ENABLE_JOINT_FABRIC | |||
| mJFAdminNOC.ClearValue(); | |||
There was a problem hiding this comment.
OK, but doesn't the AutoCommissioner code that makes copies of these external buffer things then need to do that?
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.
Role of this PR is to introduce the main building blocks for executing steps 6-7 of JCM (see spec, Figure 92 and the associated text).
High-level architecture is described in Slide 4 and the associated use-case (which will be fully implemented in following PRs) is in Slides 5-7.
Associated SDK execution plan can be found in project-chip/123, SDK section.
Fix #38178, #38201:
Testing
pairing onnetwork 1 110220033 --anchor truepairing open-commissioning-window 1 1 400 1000 1261pairing onnetwork 10 17780303 --execute-jcm true