From 5bc71340b0118f828d1efabcc51b07e8aa76154a Mon Sep 17 00:00:00 2001 From: yunhanw-google Date: Mon, 6 Dec 2021 17:41:52 -0800 Subject: [PATCH] Fix ReadEvent (#12626) --- src/app/ReadClient.h | 17 ------- src/controller/ReadInteraction.h | 28 ++++------- src/controller/TypedReadCallback.h | 18 +++---- src/controller/tests/data_model/TestRead.cpp | 52 ++++++++++++++++---- 4 files changed, 60 insertions(+), 55 deletions(-) diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h index 2d99af527e78dd..c0cb6613a0d8fb 100644 --- a/src/app/ReadClient.h +++ b/src/app/ReadClient.h @@ -271,23 +271,6 @@ class ReadClient : public Messaging::ExchangeDelegate CHIP_ERROR AbortExistingExchangeContext(); const char * GetStateStr() const; - /** - * Validate that the Event ID and Cluster ID in the header match that of the type information present in the object and - * decode the data. The template parameter T is generally expected to be a ClusterName::Events::EventName::Type struct - * - * @param [in] aEventHeader The header of the event being validated. - * @param [in] aEvent The event data. - * @param [in] aReader The tlv reader. - */ - template - CHIP_ERROR DecodeEvent(const EventHeader & aEventHeader, const EventDataT & aEvent, TLV::TLVReader & aReader) - { - VerifyOrReturnError((aEventHeader.mPath.mEventId == aEvent.GetEventId()) && - (aEventHeader.mPath.mClusterId == aEvent.GetClusterId()), - CHIP_ERROR_INVALID_ARGUMENT); - return DataModel::Decode(aReader, aEvent); - } - /** * Internal shutdown method that we use when we know what's going on with * our exchange and don't need to manually close it. diff --git a/src/controller/ReadInteraction.h b/src/controller/ReadInteraction.h index efc977b778cf38..3405afc0ab1441 100644 --- a/src/controller/ReadInteraction.h +++ b/src/controller/ReadInteraction.h @@ -88,17 +88,17 @@ CHIP_ERROR ReadAttribute(Messaging::ExchangeManager * aExchangeMgr, const Sessio * for a given attribute as well as callbacks for success and failure and either returns a decoded cluster-object representation * of the requested attribute through the provided success callback or calls the provided failure callback. * - * The EventTypeInfo is generally expected to be a ClusterName::Events::EventName::TypeInfo struct, but any + * The DecodableEventType is generally expected to be a ClusterName::Events::EventName::DecodableEventType struct, but any * object that contains type information exposed through a 'DecodableType' type declaration as well as GetClusterId() and * GetEventId() methods is expected to work. */ -template +template CHIP_ERROR ReadEvent(Messaging::ExchangeManager * apExchangeMgr, const SessionHandle sessionHandle, EndpointId endpointId, - typename TypedReadEventCallback::OnSuccessCallbackType onSuccessCb, - typename TypedReadEventCallback::OnErrorCallbackType onErrorCb) + typename TypedReadEventCallback::OnSuccessCallbackType onSuccessCb, + typename TypedReadEventCallback::OnErrorCallbackType onErrorCb) { - ClusterId clusterId = DecodableEventTypeInfo::GetClusterId(); - EventId eventId = DecodableEventTypeInfo::GetEventId(); + ClusterId clusterId = DecodableEventType::GetClusterId(); + EventId eventId = DecodableEventType::GetEventId(); app::EventPathParams eventPath(endpointId, clusterId, eventId); app::ReadPrepareParams readParams(sessionHandle); app::ReadClient * readClient = nullptr; @@ -108,17 +108,15 @@ CHIP_ERROR ReadEvent(Messaging::ExchangeManager * apExchangeMgr, const SessionHa readParams.mpEventPathParamsList = &eventPath; readParams.mEventPathParamsListSize = 1; - auto onDone = [](app::ReadClient * apReadClient, TypedReadEventCallback * callback) { + auto onDone = [](app::ReadClient * apReadClient, TypedReadEventCallback * callback) { chip::Platform::Delete(callback); }; - auto callback = chip::Platform::MakeUnique>(clusterId, eventId, onSuccessCb, - onErrorCb, onDone); + auto callback = chip::Platform::MakeUnique>(onSuccessCb, onErrorCb, onDone); VerifyOrReturnError(callback != nullptr, CHIP_ERROR_NO_MEMORY); - ReturnErrorOnFailure( - engine->NewReadClient(&readClient, app::ReadClient::InteractionType::Read, &callback.get()->GetBufferedCallback())); + ReturnErrorOnFailure(engine->NewReadClient(&readClient, app::ReadClient::InteractionType::Read, callback.get())); err = readClient->SendReadRequest(readParams); if (err != CHIP_NO_ERROR) @@ -146,13 +144,5 @@ ReadAttribute(Messaging::ExchangeManager * aExchangeMgr, const SessionHandle ses AttributeTypeInfo::GetClusterId(), AttributeTypeInfo::GetAttributeId(), onSuccessCb, onErrorCb); } - -template -CHIP_ERROR ReadEvent(Messaging::ExchangeManager * aExchangeMgr, const SessionHandle sessionHandle, EndpointId endpointId, - typename TypedReadEventCallback::OnSuccessCallbackType onSuccessCb, - typename TypedReadEventCallback::OnErrorCallbackType onErrorCb) -{ - return ReadEvent(aExchangeMgr, sessionHandle, endpointId, onSuccessCb, onErrorCb); -} } // namespace Controller } // namespace chip diff --git a/src/controller/TypedReadCallback.h b/src/controller/TypedReadCallback.h index 75cf2aa6586091..38869ecbdb5058 100644 --- a/src/controller/TypedReadCallback.h +++ b/src/controller/TypedReadCallback.h @@ -107,19 +107,17 @@ class TypedReadAttributeCallback final : public app::ReadClient::Callback app::BufferedReadCallback mBufferedReadAdapter; }; -template +template class TypedReadEventCallback final : public app::ReadClient::Callback { public: - using OnSuccessCallbackType = std::function; + using OnSuccessCallbackType = std::function; using OnErrorCallbackType = std::function; using OnDoneCallbackType = std::function; - TypedReadEventCallback(ClusterId aClusterId, EventId aEventId, OnSuccessCallbackType aOnSuccess, OnErrorCallbackType aOnError, - OnDoneCallbackType aOnDone) : - mClusterId(aClusterId), - mEventId(aEventId), mOnSuccess(aOnSuccess), mOnError(aOnError), mOnDone(aOnDone) + TypedReadEventCallback(OnSuccessCallbackType aOnSuccess, OnErrorCallbackType aOnError, OnDoneCallbackType aOnDone) : + mOnSuccess(aOnSuccess), mOnError(aOnError), mOnDone(aOnDone) {} private: @@ -127,11 +125,13 @@ class TypedReadEventCallback final : public app::ReadClient::Callback const app::StatusIB * apStatus) override { CHIP_ERROR err = CHIP_NO_ERROR; - DecodableEventTypeInfo value; + DecodableEventType value; VerifyOrExit(apData != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT); - err = apReadClient->DecodeEvent(aEventHeader, value, *apData); + VerifyOrExit((aEventHeader.mPath.mEventId == value.GetEventId()) && (aEventHeader.mPath.mClusterId == value.GetClusterId()), + CHIP_ERROR_SCHEMA_MISMATCH); + err = app::DataModel::Decode(*apData, value); SuccessOrExit(err); mOnSuccess(aEventHeader, value); @@ -150,8 +150,6 @@ class TypedReadEventCallback final : public app::ReadClient::Callback void OnDone(app::ReadClient * apReadClient) override { mOnDone(apReadClient, this); } - ClusterId mClusterId; - EventId mEventId; OnSuccessCallbackType mOnSuccess; OnErrorCallbackType mOnError; OnDoneCallbackType mOnDone; diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp index 7eab729a7ff161..f5e6da67dc437c 100644 --- a/src/controller/tests/data_model/TestRead.cpp +++ b/src/controller/tests/data_model/TestRead.cpp @@ -122,14 +122,15 @@ class TestReadInteraction public: TestReadInteraction() {} - static void TestDataResponse(nlTestSuite * apSuite, void * apContext); - static void TestAttributeError(nlTestSuite * apSuite, void * apContext); - static void TestReadTimeout(nlTestSuite * apSuite, void * apContext); + static void TestReadAttributeResponse(nlTestSuite * apSuite, void * apContext); + static void TestReadAttributeError(nlTestSuite * apSuite, void * apContext); + static void TestReadAttributeTimeout(nlTestSuite * apSuite, void * apContext); + static void TestReadEventResponse(nlTestSuite * apSuite, void * apContext); private: }; -void TestReadInteraction::TestDataResponse(nlTestSuite * apSuite, void * apContext) +void TestReadInteraction::TestReadAttributeResponse(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); auto sessionHandle = ctx.GetSessionBobToAlice(); @@ -174,7 +175,39 @@ void TestReadInteraction::TestDataResponse(nlTestSuite * apSuite, void * apConte NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } -void TestReadInteraction::TestAttributeError(nlTestSuite * apSuite, void * apContext) +void TestReadInteraction::TestReadEventResponse(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + auto sessionHandle = ctx.GetSessionBobToAlice(); + bool onSuccessCbInvoked = false, onFailureCbInvoked = false; + + // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's + // not safe to do so. + auto onSuccessCb = [apSuite, &onSuccessCbInvoked](const app::EventHeader & eventHeader, const auto & EventResponse) { + // TODO: Need to add check when IM event server integration completes + IgnoreUnusedVariable(apSuite); + onSuccessCbInvoked = true; + }; + + // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's + // not safe to do so. + auto onFailureCb = [&onFailureCbInvoked](const app::EventHeader * eventHeader, Protocols::InteractionModel::Status aIMStatus, + CHIP_ERROR aError) { onFailureCbInvoked = true; }; + + chip::Controller::ReadEvent(&ctx.GetExchangeManager(), sessionHandle, + kTestEndpointId, onSuccessCb, onFailureCb); + + ctx.DrainAndServiceIO(); + chip::app::InteractionModelEngine::GetInstance()->GetReportingEngine().Run(); + ctx.DrainAndServiceIO(); + + NL_TEST_ASSERT(apSuite, !onFailureCbInvoked); + NL_TEST_ASSERT(apSuite, chip::app::InteractionModelEngine::GetInstance()->GetNumActiveReadClients() == 0); + NL_TEST_ASSERT(apSuite, chip::app::InteractionModelEngine::GetInstance()->GetNumActiveReadHandlers() == 0); + NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); +} + +void TestReadInteraction::TestReadAttributeError(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); auto sessionHandle = ctx.GetSessionBobToAlice(); @@ -209,7 +242,7 @@ void TestReadInteraction::TestAttributeError(nlTestSuite * apSuite, void * apCon NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } -void TestReadInteraction::TestReadTimeout(nlTestSuite * apSuite, void * apContext) +void TestReadInteraction::TestReadAttributeTimeout(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); auto sessionHandle = ctx.GetSessionBobToAlice(); @@ -268,9 +301,10 @@ void TestReadInteraction::TestReadTimeout(nlTestSuite * apSuite, void * apContex // clang-format off const nlTest sTests[] = { - NL_TEST_DEF("TestDataResponse", TestReadInteraction::TestDataResponse), - NL_TEST_DEF("TestAttributeError", TestReadInteraction::TestAttributeError), - NL_TEST_DEF("TestReadTimeout", TestReadInteraction::TestReadTimeout), + NL_TEST_DEF("TestReadAttributeResponse", TestReadInteraction::TestReadAttributeResponse), + NL_TEST_DEF("TestReadEventResponse", TestReadInteraction::TestReadEventResponse), + NL_TEST_DEF("TestReadAttributeError", TestReadInteraction::TestReadAttributeError), + NL_TEST_DEF("TestReadAttributeTimeout", TestReadInteraction::TestReadAttributeTimeout), NL_TEST_SENTINEL() }; // clang-format on