Adding darwin callbacks for NOC Generation #21705
Merged
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
pullapprove
bot
requested review from
andy31415,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kpschoedel,
lazarkov,
LuDuda,
mlepage-google,
mrjerryjohns,
msandstedt,
rgoliver and
saurabhst
sharadb-amazon
force-pushed
the
ios-noc-20220807
branch
3 times, most recently
from
8af35db
to
5c16e52
Compare
|
PR #21705: Size comparison from c037fc2 to 5c16e52 Increases (24 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, nrfconnect, p6)
Decreases (8 builds for cc13x2_26x2, esp32, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
sharadb-amazon
force-pushed
the
ios-noc-20220807
branch
from
5c16e52
to
a33bbc1
Compare
|
PR #21705: Size comparison from b1f2fc7 to a33bbc1 Increases (24 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, nrfconnect, psoc6)
Decreases (8 builds for cc13x2_26x2, esp32, psoc6, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
sharadb-amazon
force-pushed
the
ios-noc-20220807
branch
from
672ca85
to
08c750f
Compare
|
PR #21705: Size comparison from d502f30 to 08c750f Increases (26 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, nrfconnect, psoc6)
Decreases (9 builds for cc13x2_26x2, esp32, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
sharadb-amazon
force-pushed
the
ios-noc-20220807
branch
from
08c750f
to
4fdd154
Compare
|
PR #21705: Size comparison from 2a5f7fe to 4fdd154 Increases (27 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, nrfconnect, psoc6, telink)
Decreases (10 builds for cc13x2_26x2, esp32, psoc6, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
sharadb-amazon
force-pushed
the
ios-noc-20220807
branch
from
4fdd154
to
b137409
Compare
|
PR #21705: Size comparison from 1f83af0 to b137409 Increases (25 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, nrfconnect, psoc6)
Decreases (9 builds for cc13x2_26x2, esp32, psoc6, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
sharadb-amazon
force-pushed
the
ios-noc-20220807
branch
from
b137409
to
79e5546
Compare
|
PR #21705: Size comparison from dfc1a70 to 45f112b Increases (30 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, nrfconnect, psoc6, telink)
Decreases (9 builds for cc13x2_26x2, esp32, psoc6)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
hawk248
approved these changes
Aug 20, 2022
Comment on lines
+38
to
+39
| * If ipk and adminSubject are passed, then they will be used in | ||
| * the AddNOC command sent to the commissionee. If they are not passed, then the values |
There was a problem hiding this comment.
This API says they are required to be non-nil, so must always be passed. Are they meant to be nullable?
| __block chip::Optional<chip::Controller::CommissioningParameters> commissioningParameters; | ||
| // Dereferencing mCppCommissioner as it would be set to point to a valid Cpp commissioner by now, as we are in the middle of | ||
| // commissioning | ||
| dispatch_sync(mChipWorkQueue, ^{ |
There was a problem hiding this comment.
Isn't this already running on the chip work queue? In which case, won't this deadlock?
| certificationDeclaration:AsData(certificationDeclarationSpan) | ||
| firmwareInfo:AsData(firmwareInfoSpan)]; | ||
|
|
||
| dispatch_sync(mNocChainIssuerQueue, ^{ |
There was a problem hiding this comment.
Why is this a sync dispatch?
Comment on lines
+235
to
+236
| // Dereferencing mCppCommissioner as it would be set to point to a valid Cpp commissioner by now, as we are in the middle of | ||
| // commissioning |
There was a problem hiding this comment.
I'm not sure this true. We're coming here from some async callback from our API consumer. The commissioner could have been shut down by now, and then this is a use-after-free.
isiu-apple
pushed a commit
to isiu-apple/connectedhomeip
that referenced
this pull request
Sep 16, 2022
* Adding darwin callbacks for NOC Generation * Fixing compiler errors in new dependencies in src/credentials * Addressing feedback from woody-apple@ and bzbarsky-apple@ * Addressing second round of feedback * Address feedback around returning errors and adding comments * Adding comments Co-authored-by: Justin Wood <woody@apple.com>
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.
Problem
Change overview
This change allows for custom credentials issuer implementations, for example, when a proprietary cloud API will perform the CSR signing.
See similar change made for android previously: #21522 and #21648