Address review comments

Author: nivi-apple

src/darwin/Framework/CHIP/MTROTAImageTransferHandler.mm

@@ -21,8 +21,6 @@
 #import "MTROTAUnsolicitedBDXMessageHandler.h"
 #import "NSStringSpanConversion.h"
 
-#include <MTRError_Internal.h>
-
 using namespace chip;
 using namespace chip::bdx;
 using namespace chip::app;
@@ -345,7 +343,7 @@
     VerifyOrReturnError(ec != nullptr, CHIP_ERROR_INCORRECT_STATE);
     CHIP_ERROR err;
 
-    // If we receive a ReceiveInit message, then we prepare for transfer
+    // If we receive a ReceiveInit message, then we prepare for transfer.
     if (payloadHeader.HasMessageType(MessageType::ReceiveInit)) {
         NodeId nodeId = ec->GetSessionHandle()->GetPeer().GetNodeId();
         FabricIndex fabricIndex = ec->GetSessionHandle()->GetFabricIndex();

src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.h

@@ -28,7 +28,9 @@ NS_ASSUME_NONNULL_BEGIN
  * This class creates an unsolicited handler for listening to all unsolicited BDX messages
  * and when it receives a BDX ReceiveInit message from a node, it creates a new
  * MTROTAImageTransferHandler object as a delegate that will prepare for transfer and
- * handle all BDX messages for the BDX transfer session with that node.
+ * handle all BDX messages for the BDX transfer session with that node. If it receives an out of order
+ * BDX message or if the message is received on a non-valid session, the OnUnsolicitedMessageReceived
+ * returns CHIP_ERROR_INCORRECT_STATE.
  *
  */
 class MTROTAUnsolicitedBDXMessageHandler : public chip::Messaging::UnsolicitedMessageHandler
@@ -55,8 +57,6 @@ class MTROTAUnsolicitedBDXMessageHandler : public chip::Messaging::UnsolicitedMe
 private:
 
     CHIP_ERROR OnUnsolicitedMessageReceived(const chip::PayloadHeader & payloadHeader, const chip::SessionHandle & session,
-                                                    chip::Messaging::ExchangeDelegate * _Nonnull & newDelegate) override;
-    CHIP_ERROR OnUnsolicitedMessageReceived(const chip::PayloadHeader & payloadHeader,
                                             chip::Messaging::ExchangeDelegate * _Nonnull & newDelegate) override;
 
     void OnExchangeCreationFailed(chip::Messaging::ExchangeDelegate * _Nonnull delegate) override;

src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.mm

@@ -18,7 +18,7 @@
 #import "MTROTAUnsolicitedBDXMessageHandler.h"
 #import "MTROTAImageTransferHandler.h"
 
-#import <protocols/bdx/BdxMessages.h>
+#include <protocols/bdx/BdxMessages.h>
 
 using namespace chip;
 using namespace chip::Messaging;
@@ -56,13 +56,7 @@
 }
 
 CHIP_ERROR MTROTAUnsolicitedBDXMessageHandler::OnUnsolicitedMessageReceived(const PayloadHeader & payloadHeader, const SessionHandle & session,
-                                                    ExchangeDelegate *  _Nonnull & newDelegate)
-{
-    return OnUnsolicitedMessageReceived(payloadHeader, newDelegate);
-}
-
-CHIP_ERROR MTROTAUnsolicitedBDXMessageHandler::OnUnsolicitedMessageReceived(
-    const PayloadHeader & payloadHeader, ExchangeDelegate * _Nonnull & newDelegate)
+    ExchangeDelegate *  _Nonnull & newDelegate)
 {
     assertChipStackLockedByCurrentThread();
 
@@ -71,19 +65,29 @@
 
     VerifyOrReturnError(mExchangeMgr != nullptr, CHIP_ERROR_INCORRECT_STATE);
 
-    // If we receive a ReceiveInit BDX message, create a new MTROTAImageTransferHandler and register it
-    // as the handler for all BDX messages that will come over this exchange and increment the number of delegates.
-    if (payloadHeader.HasMessageType(MessageType::ReceiveInit)) {
-        MTROTAImageTransferHandler * otaImageTransferHandler = new MTROTAImageTransferHandler();
-        newDelegate = otaImageTransferHandler;
+    if (GetNumberOfDelegates() >= 1)
+    {
+        return CHIP_ERROR_BUSY;
+    }
+
+    // Only proceed if there is a valid fabric index for the SessionHandle.
+    if (session->IsSecureSession() && session->AsSecureSession() != nullptr && session->AsSecureSession()->GetFabricIndex() != kUndefinedFabricIndex)
+    {
+        // If we receive a ReceiveInit BDX message, create a new MTROTAImageTransferHandler and register it
+        // as the handler for all BDX messages that will come over this exchange and increment the number of delegates.
+        if (payloadHeader.HasMessageType(MessageType::ReceiveInit)) {
+            MTROTAImageTransferHandler * otaImageTransferHandler = new MTROTAImageTransferHandler();
+            newDelegate = otaImageTransferHandler;
+            return CHIP_NO_ERROR;
+        }
     }
-    return CHIP_NO_ERROR;
+    return CHIP_ERROR_INCORRECT_STATE;
 }
 
 void MTROTAUnsolicitedBDXMessageHandler::OnExchangeCreationFailed(ExchangeDelegate * delegate)
 {
-    auto * transferHandler = static_cast<MTROTAImageTransferHandler *>(delegate);
-    transferHandler->DestroySelf();
+    auto * otaTransferHandler = static_cast<MTROTAImageTransferHandler *>(delegate);
+    otaTransferHandler->DestroySelf();
 }
 
 uint8_t MTROTAUnsolicitedBDXMessageHandler::GetNumberOfDelegates()

src/darwin/Framework/CHIPTests/MTROTAProviderTests.m

@@ -736,7 +736,6 @@ - (XCTestExpectation *)announceProviderToDevice:(MTRDevice *)device
 
 - (void)test001_ReceiveQueryImageRequest_RespondUpdateNotAvailable
 {
-    fprintf(stderr, "\n\n\n\n\n\n\n\n\nATTACH HERE: %d\n\n\n\n\n\n", getpid()); sleep(10);
     // Test that if we advertise ourselves as a provider we end up getting a
     // QueryImage callbacks that we can respond to.
     __auto_type * runner = [[MTROTARequestorAppRunner alloc] initWithPayload:kOnboardingPayload1 testcase:self];

src/protocols/bdx/AsyncTransferFacilitator.cpp

@@ -21,6 +21,10 @@
 namespace chip {
 namespace bdx {
 
+/**
+ * Releases the exchange, resets the transfer session and schedules calling the DestroySelf
+ * virtual method implemented by the subclass to delete the subclass.
+ */
 void AsyncTransferFacilitator::CleanUp()
 {
     mExchange.Release();
@@ -52,6 +56,11 @@ bdx::StatusCode AsyncTransferFacilitator::GetBdxStatusCodeFromChipError(CHIP_ERR
     return bdx::StatusCode::kUnknown;
 }
 
+/**
+ * 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()
 {
     if (mHandlingOutputEvents)
@@ -64,7 +73,7 @@ void AsyncTransferFacilitator::HandleNextOutputEvents()
 
     // 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
-    // virtual method that must be implemeted by the subclass to handle the output events.
+    // virtual method that must be implemeted by the subclass of this class to handle the BDX message.
     TransferSession::OutputEvent outEvent;
 
     mTransfer.GetNextAction(outEvent);
@@ -103,9 +112,9 @@ CHIP_ERROR AsyncTransferFacilitator::SendMessage(const TransferSession::MessageT
 
     CHIP_ERROR err = ec->SendMessage(msgTypeData.ProtocolId, msgTypeData.MessageType, std::move(msgBuf), sendFlags);
 
-    // If we failed to send the message across the exchange or if we just sent a status report, there is no way to let the other side know there was
-    // and error sending the message so call CleanUp to release the exchange and clean up so the other side can get notified the exchange is closing
-    // and clean up as needed.
+    // If we failed to send the message across the exchange, there is no way to let the other side know there was an error sending
+    // the message so call CleanUp to release the exchange so the other side can get notified the exchange is closing
+    // and clean up as needed. Also the CleanUp API resets the transfer session and destroys the subclass.
     if (err != CHIP_NO_ERROR)
     {
         CleanUp();
@@ -127,7 +136,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
-        // and clean up when we call HandleNextOutputEvents below.
+        // when we call HandleNextOutputEvents below.
         mTransfer.AbortTransfer(AsyncResponder::GetBdxStatusCodeFromChipError(err));
 
     }
@@ -176,8 +185,7 @@ void AsyncResponder::NotifyEventHandled(TransferSession::OutputEvent & event, CH
     ChipLogDetail(BDX, "NotifyEventHandled : Event %s Error %" CHIP_ERROR_FORMAT, event.ToString(event.EventType), error.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 the CleanUp method that calls the DestroySelf virtual method implemented
-    // by the subclass to delete both the sublcass and base class objects.
+    // or an error occured, we need to call CleanUp.
     if (event.EventType == TransferSession::OutputEventType::kAckEOFReceived ||
         event.EventType == TransferSession::OutputEventType::kInternalError ||
         event.EventType == TransferSession::OutputEventType::kTransferTimeout ||
@@ -187,8 +195,8 @@ void AsyncResponder::NotifyEventHandled(TransferSession::OutputEvent & event, CH
         return;
     }
 
-    // If there was an error, this should notify the tranfer object to abort transfer so it can send a status report
-    //  across the exchange and clean up when we call HandleNextOutputEvents below.
+    // 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)
     {
         mTransfer.AbortTransfer(GetBdxStatusCodeFromChipError(error));

src/protocols/bdx/AsyncTransferFacilitator.h

@@ -27,10 +27,12 @@ namespace chip {
 namespace bdx {
 
 /**
- * An abstract class with methods for handling BDX messages from an ExchangeContext. Once a message is receievd, this
- * class passes the message to the TransferSession and gets the next output event from the TransferSession state machine
- * and processes it i.e either sends a message across the exchange or calls the HandleTransferSessionOutput virtual method
- *  to notify the subclass of the event generated based on the type of event generated.
+ * An abstract class with methods for handling BDX messages received from an ExchangeContext. Once a message is receievd, this
+ * class passes the message to the TransferSession to process the received message and gets the next 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 receieves an output event of type
+ * TransferSession::OutputEventType::kNone. For messages that are sent to 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.
  *
  * This class does not define any methods for beginning a transfer or initializing the underlying TransferSession object.
  * See AsyncResponder for a class that does.
@@ -48,7 +50,7 @@ class AsyncTransferFacilitator : public Messaging::ExchangeDelegate
      * This method should be implemented to contain business-logic handling of BDX messages
      * and other TransferSession events.
      *
-     * @param[in] event An OutputEvent that contains output from the TransferSession object.
+     * @param[in] event An OutputEvent that contains the output from the TransferSession object.
      */
     virtual void HandleTransferSessionOutput(TransferSession::OutputEvent & event) = 0;
 
@@ -93,7 +95,7 @@ class AsyncTransferFacilitator : public Messaging::ExchangeDelegate
  * Provides a method for initializing the TransferSession members but still needs to be extended to implement
  * HandleTransferSessionOutput.
  *
- * An instance of some subclass of this class, should be used as the exchange delegate for a BDX transfer.
+ * An instance of some subclass of this class should be used as the exchange delegate for a BDX transfer.
  */
 class AsyncResponder : public AsyncTransferFacilitator
 {
@@ -112,12 +114,14 @@ class AsyncResponder : public AsyncTransferFacilitator
                                   System::Clock::Timeout timeout);
 
     /**
-     * This is called by the subclass implementing HandleTransferSessionOutput to notify the transfer facilitator
-     * that an error has ocurred while handling the last generated output event from this class so that the
-     * facilitator can call DestroySelf to dlet
+     * This is called by the subclass implementing HandleTransferSessionOutput to notify the AsyncTransferFacilitator
+     * that it has handled the OutputEvent specified in event and returns an error code (if any) or success in the error paramter.
+     * 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.
      * 
      *
-     * @param[in] error  The error that occured when handling the event.
+     * @param[in] event The OutputEvent that was handled by the subclass.
+     * @param[in] error The error code that occured when handling the event if an error occurs. Otherwise has CHIP_NO_ERROR.
      */
     void NotifyEventHandled(TransferSession::OutputEvent & event, CHIP_ERROR error);
 };