diff --git a/src/darwin/Framework/CHIP/MTROTAImageTransferHandler.mm b/src/darwin/Framework/CHIP/MTROTAImageTransferHandler.mm index cda7eba5b5f1ad..5bbd35c36e63c1 100644 --- a/src/darwin/Framework/CHIP/MTROTAImageTransferHandler.mm +++ b/src/darwin/Framework/CHIP/MTROTAImageTransferHandler.mm @@ -21,8 +21,6 @@ #import "MTROTAUnsolicitedBDXMessageHandler.h" #import "NSStringSpanConversion.h" -#include - 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(); diff --git a/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.h b/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.h index 3a0919929c2f96..dcbc10eb3f4403 100644 --- a/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.h +++ b/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; diff --git a/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.mm b/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.mm index 6ea633bb960b80..41b81738d2a77e 100644 --- a/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.mm +++ b/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.mm @@ -18,7 +18,7 @@ #import "MTROTAUnsolicitedBDXMessageHandler.h" #import "MTROTAImageTransferHandler.h" -#import +#include 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(delegate); - transferHandler->DestroySelf(); + auto * otaTransferHandler = static_cast(delegate); + otaTransferHandler->DestroySelf(); } uint8_t MTROTAUnsolicitedBDXMessageHandler::GetNumberOfDelegates() diff --git a/src/darwin/Framework/CHIPTests/MTROTAProviderTests.m b/src/darwin/Framework/CHIPTests/MTROTAProviderTests.m index f9259d92561cde..615aa7d5d30ef4 100644 --- a/src/darwin/Framework/CHIPTests/MTROTAProviderTests.m +++ b/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]; diff --git a/src/protocols/bdx/AsyncTransferFacilitator.cpp b/src/protocols/bdx/AsyncTransferFacilitator.cpp index 0b042e79426037..8ff303520a2696 100644 --- a/src/protocols/bdx/AsyncTransferFacilitator.cpp +++ b/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)); diff --git a/src/protocols/bdx/AsyncTransferFacilitator.h b/src/protocols/bdx/AsyncTransferFacilitator.h index fb7987691916fe..b11fe8b29b9da4 100644 --- a/src/protocols/bdx/AsyncTransferFacilitator.h +++ b/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); };