From 0c9daf66eebc312086ff358a4ebc644696af3c82 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 15 Aug 2024 13:56:11 +0000 Subject: [PATCH 1/8] Add FabricIndex to ECOINFO attributes for fabric-scoping to work --- .../ecosystem-information-server.cpp | 41 +++++++++++++------ .../ecosystem-information-server.h | 30 +++++++------- 2 files changed, 43 insertions(+), 28 deletions(-) diff --git a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp index c51a5165f3ca9d..9e5ced0e48aad2 100644 --- a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp +++ b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp @@ -128,6 +128,13 @@ EcosystemDeviceStruct::Builder & EcosystemDeviceStruct::Builder::AddUniqueLocati return *this; } +EcosystemDeviceStruct::Builder & EcosystemDeviceStruct::Builder::SetFabricIndex(FabricIndex aFabricIndex) +{ + VerifyOrDie(!mIsAlreadyBuilt); + mFabricIndex = aFabricIndex; + return *this; +} + std::unique_ptr EcosystemDeviceStruct::Builder::Build() { VerifyOrReturnValue(!mIsAlreadyBuilt, nullptr, ChipLogError(Zcl, "Build() already called")); @@ -136,6 +143,8 @@ std::unique_ptr EcosystemDeviceStruct::Builder::Build() VerifyOrReturnValue(!mDeviceTypes.empty(), nullptr, ChipLogError(Zcl, "No device types added")); VerifyOrReturnValue(mUniqueLocationIds.size() <= kUniqueLocationIdsListMaxSize, nullptr, ChipLogError(Zcl, "Too many location ids")); + VerifyOrReturnValue(mFabricIndex >= kMinValidFabricIndex, nullptr, ChipLogError(Zcl, "Fabric index is invalid")); + VerifyOrReturnValue(mFabricIndex <= kMaxValidFabricIndex, nullptr, ChipLogError(Zcl, "Fabric index is invalid")); for (auto & locationId : mUniqueLocationIds) { @@ -145,12 +154,12 @@ std::unique_ptr EcosystemDeviceStruct::Builder::Build() // std::make_unique does not have access to private constructor we workaround with using new std::unique_ptr ret{ new EcosystemDeviceStruct( std::move(mDeviceName), mDeviceNameLastEditEpochUs, mBridgedEndpoint, mOriginalEndpoint, std::move(mDeviceTypes), - std::move(mUniqueLocationIds), mUniqueLocationIdsLastEditEpochUs) }; + std::move(mUniqueLocationIds), mUniqueLocationIdsLastEditEpochUs, mFabricIndex) }; mIsAlreadyBuilt = true; return ret; } -CHIP_ERROR EcosystemDeviceStruct::Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, const FabricIndex & aFabricIndex) +CHIP_ERROR EcosystemDeviceStruct::Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder) { Structs::EcosystemDeviceStruct::Type deviceStruct; if (!mDeviceName.empty()) @@ -172,9 +181,7 @@ CHIP_ERROR EcosystemDeviceStruct::Encode(const AttributeValueEncoder::ListEncode deviceStruct.uniqueLocationIDs = DataModel::List(locationIds.data(), locationIds.size()); deviceStruct.uniqueLocationIDsLastEdit = mUniqueLocationIdsLastEditEpochUs; - - // TODO(#33223) this is a hack, use mFabricIndex when it exists. - deviceStruct.SetFabricIndex(aFabricIndex); + deviceStruct.SetFabricIndex(mFabricIndex); return aEncoder.Encode(deviceStruct); } @@ -208,31 +215,41 @@ EcosystemLocationStruct::Builder::SetLocationDescriptorLastEdit(uint64_t aLocati return *this; } +EcosystemLocationStruct::Builder & EcosystemLocationStruct::Builder::SetFabricIndex(FabricIndex aFabricIndex) +{ + VerifyOrDie(!mIsAlreadyBuilt); + mFabricIndex = aFabricIndex; + return *this; +} + + std::unique_ptr EcosystemLocationStruct::Builder::Build() { VerifyOrReturnValue(!mIsAlreadyBuilt, nullptr, ChipLogError(Zcl, "Build() already called")); VerifyOrReturnValue(!mLocationDescriptor.mLocationName.empty(), nullptr, ChipLogError(Zcl, "Must Provided Location Name")); VerifyOrReturnValue(mLocationDescriptor.mLocationName.size() <= kLocationDescriptorNameMaxSize, nullptr, ChipLogError(Zcl, "Must Location Name must be less than 64 bytes")); + VerifyOrReturnValue(mFabricIndex >= kMinValidFabricIndex, nullptr, ChipLogError(Zcl, "Fabric index is invalid")); + VerifyOrReturnValue(mFabricIndex <= kMaxValidFabricIndex, nullptr, ChipLogError(Zcl, "Fabric index is invalid")); // std::make_unique does not have access to private constructor we workaround with using new std::unique_ptr ret{ new EcosystemLocationStruct(std::move(mLocationDescriptor), - mLocationDescriptorLastEditEpochUs) }; + mLocationDescriptorLastEditEpochUs, + mFabricIndex) }; mIsAlreadyBuilt = true; return ret; } CHIP_ERROR EcosystemLocationStruct::Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, - const std::string & aUniqueLocationId, const FabricIndex & aFabricIndex) + const std::string & aUniqueLocationId) { Structs::EcosystemLocationStruct::Type locationStruct; VerifyOrDie(!aUniqueLocationId.empty()); locationStruct.uniqueLocationID = CharSpan(aUniqueLocationId.c_str(), aUniqueLocationId.size()); locationStruct.locationDescriptor = GetEncodableLocationDescriptorStruct(mLocationDescriptor); locationStruct.locationDescriptorLastEdit = mLocationDescriptorLastEditEpochUs; + locationStruct.SetFabricIndex(mFabricIndex); - // TODO(#33223) this is a hack, use mFabricIndex when it exists. - locationStruct.SetFabricIndex(aFabricIndex); return aEncoder.Encode(locationStruct); } @@ -352,11 +369,10 @@ CHIP_ERROR EcosystemInformationServer::EncodeDeviceDirectoryAttribute(EndpointId return aEncoder.EncodeEmptyList(); } - FabricIndex fabricIndex = aEncoder.AccessingFabricIndex(); return aEncoder.EncodeList([&](const auto & encoder) -> CHIP_ERROR { for (auto & device : deviceInfo.mDeviceDirectory) { - ReturnErrorOnFailure(device->Encode(encoder, fabricIndex)); + ReturnErrorOnFailure(device->Encode(encoder)); } return CHIP_NO_ERROR; }); @@ -379,11 +395,10 @@ CHIP_ERROR EcosystemInformationServer::EncodeLocationStructAttribute(EndpointId return aEncoder.EncodeEmptyList(); } - FabricIndex fabricIndex = aEncoder.AccessingFabricIndex(); return aEncoder.EncodeList([&](const auto & encoder) -> CHIP_ERROR { for (auto & [id, device] : deviceInfo.mLocationDirectory) { - ReturnErrorOnFailure(device->Encode(encoder, id, fabricIndex)); + ReturnErrorOnFailure(device->Encode(encoder, id)); } return CHIP_NO_ERROR; }); diff --git a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h index dce12e745bf14e..17c20de89132dc 100644 --- a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h +++ b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h @@ -49,6 +49,7 @@ class EcosystemDeviceStruct Builder & SetOriginalEndpoint(EndpointId aOriginalEndpoint); Builder & AddDeviceType(Structs::DeviceTypeStruct::Type aDeviceType); Builder & AddUniqueLocationId(std::string aUniqueLocationId, uint64_t aUniqueLocationIdsLastEditEpochUs); + Builder & SetFabricIndex(FabricIndex aFabricIndex); // Upon success this object will have moved all ownership of underlying // types to EcosystemDeviceStruct and should not be used afterwards. @@ -62,21 +63,25 @@ class EcosystemDeviceStruct std::vector mDeviceTypes; std::vector mUniqueLocationIds; uint64_t mUniqueLocationIdsLastEditEpochUs = 0; + FabricIndex mFabricIndex = kUndefinedFabricIndex; bool mIsAlreadyBuilt = false; }; - CHIP_ERROR Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, const FabricIndex & aFabricIndex); + CHIP_ERROR Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder); private: // Constructor is intentionally private. This is to ensure that it is only constructed with // values that conform to the spec. explicit EcosystemDeviceStruct(std::string && aDeviceName, uint64_t aDeviceNameLastEditEpochUs, EndpointId aBridgedEndpoint, EndpointId aOriginalEndpoint, std::vector && aDeviceTypes, - std::vector && aUniqueLocationIds, uint64_t aUniqueLocationIdsLastEditEpochUs) : + std::vector && aUniqueLocationIds, uint64_t aUniqueLocationIdsLastEditEpochUs, + FabricIndex aFabricIndex) : mDeviceName(std::move(aDeviceName)), mDeviceNameLastEditEpochUs(aDeviceNameLastEditEpochUs), mBridgedEndpoint(aBridgedEndpoint), mOriginalEndpoint(aOriginalEndpoint), mDeviceTypes(std::move(aDeviceTypes)), - mUniqueLocationIds(std::move(aUniqueLocationIds)), mUniqueLocationIdsLastEditEpochUs(aUniqueLocationIdsLastEditEpochUs) + mUniqueLocationIds(std::move(aUniqueLocationIds)), mUniqueLocationIdsLastEditEpochUs(aUniqueLocationIdsLastEditEpochUs), + mFabricIndex(aFabricIndex) + {} const std::string mDeviceName; @@ -86,10 +91,7 @@ class EcosystemDeviceStruct std::vector mDeviceTypes; std::vector mUniqueLocationIds; uint64_t mUniqueLocationIdsLastEditEpochUs; - // TODO(#33223) This structure needs to contain fabric index to be spec compliant. - // To keep initial PR smaller, we are going to assume that all entries - // here are for any fabric. This will allow follow up PR introducing - // fabric scoped to be more throughly reviewed with focus on fabric scoping. + FabricIndex mFabricIndex; }; struct LocationDescriptorStruct @@ -113,6 +115,7 @@ class EcosystemLocationStruct Builder & SetFloorNumber(std::optional aFloorNumber); Builder & SetAreaTypeTag(std::optional aAreaTypeTag); Builder & SetLocationDescriptorLastEdit(uint64_t aLocationDescriptorLastEditEpochUs); + Builder & SetFabricIndex(FabricIndex aFabricIndex); // Upon success this object will have moved all ownership of underlying // types to EcosystemDeviceStruct and should not be used afterwards. @@ -121,17 +124,17 @@ class EcosystemLocationStruct private: LocationDescriptorStruct mLocationDescriptor; uint64_t mLocationDescriptorLastEditEpochUs = 0; + FabricIndex mFabricIndex = kUndefinedFabricIndex; bool mIsAlreadyBuilt = false; }; - CHIP_ERROR Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, const std::string & aUniqueLocationId, - const FabricIndex & aFabricIndex); + CHIP_ERROR Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, const std::string & aUniqueLocationId); private: // Constructor is intentionally private. This is to ensure that it is only constructed with // values that conform to the spec. - explicit EcosystemLocationStruct(LocationDescriptorStruct && aLocationDescriptor, uint64_t aLocationDescriptorLastEditEpochUs) : - mLocationDescriptor(aLocationDescriptor), mLocationDescriptorLastEditEpochUs(aLocationDescriptorLastEditEpochUs) + explicit EcosystemLocationStruct(LocationDescriptorStruct && aLocationDescriptor, uint64_t aLocationDescriptorLastEditEpochUs, FabricIndex aFabricIndex) : + mLocationDescriptor(aLocationDescriptor), mLocationDescriptorLastEditEpochUs(aLocationDescriptorLastEditEpochUs), mFabricIndex(aFabricIndex) {} // EcosystemLocationStruct is used as a value in a key-value map. // Because UniqueLocationId is manditory when an entry exist, and @@ -139,10 +142,7 @@ class EcosystemLocationStruct // not explicitly in this struct. LocationDescriptorStruct mLocationDescriptor; uint64_t mLocationDescriptorLastEditEpochUs; - // TODO(#33223) This structure needs to contain fabric index to be spec compliant. - // To keep initial PR smaller, we are going to assume that all entries - // here are for any fabric. This will allow follow up PR introducing - // fabric scoped to be more throughly reviewed with focus on fabric scoping. + FabricIndex mFabricIndex; }; class EcosystemInformationServer From cde4be4f774629ff23b39a8ec81f95b6c8192ddc Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 15 Aug 2024 16:51:33 +0000 Subject: [PATCH 2/8] Fix adding fabricindex to LocationDirectory --- .../ecosystem-information-server.cpp | 30 ++++++++----------- .../ecosystem-information-server.h | 26 +++++++++------- 2 files changed, 27 insertions(+), 29 deletions(-) diff --git a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp index 9e5ced0e48aad2..ed87fe37e4a49c 100644 --- a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp +++ b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp @@ -215,40 +215,28 @@ EcosystemLocationStruct::Builder::SetLocationDescriptorLastEdit(uint64_t aLocati return *this; } -EcosystemLocationStruct::Builder & EcosystemLocationStruct::Builder::SetFabricIndex(FabricIndex aFabricIndex) -{ - VerifyOrDie(!mIsAlreadyBuilt); - mFabricIndex = aFabricIndex; - return *this; -} - - std::unique_ptr EcosystemLocationStruct::Builder::Build() { VerifyOrReturnValue(!mIsAlreadyBuilt, nullptr, ChipLogError(Zcl, "Build() already called")); VerifyOrReturnValue(!mLocationDescriptor.mLocationName.empty(), nullptr, ChipLogError(Zcl, "Must Provided Location Name")); VerifyOrReturnValue(mLocationDescriptor.mLocationName.size() <= kLocationDescriptorNameMaxSize, nullptr, ChipLogError(Zcl, "Must Location Name must be less than 64 bytes")); - VerifyOrReturnValue(mFabricIndex >= kMinValidFabricIndex, nullptr, ChipLogError(Zcl, "Fabric index is invalid")); - VerifyOrReturnValue(mFabricIndex <= kMaxValidFabricIndex, nullptr, ChipLogError(Zcl, "Fabric index is invalid")); // std::make_unique does not have access to private constructor we workaround with using new std::unique_ptr ret{ new EcosystemLocationStruct(std::move(mLocationDescriptor), - mLocationDescriptorLastEditEpochUs, - mFabricIndex) }; + mLocationDescriptorLastEditEpochUs) }; mIsAlreadyBuilt = true; return ret; } CHIP_ERROR EcosystemLocationStruct::Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, - const std::string & aUniqueLocationId) + const EcosystemLocationIdentifier & aUniqueLocationId) { Structs::EcosystemLocationStruct::Type locationStruct; - VerifyOrDie(!aUniqueLocationId.empty()); - locationStruct.uniqueLocationID = CharSpan(aUniqueLocationId.c_str(), aUniqueLocationId.size()); + locationStruct.uniqueLocationID = CharSpan(aUniqueLocationId.mUniqueLocationId.c_str(), aUniqueLocationId.mUniqueLocationId.size()); locationStruct.locationDescriptor = GetEncodableLocationDescriptorStruct(mLocationDescriptor); locationStruct.locationDescriptorLastEdit = mLocationDescriptorLastEditEpochUs; - locationStruct.SetFabricIndex(mFabricIndex); + locationStruct.SetFabricIndex(aUniqueLocationId.mFabricIndex); return aEncoder.Encode(locationStruct); } @@ -283,16 +271,22 @@ CHIP_ERROR EcosystemInformationServer::AddDeviceInfo(EndpointId aEndpoint, std:: } CHIP_ERROR EcosystemInformationServer::AddLocationInfo(EndpointId aEndpoint, const std::string & aLocationId, + FabricIndex aFabricIndex, std::unique_ptr aLocation) { VerifyOrReturnError(aLocation, CHIP_ERROR_INVALID_ARGUMENT); VerifyOrReturnError((aEndpoint != kRootEndpointId && aEndpoint != kInvalidEndpointId), CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(!aLocationId.empty(), CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(aFabricIndex >= kMinValidFabricIndex, CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(aFabricIndex <= kMaxValidFabricIndex, CHIP_ERROR_INVALID_ARGUMENT); auto & deviceInfo = mDevicesMap[aEndpoint]; - VerifyOrReturnError((deviceInfo.mLocationDirectory.find(aLocationId) == deviceInfo.mLocationDirectory.end()), + // TODO rename foobar + EcosystemLocationIdentifier foobar = {.mUniqueLocationId=aLocationId, .mFabricIndex=aFabricIndex}; + VerifyOrReturnError((deviceInfo.mLocationDirectory.find(foobar) == deviceInfo.mLocationDirectory.end()), CHIP_ERROR_INVALID_ARGUMENT); VerifyOrReturnError((deviceInfo.mLocationDirectory.size() < kLocationDirectoryMaxSize), CHIP_ERROR_NO_MEMORY); - deviceInfo.mLocationDirectory[aLocationId] = std::move(aLocation); + deviceInfo.mLocationDirectory[foobar] = std::move(aLocation); return CHIP_NO_ERROR; } diff --git a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h index 17c20de89132dc..b5f9253de385ae 100644 --- a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h +++ b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h @@ -101,6 +101,14 @@ struct LocationDescriptorStruct std::optional mAreaType; }; +struct EcosystemLocationIdentifier { + bool operator<(const EcosystemLocationIdentifier& other) const { + return mUniqueLocationId < other.mUniqueLocationId || (mUniqueLocationId == other.mUniqueLocationId && mFabricIndex < other.mFabricIndex); + } + std::string mUniqueLocationId; + FabricIndex mFabricIndex; +}; + // This intentionally mirrors Structs::EcosystemLocationStruct::Type but has ownership // of underlying types. class EcosystemLocationStruct @@ -115,7 +123,6 @@ class EcosystemLocationStruct Builder & SetFloorNumber(std::optional aFloorNumber); Builder & SetAreaTypeTag(std::optional aAreaTypeTag); Builder & SetLocationDescriptorLastEdit(uint64_t aLocationDescriptorLastEditEpochUs); - Builder & SetFabricIndex(FabricIndex aFabricIndex); // Upon success this object will have moved all ownership of underlying // types to EcosystemDeviceStruct and should not be used afterwards. @@ -124,25 +131,23 @@ class EcosystemLocationStruct private: LocationDescriptorStruct mLocationDescriptor; uint64_t mLocationDescriptorLastEditEpochUs = 0; - FabricIndex mFabricIndex = kUndefinedFabricIndex; bool mIsAlreadyBuilt = false; }; - CHIP_ERROR Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, const std::string & aUniqueLocationId); + CHIP_ERROR Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, const EcosystemLocationIdentifier & aUniqueLocationId); private: // Constructor is intentionally private. This is to ensure that it is only constructed with // values that conform to the spec. - explicit EcosystemLocationStruct(LocationDescriptorStruct && aLocationDescriptor, uint64_t aLocationDescriptorLastEditEpochUs, FabricIndex aFabricIndex) : - mLocationDescriptor(aLocationDescriptor), mLocationDescriptorLastEditEpochUs(aLocationDescriptorLastEditEpochUs), mFabricIndex(aFabricIndex) + explicit EcosystemLocationStruct(LocationDescriptorStruct && aLocationDescriptor, uint64_t aLocationDescriptorLastEditEpochUs) : + mLocationDescriptor(aLocationDescriptor), mLocationDescriptorLastEditEpochUs(aLocationDescriptorLastEditEpochUs) {} // EcosystemLocationStruct is used as a value in a key-value map. - // Because UniqueLocationId is manditory when an entry exist, and - // it is unique, we use it as a key to the key-value pair and is why it is + // Because UniqueLocationId and FabricIndex are manditory when an entry exist, + // and needs to be unique, we use it as a key to the key-value pair and is why it is // not explicitly in this struct. LocationDescriptorStruct mLocationDescriptor; uint64_t mLocationDescriptorLastEditEpochUs; - FabricIndex mFabricIndex; }; class EcosystemInformationServer @@ -186,7 +191,7 @@ class EcosystemInformationServer * @return #CHIP_NO_ERROR on success. * @return Other CHIP_ERROR associated with issue. */ - CHIP_ERROR AddLocationInfo(EndpointId aEndpoint, const std::string & aLocationId, + CHIP_ERROR AddLocationInfo(EndpointId aEndpoint, const std::string & aLocationId, FabricIndex aFabricIndex, std::unique_ptr aLocation); /** @@ -207,8 +212,7 @@ class EcosystemInformationServer { Optional mRemovedOn = NullOptional; std::vector> mDeviceDirectory; - // Map key is using the UniqueLocationId - std::map> mLocationDirectory; + std::map> mLocationDirectory; }; CHIP_ERROR EncodeRemovedOnAttribute(EndpointId aEndpoint, AttributeValueEncoder & aEncoder); From 38514c0c56a21426039dca1b2b37760d85429588 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Thu, 15 Aug 2024 16:52:52 +0000 Subject: [PATCH 3/8] Restyled by clang-format --- .../ecosystem-information-server.cpp | 8 ++++---- .../ecosystem-information-server.h | 12 ++++++++---- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp index ed87fe37e4a49c..9453ccd6706ed5 100644 --- a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp +++ b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp @@ -233,7 +233,8 @@ CHIP_ERROR EcosystemLocationStruct::Encode(const AttributeValueEncoder::ListEnco const EcosystemLocationIdentifier & aUniqueLocationId) { Structs::EcosystemLocationStruct::Type locationStruct; - locationStruct.uniqueLocationID = CharSpan(aUniqueLocationId.mUniqueLocationId.c_str(), aUniqueLocationId.mUniqueLocationId.size()); + locationStruct.uniqueLocationID = + CharSpan(aUniqueLocationId.mUniqueLocationId.c_str(), aUniqueLocationId.mUniqueLocationId.size()); locationStruct.locationDescriptor = GetEncodableLocationDescriptorStruct(mLocationDescriptor); locationStruct.locationDescriptorLastEdit = mLocationDescriptorLastEditEpochUs; locationStruct.SetFabricIndex(aUniqueLocationId.mFabricIndex); @@ -271,8 +272,7 @@ CHIP_ERROR EcosystemInformationServer::AddDeviceInfo(EndpointId aEndpoint, std:: } CHIP_ERROR EcosystemInformationServer::AddLocationInfo(EndpointId aEndpoint, const std::string & aLocationId, - FabricIndex aFabricIndex, - std::unique_ptr aLocation) + FabricIndex aFabricIndex, std::unique_ptr aLocation) { VerifyOrReturnError(aLocation, CHIP_ERROR_INVALID_ARGUMENT); VerifyOrReturnError((aEndpoint != kRootEndpointId && aEndpoint != kInvalidEndpointId), CHIP_ERROR_INVALID_ARGUMENT); @@ -282,7 +282,7 @@ CHIP_ERROR EcosystemInformationServer::AddLocationInfo(EndpointId aEndpoint, con auto & deviceInfo = mDevicesMap[aEndpoint]; // TODO rename foobar - EcosystemLocationIdentifier foobar = {.mUniqueLocationId=aLocationId, .mFabricIndex=aFabricIndex}; + EcosystemLocationIdentifier foobar = { .mUniqueLocationId = aLocationId, .mFabricIndex = aFabricIndex }; VerifyOrReturnError((deviceInfo.mLocationDirectory.find(foobar) == deviceInfo.mLocationDirectory.end()), CHIP_ERROR_INVALID_ARGUMENT); VerifyOrReturnError((deviceInfo.mLocationDirectory.size() < kLocationDirectoryMaxSize), CHIP_ERROR_NO_MEMORY); diff --git a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h index b5f9253de385ae..45af57c718561b 100644 --- a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h +++ b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h @@ -101,9 +101,12 @@ struct LocationDescriptorStruct std::optional mAreaType; }; -struct EcosystemLocationIdentifier { - bool operator<(const EcosystemLocationIdentifier& other) const { - return mUniqueLocationId < other.mUniqueLocationId || (mUniqueLocationId == other.mUniqueLocationId && mFabricIndex < other.mFabricIndex); +struct EcosystemLocationIdentifier +{ + bool operator<(const EcosystemLocationIdentifier & other) const + { + return mUniqueLocationId < other.mUniqueLocationId || + (mUniqueLocationId == other.mUniqueLocationId && mFabricIndex < other.mFabricIndex); } std::string mUniqueLocationId; FabricIndex mFabricIndex; @@ -134,7 +137,8 @@ class EcosystemLocationStruct bool mIsAlreadyBuilt = false; }; - CHIP_ERROR Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, const EcosystemLocationIdentifier & aUniqueLocationId); + CHIP_ERROR Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, + const EcosystemLocationIdentifier & aUniqueLocationId); private: // Constructor is intentionally private. This is to ensure that it is only constructed with From 2f6d4ad31128ac3a66a9bd1eb5d98c4165cad4c4 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 15 Aug 2024 17:00:58 +0000 Subject: [PATCH 4/8] Self review --- .../ecosystem-information-server.cpp | 13 +++++----- .../ecosystem-information-server.h | 26 +++++++++---------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp index 9453ccd6706ed5..b09eeb554519a2 100644 --- a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp +++ b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp @@ -230,14 +230,14 @@ std::unique_ptr EcosystemLocationStruct::Builder::Build } CHIP_ERROR EcosystemLocationStruct::Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, - const EcosystemLocationIdentifier & aUniqueLocationId) + const std::string & aUniqueLocationId, const FabricIndex & aFabricIndex) { Structs::EcosystemLocationStruct::Type locationStruct; locationStruct.uniqueLocationID = - CharSpan(aUniqueLocationId.mUniqueLocationId.c_str(), aUniqueLocationId.mUniqueLocationId.size()); + CharSpan(aUniqueLocationId.c_str(), aUniqueLocationId.size()); locationStruct.locationDescriptor = GetEncodableLocationDescriptorStruct(mLocationDescriptor); locationStruct.locationDescriptorLastEdit = mLocationDescriptorLastEditEpochUs; - locationStruct.SetFabricIndex(aUniqueLocationId.mFabricIndex); + locationStruct.SetFabricIndex(aFabricIndex); return aEncoder.Encode(locationStruct); } @@ -281,12 +281,11 @@ CHIP_ERROR EcosystemInformationServer::AddLocationInfo(EndpointId aEndpoint, con VerifyOrReturnError(aFabricIndex <= kMaxValidFabricIndex, CHIP_ERROR_INVALID_ARGUMENT); auto & deviceInfo = mDevicesMap[aEndpoint]; - // TODO rename foobar - EcosystemLocationIdentifier foobar = { .mUniqueLocationId = aLocationId, .mFabricIndex = aFabricIndex }; - VerifyOrReturnError((deviceInfo.mLocationDirectory.find(foobar) == deviceInfo.mLocationDirectory.end()), + EcosystemLocationKey key = { .mUniqueLocationId = aLocationId, .mFabricIndex = aFabricIndex }; + VerifyOrReturnError((deviceInfo.mLocationDirectory.find(key) == deviceInfo.mLocationDirectory.end()), CHIP_ERROR_INVALID_ARGUMENT); VerifyOrReturnError((deviceInfo.mLocationDirectory.size() < kLocationDirectoryMaxSize), CHIP_ERROR_NO_MEMORY); - deviceInfo.mLocationDirectory[foobar] = std::move(aLocation); + deviceInfo.mLocationDirectory[key] = std::move(aLocation); return CHIP_NO_ERROR; } diff --git a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h index 45af57c718561b..49218ad6d24f23 100644 --- a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h +++ b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h @@ -101,17 +101,6 @@ struct LocationDescriptorStruct std::optional mAreaType; }; -struct EcosystemLocationIdentifier -{ - bool operator<(const EcosystemLocationIdentifier & other) const - { - return mUniqueLocationId < other.mUniqueLocationId || - (mUniqueLocationId == other.mUniqueLocationId && mFabricIndex < other.mFabricIndex); - } - std::string mUniqueLocationId; - FabricIndex mFabricIndex; -}; - // This intentionally mirrors Structs::EcosystemLocationStruct::Type but has ownership // of underlying types. class EcosystemLocationStruct @@ -138,7 +127,7 @@ class EcosystemLocationStruct }; CHIP_ERROR Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, - const EcosystemLocationIdentifier & aUniqueLocationId); + const std::string & aUniqueLocationId, const FabricIndex & aFabricIndex); private: // Constructor is intentionally private. This is to ensure that it is only constructed with @@ -212,11 +201,22 @@ class EcosystemInformationServer CHIP_ERROR ReadAttribute(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder); private: + struct EcosystemLocationKey + { + bool operator<(const EcosystemLocationKey & other) const + { + return mUniqueLocationId < other.mUniqueLocationId || + (mUniqueLocationId == other.mUniqueLocationId && mFabricIndex < other.mFabricIndex); + } + std::string mUniqueLocationId; + FabricIndex mFabricIndex; + }; + struct DeviceInfo { Optional mRemovedOn = NullOptional; std::vector> mDeviceDirectory; - std::map> mLocationDirectory; + std::map> mLocationDirectory; }; CHIP_ERROR EncodeRemovedOnAttribute(EndpointId aEndpoint, AttributeValueEncoder & aEncoder); From 09fbc9a02082dddaa4180ab52ad459b4ed23b5bf Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Thu, 15 Aug 2024 17:01:20 +0000 Subject: [PATCH 5/8] Restyled by clang-format --- .../ecosystem-information-server.cpp | 5 ++--- .../ecosystem-information-server.h | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp index b09eeb554519a2..ddfb16ebb79493 100644 --- a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp +++ b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp @@ -233,8 +233,7 @@ CHIP_ERROR EcosystemLocationStruct::Encode(const AttributeValueEncoder::ListEnco const std::string & aUniqueLocationId, const FabricIndex & aFabricIndex) { Structs::EcosystemLocationStruct::Type locationStruct; - locationStruct.uniqueLocationID = - CharSpan(aUniqueLocationId.c_str(), aUniqueLocationId.size()); + locationStruct.uniqueLocationID = CharSpan(aUniqueLocationId.c_str(), aUniqueLocationId.size()); locationStruct.locationDescriptor = GetEncodableLocationDescriptorStruct(mLocationDescriptor); locationStruct.locationDescriptorLastEdit = mLocationDescriptorLastEditEpochUs; locationStruct.SetFabricIndex(aFabricIndex); @@ -280,7 +279,7 @@ CHIP_ERROR EcosystemInformationServer::AddLocationInfo(EndpointId aEndpoint, con VerifyOrReturnError(aFabricIndex >= kMinValidFabricIndex, CHIP_ERROR_INVALID_ARGUMENT); VerifyOrReturnError(aFabricIndex <= kMaxValidFabricIndex, CHIP_ERROR_INVALID_ARGUMENT); - auto & deviceInfo = mDevicesMap[aEndpoint]; + auto & deviceInfo = mDevicesMap[aEndpoint]; EcosystemLocationKey key = { .mUniqueLocationId = aLocationId, .mFabricIndex = aFabricIndex }; VerifyOrReturnError((deviceInfo.mLocationDirectory.find(key) == deviceInfo.mLocationDirectory.end()), CHIP_ERROR_INVALID_ARGUMENT); diff --git a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h index 49218ad6d24f23..c5c18737418269 100644 --- a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h +++ b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h @@ -126,8 +126,8 @@ class EcosystemLocationStruct bool mIsAlreadyBuilt = false; }; - CHIP_ERROR Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, - const std::string & aUniqueLocationId, const FabricIndex & aFabricIndex); + CHIP_ERROR Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, const std::string & aUniqueLocationId, + const FabricIndex & aFabricIndex); private: // Constructor is intentionally private. This is to ensure that it is only constructed with From 348d1f229c4516eb11255a0468b7cbd5bce2c601 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 15 Aug 2024 17:04:40 +0000 Subject: [PATCH 6/8] Self review --- .../ecosystem-information-server.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp index ddfb16ebb79493..1565ba008395b3 100644 --- a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp +++ b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp @@ -237,7 +237,6 @@ CHIP_ERROR EcosystemLocationStruct::Encode(const AttributeValueEncoder::ListEnco locationStruct.locationDescriptor = GetEncodableLocationDescriptorStruct(mLocationDescriptor); locationStruct.locationDescriptorLastEdit = mLocationDescriptorLastEditEpochUs; locationStruct.SetFabricIndex(aFabricIndex); - return aEncoder.Encode(locationStruct); } From fb3c66a0e7ef36fecbb451512a1cd5e71affafae Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 15 Aug 2024 22:42:49 +0000 Subject: [PATCH 7/8] Fix CI --- .../ecosystem-information-server.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp index 1565ba008395b3..2ce82394a57464 100644 --- a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp +++ b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp @@ -387,9 +387,9 @@ CHIP_ERROR EcosystemInformationServer::EncodeLocationStructAttribute(EndpointId } return aEncoder.EncodeList([&](const auto & encoder) -> CHIP_ERROR { - for (auto & [id, device] : deviceInfo.mLocationDirectory) + for (auto & [key, device] : deviceInfo.mLocationDirectory) { - ReturnErrorOnFailure(device->Encode(encoder, id)); + ReturnErrorOnFailure(device->Encode(encoder, key.mUniqueLocationId, key.mFabricIndex)); } return CHIP_NO_ERROR; }); From a0d39df2b4d3c8c55e294abb8207525b21eb0853 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Fri, 16 Aug 2024 13:49:57 -0400 Subject: [PATCH 8/8] Update src/app/clusters/ecosystem-information-server/ecosystem-information-server.h Co-authored-by: Andrei Litvin --- .../ecosystem-information-server/ecosystem-information-server.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h index c5c18737418269..d4f4d7dc3a6fc1 100644 --- a/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h +++ b/src/app/clusters/ecosystem-information-server/ecosystem-information-server.h @@ -136,7 +136,7 @@ class EcosystemLocationStruct mLocationDescriptor(aLocationDescriptor), mLocationDescriptorLastEditEpochUs(aLocationDescriptorLastEditEpochUs) {} // EcosystemLocationStruct is used as a value in a key-value map. - // Because UniqueLocationId and FabricIndex are manditory when an entry exist, + // Because UniqueLocationId and FabricIndex are mandatory when an entry exist, // and needs to be unique, we use it as a key to the key-value pair and is why it is // not explicitly in this struct. LocationDescriptorStruct mLocationDescriptor;