Skip to content

Commit

Permalink
[im] Fix wildcard path support per spec
Browse files Browse the repository at this point in the history
  • Loading branch information
@erjiaqing
erjiaqing committed Nov 3, 2021
1 parent c9db3dc commit 9072d37
Show file tree
Hide file tree
Showing 13 changed files with 102 additions and 154 deletions.
121 changes: 45 additions & 76 deletions src/app/ClusterInfo.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,103 +18,72 @@

#pragma once

#include <app/AttributePathParams.h>
#include <app/util/basic-types.h>
#include <assert.h>
#include <lib/core/Optional.h>

namespace chip {
namespace app {
static constexpr AttributeId kRootAttributeId = 0xFFFFFFFF;

/**
* ClusterInfo is the representation of an attribute path or an event path used by ReadHandler, ReadClient, WriteHandler,
* Report::Engine etc, tt uses some invalid values for representing the wildcard value for its field and contains a mpNext field so
* it can be used as a linked list.
*/
// TODO: The cluster info should be separated into AttributeInfo and EventInfo.
// Note: The change will happen after #11171 with a better linked list.
struct ClusterInfo
{
enum class Flags : uint8_t
{
kFieldIdValid = 0x01,
kListIndexValid = 0x02,
kEventIdValid = 0x03,
};
// Endpoint Id is a uint16 number, and should between 0 and 0xFFFE
static constexpr EndpointId kInvalidEndpointId = 0xFFFF;
// The ClusterId, AttributeId and EventId are MEIs,
// 0xFFFF is not a valid manufacturer code, thus 0xFFFF'FFFF is not a valid MEI
static constexpr ClusterId kInvalidClusterId = 0xFFFF'FFFF;
static constexpr AttributeId kInvalidAttributeId = 0xFFFF'FFFF;
static constexpr EventId kInvalidEventId = 0xFFFF'FFFF;
// ListIndex is a uint16 number, thus 0xFFFF is not a valid list index.
static constexpr ListIndex kInvalidListIndex = 0xFFFF;

bool IsAttributePathSupersetOf(const ClusterInfo & other) const
{
if ((other.mEndpointId != mEndpointId) || (other.mClusterId != mClusterId))
{
return false;
}

Optional<AttributeId> myFieldId =
mFlags.Has(Flags::kFieldIdValid) ? Optional<AttributeId>::Value(mFieldId) : Optional<AttributeId>::Missing();

Optional<AttributeId> otherFieldId = other.mFlags.Has(Flags::kFieldIdValid) ? Optional<AttributeId>::Value(other.mFieldId)
: Optional<AttributeId>::Missing();

Optional<ListIndex> myListIndex =
mFlags.Has(Flags::kListIndexValid) ? Optional<ListIndex>::Value(mListIndex) : Optional<ListIndex>::Missing();

Optional<ListIndex> otherListIndex = other.mFlags.Has(Flags::kListIndexValid) ? Optional<ListIndex>::Value(other.mListIndex)
: Optional<ListIndex>::Missing();
VerifyOrReturnError(!HasWildcardEndpointId() || mEndpointId == other.mEndpointId, false);
VerifyOrReturnError(!HasWildcardClusterId() || mClusterId == other.mClusterId, false);
VerifyOrReturnError(!HasWildcardAttributeId() || mFieldId == other.mFieldId, false);
VerifyOrReturnError(!HasWildcardListIndex() || mListIndex == other.mListIndex, false);

// If list index exists, field index must exist
// Field 0xFFFFFFF (any) & listindex set is invalid
assert(!(myListIndex.HasValue() && !myFieldId.HasValue()));
assert(!(otherListIndex.HasValue() && !otherFieldId.HasValue()));
assert(!(myFieldId == Optional<AttributeId>::Value(kRootAttributeId) && myListIndex.HasValue()));
assert(!(otherFieldId == Optional<AttributeId>::Value(kRootAttributeId) && otherListIndex.HasValue()));

if (myFieldId == Optional<AttributeId>::Value(kRootAttributeId))
{
return true;
}

if (myFieldId != otherFieldId)
{
return false;
}
return true;
}

// We only support top layer for attribute representation, either FieldId or FieldId + ListIndex
// Combination: if myFieldId == otherFieldId, ListIndex cannot exist without FieldId
// 1. myListIndex and otherListIndex both missing or both exactly the same, then current is superset of other
// 2. myListIndex is missing, no matter if otherListIndex is missing or not, then current is superset of other
if (myListIndex == otherListIndex)
{
// either both missing or both exactly the same
return true;
}
bool HasWildcard() const { return !HasWildcardEndpointId() || !HasWildcardClusterId() || !HasWildcardAttributeId(); }

if (!myListIndex.HasValue())
{
// difference is ok only if myListIndex is missing
return true;
}
/**
* Verify if it mets some basic constrants of a attribute path: If list index is valid, then field id must be valid too.
* There are other conditions that the path is not a valid path, e.g. the path does not exists, wildcard cluster id with
* non-wildcard attribute id and valid list index on a non-array attributes etc.
*/
bool IsValidAttributePath() const { return mListIndex == kInvalidListIndex || mFieldId != kInvalidAttributeId; }

return false;
}
inline bool HasWildcardNodeId() const { return mNodeId != kUndefinedNodeId; }
inline bool HasWildcardEndpointId() const { return mEndpointId != kInvalidEndpointId; }
inline bool HasWildcardClusterId() const { return mClusterId != kInvalidClusterId; }
inline bool HasWildcardAttributeId() const { return mFieldId != kInvalidAttributeId; }
inline bool HasWildcardListIndex() const { return mListIndex != kInvalidListIndex; }
inline bool HasWildcardEventId() const { return mEventId != kInvalidEventId; }

ClusterInfo() {}
NodeId mNodeId = 0;
ClusterId mClusterId = 0;
ListIndex mListIndex = 0;
AttributeId mFieldId = 0;
EndpointId mEndpointId = 0;
BitFlags<Flags> mFlags;
ClusterInfo * mpNext = nullptr;
EventId mEventId = 0;
/* For better structure alignment
/*
* 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
* Changing order to something more natural (e.g. endpoint id before cluster id) will result
* in extra memory alignment padding.
* uint64 mNodeId
* uint16_t mClusterId
* uint16_t mListIndex
* uint8_t FieldId
* uint8_t EndpointId
* uint8_t mDirty
* uint8_t mType
* uint32_t mpNext
* uint16_t EventId
* padding 2 bytes
*/
ClusterInfo * mpNext = nullptr; // pointer width (32/64 bits)
NodeId mNodeId = kUndefinedNodeId; // uint64
ClusterId mClusterId = kInvalidClusterId; // uint32
AttributeId mFieldId = kInvalidAttributeId; // uint32
EventId mEventId = kInvalidEventId; // uint32
ListIndex mListIndex = kInvalidListIndex; // uint16
EndpointId mEndpointId = kInvalidEndpointId; // uint16
};
} // namespace app
} // namespace chip
1 change: 1 addition & 0 deletions src/app/EventManagement.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,7 @@ static bool IsInterestedEventPaths(EventLoadOutContext * eventLoadOutContext, co
}
while (interestedEventPaths != nullptr)
{
// TODO: Support wildcard event path
if (interestedEventPaths->mNodeId == event.mNodeId && interestedEventPaths->mEndpointId == event.mEndpointId &&
interestedEventPaths->mClusterId == event.mClusterId && interestedEventPaths->mEventId == event.mEventId)
{
Expand Down
2 changes: 0 additions & 2 deletions src/app/InteractionModelEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,6 @@ void InteractionModelEngine::ReleaseClusterInfoList(ClusterInfo *& aClusterInfo)
{
lastClusterInfo = lastClusterInfo->mpNext;
}
lastClusterInfo->mFlags.ClearAll();
lastClusterInfo->mpNext = mpNextAvailableClusterInfo;
mpNextAvailableClusterInfo = aClusterInfo;
aClusterInfo = nullptr;
Expand Down Expand Up @@ -502,7 +501,6 @@ bool InteractionModelEngine::MergeOverlappedAttributePath(ClusterInfo * apAttrib
{
runner->mListIndex = aAttributePath.mListIndex;
runner->mFieldId = aAttributePath.mFieldId;
runner->mFlags = aAttributePath.mFlags;
return true;
}
runner = runner->mpNext;
Expand Down
4 changes: 2 additions & 2 deletions src/app/MessageDef/Parser.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,11 @@ class Parser
CHIP_ERROR err = CHIP_NO_ERROR;
chip::TLV::TLVReader reader;

*apLValue = 0;

err = mReader.FindElementWithTag(chip::TLV::ContextTag(aContextTag), reader);
SuccessOrExit(err);

*apLValue = 0;

VerifyOrExit(aTLVType == reader.GetType(), err = CHIP_ERROR_WRONG_TLV_TYPE);

err = reader.Get(*apLValue);
Expand Down
25 changes: 11 additions & 14 deletions src/app/ReadClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -475,32 +475,25 @@ CHIP_ERROR ReadClient::ProcessAttributeDataList(TLV::TLVReader & aAttributeDataL
SuccessOrExit(err);

err = attributePathParser.GetNodeId(&(clusterInfo.mNodeId));
if (err == CHIP_END_OF_TLV)
{
err = CHIP_NO_ERROR;
}
SuccessOrExit(err);

// The ReportData must contain a concrete attribute path

err = attributePathParser.GetEndpointId(&(clusterInfo.mEndpointId));
SuccessOrExit(err);

err = attributePathParser.GetClusterId(&(clusterInfo.mClusterId));
SuccessOrExit(err);

err = attributePathParser.GetFieldId(&(clusterInfo.mFieldId));
if (CHIP_NO_ERROR == err)
{
clusterInfo.mFlags.Set(ClusterInfo::Flags::kFieldIdValid);
}
else if (CHIP_END_OF_TLV == err)
{
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)
if (CHIP_END_OF_TLV == err)
{
err = CHIP_NO_ERROR;
}
Expand Down Expand Up @@ -529,6 +522,10 @@ CHIP_ERROR ReadClient::ProcessAttributeDataList(TLV::TLVReader & aAttributeDataL
}

exit:
if (err != CHIP_NO_ERROR)
{
err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH;
}
return err;
}

Expand Down
31 changes: 18 additions & 13 deletions src/app/ReadHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,28 +335,37 @@ CHIP_ERROR ReadHandler::ProcessAttributePathList(AttributePathList::Parser & aAt
AttributePath::Parser path;
err = path.Init(reader);
SuccessOrExit(err);
err = path.GetNodeId(&(clusterInfo.mNodeId));
SuccessOrExit(err);
// TODO: Support wildcard paths here
// TODO: MEIs (ClusterId and AttributeId) have a invalid pattern instead of a single invalid value, need to add separate
// functions for checking if we have received valid values.
err = path.GetEndpointId(&(clusterInfo.mEndpointId));
if (err == CHIP_NO_ERROR)
{
VerifyOrExit(clusterInfo.mEndpointId != ClusterInfo::kInvalidEndpointId, err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
}
SuccessOrExit(err);
err = path.GetClusterId(&(clusterInfo.mClusterId));
if (err == CHIP_NO_ERROR)
{
VerifyOrExit(clusterInfo.mClusterId != ClusterInfo::kInvalidClusterId, err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
}

SuccessOrExit(err);
err = path.GetFieldId(&(clusterInfo.mFieldId));
if (CHIP_NO_ERROR == err)
if (CHIP_END_OF_TLV == err)
{
clusterInfo.mFlags.Set(ClusterInfo::Flags::kFieldIdValid);
err = CHIP_NO_ERROR;
}
else if (CHIP_END_OF_TLV == err)
else if (err == CHIP_NO_ERROR)
{
err = CHIP_NO_ERROR;
VerifyOrExit(clusterInfo.mFieldId != ClusterInfo::kInvalidAttributeId, err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
}
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);
VerifyOrExit(clusterInfo.mFieldId != ClusterInfo::kInvalidAttributeId, err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
}
else if (CHIP_END_OF_TLV == err)
{
Expand Down Expand Up @@ -398,11 +407,7 @@ CHIP_ERROR ReadHandler::ProcessEventPaths(EventPaths::Parser & aEventPathsParser
err = path.GetCluster(&(clusterInfo.mClusterId));
SuccessOrExit(err);
err = path.GetEvent(&(clusterInfo.mEventId));
if (CHIP_NO_ERROR == err)
{
clusterInfo.mFlags.Set(ClusterInfo::Flags::kEventIdValid);
}
else if (CHIP_END_OF_TLV == err)
if (CHIP_END_OF_TLV == err)
{
err = CHIP_NO_ERROR;
}
Expand Down
21 changes: 9 additions & 12 deletions src/app/WriteHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,10 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataList(TLV::TLVReader & aAttributeDat
SuccessOrExit(err);

err = attributePath.GetNodeId(&(clusterInfo.mNodeId));
SuccessOrExit(err);
if (CHIP_END_OF_TLV == err)
{
err = CHIP_NO_ERROR;
}

err = attributePath.GetEndpointId(&(clusterInfo.mEndpointId));
SuccessOrExit(err);
Expand All @@ -133,23 +136,17 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataList(TLV::TLVReader & aAttributeDat
SuccessOrExit(err);

err = attributePath.GetFieldId(&(clusterInfo.mFieldId));
if (CHIP_NO_ERROR == err)
{
clusterInfo.mFlags.Set(ClusterInfo::Flags::kFieldIdValid);
}
else if (CHIP_END_OF_TLV == err)
{
err = CHIP_NO_ERROR;
}
SuccessOrExit(err);

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

VerifyOrExit(clusterInfo.IsValidAttributePath() && !clusterInfo.HasWildcard(),
err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);

err = element.GetData(&dataReader);
SuccessOrExit(err);

Expand Down
3 changes: 1 addition & 2 deletions src/app/reporting/Engine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,7 @@ Engine::RetrieveClusterData(FabricIndex aAccessingFabricIndex, AttributeDataList
ConcreteAttributePath path(aClusterInfo.mEndpointId, aClusterInfo.mClusterId, aClusterInfo.mFieldId);
AttributeDataElement::Builder attributeDataElementBuilder = aAttributeDataList.CreateAttributeDataElementBuilder();
AttributePath::Builder attributePathBuilder = attributeDataElementBuilder.CreateAttributePathBuilder();
attributePathBuilder.NodeId(aClusterInfo.mNodeId)
.EndpointId(aClusterInfo.mEndpointId)
attributePathBuilder.EndpointId(aClusterInfo.mEndpointId)
.ClusterId(aClusterInfo.mClusterId)
.FieldId(aClusterInfo.mFieldId)
.EndOfAttributePath();
Expand Down
18 changes: 6 additions & 12 deletions src/app/tests/TestClusterInfo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,15 @@ void TestAttributePathIncludedSameFieldId(nlTestSuite * apSuite, void * apContex
ClusterInfo clusterInfo1;
ClusterInfo clusterInfo2;
ClusterInfo clusterInfo3;
clusterInfo1.mFlags.Set(ClusterInfo::Flags::kFieldIdValid);
clusterInfo2.mFlags.Set(ClusterInfo::Flags::kFieldIdValid);
clusterInfo3.mFlags.Set(ClusterInfo::Flags::kFieldIdValid);
clusterInfo1.mFieldId = 1;
clusterInfo2.mFieldId = 1;
clusterInfo3.mFieldId = 1;
NL_TEST_ASSERT(apSuite, clusterInfo1.IsAttributePathSupersetOf(clusterInfo2));
clusterInfo2.mFlags.Set(ClusterInfo::Flags::kListIndexValid);
clusterInfo2.mListIndex = 1;
NL_TEST_ASSERT(apSuite, clusterInfo1.IsAttributePathSupersetOf(clusterInfo2));
clusterInfo1.mFlags.Set(ClusterInfo::Flags::kListIndexValid);
clusterInfo1.mListIndex = 0;
NL_TEST_ASSERT(apSuite, !clusterInfo1.IsAttributePathSupersetOf(clusterInfo3));
clusterInfo3.mFlags.Set(ClusterInfo::Flags::kListIndexValid);
clusterInfo3.mListIndex = 0;
NL_TEST_ASSERT(apSuite, clusterInfo1.IsAttributePathSupersetOf(clusterInfo3));
clusterInfo3.mListIndex = 1;
NL_TEST_ASSERT(apSuite, !clusterInfo1.IsAttributePathSupersetOf(clusterInfo3));
Expand All @@ -56,19 +52,17 @@ void TestAttributePathIncludedDifferentFieldId(nlTestSuite * apSuite, void * apC
{
ClusterInfo clusterInfo1;
ClusterInfo clusterInfo2;
clusterInfo1.mFlags.Set(ClusterInfo::Flags::kFieldIdValid);
clusterInfo2.mFlags.Set(ClusterInfo::Flags::kFieldIdValid);
clusterInfo1.mFieldId = 1;
clusterInfo2.mFieldId = 2;
NL_TEST_ASSERT(apSuite, !clusterInfo1.IsAttributePathSupersetOf(clusterInfo2));
clusterInfo1.mFieldId = 0xFFFFFFFF;
clusterInfo1.mFieldId = ClusterInfo::kInvalidAttributeId;
clusterInfo2.mFieldId = 2;
NL_TEST_ASSERT(apSuite, clusterInfo1.IsAttributePathSupersetOf(clusterInfo2));
clusterInfo1.mFieldId = 0xFFFFFFFF;
clusterInfo2.mFieldId = 0xFFFFFFFF;
clusterInfo1.mFieldId = ClusterInfo::kInvalidAttributeId;
clusterInfo2.mFieldId = ClusterInfo::kInvalidAttributeId;
NL_TEST_ASSERT(apSuite, clusterInfo1.IsAttributePathSupersetOf(clusterInfo2));
clusterInfo1.mFieldId = 1;
clusterInfo2.mFieldId = 0xFFFFFFFF;
clusterInfo2.mFieldId = ClusterInfo::kInvalidAttributeId;
NL_TEST_ASSERT(apSuite, !clusterInfo1.IsAttributePathSupersetOf(clusterInfo2));
}

Expand Down
Loading

0 comments on commit 9072d37

Please sign in to comment.