From a2fa43488d3a3418d353d0f2bc0095b974170d46 Mon Sep 17 00:00:00 2001 From: chaitanya jandhyala Date: Thu, 9 May 2024 11:44:19 -0700 Subject: [PATCH] fixed review comments, changed functions to taking spans, not separate pointer/length arguments --- .../platform/linux/CommissionableInit.cpp | 26 ++++++++------- examples/platform/linux/Options.cpp | 10 +++--- examples/platform/linux/Options.h | 2 ++ src/include/platform/ConfigurationManager.h | 11 +++---- .../GenericConfigurationManagerImpl.h | 10 +++--- .../GenericConfigurationManagerImpl.ipp | 33 ++++++++++--------- .../GenericDeviceInstanceInfoProvider.ipp | 20 ++++++----- src/platform/fake/ConfigurationManagerImpl.h | 2 +- src/platform/tests/TestConfigurationMgr.cpp | 6 ++-- 9 files changed, 65 insertions(+), 55 deletions(-) diff --git a/examples/platform/linux/CommissionableInit.cpp b/examples/platform/linux/CommissionableInit.cpp index 14414264f92fcf..effb511c85165f 100644 --- a/examples/platform/linux/CommissionableInit.cpp +++ b/examples/platform/linux/CommissionableInit.cpp @@ -102,31 +102,33 @@ CHIP_ERROR InitConfigurationManager(ConfigurationManagerImpl & configManager, Li if (options.vendorName.HasValue()) { - const char * vendorName = options.vendorName.Value().c_str(); - configManager.StoreVendorName(vendorName, strlen(vendorName)); + chip::Span vendor_name(options.vendorName.Value().c_str(), options.vendorName.Value().size()); + VerifyOrDie(configManager.StoreVendorName(vendor_name) == CHIP_NO_ERROR); } if (options.productName.HasValue()) { - const char * productName = options.productName.Value().c_str(); - configManager.StoreProductName(productName,strlen(productName)); + chip::Span product_name(options.productName.Value().c_str(), options.productName.Value().size()); + VerifyOrDie(configManager.StoreProductName(product_name) == CHIP_NO_ERROR); } if (options.hardwareVersionString.HasValue()) { - const char * hardwareVersionString = options.hardwareVersionString.Value().c_str(); - configManager.StoreHardwareVersionString(hardwareVersionString,strlen(hardwareVersionString)); + chip::Span hardware_version_string(options.hardwareVersionString.Value().c_str(), + options.hardwareVersionString.Value().size()); + VerifyOrDie(configManager.StoreHardwareVersionString(hardware_version_string) == CHIP_NO_ERROR); } if (options.softwareVersionString.HasValue()) { - const char * softwareVersionString = options.softwareVersionString.Value().c_str(); - configManager.StoreSoftwareVersionString(softwareVersionString,strlen(softwareVersionString)); + chip::Span software_version_string(options.softwareVersionString.Value().c_str(), + options.softwareVersionString.Value().size()); + VerifyOrDie(configManager.StoreSoftwareVersionString(software_version_string) == CHIP_NO_ERROR); } - + if (options.serialNumber.HasValue()) { - const char * serialNumber = options.serialNumber.Value().c_str(); - configManager.StoreSerialNumber(serialNumber, strlen(serialNumber)); + chip::Span serial_number(options.serialNumber.Value().c_str(), options.serialNumber.Value().size()); + VerifyOrDie(configManager.StoreSerialNumber(serial_number) == CHIP_NO_ERROR); } - + return CHIP_NO_ERROR; } diff --git a/examples/platform/linux/Options.cpp b/examples/platform/linux/Options.cpp index 19098c0d657d9f..4efdb66cb8c91c 100644 --- a/examples/platform/linux/Options.cpp +++ b/examples/platform/linux/Options.cpp @@ -215,19 +215,19 @@ const char * sDeviceOptionHelp = " --product-id \n" " The Product ID is specified by vendor.\n" "\n" - " --vendor-name \n" + " --vendor-name \n" " The vendor name specified by the vendor.\n" "\n" - " --product-name \n" + " --product-name \n" " The product name specified by vendor.\n" "\n" - " --hardware-version-string \n" + " --hardware-version-string \n" " The hardware version string specified by vendor.\n" "\n" - " --software-version-string \n" + " --software-version-string \n" " The software version string specified by vendor.\n" "\n" - " --serial-number \n" + " --serial-number \n" " The serial number specified by vendor.\n" "\n" " --custom-flow \n" diff --git a/examples/platform/linux/Options.h b/examples/platform/linux/Options.h index c3b8af9fa1880f..30c630bccf63c5 100644 --- a/examples/platform/linux/Options.h +++ b/examples/platform/linux/Options.h @@ -80,7 +80,9 @@ struct LinuxDeviceOptions chip::Optional productName; chip::Optional hardwareVersionString; chip::Optional softwareVersionString; +#if defined(CHIP_DEVICE_CONFIG_TEST_SERIAL_NUMBER) chip::Optional serialNumber; +#endif static LinuxDeviceOptions & GetInstance(); }; diff --git a/src/include/platform/ConfigurationManager.h b/src/include/platform/ConfigurationManager.h index bed6eea5cadb80..e00a1ab19c99d2 100644 --- a/src/include/platform/ConfigurationManager.h +++ b/src/include/platform/ConfigurationManager.h @@ -114,7 +114,6 @@ class ConfigurationManager #endif virtual CHIP_ERROR GetRegulatoryLocation(uint8_t & location) = 0; virtual CHIP_ERROR GetCountryCode(char * buf, size_t bufSize, size_t & codeLen) = 0; - virtual CHIP_ERROR StoreSerialNumber(const char * serialNum, size_t serialNumLen) = 0; virtual CHIP_ERROR StoreManufacturingDate(const char * mfgDate, size_t mfgDateLen) = 0; virtual CHIP_ERROR StoreSoftwareVersion(uint32_t softwareVer) = 0; virtual CHIP_ERROR StoreHardwareVersion(uint16_t hardwareVer) = 0; @@ -133,11 +132,11 @@ class ConfigurationManager virtual CHIP_ERROR SetFailSafeArmed(bool val) = 0; virtual CHIP_ERROR GetBLEDeviceIdentificationInfo(Ble::ChipBLEDeviceIdentificationInfo & deviceIdInfo) = 0; - - virtual CHIP_ERROR StoreVendorName(const char * vendorName, size_t vendorNameLen) = 0; - virtual CHIP_ERROR StoreProductName(const char * productName, size_t productNameLen) = 0; - virtual CHIP_ERROR StoreHardwareVersionString(const char * hardwareVersionString, size_t hardwareVersionStringLen) = 0; - virtual CHIP_ERROR StoreSoftwareVersionString(const char * softwareVersionString, size_t softwareVersionStringLen) = 0; + virtual CHIP_ERROR StoreSerialNumber(chip::Span serialNumber) = 0; + virtual CHIP_ERROR StoreVendorName(chip::Span vendorName) = 0; + virtual CHIP_ERROR StoreProductName(chip::Span productName) = 0; + virtual CHIP_ERROR StoreHardwareVersionString(chip::Span hardwareVersionString) = 0; + virtual CHIP_ERROR StoreSoftwareVersionString(chip::Span softwareVersionString) = 0; #if CHIP_CONFIG_TEST virtual void RunUnitTests() = 0; diff --git a/src/include/platform/internal/GenericConfigurationManagerImpl.h b/src/include/platform/internal/GenericConfigurationManagerImpl.h index 22189acd9b23e4..4a5c2dab475ac8 100644 --- a/src/include/platform/internal/GenericConfigurationManagerImpl.h +++ b/src/include/platform/internal/GenericConfigurationManagerImpl.h @@ -68,7 +68,7 @@ class GenericConfigurationManagerImpl : public ConfigurationManager CHIP_ERROR StoreSoftwareVersion(uint32_t softwareVer) override; CHIP_ERROR GetFirmwareBuildChipEpochTime(System::Clock::Seconds32 & buildTime) override; CHIP_ERROR SetFirmwareBuildChipEpochTime(System::Clock::Seconds32 buildTime) override; - CHIP_ERROR StoreSerialNumber(const char * serialNum, size_t serialNumLen) override; + CHIP_ERROR StoreSerialNumber(chip::Span serialNumber) override; CHIP_ERROR GetPrimaryMACAddress(MutableByteSpan buf) override; CHIP_ERROR GetPrimaryWiFiMACAddress(uint8_t * buf) override; CHIP_ERROR GetPrimary802154MACAddress(uint8_t * buf) override; @@ -104,10 +104,10 @@ class GenericConfigurationManagerImpl : public ConfigurationManager CHIP_ERROR StoreUniqueId(const char * uniqueId, size_t uniqueIdLen) override; CHIP_ERROR GenerateUniqueId(char * buf, size_t bufSize) override; - CHIP_ERROR StoreVendorName(const char * vendorName, size_t vendorNameLen) override; - CHIP_ERROR StoreProductName(const char * productName, size_t productNameLen) override; - CHIP_ERROR StoreHardwareVersionString(const char * hardwareVersionString, size_t hardwareVersionStringLen) override; - CHIP_ERROR StoreSoftwareVersionString(const char * softwareVersionString, size_t softwareVersionStringLen) override; + CHIP_ERROR StoreVendorName(chip::Span vendorName) override; + CHIP_ERROR StoreProductName(chip::Span productName) override; + CHIP_ERROR StoreHardwareVersionString(chip::Span hardwareVersionString) override; + CHIP_ERROR StoreSoftwareVersionString(chip::Span softwareVersionString) override; #if CHIP_CONFIG_TEST void RunUnitTests() override; #endif diff --git a/src/include/platform/internal/GenericConfigurationManagerImpl.ipp b/src/include/platform/internal/GenericConfigurationManagerImpl.ipp index 05148a6dc460ef..bd3ab9d0e9c67f 100644 --- a/src/include/platform/internal/GenericConfigurationManagerImpl.ipp +++ b/src/include/platform/internal/GenericConfigurationManagerImpl.ipp @@ -351,20 +351,20 @@ CHIP_ERROR GenericConfigurationManagerImpl::GetSecondaryPairingHint template CHIP_ERROR GenericConfigurationManagerImpl::GetSoftwareVersionString(char * buf, size_t bufSize) { - ChipError err = CHIP_NO_ERROR; + ChipError err = CHIP_NO_ERROR; size_t softwareVersionStringLen = 0; // without counting null-terminator err = ReadConfigValueStr(ConfigClass::kConfigKey_SoftwareVersionString, buf, bufSize, softwareVersionStringLen); -#ifdef CHIP_DEVICE_CONFIG_DEVICE_SOFTWARE_VERSION_STRING + if (CHIP_DEVICE_CONFIG_DEVICE_SOFTWARE_VERSION_STRING[0] != 0 && err == CHIP_DEVICE_ERROR_CONFIG_NOT_FOUND) { ReturnErrorCodeIf(bufSize < sizeof(CHIP_DEVICE_CONFIG_DEVICE_SOFTWARE_VERSION_STRING), CHIP_ERROR_BUFFER_TOO_SMALL); memcpy(buf, CHIP_DEVICE_CONFIG_DEVICE_SOFTWARE_VERSION_STRING, sizeof(CHIP_DEVICE_CONFIG_DEVICE_SOFTWARE_VERSION_STRING)); softwareVersionStringLen = sizeof(CHIP_DEVICE_CONFIG_DEVICE_SOFTWARE_VERSION_STRING) - 1; - err = CHIP_NO_ERROR; + err = CHIP_NO_ERROR; } -#endif - ReturnErrorOnFailure(err); + + VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_ERROR_INTERNAL); ReturnErrorCodeIf(softwareVersionStringLen >= bufSize, CHIP_ERROR_BUFFER_TOO_SMALL); ReturnErrorCodeIf(buf[softwareVersionStringLen] != 0, CHIP_ERROR_INVALID_STRING_LENGTH); @@ -373,29 +373,32 @@ CHIP_ERROR GenericConfigurationManagerImpl::GetSoftwareVersionStrin } template -CHIP_ERROR GenericConfigurationManagerImpl::StoreSerialNumber(const char * serialNum, size_t serialNumLen) +CHIP_ERROR GenericConfigurationManagerImpl::StoreSerialNumber(chip::Span serialNumber) { - return WriteConfigValueStr(ConfigClass::kConfigKey_SerialNum, serialNum, serialNumLen); + return WriteConfigValueStr(ConfigClass::kConfigKey_SerialNum, serialNumber.data(), serialNumber.size()); } template -CHIP_ERROR GenericConfigurationManagerImpl::StoreVendorName(const char * vendorName, size_t vendorNameLen) +CHIP_ERROR GenericConfigurationManagerImpl::StoreVendorName(chip::Span vendorName) { - return WriteConfigValueStr(ConfigClass::kConfigKey_VendorName, vendorName, vendorNameLen); + + return WriteConfigValueStr(ConfigClass::kConfigKey_VendorName, vendorName.data(), vendorName.size()); } template -CHIP_ERROR GenericConfigurationManagerImpl::StoreProductName(const char * productName, size_t productNameLen) +CHIP_ERROR GenericConfigurationManagerImpl::StoreProductName(chip::Span productName) { - return WriteConfigValueStr(ConfigClass::kConfigKey_ProductName, productName, productNameLen); + return WriteConfigValueStr(ConfigClass::kConfigKey_ProductName, productName.data(), productName.size()); } template -CHIP_ERROR GenericConfigurationManagerImpl::StoreHardwareVersionString(const char * hardwareVersionString, size_t hardwareVersionStringLen) +CHIP_ERROR GenericConfigurationManagerImpl::StoreHardwareVersionString(chip::Span hardwareVersionString) { - return WriteConfigValueStr(ConfigClass::kConfigKey_HardwareVersionString, hardwareVersionString, hardwareVersionStringLen); + return WriteConfigValueStr(ConfigClass::kConfigKey_HardwareVersionString, hardwareVersionString.data(), + hardwareVersionString.size()); } template -CHIP_ERROR GenericConfigurationManagerImpl::StoreSoftwareVersionString(const char * softwareVersionString, size_t softwareVersionStringLen) +CHIP_ERROR GenericConfigurationManagerImpl::StoreSoftwareVersionString(chip::Span softwareVersionString) { - return WriteConfigValueStr(ConfigClass::kConfigKey_SoftwareVersionString, softwareVersionString, softwareVersionStringLen); + return WriteConfigValueStr(ConfigClass::kConfigKey_SoftwareVersionString, softwareVersionString.data(), + softwareVersionString.size()); } template diff --git a/src/include/platform/internal/GenericDeviceInstanceInfoProvider.ipp b/src/include/platform/internal/GenericDeviceInstanceInfoProvider.ipp index a64c633e9011a0..474117680ddeff 100644 --- a/src/include/platform/internal/GenericDeviceInstanceInfoProvider.ipp +++ b/src/include/platform/internal/GenericDeviceInstanceInfoProvider.ipp @@ -38,7 +38,7 @@ CHIP_ERROR GenericDeviceInstanceInfoProvider::GetProductId(uint16_t template CHIP_ERROR GenericDeviceInstanceInfoProvider::GetVendorName(char * buf, size_t bufSize) { - ChipError err = CHIP_NO_ERROR; + ChipError err = CHIP_NO_ERROR; size_t vendorNameLen = 0; // without counting null-terminator err = mGenericConfigManager.ReadConfigValueStr(ConfigClass::kConfigKey_VendorName, buf, bufSize, vendorNameLen); @@ -48,7 +48,7 @@ CHIP_ERROR GenericDeviceInstanceInfoProvider::GetVendorName(char * ReturnErrorCodeIf(bufSize < sizeof(CHIP_DEVICE_CONFIG_DEVICE_VENDOR_NAME), CHIP_ERROR_BUFFER_TOO_SMALL); memcpy(buf, CHIP_DEVICE_CONFIG_DEVICE_VENDOR_NAME, sizeof(CHIP_DEVICE_CONFIG_DEVICE_VENDOR_NAME)); vendorNameLen = sizeof(CHIP_DEVICE_CONFIG_DEVICE_VENDOR_NAME) - 1; - err = CHIP_NO_ERROR; + err = CHIP_NO_ERROR; } #endif ReturnErrorOnFailure(err); @@ -62,7 +62,7 @@ CHIP_ERROR GenericDeviceInstanceInfoProvider::GetVendorName(char * template CHIP_ERROR GenericDeviceInstanceInfoProvider::GetProductName(char * buf, size_t bufSize) { - ChipError err = CHIP_NO_ERROR; + ChipError err = CHIP_NO_ERROR; size_t productNameLen = 0; // without counting null-terminator err = mGenericConfigManager.ReadConfigValueStr(ConfigClass::kConfigKey_ProductName, buf, bufSize, productNameLen); @@ -73,7 +73,7 @@ CHIP_ERROR GenericDeviceInstanceInfoProvider::GetProductName(char * ReturnErrorCodeIf(bufSize < sizeof(CHIP_DEVICE_CONFIG_DEVICE_PRODUCT_NAME), CHIP_ERROR_BUFFER_TOO_SMALL); memcpy(buf, CHIP_DEVICE_CONFIG_DEVICE_PRODUCT_NAME, sizeof(CHIP_DEVICE_CONFIG_DEVICE_PRODUCT_NAME)); productNameLen = sizeof(CHIP_DEVICE_CONFIG_DEVICE_PRODUCT_NAME) - 1; - err = CHIP_NO_ERROR; + err = CHIP_NO_ERROR; } #endif ReturnErrorOnFailure(err); @@ -208,18 +208,20 @@ CHIP_ERROR GenericDeviceInstanceInfoProvider::GetHardwareVersion(ui template CHIP_ERROR GenericDeviceInstanceInfoProvider::GetHardwareVersionString(char * buf, size_t bufSize) { - ChipError err = CHIP_NO_ERROR; + ChipError err = CHIP_NO_ERROR; size_t hardwareVersionStringLen = 0; // without counting null-terminator - err = mGenericConfigManager.ReadConfigValueStr(ConfigClass::kConfigKey_HardwareVersionString, buf, bufSize, hardwareVersionStringLen); + err = mGenericConfigManager.ReadConfigValueStr(ConfigClass::kConfigKey_HardwareVersionString, buf, bufSize, + hardwareVersionStringLen); #ifdef CHIP_DEVICE_CONFIG_DEFAULT_DEVICE_HARDWARE_VERSION_STRING if (CHIP_DEVICE_CONFIG_DEFAULT_DEVICE_HARDWARE_VERSION_STRING[0] != 0 && err == CHIP_DEVICE_ERROR_CONFIG_NOT_FOUND) { ReturnErrorCodeIf(bufSize < sizeof(CHIP_DEVICE_CONFIG_DEFAULT_DEVICE_HARDWARE_VERSION_STRING), CHIP_ERROR_BUFFER_TOO_SMALL); - memcpy(buf, CHIP_DEVICE_CONFIG_DEFAULT_DEVICE_HARDWARE_VERSION_STRING, sizeof(CHIP_DEVICE_CONFIG_DEFAULT_DEVICE_HARDWARE_VERSION_STRING)); - hardwareVersionStringLen = sizeof(CHIP_DEVICE_CONFIG_TEST_SERIAL_NUMBER) - 1; - err = CHIP_NO_ERROR; + memcpy(buf, CHIP_DEVICE_CONFIG_DEFAULT_DEVICE_HARDWARE_VERSION_STRING, + sizeof(CHIP_DEVICE_CONFIG_DEFAULT_DEVICE_HARDWARE_VERSION_STRING)); + hardwareVersionStringLen = sizeof(CHIP_DEVICE_CONFIG_DEFAULT_DEVICE_HARDWARE_VERSION_STRING) - 1; + err = CHIP_NO_ERROR; } #endif ReturnErrorOnFailure(err); diff --git a/src/platform/fake/ConfigurationManagerImpl.h b/src/platform/fake/ConfigurationManagerImpl.h index 510b2fc0dacc83..98da2c3c88ee22 100644 --- a/src/platform/fake/ConfigurationManagerImpl.h +++ b/src/platform/fake/ConfigurationManagerImpl.h @@ -49,7 +49,7 @@ class ConfigurationManagerImpl : public ConfigurationManager } CHIP_ERROR GetSoftwareVersion(uint32_t & softwareVer) override { return CHIP_ERROR_NOT_IMPLEMENTED; } CHIP_ERROR StoreSoftwareVersion(uint32_t softwareVer) override { return CHIP_ERROR_NOT_IMPLEMENTED; } - CHIP_ERROR StoreSerialNumber(const char * serialNum, size_t serialNumLen) override { return CHIP_ERROR_NOT_IMPLEMENTED; } + CHIP_ERROR StoreSerialNumber(chip::Span serialNumber) override { return CHIP_ERROR_NOT_IMPLEMENTED; } CHIP_ERROR GetPrimaryMACAddress(MutableByteSpan buf) override { return CHIP_ERROR_NOT_IMPLEMENTED; } CHIP_ERROR GetPrimaryWiFiMACAddress(uint8_t * buf) override { return CHIP_ERROR_NOT_IMPLEMENTED; } CHIP_ERROR GetPrimary802154MACAddress(uint8_t * buf) override { return CHIP_ERROR_NOT_IMPLEMENTED; } diff --git a/src/platform/tests/TestConfigurationMgr.cpp b/src/platform/tests/TestConfigurationMgr.cpp index ad60d44583250f..2d6b710463029a 100644 --- a/src/platform/tests/TestConfigurationMgr.cpp +++ b/src/platform/tests/TestConfigurationMgr.cpp @@ -83,8 +83,9 @@ TEST_F(TestConfigurationMgr, SerialNumber) char buf[64]; const char * serialNumber = "89051AAZZ236"; + chip::Span serial_number(serialNumber.Value().c_str(), serialNumber.Value().size()); - err = ConfigurationMgr().StoreSerialNumber(serialNumber, strlen(serialNumber)); + err = ConfigurationMgr().StoreSerialNumber(serial_number); EXPECT_EQ(err, CHIP_NO_ERROR); err = GetDeviceInstanceInfoProvider()->GetSerialNumber(buf, 64); @@ -93,7 +94,8 @@ TEST_F(TestConfigurationMgr, SerialNumber) EXPECT_EQ(strlen(buf), 12u); EXPECT_STREQ(buf, serialNumber); - err = ConfigurationMgr().StoreSerialNumber(serialNumber, 5); + chip::Span serial_number(serialNumber.Value().c_str(), 5); + err = ConfigurationMgr().StoreSerialNumber(serial_number); EXPECT_EQ(err, CHIP_NO_ERROR); err = GetDeviceInstanceInfoProvider()->GetSerialNumber(buf, 64);