Skip to content

Commit

Permalink
FieldId and Listindex should not be exclusive (#7295)
Browse files Browse the repository at this point in the history 
Problems:
We would only support toplevel attributes and list items inside toplevel attribute

Summary of Changes:
--Support two situation in attribute path, one is FieldId only, another
is FieldId + ListIndex.
  • Loading branch information
@yunhanw-google @pull
yunhanw-google authored and pull[bot] committed Jun 9, 2021
1 parent b1f69cf commit 0480d46
Show file tree
Hide file tree
Showing 11 changed files with 288 additions and 194 deletions.
46 changes: 13 additions & 33 deletions src/app/AttributePathParams.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,46 +23,26 @@

namespace chip {
namespace app {
enum class AttributePathFlags : uint8_t
{
kFieldIdValid = 0x01,
kListIndexValid = 0x02,
};

struct AttributePathParams
{
enum class Flags : uint8_t
{
kFieldIdValid = 0x01,
kListIndexValid = 0x02,
};

AttributePathParams(NodeId aNodeId, EndpointId aEndpointId, ClusterId aClusterId, FieldId aFieldId, ListIndex aListIndex,
const BitFlags<AttributePathFlags> aFlags) :
const BitFlags<Flags> aFlags) :
mNodeId(aNodeId),
mEndpointId(aEndpointId), mClusterId(aClusterId), mFieldId(aFieldId), mListIndex(aListIndex), mFlags(aFlags)
{}
AttributePathParams() {}
bool IsSamePath(const AttributePathParams & other) const
{
if (other.mNodeId != mNodeId || other.mEndpointId != mEndpointId || other.mClusterId != mClusterId)
{
return false;
}
if (mFlags != other.mFlags)
{
return false;
}
if (mFlags == AttributePathFlags::kFieldIdValid && other.mFieldId != mFieldId)
{
return false;
}
if (mFlags == AttributePathFlags::kListIndexValid && other.mListIndex != mListIndex)
{
return false;
}
return true;
}
chip::NodeId mNodeId = 0;
chip::EndpointId mEndpointId = 0;
chip::ClusterId mClusterId = 0;
chip::FieldId mFieldId = 0;
chip::ListIndex mListIndex = 0;
BitFlags<AttributePathFlags> mFlags;
NodeId mNodeId = 0;
EndpointId mEndpointId = 0;
ClusterId mClusterId = 0;
FieldId mFieldId = 0;
ListIndex mListIndex = 0;
BitFlags<Flags> mFlags;
};
} // namespace app
} // namespace chip
9 changes: 4 additions & 5 deletions src/app/ClusterInfo.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,8 @@ namespace chip {
namespace app {
struct ClusterInfo
{
enum class Type : uint8_t
enum class Flags : uint8_t
{
kInvalid = 0,
kFieldIdValid = 0x01,
kListIndexValid = 0x02,
kEventIdValid = 0x03,
Expand All @@ -43,9 +42,9 @@ struct ClusterInfo
FieldId mFieldId = 0;
EndpointId mEndpointId = 0;
bool mDirty = false;
Type mType = Type::kInvalid;
ClusterInfo * mpNext = nullptr;
EventId mEventId = 0;
BitFlags<Flags> mFlags;
ClusterInfo * mpNext = nullptr;
EventId mEventId = 0;
/* For better structure alignment
* Above ordering is by bit-size to ensure least amount of memory alignment padding.
* Changing order to something more natural (e.g. clusterid before nodeid) will result
Expand Down
2 changes: 1 addition & 1 deletion src/app/InteractionModelEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ void InteractionModelEngine::ReleaseClusterInfoList(ClusterInfo *& aClusterInfo)
lastClusterInfo = lastClusterInfo->mpNext;
}
lastClusterInfo->ClearDirty();
lastClusterInfo->mType = ClusterInfo::Type::kInvalid;
lastClusterInfo->mFlags.ClearAll();
lastClusterInfo->mpNext = mpNextAvailableClusterInfo;
mpNextAvailableClusterInfo = aClusterInfo;
aClusterInfo = nullptr;
Expand Down
10 changes: 4 additions & 6 deletions src/app/MessageDef/AttributePath.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,16 +151,14 @@ CHIP_ERROR AttributePath::Parser::CheckSchemaValidity() const
// if we have exhausted this container
if (CHIP_END_OF_TLV == err)
{
// check for required fields:
const uint16_t RequiredFields = (1 << kCsTag_EndpointId) | (1 << kCsTag_ClusterId);

if ((TagPresenceMask & RequiredFields) == RequiredFields)
// Not allow for situation where ListIndex exists, but FieldId not exists
if ((TagPresenceMask & (1 << kCsTag_FieldId)) == 0 && (TagPresenceMask & (1 << kCsTag_ListIndex)) != 0)
{
err = CHIP_NO_ERROR;
err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH;
}
else
{
err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH;
err = CHIP_NO_ERROR;
}
}
SuccessOrExit(err);
Expand Down
76 changes: 46 additions & 30 deletions src/app/ReadClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,32 +132,8 @@ CHIP_ERROR ReadClient::SendReadRequest(NodeId aNodeId, Transport::AdminId aAdmin

if (aAttributePathParamsListSize != 0 && apAttributePathParamsList != nullptr)
{
AttributePathList::Builder attributePathListBuilder = request.CreateAttributePathListBuilder();
SuccessOrExit(err = attributePathListBuilder.GetError());
for (size_t index = 0; index < aAttributePathParamsListSize; index++)
{
AttributePath::Builder attributePathBuilder = attributePathListBuilder.CreateAttributePathBuilder();
attributePathBuilder.NodeId(apAttributePathParamsList[index].mNodeId)
.EndpointId(apAttributePathParamsList[index].mEndpointId)
.ClusterId(apAttributePathParamsList[index].mClusterId);
if (apAttributePathParamsList[index].mFlags == AttributePathFlags::kFieldIdValid)
{
attributePathBuilder.FieldId(apAttributePathParamsList[index].mFieldId);
}
else if (apAttributePathParamsList[index].mFlags == AttributePathFlags::kListIndexValid)
{
attributePathBuilder.ListIndex(apAttributePathParamsList[index].mListIndex);
}
else
{
err = CHIP_ERROR_INVALID_ARGUMENT;
ExitNow();
}
attributePathBuilder.EndOfAttributePath();
SuccessOrExit(err = attributePathBuilder.GetError());
}
attributePathListBuilder.EndOfAttributePathList();
SuccessOrExit(err = attributePathListBuilder.GetError());
err = GenerateAttributePathList(request, apAttributePathParamsList, aAttributePathParamsListSize);
SuccessOrExit(err);
}

request.EndOfReadRequest();
Expand Down Expand Up @@ -187,6 +163,36 @@ CHIP_ERROR ReadClient::SendReadRequest(NodeId aNodeId, Transport::AdminId aAdmin
return err;
}

CHIP_ERROR ReadClient::GenerateAttributePathList(ReadRequest::Builder & aRequest, AttributePathParams * apAttributePathParamsList,
size_t aAttributePathParamsListSize)
{
AttributePathList::Builder attributePathListBuilder = aRequest.CreateAttributePathListBuilder();
ReturnErrorOnFailure(attributePathListBuilder.GetError());
for (size_t index = 0; index < aAttributePathParamsListSize; index++)
{
AttributePath::Builder attributePathBuilder = attributePathListBuilder.CreateAttributePathBuilder();
attributePathBuilder.NodeId(apAttributePathParamsList[index].mNodeId)
.EndpointId(apAttributePathParamsList[index].mEndpointId)
.ClusterId(apAttributePathParamsList[index].mClusterId);
if (apAttributePathParamsList[index].mFlags.Has(AttributePathParams::Flags::kFieldIdValid))
{
attributePathBuilder.FieldId(apAttributePathParamsList[index].mFieldId);
}

if (apAttributePathParamsList[index].mFlags.Has(AttributePathParams::Flags::kListIndexValid))
{
VerifyOrReturnError(apAttributePathParamsList[index].mFlags.Has(AttributePathParams::Flags::kFieldIdValid),
CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
attributePathBuilder.ListIndex(apAttributePathParamsList[index].mListIndex);
}

attributePathBuilder.EndOfAttributePath();
ReturnErrorOnFailure(attributePathBuilder.GetError());
}
attributePathListBuilder.EndOfAttributePathList();
return attributePathListBuilder.GetError();
}

void ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PacketHeader & aPacketHeader,
const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload)
{
Expand Down Expand Up @@ -351,13 +357,23 @@ CHIP_ERROR ReadClient::ProcessAttributeDataList(TLV::TLVReader & aAttributeDataL
err = attributePathParser.GetFieldId(&(clusterInfo.mFieldId));
if (CHIP_NO_ERROR == err)
{
clusterInfo.mType = ClusterInfo::Type::kFieldIdValid;
clusterInfo.mFlags.Set(ClusterInfo::Flags::kFieldIdValid);
}
else if (CHIP_END_OF_TLV == err)
{
err = attributePathParser.GetListIndex(&(clusterInfo.mListIndex));
SuccessOrExit(err);
clusterInfo.mType = ClusterInfo::Type::kListIndexValid;
err = CHIP_NO_ERROR;
}
SuccessOrExit(err);

err = attributePathParser.GetListIndex(&(clusterInfo.mListIndex));
if (CHIP_NO_ERROR == err)
{
VerifyOrExit(clusterInfo.mFlags.Has(ClusterInfo::Flags::kFieldIdValid), err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
clusterInfo.mFlags.Set(ClusterInfo::Flags::kListIndexValid);
}
else if (CHIP_END_OF_TLV == err)
{
err = CHIP_NO_ERROR;
}
SuccessOrExit(err);

Expand Down
2 changes: 2 additions & 0 deletions src/app/ReadClient.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ class ReadClient : public Messaging::ExchangeDelegate
*/
bool IsFree() const { return mState == ClientState::Uninitialized; };

CHIP_ERROR GenerateAttributePathList(ReadRequest::Builder & aRequest, AttributePathParams * apAttributePathParamsList,
size_t aAttributePathParamsListSize);
CHIP_ERROR ProcessAttributeDataList(TLV::TLVReader & aAttributeDataListReader);

void MoveToState(const ClientState aTargetState);
Expand Down
30 changes: 24 additions & 6 deletions src/app/ReadHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,15 +184,26 @@ CHIP_ERROR ReadHandler::ProcessAttributePathList(AttributePathList::Parser & aAt
err = path.GetFieldId(&(clusterInfo.mFieldId));
if (CHIP_NO_ERROR == err)
{
clusterInfo.mType = ClusterInfo::Type::kFieldIdValid;
clusterInfo.mFlags.Set(ClusterInfo::Flags::kFieldIdValid);
}
else if (CHIP_END_OF_TLV == err)
{
err = path.GetListIndex(&(clusterInfo.mListIndex));
SuccessOrExit(err);
clusterInfo.mType = ClusterInfo::Type::kListIndexValid;
err = CHIP_NO_ERROR;
}
SuccessOrExit(err);

err = path.GetListIndex(&(clusterInfo.mListIndex));
if (CHIP_NO_ERROR == err)
{
VerifyOrExit(clusterInfo.mFlags.Has(ClusterInfo::Flags::kFieldIdValid), err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
clusterInfo.mFlags.Set(ClusterInfo::Flags::kListIndexValid);
}
else if (CHIP_END_OF_TLV == err)
{
err = CHIP_NO_ERROR;
}
SuccessOrExit(err);

err = InteractionModelEngine::GetInstance()->PushFront(mpAttributeClusterInfoList, clusterInfo);
SuccessOrExit(err);
mpAttributeClusterInfoList->SetDirty();
Expand Down Expand Up @@ -229,9 +240,16 @@ CHIP_ERROR ReadHandler::ProcessEventPathList(EventPathList::Parser & aEventPathL
err = path.GetClusterId(&(clusterInfo.mClusterId));
SuccessOrExit(err);
err = path.GetEventId(&(clusterInfo.mEventId));
if (CHIP_NO_ERROR == err)
{
clusterInfo.mFlags.Set(ClusterInfo::Flags::kEventIdValid);
}
else if (CHIP_END_OF_TLV == err)
{
err = CHIP_NO_ERROR;
}
SuccessOrExit(err);
clusterInfo.mType = ClusterInfo::Type::kEventIdValid;
err = InteractionModelEngine::GetInstance()->PushFront(mpEventClusterInfoList, clusterInfo);
err = InteractionModelEngine::GetInstance()->PushFront(mpEventClusterInfoList, clusterInfo);
SuccessOrExit(err);
}

Expand Down
1 change: 0 additions & 1 deletion src/app/tests/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ chip_test_suite("tests") {
output_name = "libAppTests"

test_sources = [
"TestAttributePathParams.cpp",
"TestClusterInfo.cpp",
"TestCommandInteraction.cpp",
"TestCommandPathParams.cpp",
Expand Down
104 changes: 0 additions & 104 deletions src/app/tests/TestAttributePathParams.cpp
View file

This file was deleted.

Loading

0 comments on commit 0480d46

Please sign in to comment.