From a957601e457eba3d08ebc5494057427f7243bbda Mon Sep 17 00:00:00 2001 From: Nivedita Sarkar Date: Tue, 31 Jan 2023 20:41:01 -0800 Subject: [PATCH] Add a timer to track whether we have received BDX init after a query image was successful - Currently if an ota requester has a successful query image and an image it available but if for any reason, the ota requester doesn't send BDX init, the provider will be stuck until we reboot the resident. In order to have a fail safe, we are adding a timer that starts after query image returns image available and waits for a BDX init to come. In case BDX init doesn't come, it times out and resets state - Add code to reset state if any API fails on the provider once we prepare for BDX transfer - Stop polling when BDX transfer reset is called - Return QueryImageResponse status busy instead of general failure if the sdk is busy and gets a sexond query image so accessory can handle the error correctly and retry until the sdk is done --- .../CHIP/MTROTAProviderDelegateBridge.h | 3 + .../CHIP/MTROTAProviderDelegateBridge.mm | 94 +++++++++++++------ src/protocols/bdx/TransferFacilitator.cpp | 1 + 3 files changed, 67 insertions(+), 31 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h b/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h index a4b69f231dacd7..57f59d0bfc6887 100644 --- a/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h +++ b/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h @@ -33,6 +33,8 @@ class MTROTAProviderDelegateBridge : public chip::app::Clusters::OTAProviderDele // touches Matter objects. void Shutdown(); + void ResetState(); + // ControllerShuttingDown must be called on the Matter work queue, since it // touches Matter objects. void ControllerShuttingDown(MTRDeviceController * controller); @@ -65,6 +67,7 @@ class MTROTAProviderDelegateBridge : public chip::app::Clusters::OTAProviderDele static void ConvertToNotifyUpdateAppliedParams( const chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::NotifyUpdateApplied::DecodableType & commandData, MTROTASoftwareUpdateProviderClusterNotifyUpdateAppliedParams * commandParams); + static void handleBdxInitReceivedTimeoutExpired(chip::System::Layer * systemLayer, void * state); _Nullable id mDelegate; dispatch_queue_t mDelegateNotificationQueue; diff --git a/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm b/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm index 8592a645982d89..86c50897a894ca 100644 --- a/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm +++ b/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm @@ -42,6 +42,11 @@ // TODO Expose a method onto the delegate to make that configurable. constexpr uint32_t kMaxBdxBlockSize = 1024; constexpr uint32_t kMaxBDXURILen = 256; + +// 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 constexpr System::Clock::Timeout kBdxPollIntervalMs = System::Clock::Milliseconds32(50); constexpr bdx::TransferRole kBdxRole = bdx::TransferRole::kSender; @@ -89,13 +94,12 @@ CHIP_ERROR Shutdown() VerifyOrReturnError(mExchangeMgr != nullptr, CHIP_ERROR_INCORRECT_STATE); mExchangeMgr->UnregisterUnsolicitedMessageHandlerForProtocol(Protocols::BDX::Id); + ResetState(); mExchangeMgr = nullptr; mSystemLayer = nullptr; mDelegateNotificationQueue = nil; - ResetState(); - return CHIP_NO_ERROR; } @@ -120,7 +124,40 @@ void SetDelegate(id delegate, dispatch_queue_t delegateN } } + void ResetState() + { + assertChipStackLockedByCurrentThread(); + if (mSystemLayer) { + mSystemLayer->CancelTimer(handleBdxInitReceivedTimeoutExpired, this); + } + + if (!mInitialized) { + return; + } + Responder::ResetTransfer(); + ++mTransferGeneration; + mFabricIndex.ClearValue(); + mNodeId.ClearValue(); + + if (mExchangeCtx != nullptr) { + mExchangeCtx->Close(); + mExchangeCtx = nullptr; + } + + mInitialized = false; + } + private: + /** + * The callback when the BDX init received timer expires to track if a BDX init was received after a successful query image + * within a reasonable amount of time. + */ + static void handleBdxInitReceivedTimeoutExpired(chip::System::Layer * systemLayer, void * state) + { + VerifyOrReturn(state != nullptr); + static_cast(state)->ResetState(); + } + CHIP_ERROR OnMessageToSend(TransferSession::OutputEvent & event) { assertChipStackLockedByCurrentThread(); @@ -143,7 +180,10 @@ CHIP_ERROR OnMessageToSend(TransferSession::OutputEvent & event) CHIP_ERROR OnTransferSessionBegin(TransferSession::OutputEvent & event) { assertChipStackLockedByCurrentThread(); - + // Once we receive the BDX init, cancel the BDX Init timeout and start the BDX session + if (mSystemLayer) { + mSystemLayer->CancelTimer(handleBdxInitReceivedTimeoutExpired, this); + } VerifyOrReturnError(mFabricIndex.HasValue(), CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mNodeId.HasValue(), CHIP_ERROR_INCORRECT_STATE); uint16_t fdl = 0; @@ -165,6 +205,7 @@ CHIP_ERROR OnTransferSessionBegin(TransferSession::OutputEvent & event) if (!mInitialized || mTransferGeneration != transferGeneration) { // Callback for a stale transfer. + LogErrorOnFailure(mTransfer.AbortTransfer(bdx::StatusCode::kUnexpectedMessage)); return; } @@ -370,6 +411,9 @@ CHIP_ERROR ConfigureState(chip::FabricIndex fabricIndex, chip::NodeId nodeId) ResetState(); } + // Start a timer to track whether we receive a BDX init after a successful query image in a reasonabale amount of time + mSystemLayer->StartTimer(kBdxInitReceivedTimeout, handleBdxInitReceivedTimeoutExpired, this); + mFabricIndex.SetValue(fabricIndex); mNodeId.SetValue(nodeId); @@ -378,27 +422,6 @@ CHIP_ERROR ConfigureState(chip::FabricIndex fabricIndex, chip::NodeId nodeId) return CHIP_NO_ERROR; } - void ResetState() - { - assertChipStackLockedByCurrentThread(); - - if (!mInitialized) { - return; - } - - Responder::ResetTransfer(); - ++mTransferGeneration; - mFabricIndex.ClearValue(); - mNodeId.ClearValue(); - - if (mExchangeCtx != nullptr) { - mExchangeCtx->Close(); - mExchangeCtx = nullptr; - } - - mInitialized = false; - } - bool mInitialized = false; Optional mFabricIndex; Optional mNodeId; @@ -574,11 +597,18 @@ bool GetPeerNodeInfo(CommandHandler * commandHandler, const ConcreteCommandPath CHIP_ERROR err = gOtaSender.PrepareForTransfer(fabricIndex, nodeId); if (CHIP_NO_ERROR != err) { LogErrorOnFailure(err); - handler->AddStatus(cachedCommandPath, Protocols::InteractionModel::Status::Failure); - handle.Release(); - return; + if (err == CHIP_ERROR_BUSY) { + response.status = static_cast(MTROTASoftwareUpdateProviderOTAQueryStatusBusy); + handler->AddResponse(cachedCommandPath, response); + handle.Release(); + return; + } else { + handler->AddStatus(cachedCommandPath, Protocols::InteractionModel::Status::Failure); + handle.Release(); + gOtaSender.ResetState(); + return; + } } - auto targetNodeId = handler->GetExchangeContext()->GetSessionHandle()->AsSecureSession()->GetLocalScopedNodeId(); @@ -589,6 +619,7 @@ bool GetPeerNodeInfo(CommandHandler * commandHandler, const ConcreteCommandPath LogErrorOnFailure(err); handler->AddStatus(cachedCommandPath, Protocols::InteractionModel::Status::Failure); handle.Release(); + gOtaSender.ResetState(); return; } @@ -600,11 +631,12 @@ bool GetPeerNodeInfo(CommandHandler * commandHandler, const ConcreteCommandPath handler->AddResponse(cachedCommandPath, response); handle.Release(); + gOtaSender.ResetState(); } - errorHandler:^(NSError *) { - // Not much we can do here - }]; + errorHandler:^(NSError *) { + gOtaSender.ResetState(); + }]; }; auto strongDelegate = mDelegate; diff --git a/src/protocols/bdx/TransferFacilitator.cpp b/src/protocols/bdx/TransferFacilitator.cpp index 65ab653420c2cd..767d16313994e4 100644 --- a/src/protocols/bdx/TransferFacilitator.cpp +++ b/src/protocols/bdx/TransferFacilitator.cpp @@ -112,6 +112,7 @@ CHIP_ERROR Responder::PrepareForTransfer(System::Layer * layer, TransferRole rol void Responder::ResetTransfer() { mTransfer.Reset(); + mStopPolling = true; } CHIP_ERROR Initiator::InitiateTransfer(System::Layer * layer, TransferRole role, const TransferSession::TransferInitData & initData,