[darwin-framework-tool][XPC] Add XPC support #36719
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 |
woody-apple
approved these changes
Dec 4, 2024
vivien-apple
force-pushed
the
DFT_XPC2
branch
from
39e8086 to
aaed30a
Compare
|
PR #36719: Size comparison from 38ad07d to aaed30a Full report (14 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
vivien-apple
force-pushed
the
DFT_XPC2
branch
from
aaed30a to
6a77c76
Compare
|
PR #36719: Size comparison from 38ad07d to 6a77c76 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| @@ -52,6 +52,9 @@ class CHIPCommandBridge : public Command { | |||
| AddArgument("commissioner-vendor-id", 0, UINT16_MAX, &mCommissionerVendorId, | |||
| "The vendor id to use for darwin-framework-tool. If not provided, chip::VendorId::TestVendor1 (65521, 0xFFF1) will be " | |||
| "used."); | |||
| AddArgument("use-xpc", 0, 1, &mUseXPC, "This option uses the XPC implementation."); | |||
There was a problem hiding this comment.
Should document what the default behavior is.
There was a problem hiding this comment.
I have opened #36786 to make it possible to merge use-xpc and use-xpc-service-name into a single command line argument.
There was a problem hiding this comment.
Does AddArgument("use-xpc", &mUseXPC, "Use the XPC version of the controllers to connect to local endpoints. Optionally, specify a Mach service name to connect to a remote endpoint."); is easier to understand ?
There was a problem hiding this comment.
How about:
Use a controller that will connect to an XPC endpoint instead of talking to devices directly. If a string argument is provided, it must identify a Mach service name that can be used to connect to a remote endpoint. If no argument is provided, a local endpoint will be used.
?
examples/darwin-framework-tool/commands/common/CHIPCommandBridge.h
Outdated
Show resolved
Hide resolved
examples/darwin-framework-tool/commands/common/CHIPCommandBridge.mm
Outdated
Show resolved
Hide resolved
| params.vendorID = @(mCommissionerVendorId.ValueOr(chip::VendorId::TestVendor1)); | ||
| controller = [factory createControllerOnNewFabric:params error:&error]; | ||
| MTRDeviceController * controller = nil; | ||
| if (mUseXPC.ValueOr(false) || mUseXPCServiceName.HasValue()) { |
There was a problem hiding this comment.
I don't think the XPC stuff makes sense in the shared-storage situation. Why are we trying to support that? The framework doesn't really envision XPC being used with shared storage.
Or I guess this is for the "old" XPC setup?
There was a problem hiding this comment.
I included this for a few reasons:
- It may help with debugging issues related to the “old” setup.
- Since we still support the shared storage scenario, it’s more consistent to maintain feature parity.
- I already had this code locally, and I’ve structured the PR so that removing it is straightforward—just delete the file and remove two lines in CHIPCommandBridge. This makes it easy to remove later if needed.
examples/darwin-framework-tool/commands/common/xpc/DeviceControllerServer.mm
Outdated
Show resolved
Hide resolved
| { | ||
| for (MTRDeviceController * controller in _controllers) { | ||
| if ([controller.uniqueIdentifier isEqual:uniqueIdentifier]) { | ||
| return [MTRDevice deviceWithNodeID:nodeID controller:controller]; |
There was a problem hiding this comment.
Followup: we will want to have these devices held on to, and set delegates on them so they actually get some state in them (like values of attributes), right?
vivien-apple
force-pushed
the
DFT_XPC2
branch
from
6a77c76 to
bbb6a42
Compare
|
PR #36719: Size comparison from 9c8a552 to bbb6a42 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Dec 13, 2024
There was a problem hiding this comment.
@vivien-apple please add a Testing section to this to describe how this was tested (and if manual provide sufficient details and justification why automated testing of the new code is impossible)
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
This PR adds XPC supports to DFT.