From ef73ba2e323dfb951d65a314362f140d2f6eee10 Mon Sep 17 00:00:00 2001 From: yunhanw-google Date: Mon, 30 Aug 2021 14:12:56 -0700 Subject: [PATCH] Cleanup IM read (#9305) Summary of Changes: -- SendReadRequest takes too many parameters, we need to add one structure to pack those paramters. --- src/app/InteractionModelEngine.cpp | 9 +- src/app/InteractionModelEngine.h | 5 +- src/app/ReadClient.cpp | 30 +++---- src/app/ReadClient.h | 6 +- src/app/ReadPrepareParams.h | 83 +++++++++++++++++++ src/app/tests/TestReadInteraction.cpp | 26 +++--- .../tests/integration/chip_im_initiator.cpp | 11 ++- src/controller/CHIPDevice.cpp | 10 ++- 8 files changed, 125 insertions(+), 55 deletions(-) create mode 100644 src/app/ReadPrepareParams.h diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 7d2aa40fd08f66..5ae3333242edca 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -317,17 +317,12 @@ void InteractionModelEngine::OnResponseTimeout(Messaging::ExchangeContext * ec) ChipLogProgress(InteractionModel, "Time out! failed to receive echo response from Exchange: %d", ec->GetExchangeId()); } -CHIP_ERROR InteractionModelEngine::SendReadRequest(NodeId aNodeId, FabricIndex aFabricIndex, SessionHandle * apSecureSession, - EventPathParams * apEventPathParamsList, size_t aEventPathParamsListSize, - AttributePathParams * apAttributePathParamsList, - size_t aAttributePathParamsListSize, EventNumber aEventNumber, - uint64_t aAppIdentifier) +CHIP_ERROR InteractionModelEngine::SendReadRequest(ReadPrepareParams & aReadPrepareParams, uint64_t aAppIdentifier) { ReadClient * client = nullptr; CHIP_ERROR err = CHIP_NO_ERROR; ReturnErrorOnFailure(NewReadClient(&client, aAppIdentifier)); - err = client->SendReadRequest(aNodeId, aFabricIndex, apSecureSession, apEventPathParamsList, aEventPathParamsListSize, - apAttributePathParamsList, aAttributePathParamsListSize, aEventNumber); + err = client->SendReadRequest(aReadPrepareParams); if (err != CHIP_NO_ERROR) { client->Shutdown(); diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index a0288f1d771283..39af51e87ac824 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -110,10 +110,7 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate * @retval #CHIP_ERROR_NO_MEMORY If there is no ReadClient available * @retval #CHIP_NO_ERROR On success. */ - CHIP_ERROR SendReadRequest(NodeId aNodeId, FabricIndex aFabricIndex, SessionHandle * apSecureSession, - EventPathParams * apEventPathParamsList, size_t aEventPathParamsListSize, - AttributePathParams * apAttributePathParamsList, size_t aAttributePathParamsListSize, - EventNumber aEventNumber, uint64_t aAppIdentifier = 0); + CHIP_ERROR SendReadRequest(ReadPrepareParams & aReadPrepareParams, uint64_t aAppIdentifier = 0); /** * Retrieve a WriteClient that the SDK consumer can use to send a write. If the call succeeds, diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index a60e69a85db498..e33afb5cc24a5a 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -91,10 +91,7 @@ void ReadClient::MoveToState(const ClientState aTargetState) GetStateStr()); } -CHIP_ERROR ReadClient::SendReadRequest(NodeId aNodeId, FabricIndex aFabricIndex, SessionHandle * apSecureSession, - EventPathParams * apEventPathParamsList, size_t aEventPathParamsListSize, - AttributePathParams * apAttributePathParamsList, size_t aAttributePathParamsListSize, - EventNumber aEventNumber, uint32_t timeout) +CHIP_ERROR ReadClient::SendReadRequest(ReadPrepareParams & aReadPrepareParams) { // TODO: SendRequest parameter is too long, need to have the structure to represent it CHIP_ERROR err = CHIP_NO_ERROR; @@ -120,24 +117,26 @@ CHIP_ERROR ReadClient::SendReadRequest(NodeId aNodeId, FabricIndex aFabricIndex, err = request.Init(&writer); SuccessOrExit(err); - if (aEventPathParamsListSize != 0 && apEventPathParamsList != nullptr) + if (aReadPrepareParams.mEventPathParamsListSize != 0 && aReadPrepareParams.mpEventPathParamsList != nullptr) { EventPathList::Builder & eventPathListBuilder = request.CreateEventPathListBuilder(); SuccessOrExit(err = eventPathListBuilder.GetError()); - err = GenerateEventPathList(eventPathListBuilder, apEventPathParamsList, aEventPathParamsListSize); + err = GenerateEventPathList(eventPathListBuilder, aReadPrepareParams.mpEventPathParamsList, + aReadPrepareParams.mEventPathParamsListSize); SuccessOrExit(err); - if (aEventNumber != 0) + if (aReadPrepareParams.mEventNumber != 0) { // EventNumber is optional - request.EventNumber(aEventNumber); + request.EventNumber(aReadPrepareParams.mEventNumber); } } - if (aAttributePathParamsListSize != 0 && apAttributePathParamsList != nullptr) + if (aReadPrepareParams.mAttributePathParamsListSize != 0 && aReadPrepareParams.mpAttributePathParamsList != nullptr) { AttributePathList::Builder attributePathListBuilder = request.CreateAttributePathListBuilder(); SuccessOrExit(err = attributePathListBuilder.GetError()); - err = GenerateAttributePathList(attributePathListBuilder, apAttributePathParamsList, aAttributePathParamsListSize); + err = GenerateAttributePathList(attributePathListBuilder, aReadPrepareParams.mpAttributePathParamsList, + aReadPrepareParams.mAttributePathParamsListSize); SuccessOrExit(err); } @@ -148,16 +147,9 @@ CHIP_ERROR ReadClient::SendReadRequest(NodeId aNodeId, FabricIndex aFabricIndex, SuccessOrExit(err); } - if (apSecureSession != nullptr) - { - mpExchangeCtx = mpExchangeMgr->NewContext(*apSecureSession, this); - } - else - { - mpExchangeCtx = mpExchangeMgr->NewContext(SessionHandle(aNodeId, 0, 0, aFabricIndex), this); - } + mpExchangeCtx = mpExchangeMgr->NewContext(aReadPrepareParams.mSessionHandle, this); VerifyOrExit(mpExchangeCtx != nullptr, err = CHIP_ERROR_NO_MEMORY); - mpExchangeCtx->SetResponseTimeout(timeout); + mpExchangeCtx->SetResponseTimeout(aReadPrepareParams.mTimeout); err = mpExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::ReadRequest, std::move(msgBuf), Messaging::SendFlags(Messaging::SendMessageFlags::kExpectResponse)); diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h index 4279941bc3a073..4d5fb6bef7feb0 100644 --- a/src/app/ReadClient.h +++ b/src/app/ReadClient.h @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -75,10 +76,7 @@ class ReadClient : public Messaging::ExchangeDelegate * @retval #others fail to send read request * @retval #CHIP_NO_ERROR On success. */ - CHIP_ERROR SendReadRequest(NodeId aNodeId, FabricIndex aFabricIndex, SessionHandle * aSecureSession, - EventPathParams * apEventPathParamsList, size_t aEventPathParamsListSize, - AttributePathParams * apAttributePathParamsList, size_t aAttributePathParamsListSize, - EventNumber aEventNumber, uint32_t timeout = kImMessageTimeoutMsec); + CHIP_ERROR SendReadRequest(ReadPrepareParams & aReadPrepareParams); uint64_t GetAppIdentifier() const { return mAppIdentifier; } Messaging::ExchangeContext * GetExchangeContext() const { return mpExchangeCtx; } diff --git a/src/app/ReadPrepareParams.h b/src/app/ReadPrepareParams.h new file mode 100644 index 00000000000000..04f2e730c25f05 --- /dev/null +++ b/src/app/ReadPrepareParams.h @@ -0,0 +1,83 @@ +/* + * + * Copyright (c) 2021 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include +#include +#include +#include +#include +#include + +namespace chip { +namespace app { +struct ReadPrepareParams +{ + SessionHandle mSessionHandle; + EventPathParams * mpEventPathParamsList = nullptr; + size_t mEventPathParamsListSize = 0; + AttributePathParams * mpAttributePathParamsList = nullptr; + size_t mAttributePathParamsListSize = 0; + EventNumber mEventNumber = 0; + uint32_t mTimeout = kImMessageTimeoutMsec; + uint16_t mMinIntervalSeconds = 0; + uint16_t mMaxIntervalSeconds = 0; + + ReadPrepareParams() {} + ReadPrepareParams(ReadPrepareParams && other) + { + mSessionHandle = other.mSessionHandle; + mpEventPathParamsList = other.mpEventPathParamsList; + mEventPathParamsListSize = other.mEventPathParamsListSize; + mpAttributePathParamsList = other.mpAttributePathParamsList; + mAttributePathParamsListSize = other.mAttributePathParamsListSize; + mEventNumber = other.mEventNumber; + mMinIntervalSeconds = other.mMinIntervalSeconds; + mMaxIntervalSeconds = other.mMaxIntervalSeconds; + mTimeout = other.mTimeout; + other.mpEventPathParamsList = nullptr; + other.mEventPathParamsListSize = 0; + other.mpAttributePathParamsList = nullptr; + other.mAttributePathParamsListSize = 0; + } + + ReadPrepareParams & operator=(ReadPrepareParams && other) + { + if (&other == this) + return *this; + + mSessionHandle = other.mSessionHandle; + mpEventPathParamsList = other.mpEventPathParamsList; + mEventPathParamsListSize = other.mEventPathParamsListSize; + mpAttributePathParamsList = other.mpAttributePathParamsList; + mAttributePathParamsListSize = other.mAttributePathParamsListSize; + mEventNumber = other.mEventNumber; + mMinIntervalSeconds = other.mMinIntervalSeconds; + mMaxIntervalSeconds = other.mMaxIntervalSeconds; + mTimeout = other.mTimeout; + other.mpEventPathParamsList = nullptr; + other.mEventPathParamsListSize = 0; + other.mpAttributePathParamsList = nullptr; + other.mAttributePathParamsListSize = 0; + + return *this; + } +}; +} // namespace app +} // namespace chip diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index 7c0ba25e6ed332..844d1c33bf08e4 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -265,16 +265,13 @@ void TestReadInteraction::TestReadClient(nlTestSuite * apSuite, void * apContext CHIP_ERROR err = CHIP_NO_ERROR; TestContext & ctx = *static_cast(apContext); app::ReadClient readClient; - EventNumber eventNumber = 0; - chip::app::InteractionModelDelegate delegate; System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); err = readClient.Init(&ctx.GetExchangeManager(), &delegate, 0 /* application identifier */); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - SessionHandle session = ctx.GetSessionLocalToPeer(); - err = readClient.SendReadRequest(ctx.GetDestinationNodeId(), ctx.GetFabricIndex(), &session, nullptr /*apEventPathParamsList*/, - 0 /*aEventPathParamsListSize*/, nullptr /*apAttributePathParamsList*/, - 0 /*aAttributePathParamsListSize*/, eventNumber /*aEventNumber*/); + ReadPrepareParams readPrepareParams; + readPrepareParams.mSessionHandle = ctx.GetSessionLocalToPeer(); + err = readClient.SendReadRequest(readPrepareParams); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); GenerateReportData(apSuite, apContext, buf); @@ -395,16 +392,14 @@ void TestReadInteraction::TestReadClientInvalidReport(nlTestSuite * apSuite, voi TestContext & ctx = *static_cast(apContext); app::ReadClient readClient; chip::app::InteractionModelDelegate delegate; - EventNumber eventNumber = 0; System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); err = readClient.Init(&ctx.GetExchangeManager(), &delegate, 0 /* application identifier */); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - SessionHandle session = ctx.GetSessionLocalToPeer(); - err = readClient.SendReadRequest(ctx.GetDestinationNodeId(), ctx.GetFabricIndex(), &session, nullptr /*apEventPathParamsList*/, - 0 /*aEventPathParamsListSize*/, nullptr /*apAttributePathParamsList*/, - 0 /*aAttributePathParamsListSize*/, eventNumber /*aEventNumber*/); + ReadPrepareParams readPrepareParams; + readPrepareParams.mSessionHandle = ctx.GetSessionLocalToPeer(); + err = readClient.SendReadRequest(readPrepareParams); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); GenerateReportData(apSuite, apContext, buf, true /*aNeedInvalidReport*/); @@ -594,9 +589,12 @@ void TestReadInteraction::TestReadEventRoundtrip(nlTestSuite * apSuite, void * a eventPathParams[1].mClusterId = kTestClusterId; eventPathParams[1].mEventId = kTestEventIdCritical; - SessionHandle session = ctx.GetSessionLocalToPeer(); - err = chip::app::InteractionModelEngine::GetInstance()->SendReadRequest(ctx.GetDestinationNodeId(), ctx.GetFabricIndex(), - &session, eventPathParams, 2, nullptr, 1, 0); + ReadPrepareParams readPrepareParams; + readPrepareParams.mSessionHandle = ctx.GetSessionLocalToPeer(); + readPrepareParams.mpEventPathParamsList = eventPathParams; + readPrepareParams.mEventPathParamsListSize = 2; + + err = chip::app::InteractionModelEngine::GetInstance()->SendReadRequest(readPrepareParams); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); InteractionModelEngine::GetInstance()->GetReportingEngine().Run(); diff --git a/src/app/tests/integration/chip_im_initiator.cpp b/src/app/tests/integration/chip_im_initiator.cpp index 87550a1d50c441..5f389d9c76424f 100644 --- a/src/app/tests/integration/chip_im_initiator.cpp +++ b/src/app/tests/integration/chip_im_initiator.cpp @@ -180,8 +180,8 @@ CHIP_ERROR SendBadCommandRequest(chip::app::CommandSender * commandSender) CHIP_ERROR SendReadRequest() { - CHIP_ERROR err = CHIP_NO_ERROR; - chip::EventNumber number = 0; + CHIP_ERROR err = CHIP_NO_ERROR; + chip::app::ReadPrepareParams readPrepareParams; chip::app::EventPathParams eventPathParams[2]; eventPathParams[0].mNodeId = kTestNodeId; eventPathParams[0].mEndpointId = kTestEndpointId; @@ -198,8 +198,11 @@ CHIP_ERROR SendReadRequest() printf("\nSend read request message to Node: %" PRIu64 "\n", chip::kTestDeviceNodeId); - err = chip::app::InteractionModelEngine::GetInstance()->SendReadRequest( - chip::kTestDeviceNodeId, gFabricIndex, nullptr, eventPathParams, 2, &attributePathParams, 1, number, gMessageTimeoutMsec); + readPrepareParams.mSessionHandle = chip::SessionHandle(chip::kTestDeviceNodeId, 0, 0, gFabricIndex); + readPrepareParams.mTimeout = gMessageTimeoutMsec; + readPrepareParams.mpEventPathParamsList = eventPathParams; + readPrepareParams.mEventPathParamsListSize = 2; + err = chip::app::InteractionModelEngine::GetInstance()->SendReadRequest(readPrepareParams, gMessageTimeoutMsec); SuccessOrExit(err); exit: diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index b3610c2801b5e5..2bbff727b35b26 100644 --- a/src/controller/CHIPDevice.cpp +++ b/src/controller/CHIPDevice.cpp @@ -38,6 +38,7 @@ #endif // CHIP_SYSTEM_CONFIG_USE_LWIP #include +#include #include #include #include @@ -678,6 +679,7 @@ void Device::AddReportHandler(EndpointId endpoint, ClusterId cluster, AttributeI CHIP_ERROR Device::SendReadAttributeRequest(app::AttributePathParams aPath, Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, app::TLVDataFilter aTlvDataFilter) { + chip::app::ReadPrepareParams readPrepareParams; bool loadedSecureSession = false; uint8_t seqNum = GetNextSequenceNumber(); aPath.mNodeId = GetDeviceId(); @@ -690,9 +692,11 @@ CHIP_ERROR Device::SendReadAttributeRequest(app::AttributePathParams aPath, Call } // The application context is used to identify different requests from client applicaiton the type of it is intptr_t, here we // use the seqNum. - CHIP_ERROR err = chip::app::InteractionModelEngine::GetInstance()->SendReadRequest( - GetDeviceId(), 0, &mSecureSession, nullptr /*event path params list*/, 0, &aPath, 1, 0 /* event number */, - seqNum /* application context */); + readPrepareParams.mSessionHandle = mSecureSession; + readPrepareParams.mpAttributePathParamsList = &aPath; + readPrepareParams.mAttributePathParamsListSize = 1; + CHIP_ERROR err = + chip::app::InteractionModelEngine::GetInstance()->SendReadRequest(readPrepareParams, seqNum /* application context */); if (err != CHIP_NO_ERROR) { CancelResponseHandler(seqNum);