From 6d357573d2b735c89c98d4a99ceeee676269adfb Mon Sep 17 00:00:00 2001 From: Nivedita Sarkar Date: Sat, 30 Sep 2023 18:49:48 -0700 Subject: [PATCH] see if tests pass --- .../CHIP/MTROTAImageTransferHandler.mm | 3 +- .../CHIP/MTROTAProviderDelegateBridge.h | 8 ++++ .../CHIP/MTROTAProviderDelegateBridge.mm | 39 +++++++++++++++---- .../CHIP/MTROTAUnsolicitedBDXMessageHandler.h | 8 ++++ .../MTROTAUnsolicitedBDXMessageHandler.mm | 35 ++++++++++++++--- .../bdx/AsyncTransferFacilitator.cpp | 38 +++++++++--------- 6 files changed, 96 insertions(+), 35 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTROTAImageTransferHandler.mm b/src/darwin/Framework/CHIP/MTROTAImageTransferHandler.mm index 53d2b9311aeee5..de1a90a62fe950 100644 --- a/src/darwin/Framework/CHIP/MTROTAImageTransferHandler.mm +++ b/src/darwin/Framework/CHIP/MTROTAImageTransferHandler.mm @@ -153,8 +153,7 @@ CHIP_ERROR error = CHIP_NO_ERROR; if (event.EventType == TransferSession::OutputEventType::kTransferTimeout) { error = CHIP_ERROR_TIMEOUT; - } else if (event.EventType != TransferSession::OutputEventType::kAckEOFReceived - || event.EventType != TransferSession::OutputEventType::kAckEOFReceived) { + } else if (event.EventType != TransferSession::OutputEventType::kAckEOFReceived) { error = CHIP_ERROR_INTERNAL; } diff --git a/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h b/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h index 63fdc822fedfd0..79800269610ef3 100644 --- a/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h +++ b/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h @@ -30,6 +30,14 @@ class MTROTAProviderDelegateBridge : public chip::app::Clusters::OTAProviderDele CHIP_ERROR Init(chip::Messaging::ExchangeManager * exchangeMgr); + // Shutdown must be called after the event loop has been stopped, since it + // touches Matter objects. + void Shutdown(); + + // ControllerShuttingDown must be called on the Matter work queue, since it + // touches Matter objects. + void ControllerShuttingDown(MTRDeviceController * controller); + void HandleQueryImage( chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath, const chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::QueryImage::DecodableType & commandData) override; diff --git a/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm b/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm index ff30a9ebfca90b..ee8aefa9bde0d5 100644 --- a/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm +++ b/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm @@ -50,18 +50,17 @@ MTROTAProviderDelegateBridge::MTROTAProviderDelegateBridge() { Clusters::OTAProvider::SetDelegate(kOtaProviderEndpoint, this); - - // The singleton unsolicited BDX message handler which will create - // the MTROTAImageTransferHandler object for handling each BDX session. - sOtaUnsolicitedBDXMsgHandler = new MTROTAUnsolicitedBDXMessageHandler(); } MTROTAProviderDelegateBridge::~MTROTAProviderDelegateBridge() { Clusters::OTAProvider::SetDelegate(kOtaProviderEndpoint, nullptr); - delete sOtaUnsolicitedBDXMsgHandler; - sOtaUnsolicitedBDXMsgHandler = nullptr; + if (sOtaUnsolicitedBDXMsgHandler != nil) + { + delete sOtaUnsolicitedBDXMsgHandler; + sOtaUnsolicitedBDXMsgHandler = nil; + } } CHIP_ERROR MTROTAProviderDelegateBridge::Init(Messaging::ExchangeManager * exchangeMgr) @@ -70,9 +69,13 @@ VerifyOrReturnError(exchangeMgr != nullptr, CHIP_ERROR_INCORRECT_STATE); - // Initialize the singleton MTROTAUnsolicitedBDXMessageHandler - VerifyOrReturnValue(sOtaUnsolicitedBDXMsgHandler != nil, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnValue(sOtaUnsolicitedBDXMsgHandler == nil, CHIP_ERROR_INCORRECT_STATE); + // The singleton unsolicited BDX message handler which will create + // the MTROTAImageTransferHandler object for handling each BDX session. + sOtaUnsolicitedBDXMsgHandler = new MTROTAUnsolicitedBDXMessageHandler(); + + // Initialize the singleton MTROTAUnsolicitedBDXMessageHandler CHIP_ERROR err = sOtaUnsolicitedBDXMsgHandler->Init(exchangeMgr); if (err != CHIP_NO_ERROR) { ChipLogError(Controller, "Failed to initialize the unsolicited BDX Message handler with err %s", err.AsString()); @@ -80,6 +83,26 @@ return err; } +void MTROTAProviderDelegateBridge::Shutdown() +{ + assertChipStackLockedByCurrentThread(); + + if (sOtaUnsolicitedBDXMsgHandler != nullptr) + { + sOtaUnsolicitedBDXMsgHandler->Shutdown(); + } +} + +void MTROTAProviderDelegateBridge::ControllerShuttingDown(MTRDeviceController * controller) +{ + assertChipStackLockedByCurrentThread(); + + if (sOtaUnsolicitedBDXMsgHandler != nullptr) + { + sOtaUnsolicitedBDXMsgHandler->ControllerShuttingDown(controller); + } +} + namespace { // Return false if we could not get peer node info (a running controller for // the fabric and a node id). In that case we will have already added an diff --git a/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.h b/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.h index 244739d95408fa..233ae81b1605a6 100644 --- a/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.h +++ b/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.h @@ -36,6 +36,14 @@ class MTROTAUnsolicitedBDXMessageHandler : public chip::Messaging::UnsolicitedMe CHIP_ERROR Init(chip::Messaging::ExchangeManager * exchangeManager); + // Shutdown must be called after the event loop has been stopped, since it + // touches Matter objects. + void Shutdown(); + + // ControllerShuttingDown must be called on the Matter work queue, since it + // touches Matter objects. + void ControllerShuttingDown(MTRDeviceController * controller); + // Returns the number of delegates that are currently handling BDX transfers static uint8_t GetNumberOfDelegates(); diff --git a/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.mm b/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.mm index 2352adfb3f162d..3f97b9b3e4745f 100644 --- a/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.mm +++ b/src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.mm @@ -27,11 +27,8 @@ MTROTAUnsolicitedBDXMessageHandler::~MTROTAUnsolicitedBDXMessageHandler() { assertChipStackLockedByCurrentThread(); + mExchangeMgr = nullptr; - if (mExchangeMgr) { - mExchangeMgr->UnregisterUnsolicitedMessageHandlerForProtocol(Protocols::BDX::Id); - mExchangeMgr = nullptr; - } MTROTAUnsolicitedBDXMessageHandler::mNumberOfDelegates = 0; } @@ -47,6 +44,26 @@ return mExchangeMgr->RegisterUnsolicitedMessageHandlerForProtocol(Protocols::BDX::Id, this); } +void MTROTAUnsolicitedBDXMessageHandler::Shutdown() +{ + assertChipStackLockedByCurrentThread(); + + if (mExchangeMgr) { + mExchangeMgr->UnregisterUnsolicitedMessageHandlerForProtocol(Protocols::BDX::Id); + } + MTROTAUnsolicitedBDXMessageHandler::mNumberOfDelegates = 0; +} + +void MTROTAUnsolicitedBDXMessageHandler::ControllerShuttingDown(MTRDeviceController * controller) +{ + assertChipStackLockedByCurrentThread(); + + if (mExchangeMgr) { + mExchangeMgr->UnregisterUnsolicitedMessageHandlerForProtocol(Protocols::BDX::Id); + } + MTROTAUnsolicitedBDXMessageHandler::mNumberOfDelegates = 0; +} + CHIP_ERROR MTROTAUnsolicitedBDXMessageHandler::OnUnsolicitedMessageReceived( const PayloadHeader & payloadHeader, ExchangeDelegate * _Nonnull & newDelegate) { @@ -71,6 +88,12 @@ return MTROTAUnsolicitedBDXMessageHandler::mNumberOfDelegates; } -void MTROTAUnsolicitedBDXMessageHandler::IncrementNumberOfDelegates() { MTROTAUnsolicitedBDXMessageHandler::mNumberOfDelegates++; } +void MTROTAUnsolicitedBDXMessageHandler::IncrementNumberOfDelegates() +{ + MTROTAUnsolicitedBDXMessageHandler::mNumberOfDelegates++; +} -void MTROTAUnsolicitedBDXMessageHandler::DecrementNumberOfDelegates() { MTROTAUnsolicitedBDXMessageHandler::mNumberOfDelegates--; } +void MTROTAUnsolicitedBDXMessageHandler::DecrementNumberOfDelegates() +{ + MTROTAUnsolicitedBDXMessageHandler::mNumberOfDelegates--; +} diff --git a/src/protocols/bdx/AsyncTransferFacilitator.cpp b/src/protocols/bdx/AsyncTransferFacilitator.cpp index d679b56405d5f8..e6a92189c0979f 100644 --- a/src/protocols/bdx/AsyncTransferFacilitator.cpp +++ b/src/protocols/bdx/AsyncTransferFacilitator.cpp @@ -28,7 +28,7 @@ AsyncTransferFacilitator::~AsyncTransferFacilitator() void AsyncTransferFacilitator::Reset() { - mExchange.Release(); + //mExchange.Release(); mTransfer.Reset(); } @@ -94,12 +94,7 @@ CHIP_ERROR AsyncTransferFacilitator::OnMessageReceived(Messaging::ExchangeContex // If its a message of type kMsgToSend, send the message over the exchange. if (outEvent.EventType == TransferSession::OutputEventType::kMsgToSend) { - err = SendMessage(outEvent); - if (err != CHIP_NO_ERROR) - { - // TODO: Should we abort transfer here or let the other side timeout - return err; - } + SendMessage(outEvent); } else { @@ -127,7 +122,7 @@ CHIP_ERROR AsyncTransferFacilitator::OnMessageReceived(Messaging::ExchangeContex void AsyncTransferFacilitator::OnExchangeClosing(Messaging::ExchangeContext * ec) { ChipLogDetail(BDX, "OnExchangeClosing, ec: " ChipLogFormatExchange, ChipLogValueExchange(ec)); - CleanUp(); + mExchange.Release(); } void AsyncTransferFacilitator::OnResponseTimeout(Messaging::ExchangeContext * ec) @@ -188,20 +183,25 @@ void AsyncResponder::NotifyEventHandled(CHIP_ERROR eventHandlingResult) { if (outEvent.EventType == TransferSession::OutputEventType::kMsgToSend) { - CHIP_ERROR err = AsyncTransferFacilitator::SendMessage(outEvent); - if (err != CHIP_NO_ERROR) - { - // TODO: Should we abort transfer here or let the other side timeout - return; - } + AsyncTransferFacilitator::SendMessage(outEvent); } - else if (outEvent.EventType == TransferSession::OutputEventType::kInternalError || - outEvent.EventType == TransferSession::OutputEventType::kTransferTimeout || - outEvent.EventType == TransferSession::OutputEventType::kStatusReceived) + else { + // Call the HandleTransferSessionOutput virtual method that's implemeted by the subclass to handle + // the output events. HandleTransferSessionOutput(outEvent); - CleanUp(); - return; + + // 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 virtual method implemented by the subclass to clean up and + // delete both the sublcass and base class objects. + if (outEvent.EventType == TransferSession::OutputEventType::kAckEOFReceived || + outEvent.EventType == TransferSession::OutputEventType::kInternalError || + outEvent.EventType == TransferSession::OutputEventType::kTransferTimeout || + outEvent.EventType == TransferSession::OutputEventType::kStatusReceived) + { + CleanUp(); + return; + } } mTransfer.GetNextAction(outEvent); };