Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert PR 16743 #17427

Merged
merged 2 commits into from
Apr 18, 2022
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Revert "- Removed timeout from TransferSession::StartTransfer() and T…
…ransferSession::WaitForTransfer()"

This reverts commit 7a4061a.
isiu-apple committed Apr 15, 2022
commit 3a9868ea86b40eb0d8f37a56c399cb98dc096918
3 changes: 2 additions & 1 deletion src/app/clusters/ota-requestor/BDXDownloader.cpp
Original file line number Diff line number Diff line change
@@ -104,7 +104,8 @@ CHIP_ERROR BDXDownloader::SetBDXParams(const chip::bdx::TransferSession::Transfe

// Must call StartTransfer() here to store the the pointer data contained in bdxInitData in the TransferSession object.
// Otherwise it could be freed before we can use it.
ReturnErrorOnFailure(mBdxTransfer.StartTransfer(chip::bdx::TransferRole::kReceiver, bdxInitData));
ReturnErrorOnFailure(mBdxTransfer.StartTransfer(chip::bdx::TransferRole::kReceiver, bdxInitData,
/* TODO:(#12520) */ chip::System::Clock::Seconds16(30)));

return CHIP_NO_ERROR;
}
4 changes: 2 additions & 2 deletions src/protocols/bdx/BdxTransferSession.cpp
Original file line number Diff line number Diff line change
@@ -134,7 +134,7 @@ void TransferSession::PollOutput(OutputEvent & event, System::Clock::Timestamp c
mPendingOutput = OutputEventType::kNone;
}

CHIP_ERROR TransferSession::StartTransfer(TransferRole role, const TransferInitData & initData)
CHIP_ERROR TransferSession::StartTransfer(TransferRole role, const TransferInitData & initData, System::Clock::Timeout timeout)
{
VerifyOrReturnError(mState == TransferState::kUnitialized, CHIP_ERROR_INCORRECT_STATE);

@@ -177,7 +177,7 @@ CHIP_ERROR TransferSession::StartTransfer(TransferRole role, const TransferInitD
}

CHIP_ERROR TransferSession::WaitForTransfer(TransferRole role, BitFlags<TransferControlFlags> xferControlOpts,
uint16_t maxBlockSize)
uint16_t maxBlockSize, System::Clock::Timeout timeout)
{
VerifyOrReturnError(mState == TransferState::kUnitialized, CHIP_ERROR_INCORRECT_STATE);

8 changes: 6 additions & 2 deletions src/protocols/bdx/BdxTransferSession.h
Original file line number Diff line number Diff line change
@@ -173,11 +173,13 @@ class DLL_EXPORT TransferSession
* @param role Inidcates whether this object will be sending or receiving data
* @param initData Data for initializing this object and for populating a TransferInit message
* The role parameter will determine whether to populate a ReceiveInit or SendInit
* @param timeout The amount of time to wait for a response before considering the transfer failed
* @param curTime The current time since epoch. Needed to set a start time for the transfer timeout.
*
* @return CHIP_ERROR Result of initialization and preparation of a TransferInit message. May also indicate if the
* TransferSession object is unable to handle this request.
*/
CHIP_ERROR StartTransfer(TransferRole role, const TransferInitData & initData);
CHIP_ERROR StartTransfer(TransferRole role, const TransferInitData & initData, System::Clock::Timeout timeout);

/**
* @brief
@@ -188,11 +190,13 @@ class DLL_EXPORT TransferSession
* @param role Inidcates whether this object will be sending or receiving data
* @param xferControlOpts Indicates all supported control modes. Used to respond to a TransferInit message
* @param maxBlockSize The max Block size that this object supports.
* @param timeout The amount of time to wait for a response before considering the transfer failed
*
* @return CHIP_ERROR Result of initialization. May also indicate if the TransferSession object is unable to handle this
* request.
*/
CHIP_ERROR WaitForTransfer(TransferRole role, BitFlags<TransferControlFlags> xferControlOpts, uint16_t maxBlockSize);
CHIP_ERROR WaitForTransfer(TransferRole role, BitFlags<TransferControlFlags> xferControlOpts, uint16_t maxBlockSize,
System::Clock::Timeout timeout);

/**
* @brief
4 changes: 2 additions & 2 deletions src/protocols/bdx/TransferFacilitator.cpp
Original file line number Diff line number Diff line change
@@ -95,7 +95,7 @@ CHIP_ERROR Responder::PrepareForTransfer(System::Layer * layer, TransferRole rol
mPollFreq = pollFreq;
mSystemLayer = layer;

ReturnErrorOnFailure(mTransfer.WaitForTransfer(role, xferControlOpts, maxBlockSize));
ReturnErrorOnFailure(mTransfer.WaitForTransfer(role, xferControlOpts, maxBlockSize, timeout));

mSystemLayer->StartTimer(mPollFreq, PollTimerHandler, this);
return CHIP_NO_ERROR;
@@ -109,7 +109,7 @@ CHIP_ERROR Initiator::InitiateTransfer(System::Layer * layer, TransferRole role,
mPollFreq = pollFreq;
mSystemLayer = layer;

ReturnErrorOnFailure(mTransfer.StartTransfer(role, initData));
ReturnErrorOnFailure(mTransfer.StartTransfer(role, initData, timeout));

mSystemLayer->StartTimer(mPollFreq, PollTimerHandler, this);
return CHIP_NO_ERROR;
83 changes: 63 additions & 20 deletions src/protocols/bdx/tests/TestBdxTransferSession.cpp
Original file line number Diff line number Diff line change
@@ -20,6 +20,8 @@ using namespace ::chip::bdx;
using namespace ::chip::Protocols;

namespace {
// Use this as a timestamp if not needing to test BDX timeouts.
constexpr System::Clock::Timestamp kNoAdvanceTime = System::Clock::kZero;

const TLV::Tag tlvStrTag = TLV::ContextTag(4);
const TLV::Tag tlvListTag = TLV::ProfileTag(7777, 8888);
@@ -84,7 +86,7 @@ CHIP_ERROR AttachHeaderAndSend(TransferSession::MessageTypeData typeData, chip::
chip::PayloadHeader payloadHeader;
payloadHeader.SetMessageType(typeData.ProtocolId, typeData.MessageType);

ReturnErrorOnFailure(receiver.HandleMessageReceived(payloadHeader, std::move(msgBuf)));
ReturnErrorOnFailure(receiver.HandleMessageReceived(payloadHeader, std::move(msgBuf), kNoAdvanceTime));
return CHIP_NO_ERROR;
}

@@ -129,7 +131,7 @@ void VerifyStatusReport(nlTestSuite * inSuite, void * inContext, const System::P
void VerifyNoMoreOutput(nlTestSuite * inSuite, void * inContext, TransferSession & transferSession)
{
TransferSession::OutputEvent event;
transferSession.PollOutput(event);
transferSession.PollOutput(event, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, event.EventType == TransferSession::OutputEventType::kNone);
}

@@ -145,22 +147,22 @@ void SendAndVerifyTransferInit(nlTestSuite * inSuite, void * inContext, Transfer
MessageType expectedInitMsg = (initiatorRole == TransferRole::kSender) ? MessageType::SendInit : MessageType::ReceiveInit;

// Initializer responder to wait for transfer
err = responder.WaitForTransfer(responderRole, responderControlOpts, responderMaxBlock);
err = responder.WaitForTransfer(responderRole, responderControlOpts, responderMaxBlock, timeout);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
VerifyNoMoreOutput(inSuite, inContext, responder);

// Verify initiator outputs respective Init message (depending on role) after StartTransfer()
err = initiator.StartTransfer(initiatorRole, initData);
err = initiator.StartTransfer(initiatorRole, initData, timeout);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
initiator.PollOutput(outEvent);
initiator.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kMsgToSend);
VerifyBdxMessageToSend(inSuite, inContext, outEvent, expectedInitMsg);
VerifyNoMoreOutput(inSuite, inContext, initiator);

// Verify that all parsed TransferInit fields match what was sent by the initiator
err = AttachHeaderAndSend(outEvent.msgTypeData, std::move(outEvent.MsgData), responder);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
responder.PollOutput(outEvent);
responder.PollOutput(outEvent, kNoAdvanceTime);
VerifyNoMoreOutput(inSuite, inContext, responder);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kInitReceived);
NL_TEST_ASSERT(inSuite, outEvent.transferInitData.TransferCtlFlags == initData.TransferCtlFlags);
@@ -213,7 +215,7 @@ void SendAndVerifyAcceptMsg(nlTestSuite * inSuite, void * inContext, TransferSes
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

// Verify Sender emits ReceiveAccept message for sending
acceptSender.PollOutput(outEvent);
acceptSender.PollOutput(outEvent, kNoAdvanceTime);
VerifyNoMoreOutput(inSuite, inContext, acceptSender);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kMsgToSend);
VerifyBdxMessageToSend(inSuite, inContext, outEvent, expectedMsg);
@@ -225,7 +227,7 @@ void SendAndVerifyAcceptMsg(nlTestSuite * inSuite, void * inContext, TransferSes
// Verify received ReceiveAccept.
// Client may want to inspect TransferControl, MaxBlockSize, StartOffset, Length, and Metadata, and may choose to reject the
// Transfer at this point.
acceptReceiver.PollOutput(outEvent);
acceptReceiver.PollOutput(outEvent, kNoAdvanceTime);
VerifyNoMoreOutput(inSuite, inContext, acceptReceiver);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kAcceptReceived);
NL_TEST_ASSERT(inSuite, outEvent.transferAcceptData.ControlMode == acceptData.ControlMode);
@@ -260,15 +262,15 @@ void SendAndVerifyQuery(nlTestSuite * inSuite, void * inContext, TransferSession
// Verify that querySender emits BlockQuery message
CHIP_ERROR err = querySender.PrepareBlockQuery();
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
querySender.PollOutput(outEvent);
querySender.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kMsgToSend);
VerifyBdxMessageToSend(inSuite, inContext, outEvent, MessageType::BlockQuery);
VerifyNoMoreOutput(inSuite, inContext, querySender);

// Pass BlockQuery to queryReceiver and verify queryReceiver emits QueryReceived event
err = AttachHeaderAndSend(outEvent.msgTypeData, std::move(outEvent.MsgData), queryReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
queryReceiver.PollOutput(outEvent);
queryReceiver.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kQueryReceived);
VerifyNoMoreOutput(inSuite, inContext, queryReceiver);
}
@@ -303,15 +305,15 @@ void SendAndVerifyArbitraryBlock(nlTestSuite * inSuite, void * inContext, Transf
// Provide Block data and verify sender emits Block message
err = sender.PrepareBlock(blockData);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
sender.PollOutput(outEvent);
sender.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kMsgToSend);
VerifyBdxMessageToSend(inSuite, inContext, outEvent, expected);
VerifyNoMoreOutput(inSuite, inContext, sender);

// Pass Block message to receiver and verify matching Block is received
err = AttachHeaderAndSend(outEvent.msgTypeData, std::move(outEvent.MsgData), receiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
receiver.PollOutput(outEvent);
receiver.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kBlockReceived);
NL_TEST_ASSERT(inSuite, outEvent.blockdata.Data != nullptr);
if (outEvent.EventType == TransferSession::OutputEventType::kBlockReceived && outEvent.blockdata.Data != nullptr)
@@ -333,15 +335,15 @@ void SendAndVerifyBlockAck(nlTestSuite * inSuite, void * inContext, TransferSess
// Verify PrepareBlockAck() outputs message to send
CHIP_ERROR err = ackSender.PrepareBlockAck();
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
ackSender.PollOutput(outEvent);
ackSender.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kMsgToSend);
VerifyBdxMessageToSend(inSuite, inContext, outEvent, expectedMsgType);
VerifyNoMoreOutput(inSuite, inContext, ackSender);

// Pass BlockAck to ackReceiver and verify it was received
err = AttachHeaderAndSend(outEvent.msgTypeData, std::move(outEvent.MsgData), ackReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
ackReceiver.PollOutput(outEvent);
ackReceiver.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == expectedEventType);
VerifyNoMoreOutput(inSuite, inContext, ackReceiver);
}
@@ -574,6 +576,46 @@ void TestBadAcceptMessageFields(nlTestSuite * inSuite, void * inContext)
NL_TEST_ASSERT(inSuite, err != CHIP_NO_ERROR);
}

// Test that a TransferSession will emit kTransferTimeout if the specified timeout is exceeded while waiting for a response.
void TestTimeout(nlTestSuite * inSuite, void * inContext)
{
CHIP_ERROR err = CHIP_NO_ERROR;
TransferSession initiator;
TransferSession::OutputEvent outEvent;

System::Clock::Timeout timeout = System::Clock::Milliseconds32(24);
System::Clock::Timestamp startTime = System::Clock::Milliseconds64(100);
System::Clock::Timestamp endTime = System::Clock::Milliseconds64(124);

// Initialize struct with arbitrary TransferInit parameters
TransferSession::TransferInitData initOptions;
initOptions.TransferCtlFlags = TransferControlFlags::kReceiverDrive;
initOptions.MaxBlockSize = 64;
initOptions.StartOffset = 0;
initOptions.Length = 0;
char testFileDes[9] = { "test.txt" }; // arbitrary file designator
initOptions.FileDesLength = static_cast<uint16_t>(strlen(testFileDes));
initOptions.FileDesignator = reinterpret_cast<uint8_t *>(testFileDes);
initOptions.Metadata = nullptr;
initOptions.MetadataLength = 0;

TransferRole role = TransferRole::kReceiver;

// Verify initiator outputs respective Init message (depending on role) after StartTransfer()
err = initiator.StartTransfer(role, initOptions, timeout);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

// First PollOutput() should output the TransferInit message
initiator.PollOutput(outEvent, startTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kMsgToSend);
MessageType expectedInitMsg = (role == TransferRole::kSender) ? MessageType::SendInit : MessageType::ReceiveInit;
VerifyBdxMessageToSend(inSuite, inContext, outEvent, expectedInitMsg);

// Second PollOutput() with no call to HandleMessageReceived() should result in a timeout.
initiator.PollOutput(outEvent, endTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kTransferTimeout);
}

// Test that sending the same block twice (with same block counter) results in a StatusReport message with BadBlockCounter. Also
// test that receiving the StatusReport ends the transfer on the other node.
void TestDuplicateBlockError(nlTestSuite * inSuite, void * inContext)
@@ -633,7 +675,7 @@ void TestDuplicateBlockError(nlTestSuite * inSuite, void * inContext)
// Provide Block data and verify sender emits Block message
err = respondingSender.PrepareBlock(blockData);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
respondingSender.PollOutput(eventWithBlock);
respondingSender.PollOutput(eventWithBlock, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, eventWithBlock.EventType == TransferSession::OutputEventType::kMsgToSend);
VerifyBdxMessageToSend(inSuite, inContext, eventWithBlock, MessageType::Block);
VerifyNoMoreOutput(inSuite, inContext, respondingSender);
@@ -643,7 +685,7 @@ void TestDuplicateBlockError(nlTestSuite * inSuite, void * inContext)
// Pass Block message to receiver and verify matching Block is received
err = AttachHeaderAndSend(eventWithBlock.msgTypeData, std::move(eventWithBlock.MsgData), initiatingReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
initiatingReceiver.PollOutput(outEvent);
initiatingReceiver.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kBlockReceived);
NL_TEST_ASSERT(inSuite, outEvent.blockdata.Data != nullptr);
VerifyNoMoreOutput(inSuite, inContext, initiatingReceiver);
@@ -653,7 +695,7 @@ void TestDuplicateBlockError(nlTestSuite * inSuite, void * inContext)
// Verify receiving same Block twice fails and results in StatusReport event, and then InternalError event
err = AttachHeaderAndSend(eventWithBlock.msgTypeData, std::move(blockCopy), initiatingReceiver);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
initiatingReceiver.PollOutput(outEvent);
initiatingReceiver.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kMsgToSend);
System::PacketBufferHandle statusReportMsg = outEvent.MsgData.Retain();
TransferSession::MessageTypeData statusReportMsgTypeData = outEvent.msgTypeData;
@@ -662,21 +704,21 @@ void TestDuplicateBlockError(nlTestSuite * inSuite, void * inContext)
// All subsequent PollOutput() calls should return kInternalError
for (int i = 0; i < 5; ++i)
{
initiatingReceiver.PollOutput(outEvent);
initiatingReceiver.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kInternalError);
NL_TEST_ASSERT(inSuite, outEvent.statusData.statusCode == StatusCode::kBadBlockCounter);
}

err = AttachHeaderAndSend(statusReportMsgTypeData, std::move(statusReportMsg), respondingSender);
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
respondingSender.PollOutput(outEvent);
respondingSender.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kStatusReceived);
NL_TEST_ASSERT(inSuite, outEvent.statusData.statusCode == StatusCode::kBadBlockCounter);

// All subsequent PollOutput() calls should return kInternalError
for (int i = 0; i < 5; ++i)
{
respondingSender.PollOutput(outEvent);
respondingSender.PollOutput(outEvent, kNoAdvanceTime);
NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kInternalError);
NL_TEST_ASSERT(inSuite, outEvent.statusData.statusCode == StatusCode::kBadBlockCounter);
}
@@ -693,6 +735,7 @@ static const nlTest sTests[] =
NL_TEST_DEF("TestInitiatingReceiverReceiverDrive", TestInitiatingReceiverReceiverDrive),
NL_TEST_DEF("TestInitiatingSenderSenderDrive", TestInitiatingSenderSenderDrive),
NL_TEST_DEF("TestBadAcceptMessageFields", TestBadAcceptMessageFields),
NL_TEST_DEF("TestTimeout", TestTimeout),
NL_TEST_DEF("TestDuplicateBlockError", TestDuplicateBlockError),
NL_TEST_SENTINEL()
};