Add AsyncFacilitator to enable an async, event-driven, non-polling me… #29364
+1,119
−474
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
nivi-apple
force-pushed
the
add_support_for_async_bdx_transfer
branch
from
9ebcf9c
to
73c6d9a
Compare
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
electrocucaracha,
gjc13,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kkasperczyk-no,
kpschoedel,
ksperling-apple,
lazarkov,
lpbeliveau-silabs and
LuDuda
|
PR #29364: Size comparison from b6a1304 to db59df6 Full report (22 builds for cc13x4_26x4, cc32xx, nrfconnect, nxp, qpg, stm32, tizen)
|
nivi-apple
commented
Sep 24, 2024
| return CHIP_NO_ERROR; | ||
| } | ||
| } | ||
| return CHIP_ERROR_INCORRECT_STATE; |
There was a problem hiding this comment.
Returning CHIP_ERROR_INCORRECT_STATE. I looked at the spec and there is Unexpected Message status code that should be sent for any BDX messages but we have not yet established a session yet. the unexpected message does map to CHIP_ERROR_INCORRECT_STATE so thats what I used here.
|
PR #29364: Size comparison from b6a1304 to d344a19 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
|
PR #29364: Size comparison from 4a21a8d to 4eb2b86 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #29364: Size comparison from 4a21a8d to afb50a5 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
nivi-apple
force-pushed
the
add_support_for_async_bdx_transfer
branch
from
27e25d2
to
01f12ac
Compare
nivi-apple
commented
Oct 24, 2024
| assertChipStackLockedByCurrentThread(); | ||
|
|
||
| // TODO: #36181 - Store the imageTransferHandler in a set of MTROTAImageTransferHandler objects. | ||
| mOTAImageTransferHandler = static_cast<MTROTAImageTransferHandler *>(imageTransferHandler); |
There was a problem hiding this comment.
@bzbarsky-apple should i re-assign this here or just verify that it matches. if it doesn't clean up everything by calling destroyself?
|
PR #29364: Size comparison from 1febc9b to 4029261 Full report (3 builds for cc32xx, stm32)
|
nivi-apple
force-pushed
the
add_support_for_async_bdx_transfer
branch
from
c5a62a6
to
d3713d7
Compare
|
PR #29364: Size comparison from 1febc9b to d3713d7 Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| /** | ||
| * An abstract class with methods for receiving and sending BDX messages on an ExchangeContext. It interacts with the Transfer | ||
| * Session state machine to process the received messages and send any outgoing messages. | ||
| * Note: If the relevant fabric shuts down, it is the responsibility of the subclass that implements the HandleTransferSessionOutput |
There was a problem hiding this comment.
Suggested change
| * Note: If the relevant fabric shuts down, it is the responsibility of the subclass that implements the HandleTransferSessionOutput | |
| * Note: If the relevant fabric shuts down, it is the responsibility of the subclass that implements HandleTransferSessionOutput |
| * An abstract class with methods for receiving and sending BDX messages on an ExchangeContext. It interacts with the Transfer | ||
| * Session state machine to process the received messages and send any outgoing messages. | ||
| * Note: If the relevant fabric shuts down, it is the responsibility of the subclass that implements the HandleTransferSessionOutput | ||
| * to destroy this object and its subclasses. |
There was a problem hiding this comment.
Suggested change
| * to destroy this object and its subclasses. | |
| * to destroy itself (and hence this object). |
| CHIP_ERROR Init(System::Layer * layer, Messaging::ExchangeContext * exchangeCtx, System::Clock::Timeout timeout); | ||
|
|
||
| // If subclasses override this method and they call the superclass's OnMessageReceived, the superclass | ||
| // may destroy the subclass's object. |
There was a problem hiding this comment.
Suggested change
| // may destroy the subclass's object. | |
| // may destroy the subclass's object before OnMessageReceived returns. |
| System::PacketBufferHandle && payload) override; | ||
|
|
||
| // If subclasses override this method and they call the superclass's OnResponseTimeout, the superclass | ||
| // may destroy the subclass's object. |
There was a problem hiding this comment.
Suggested change
| // may destroy the subclass's object. | |
| // may destroy the subclass's object before OnResponseTimeout returns. |
| virtual void HandleTransferSessionOutput(TransferSession::OutputEvent & event) = 0; | ||
|
|
||
| /** | ||
| * This method should be implemented to destroy the object subclassing AsyncTransferFacilitator. |
There was a problem hiding this comment.
Suggested change
| * This method should be implemented to destroy the object subclassing AsyncTransferFacilitator. | |
| * This method should be implemented to destroy the object subclassing AsyncTransferFacilitator. | |
| * | |
| * This is a hook that is expected to be called by AsyncTransferFacilitator and allows subclasses | |
| * to select an allocation strategy of their choice. |
Comment on lines
+91
to
+92
| // If we send out a status report across the exchange, schedule a call to DestroySelf() at a little bit later time | ||
| // since we want the message to be sent before we clean up. |
There was a problem hiding this comment.
Suggested change
| // If we send out a status report across the exchange, schedule a call to DestroySelf() at a little bit later time | |
| // since we want the message to be sent before we clean up. | |
| // If we send out a status report across the exchange, that means there was an error. | |
| // We've sent our report about that error and can now abort the transfer. Our peer | |
| // will respond to the status report by tearing down their side. |
Comment on lines
+95
to
+96
| DestroySelf(); | ||
| return; |
There was a problem hiding this comment.
You could also do:
Suggested change
| DestroySelf(); | |
| return; | |
| mDestroySelf = true; | |
| break; |
and similar in the case above, just to have fewer places/ways in which this object can be destroyed....
| // the end of the transfer. | ||
| if (!msgTypeData.HasMessageType(Protocols::SecureChannel::MsgType::StatusReport)) | ||
| { | ||
| sendFlags.Set(Messaging::SendMessageFlags::kExpectResponse); |
There was a problem hiding this comment.
Followup: instead of having logic in the caller that has to match this logic in terms of which types of messages expect responses, we should have either sendFlags or just the boolean of whether we set kExpectResponse be an outparam of this method. But please don't make that change in this PR; do it separately.
Comment on lines
+184
to
+185
| // If it's a message indicating either the end of the transfer or a timeout reported by the transfer session | ||
| // or an error occured, we need to call DestroySelf(). |
There was a problem hiding this comment.
So I think what really needs to be documented here is why we need to call ProcessOutputEvents() at all in this case, instead of just destroying ourselves directly. If that's important, please make sure it's written down why it's important so someone doesn't accidentally break things later.
Comment on lines
+192
to
+193
| // Set the mDestroySelf flag to true so that when ProcessOutputEvents finishes processing all pending events, it | ||
| // will call DestroySelf() to clean up. |
There was a problem hiding this comment.
This comment would be wholly unnecessary if the flag were named clearly; see naming suggestion.
bzbarsky-apple
requested changes
Nov 2, 2024
Comment on lines
+25
to
+33
| * This class inherits from the AsyncResponder class and handles the BDX messages for a BDX transfer session. | ||
| * It overrides the HandleTransferSessionOutput virtual method and provides an implementation for it to handle | ||
| * the OutputEvents that are generated by the BDX transfer session state machine. | ||
| * | ||
| * An MTROTAImageTransferHandler will be associated with a specific BDX transfer session. | ||
| * | ||
| * The lifecycle of this class is managed by the AsyncFacilitator class which calls the virtual DestroySelf method | ||
| * that is implemented by this class to clean up and destroy itself and the AsyncFacilitator instances. | ||
| * Note: An object of this class can't be used after DestroySelf has been called. |
There was a problem hiding this comment.
Suggested change
| * This class inherits from the AsyncResponder class and handles the BDX messages for a BDX transfer session. | |
| * It overrides the HandleTransferSessionOutput virtual method and provides an implementation for it to handle | |
| * the OutputEvents that are generated by the BDX transfer session state machine. | |
| * | |
| * An MTROTAImageTransferHandler will be associated with a specific BDX transfer session. | |
| * | |
| * The lifecycle of this class is managed by the AsyncFacilitator class which calls the virtual DestroySelf method | |
| * that is implemented by this class to clean up and destroy itself and the AsyncFacilitator instances. | |
| * Note: An object of this class can't be used after DestroySelf has been called. | |
| * This class handles the BDX events for a BDX transfer session. | |
| * | |
| * This object may be destroyed directly, when not processing some call it made, to interrupt the BDX | |
| * transfer. Specifically, this has to be done if the fabric it's associated with is shut down. This | |
| * method of interrupting the BDX transfer will not notify the peer that we are interrupting and it will | |
| * keep waiting until it times out. | |
| * | |
| * If not otherwise destroyed, this object will destroy itself when the transfer completes. |
| // The node id of the node with which the BDX session is established. | ||
| chip::Optional<chip::NodeId> mNodeId; | ||
|
|
||
| // The OTA provider delegate used by the controller. |
There was a problem hiding this comment.
Suggested change
| // The OTA provider delegate used by the controller. | |
| // The OTA provider delegate that is responding to the BDX transfer request. |
| id<MTROTAProviderDelegate> mDelegate = nil; | ||
| chip::System::Layer * mSystemLayer = nil; | ||
|
|
||
| // The OTA provider delegate queue used by the controller. |
There was a problem hiding this comment.
Suggested change
| // The OTA provider delegate queue used by the controller. | |
| // The queue mDelegate must be called on. |
|
|
||
| constexpr bdx::TransferRole kBdxRole = bdx::TransferRole::kSender; | ||
|
|
||
| @interface MTROTAImageTransferHandlerWrapper : NSObject |
There was a problem hiding this comment.
Suggested change
| @interface MTROTAImageTransferHandlerWrapper : NSObject | |
| // An ARC-managed object that lets us do weak references to a MTROTAImageTransferHandler | |
| // (which is a C++ object). | |
| @interface MTROTAImageTransferHandlerWrapper : NSObject |
|
|
||
| @interface MTROTAImageTransferHandlerWrapper : NSObject | ||
| - (instancetype)initWithMTROTAImageTransferHandler:(MTROTAImageTransferHandler *)otaImageTransferHandler; | ||
| @property (nonatomic, nullable, readwrite, assign) MTROTAImageTransferHandler * otaImageTransferHandler; |
There was a problem hiding this comment.
So why is nonatomic OK here? Presumably because this property is only read/written on the Matter thread, right?
But then that should be documented. And as a followup (not part of this PR), we should have the property getter/setter assert that.
| void OnExchangeCreationFailed(chip::Messaging::ExchangeDelegate * _Nonnull delegate) override; | ||
|
|
||
| // TODO: #36181 - Have a set of MTROTAImageTransferHandler objects. | ||
| MTROTAImageTransferHandler * mOTAImageTransferHandler = nullptr; |
There was a problem hiding this comment.
Suggested change
| MTROTAImageTransferHandler * mOTAImageTransferHandler = nullptr; | |
| MTROTAImageTransferHandler * _Nullable mOTAImageTransferHandler = nullptr; |
since the header is ASSUME_NONNULL and this thing is very much not "nonnull".
Comment on lines
+54
to
+55
| mOTAImageTransferHandler->DestroySelf(); | ||
| delete mOTAImageTransferHandler; |
There was a problem hiding this comment.
This ... this is a double-delete. With some allocators, this will crash. Please do not do that. Just do the delete, take out the DestroySelf call, DestroySelf should not be public API as mentioned elsewhere.
What does need to happen and what this code is not doing: mOTAImageTransferHandler = nullptr;
| // Since the OTA provider delegate only supports one BDX transfer at a time, calling ShutDown is fine for now. | ||
| // TODO: #36181 - This class needs to keep a list of all MTROTAImageTransferHandlers mapped to the fabric index and only | ||
| // delete the MTROTAImageTransferHandler objects with a fabric index matching the MTRDeviceController's fabric index. | ||
| Shutdown(); |
There was a problem hiding this comment.
How is this possibly fine? After this we will stop listening for BDX messages entirely, no? Was this actually tested with multiple controllers? Please add to the unit tests to exercise this case.
There was a problem hiding this comment.
ah the BDX listener should not be shut down. Yeah i need to add tests with multiple controllers
| VerifyOrReturnError(mExchangeMgr != nullptr, CHIP_ERROR_INCORRECT_STATE); | ||
|
|
||
| if (GetNumberOfDelegates() >= 1) { | ||
| return CHIP_ERROR_BUSY; |
There was a problem hiding this comment.
So... this is not responding with a BUSY status on the exchange or anything. It's going to just drop everything on the floor. Is this actually the behavior we want? And if it is, why do we have the higher-level "busy" handling above that sends nice cluster response commands? This seems pretty broken to me....
There was a problem hiding this comment.
hmm let me figure out how to pipe the busy correctly. the tests pass where we test busy so either the tests don't test the right thing or something is sending the right response but cleaning up the class which it shouldn't.
| auto * otaTransferHandler = static_cast<MTROTAImageTransferHandler *>(delegate); | ||
| VerifyOrReturn(otaTransferHandler != nullptr); | ||
|
|
||
| otaTransferHandler->DestroySelf(); |
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
…chanism for facilitating BDX transfers
Currently BDX transfers can only use the TransferFacilitator which polls the BDXTransferSession with a poll interval that adds unneccessary delays and is not the ideal approach to handle BDX transfers
Add support for an AsyncTrasferFacilitator that uses a non-polling, event-driven mechanism based on BDX messages received over the exchange context and gets the next output event from the transfer session to drive the BDX transfer
Add support for an AsyncResponder that uses the AsyncTrasferFacilitator and enables a provider delegate to handle BDX transfers
Modify the darwin OTA provider code to use the AsyncResponder
Add support for handling multiple OTA requests to the ota provider by creating an MTROTAImageTransferHandler object that handles a BDX transfer session independently from a singleton unsolicited BDX message handler that is created by the MTROTAProviderDelegateBridge.
Support only one BDX transfer session and return busy to any query images coming from other nodes for now to match the test expectations (to be removed once tests for multiple OTA are in place)