Skip to content

Commit 9898361

Browse files
erjiaqingpull[bot]
authored andcommitted
[im] Fix wildcard path support per spec (#11115)
* [im] Fix wildcard path support per spec * Fix * Move kInvalid* to private
1 parent 99d19cc commit 9898361

14 files changed

+180
-185
lines changed

src/app/ClusterInfo.h

+49-78
Original file line numberDiff line numberDiff line change
@@ -18,103 +18,74 @@
1818

1919
#pragma once
2020

21-
#include <app/AttributePathParams.h>
2221
#include <app/util/basic-types.h>
2322
#include <assert.h>
2423
#include <lib/core/Optional.h>
2524

2625
namespace chip {
2726
namespace app {
28-
static constexpr AttributeId kRootAttributeId = 0xFFFFFFFF;
2927

28+
/**
29+
* ClusterInfo is the representation of an attribute path or an event path used by ReadHandler, ReadClient, WriteHandler,
30+
* Report::Engine etc, it uses some invalid values for representing the wildcard values for its fields and contains a mpNext field
31+
* so it can be used as a linked list.
32+
*/
33+
// TODO: The cluster info should be separated into AttributeInfo and EventInfo.
34+
// Note: The change will happen after #11171 with a better linked list.
3035
struct ClusterInfo
3136
{
32-
enum class Flags : uint8_t
33-
{
34-
kFieldIdValid = 0x01,
35-
kListIndexValid = 0x02,
36-
kEventIdValid = 0x03,
37-
};
38-
37+
private:
38+
// Endpoint Id is a uint16 number, and should between 0 and 0xFFFE
39+
static constexpr EndpointId kInvalidEndpointId = 0xFFFF;
40+
// The ClusterId, AttributeId and EventId are MEIs,
41+
// 0xFFFF is not a valid manufacturer code, thus 0xFFFF'FFFF is not a valid MEI
42+
static constexpr ClusterId kInvalidClusterId = 0xFFFF'FFFF;
43+
static constexpr AttributeId kInvalidAttributeId = 0xFFFF'FFFF;
44+
static constexpr EventId kInvalidEventId = 0xFFFF'FFFF;
45+
// ListIndex is a uint16 number, thus 0xFFFF is not a valid list index.
46+
static constexpr ListIndex kInvalidListIndex = 0xFFFF;
47+
48+
public:
3949
bool IsAttributePathSupersetOf(const ClusterInfo & other) const
4050
{
41-
if ((other.mEndpointId != mEndpointId) || (other.mClusterId != mClusterId))
42-
{
43-
return false;
44-
}
45-
46-
Optional<AttributeId> myFieldId =
47-
mFlags.Has(Flags::kFieldIdValid) ? Optional<AttributeId>::Value(mFieldId) : Optional<AttributeId>::Missing();
48-
49-
Optional<AttributeId> otherFieldId = other.mFlags.Has(Flags::kFieldIdValid) ? Optional<AttributeId>::Value(other.mFieldId)
50-
: Optional<AttributeId>::Missing();
51-
52-
Optional<ListIndex> myListIndex =
53-
mFlags.Has(Flags::kListIndexValid) ? Optional<ListIndex>::Value(mListIndex) : Optional<ListIndex>::Missing();
51+
VerifyOrReturnError(HasWildcardEndpointId() || mEndpointId == other.mEndpointId, false);
52+
VerifyOrReturnError(HasWildcardClusterId() || mClusterId == other.mClusterId, false);
53+
VerifyOrReturnError(HasWildcardAttributeId() || mFieldId == other.mFieldId, false);
54+
VerifyOrReturnError(HasWildcardListIndex() || mListIndex == other.mListIndex, false);
5455

55-
Optional<ListIndex> otherListIndex = other.mFlags.Has(Flags::kListIndexValid) ? Optional<ListIndex>::Value(other.mListIndex)
56-
: Optional<ListIndex>::Missing();
57-
58-
// If list index exists, field index must exist
59-
// Field 0xFFFFFFF (any) & listindex set is invalid
60-
assert(!(myListIndex.HasValue() && !myFieldId.HasValue()));
61-
assert(!(otherListIndex.HasValue() && !otherFieldId.HasValue()));
62-
assert(!(myFieldId == Optional<AttributeId>::Value(kRootAttributeId) && myListIndex.HasValue()));
63-
assert(!(otherFieldId == Optional<AttributeId>::Value(kRootAttributeId) && otherListIndex.HasValue()));
64-
65-
if (myFieldId == Optional<AttributeId>::Value(kRootAttributeId))
66-
{
67-
return true;
68-
}
69-
70-
if (myFieldId != otherFieldId)
71-
{
72-
return false;
73-
}
56+
return true;
57+
}
7458

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

85-
if (!myListIndex.HasValue())
86-
{
87-
// difference is ok only if myListIndex is missing
88-
return true;
89-
}
61+
/**
62+
* Check that the path meets some basic constraints of an attribute path: If list index is not wildcard, then field id must not
63+
* be wildcard. This does not verify that the attribute being targeted is actually of list type when the list index is not
64+
* wildcard.
65+
*/
66+
bool IsValidAttributePath() const { return HasWildcardListIndex() || !HasWildcardAttributeId(); }
9067

91-
return false;
92-
}
68+
inline bool HasWildcardNodeId() const { return mNodeId == kUndefinedNodeId; }
69+
inline bool HasWildcardEndpointId() const { return mEndpointId == kInvalidEndpointId; }
70+
inline bool HasWildcardClusterId() const { return mClusterId == kInvalidClusterId; }
71+
inline bool HasWildcardAttributeId() const { return mFieldId == kInvalidAttributeId; }
72+
inline bool HasWildcardListIndex() const { return mListIndex == kInvalidListIndex; }
73+
inline bool HasWildcardEventId() const { return mEventId == kInvalidEventId; }
9374

9475
ClusterInfo() {}
95-
NodeId mNodeId = 0;
96-
ClusterId mClusterId = 0;
97-
ListIndex mListIndex = 0;
98-
AttributeId mFieldId = 0;
99-
EndpointId mEndpointId = 0;
100-
BitFlags<Flags> mFlags;
101-
ClusterInfo * mpNext = nullptr;
102-
EventId mEventId = 0;
103-
/* For better structure alignment
104-
* Above ordering is by bit-size to ensure least amount of memory alignment padding.
105-
* Changing order to something more natural (e.g. clusterid before nodeid) will result
76+
/*
77+
* For better structure alignment
78+
* Below ordering is by bit-size to ensure least amount of memory alignment padding.
79+
* Changing order to something more natural (e.g. endpoint id before cluster id) will result
10680
* in extra memory alignment padding.
107-
* uint64 mNodeId
108-
* uint16_t mClusterId
109-
* uint16_t mListIndex
110-
* uint8_t FieldId
111-
* uint8_t EndpointId
112-
* uint8_t mDirty
113-
* uint8_t mType
114-
* uint32_t mpNext
115-
* uint16_t EventId
116-
* padding 2 bytes
11781
*/
82+
NodeId mNodeId = kUndefinedNodeId; // uint64
83+
ClusterInfo * mpNext = nullptr; // pointer width (32/64 bits)
84+
ClusterId mClusterId = kInvalidClusterId; // uint32
85+
AttributeId mFieldId = kInvalidAttributeId; // uint32
86+
EventId mEventId = kInvalidEventId; // uint32
87+
ListIndex mListIndex = kInvalidListIndex; // uint16
88+
EndpointId mEndpointId = kInvalidEndpointId; // uint16
11889
};
11990
} // namespace app
12091
} // namespace chip

src/app/EventManagement.cpp

+1
Original file line numberDiff line numberDiff line change
@@ -663,6 +663,7 @@ static bool IsInterestedEventPaths(EventLoadOutContext * eventLoadOutContext, co
663663
}
664664
while (interestedEventPaths != nullptr)
665665
{
666+
// TODO: Support wildcard event path
666667
if (interestedEventPaths->mNodeId == event.mNodeId && interestedEventPaths->mEndpointId == event.mEndpointId &&
667668
interestedEventPaths->mClusterId == event.mClusterId && interestedEventPaths->mEventId == event.mEventId)
668669
{

src/app/InteractionModelEngine.cpp

-2
Original file line numberDiff line numberDiff line change
@@ -465,7 +465,6 @@ void InteractionModelEngine::ReleaseClusterInfoList(ClusterInfo *& aClusterInfo)
465465
{
466466
lastClusterInfo = lastClusterInfo->mpNext;
467467
}
468-
lastClusterInfo->mFlags.ClearAll();
469468
lastClusterInfo->mpNext = mpNextAvailableClusterInfo;
470469
mpNextAvailableClusterInfo = aClusterInfo;
471470
aClusterInfo = nullptr;
@@ -502,7 +501,6 @@ bool InteractionModelEngine::MergeOverlappedAttributePath(ClusterInfo * apAttrib
502501
{
503502
runner->mListIndex = aAttributePath.mListIndex;
504503
runner->mFieldId = aAttributePath.mFieldId;
505-
runner->mFlags = aAttributePath.mFlags;
506504
return true;
507505
}
508506
runner = runner->mpNext;

src/app/MessageDef/AttributePath.h

+10-5
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,8 @@ class Parser : public chip::app::Parser
7474
CHIP_ERROR CheckSchemaValidity() const;
7575
#endif
7676
/**
77-
* @brief Get a TLVReader for the NodeId. Next() must be called before accessing them.
77+
* @brief Get a TLVReader for the NodeId. Next() must be called before accessing them. If the node id field does not exist, the
78+
* nodeid will not be touched.
7879
*
7980
* @param [in] apNodeId A pointer to apNodeId
8081
*
@@ -85,7 +86,8 @@ class Parser : public chip::app::Parser
8586
CHIP_ERROR GetNodeId(chip::NodeId * const apNodeId) const;
8687

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

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

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

120124
/**
121-
* @brief Get a TLVReader for the ListIndex. Next() must be called before accessing them.
125+
* @brief Get a TLVReader for the ListIndex. Next() must be called before accessing them. If the list index field does not
126+
* exist, the value will not be touched.
122127
*
123128
* @param [in] apListIndex A pointer to apListIndex
124129
*

src/app/MessageDef/Parser.h

+16-2
Original file line numberDiff line numberDiff line change
@@ -74,23 +74,37 @@ class Parser
7474
chip::TLV::TLVType mOuterContainerType;
7575
Parser();
7676

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

90+
/**
91+
* Gets a scalar value with the given tag, the value is not touched when the tag is not found in the TLV.
92+
*
93+
* @return #CHIP_NO_ERROR on success
94+
* #CHIP_ERROR_WRONG_TLV_TYPE if there is such element but it's not any of the defined unsigned integer types
95+
* #CHIP_END_OF_TLV if there is no such element
96+
*/
8397
template <typename T>
8498
CHIP_ERROR GetSimpleValue(const uint8_t aContextTag, const chip::TLV::TLVType aTLVType, T * const apLValue) const
8599
{
86100
CHIP_ERROR err = CHIP_NO_ERROR;
87101
chip::TLV::TLVReader reader;
88102

89-
*apLValue = 0;
90-
91103
err = mReader.FindElementWithTag(chip::TLV::ContextTag(aContextTag), reader);
92104
SuccessOrExit(err);
93105

106+
*apLValue = 0;
107+
94108
VerifyOrExit(aTLVType == reader.GetType(), err = CHIP_ERROR_WRONG_TLV_TYPE);
95109

96110
err = reader.Get(*apLValue);

src/app/ReadClient.cpp

+18-20
Original file line numberDiff line numberDiff line change
@@ -473,39 +473,37 @@ CHIP_ERROR ReadClient::ProcessAttributeDataList(TLV::TLVReader & aAttributeDataL
473473
SuccessOrExit(err);
474474

475475
err = element.GetAttributePath(&attributePathParser);
476-
SuccessOrExit(err);
476+
VerifyOrExit(err == CHIP_NO_ERROR, err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
477+
478+
// We are using the feature that the parser won't touch the value if the field does not exist, since all fields in the
479+
// cluster info will be invalid / wildcard, it is safe ignore CHIP_END_OF_TLV directly.
477480

478481
err = attributePathParser.GetNodeId(&(clusterInfo.mNodeId));
479-
SuccessOrExit(err);
482+
if (err == CHIP_END_OF_TLV)
483+
{
484+
err = CHIP_NO_ERROR;
485+
}
486+
VerifyOrExit(err == CHIP_NO_ERROR, err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
487+
488+
// The ReportData must contain a concrete attribute path
480489

481490
err = attributePathParser.GetEndpointId(&(clusterInfo.mEndpointId));
482-
SuccessOrExit(err);
491+
VerifyOrExit(err == CHIP_NO_ERROR, err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
483492

484493
err = attributePathParser.GetClusterId(&(clusterInfo.mClusterId));
485-
SuccessOrExit(err);
494+
VerifyOrExit(err == CHIP_NO_ERROR, err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
486495

487496
err = attributePathParser.GetFieldId(&(clusterInfo.mFieldId));
488-
if (CHIP_NO_ERROR == err)
489-
{
490-
clusterInfo.mFlags.Set(ClusterInfo::Flags::kFieldIdValid);
491-
}
492-
else if (CHIP_END_OF_TLV == err)
493-
{
494-
err = CHIP_NO_ERROR;
495-
}
496-
SuccessOrExit(err);
497+
VerifyOrExit(err == CHIP_NO_ERROR, err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
497498

498499
err = attributePathParser.GetListIndex(&(clusterInfo.mListIndex));
499-
if (CHIP_NO_ERROR == err)
500-
{
501-
VerifyOrExit(clusterInfo.mFlags.Has(ClusterInfo::Flags::kFieldIdValid), err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
502-
clusterInfo.mFlags.Set(ClusterInfo::Flags::kListIndexValid);
503-
}
504-
else if (CHIP_END_OF_TLV == err)
500+
if (CHIP_END_OF_TLV == err)
505501
{
506502
err = CHIP_NO_ERROR;
507503
}
508-
SuccessOrExit(err);
504+
VerifyOrExit(err == CHIP_NO_ERROR, err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
505+
506+
VerifyOrExit(clusterInfo.IsValidAttributePath(), err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
509507

510508
err = element.GetData(&dataReader);
511509
if (err == CHIP_END_OF_TLV)

src/app/ReadHandler.cpp

+19-13
Original file line numberDiff line numberDiff line change
@@ -335,28 +335,38 @@ CHIP_ERROR ReadHandler::ProcessAttributePathList(AttributePathList::Parser & aAt
335335
AttributePath::Parser path;
336336
err = path.Init(reader);
337337
SuccessOrExit(err);
338-
err = path.GetNodeId(&(clusterInfo.mNodeId));
339-
SuccessOrExit(err);
338+
// TODO: Support wildcard paths here
339+
// TODO: MEIs (ClusterId and AttributeId) have a invalid pattern instead of a single invalid value, need to add separate
340+
// functions for checking if we have received valid values.
340341
err = path.GetEndpointId(&(clusterInfo.mEndpointId));
342+
if (err == CHIP_NO_ERROR)
343+
{
344+
VerifyOrExit(!clusterInfo.HasWildcardEndpointId(), err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
345+
}
341346
SuccessOrExit(err);
342347
err = path.GetClusterId(&(clusterInfo.mClusterId));
348+
if (err == CHIP_NO_ERROR)
349+
{
350+
VerifyOrExit(!clusterInfo.HasWildcardClusterId(), err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
351+
}
352+
343353
SuccessOrExit(err);
344354
err = path.GetFieldId(&(clusterInfo.mFieldId));
345-
if (CHIP_NO_ERROR == err)
355+
if (CHIP_END_OF_TLV == err)
346356
{
347-
clusterInfo.mFlags.Set(ClusterInfo::Flags::kFieldIdValid);
357+
err = CHIP_NO_ERROR;
348358
}
349-
else if (CHIP_END_OF_TLV == err)
359+
else if (err == CHIP_NO_ERROR)
350360
{
351-
err = CHIP_NO_ERROR;
361+
VerifyOrExit(!clusterInfo.HasWildcardAttributeId(), err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
352362
}
353363
SuccessOrExit(err);
354364

355365
err = path.GetListIndex(&(clusterInfo.mListIndex));
356366
if (CHIP_NO_ERROR == err)
357367
{
358-
VerifyOrExit(clusterInfo.mFlags.Has(ClusterInfo::Flags::kFieldIdValid), err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
359-
clusterInfo.mFlags.Set(ClusterInfo::Flags::kListIndexValid);
368+
VerifyOrExit(!clusterInfo.HasWildcardAttributeId() && !clusterInfo.HasWildcardListIndex(),
369+
err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
360370
}
361371
else if (CHIP_END_OF_TLV == err)
362372
{
@@ -398,11 +408,7 @@ CHIP_ERROR ReadHandler::ProcessEventPaths(EventPaths::Parser & aEventPathsParser
398408
err = path.GetCluster(&(clusterInfo.mClusterId));
399409
SuccessOrExit(err);
400410
err = path.GetEvent(&(clusterInfo.mEventId));
401-
if (CHIP_NO_ERROR == err)
402-
{
403-
clusterInfo.mFlags.Set(ClusterInfo::Flags::kEventIdValid);
404-
}
405-
else if (CHIP_END_OF_TLV == err)
411+
if (CHIP_END_OF_TLV == err)
406412
{
407413
err = CHIP_NO_ERROR;
408414
}

0 commit comments

Comments
 (0)