From 99fb25952502ef20a51c59c715613bc4e190968b Mon Sep 17 00:00:00 2001 From: Nivedita Sarkar Date: Wed, 27 Sep 2023 17:41:02 -0700 Subject: [PATCH] Addressed review comments --- .../CHIP/MTROTAImageTransferHandler.h | 28 +-- .../CHIP/MTROTAImageTransferHandler.mm | 140 +++------------ .../CHIP/MTROTAUnsolicitedBDXMessageHandler.h | 14 +- .../MTROTAUnsolicitedBDXMessageHandler.mm | 8 +- .../Framework/CHIPTests/MTROTAProviderTests.m | 22 +-- .../bdx/AsyncTransferFacilitator.cpp | 169 ++++++++++++------ src/protocols/bdx/AsyncTransferFacilitator.h | 43 +++-- 7 files changed, 203 insertions(+), 221 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTROTAImageTransferHandler.h b/src/darwin/Framework/CHIP/MTROTAImageTransferHandler.h index ee06858553accf..2b895531ce26f3 100644 --- a/src/darwin/Framework/CHIP/MTROTAImageTransferHandler.h +++ b/src/darwin/Framework/CHIP/MTROTAImageTransferHandler.h @@ -23,9 +23,14 @@ NS_ASSUME_NONNULL_BEGIN /** * This class inherits from the AsyncResponder class and handles the BDX messages for a BDX transfer session. - * It overrides the HandleAsyncTransferSessionOutput virtual method and provides an implementation for it. + * 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. * - * For each BDX transfer, we will have an instance of MTROTAImageTransferHandler. + * 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 CleanUp 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 CleanUp has been called. */ class MTROTAImageTransferHandler : public chip::bdx::AsyncResponder { @@ -33,18 +38,19 @@ class MTROTAImageTransferHandler : public chip::bdx::AsyncResponder MTROTAImageTransferHandler(); ~MTROTAImageTransferHandler(); - void HandleAsyncTransferSessionOutput(chip::bdx::TransferSession::OutputEvent & event) override; + void HandleTransferSessionOutput(chip::bdx::TransferSession::OutputEvent & event) override; + void CleanUp() override; + +protected: + CHIP_ERROR OnMessageReceived(chip::Messaging::ExchangeContext * ec, const chip::PayloadHeader & payloadHeader, + chip::System::PacketBufferHandle && payload) override; private: CHIP_ERROR PrepareForTransfer(chip::Messaging::ExchangeContext * exchangeCtx, chip::FabricIndex fabricIndex, chip::NodeId nodeId); - void ResetState(); - CHIP_ERROR ConfigureState(chip::FabricIndex fabricIndex, chip::NodeId nodeId); - static void HandleBdxInitReceivedTimeoutExpired(chip::System::Layer * systemLayer, void * state); - CHIP_ERROR OnMessageToSend(chip::bdx::TransferSession::OutputEvent & event); CHIP_ERROR OnTransferSessionBegin(chip::bdx::TransferSession::OutputEvent & event); @@ -53,14 +59,10 @@ class MTROTAImageTransferHandler : public chip::bdx::AsyncResponder CHIP_ERROR OnBlockQuery(chip::bdx::TransferSession::OutputEvent & event); - // Inherited from ExchangeContext - CHIP_ERROR OnMessageReceived(chip::Messaging::ExchangeContext * ec, const chip::PayloadHeader & payloadHeader, - chip::System::PacketBufferHandle && payload) override; - - // The fabric index of the peer node. + // The fabric index of the node with which the BDX session is established. chip::Optional mFabricIndex; - // The node id of the peer node. + // The node id of the node with which the BDX session is established. chip::Optional mNodeId; // The OTA provider delegate used by the controller. diff --git a/src/darwin/Framework/CHIP/MTROTAImageTransferHandler.mm b/src/darwin/Framework/CHIP/MTROTAImageTransferHandler.mm index 1b939247b67d5b..53d2b9311aeee5 100644 --- a/src/darwin/Framework/CHIP/MTROTAImageTransferHandler.mm +++ b/src/darwin/Framework/CHIP/MTROTAImageTransferHandler.mm @@ -30,16 +30,13 @@ // TODO Expose a method onto the delegate to make that configurable. constexpr uint32_t kMaxBdxBlockSize = 1024; -// Since the BDX timeout is 5 minutes and we are starting this after query image is available and before the BDX init comes, -// we just double the timeout to give enough time for the BDX init to come in a reasonable amount of time. -constexpr System::Clock::Timeout kBdxInitReceivedTimeout = System::Clock::Seconds16(10 * 60); - -constexpr System::Clock::Timeout kBdxTimeout = System::Clock::Seconds16(5 * 60); // OTA Spec mandates >= 5 minutes +// Timeout for the BDX transfer session. The OTA Spec mandates this should be >= 5 minutes. +constexpr System::Clock::Timeout kBdxTimeout = System::Clock::Seconds16(5 * 60); constexpr bdx::TransferRole kBdxRole = bdx::TransferRole::kSender; MTROTAImageTransferHandler::MTROTAImageTransferHandler() { - // Increment the number of delegates by 1. + // Increment the number of delegates handling BDX by 1. MTROTAUnsolicitedBDXMessageHandler::IncrementNumberOfDelegates(); } @@ -55,77 +52,20 @@ return AsyncResponder::PrepareForTransfer(exchangeCtx, kBdxRole, flags, kMaxBdxBlockSize, kBdxTimeout); } -MTROTAImageTransferHandler::~MTROTAImageTransferHandler() { ResetState(); } - -void MTROTAImageTransferHandler::ResetState() +MTROTAImageTransferHandler::~MTROTAImageTransferHandler() { - assertChipStackLockedByCurrentThread(); - if (mNodeId.HasValue() && mFabricIndex.HasValue()) { - ChipLogProgress(Controller, - "Resetting state for OTA Provider; no longer providing an update for node id 0x" ChipLogFormatX64 ", fabric index %u", - ChipLogValueX64(mNodeId.Value()), mFabricIndex.Value()); - } else { - ChipLogProgress(Controller, "Resetting state for OTA Provider"); - } - chip::DeviceLayer::SystemLayer().CancelTimer(HandleBdxInitReceivedTimeoutExpired, this); - - AsyncResponder::ResetTransfer(); mFabricIndex.ClearValue(); mNodeId.ClearValue(); mDelegate = nil; mDelegateNotificationQueue = nil; - MTROTAUnsolicitedBDXMessageHandler::DecrementNumberOfDelegates(); -} -/** - * Timer callback called when we don't receive a BDX init within a reasonable time after a successful QueryImage response. - */ -void MTROTAImageTransferHandler::HandleBdxInitReceivedTimeoutExpired(chip::System::Layer * systemLayer, void * state) -{ - VerifyOrReturn(state != nullptr); - static_cast(state)->ResetState(); -} -CHIP_ERROR MTROTAImageTransferHandler::OnMessageToSend(TransferSession::OutputEvent & event) -{ - assertChipStackLockedByCurrentThread(); - - VerifyOrReturnError(AsyncTransferFacilitator::GetExchangeContext() != nullptr, CHIP_ERROR_INCORRECT_STATE); - VerifyOrReturnError(mDelegate != nil, CHIP_ERROR_INCORRECT_STATE); - - Messaging::SendFlags sendFlags; - - // All messages sent from the Sender expect a response, except for a StatusReport which would indicate an error and - // the end of the transfer. - if (!event.msgTypeData.HasMessageType(Protocols::SecureChannel::MsgType::StatusReport)) { - sendFlags.Set(Messaging::SendMessageFlags::kExpectResponse); - } - - auto & msgTypeData = event.msgTypeData; - ChipLogError(BDX, "OnMessageToSend msgTypeData.MessageType = %hu", msgTypeData.MessageType); - Messaging::ExchangeContext * ec = AsyncTransferFacilitator::GetExchangeContext(); - VerifyOrReturnError(ec != nullptr, CHIP_ERROR_INCORRECT_STATE); - - CHIP_ERROR err = CHIP_NO_ERROR; - if (ec != nullptr) { - // If there's an error sending the message, call ResetState. - err = ec->SendMessage(msgTypeData.ProtocolId, msgTypeData.MessageType, std::move(event.MsgData), sendFlags); - if (err != CHIP_NO_ERROR) { - ResetState(); - } else if (event.msgTypeData.HasMessageType(Protocols::SecureChannel::MsgType::StatusReport)) { - // If the send was successful for a status report, since we are not expecting a response the exchange context is - // already closed. We need to null out the reference to avoid having a dangling pointer. - ec = nullptr; - ResetState(); - } - } - return err; + // Decrement the number of delegates handling BDX by 1. + MTROTAUnsolicitedBDXMessageHandler::DecrementNumberOfDelegates(); } CHIP_ERROR MTROTAImageTransferHandler::OnTransferSessionBegin(TransferSession::OutputEvent & event) { assertChipStackLockedByCurrentThread(); - // Once we receive the BDX init, cancel the BDX Init timeout and start the BDX session - chip::DeviceLayer::SystemLayer().CancelTimer(HandleBdxInitReceivedTimeoutExpired, this); VerifyOrReturnError(mFabricIndex.HasValue(), CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mNodeId.HasValue(), CHIP_ERROR_INCORRECT_STATE); @@ -152,8 +92,7 @@ if (error != nil) { CHIP_ERROR err = [MTRError errorToCHIPErrorCode:error]; LogErrorOnFailure(err); - ResetState(); - AsyncResponder::NotifyEventHandledWithError(err); + AsyncResponder::NotifyEventHandled(err); return; } @@ -166,9 +105,9 @@ acceptData.StartOffset = mTransfer.GetStartOffset(); acceptData.Length = mTransfer.GetTransferLength(); - LogErrorOnFailure(mTransfer.AcceptTransfer(acceptData)); - AsyncResponder::NotifyEventHandledWithError([MTRError errorToCHIPErrorCode:error]); - return; + CHIP_ERROR err = mTransfer.AcceptTransfer(acceptData); + LogErrorOnFailure(err); + AsyncResponder::NotifyEventHandled(err); } errorHandler:^(NSError *) { // Not much we can do here @@ -181,7 +120,7 @@ auto delagateQueue = mDelegateNotificationQueue; if (strongDelegate == nil || delagateQueue == nil) { LogErrorOnFailure(CHIP_ERROR_INCORRECT_STATE); - AsyncResponder::NotifyEventHandledWithError(CHIP_ERROR_INCORRECT_STATE); + AsyncResponder::NotifyEventHandled(CHIP_ERROR_INCORRECT_STATE); return CHIP_ERROR_INCORRECT_STATE; } @@ -201,7 +140,6 @@ completionHandler:completionHandler]; } }); - return CHIP_NO_ERROR; } @@ -228,7 +166,7 @@ auto delagateQueue = mDelegateNotificationQueue; if (strongDelegate == nil || delagateQueue == nil) { LogErrorOnFailure(CHIP_ERROR_INCORRECT_STATE); - AsyncResponder::NotifyEventHandledWithError(CHIP_ERROR_INCORRECT_STATE); + AsyncResponder::NotifyEventHandled(CHIP_ERROR_INCORRECT_STATE); return CHIP_ERROR_INCORRECT_STATE; } @@ -239,8 +177,6 @@ error:[MTRError errorForCHIPErrorCode:error]]; }); } - - ResetState(); return CHIP_NO_ERROR; } @@ -268,7 +204,7 @@ assertChipStackLockedByCurrentThread(); if (data == nil) { - AsyncResponder::NotifyEventHandledWithError(CHIP_ERROR_INCORRECT_STATE); + AsyncResponder::NotifyEventHandled(CHIP_ERROR_INCORRECT_STATE); return; } @@ -278,10 +214,8 @@ blockData.IsEof = isEOF; CHIP_ERROR err = mTransfer.PrepareBlock(blockData); - if (CHIP_NO_ERROR != err) { - LogErrorOnFailure(err); - } - AsyncResponder::NotifyEventHandledWithError(err); + LogErrorOnFailure(err); + AsyncResponder::NotifyEventHandled(err); } errorHandler:^(NSError *) { // Not much we can do here @@ -296,7 +230,7 @@ auto delagateQueue = mDelegateNotificationQueue; if (strongDelegate == nil || delagateQueue == nil) { LogErrorOnFailure(CHIP_ERROR_INCORRECT_STATE); - AsyncResponder::NotifyEventHandledWithError(CHIP_ERROR_INCORRECT_STATE); + AsyncResponder::NotifyEventHandled(CHIP_ERROR_INCORRECT_STATE); return CHIP_ERROR_INCORRECT_STATE; } @@ -318,11 +252,10 @@ completionHandler:completionHandler]; } }); - return CHIP_NO_ERROR; } -void MTROTAImageTransferHandler::HandleAsyncTransferSessionOutput(TransferSession::OutputEvent & event) +void MTROTAImageTransferHandler::HandleTransferSessionOutput(TransferSession::OutputEvent & event) { VerifyOrReturn(mDelegate != nil); @@ -332,8 +265,7 @@ err = OnTransferSessionBegin(event); if (err != CHIP_NO_ERROR) { LogErrorOnFailure(err); - AsyncResponder::NotifyEventHandledWithError(err); - ResetState(); + AsyncResponder::NotifyEventHandled(err); } break; case TransferSession::OutputEventType::kStatusReceived: @@ -343,27 +275,13 @@ case TransferSession::OutputEventType::kInternalError: case TransferSession::OutputEventType::kTransferTimeout: err = OnTransferSessionEnd(event); - if (err != CHIP_NO_ERROR) { - LogErrorOnFailure(err); - AsyncResponder::NotifyEventHandledWithError(err); - ResetState(); - } break; case TransferSession::OutputEventType::kQueryWithSkipReceived: case TransferSession::OutputEventType::kQueryReceived: err = OnBlockQuery(event); if (err != CHIP_NO_ERROR) { LogErrorOnFailure(err); - AsyncResponder::NotifyEventHandledWithError(err); - ResetState(); - } - break; - case TransferSession::OutputEventType::kMsgToSend: - err = OnMessageToSend(event); - if (err != CHIP_NO_ERROR) { - LogErrorOnFailure(err); - AsyncResponder::NotifyEventHandledWithError(err); - ResetState(); + AsyncResponder::NotifyEventHandled(err); } break; case TransferSession::OutputEventType::kNone: @@ -377,7 +295,13 @@ chipDie(); break; } - LogErrorOnFailure(err); +} + +void MTROTAImageTransferHandler::CleanUp() +{ + assertChipStackLockedByCurrentThread(); + + delete this; } CHIP_ERROR MTROTAImageTransferHandler::ConfigureState(chip::FabricIndex fabricIndex, chip::NodeId nodeId) @@ -394,13 +318,6 @@ VerifyOrReturnError(mDelegate != nil, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mDelegateNotificationQueue != nil, CHIP_ERROR_INCORRECT_STATE); - // Start a timer to track whether we receive a BDX init after a successful query image in a reasonable amount of time - CHIP_ERROR err - = chip::DeviceLayer::SystemLayer().StartTimer(kBdxInitReceivedTimeout, HandleBdxInitReceivedTimeoutExpired, this); - LogErrorOnFailure(err); - - ReturnErrorOnFailure(err); - mFabricIndex.SetValue(fabricIndex); mNodeId.SetValue(nodeId); @@ -408,11 +325,11 @@ } CHIP_ERROR MTROTAImageTransferHandler::OnMessageReceived( - chip::Messaging::ExchangeContext * ec, const chip::PayloadHeader & payloadHeader, chip::System::PacketBufferHandle && payload) + Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader, System::PacketBufferHandle && payload) { VerifyOrReturnError(ec != nullptr, CHIP_ERROR_INCORRECT_STATE); CHIP_ERROR err; - ChipLogProgress(BDX, "%s: message " ChipLogFormatMessageType " protocol " ChipLogFormatProtocolId, __FUNCTION__, + ChipLogProgress(BDX, "OnMessageReceived: message " ChipLogFormatMessageType " protocol " ChipLogFormatProtocolId, payloadHeader.GetMessageType(), ChipLogValueProtocolId(payloadHeader.GetProtocolID())); // If we receive a ReceiveInit message, then we prepare for transfer @@ -424,6 +341,7 @@ err = PrepareForTransfer(ec, fabricIndex, nodeId); if (err != CHIP_NO_ERROR) { ChipLogError(Controller, "Failed to prepare for transfer for BDX"); + return err; } } } diff --git a/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.h b/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.h index 0965c33b0bcbdc..244739d95408fa 100644 --- a/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.h +++ b/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.h @@ -22,11 +22,11 @@ NS_ASSUME_NONNULL_BEGIN /** - * This class creates a handler for listening to all unsolicited BDX messages and when a BDX ReceiveInit - * message is received from a node, it creates a new MTROTAImageTransferHandler object as a delegate - * that handles the OTA image file transfer for that node. + * This class creates an unsolicited handler for listening to all unsolicited BDX messages + * and when it receives a BDX ReceiveInit message from a node, it creates a new + * MTROTAImageTransferHandler object as a delegate that will prepare for transfer and + * handle all BDX messages for the BDX transfer session with that node. * - * Each MTROTAImageTransferHandler instance will handle one BDX transfer session. */ class MTROTAUnsolicitedBDXMessageHandler : public chip::Messaging::UnsolicitedMessageHandler { @@ -39,10 +39,10 @@ class MTROTAUnsolicitedBDXMessageHandler : public chip::Messaging::UnsolicitedMe // Returns the number of delegates that are currently handling BDX transfers static uint8_t GetNumberOfDelegates(); - // Increase the number of delegates by 1. + // Increase the number of delegates handling BDX transfers by 1. static void IncrementNumberOfDelegates(); - // Decrease the number of delegates by 1. + // Decrease the number of delegates handling BDX transfers by 1. static void DecrementNumberOfDelegates(); private: @@ -52,8 +52,6 @@ class MTROTAUnsolicitedBDXMessageHandler : public chip::Messaging::UnsolicitedMe protected: chip::Messaging::ExchangeManager * mExchangeMgr; - MTROTAImageTransferHandler * otaImageTransferHandler; - static inline uint8_t mNumberOfDelegates = 0; }; diff --git a/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.mm b/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.mm index ec0ad3c6527df6..2352adfb3f162d 100644 --- a/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.mm +++ b/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.mm @@ -27,6 +27,7 @@ MTROTAUnsolicitedBDXMessageHandler::~MTROTAUnsolicitedBDXMessageHandler() { assertChipStackLockedByCurrentThread(); + if (mExchangeMgr) { mExchangeMgr->UnregisterUnsolicitedMessageHandlerForProtocol(Protocols::BDX::Id); mExchangeMgr = nullptr; @@ -50,15 +51,16 @@ const PayloadHeader & payloadHeader, ExchangeDelegate * _Nonnull & newDelegate) { assertChipStackLockedByCurrentThread(); - ChipLogDetail(BDX, "%s: message " ChipLogFormatMessageType " protocol " ChipLogFormatProtocolId, __FUNCTION__, + + ChipLogDetail(BDX, "OnUnsolicitedMessageReceived: message " ChipLogFormatMessageType " protocol " ChipLogFormatProtocolId, payloadHeader.GetMessageType(), ChipLogValueProtocolId(payloadHeader.GetProtocolID())); VerifyOrReturnError(mExchangeMgr != nullptr, CHIP_ERROR_INCORRECT_STATE); // If we receive a ReceiveInit BDX message, create a new MTROTAImageTransferHandler and register it - // as the handler for all BDX messages for the OTA file transfer that will come over this exchange. + // as the handler for all BDX messages that will come over this exchange. if (payloadHeader.HasMessageType(MessageType::ReceiveInit)) { - otaImageTransferHandler = new MTROTAImageTransferHandler(); + MTROTAImageTransferHandler * otaImageTransferHandler = new MTROTAImageTransferHandler(); newDelegate = otaImageTransferHandler; } return CHIP_NO_ERROR; diff --git a/src/darwin/Framework/CHIPTests/MTROTAProviderTests.m b/src/darwin/Framework/CHIPTests/MTROTAProviderTests.m index 478a42fdf42ab4..f2b14ea474a124 100644 --- a/src/darwin/Framework/CHIPTests/MTROTAProviderTests.m +++ b/src/darwin/Framework/CHIPTests/MTROTAProviderTests.m @@ -32,15 +32,6 @@ #define ENABLE_OTA_TESTS 1 #endif -// TODO: Disable test005_DoBDXTransferAllowUpdateRequest, -// test006_DoBDXTransferWithTwoOTARequesters and -// test007_DoBDXTransferIncrementalOtaUpdate until PR #26040 is merged. -// Currently the poll interval causes delays in the BDX transfer and -// results in the test taking a long time. -#ifdef ENABLE_REAL_OTA_UPDATE_TESTS -#undef ENABLE_REAL_OTA_UPDATE_TESTS -#endif - #if ENABLE_OTA_TESTS static const uint16_t kPairingTimeoutInSeconds = 10; @@ -48,15 +39,12 @@ static const uint16_t kTimeoutWithUpdateInSeconds = 60; static const uint64_t kDeviceId1 = 0x12341234; static const uint64_t kDeviceId2 = 0x12341235; -#ifdef ENABLE_REAL_OTA_UPDATE_TESTS static const uint64_t kDeviceId3 = 0x12341236; -#endif // ENABLE_REAL_OTA_UPDATE_TESTS + // NOTE: These onboarding payloads are for the chip-ota-requestor-app, not chip-all-clusters-app static NSString * kOnboardingPayload1 = @"MT:-24J0SO527K10648G00"; // Discriminator: 1111 static NSString * kOnboardingPayload2 = @"MT:-24J0AFN00L10648G00"; // Discriminator: 1112 -#ifdef ENABLE_REAL_OTA_UPDATE_TESTS static NSString * kOnboardingPayload3 = @"MT:-24J0IRV01L10648G00"; // Discriminator: 1113 -#endif // ENABLE_REAL_OTA_UPDATE_TESTS static const uint16_t kLocalPort = 5541; static const uint16_t kTestVendorId = 0xFFF1u; @@ -724,6 +712,9 @@ - (NSString *)absolutePathFor:(NSString *)matterRootRelativePath // Find the right absolute path to our file. PWD should // point to our src/darwin/Framework. NSString * pwd = [[NSProcessInfo processInfo] environment][@"PWD"]; + if (pwd == nil) { + return pwd; + } NSMutableArray * pathComponents = [[NSMutableArray alloc] init]; [pathComponents addObject:[pwd substringToIndex:(pwd.length - @"src/darwin/Framework".length)]]; [pathComponents addObjectsFromArray:[matterRootRelativePath pathComponents]]; @@ -1053,10 +1044,6 @@ - (void)test004_DoBDXTransferDenyUpdateRequest [self waitForExpectations:@[ checker.notifyUpdateAppliedExpectation ] timeout:kTimeoutInSeconds]; } -// TODO: Enable tests 005, 006 and 007 when PR #26040 is merged. Currently the poll interval causes delays in the BDX transfer -// and results in the tests taking a long time. With PR #26040 we eliminate the poll interval completely and hence the tests can run -// in a short time. -#ifdef ENABLE_REAL_OTA_UPDATE_TESTS - (void)test005_DoBDXTransferAllowUpdateRequest { // In this test we do the following: @@ -1562,7 +1549,6 @@ - (void)test007_DoBDXTransferIncrementalOtaUpdate // to _any_ of the above expectations. [self waitForExpectations:@[ announceResponseExpectation1 ] timeout:kTimeoutInSeconds]; } -#endif // ENABLE_REAL_OTA_UPDATE_TESTS - (void)test999_TearDown { diff --git a/src/protocols/bdx/AsyncTransferFacilitator.cpp b/src/protocols/bdx/AsyncTransferFacilitator.cpp index ebfc95045d4010..41055b67f93fff 100644 --- a/src/protocols/bdx/AsyncTransferFacilitator.cpp +++ b/src/protocols/bdx/AsyncTransferFacilitator.cpp @@ -21,61 +21,119 @@ namespace chip { namespace bdx { -CHIP_ERROR AsyncTransferFacilitator::OnMessageReceived(Messaging::ExchangeContext * ec, - const PayloadHeader & payloadHeader, +AsyncTransferFacilitator::~AsyncTransferFacilitator() +{ + Reset(); +} + +void AsyncTransferFacilitator::Reset() +{ + mExchange.Release(); + mTransfer.Reset(); +} + +CHIP_ERROR AsyncTransferFacilitator::SendMessage(TransferSession::OutputEvent & event) +{ + assertChipStackLockedByCurrentThread(); + + VerifyOrReturnError(mExchange, CHIP_ERROR_INCORRECT_STATE); + + Messaging::SendFlags sendFlags; + + // All messages sent from the Sender expect a response, except for a StatusReport which would indicate an error and + // the end of the transfer. + if (!event.msgTypeData.HasMessageType(Protocols::SecureChannel::MsgType::StatusReport)) + { + sendFlags.Set(Messaging::SendMessageFlags::kExpectResponse); + } + + auto & msgTypeData = event.msgTypeData; + + Messaging::ExchangeContext * ec = mExchange.Get(); + VerifyOrReturnError(ec != nullptr, CHIP_ERROR_INCORRECT_STATE); + + // Set the response timeout on the exchange before sending the message. + ec->SetResponseTimeout(mTimeout); + + CHIP_ERROR err = ec->SendMessage(msgTypeData.ProtocolId, msgTypeData.MessageType, std::move(event.MsgData), sendFlags); + + // If there's an error sending the message, release the exchange. + if (err != CHIP_NO_ERROR) + { + mExchange.Release(); + } + return err; +} + +CHIP_ERROR AsyncTransferFacilitator::OnMessageReceived(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader, System::PacketBufferHandle && payload) { VerifyOrReturnError(ec == mExchange.Get(), CHIP_ERROR_INCORRECT_STATE); - ChipLogDetail(BDX, " %s: message " ChipLogFormatMessageType " protocol " ChipLogFormatProtocolId, __FUNCTION__, - payloadHeader.GetMessageType(), ChipLogValueProtocolId(payloadHeader.GetProtocolID())); CHIP_ERROR err = mTransfer.HandleMessageReceived(payloadHeader, std::move(payload), System::SystemClock().GetMonotonicTimestamp()); if (err != CHIP_NO_ERROR) { ChipLogError(BDX, "failed to handle message: %" CHIP_ERROR_FORMAT, err.Format()); + mTransfer.AbortTransfer(AsyncResponder::GetBdxStatusCodeFromChipError(err)); } + else if (!payloadHeader.HasMessageType(MessageType::BlockAckEOF)) + { - // Almost every BDX message will follow up with a response on the exchange. Even messages that might signify the end of a - // transfer could necessitate a response if they are received at the wrong time. - // For this reason, it is left up to the application logic to call ExchangeContext::Close() when it has determined that the - // transfer is finished. - ec->WillSendMessage(); + // Almost every BDX message expect BlockAckEOF will follow up with a response on the exchange. + ec->WillSendMessage(); + } - // Get the next output event and send it to the delegate. + // Get the next output event and handle it based on the type of event. TransferSession::OutputEvent outEvent; - do + + mTransfer.GetNextAction(outEvent); + while (outEvent.EventType != TransferSession::OutputEventType::kNone) { - mTransfer.GetNextAction(outEvent); - if (outEvent.EventType == TransferSession::OutputEventType::kTransferTimeout) + // If its a message of type kMsgToSend, send the message over the exchange. + if (outEvent.EventType == TransferSession::OutputEventType::kMsgToSend) { - OnResponseTimeout(ec); + err = SendMessage(outEvent); + if (err != CHIP_NO_ERROR) + { + // TODO: Should we abort transfer here or let the other side timeout + return err; + } } else { - HandleAsyncTransferSessionOutput(outEvent); + // Call the HandleTransferSessionOutput virtual method that's implemeted by the subclass to handle + // the output events. + HandleTransferSessionOutput(outEvent); + + // 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 the CleanUp virtual method implemented by the subclass to clean up and + // delete both the sublcass and base class objects. + if (outEvent.EventType == TransferSession::OutputEventType::kAckEOFReceived || + outEvent.EventType == TransferSession::OutputEventType::kInternalError || + outEvent.EventType == TransferSession::OutputEventType::kTransferTimeout || + outEvent.EventType == TransferSession::OutputEventType::kStatusReceived) + { + CleanUp(); + return err; + } } - } while (outEvent.EventType != TransferSession::OutputEventType::kNone); - + mTransfer.GetNextAction(outEvent); + }; return err; } -Messaging::ExchangeContext * AsyncTransferFacilitator::GetExchangeContext() +void AsyncTransferFacilitator::OnExchangeClosing(Messaging::ExchangeContext * ec) { - return mExchange.Get(); -} - -void AsyncTransferFacilitator::OnExchangeClosing(chip::Messaging::ExchangeContext * ec) -{ - ChipLogDetail(BDX, "%s, ec: " ChipLogFormatExchange, __FUNCTION__, ChipLogValueExchange(ec)); - mExchange.Release(); + ChipLogDetail(BDX, "OnExchangeClosing, ec: " ChipLogFormatExchange, ChipLogValueExchange(ec)); + CleanUp(); } void AsyncTransferFacilitator::OnResponseTimeout(Messaging::ExchangeContext * ec) { - ChipLogDetail(BDX, "%s, ec: " ChipLogFormatExchange, __FUNCTION__, ChipLogValueExchange(ec)); - mExchange.Release(); + ChipLogDetail(BDX, "OnResponseTimeout, ec: " ChipLogFormatExchange, ChipLogValueExchange(ec)); + CleanUp(); } CHIP_ERROR AsyncResponder::PrepareForTransfer(Messaging::ExchangeContext * exchangeCtx, TransferRole role, @@ -83,24 +141,17 @@ CHIP_ERROR AsyncResponder::PrepareForTransfer(Messaging::ExchangeContext * excha System::Clock::Timeout timeout) { VerifyOrReturnError(exchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(!mExchange, CHIP_ERROR_INCORRECT_STATE); - ReturnErrorOnFailure(mTransfer.WaitForTransfer(role, xferControlOpts, maxBlockSize, timeout)); + mTimeout = timeout; + + ReturnErrorOnFailure(mTransfer.WaitForTransfer(role, xferControlOpts, maxBlockSize, mTimeout)); - VerifyOrReturnError(!mExchange, CHIP_ERROR_INCORRECT_STATE); mExchange.Grab(exchangeCtx); return CHIP_NO_ERROR; } -void AsyncResponder::ResetTransfer() -{ - if (mExchange) - { - mExchange.Release(); - } - mTransfer.Reset(); -} - bdx::StatusCode AsyncResponder::GetBdxStatusCodeFromChipError(CHIP_ERROR err) { if (err == CHIP_ERROR_INCORRECT_STATE) @@ -114,29 +165,45 @@ bdx::StatusCode AsyncResponder::GetBdxStatusCodeFromChipError(CHIP_ERROR err) return bdx::StatusCode::kUnknown; } -void AsyncResponder::NotifyEventHandledWithError(CHIP_ERROR error) +void AsyncResponder::NotifyEventHandled(CHIP_ERROR eventHandlingResult) { - ChipLogDetail(BDX, "%s : error %d", __FUNCTION__, error.AsInteger()); - if (error != CHIP_NO_ERROR) + // We can only come here after we have handled a BDX message (like ReceiveInit/BlockQuery) asynchronously + // in the subclass. The base class needs to notify us when the handling of the event is done and the event + // handling result - success or failure. + ChipLogDetail(BDX, "NotifyEventHandled : error %" CHIP_ERROR_FORMAT, eventHandlingResult.Format()); + + // If there was an error, we abort the transfer. + if (eventHandlingResult != CHIP_NO_ERROR) { - mTransfer.AbortTransfer(GetBdxStatusCodeFromChipError(error)); + mTransfer.AbortTransfer(GetBdxStatusCodeFromChipError(eventHandlingResult)); } - // Get the next output event and send it to the delegate. + // We need to get the next output event from the state machine which is either a response to the + // BDX message handled by the subclass (ReceiveAccept/Block) or a status report or error or timeout + // event types. TransferSession::OutputEvent outEvent; - do - { - mTransfer.GetNextAction(outEvent); - if (outEvent.EventType == TransferSession::OutputEventType::kTransferTimeout) + mTransfer.GetNextAction(outEvent); + while (outEvent.EventType != TransferSession::OutputEventType::kNone) + { + if (outEvent.EventType == TransferSession::OutputEventType::kMsgToSend) { - OnResponseTimeout(mExchange.Get()); + CHIP_ERROR err = AsyncTransferFacilitator::SendMessage(outEvent); + if (err != CHIP_NO_ERROR) + { + // TODO: Should we abort transfer here or let the other side timeout + } } - else + else if (outEvent.EventType == TransferSession::OutputEventType::kInternalError || + outEvent.EventType == TransferSession::OutputEventType::kTransferTimeout || + outEvent.EventType == TransferSession::OutputEventType::kStatusReceived) { - HandleAsyncTransferSessionOutput(outEvent); + HandleTransferSessionOutput(outEvent); + CleanUp(); + return; } - } while (outEvent.EventType != TransferSession::OutputEventType::kNone); + mTransfer.GetNextAction(outEvent); + }; } } // namespace bdx diff --git a/src/protocols/bdx/AsyncTransferFacilitator.h b/src/protocols/bdx/AsyncTransferFacilitator.h index 725b55c8b5d518..05aa7b0dcaa6b7 100644 --- a/src/protocols/bdx/AsyncTransferFacilitator.h +++ b/src/protocols/bdx/AsyncTransferFacilitator.h @@ -19,6 +19,7 @@ #include #include #include +#include #pragma once @@ -27,7 +28,7 @@ namespace bdx { /** * An abstract class with methods for handling BDX messages from an ExchangeContext and using an event driven - * async approach to get the next action from the transfer session state machine. + * asynchronous approach to get the next action from the transfer session state machine. * * This class does not define any methods for beginning a transfer or initializing the underlying TransferSession object. * See AsyncResponder for a class that does. @@ -39,41 +40,52 @@ class AsyncTransferFacilitator : public Messaging::ExchangeDelegate { public: AsyncTransferFacilitator() : mExchange(*this) {} - ~AsyncTransferFacilitator() override = default; + ~AsyncTransferFacilitator(); /** - * This method should be implemented to contain business-logic handling of BDX messages and other TransferSession events. + * This method should be implemented to contain business-logic handling of BDX messages + * and other TransferSession events. * * @param[in] event An OutputEvent that contains output from the TransferSession object. */ virtual void HandleTransferSessionOutput(TransferSession::OutputEvent & event) = 0; /** - * This method returns the exchange on which the BDX transfer is happening. + * This method should be implemented to clean up and destroy the instance of the sub class + * implemeting it and the base class. * - * May return null if there is no such exchange (e.g. it has been closed due to the transfer completing - * or some error condition). + * Note: After this method has been called, the instance of the subclass on which this + * is called can't be used. */ - Messaging::ExchangeContext * GetExchangeContext(); + virtual void CleanUp() = 0; +protected: CHIP_ERROR OnMessageReceived(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader, System::PacketBufferHandle && payload) override; void OnResponseTimeout(Messaging::ExchangeContext * ec) override; void OnExchangeClosing(Messaging::ExchangeContext * ec) override; -protected: + CHIP_ERROR SendMessage(TransferSession::OutputEvent & event); + // The transfer session coresponding to this AsynTransferFacilitator object. TransferSession mTransfer; - // The Exchange holder that holds the exchange context used for the BDX messages. + // The Exchange holder that holds the exchange context used for BDX messages. Messaging::ExchangeHolder mExchange; + + // The timeout for the BDX transfer session. + System::Clock::Timeout mTimeout; + +private: + // Releases the exchange context and calls reset on the BDX transfer session. + void Reset(); }; /** * An AsyncTransferFacilitator that is initialized to respond to an incoming BDX transfer request. * * Provides a method for initializing the TransferSession member but still needs to be extended to implement - * HandleAsyncTransferSessionOutput. + * HandleTransferSessionOutput. * * This class, or a subclass of it, should be used as the exchange delegate for a BDX transfer. */ @@ -93,12 +105,9 @@ class AsyncResponder : public AsyncTransferFacilitator BitFlags xferControlOpts, uint16_t maxBlockSize, System::Clock::Timeout timeout); - void ResetTransfer(); - /** - * Notifies the transfer facilitator that an output event has been handled by the delegate and passes any error(s) that occured - * while handling the event. This is needed as the delegate calls async callbacks for handling the BDX messages and we need to - * get the next action from the state machine based on how the delegate handled the message. + * Notifies the transfer facilitator that an output event has been handled by the subclass that implements the + * HandleTransferSessionOutput and passes any error(s) that occured while handling the event back to the facilitator. * * @param[in] eventHandlingResult The result of handling the event. CHIP_NO_ERROR if * it was handled successfully, otherwise the error that @@ -106,8 +115,8 @@ class AsyncResponder : public AsyncTransferFacilitator */ void NotifyEventHandled(CHIP_ERROR eventHandlingResult); -private: - bdx::StatusCode GetBdxStatusCodeFromChipError(CHIP_ERROR err); + static bdx::StatusCode GetBdxStatusCodeFromChipError(CHIP_ERROR err); }; + } // namespace bdx } // namespace chip