Issue 18505 - Darwin: synchronous API part 1: basic structure and serial read #20519
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
|
PR #20519: Size comparison from d23106c to bd0d87b Increases (3 builds for cyw30739)
Full report (16 builds for cyw30739, k32w, linux, mbed, nrfconnect, p6, telink)
|
jtung-apple
force-pushed
the
issue-18505-serial-read
branch
from
bd0d87b to
c219a45
Compare
|
PR #20519: Size comparison from f9a97b0 to c219a45 Increases (4 builds for cc13x2_26x2, esp32, nrfconnect, telink)
Decreases (2 builds for cc13x2_26x2)
Full report (41 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
jtung-apple
force-pushed
the
issue-18505-serial-read
branch
from
c219a45 to
560e7d2
Compare
|
PR #20519: Size comparison from 293ce20 to 560e7d2 Increases (1 build for cc13x2_26x2)
Decreases (4 builds for bl602, cc13x2_26x2, telink)
Full report (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
|
PR #20519: Size comparison from 293ce20 to a23af8c Increases above 0.2%:
Increases (17 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6)
Decreases (6 builds for cc13x2_26x2, telink)
Full report (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
jtung-apple
force-pushed
the
issue-18505-serial-read
branch
from
a23af8c to
6f47b10
Compare
pullapprove
bot
requested review from
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13 and
harimau-qirex
pullapprove
bot
requested review from
saurabhst,
selissia,
tecimovic,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21,
yufengwangca and
yunhanw-google
|
PR #20519: Size comparison from 8c18775 to 6f47b10 Increases (2 builds for cc13x2_26x2, telink)
Decreases (2 builds for cc13x2_26x2, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
jtung-apple
force-pushed
the
issue-18505-serial-read
branch
from
6f47b10 to
e058b6c
Compare
|
PR #20519: Size comparison from cc3e2b7 to e058b6c Increases (2 builds for cc13x2_26x2, telink)
Decreases (3 builds for cc13x2_26x2, nrfconnect)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
jtung-apple
force-pushed
the
issue-18505-serial-read
branch
from
e058b6c to
0335ad2
Compare
|
PR #20519: Size comparison from 21b1e5a to 0335ad2 Increases (2 builds for cc13x2_26x2, nrfconnect)
Decreases (1 build for cc13x2_26x2)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
jtung-apple
force-pushed
the
issue-18505-serial-read
branch
from
0335ad2 to
e6c5780
Compare
|
PR #20519: Size comparison from 21b1e5a to e6c5780 Increases (2 builds for cc13x2_26x2, nrfconnect)
Decreases (2 builds for cc13x2_26x2, esp32)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
|
PR #20519: Size comparison from 21b1e5a to 29ad017 Increases above 0.2%:
Increases (32 builds for bl602, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, telink)
Decreases (1 build for linux)
Full report (32 builds for bl602, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, telink)
|
jtung-apple
force-pushed
the
issue-18505-serial-read
branch
from
5574dfe to
4f5d44c
Compare
|
PR #20519: Size comparison from 3deee28 to 4f5d44c Decreases (2 builds for bl602, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
woody-apple
added a commit
that referenced
this pull request
Aug 3, 2022
…ial read (#20519) (#21566) Co-authored-by: Jeff Tung <[email protected]>
| // - its body is to do work with the device | ||
| // - at the end of work, call on the work item object: |
There was a problem hiding this comment.
I think we should make it clear that the readyHandler will do some work, which may involve async operations, and when, possibly asynchronously, that work completes will call either endWork or retryWork on the item it's attached to.
The important part being that the call to the ready handler may return before endWork or retryWork have been called. Or after (is that true? Or does the work handler need to guarantee async callbacks?).
There was a problem hiding this comment.
Missed this comment, and I feel that this comment description may not have explained it clearly, and let me try a bit here:
The MTRAsyncCallbackQueue was made to call one readyHandler block at a time asynchronously, and the readyHandler is expected to start/schedule a task. When the task finishes asynchronously in the future (at any time, from any queue or thread), it is expected to ask the workItem object to either endWork or retryWork.
I'll update the header to be a bit clearer.
| - (instancetype)init NS_UNAVAILABLE; | ||
| + (instancetype)new NS_UNAVAILABLE; | ||
|
|
||
| - (void)enqueueWorkItem:(MTRAsyncCallbackQueueWorkItem *)item; |
There was a problem hiding this comment.
I think we should document the concurrency behavior here clearly. In particular, that enqueueWorkItem can be called on any work queue (assuming that's true).
There was a problem hiding this comment.
Will do. In general documentation could definitely be improved - please call out places where I failed to explain.
| @property (nonatomic, strong) MTRAsyncCallbackReadyHandler readyHandler; | ||
| @property (nonatomic, strong) dispatch_block_t cancelHandler; | ||
|
|
||
| // Called by Cluster object's after async work is done |
There was a problem hiding this comment.
Suggested change
| // Called by Cluster object's after async work is done | |
| // Called by the implementation of readyHandler after async work is done |
but again, is it allowed to be called sync under the readyHandler invocation?
There was a problem hiding this comment.
Thank you for noticing this - I missed this spot when I changed this whole mechanism to take/forward a generic context.
And now that I think about it, I should probably remove MTRAsyncCallbackWorkQueue_Internal.h and move all that into the regular header, now that it's a generic class.
| for (MTRAsyncCallbackQueueWorkItem * item in invalidateItems) { | ||
| [item cancel]; | ||
| } | ||
| [invalidateItems removeAllObjects]; |
There was a problem hiding this comment.
Is this a "just in case someone somewhere else has a ref to this list"? Because I would have expected invalidateItems to be destroyed once this method completes, which would drop the refs to the objects..... Worth documenting why the explicit remove is here.
There was a problem hiding this comment.
This is just in case invalidate is called twice from two different threads at the same time, that each item is only canceled once.
There was a problem hiding this comment.
There is no circumstance that it's used this way, but I intended this class to be generic and can be used anywhere else that needs this mechanism
There was a problem hiding this comment.
But invalidateItems is set while the lock is held. If invalidate is called on two threads at once, one of them will take the lock and set invalidateItems on its stack and then set _items to nil. Then the other gets the lock and sees nil for the list, no? So they will never both get to cancel the same item....
| { | ||
| // Make a copy of params before we go async. | ||
| params = [params copy]; | ||
| [self.device connectAndPerformAsync:^(MTRBaseDevice * baseDevice) { |
There was a problem hiding this comment.
Why do we need connectAndPerformAsync here? In particular, can't we just use the bridge constructor that takes nodeId and controller, which we should already have access to via _device?
There was a problem hiding this comment.
You're right - after the BaseDevice/CallbackBridge rework, connectAndPerformAsync should be removed entirely.
| return self; | ||
| } | ||
|
|
||
| {{#chip_cluster_commands}} |
There was a problem hiding this comment.
This is pretty similar to the MTRBaseClusters version... I wonder whether we could share the code via a parametrized partial.
There was a problem hiding this comment.
Filed issue to deal with this separately #21643
| queue:(dispatch_queue_t)queue NS_DESIGNATED_INITIALIZER; | ||
|
|
||
| {{#chip_cluster_commands}} | ||
| - (void){{asLowerCamelCase name}}WithParams:(MTR{{asUpperCamelCase parent.name}}Cluster{{asUpperCamelCase name}}Params * {{#unless (commandHasRequiredField .)}}_Nullable{{/unless}})params expectedValues:(NSArray<NSDictionary<NSString *, id> *> *)expectedDataValueDictionaries expectedValueInterval:(NSNumber *)expectedValueIntervalMs completionHandler:({{>command_completion_type command=.}})completionHandler; |
There was a problem hiding this comment.
Should the expected* bits be nullable here?
There was a problem hiding this comment.
Yes as you also pointed out for the generic one in MTRDevice.h.
| {{> header}} | ||
|
|
||
| #import <Foundation/Foundation.h> | ||
| #import <os/lock.h> |
There was a problem hiding this comment.
Where do we use lock here?
There was a problem hiding this comment.
Thanks for catching this, which was just leftover from when mid-implementation.
isiu-apple
pushed a commit
to isiu-apple/connectedhomeip
that referenced
this pull request
Sep 16, 2022
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
We are looking to add a more natural synchronous API for UI development, as the preferred interface for the Darwin Framework.
Given the titular issue #18505 requires a solution that touches a lot of the files the above, we decided to put that in as part of this effort. This draft patch is the first part that includes the serial work queue needed to support an underlying async/callback model.
This is a work in progress.
Change overview
Note this is a draft patch with overall structure but lot of logic missing.
Testing