Add AsyncFacilitator to enable an async, event-driven, non-polling me… #29364
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
github-actions
bot
added
scripts
integrations
transport
platform
lib
inet
controller
app
github
tests
support
tizen
nivi-apple
force-pushed
the
add_support_for_async_bdx_transfer
branch
2 times, most recently
from
8e8d62f
to
277f287
Compare
…vent driven approach rather than polling - Update the OTA provider darwin implementation to use the AsyncTransferFacilitator to transfer the OTA file using BDX
nivi-apple
force-pushed
the
add_support_for_async_bdx_transfer
branch
from
277f287
to
db59df6
Compare
|
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)
|
Comment on lines
+35
to
+36
| * subclass must call the NotifyEventHandled to notify the AsyncTransferFacilitator that the event has been handled and returns an | ||
| * error code for error cases or success. |
There was a problem hiding this comment.
This isn't very clear to me. If the subclass returns error, does it still need to call NotifyEventHandled? What does "returns error" mean? HandleTransferSessionOutput returns void.
In general, it seems like all the documentation related to NotifyEventHandled should live on the interface that defines that method, which is not this one?
There was a problem hiding this comment.
I will try to clarify a bit more and move the details to the interface
| System::Clock::Timeout timeout); | ||
|
|
||
| /** | ||
| * This is called by the subclass implementing HandleTransferSessionOutput to notify the AsyncTransferFacilitator |
There was a problem hiding this comment.
No, it's notifying the AsyncResponder.
| /** | ||
| * This is called by the subclass implementing HandleTransferSessionOutput to notify the AsyncTransferFacilitator | ||
| * that it has handled the OutputEvent specified in event and returns an error code (if any) or success in the error paramter. | ||
| * Once this is called the AsyncTransferFacilitator either aborts the transfer if an error has ocurred or drives the |
There was a problem hiding this comment.
Again, this is the AsyncResponder, not the AsyncTransferFacilitator.
Does this need to be documented in the API? Does the API consumer (subclass) need to know what happens when it calls this function?
There was a problem hiding this comment.
no. i will remove additional details
There was a problem hiding this comment.
will fix the AsyncTransferFacilitator-> AsyncResponder
| * @param[in] event The OutputEvent that was handled by the subclass. | ||
| * @param[in] error The error code that occured when handling the event if an error occurs. Otherwise has CHIP_NO_ERROR. | ||
| */ | ||
| void NotifyEventHandled(TransferSession::OutputEvent & event, CHIP_ERROR error); |
There was a problem hiding this comment.
Does the callee here modify event? If not, shouldn't this be a const ref?
Co-authored-by: Boris Zbarsky <[email protected]>
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.
…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)