From 6d0718498a4267c0536f5edf9471d8b2a0291af5 Mon Sep 17 00:00:00 2001 From: Carol Yang Date: Wed, 12 Jan 2022 20:54:50 -0800 Subject: [PATCH] Address code review comments - Add missing idle updates - Fix logic for setting UpdateStateProgress - Remove OtaRequestorServer class and define the setters as global functions --- .../clusters/ota-requestor/OTARequestor.cpp | 9 +++--- src/app/clusters/ota-requestor/OTARequestor.h | 4 +-- .../ota-requestor/ota-requestor-server.cpp | 29 ++++--------------- .../ota-requestor/ota-requestor-server.h | 14 +++------ 4 files changed, 17 insertions(+), 39 deletions(-) diff --git a/src/app/clusters/ota-requestor/OTARequestor.cpp b/src/app/clusters/ota-requestor/OTARequestor.cpp index fd1677ad032f10..1e23db274e6fb6 100644 --- a/src/app/clusters/ota-requestor/OTARequestor.cpp +++ b/src/app/clusters/ota-requestor/OTARequestor.cpp @@ -20,7 +20,6 @@ * OTA Requestor logic is contained in this class. */ -#include #include #include #include @@ -90,12 +89,12 @@ static void LogApplyUpdateResponse(const ApplyUpdateResponse::DecodableType & re static void SetUpdateStateAttribute(OTAUpdateStateEnum state) { - OtaRequestorServer::GetInstance().SetUpdateState(state); + OtaRequestorServerSetUpdateState(state); // The UpdateStateProgress attribute only applies to the querying state if (state != OTAUpdateStateEnum::kQuerying) { - OtaRequestorServer::GetInstance().SetUpdateStateProgress(0); + OtaRequestorServerSetUpdateStateProgress(0); } } @@ -302,6 +301,7 @@ void OTARequestor::OnConnected(void * context, OperationalDeviceProxy * devicePr { ChipLogError(SoftwareUpdate, "Failed to start download: %" CHIP_ERROR_FORMAT, err.Format()); requestorCore->mOtaRequestorDriver->HandleError(OTAUpdateStateEnum::kDownloading, err); + SetUpdateStateAttribute(OTAUpdateStateEnum::kIdle); return; } @@ -315,6 +315,7 @@ void OTARequestor::OnConnected(void * context, OperationalDeviceProxy * devicePr { ChipLogError(SoftwareUpdate, "Failed to send ApplyUpdate command: %" CHIP_ERROR_FORMAT, err.Format()); requestorCore->mOtaRequestorDriver->HandleError(OTAUpdateStateEnum::kApplying, err); + SetUpdateStateAttribute(OTAUpdateStateEnum::kIdle); return; } @@ -394,7 +395,7 @@ void OTARequestor::OnDownloadStateChanged(OTADownloader::State state) void OTARequestor::OnUpdateProgressChanged(uint8_t percent) { - OtaRequestorServer::GetInstance().SetUpdateStateProgress(percent); + OtaRequestorServerSetUpdateStateProgress(percent); } CHIP_ERROR OTARequestor::SendQueryImageRequest(OperationalDeviceProxy & deviceProxy) diff --git a/src/app/clusters/ota-requestor/OTARequestor.h b/src/app/clusters/ota-requestor/OTARequestor.h index b8c0c082e1c4f3..ee5184ffaa4d76 100644 --- a/src/app/clusters/ota-requestor/OTARequestor.h +++ b/src/app/clusters/ota-requestor/OTARequestor.h @@ -80,8 +80,8 @@ class OTARequestor : public OTARequestorInterface, public BDXDownloader::StateDe mOtaRequestorDriver = driver; mBdxDownloader = downloader; - OtaRequestorServer::GetInstance().SetUpdateState(app::Clusters::OtaSoftwareUpdateRequestor::OTAUpdateStateEnum::kIdle); - OtaRequestorServer::GetInstance().SetUpdateStateProgress(0); + OtaRequestorServerSetUpdateState(app::Clusters::OtaSoftwareUpdateRequestor::OTAUpdateStateEnum::kIdle); + OtaRequestorServerSetUpdateStateProgress(0); } /** diff --git a/src/app/clusters/ota-requestor/ota-requestor-server.cpp b/src/app/clusters/ota-requestor/ota-requestor-server.cpp index 437ed5d96daf09..0fc2bffba7ce45 100644 --- a/src/app/clusters/ota-requestor/ota-requestor-server.cpp +++ b/src/app/clusters/ota-requestor/ota-requestor-server.cpp @@ -20,10 +20,8 @@ * to the OTA Requestor object that handles them */ -#include #include #include -#include #include #include @@ -81,19 +79,9 @@ CHIP_ERROR OtaSoftwareUpdateRequestorAttrAccess::Write(const ConcreteDataAttribu } // namespace -namespace chip { - // ----------------------------------------------------------------------------- -// OtaRequestorServer implementation - -static OtaRequestorServer sInstance; - -OtaRequestorServer & OtaRequestorServer::GetInstance(void) -{ - return sInstance; -} - -EmberAfStatus OtaRequestorServer::SetUpdateState(OTAUpdateStateEnum value) +// Global functions +EmberAfStatus OtaRequestorServerSetUpdateState(OTAUpdateStateEnum value) { EmberAfStatus status = EMBER_ZCL_STATUS_SUCCESS; @@ -114,7 +102,7 @@ EmberAfStatus OtaRequestorServer::SetUpdateState(OTAUpdateStateEnum value) return status; } -EmberAfStatus OtaRequestorServer::SetUpdateStateProgress(uint8_t value) +EmberAfStatus OtaRequestorServerSetUpdateStateProgress(uint8_t value) { EmberAfStatus status = EMBER_ZCL_STATUS_SUCCESS; @@ -124,21 +112,16 @@ EmberAfStatus OtaRequestorServer::SetUpdateStateProgress(uint8_t value) app::DataModel::Nullable currentValue; status = Attributes::UpdateStateProgress::Get(endpoint, currentValue); VerifyOrDie(EMBER_ZCL_STATUS_SUCCESS == status); - if (!currentValue.IsNull()) + if (currentValue.IsNull() || currentValue.Value() != value) { - if (currentValue.Value() != value) - { - status = Attributes::UpdateStateProgress::Set(endpoint, value); - VerifyOrDie(EMBER_ZCL_STATUS_SUCCESS == status); - } + status = Attributes::UpdateStateProgress::Set(endpoint, value); + VerifyOrDie(EMBER_ZCL_STATUS_SUCCESS == status); } } return status; } -} // namespace chip - // ----------------------------------------------------------------------------- // Callbacks implementation diff --git a/src/app/clusters/ota-requestor/ota-requestor-server.h b/src/app/clusters/ota-requestor/ota-requestor-server.h index 2f483ef71f9273..9a5afcfc193979 100644 --- a/src/app/clusters/ota-requestor/ota-requestor-server.h +++ b/src/app/clusters/ota-requestor/ota-requestor-server.h @@ -16,15 +16,9 @@ * limitations under the License. */ -namespace chip { +#pragma once -class OtaRequestorServer -{ -public: - static OtaRequestorServer & GetInstance(void); +#include - EmberAfStatus SetUpdateState(chip::app::Clusters::OtaSoftwareUpdateRequestor::OTAUpdateStateEnum value); - EmberAfStatus SetUpdateStateProgress(uint8_t value); -}; - -} // namespace chip +EmberAfStatus OtaRequestorServerSetUpdateState(chip::app::Clusters::OtaSoftwareUpdateRequestor::OTAUpdateStateEnum value); +EmberAfStatus OtaRequestorServerSetUpdateStateProgress(uint8_t value);