Skip to content

Commit

Permalink
Revert "Fix status report handling in write handler (#21440)"
Browse files Browse the repository at this point in the history 
This reverts commit 75ffda4.
  • Loading branch information
@woody-apple
woody-apple authored Aug 6, 2022
1 parent 75ffda4 commit 9cc7245
Show file tree
Hide file tree
Showing 5 changed files with 9 additions and 116 deletions.
18 changes: 0 additions & 18 deletions src/app/InteractionModelEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 0 additions & 5 deletions src/app/InteractionModelEngine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
*/
Expand Down
15 changes: 5 additions & 10 deletions src/app/WriteHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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;
}
Expand All @@ -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();
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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));
}

Expand Down
8 changes: 3 additions & 5 deletions src/app/WriteHandler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,18 +114,16 @@ class WriteHandler : public Messaging::ExchangeDelegate
}

private:
friend class TestWriteInteraction;
enum class State
{
Uninitialized = 0, // The handler has not been initialized
Initialized, // The handler has been initialized and is ready
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);
Expand Down
79 changes: 1 addition & 78 deletions src/app/tests/TestWriteInteraction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,7 @@
#include <messaging/ExchangeContext.h>
#include <messaging/Flags.h>

#include <memory>
#include <nlunit-test.h>
#include <utility>

using TestContext = chip::Test::AppContext;

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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<TestContext *>(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<uint16_t>::Missing(),
static_cast<uint16_t>(900) /* reserved buffer size */);

ByteSpan list[5];

err = writeClient.EncodeAttribute(attributePath, app::DataModel::List<ByteSpan>(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

Expand All @@ -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
Expand Down

0 comments on commit 9cc7245

Please sign in to comment.