From 95c5c88be60857a62814cb3de02e023c8269423b Mon Sep 17 00:00:00 2001 From: Carol Yang Date: Sat, 22 Jan 2022 20:40:23 -0800 Subject: [PATCH 1/3] [OTA] Use ConfigurationManager as the source of truth for Basic cluster attributes --- examples/common/pigweed/rpc_services/Device.h | 2 +- src/app/clusters/basic/basic.cpp | 2 +- .../clusters/ota-requestor/OTARequestor.cpp | 33 ++++++++++++------- src/app/clusters/ota-requestor/OTARequestor.h | 2 +- src/include/platform/ConfigurationManager.h | 2 +- .../GenericConfigurationManagerImpl.h | 4 +-- src/platform/Linux/PlatformManagerImpl.cpp | 2 +- .../android/ConfigurationManagerImpl.cpp | 2 +- .../android/ConfigurationManagerImpl.h | 2 +- src/platform/fake/ConfigurationManagerImpl.h | 2 +- 10 files changed, 32 insertions(+), 21 deletions(-) diff --git a/examples/common/pigweed/rpc_services/Device.h b/examples/common/pigweed/rpc_services/Device.h index de41f5927165ca..e647fa415a7ba7 100644 --- a/examples/common/pigweed/rpc_services/Device.h +++ b/examples/common/pigweed/rpc_services/Device.h @@ -94,7 +94,7 @@ class Device : public pw_rpc::nanopb::Device::Service response.product_id = CHIP_DEVICE_CONFIG_DEVICE_PRODUCT_ID; } - uint16_t software_version; + uint32_t software_version; if (DeviceLayer::ConfigurationMgr().GetSoftwareVersion(software_version) == CHIP_NO_ERROR) { response.software_version = software_version; diff --git a/src/app/clusters/basic/basic.cpp b/src/app/clusters/basic/basic.cpp index b162bd5210fa8c..8b9368d95b4521 100644 --- a/src/app/clusters/basic/basic.cpp +++ b/src/app/clusters/basic/basic.cpp @@ -248,7 +248,7 @@ void emberAfBasicClusterServerInitCallback(chip::EndpointId endpoint) VerifyOrdo(EMBER_ZCL_STATUS_SUCCESS == status, ChipLogError(Zcl, "Error setting Software Version String: 0x%02x", status)); } - uint16_t softwareVersion; + uint32_t softwareVersion; if (ConfigurationMgr().GetSoftwareVersion(softwareVersion) == CHIP_NO_ERROR) { status = Attributes::SoftwareVersion::Set(endpoint, softwareVersion); diff --git a/src/app/clusters/ota-requestor/OTARequestor.cpp b/src/app/clusters/ota-requestor/OTARequestor.cpp index c44d8a1754fb61..bdea99609c8d45 100644 --- a/src/app/clusters/ota-requestor/OTARequestor.cpp +++ b/src/app/clusters/ota-requestor/OTARequestor.cpp @@ -36,6 +36,7 @@ using namespace app::Clusters::OtaSoftwareUpdateProvider; using namespace app::Clusters::OtaSoftwareUpdateProvider::Commands; using namespace app::Clusters::OtaSoftwareUpdateRequestor; using namespace app::Clusters::OtaSoftwareUpdateRequestor::Commands; +using namespace DeviceLayer; using app::DataModel::Nullable; using bdx::TransferSession; @@ -388,12 +389,11 @@ void OTARequestor::ApplyUpdate() void OTARequestor::NotifyUpdateApplied(uint32_t version) { // New version is executing so update where applicable - VerifyOrReturn(Basic::Attributes::SoftwareVersion::Set(kRootEndpointId, version) == EMBER_ZCL_STATUS_SUCCESS); mCurrentVersion = version; // Log the VersionApplied event uint16_t productId; - VerifyOrReturn(Basic::Attributes::ProductID::Get(kRootEndpointId, &productId) == EMBER_ZCL_STATUS_SUCCESS); + VerifyOrReturn(ConfigurationMgr().GetProductId(productId) == CHIP_NO_ERROR); OtaRequestorServerOnVersionApplied(version, productId); ConnectToProvider(kNotifyUpdateApplied); @@ -491,28 +491,39 @@ CHIP_ERROR OTARequestor::SendQueryImageRequest(OperationalDeviceProxy & devicePr constexpr OTADownloadProtocol kProtocolsSupported[] = { OTADownloadProtocol::kBDXSynchronous }; QueryImage::Type args; - VerifyOrReturnError(Basic::Attributes::VendorID::Get(kRootEndpointId, &args.vendorId) == EMBER_ZCL_STATUS_SUCCESS, - CHIP_ERROR_READ_FAILED); + uint16_t vendorId; + VerifyOrReturnError(ConfigurationMgr().GetVendorId(vendorId) == CHIP_NO_ERROR, CHIP_ERROR_READ_FAILED); + args.vendorId = static_cast(vendorId); - VerifyOrReturnError(Basic::Attributes::ProductID::Get(kRootEndpointId, &args.productId) == EMBER_ZCL_STATUS_SUCCESS, - CHIP_ERROR_READ_FAILED); + VerifyOrReturnError(ConfigurationMgr().GetProductId(args.productId) == CHIP_NO_ERROR, CHIP_ERROR_READ_FAILED); - VerifyOrReturnError(Basic::Attributes::SoftwareVersion::Get(kRootEndpointId, &args.softwareVersion) == EMBER_ZCL_STATUS_SUCCESS, - CHIP_ERROR_READ_FAILED); + VerifyOrReturnError(ConfigurationMgr().GetSoftwareVersion(args.softwareVersion) == CHIP_NO_ERROR, CHIP_ERROR_READ_FAILED); args.protocolsSupported = kProtocolsSupported; args.requestorCanConsent.SetValue(mOtaRequestorDriver->CanConsent()); uint16_t hardwareVersion; - if (Basic::Attributes::HardwareVersion::Get(kRootEndpointId, &hardwareVersion) == EMBER_ZCL_STATUS_SUCCESS) + if (ConfigurationMgr().GetHardwareVersion(hardwareVersion) == CHIP_NO_ERROR) { args.hardwareVersion.SetValue(hardwareVersion); } char location[DeviceLayer::ConfigurationManager::kMaxLocationLength]; - if (Basic::Attributes::Location::Get(kRootEndpointId, MutableCharSpan(location)) == EMBER_ZCL_STATUS_SUCCESS) + size_t codeLen = 0; + if (ConfigurationMgr().GetCountryCode(location, sizeof(location), codeLen) == CHIP_NO_ERROR) { - args.location.SetValue(CharSpan(location)); + if (codeLen == 0) + { + args.location.SetValue(CharSpan("XX", DeviceLayer::ConfigurationManager::kMaxLocationLength)); + } + else + { + args.location.SetValue(CharSpan(location, DeviceLayer::ConfigurationManager::kMaxLocationLength)); + } + } + else + { + args.location.SetValue(CharSpan("XX", DeviceLayer::ConfigurationManager::kMaxLocationLength)); } Controller::OtaSoftwareUpdateProviderCluster cluster; diff --git a/src/app/clusters/ota-requestor/OTARequestor.h b/src/app/clusters/ota-requestor/OTARequestor.h index 0f650f78ddd618..fa8e41a2b9926b 100644 --- a/src/app/clusters/ota-requestor/OTARequestor.h +++ b/src/app/clusters/ota-requestor/OTARequestor.h @@ -86,7 +86,7 @@ class OTARequestor : public OTARequestorInterface, public BDXDownloader::StateDe mBdxDownloader = downloader; uint32_t version; - VerifyOrDie(app::Clusters::Basic::Attributes::SoftwareVersion::Get(kRootEndpointId, &version) == EMBER_ZCL_STATUS_SUCCESS); + VerifyOrDie(DeviceLayer::ConfigurationMgr().GetSoftwareVersion(version) == CHIP_NO_ERROR); mCurrentVersion = version; OtaRequestorServerSetUpdateState(mCurrentUpdateState); diff --git a/src/include/platform/ConfigurationManager.h b/src/include/platform/ConfigurationManager.h index a8977307b14aee..f05c9dcc6f3130 100644 --- a/src/include/platform/ConfigurationManager.h +++ b/src/include/platform/ConfigurationManager.h @@ -93,7 +93,7 @@ class ConfigurationManager virtual CHIP_ERROR GetPrimary802154MACAddress(uint8_t * buf) = 0; virtual CHIP_ERROR GetManufacturingDate(uint16_t & year, uint8_t & month, uint8_t & dayOfMonth) = 0; virtual CHIP_ERROR GetSoftwareVersionString(char * buf, size_t bufSize) = 0; - virtual CHIP_ERROR GetSoftwareVersion(uint16_t & softwareVer) = 0; + virtual CHIP_ERROR GetSoftwareVersion(uint32_t & softwareVer) = 0; virtual CHIP_ERROR GetSetupPinCode(uint32_t & setupPinCode) = 0; virtual CHIP_ERROR GetSetupDiscriminator(uint16_t & setupDiscriminator) = 0; // Lifetime counter is monotonic counter that is incremented only in the case of a factory reset diff --git a/src/include/platform/internal/GenericConfigurationManagerImpl.h b/src/include/platform/internal/GenericConfigurationManagerImpl.h index e498eb0e60854e..f084aa77badfe4 100644 --- a/src/include/platform/internal/GenericConfigurationManagerImpl.h +++ b/src/include/platform/internal/GenericConfigurationManagerImpl.h @@ -61,7 +61,7 @@ class GenericConfigurationManagerImpl : public ConfigurationManager CHIP_ERROR GetHardwareVersion(uint16_t & hardwareVer) override; CHIP_ERROR StoreHardwareVersion(uint16_t hardwareVer) override; CHIP_ERROR GetSoftwareVersionString(char * buf, size_t bufSize) override; - CHIP_ERROR GetSoftwareVersion(uint16_t & softwareVer) override; + CHIP_ERROR GetSoftwareVersion(uint32_t & softwareVer) override; CHIP_ERROR GetSerialNumber(char * buf, size_t bufSize) override; CHIP_ERROR StoreSerialNumber(const char * serialNum, size_t serialNumLen) override; CHIP_ERROR GetPrimaryMACAddress(MutableByteSpan buf) override; @@ -159,7 +159,7 @@ inline CHIP_ERROR GenericConfigurationManagerImpl::GetProductId(uin } template -inline CHIP_ERROR GenericConfigurationManagerImpl::GetSoftwareVersion(uint16_t & softwareVer) +inline CHIP_ERROR GenericConfigurationManagerImpl::GetSoftwareVersion(uint32_t & softwareVer) { softwareVer = static_cast(CHIP_DEVICE_CONFIG_DEVICE_SOFTWARE_VERSION); return CHIP_NO_ERROR; diff --git a/src/platform/Linux/PlatformManagerImpl.cpp b/src/platform/Linux/PlatformManagerImpl.cpp index 9c93e687d49f52..9bb1bf10163299 100644 --- a/src/platform/Linux/PlatformManagerImpl.cpp +++ b/src/platform/Linux/PlatformManagerImpl.cpp @@ -395,7 +395,7 @@ void PlatformManagerImpl::HandleDeviceRebooted(intptr_t arg) // The StartUp event SHALL be emitted by a Node after completing a boot or reboot process if (platformManagerDelegate != nullptr) { - uint16_t softwareVersion; + uint32_t softwareVersion; ReturnOnFailure(ConfigurationMgr().GetSoftwareVersion(softwareVersion)); platformManagerDelegate->OnStartUp(softwareVersion); diff --git a/src/platform/android/ConfigurationManagerImpl.cpp b/src/platform/android/ConfigurationManagerImpl.cpp index 446d46d34be36a..65f6d18ad80941 100644 --- a/src/platform/android/ConfigurationManagerImpl.cpp +++ b/src/platform/android/ConfigurationManagerImpl.cpp @@ -195,7 +195,7 @@ CHIP_ERROR ConfigurationManagerImpl::GetProductName(char * buf, size_t bufSize) return CHIP_NO_ERROR; } -CHIP_ERROR ConfigurationManagerImpl::GetSoftwareVersion(uint16_t & softwareVer) +CHIP_ERROR ConfigurationManagerImpl::GetSoftwareVersion(uint32_t & softwareVer) { CHIP_ERROR err; uint32_t u32SoftwareVer = 0; diff --git a/src/platform/android/ConfigurationManagerImpl.h b/src/platform/android/ConfigurationManagerImpl.h index 75aa7b74393b45..5f674da0d50786 100644 --- a/src/platform/android/ConfigurationManagerImpl.h +++ b/src/platform/android/ConfigurationManagerImpl.h @@ -44,7 +44,7 @@ class ConfigurationManagerImpl : public Internal::GenericConfigurationManagerImp CHIP_ERROR GetProductName(char * buf, size_t bufSize) override; CHIP_ERROR GetHardwareVersionString(char * buf, size_t bufSize) override; CHIP_ERROR GetSoftwareVersionString(char * buf, size_t bufSize) override; - CHIP_ERROR GetSoftwareVersion(uint16_t & softwareVer) override; + CHIP_ERROR GetSoftwareVersion(uint32_t & softwareVer) override; CHIP_ERROR GetNodeLabel(char * buf, size_t bufSize) override; CHIP_ERROR StoreNodeLabel(const char * buf, size_t bufSize) override; CHIP_ERROR GetPartNumber(char * buf, size_t bufSize) override; diff --git a/src/platform/fake/ConfigurationManagerImpl.h b/src/platform/fake/ConfigurationManagerImpl.h index 88d0f452618541..4a0182b6a11e90 100644 --- a/src/platform/fake/ConfigurationManagerImpl.h +++ b/src/platform/fake/ConfigurationManagerImpl.h @@ -43,7 +43,7 @@ class ConfigurationManagerImpl : public ConfigurationManager CHIP_ERROR GetHardwareVersion(uint16_t & hardwareVer) override { return CHIP_ERROR_NOT_IMPLEMENTED; } CHIP_ERROR StoreHardwareVersion(uint16_t hardwareVer) override { return CHIP_ERROR_NOT_IMPLEMENTED; } CHIP_ERROR GetSoftwareVersionString(char * buf, size_t bufSize) override { return CHIP_ERROR_NOT_IMPLEMENTED; } - CHIP_ERROR GetSoftwareVersion(uint16_t & softwareVer) override { return CHIP_ERROR_NOT_IMPLEMENTED; } + CHIP_ERROR GetSoftwareVersion(uint32_t & softwareVer) override { return CHIP_ERROR_NOT_IMPLEMENTED; } CHIP_ERROR GetSerialNumber(char * buf, size_t bufSize) override { return CHIP_ERROR_NOT_IMPLEMENTED; } CHIP_ERROR StoreSerialNumber(const char * serialNum, size_t serialNumLen) override { return CHIP_ERROR_NOT_IMPLEMENTED; } CHIP_ERROR GetPrimaryMACAddress(MutableByteSpan buf) override { return CHIP_ERROR_NOT_IMPLEMENTED; } From 7d1ad12c3428d99c35801fe528515defa0b603a3 Mon Sep 17 00:00:00 2001 From: Carol Yang Date: Mon, 24 Jan 2022 11:44:51 -0800 Subject: [PATCH 2/3] Address code review comments - Use DeviceLayer namespace directly - Return actual error from failing ConfigurationManager calls - Refactor reading of country code from OTA --- .../clusters/ota-requestor/OTARequestor.cpp | 30 +++++++------------ src/app/clusters/ota-requestor/OTARequestor.h | 2 +- 2 files changed, 11 insertions(+), 21 deletions(-) diff --git a/src/app/clusters/ota-requestor/OTARequestor.cpp b/src/app/clusters/ota-requestor/OTARequestor.cpp index bdea99609c8d45..59477d7edab102 100644 --- a/src/app/clusters/ota-requestor/OTARequestor.cpp +++ b/src/app/clusters/ota-requestor/OTARequestor.cpp @@ -36,7 +36,6 @@ using namespace app::Clusters::OtaSoftwareUpdateProvider; using namespace app::Clusters::OtaSoftwareUpdateProvider::Commands; using namespace app::Clusters::OtaSoftwareUpdateRequestor; using namespace app::Clusters::OtaSoftwareUpdateRequestor::Commands; -using namespace DeviceLayer; using app::DataModel::Nullable; using bdx::TransferSession; @@ -393,7 +392,7 @@ void OTARequestor::NotifyUpdateApplied(uint32_t version) // Log the VersionApplied event uint16_t productId; - VerifyOrReturn(ConfigurationMgr().GetProductId(productId) == CHIP_NO_ERROR); + VerifyOrReturn(DeviceLayer::ConfigurationMgr().GetProductId(productId) == CHIP_NO_ERROR); OtaRequestorServerOnVersionApplied(version, productId); ConnectToProvider(kNotifyUpdateApplied); @@ -492,39 +491,30 @@ CHIP_ERROR OTARequestor::SendQueryImageRequest(OperationalDeviceProxy & devicePr QueryImage::Type args; uint16_t vendorId; - VerifyOrReturnError(ConfigurationMgr().GetVendorId(vendorId) == CHIP_NO_ERROR, CHIP_ERROR_READ_FAILED); + ReturnErrorOnFailure(DeviceLayer::ConfigurationMgr().GetVendorId(vendorId)); args.vendorId = static_cast(vendorId); - VerifyOrReturnError(ConfigurationMgr().GetProductId(args.productId) == CHIP_NO_ERROR, CHIP_ERROR_READ_FAILED); + ReturnErrorOnFailure(DeviceLayer::ConfigurationMgr().GetProductId(args.productId)); - VerifyOrReturnError(ConfigurationMgr().GetSoftwareVersion(args.softwareVersion) == CHIP_NO_ERROR, CHIP_ERROR_READ_FAILED); + ReturnErrorOnFailure(DeviceLayer::ConfigurationMgr().GetSoftwareVersion(args.softwareVersion)); args.protocolsSupported = kProtocolsSupported; args.requestorCanConsent.SetValue(mOtaRequestorDriver->CanConsent()); uint16_t hardwareVersion; - if (ConfigurationMgr().GetHardwareVersion(hardwareVersion) == CHIP_NO_ERROR) + if (DeviceLayer::ConfigurationMgr().GetHardwareVersion(hardwareVersion) == CHIP_NO_ERROR) { args.hardwareVersion.SetValue(hardwareVersion); } - char location[DeviceLayer::ConfigurationManager::kMaxLocationLength]; + char location[DeviceLayer::ConfigurationManager::kMaxLocationLength] = { 'X', 'X' }; + char countryCode[DeviceLayer::ConfigurationManager::kMaxLocationLength]; size_t codeLen = 0; - if (ConfigurationMgr().GetCountryCode(location, sizeof(location), codeLen) == CHIP_NO_ERROR) + if ((DeviceLayer::ConfigurationMgr().GetCountryCode(countryCode, sizeof(countryCode), codeLen) == CHIP_NO_ERROR) && (codeLen > 0)) { - if (codeLen == 0) - { - args.location.SetValue(CharSpan("XX", DeviceLayer::ConfigurationManager::kMaxLocationLength)); - } - else - { - args.location.SetValue(CharSpan(location, DeviceLayer::ConfigurationManager::kMaxLocationLength)); - } - } - else - { - args.location.SetValue(CharSpan("XX", DeviceLayer::ConfigurationManager::kMaxLocationLength)); + strncpy(location, countryCode, codeLen); } + args.location.SetValue(CharSpan(location, strlen(location))); Controller::OtaSoftwareUpdateProviderCluster cluster; cluster.Associate(&deviceProxy, mProviderEndpointId); diff --git a/src/app/clusters/ota-requestor/OTARequestor.h b/src/app/clusters/ota-requestor/OTARequestor.h index fa8e41a2b9926b..8e48d3ef90bf40 100644 --- a/src/app/clusters/ota-requestor/OTARequestor.h +++ b/src/app/clusters/ota-requestor/OTARequestor.h @@ -86,7 +86,7 @@ class OTARequestor : public OTARequestorInterface, public BDXDownloader::StateDe mBdxDownloader = downloader; uint32_t version; - VerifyOrDie(DeviceLayer::ConfigurationMgr().GetSoftwareVersion(version) == CHIP_NO_ERROR); + ReturnOnFailure(DeviceLayer::ConfigurationMgr().GetSoftwareVersion(version)); mCurrentVersion = version; OtaRequestorServerSetUpdateState(mCurrentUpdateState); From 7cc6b50e41bd58e221514cfd07c4f834fe617f9f Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Mon, 24 Jan 2022 19:47:14 +0000 Subject: [PATCH 3/3] Restyled by clang-format --- src/app/clusters/ota-requestor/OTARequestor.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/app/clusters/ota-requestor/OTARequestor.cpp b/src/app/clusters/ota-requestor/OTARequestor.cpp index 59477d7edab102..de5084a92b11ef 100644 --- a/src/app/clusters/ota-requestor/OTARequestor.cpp +++ b/src/app/clusters/ota-requestor/OTARequestor.cpp @@ -510,7 +510,8 @@ CHIP_ERROR OTARequestor::SendQueryImageRequest(OperationalDeviceProxy & devicePr char location[DeviceLayer::ConfigurationManager::kMaxLocationLength] = { 'X', 'X' }; char countryCode[DeviceLayer::ConfigurationManager::kMaxLocationLength]; size_t codeLen = 0; - if ((DeviceLayer::ConfigurationMgr().GetCountryCode(countryCode, sizeof(countryCode), codeLen) == CHIP_NO_ERROR) && (codeLen > 0)) + if ((DeviceLayer::ConfigurationMgr().GetCountryCode(countryCode, sizeof(countryCode), codeLen) == CHIP_NO_ERROR) && + (codeLen > 0)) { strncpy(location, countryCode, codeLen); }