Skip to content

Commit

Permalink
Merge branch 'master' into enforce-length-constraints
Browse files Browse the repository at this point in the history
  • Loading branch information
@mlepage-google
mlepage-google committed Apr 29, 2022
2 parents a4d0cd3 + 9dc2682 commit c988931
Show file tree
Hide file tree
Showing 6 changed files with 11 additions and 307 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/darwin-tests.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ jobs:
./scripts/run_in_build_env.sh \
"./scripts/tests/run_test_suite.py \
--chip-tool ./out/darwin-x64-chip-tool-darwin-${BUILD_VARIANT}/chip-tool-darwin \
--target-skip-glob '{TestGroupMessaging,TV_*}' \
--target-skip-glob '{TestGroupMessaging,TV_*,DL_*}' \
run \
--iterations 1 \
--all-clusters-app ./out/darwin-x64-all-clusters-${BUILD_VARIANT}/chip-all-clusters-app \
Expand Down
13 changes: 0 additions & 13 deletions src/app/AttributePathParams.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,6 @@ struct AttributePathParams

bool IsWildcardPath() const { return HasWildcardEndpointId() || HasWildcardClusterId() || HasWildcardAttributeId(); }

bool operator==(const AttributePathParams & aOther) const
{
return mEndpointId == aOther.mEndpointId && mClusterId == aOther.mClusterId && mAttributeId == aOther.mAttributeId &&
mListIndex == aOther.mListIndex;
}

/**
* SPEC 8.9.2.2
* Check that the path meets some basic constraints of an attribute path: If list index is not wildcard, then field id must not
Expand All @@ -69,13 +63,6 @@ struct AttributePathParams
inline bool HasWildcardClusterId() const { return mClusterId == kInvalidClusterId; }
inline bool HasWildcardAttributeId() const { return mAttributeId == kInvalidAttributeId; }
inline bool HasWildcardListIndex() const { return mListIndex == kInvalidListIndex; }
inline void SetWildcardEndpointId() { mEndpointId = kInvalidEndpointId; }
inline void SetWildcardClusterId() { mClusterId = kInvalidClusterId; }
inline void SetWildcardAttributeId()
{
mAttributeId = kInvalidAttributeId;
mListIndex = kInvalidListIndex;
}

bool IsAttributePathSupersetOf(const AttributePathParams & other) const
{
Expand Down
124 changes: 10 additions & 114 deletions src/app/reporting/Engine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -658,116 +658,6 @@ bool Engine::MergeOverlappedAttributePath(const AttributePathParams & aAttribute
});
}

bool Engine::ClearTombPaths()
{
bool pathReleased = false;
mGlobalDirtySet.ForEachActiveObject([&](auto * path) {
if (path->mGeneration == 0)
{
mGlobalDirtySet.ReleaseObject(path);
pathReleased = true;
}
return Loop::Continue;
});
return pathReleased;
}

bool Engine::MergeDirtyPathsUnderSameCluster()
{
mGlobalDirtySet.ForEachActiveObject([&](auto * outerPath) {
if (outerPath->HasWildcardClusterId() || outerPath->mGeneration == 0)
{
return Loop::Continue;
}
mGlobalDirtySet.ForEachActiveObject([&](auto * innerPath) {
if (innerPath == outerPath)
{
return Loop::Continue;
}
// We don't support paths with a wildcard endpoint + a concrete cluster in global dirty set, so we do a simple == check
// here.
if (innerPath->mEndpointId != outerPath->mEndpointId || innerPath->mClusterId != outerPath->mClusterId)
{
return Loop::Continue;
}
if (innerPath->mGeneration > outerPath->mGeneration)
{
outerPath->mGeneration = innerPath->mGeneration;
}
outerPath->SetWildcardAttributeId();

// The object pool does not allow us to release objects in a nested iteration, mark the path as a tomb by setting its
// generation to 0 and then clear it later.
innerPath->mGeneration = 0;
return Loop::Continue;
});
return Loop::Continue;
});

return ClearTombPaths();
}

bool Engine::MergeDirtyPathsUnderSameEndpoint()
{
mGlobalDirtySet.ForEachActiveObject([&](auto * outerPath) {
if (outerPath->HasWildcardEndpointId() || outerPath->mGeneration == 0)
{
return Loop::Continue;
}
mGlobalDirtySet.ForEachActiveObject([&](auto * innerPath) {
if (innerPath == outerPath)
{
return Loop::Continue;
}
if (innerPath->mEndpointId != outerPath->mEndpointId)
{
return Loop::Continue;
}
if (innerPath->mGeneration > outerPath->mGeneration)
{
outerPath->mGeneration = innerPath->mGeneration;
}
outerPath->SetWildcardClusterId();
outerPath->SetWildcardAttributeId();

// The object pool does not allow us to release objects in a nested iteration, mark the path as a tomb by setting its
// generation to 0 and then clear it later.
innerPath->mGeneration = 0;
return Loop::Continue;
});
return Loop::Continue;
});
return ClearTombPaths();
}

CHIP_ERROR Engine::InsertPathIntoDirtySet(const AttributePathParams & aAttributePath)
{
ReturnErrorCodeIf(MergeOverlappedAttributePath(aAttributePath), CHIP_NO_ERROR);

if (mGlobalDirtySet.Exhausted() && !MergeDirtyPathsUnderSameCluster() && !MergeDirtyPathsUnderSameEndpoint())
{
ChipLogDetail(DataManagement, "Global dirty set pool exhausted, merge all paths.");
mGlobalDirtySet.ReleaseAll();
auto object = mGlobalDirtySet.CreateObject();
object->mGeneration = GetDirtySetGeneration();
}

ReturnErrorCodeIf(MergeOverlappedAttributePath(aAttributePath), CHIP_NO_ERROR);
ChipLogDetail(DataManagement, "Cannot merge the new path into any existing path, create one.");

auto object = mGlobalDirtySet.CreateObject();
if (object == nullptr)
{
// This should not happen, this path should be merged into the wildcard endpoint at least.
ChipLogError(DataManagement, "mGlobalDirtySet pool full, cannot handle more entries!");
return CHIP_ERROR_NO_MEMORY;
}
*object = aAttributePath;
object->mGeneration = GetDirtySetGeneration();

return CHIP_NO_ERROR;
}

CHIP_ERROR Engine::SetDirty(AttributePathParams & aAttributePath)
{
BumpDirtySetGeneration();
Expand All @@ -792,13 +682,19 @@ CHIP_ERROR Engine::SetDirty(AttributePathParams & aAttributePath)
return Loop::Continue;
});

if (!InteractionModelEngine::GetInstance()->IsOverlappedAttributePath(aAttributePath))
if (!MergeOverlappedAttributePath(aAttributePath) &&
InteractionModelEngine::GetInstance()->IsOverlappedAttributePath(aAttributePath))
{
return CHIP_NO_ERROR;
auto object = mGlobalDirtySet.CreateObject();
if (object == nullptr)
{
ChipLogError(DataManagement, "mGlobalDirtySet pool full, cannot handle more entries!");
return CHIP_ERROR_NO_MEMORY;
}
*object = aAttributePath;
object->mGeneration = GetDirtySetGeneration();
}

ReturnErrorOnFailure(InsertPathIntoDirtySet(aAttributePath));

// Schedule work to run asynchronously on the CHIP thread. The scheduled
// work won't execute until the current execution context has
// completed. This ensures that we can 'gather up' multiple attribute
Expand Down
30 changes: 0 additions & 30 deletions src/app/reporting/Engine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,31 +188,6 @@ class Engine
*/
bool MergeOverlappedAttributePath(const AttributePathParams & aAttributePath);

/**
* If we are running out of ObjectPool for the global dirty set, we will try to merge the existing items by clusters.
*
* Returns whether we have released any paths.
*/
bool MergeDirtyPathsUnderSameCluster();

/**
* If we are running out of ObjectPool for the global dirty set and we cannot find a slot after merging the existing items by
* clusters, we will try to merge the existing items by endpoints.
*
* Returns whether we have released any paths.
*/
bool MergeDirtyPathsUnderSameEndpoint();

/**
* During the iterating of the paths, releasing the object in the inner loop will cause undefined behavior of the ObjectPool, so
* we replace the items to be cleared by a tomb first, then clear all the tombs after the iteration.
*
* Returns whether we have released any paths.
*/
bool ClearTombPaths();

CHIP_ERROR InsertPathIntoDirtySet(const AttributePathParams & aAttributePath);

inline void BumpDirtySetGeneration() { mDirtyGeneration++; }

/**
Expand Down Expand Up @@ -243,12 +218,7 @@ class Engine
* mGlobalDirtySet is used to track the set of attribute/event paths marked dirty for reporting purposes.
*
*/
#if CONFIG_IM_BUILD_FOR_UNIT_TEST
// For unit tests, always use inline allocation for code coverage.
ObjectPool<AttributePathParamsWithGeneration, CHIP_IM_SERVER_MAX_NUM_DIRTY_SET, ObjectPoolMem::kInline> mGlobalDirtySet;
#else
ObjectPool<AttributePathParamsWithGeneration, CHIP_IM_SERVER_MAX_NUM_DIRTY_SET> mGlobalDirtySet;
#endif

/**
* A generation counter for the dirty attrbute set.
Expand Down
144 changes: 0 additions & 144 deletions src/app/tests/TestReportingEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
#include <messaging/ExchangeContext.h>
#include <messaging/Flags.h>

#include <cinttypes>
#include <nlunit-test.h>

using TestContext = chip::Test::AppContext;
Expand All @@ -54,54 +53,6 @@ class TestReportingEngine
public:
static void TestBuildAndSendSingleReportData(nlTestSuite * apSuite, void * apContext);
static void TestMergeOverlappedAttributePath(nlTestSuite * apSuite, void * apContext);
static void TestMergeAttributePathWhenDirtySetPoolExhausted(nlTestSuite * apSuite, void * apContext);

private:
static bool InsertToDirtySet(const AttributePathParams & aPath);

struct ExpectedDirtySetContent : public AttributePathParams
{
ExpectedDirtySetContent(const AttributePathParams & path) : AttributePathParams(path) {}
bool verified = false;
};

template <typename... Args>
static bool VerifyDirtySetContent(const Args &... args)
{
const int size = sizeof...(args);
ExpectedDirtySetContent content[size] = { ExpectedDirtySetContent(args)... };

if (InteractionModelEngine::GetInstance()->GetReportingEngine().mGlobalDirtySet.ForEachActiveObject([&](auto * path) {
for (int i = 0; i < size; i++)
{
if (static_cast<AttributePathParams>(content[i]) == static_cast<AttributePathParams>(*path))
{
content[i].verified = true;
return Loop::Continue;
}
}
ChipLogDetail(DataManagement,
"Dirty path Endpoint %" PRIx16 " Cluster %" PRIx32 ", Attribute %" PRIx32 " is not expected",
uint16_t(path->mEndpointId), path->mClusterId, path->mAttributeId);
return Loop::Break;
}) == Loop::Break)
{
return false;
}

for (int i = 0; i < size; i++)
{
if (!content[i].verified)
{
ChipLogDetail(DataManagement,
"Dirty path Endpoint %" PRIx16 " Cluster %" PRIx32 ", Attribute %" PRIx32
" is not found in the dirty set",
uint16_t(content[i].mEndpointId), content[i].mClusterId, content[i].mAttributeId);
return false;
}
}
return true;
}
};

class TestExchangeDelegate : public Messaging::ExchangeDelegate
Expand Down Expand Up @@ -225,100 +176,6 @@ void TestReportingEngine::TestMergeOverlappedAttributePath(nlTestSuite * apSuite
InteractionModelEngine::GetInstance()->GetReportingEngine().Shutdown();
}

bool TestReportingEngine::InsertToDirtySet(const AttributePathParams & aPath)
{
auto path = InteractionModelEngine::GetInstance()->GetReportingEngine().mGlobalDirtySet.CreateObject();
VerifyOrReturnError(path != nullptr, false);
*path = aPath;
path->mGeneration = InteractionModelEngine::GetInstance()->GetReportingEngine().GetDirtySetGeneration();
return true;
}

void TestReportingEngine::TestMergeAttributePathWhenDirtySetPoolExhausted(nlTestSuite * apSuite, void * apContext)
{
TestContext & ctx = *static_cast<TestContext *>(apContext);
CHIP_ERROR err = CHIP_NO_ERROR;
err = InteractionModelEngine::GetInstance()->Init(&ctx.GetExchangeManager());
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

InteractionModelEngine::GetInstance()->GetReportingEngine().mGlobalDirtySet.ReleaseAll();
InteractionModelEngine::GetInstance()->GetReportingEngine().BumpDirtySetGeneration();

// Case 1: All dirty paths including the new one are under the same cluster.
// -> Expected behavior: The dirty set is replaced by a wildcard attribute path under the same cluster.
for (AttributeId i = 1; i <= CHIP_IM_SERVER_MAX_NUM_DIRTY_SET; i++)
{
NL_TEST_ASSERT(apSuite, InsertToDirtySet(AttributePathParams(kTestEndpointId, kTestClusterId, i)));
}
NL_TEST_ASSERT(apSuite,
CHIP_NO_ERROR ==
InteractionModelEngine::GetInstance()->GetReportingEngine().InsertPathIntoDirtySet(
AttributePathParams(kTestEndpointId, kTestClusterId, CHIP_IM_SERVER_MAX_NUM_DIRTY_SET + 1)));
NL_TEST_ASSERT(apSuite, VerifyDirtySetContent(AttributePathParams(kTestEndpointId, kTestClusterId)));

InteractionModelEngine::GetInstance()->GetReportingEngine().mGlobalDirtySet.ReleaseAll();

// Case 2: All dirty paths including the new one are under the same endpoint.
// -> Expected behavior: The dirty set is replaced by a wildcard cluster path under the same endpoint.
for (ClusterId i = 1; i <= CHIP_IM_SERVER_MAX_NUM_DIRTY_SET; i++)
{
NL_TEST_ASSERT(apSuite, InsertToDirtySet(AttributePathParams(kTestEndpointId, i, 1)));
}
NL_TEST_ASSERT(apSuite,
CHIP_NO_ERROR ==
InteractionModelEngine::GetInstance()->GetReportingEngine().InsertPathIntoDirtySet(
AttributePathParams(kTestEndpointId, ClusterId(CHIP_IM_SERVER_MAX_NUM_DIRTY_SET + 1), 1)));
NL_TEST_ASSERT(apSuite, VerifyDirtySetContent(AttributePathParams(kTestEndpointId, kInvalidClusterId)));

InteractionModelEngine::GetInstance()->GetReportingEngine().mGlobalDirtySet.ReleaseAll();

// Case 3: All dirty paths including the new one are under the different endpoints.
// -> Expected behavior: The dirty set is replaced by a wildcard endpoint.
for (EndpointId i = 1; i <= CHIP_IM_SERVER_MAX_NUM_DIRTY_SET; i++)
{
NL_TEST_ASSERT(apSuite, InsertToDirtySet(AttributePathParams(EndpointId(i), i, i)));
}
NL_TEST_ASSERT(apSuite,
CHIP_NO_ERROR ==
InteractionModelEngine::GetInstance()->GetReportingEngine().InsertPathIntoDirtySet(
AttributePathParams(EndpointId(CHIP_IM_SERVER_MAX_NUM_DIRTY_SET + 1), 1, 1)));
NL_TEST_ASSERT(apSuite, VerifyDirtySetContent(AttributePathParams()));

InteractionModelEngine::GetInstance()->GetReportingEngine().mGlobalDirtySet.ReleaseAll();

// Case 4: All existing dirty paths are under the same cluster, the new path comes from another cluster.
// -> Expected behavior: The existing paths are merged into one single wildcard attribute path. New path is inserted as-is.
for (EndpointId i = 1; i <= CHIP_IM_SERVER_MAX_NUM_DIRTY_SET; i++)
{
NL_TEST_ASSERT(apSuite, InsertToDirtySet(AttributePathParams(kTestEndpointId, kTestClusterId, i)));
}
NL_TEST_ASSERT(apSuite,
CHIP_NO_ERROR ==
InteractionModelEngine::GetInstance()->GetReportingEngine().InsertPathIntoDirtySet(
AttributePathParams(kTestEndpointId + 1, kTestClusterId + 1, 1)));
NL_TEST_ASSERT(apSuite,
VerifyDirtySetContent(AttributePathParams(kTestEndpointId, kTestClusterId),
AttributePathParams(kTestEndpointId + 1, kTestClusterId + 1, 1)));

InteractionModelEngine::GetInstance()->GetReportingEngine().mGlobalDirtySet.ReleaseAll();

// Case 5: All existing dirty paths are under the same endpoint, the new path comes from another endpoint.
// -> Expected behavior: The existing paths are merged into one single wildcard cluster path. New path is inserted as-is.
for (EndpointId i = 1; i <= CHIP_IM_SERVER_MAX_NUM_DIRTY_SET; i++)
{
NL_TEST_ASSERT(apSuite, InsertToDirtySet(AttributePathParams(kTestEndpointId, i, 1)));
}
NL_TEST_ASSERT(apSuite,
CHIP_NO_ERROR ==
InteractionModelEngine::GetInstance()->GetReportingEngine().InsertPathIntoDirtySet(
AttributePathParams(kTestEndpointId + 1, kTestClusterId + 1, 1)));
NL_TEST_ASSERT(apSuite,
VerifyDirtySetContent(AttributePathParams(kTestEndpointId, kInvalidClusterId),
AttributePathParams(kTestEndpointId + 1, kTestClusterId + 1, 1)));

InteractionModelEngine::GetInstance()->GetReportingEngine().Shutdown();
}

} // namespace reporting
} // namespace app
} // namespace chip
Expand All @@ -329,7 +186,6 @@ const nlTest sTests[] =
{
NL_TEST_DEF("CheckBuildAndSendSingleReportData", chip::app::reporting::TestReportingEngine::TestBuildAndSendSingleReportData),
NL_TEST_DEF("TestMergeOverlappedAttributePath", chip::app::reporting::TestReportingEngine::TestMergeOverlappedAttributePath),
NL_TEST_DEF("TestMergeAttributePathWhenDirtySetPoolExhausted", chip::app::reporting::TestReportingEngine::TestMergeAttributePathWhenDirtySetPoolExhausted),
NL_TEST_SENTINEL()
};
// clang-format on
Expand Down
Loading

0 comments on commit c988931

Please sign in to comment.