Skip to content

Commit

Permalink
[im] Fix wildcard path support per spec (#11115)
Browse files Browse the repository at this point in the history 
* [im] Fix wildcard path support per spec

* Fix

* Move kInvalid* to private
  • Loading branch information
@erjiaqing @pull
erjiaqing authored and pull[bot] committed Jun 23, 2022
1 parent b3c3d1d commit 1308664
Show file tree
Hide file tree
Showing 14 changed files with 180 additions and 185 deletions.
127 changes: 49 additions & 78 deletions src/app/ClusterInfo.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,103 +18,74 @@

#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, it uses some invalid values for representing the wildcard values for its fields 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,
};

private:
// 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;

public:
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();
VerifyOrReturnError(HasWildcardEndpointId() || mEndpointId == other.mEndpointId, false);
VerifyOrReturnError(HasWildcardClusterId() || mClusterId == other.mClusterId, false);
VerifyOrReturnError(HasWildcardAttributeId() || mFieldId == other.mFieldId, false);
VerifyOrReturnError(HasWildcardListIndex() || mListIndex == other.mListIndex, false);

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

// 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;
}
/**
* Check that the path meets some basic constraints of an attribute path: If list index is not wildcard, then field id must not
* be wildcard. This does not verify that the attribute being targeted is actually of list type when the list index is not
* wildcard.
*/
bool IsValidAttributePath() const { return HasWildcardListIndex() || !HasWildcardAttributeId(); }

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
* 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
/*
* For better structure alignment
* Below ordering is by bit-size to ensure least amount of memory alignment padding.
* 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
*/
NodeId mNodeId = kUndefinedNodeId; // uint64
ClusterInfo * mpNext = nullptr; // pointer width (32/64 bits)
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 @@ -663,6 +663,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
15 changes: 10 additions & 5 deletions src/app/MessageDef/AttributePath.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ class Parser : public chip::app::Parser
CHIP_ERROR CheckSchemaValidity() const;
#endif
/**
* @brief Get a TLVReader for the NodeId. Next() must be called before accessing them.
* @brief Get a TLVReader for the NodeId. Next() must be called before accessing them. If the node id field does not exist, the
* nodeid will not be touched.
*
* @param [in] apNodeId A pointer to apNodeId
*
Expand All @@ -85,7 +86,8 @@ class Parser : public chip::app::Parser
CHIP_ERROR GetNodeId(chip::NodeId * const apNodeId) const;

/**
* @brief Get a TLVReader for the EndpointId. Next() must be called before accessing them.
* @brief Get a TLVReader for the EndpointId. Next() must be called before accessing them. If the endpoint id field does not
* exist, the value will not be touched.
*
* @param [in] apEndpointId A pointer to apEndpointId
*
Expand All @@ -96,7 +98,8 @@ class Parser : public chip::app::Parser
CHIP_ERROR GetEndpointId(chip::EndpointId * const apEndpointId) const;

/**
* @brief Get a TLVReader for the ClusterId. Next() must be called before accessing them.
* @brief Get a TLVReader for the ClusterId. Next() must be called before accessing them. If the cluster id field does not
* exist, the value will not be touched.
*
* @param [in] apClusterId A pointer to apClusterId
*
Expand All @@ -107,7 +110,8 @@ class Parser : public chip::app::Parser
CHIP_ERROR GetClusterId(chip::ClusterId * const apClusterId) const;

/**
* @brief Get a TLVReader for the FieldId. Next() must be called before accessing them.
* @brief Get a TLVReader for the FieldId. Next() must be called before accessing them. If the field id field does not exist,
* the value will not be touched.
*
* @param [in] apFieldId A pointer to apFieldId
*
Expand All @@ -118,7 +122,8 @@ class Parser : public chip::app::Parser
CHIP_ERROR GetFieldId(chip::AttributeId * const apFieldId) const;

/**
* @brief Get a TLVReader for the ListIndex. Next() must be called before accessing them.
* @brief Get a TLVReader for the ListIndex. Next() must be called before accessing them. If the list index field does not
* exist, the value will not be touched.
*
* @param [in] apListIndex A pointer to apListIndex
*
Expand Down
18 changes: 16 additions & 2 deletions src/app/MessageDef/Parser.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,23 +74,37 @@ class Parser
chip::TLV::TLVType mOuterContainerType;
Parser();

/**
* Gets a unsigned integer value with the given tag, the value is not touched when the tag is not found in the TLV.
*
* @return #CHIP_NO_ERROR on success
* #CHIP_ERROR_WRONG_TLV_TYPE if there is such element but it's not any of the defined unsigned integer types
* #CHIP_END_OF_TLV if there is no such element
*/
template <typename T>
CHIP_ERROR GetUnsignedInteger(const uint8_t aContextTag, T * const apLValue) const
{
return GetSimpleValue(aContextTag, chip::TLV::kTLVType_UnsignedInteger, apLValue);
};

/**
* Gets a scalar value with the given tag, the value is not touched when the tag is not found in the TLV.
*
* @return #CHIP_NO_ERROR on success
* #CHIP_ERROR_WRONG_TLV_TYPE if there is such element but it's not any of the defined unsigned integer types
* #CHIP_END_OF_TLV if there is no such element
*/
template <typename T>
CHIP_ERROR GetSimpleValue(const uint8_t aContextTag, const chip::TLV::TLVType aTLVType, T * const apLValue) const
{
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
38 changes: 18 additions & 20 deletions src/app/ReadClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -473,39 +473,37 @@ CHIP_ERROR ReadClient::ProcessAttributeDataList(TLV::TLVReader & aAttributeDataL
SuccessOrExit(err);

err = element.GetAttributePath(&attributePathParser);
SuccessOrExit(err);
VerifyOrExit(err == CHIP_NO_ERROR, err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);

// We are using the feature that the parser won't touch the value if the field does not exist, since all fields in the
// cluster info will be invalid / wildcard, it is safe ignore CHIP_END_OF_TLV directly.

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

// The ReportData must contain a concrete attribute path

err = attributePathParser.GetEndpointId(&(clusterInfo.mEndpointId));
SuccessOrExit(err);
VerifyOrExit(err == CHIP_NO_ERROR, err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);

err = attributePathParser.GetClusterId(&(clusterInfo.mClusterId));
SuccessOrExit(err);
VerifyOrExit(err == CHIP_NO_ERROR, err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);

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);
VerifyOrExit(err == CHIP_NO_ERROR, err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);

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;
}
SuccessOrExit(err);
VerifyOrExit(err == CHIP_NO_ERROR, err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);

VerifyOrExit(clusterInfo.IsValidAttributePath(), err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);

err = element.GetData(&dataReader);
if (err == CHIP_END_OF_TLV)
Expand Down
32 changes: 19 additions & 13 deletions src/app/ReadHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,28 +335,38 @@ 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.HasWildcardEndpointId(), err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
}
SuccessOrExit(err);
err = path.GetClusterId(&(clusterInfo.mClusterId));
if (err == CHIP_NO_ERROR)
{
VerifyOrExit(!clusterInfo.HasWildcardClusterId(), 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.HasWildcardAttributeId(), 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.HasWildcardAttributeId() && !clusterInfo.HasWildcardListIndex(),
err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
}
else if (CHIP_END_OF_TLV == err)
{
Expand Down Expand Up @@ -398,11 +408,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
Loading

0 comments on commit 1308664

Please sign in to comment.