From 7b42e10aeda7a05bccc2ec7f734b40e908c9f99f Mon Sep 17 00:00:00 2001 From: "Irene Siu (Apple)" Date: Fri, 4 Mar 2022 17:54:28 -0800 Subject: [PATCH] Issue #11760 - [ota-requestor-app] Abort query image download on timeout (#15768) * Issue #11760 - [ota-provider-app] Support for error/timeout midway in transfer * restyle-diff.sh changes * Code review changes. - Moved mTimeout to BDXDownloader class. - Moved prevPercentageComplete to BDXDownloader class. - Renamed StartTimeoutTimerHandler to TransferTimeoutCheckHandler. - Added sanity checks. * Code review changes. - Moved mTimeoutSec to a constant define kTimeoutSec. - Moved resetting of timeout timer and prevPercentageComplete to a Reset() function. * Code review changes. - Renamed prevPercentageComplete to mPrevPercentageComplete * Code review changes. - Renmed CheckTransferTimeout() to HasTransferTimedOut(). - Calculate timeout using block counter instead of percentage complete. - Use 5*60 instead of 300 for timeout value for readability. * Fix merge issues. --- .../clusters/ota-requestor/BDXDownloader.cpp | 64 ++++++++++++++++++- .../clusters/ota-requestor/BDXDownloader.h | 12 +++- .../clusters/ota-requestor/OTARequestor.cpp | 5 +- src/protocols/bdx/BdxTransferSession.h | 1 + 4 files changed, 78 insertions(+), 4 deletions(-) diff --git a/src/app/clusters/ota-requestor/BDXDownloader.cpp b/src/app/clusters/ota-requestor/BDXDownloader.cpp index 76c86627aa0eed..db92bb9e983366 100644 --- a/src/app/clusters/ota-requestor/BDXDownloader.cpp +++ b/src/app/clusters/ota-requestor/BDXDownloader.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include /* TODO:(#12520) remove */ #include @@ -35,6 +36,50 @@ using chip::bdx::TransferSession; namespace chip { +void TransferTimeoutCheckHandler(System::Layer * systemLayer, void * appState) +{ + VerifyOrReturn(appState != nullptr); + BDXDownloader * bdxDownloader = static_cast(appState); + + if (bdxDownloader->HasTransferTimedOut()) + { + // End download if transfer timeout has been detected + bdxDownloader->OnDownloadTimeout(); + } + else + { + // Else restart the timer + systemLayer->StartTimer(bdxDownloader->GetTimeout(), TransferTimeoutCheckHandler, appState); + } +} + +System::Clock::Timeout BDXDownloader::GetTimeout() +{ + return mTimeout; +} + +void BDXDownloader::Reset() +{ + mPrevBlockCounter = 0; + DeviceLayer::SystemLayer().StartTimer(mTimeout, TransferTimeoutCheckHandler, this); +} + +bool BDXDownloader::HasTransferTimedOut() +{ + uint32_t curBlockCounter = mBdxTransfer.GetNextBlockNum(); + + if (curBlockCounter > mPrevBlockCounter) + { + mPrevBlockCounter = curBlockCounter; + return false; + } + else + { + ChipLogError(BDX, "BDX transfer timeout"); + return true; + } +} + void BDXDownloader::OnMessageReceived(const chip::PayloadHeader & payloadHeader, chip::System::PacketBufferHandle msg) { VerifyOrReturn(mState == State::kInProgress, ChipLogError(BDX, "Can't accept messages, no transfer in progress")); @@ -50,9 +95,11 @@ void BDXDownloader::OnMessageReceived(const chip::PayloadHeader & payloadHeader, PollTransferSession(); } -CHIP_ERROR BDXDownloader::SetBDXParams(const chip::bdx::TransferSession::TransferInitData & bdxInitData) +CHIP_ERROR BDXDownloader::SetBDXParams(const chip::bdx::TransferSession::TransferInitData & bdxInitData, + System::Clock::Timeout timeout) { - mState = State::kIdle; + mTimeout = timeout; + mState = State::kIdle; mBdxTransfer.Reset(); VerifyOrReturnError(mState == State::kIdle, CHIP_ERROR_INCORRECT_STATE); @@ -69,6 +116,10 @@ CHIP_ERROR BDXDownloader::BeginPrepareDownload() { VerifyOrReturnError(mState == State::kIdle, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mImageProcessor != nullptr, CHIP_ERROR_INCORRECT_STATE); + + mPrevBlockCounter = 0; + DeviceLayer::SystemLayer().StartTimer(mTimeout, TransferTimeoutCheckHandler, this); + ReturnErrorOnFailure(mImageProcessor->PrepareDownload()); SetState(State::kPreparing, OTAChangeReasonEnum::kSuccess); @@ -90,6 +141,7 @@ CHIP_ERROR BDXDownloader::OnPreparedForDownload(CHIP_ERROR status) else { ChipLogError(BDX, "failed to prepare download: %" CHIP_ERROR_FORMAT, status.Format()); + Reset(); mBdxTransfer.Reset(); SetState(State::kIdle, OTAChangeReasonEnum::kFailure); } @@ -108,6 +160,8 @@ CHIP_ERROR BDXDownloader::FetchNextData() void BDXDownloader::OnDownloadTimeout() { + Reset(); + if (mState == State::kInProgress) { ChipLogDetail(BDX, "aborting due to timeout"); @@ -126,6 +180,8 @@ void BDXDownloader::OnDownloadTimeout() void BDXDownloader::EndDownload(CHIP_ERROR reason) { + Reset(); + if (mState == State::kInProgress) { bdx::StatusCode status = bdx::StatusCode::kUnknown; @@ -186,6 +242,8 @@ CHIP_ERROR BDXDownloader::HandleBdxEvent(const chip::bdx::TransferSession::Outpu ReturnErrorOnFailure(mMsgDelegate->SendMessage(outEvent)); if (outEvent.msgTypeData.HasMessageType(chip::bdx::MessageType::BlockAckEOF)) { + Reset(); + // BDX transfer is not complete until BlockAckEOF has been sent SetState(State::kComplete, OTAChangeReasonEnum::kSuccess); } @@ -212,11 +270,13 @@ CHIP_ERROR BDXDownloader::HandleBdxEvent(const chip::bdx::TransferSession::Outpu break; case TransferSession::OutputEventType::kInternalError: ChipLogError(BDX, "TransferSession error"); + Reset(); mBdxTransfer.Reset(); ReturnErrorOnFailure(mImageProcessor->Abort()); break; case TransferSession::OutputEventType::kTransferTimeout: ChipLogError(BDX, "Transfer timed out"); + Reset(); mBdxTransfer.Reset(); ReturnErrorOnFailure(mImageProcessor->Abort()); break; diff --git a/src/app/clusters/ota-requestor/BDXDownloader.h b/src/app/clusters/ota-requestor/BDXDownloader.h index 96c63d941d9180..a423f6cac9f728 100644 --- a/src/app/clusters/ota-requestor/BDXDownloader.h +++ b/src/app/clusters/ota-requestor/BDXDownloader.h @@ -63,7 +63,7 @@ class BDXDownloader : public chip::OTADownloader void SetStateDelegate(StateDelegate * delegate) { mStateDelegate = delegate; } // Initialize a BDX transfer session but will not proceed until OnPreparedForDownload() is called. - CHIP_ERROR SetBDXParams(const chip::bdx::TransferSession::TransferInitData & bdxInitData); + CHIP_ERROR SetBDXParams(const chip::bdx::TransferSession::TransferInitData & bdxInitData, System::Clock::Timeout timeout); // OTADownloader Overrides CHIP_ERROR BeginPrepareDownload() override; @@ -75,14 +75,24 @@ class BDXDownloader : public chip::OTADownloader CHIP_ERROR FetchNextData() override; // TODO: override SkipData + System::Clock::Timeout GetTimeout(); + // If True, there's been a timeout in the transfer as measured by no download progress after 'mTimeout' seconds. + // If False, there's been progress in the transfer. + bool HasTransferTimedOut(); + private: void PollTransferSession(); CHIP_ERROR HandleBdxEvent(const chip::bdx::TransferSession::OutputEvent & outEvent); void SetState(State state, app::Clusters::OtaSoftwareUpdateRequestor::OTAChangeReasonEnum reason); + void Reset(); chip::bdx::TransferSession mBdxTransfer; MessagingDelegate * mMsgDelegate = nullptr; StateDelegate * mStateDelegate = nullptr; + // Timeout value in seconds to abort the download if there's no progress in the transfer session. + System::Clock::Timeout mTimeout = System::Clock::kZero; + // Tracks the last block counter used during the transfer session as of the previous check. + uint32_t mPrevBlockCounter = 0; }; } // namespace chip diff --git a/src/app/clusters/ota-requestor/OTARequestor.cpp b/src/app/clusters/ota-requestor/OTARequestor.cpp index 3f85f1ccc816af..956699fb215487 100644 --- a/src/app/clusters/ota-requestor/OTARequestor.cpp +++ b/src/app/clusters/ota-requestor/OTARequestor.cpp @@ -44,6 +44,9 @@ using bdx::TransferSession; // Global instance of the OTARequestorInterface. OTARequestorInterface * globalOTARequestorInstance = nullptr; +// Abort the QueryImage download request if there's been no progress for 5 minutes +static constexpr System::Clock::Timeout kDownloadTimeoutSec = chip::System::Clock::Seconds32(5 * 60); + static void LogQueryImageResponse(const QueryImageResponse::DecodableType & response) { ChipLogDetail(SoftwareUpdate, "QueryImageResponse:"); @@ -701,7 +704,7 @@ CHIP_ERROR OTARequestor::StartDownload(OperationalDeviceProxy & deviceProxy) mBdxDownloader->SetMessageDelegate(&mBdxMessenger); mBdxDownloader->SetStateDelegate(this); - ReturnErrorOnFailure(mBdxDownloader->SetBDXParams(initOptions)); + ReturnErrorOnFailure(mBdxDownloader->SetBDXParams(initOptions, kDownloadTimeoutSec)); return mBdxDownloader->BeginPrepareDownload(); } diff --git a/src/protocols/bdx/BdxTransferSession.h b/src/protocols/bdx/BdxTransferSession.h index a6ff92abc21409..b80086c3d89650 100644 --- a/src/protocols/bdx/BdxTransferSession.h +++ b/src/protocols/bdx/BdxTransferSession.h @@ -297,6 +297,7 @@ class DLL_EXPORT TransferSession uint64_t GetStartOffset() const { return mStartOffset; } uint64_t GetTransferLength() const { return mTransferLength; } uint16_t GetTransferBlockSize() const { return mTransferMaxBlockSize; } + uint32_t GetNextBlockNum() { return mNextBlockNum; } size_t GetNumBytesProcessed() const { return mNumBytesProcessed; } const uint8_t * GetFileDesignator(uint16_t & fileDesignatorLen) const {