From 9cc7245a07d286266e1a6d57101b571e25f5aee1 Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Sat, 6 Aug 2022 09:57:22 -0700 Subject: [PATCH] Revert "Fix status report handling in write handler (#21440)" This reverts commit 75ffda4c587e595e913a1b17c74a3e9e9a580898. --- src/app/InteractionModelEngine.cpp | 18 ------ src/app/InteractionModelEngine.h | 5 -- src/app/WriteHandler.cpp | 15 ++--- src/app/WriteHandler.h | 8 +-- src/app/tests/TestWriteInteraction.cpp | 79 +------------------------- 5 files changed, 9 insertions(+), 116 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 94233c874889da..b21d5b5a976604 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -202,24 +202,6 @@ ReadHandler * InteractionModelEngine::ActiveHandlerAt(unsigned int aIndex) return ret; } -WriteHandler * InteractionModelEngine::ActiveWriteHandlerAt(unsigned int aIndex) -{ - unsigned int i = 0; - - for (auto & writeHandler : mWriteHandlers) - { - if (!writeHandler.IsFree()) - { - if (i == aIndex) - { - return &writeHandler; - } - i++; - } - } - return nullptr; -} - uint32_t InteractionModelEngine::GetNumActiveWriteHandlers() const { uint32_t numActive = 0; diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index 9f21baf4b4726d..28637499085f9a 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -159,11 +159,6 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, */ ReadHandler * ActiveHandlerAt(unsigned int aIndex); - /** - * Returns the write handler at a particular index within the active handler list. - */ - WriteHandler * ActiveWriteHandlerAt(unsigned int aIndex); - /** * The Magic number of this InteractionModelEngine, the magic number is set during Init() */ diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp index 36d92ca0851b55..12a2086e7c48d3 100644 --- a/src/app/WriteHandler.cpp +++ b/src/app/WriteHandler.cpp @@ -31,7 +31,7 @@ namespace chip { namespace app { using namespace Protocols::InteractionModel; -using Status = Protocols::InteractionModel::Status; + constexpr uint8_t kListAttributeType = 0x48; CHIP_ERROR WriteHandler::Init() @@ -118,14 +118,7 @@ CHIP_ERROR WriteHandler::OnMessageReceived(Messaging::ExchangeContext * apExchan "OnMessageReceived should not be called on GroupExchangeContext"); if (!aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::WriteRequest)) { - if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::StatusResponse)) - { - CHIP_ERROR statusError = CHIP_NO_ERROR; - //Logging purpose to print the status error code - StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError); - } ChipLogDetail(DataManagement, "Unexpected message type %d", aPayloadHeader.GetMessageType()); - StatusResponse::Send(Status::InvalidAction, apExchangeContext, false /*aExpectResponse*/); Close(); return CHIP_ERROR_INVALID_MESSAGE_TYPE; } @@ -140,7 +133,7 @@ CHIP_ERROR WriteHandler::OnMessageReceived(Messaging::ExchangeContext * apExchan Close(); } } - else + else if (status != Protocols::InteractionModel::Status::Success) { err = StatusResponse::Send(status, apExchangeContext, false /*aExpectResponse*/); Close(); @@ -319,7 +312,7 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData // it with Busy status code. (dataAttributePath.IsListItemOperation() && !IsSameAttribute(mProcessingAttributePath, dataAttributePath))) { - err = AddStatus(dataAttributePath, StatusIB(Status::Busy)); + err = AddStatus(dataAttributePath, StatusIB(Protocols::InteractionModel::Status::Busy)); continue; } @@ -626,11 +619,13 @@ CHIP_ERROR WriteHandler::AddStatus(const ConcreteDataAttributePath & aPath, cons CHIP_ERROR WriteHandler::AddClusterSpecificSuccess(const ConcreteDataAttributePath & aPath, ClusterStatus aClusterStatus) { + using Protocols::InteractionModel::Status; return AddStatus(aPath, StatusIB(Status::Success, aClusterStatus)); } CHIP_ERROR WriteHandler::AddClusterSpecificFailure(const ConcreteDataAttributePath & aPath, ClusterStatus aClusterStatus) { + using Protocols::InteractionModel::Status; return AddStatus(aPath, StatusIB(Status::Failure, aClusterStatus)); } diff --git a/src/app/WriteHandler.h b/src/app/WriteHandler.h index 7a27266d4722d3..43163a2b8abab9 100644 --- a/src/app/WriteHandler.h +++ b/src/app/WriteHandler.h @@ -114,7 +114,6 @@ class WriteHandler : public Messaging::ExchangeDelegate } private: - friend class TestWriteInteraction; enum class State { Uninitialized = 0, // The handler has not been initialized @@ -122,10 +121,9 @@ class WriteHandler : public Messaging::ExchangeDelegate AddStatus, // The handler has added status code Sending, // The handler has sent out the write response }; - using Status = Protocols::InteractionModel::Status; - Status ProcessWriteRequest(System::PacketBufferHandle && aPayload, bool aIsTimedWrite); - Status HandleWriteRequestMessage(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload, - bool aIsTimedWrite); + Protocols::InteractionModel::Status ProcessWriteRequest(System::PacketBufferHandle && aPayload, bool aIsTimedWrite); + Protocols::InteractionModel::Status HandleWriteRequestMessage(Messaging::ExchangeContext * apExchangeContext, + System::PacketBufferHandle && aPayload, bool aIsTimedWrite); CHIP_ERROR FinalizeMessage(System::PacketBufferTLVWriter && aMessageWriter, System::PacketBufferHandle & packet); CHIP_ERROR SendWriteResponse(System::PacketBufferTLVWriter && aMessageWriter); diff --git a/src/app/tests/TestWriteInteraction.cpp b/src/app/tests/TestWriteInteraction.cpp index 711577059ab8cb..f1af80cef330b7 100644 --- a/src/app/tests/TestWriteInteraction.cpp +++ b/src/app/tests/TestWriteInteraction.cpp @@ -32,9 +32,7 @@ #include #include -#include #include -#include using TestContext = chip::Test::AppContext; @@ -63,7 +61,6 @@ class TestWriteInteraction static void TestWriteRoundtripWithClusterObjects(nlTestSuite * apSuite, void * apContext); static void TestWriteRoundtripWithClusterObjectsVersionMatch(nlTestSuite * apSuite, void * apContext); static void TestWriteRoundtripWithClusterObjectsVersionMismatch(nlTestSuite * apSuite, void * apContext); - static void TestWriteHandlerReceiveInvalidMessage(nlTestSuite * apSuite, void * apContext); private: static void AddAttributeDataIB(nlTestSuite * apSuite, void * apContext, WriteClient & aWriteClient); @@ -93,18 +90,13 @@ class TestWriteClientCallback : public chip::app::WriteClient::Callback mStatus = status; mOnSuccessCalled++; } - void OnError(const WriteClient * apWriteClient, CHIP_ERROR chipError) override - { - mOnErrorCalled++; - mLastErrorReason = app::StatusIB(chipError); - } + void OnError(const WriteClient * apWriteClient, CHIP_ERROR chipError) override { mOnErrorCalled++; } void OnDone(WriteClient * apWriteClient) override { mOnDoneCalled++; } int mOnSuccessCalled = 0; int mOnErrorCalled = 0; int mOnDoneCalled = 0; StatusIB mStatus; - StatusIB mLastErrorReason; }; void TestWriteInteraction::AddAttributeDataIB(nlTestSuite * apSuite, void * apContext, WriteClient & aWriteClient) @@ -551,74 +543,6 @@ void TestWriteInteraction::TestWriteRoundtrip(nlTestSuite * apSuite, void * apCo engine->Shutdown(); } -// This test creates a chunked write request, we drop the second write chunk message, then write handler receives unknown -// report message and sends out a status report with invalid action. -void TestWriteInteraction::TestWriteHandlerReceiveInvalidMessage(nlTestSuite * apSuite, void * apContext) -{ - TestContext & ctx = *static_cast(apContext); - auto sessionHandle = ctx.GetSessionBobToAlice(); - - app::AttributePathParams attributePath(2, 3, 4); - - CHIP_ERROR err = CHIP_NO_ERROR; - Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); - // Shouldn't have anything in the retransmit table when starting the test. - NL_TEST_ASSERT(apSuite, rm->TestGetCountRetransTable() == 0); - - TestWriteClientCallback writeCallback; - auto * engine = chip::app::InteractionModelEngine::GetInstance(); - err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable()); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - - app::WriteClient writeClient(&ctx.GetExchangeManager(), &writeCallback, Optional::Missing(), - static_cast(900) /* reserved buffer size */); - - ByteSpan list[5]; - - err = writeClient.EncodeAttribute(attributePath, app::DataModel::List(list, 5)); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - - ctx.GetLoopback().mSentMessageCount = 0; - ctx.GetLoopback().mNumMessagesToDrop = 1; - ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 2; - err = writeClient.SendWriteRequest(sessionHandle); - NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - ctx.DrainAndServiceIO(); - - NL_TEST_ASSERT(apSuite, InteractionModelEngine::GetInstance()->GetNumActiveWriteHandlers() == 1); - NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 3); - NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1); - - System::PacketBufferHandle msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes); - NL_TEST_ASSERT(apSuite, !msgBuf.IsNull()); - System::PacketBufferTLVWriter writer; - writer.Init(std::move(msgBuf)); - - ReportDataMessage::Builder response; - response.Init(&writer); - NL_TEST_ASSERT(apSuite, writer.Finalize(&msgBuf) == CHIP_NO_ERROR); - - PayloadHeader payloadHeader; - payloadHeader.SetExchangeID(0); - payloadHeader.SetMessageType(chip::Protocols::InteractionModel::MsgType::ReportData); - - auto * writeHandler = InteractionModelEngine::GetInstance()->ActiveWriteHandlerAt(0); - rm->ClearRetransTable(writeClient.mExchangeCtx.Get()); - rm->ClearRetransTable(writeHandler->mExchangeCtx.Get()); - ctx.GetLoopback().mSentMessageCount = 0; - ctx.GetLoopback().mNumMessagesToDrop = 0; - writeHandler->OnMessageReceived(writeHandler->mExchangeCtx.Get(), payloadHeader, std::move(msgBuf)); - ctx.DrainAndServiceIO(); - - NL_TEST_ASSERT(apSuite, writeCallback.mLastErrorReason.mStatus == Protocols::InteractionModel::Status::InvalidAction); - NL_TEST_ASSERT(apSuite, InteractionModelEngine::GetInstance()->GetNumActiveWriteHandlers() == 0); - engine->Shutdown(); - ctx.ExpireSessionAliceToBob(); - ctx.ExpireSessionBobToAlice(); - ctx.CreateSessionAliceToBob(); - ctx.CreateSessionBobToAlice(); -} - } // namespace app } // namespace chip @@ -638,7 +562,6 @@ const nlTest sTests[] = NL_TEST_DEF("TestWriteRoundtripWithClusterObjects", chip::app::TestWriteInteraction::TestWriteRoundtripWithClusterObjects), NL_TEST_DEF("TestWriteRoundtripWithClusterObjectsVersionMatch", chip::app::TestWriteInteraction::TestWriteRoundtripWithClusterObjectsVersionMatch), NL_TEST_DEF("TestWriteRoundtripWithClusterObjectsVersionMismatch", chip::app::TestWriteInteraction::TestWriteRoundtripWithClusterObjectsVersionMismatch), - NL_TEST_DEF("TestWriteHandlerReceiveInvalidMessage", chip::app::TestWriteInteraction::TestWriteHandlerReceiveInvalidMessage), NL_TEST_SENTINEL() }; // clang-format on