- Removed timeout from TransferSession::StartTransfer() and TransferS…

…ession::WaitForTransfer() - Remove kNoAdvanceTime and its usage - Removed TestTimeout() test suite
project-chip · Mar 28, 2022 · 7a4061a · 7a4061a
1 parent b3528c5
commit 7a4061a
Show file tree

Hide file tree

Showing 5 changed files with 27 additions and 75 deletions.
diff --git a/src/app/clusters/ota-requestor/BDXDownloader.cpp b/src/app/clusters/ota-requestor/BDXDownloader.cpp
@@ -104,8 +104,7 @@ 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,
-                                                    /* TODO:(#12520) */ chip::System::Clock::Seconds16(30)));
+    ReturnErrorOnFailure(mBdxTransfer.StartTransfer(chip::bdx::TransferRole::kReceiver, bdxInitData));
 
     return CHIP_NO_ERROR;
 }

diff --git a/src/protocols/bdx/BdxTransferSession.cpp b/src/protocols/bdx/BdxTransferSession.cpp
@@ -119,7 +119,7 @@ void TransferSession::PollOutput(OutputEvent & event)
     mPendingOutput = OutputEventType::kNone;
 }
 
-CHIP_ERROR TransferSession::StartTransfer(TransferRole role, const TransferInitData & initData, System::Clock::Timeout timeout)
+CHIP_ERROR TransferSession::StartTransfer(TransferRole role, const TransferInitData & initData)
 {
     VerifyOrReturnError(mState == TransferState::kUnitialized, CHIP_ERROR_INCORRECT_STATE);
 
@@ -161,7 +161,7 @@ CHIP_ERROR TransferSession::StartTransfer(TransferRole role, const TransferInitD
 }
 
 CHIP_ERROR TransferSession::WaitForTransfer(TransferRole role, BitFlags<TransferControlFlags> xferControlOpts,
-                                            uint16_t maxBlockSize, System::Clock::Timeout timeout)
+                                            uint16_t maxBlockSize)
 {
     VerifyOrReturnError(mState == TransferState::kUnitialized, CHIP_ERROR_INCORRECT_STATE);
 

diff --git a/src/protocols/bdx/BdxTransferSession.h b/src/protocols/bdx/BdxTransferSession.h
@@ -172,13 +172,11 @@ 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, System::Clock::Timeout timeout);
+    CHIP_ERROR StartTransfer(TransferRole role, const TransferInitData & initData);
 
     /**
      * @brief
@@ -189,13 +187,11 @@ 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,
-                               System::Clock::Timeout timeout);
+    CHIP_ERROR WaitForTransfer(TransferRole role, BitFlags<TransferControlFlags> xferControlOpts, uint16_t maxBlockSize);
 
     /**
      * @brief

diff --git a/src/protocols/bdx/TransferFacilitator.cpp b/src/protocols/bdx/TransferFacilitator.cpp
@@ -94,7 +94,7 @@ CHIP_ERROR Responder::PrepareForTransfer(System::Layer * layer, TransferRole rol
     mPollFreq    = pollFreq;
     mSystemLayer = layer;
 
-    ReturnErrorOnFailure(mTransfer.WaitForTransfer(role, xferControlOpts, maxBlockSize, timeout));
+    ReturnErrorOnFailure(mTransfer.WaitForTransfer(role, xferControlOpts, maxBlockSize));
 
     mSystemLayer->StartTimer(mPollFreq, PollTimerHandler, this);
     return CHIP_NO_ERROR;
@@ -108,7 +108,7 @@ CHIP_ERROR Initiator::InitiateTransfer(System::Layer * layer, TransferRole role,
     mPollFreq    = pollFreq;
     mSystemLayer = layer;
 
-    ReturnErrorOnFailure(mTransfer.StartTransfer(role, initData, timeout));
+    ReturnErrorOnFailure(mTransfer.StartTransfer(role, initData));
 
     mSystemLayer->StartTimer(mPollFreq, PollTimerHandler, this);
     return CHIP_NO_ERROR;

diff --git a/src/protocols/bdx/tests/TestBdxTransferSession.cpp b/src/protocols/bdx/tests/TestBdxTransferSession.cpp
@@ -20,8 +20,6 @@ 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);
@@ -86,7 +84,7 @@ CHIP_ERROR AttachHeaderAndSend(TransferSession::MessageTypeData typeData, chip::
     chip::PayloadHeader payloadHeader;
     payloadHeader.SetMessageType(typeData.ProtocolId, typeData.MessageType);
 
-    ReturnErrorOnFailure(receiver.HandleMessageReceived(payloadHeader, std::move(msgBuf), kNoAdvanceTime));
+    ReturnErrorOnFailure(receiver.HandleMessageReceived(payloadHeader, std::move(msgBuf)));
     return CHIP_NO_ERROR;
 }
 
@@ -131,7 +129,7 @@ void VerifyStatusReport(nlTestSuite * inSuite, void * inContext, const System::P
 void VerifyNoMoreOutput(nlTestSuite * inSuite, void * inContext, TransferSession & transferSession)
 {
     TransferSession::OutputEvent event;
-    transferSession.PollOutput(event, kNoAdvanceTime);
+    transferSession.PollOutput(event);
     NL_TEST_ASSERT(inSuite, event.EventType == TransferSession::OutputEventType::kNone);
 }
 
@@ -147,22 +145,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, timeout);
+    err = responder.WaitForTransfer(responderRole, responderControlOpts, responderMaxBlock);
     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, timeout);
+    err = initiator.StartTransfer(initiatorRole, initData);
     NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
-    initiator.PollOutput(outEvent, kNoAdvanceTime);
+    initiator.PollOutput(outEvent);
     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, kNoAdvanceTime);
+    responder.PollOutput(outEvent);
     VerifyNoMoreOutput(inSuite, inContext, responder);
     NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kInitReceived);
     NL_TEST_ASSERT(inSuite, outEvent.transferInitData.TransferCtlFlags == initData.TransferCtlFlags);
@@ -215,7 +213,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, kNoAdvanceTime);
+    acceptSender.PollOutput(outEvent);
     VerifyNoMoreOutput(inSuite, inContext, acceptSender);
     NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kMsgToSend);
     VerifyBdxMessageToSend(inSuite, inContext, outEvent, expectedMsg);
@@ -227,7 +225,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, kNoAdvanceTime);
+    acceptReceiver.PollOutput(outEvent);
     VerifyNoMoreOutput(inSuite, inContext, acceptReceiver);
     NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kAcceptReceived);
     NL_TEST_ASSERT(inSuite, outEvent.transferAcceptData.ControlMode == acceptData.ControlMode);
@@ -262,15 +260,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, kNoAdvanceTime);
+    querySender.PollOutput(outEvent);
     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, kNoAdvanceTime);
+    queryReceiver.PollOutput(outEvent);
     NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kQueryReceived);
     VerifyNoMoreOutput(inSuite, inContext, queryReceiver);
 }
@@ -305,15 +303,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, kNoAdvanceTime);
+    sender.PollOutput(outEvent);
     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, kNoAdvanceTime);
+    receiver.PollOutput(outEvent);
     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)
@@ -335,15 +333,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, kNoAdvanceTime);
+    ackSender.PollOutput(outEvent);
     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, kNoAdvanceTime);
+    ackReceiver.PollOutput(outEvent);
     NL_TEST_ASSERT(inSuite, outEvent.EventType == expectedEventType);
     VerifyNoMoreOutput(inSuite, inContext, ackReceiver);
 }
@@ -576,46 +574,6 @@ 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)
@@ -675,7 +633,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, kNoAdvanceTime);
+    respondingSender.PollOutput(eventWithBlock);
     NL_TEST_ASSERT(inSuite, eventWithBlock.EventType == TransferSession::OutputEventType::kMsgToSend);
     VerifyBdxMessageToSend(inSuite, inContext, eventWithBlock, MessageType::Block);
     VerifyNoMoreOutput(inSuite, inContext, respondingSender);
@@ -685,7 +643,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, kNoAdvanceTime);
+    initiatingReceiver.PollOutput(outEvent);
     NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kBlockReceived);
     NL_TEST_ASSERT(inSuite, outEvent.blockdata.Data != nullptr);
     VerifyNoMoreOutput(inSuite, inContext, initiatingReceiver);
@@ -695,7 +653,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, kNoAdvanceTime);
+    initiatingReceiver.PollOutput(outEvent);
     NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kMsgToSend);
     System::PacketBufferHandle statusReportMsg               = outEvent.MsgData.Retain();
     TransferSession::MessageTypeData statusReportMsgTypeData = outEvent.msgTypeData;
@@ -704,21 +662,21 @@ void TestDuplicateBlockError(nlTestSuite * inSuite, void * inContext)
     // All subsequent PollOutput() calls should return kInternalError
     for (int i = 0; i < 5; ++i)
     {
-        initiatingReceiver.PollOutput(outEvent, kNoAdvanceTime);
+        initiatingReceiver.PollOutput(outEvent);
         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, kNoAdvanceTime);
+    respondingSender.PollOutput(outEvent);
     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, kNoAdvanceTime);
+        respondingSender.PollOutput(outEvent);
         NL_TEST_ASSERT(inSuite, outEvent.EventType == TransferSession::OutputEventType::kInternalError);
         NL_TEST_ASSERT(inSuite, outEvent.statusData.statusCode == StatusCode::kBadBlockCounter);
     }
@@ -735,7 +693,6 @@ 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()
 };