Skip to content

Commit

Permalink
Address PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@tehampson
tehampson committed Jul 24, 2024
1 parent eeb0e77 commit d97eb61
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 33 deletions.
52 changes: 29 additions & 23 deletions src/app/clusters/ecosystem-information-server/ecosystem-information-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,22 +131,14 @@ EcosystemDeviceStruct::Builder & EcosystemDeviceStruct::Builder::AddUniqueLocati

std::unique_ptr<EcosystemDeviceStruct> EcosystemDeviceStruct::Builder::Build()
{
bool deviceNameIsInvalid = mDeviceName.size() > kDeviceNameMaxSize;
bool originalEndpointIsInvalid = mOriginalEndpoint == kInvalidEndpointId;
bool deviceTypesListIsInvalid = mDeviceTypes.empty();
bool uniqueLocationIdsListIsInvalid = mUniqueLocationIds.size() > kUniqueLocationIdsListMaxSize;
if (mIsAlreadyBuilt || deviceNameIsInvalid || originalEndpointIsInvalid || deviceTypesListIsInvalid ||
uniqueLocationIdsListIsInvalid)
{
return nullptr;
}
VerifyOrReturnValue(mDeviceName.size() <= kDeviceNameMaxSize, nullptr, ChipLogError(Zcl, "Device name too large"));
VerifyOrReturnValue(mOriginalEndpoint != kInvalidEndpointId, nullptr, ChipLogError(Zcl, "Invalid original endpoint"));
VerifyOrReturnValue(!mDeviceTypes.empty(), nullptr, ChipLogError(Zcl, "No device types added"));
VerifyOrReturnValue(mUniqueLocationIds.size() <= kUniqueLocationIdsListMaxSize, nullptr, ChipLogError(Zcl, "Too many location ids"));

for (auto & locationId : mUniqueLocationIds)
{
if (locationId.size() > kUniqueLocationIdMaxSize)
{
return nullptr;
}
VerifyOrReturnValue(locationId.size() <= kUniqueLocationIdMaxSize, nullptr, ChipLogError(Zcl, "Location id too long"));
}

// std::make_unique does not have access to private constructor we workaround with using new
Expand Down Expand Up @@ -260,7 +252,7 @@ CHIP_ERROR EcosystemInformationServer::AddDeviceInfo(EndpointId aEndpoint, std::
VerifyOrReturnError((aEndpoint != kRootEndpointId && aEndpoint != kInvalidEndpointId), CHIP_ERROR_INVALID_ARGUMENT);

auto & deviceInfo = mDevicesMap[aEndpoint];
VerifyOrReturnError((deviceInfo.mDeviceDirectory.size() >= kDeviceDirectoryMaxSize), CHIP_ERROR_NO_MEMORY);
VerifyOrReturnError((deviceInfo.mDeviceDirectory.size() < kDeviceDirectoryMaxSize), CHIP_ERROR_NO_MEMORY);
deviceInfo.mDeviceDirectory.push_back(std::move(aDevice));
return CHIP_NO_ERROR;
}
Expand All @@ -274,27 +266,32 @@ CHIP_ERROR EcosystemInformationServer::AddLocationInfo(EndpointId aEndpoint, con
auto & deviceInfo = mDevicesMap[aEndpoint];
VerifyOrReturnError((deviceInfo.mLocationDirectory.find(aLocationId) == deviceInfo.mLocationDirectory.end()),
CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError((deviceInfo.mLocationDirectory.size() >= kLocationDirectoryMaxSize), CHIP_ERROR_NO_MEMORY);
VerifyOrReturnError((deviceInfo.mLocationDirectory.size() < kLocationDirectoryMaxSize), CHIP_ERROR_NO_MEMORY);
deviceInfo.mLocationDirectory[aLocationId] = std::move(aLocation);
return CHIP_NO_ERROR;
}

CHIP_ERROR EcosystemInformationServer::RemoveDevice(EndpointId aEndpoint, uint64_t aEpochUs)
{
VerifyOrReturnError((mDevicesMap.find(aEndpoint) != mDevicesMap.end()), CHIP_ERROR_INVALID_ARGUMENT);
auto & deviceInfo = mDevicesMap[aEndpoint];
auto it = mDevicesMap.find(aEndpoint);
VerifyOrReturnError((it != mDevicesMap.end()), CHIP_ERROR_INVALID_ARGUMENT);
auto & deviceInfo = it->second;
deviceInfo.mRemovedOn.SetValue(aEpochUs);
return CHIP_NO_ERROR;
}

CHIP_ERROR EcosystemInformationServer::EncodeRemovedOnAttribute(EndpointId aEndpoint, AttributeValueEncoder & aEncoder)
{
if (mDevicesMap.find(aEndpoint) == mDevicesMap.end())
auto it = mDevicesMap.find(aEndpoint);
if (it == mDevicesMap.end())
{
// We are always going to be given a valid endpoint. If the endpoint
// doesn't exist in our map that indicate that the cluster was not
// added on this endpoint, hence UnsupportedCluster.
return CHIP_IM_GLOBAL_STATUS(UnsupportedCluster);
}

auto & deviceInfo = mDevicesMap[aEndpoint];
auto& deviceInfo = it->second;
if (!deviceInfo.mRemovedOn.HasValue())
{
aEncoder.EncodeNull();
Expand All @@ -307,12 +304,17 @@ CHIP_ERROR EcosystemInformationServer::EncodeRemovedOnAttribute(EndpointId aEndp

CHIP_ERROR EcosystemInformationServer::EncodeDeviceDirectoryAttribute(EndpointId aEndpoint, AttributeValueEncoder & aEncoder)
{
if (mDevicesMap.find(aEndpoint) == mDevicesMap.end())

auto it = mDevicesMap.find(aEndpoint);
if (it == mDevicesMap.end())
{
// We are always going to be given a valid endpoint. If the endpoint
// doesn't exist in our map that indicate that the cluster was not
// added on this endpoint, hence UnsupportedCluster.
return CHIP_IM_GLOBAL_STATUS(UnsupportedCluster);
}

auto & deviceInfo = mDevicesMap[aEndpoint];
auto & deviceInfo = it->second;
if (deviceInfo.mDeviceDirectory.empty() || deviceInfo.mRemovedOn.HasValue())
{
return aEncoder.EncodeEmptyList();
Expand All @@ -330,12 +332,16 @@ CHIP_ERROR EcosystemInformationServer::EncodeDeviceDirectoryAttribute(EndpointId

CHIP_ERROR EcosystemInformationServer::EncodeLocationStructAttribute(EndpointId aEndpoint, AttributeValueEncoder & aEncoder)
{
if (mDevicesMap.find(aEndpoint) == mDevicesMap.end())
auto it = mDevicesMap.find(aEndpoint);
if (it == mDevicesMap.end())
{
// We are always going to be given a valid endpoint. If the endpoint
// doesn't exist in our map that indicate that the cluster was not
// added on this endpoint, hence UnsupportedCluster.
return CHIP_IM_GLOBAL_STATUS(UnsupportedCluster);
}

auto & deviceInfo = mDevicesMap[aEndpoint];
auto & deviceInfo = it->second;
if (deviceInfo.mLocationDirectory.empty() || deviceInfo.mRemovedOn.HasValue())
{
return aEncoder.EncodeEmptyList();
Expand Down
14 changes: 4 additions & 10 deletions src/app/clusters/ecosystem-information-server/ecosystem-information-server.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ class EcosystemDeviceStruct
{
public:
Builder(){};
Builder(const Builder &) = delete;

Builder & SetDeviceName(std::string aDeviceName, uint64_t aDeviceNameLastEditEpochUs);
Builder & SetBrigedEndpoint(EndpointId aBridgedEndpoint);
Expand All @@ -66,16 +65,14 @@ class EcosystemDeviceStruct
bool mIsAlreadyBuilt = false;
};

EcosystemDeviceStruct(const EcosystemDeviceStruct &) = delete;

CHIP_ERROR Encode(const AttributeValueEncoder::ListEncodeHelper & aEncoder, const FabricIndex & aFabricIndex);

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<Structs::DeviceTypeStruct::Type> aDeviceTypes,
std::vector<std::string> aUniqueLocationIds, uint64_t aUniqueLocationIdsLastEditEpochUs) :
explicit EcosystemDeviceStruct(std::string && aDeviceName, uint64_t aDeviceNameLastEditEpochUs, EndpointId aBridgedEndpoint,
EndpointId aOriginalEndpoint, std::vector<Structs::DeviceTypeStruct::Type> && aDeviceTypes,
std::vector<std::string> && aUniqueLocationIds, uint64_t aUniqueLocationIdsLastEditEpochUs) :
mDeviceName(std::move(aDeviceName)),
mDeviceNameLastEditEpochUs(aDeviceNameLastEditEpochUs), mBridgedEndpoint(aBridgedEndpoint),
mOriginalEndpoint(aOriginalEndpoint), mDeviceTypes(std::move(aDeviceTypes)),
Expand Down Expand Up @@ -111,7 +108,6 @@ class EcosystemLocationStruct
{
public:
Builder(){};
Builder(const Builder &) = delete;

Builder & SetLocationName(std::string aLocationName);
Builder & SetFloorNumber(std::optional<int16_t> aFloorNumber);
Expand All @@ -128,15 +124,13 @@ class EcosystemLocationStruct
bool mIsAlreadyBuilt = false;
};

EcosystemLocationStruct(const EcosystemLocationStruct &) = delete;

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
// values that conform to the spec.
explicit EcosystemLocationStruct(HomeLocationStruct aHomeLocation, uint64_t aHomeLocationLastEditEpochUs) :
explicit EcosystemLocationStruct(HomeLocationStruct && aHomeLocation, uint64_t aHomeLocationLastEditEpochUs) :
mHomeLocation(aHomeLocation), mHomeLocationLastEditEpochUs(aHomeLocationLastEditEpochUs)
{}
// EcosystemLocationStruct is used as a value in a key-value map.
Expand Down

0 comments on commit d97eb61

Please sign in to comment.