Add Darwin API for starting multiple controllers on the same fabric. #27972
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
amitnj,
andy31415,
anush-apple,
arkq,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
CodeChronos928,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
gjc13,
harsha-rajendran,
hawk248,
hicklin,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kkasperczyk-no,
ksperling-apple,
lazarkov and
lpbeliveau-silabs
pullapprove
bot
requested review from
selissia,
sharadb-amazon,
tcarmelveilleux,
tecimovic,
tima-q,
tobiasgraf,
turon,
vijs,
vivien-apple,
woody-apple,
xylophone21,
younghak-hwang,
yufengwangca and
yunhanw-google
nivi-apple
reviewed
Jul 14, 2023
woody-apple
requested changes
Jul 14, 2023
| * the rules for creating a controller on a new fabric. In particular, the | ||
| * vendor ID must not be nil. | ||
| */ | ||
| - (MTRDeviceController * _Nullable)createAdditionalControllerOnExistingFabric:(MTRDeviceControllerStartupParams *)startupParams |
There was a problem hiding this comment.
Rather than creating another new method here, I'd create a second method with the same signature, except add something before error like: "secondary:(BOOL)secondary" that does the right thing. Cascading methods like this are more normal in ObjectiveC
There was a problem hiding this comment.
Right, this comes down to what the semantics we want are. If it's a "create controller on existing fabric, optionally secondary", what you suggest makes sense and I will switch to it if we keep these semantics.
If we change to "just create a controller, without doing fabric checks" semantics-wise, then it's not really parallel to either of the two things we have (though if we had thought about it up front we could have had createControllerWithParams:fabricCheck:error: or something where fabricCheck: takes an enum describing the behavior....
There was a problem hiding this comment.
Creating a new init method with a vague name isn't great. We should keep all existing semantics with the current one, and you can add an enum for fabricCheck (better name? fabricBehavior:?) that by default the current one does what it does already.
pullapprove
bot
requested review from
andyg-apple,
aronrosenberg,
mrjerryjohns,
mkardous-silabs and
rcasallas-silabs
bzbarsky-apple
force-pushed
the
darwin-additional-controllers
branch
from
393591d to
4923b71
Compare
bzbarsky-apple
force-pushed
the
darwin-additional-controllers
branch
from
4923b71 to
0d50134
Compare
|
PR #27972: Size comparison from 0d81aa5 to 0d50134 Increases (11 builds for bl602, cc32xx, cyw30739, esp32, nrfconnect, psoc6, telink)
Decreases (12 builds for bl702, bl702l, k32w, nrfconnect, psoc6, telink)
Full report (60 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
This PR has been superseded by #28533. I've moved the tests this PR was added into that PR. |
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.
Fixes #27394
REVIEW NOTE: One thing I am not sure about is whether it's better to have this API or something like: