Addresssed review comments

Author: nivi-apple

src/darwin/Framework/CHIP/MTROTAImageTransferHandler.h

@@ -46,7 +46,7 @@ class MTROTAImageTransferHandler : public chip::bdx::AsyncResponder
                                  chip::System::PacketBufferHandle && payload) override;
 
 private:
-    CHIP_ERROR PrepareForTransfer(chip::System::Layer * layer, chip::Messaging::ExchangeContext * exchangeCtx,
+    CHIP_ERROR Init(chip::System::Layer * layer, chip::Messaging::ExchangeContext * exchangeCtx,
                                   chip::FabricIndex fabricIndex, chip::NodeId nodeId);
 
     CHIP_ERROR ConfigureState(chip::FabricIndex fabricIndex, chip::NodeId nodeId);

src/darwin/Framework/CHIP/MTROTAImageTransferHandler.mm

@@ -38,7 +38,7 @@
     MTROTAUnsolicitedBDXMessageHandler::IncrementNumberOfDelegates();
 }
 
-CHIP_ERROR MTROTAImageTransferHandler::PrepareForTransfer(System::Layer * layer,
+CHIP_ERROR MTROTAImageTransferHandler::Init(System::Layer * layer,
     Messaging::ExchangeContext * exchangeCtx, FabricIndex fabricIndex, NodeId nodeId)
 {
     assertChipStackLockedByCurrentThread();
@@ -50,7 +50,7 @@
 
     BitFlags<bdx::TransferControlFlags> flags(bdx::TransferControlFlags::kReceiverDrive);
 
-    return AsyncResponder::PrepareForTransfer(layer, exchangeCtx, kBdxRole, flags, kMaxBdxBlockSize, kBdxTimeout);
+    return AsyncResponder::Init(mSystemLayer, exchangeCtx, kBdxRole, flags, kMaxBdxBlockSize, kBdxTimeout);
 }
 
 MTROTAImageTransferHandler::~MTROTAImageTransferHandler()
@@ -350,7 +350,7 @@
         FabricIndex fabricIndex = ec->GetSessionHandle()->GetFabricIndex();
 
         if (nodeId != kUndefinedNodeId && fabricIndex != kUndefinedFabricIndex) {
-            err = PrepareForTransfer(&DeviceLayer::SystemLayer(), ec, fabricIndex, nodeId);
+            err = Init(&DeviceLayer::SystemLayer(), ec, fabricIndex, nodeId);
             if (err != CHIP_NO_ERROR) {
                 ChipLogError(Controller, "OnMessageReceived: Failed to prepare for transfer for BDX");
                 return err;

src/protocols/bdx/AsyncTransferFacilitator.cpp

@@ -54,21 +54,34 @@ bdx::StatusCode AsyncTransferFacilitator::GetBdxStatusCodeFromChipError(CHIP_ERR
     return bdx::StatusCode::kUnknown;
 }
 
+CHIP_ERROR AsyncTransferFacilitator::Init(System::Layer * layer, Messaging::ExchangeContext * exchangeCtx,
+                                  System::Clock::Timeout timeout)
+{
+    VerifyOrReturnError(layer != nullptr, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(exchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(!mExchange, CHIP_ERROR_INCORRECT_STATE);
+
+    mSystemLayer = layer;
+    mExchange.Grab(exchangeCtx);
+    mTimeout = timeout;
+    return CHIP_NO_ERROR;
+}
+
 /**
  * Calls the GetNextAction on the TransferSession to get the next output events until it receives
  * TransferSession::OutputEventType::kNone If the output event is of type TransferSession::OutputEventType::kMsgToSend, it sends the
  * message over the exchange context, otherwise it calls the HandleTransferSessionOutput method implemented by the subclass to
  * handle the BDX message.
  */
-void AsyncTransferFacilitator::HandleNextOutputEvents()
+void AsyncTransferFacilitator::ProcessOutputEvents()
 {
-    if (mHandlingOutputEvents)
+    if (mProcessingOutputEvents)
     {
-        ChipLogDetail(BDX, "HandleNextOutputEvents: Still getting and processing output events from a previous call. Return.");
+        ChipLogDetail(BDX, "ProcessOutputEvents: Still getting and processing output events from a previous call. Return.");
         return;
     }
 
-    mHandlingOutputEvents = true;
+    mProcessingOutputEvents = true;
 
     // Get the next output event and handle it based on the type of event.
     // If its of type kMsgToSend send it over the exchange, otherwise call the HandleTransferSessionOutput
@@ -88,7 +101,7 @@ void AsyncTransferFacilitator::HandleNextOutputEvents()
         }
         mTransfer.GetNextAction(outEvent);
     }
-    mHandlingOutputEvents = false;
+    mProcessingOutputEvents = false;
 }
 
 CHIP_ERROR AsyncTransferFacilitator::SendMessage(const TransferSession::MessageTypeData msgTypeData,
@@ -136,7 +149,7 @@ CHIP_ERROR AsyncTransferFacilitator::OnMessageReceived(Messaging::ExchangeContex
         ChipLogError(BDX, "OnMessageReceived: Failed to handle message: %" CHIP_ERROR_FORMAT, err.Format());
 
         // This should notify the tranfer object to abort transfer so it can send a status report across the exchange
-        // when we call HandleNextOutputEvents below.
+        // when we call ProcessOutputEvents below.
         mTransfer.AbortTransfer(AsyncResponder::GetBdxStatusCodeFromChipError(err));
     }
     else if (!payloadHeader.HasMessageType(MessageType::BlockAckEOF))
@@ -146,7 +159,7 @@ CHIP_ERROR AsyncTransferFacilitator::OnMessageReceived(Messaging::ExchangeContex
         ec->WillSendMessage();
     }
 
-    HandleNextOutputEvents();
+    ProcessOutputEvents();
     return err;
 }
 
@@ -162,26 +175,19 @@ void AsyncTransferFacilitator::OnResponseTimeout(Messaging::ExchangeContext * ec
     CleanUp();
 }
 
-CHIP_ERROR AsyncResponder::PrepareForTransfer(System::Layer * layer, Messaging::ExchangeContext * exchangeCtx, TransferRole role,
+CHIP_ERROR AsyncResponder::Init(System::Layer * layer, Messaging::ExchangeContext * exchangeCtx, TransferRole role,
                                               BitFlags<TransferControlFlags> xferControlOpts, uint16_t maxBlockSize,
                                               System::Clock::Timeout timeout)
 {
-    VerifyOrReturnError(exchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE);
-    VerifyOrReturnError(!mExchange, CHIP_ERROR_INCORRECT_STATE);
-
-    mSystemLayer = layer;
-    mTimeout     = timeout;
-
-    ReturnErrorOnFailure(mTransfer.WaitForTransfer(role, xferControlOpts, maxBlockSize, mTimeout));
-
-    mExchange.Grab(exchangeCtx);
+    AsyncTransferFacilitator::Init(layer, exchangeCtx, timeout);
+    ReturnErrorOnFailure(mTransfer.WaitForTransfer(role, xferControlOpts, maxBlockSize, timeout));
 
     return CHIP_NO_ERROR;
 }
 
-void AsyncResponder::NotifyEventHandled(TransferSession::OutputEvent & event, CHIP_ERROR error)
+void AsyncResponder::NotifyEventHandled(TransferSession::OutputEvent & event, CHIP_ERROR status)
 {
-    ChipLogDetail(BDX, "NotifyEventHandled : Event %s Error %" CHIP_ERROR_FORMAT, event.ToString(event.EventType), error.Format());
+    ChipLogDetail(BDX, "NotifyEventHandled : Event %s Error %" CHIP_ERROR_FORMAT, event.ToString(event.EventType), status.Format());
 
     // If it's a message indicating either the end of the transfer or a timeout reported by the transfer session
     // or an error occured, we need to call CleanUp.
@@ -195,14 +201,13 @@ void AsyncResponder::NotifyEventHandled(TransferSession::OutputEvent & event, CH
     }
 
     // If there was an error handling the output event, this should notify the tranfer object to abort transfer so it can send a
-    // status report
-    //  across the exchange when we call HandleNextOutputEvents below.
-    if (error != CHIP_NO_ERROR)
+    // status report across the exchange when we call ProcessOutputEvents below.
+    if (status != CHIP_NO_ERROR)
     {
-        mTransfer.AbortTransfer(GetBdxStatusCodeFromChipError(error));
+        mTransfer.AbortTransfer(GetBdxStatusCodeFromChipError(status));
     }
 
-    HandleNextOutputEvents();
+    ProcessOutputEvents();
 }
 
 } // namespace bdx

src/protocols/bdx/AsyncTransferFacilitator.h

@@ -28,12 +28,13 @@ namespace bdx {
 
 /**
  * An abstract class with methods for handling BDX messages received from an ExchangeContext. Once a message is received, this
- * class passes the message to the TransferSession to process the received message and gets the resulting output events from the
- * TransferSession state machine and either sends a message accross the exchange or calls the HandleTransferSessionOutput virtual
- * method to notify the subclass of the event generated. It keeps getting the next output event until it recieves an output event
- * of type TransferSession::OutputEventType::kNone. For events that are handled via the HandleTransferSessionOutput method, the
- * subclass must call the NotifyEventHandled to notify the AsyncTransferFacilitator that the event has been handled and returns an
- * error code for error cases or success.
+ * class passes the message to the TransferSession to process the received messages and calls ProcessOutputEvents to get the next
+ * output events generated by the TransferSession state machine until it receives an output event of type TransferSession::OutputEventType::kNone.
+ * For each output event it receives, it either sends a message accross the exchange if the TransferSession generated an event of
+ * type TransferSession::OutputEventType::kMsgToSend or calls the HandleTransferSessionOutput method to notify the subclass of the output event
+ * which has the business logic of handling the BDX message and calling the TransferSession API to proceed with the BDX transfer. For e.g.,
+ * If this class received a ReceiveInit message and passed it to the HandleTransferSessionOutput method, the subclass would call the
+ * AcceptTransfer on the TransferSession object to negotiate params and prepare a ReceiveAccept message.
  *
  * This class does not define any methods for beginning a transfer or initializing the underlying TransferSession object.
  * See AsyncResponder for a class that does.
@@ -47,6 +48,18 @@ class AsyncTransferFacilitator : public Messaging::ExchangeDelegate
     AsyncTransferFacilitator() : mExchange(*this) {}
     ~AsyncTransferFacilitator() override;
 
+protected:
+    CHIP_ERROR Init(System::Layer * layer, Messaging::ExchangeContext * exchangeCtx,
+                                  System::Clock::Timeout timeout);
+    CHIP_ERROR OnMessageReceived(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader,
+                                 System::PacketBufferHandle && payload) override;
+    void OnResponseTimeout(Messaging::ExchangeContext * ec) override;
+    void OnExchangeClosing(Messaging::ExchangeContext * ec) override;
+
+    CHIP_ERROR SendMessage(const TransferSession::MessageTypeData msgTypeData, System::PacketBufferHandle & msgBuf);
+
+    static bdx::StatusCode GetBdxStatusCodeFromChipError(CHIP_ERROR err);
+
     /**
      * This method should be implemented to contain business-logic handling of BDX messages
      * and other TransferSession events.
@@ -60,23 +73,16 @@ class AsyncTransferFacilitator : public Messaging::ExchangeDelegate
      */
     virtual void DestroySelf() = 0;
 
-protected:
-    CHIP_ERROR OnMessageReceived(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader,
-                                 System::PacketBufferHandle && payload) override;
-    void OnResponseTimeout(Messaging::ExchangeContext * ec) override;
-    void OnExchangeClosing(Messaging::ExchangeContext * ec) override;
-
-    CHIP_ERROR SendMessage(const TransferSession::MessageTypeData msgTypeData, System::PacketBufferHandle & msgBuf);
-
-    static bdx::StatusCode GetBdxStatusCodeFromChipError(CHIP_ERROR err);
+    void ProcessOutputEvents();
 
     void CleanUp();
 
-    void HandleNextOutputEvents();
-
     // The transfer session coresponding to this AsyncTransferFacilitator object.
     TransferSession mTransfer;
 
+private:
+    bool mProcessingOutputEvents = false;
+
     // The Exchange holder that holds the exchange context used for sending and receiving BDX messages.
     Messaging::ExchangeHolder mExchange;
 
@@ -85,12 +91,11 @@ class AsyncTransferFacilitator : public Messaging::ExchangeDelegate
 
     System::Layer * mSystemLayer;
 
-private:
-    bool mHandlingOutputEvents;
 };
 
 /**
  * An AsyncTransferFacilitator that is initialized to respond to an incoming BDX transfer request.
+ * An AsyncResponder object is associated with an exchange and handles all BDX messages sent over that exchange.
  *
  * Provides a method for initializing the TransferSession members but still needs to be extended to implement
  * HandleTransferSessionOutput.
@@ -109,21 +114,24 @@ class AsyncResponder : public AsyncTransferFacilitator
      * @param[in] maxBlockSize    The maximum supported size of BDX Block data
      * @param[in] timeout         The chosen timeout delay for the BDX transfer
      */
-    CHIP_ERROR PrepareForTransfer(System::Layer * layer, Messaging::ExchangeContext * exchangeCtx, TransferRole role,
+    CHIP_ERROR Init(System::Layer * layer, Messaging::ExchangeContext * exchangeCtx, TransferRole role,
                                   BitFlags<TransferControlFlags> xferControlOpts, uint16_t maxBlockSize,
                                   System::Clock::Timeout timeout);
 
     /**
-     * This is called by the subclass implementing HandleTransferSessionOutput to notify the AsyncTransferFacilitator
+     * This is called by the subclass implementing HandleTransferSessionOutput to notify the AsyncResponder
      * that it has handled the OutputEvent specified in "event" and "status" is the result of handling the event.
-     * Once this is called the AsyncTransferFacilitator either aborts the transfer if an error has ocurred or drives the
-     * TransferSession state machine to generate the next output events to establish and continue the BDX session further.
-     *
+     * If the OutputEvent was handled successfully by the subclass that means the TransferSession API call to handle the output event
+     * was successful and the TransferSession has the next message prepared. In that case, the "status" is set to CHIP_NO_ERROR and the
+     * AsyncResponder should call ProcessOutputEvents to get and process the next output events from the TransferSession to proceed with
+     * the BDX transfer. Otherwise if the output event indicates the end of BDX transfer (like EOF receieved or transfer timeout), it will
+     * clean up and destroy itself and the subclass. If the "status" is set to an error code, the AsyncResponder should abort the transfer
+     * and clean up once the status report is sent over the exchange.
      *
      * @param[in] event The OutputEvent that was handled by the subclass.
      * @param[in] status The error code that occured when handling the event if an error occurs. Otherwise CHIP_NO_ERROR.
      */
-    void NotifyEventHandled(TransferSession::OutputEvent & event, CHIP_ERROR error);
+    void NotifyEventHandled(TransferSession::OutputEvent & event, CHIP_ERROR status);
 };
 
 } // namespace bdx