Skip to content

Commit

Permalink
Addressed review comments
Browse files Browse the repository at this point in the history 
- Enabled the OTA tests that use a real OTA image file
  • Loading branch information
@nivi-apple
nivi-apple committed Oct 2, 2023
1 parent c78db6d commit bfba54f
Show file tree
Hide file tree
Showing 8 changed files with 198 additions and 216 deletions.
28 changes: 15 additions & 13 deletions src/darwin/Framework/CHIP/MTROTAImageTransferHandler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,28 +23,34 @@ NS_ASSUME_NONNULL_BEGIN

/**
* This class inherits from the AsyncResponder class and handles the BDX messages for a BDX transfer session.
* It overrides the HandleAsyncTransferSessionOutput virtual method and provides an implementation for it.
* It overrides the HandleTransferSessionOutput virtual method and provides an implementation for it to handle
* the OutputEvents that are generated by the BDX transfer session state machine.
*
* For each BDX transfer, we will have an instance of MTROTAImageTransferHandler.
* An MTROTAImageTransferHandler will be associated with a specific BDX transfer session.
*
* The lifecycle of this class is managed by the AsyncFacilitator class which calls the virtual CleanUp method
* that is implemented by this class to clean up and destroy itself and the AsyncFacilitator instances.
* Note: An object of this class can't be used after CleanUp has been called.
*/
class MTROTAImageTransferHandler : public chip::bdx::AsyncResponder
{
public:
MTROTAImageTransferHandler();
~MTROTAImageTransferHandler();

void HandleAsyncTransferSessionOutput(chip::bdx::TransferSession::OutputEvent & event) override;
void HandleTransferSessionOutput(chip::bdx::TransferSession::OutputEvent & event) override;
void CleanUp() override;

protected:
CHIP_ERROR OnMessageReceived(chip::Messaging::ExchangeContext * ec, const chip::PayloadHeader & payloadHeader,
chip::System::PacketBufferHandle && payload) override;

private:
CHIP_ERROR PrepareForTransfer(chip::Messaging::ExchangeContext * exchangeCtx, chip::FabricIndex fabricIndex,
chip::NodeId nodeId);

void ResetState();

CHIP_ERROR ConfigureState(chip::FabricIndex fabricIndex, chip::NodeId nodeId);

static void HandleBdxInitReceivedTimeoutExpired(chip::System::Layer * systemLayer, void * state);

CHIP_ERROR OnMessageToSend(chip::bdx::TransferSession::OutputEvent & event);

CHIP_ERROR OnTransferSessionBegin(chip::bdx::TransferSession::OutputEvent & event);
Expand All @@ -53,14 +59,10 @@ class MTROTAImageTransferHandler : public chip::bdx::AsyncResponder

CHIP_ERROR OnBlockQuery(chip::bdx::TransferSession::OutputEvent & event);

// Inherited from ExchangeContext
CHIP_ERROR OnMessageReceived(chip::Messaging::ExchangeContext * ec, const chip::PayloadHeader & payloadHeader,
chip::System::PacketBufferHandle && payload) override;

// The fabric index of the peer node.
// The fabric index of the node with which the BDX session is established.
chip::Optional<chip::FabricIndex> mFabricIndex;

// The node id of the peer node.
// The node id of the node with which the BDX session is established.
chip::Optional<chip::NodeId> mNodeId;

// The OTA provider delegate used by the controller.
Expand Down
140 changes: 29 additions & 111 deletions src/darwin/Framework/CHIP/MTROTAImageTransferHandler.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,13 @@
// TODO Expose a method onto the delegate to make that configurable.
constexpr uint32_t kMaxBdxBlockSize = 1024;

// Since the BDX timeout is 5 minutes and we are starting this after query image is available and before the BDX init comes,
// we just double the timeout to give enough time for the BDX init to come in a reasonable amount of time.
constexpr System::Clock::Timeout kBdxInitReceivedTimeout = System::Clock::Seconds16(10 * 60);

constexpr System::Clock::Timeout kBdxTimeout = System::Clock::Seconds16(5 * 60); // OTA Spec mandates >= 5 minutes
// Timeout for the BDX transfer session. The OTA Spec mandates this should be >= 5 minutes.
constexpr System::Clock::Timeout kBdxTimeout = System::Clock::Seconds16(5 * 60);
constexpr bdx::TransferRole kBdxRole = bdx::TransferRole::kSender;

MTROTAImageTransferHandler::MTROTAImageTransferHandler()
{
// Increment the number of delegates by 1.
// Increment the number of delegates handling BDX by 1.
MTROTAUnsolicitedBDXMessageHandler::IncrementNumberOfDelegates();
}

Expand All @@ -55,77 +52,20 @@
return AsyncResponder::PrepareForTransfer(exchangeCtx, kBdxRole, flags, kMaxBdxBlockSize, kBdxTimeout);
}

MTROTAImageTransferHandler::~MTROTAImageTransferHandler() { ResetState(); }

void MTROTAImageTransferHandler::ResetState()
MTROTAImageTransferHandler::~MTROTAImageTransferHandler()
{
assertChipStackLockedByCurrentThread();
if (mNodeId.HasValue() && mFabricIndex.HasValue()) {
ChipLogProgress(Controller,
"Resetting state for OTA Provider; no longer providing an update for node id 0x" ChipLogFormatX64 ", fabric index %u",
ChipLogValueX64(mNodeId.Value()), mFabricIndex.Value());
} else {
ChipLogProgress(Controller, "Resetting state for OTA Provider");
}
chip::DeviceLayer::SystemLayer().CancelTimer(HandleBdxInitReceivedTimeoutExpired, this);

AsyncResponder::ResetTransfer();
mFabricIndex.ClearValue();
mNodeId.ClearValue();
mDelegate = nil;
mDelegateNotificationQueue = nil;
MTROTAUnsolicitedBDXMessageHandler::DecrementNumberOfDelegates();
}
/**
* Timer callback called when we don't receive a BDX init within a reasonable time after a successful QueryImage response.
*/
void MTROTAImageTransferHandler::HandleBdxInitReceivedTimeoutExpired(chip::System::Layer * systemLayer, void * state)
{
VerifyOrReturn(state != nullptr);
static_cast<MTROTAImageTransferHandler *>(state)->ResetState();
}

CHIP_ERROR MTROTAImageTransferHandler::OnMessageToSend(TransferSession::OutputEvent & event)
{
assertChipStackLockedByCurrentThread();

VerifyOrReturnError(AsyncTransferFacilitator::GetExchangeContext() != nullptr, CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mDelegate != nil, CHIP_ERROR_INCORRECT_STATE);

Messaging::SendFlags sendFlags;

// All messages sent from the Sender expect a response, except for a StatusReport which would indicate an error and
// the end of the transfer.
if (!event.msgTypeData.HasMessageType(Protocols::SecureChannel::MsgType::StatusReport)) {
sendFlags.Set(Messaging::SendMessageFlags::kExpectResponse);
}

auto & msgTypeData = event.msgTypeData;
ChipLogError(BDX, "OnMessageToSend msgTypeData.MessageType = %hu", msgTypeData.MessageType);
Messaging::ExchangeContext * ec = AsyncTransferFacilitator::GetExchangeContext();
VerifyOrReturnError(ec != nullptr, CHIP_ERROR_INCORRECT_STATE);

CHIP_ERROR err = CHIP_NO_ERROR;
if (ec != nullptr) {
// If there's an error sending the message, call ResetState.
err = ec->SendMessage(msgTypeData.ProtocolId, msgTypeData.MessageType, std::move(event.MsgData), sendFlags);
if (err != CHIP_NO_ERROR) {
ResetState();
} else if (event.msgTypeData.HasMessageType(Protocols::SecureChannel::MsgType::StatusReport)) {
// If the send was successful for a status report, since we are not expecting a response the exchange context is
// already closed. We need to null out the reference to avoid having a dangling pointer.
ec = nullptr;
ResetState();
}
}
return err;
// Decrement the number of delegates handling BDX by 1.
MTROTAUnsolicitedBDXMessageHandler::DecrementNumberOfDelegates();
}

CHIP_ERROR MTROTAImageTransferHandler::OnTransferSessionBegin(TransferSession::OutputEvent & event)
{
assertChipStackLockedByCurrentThread();
// Once we receive the BDX init, cancel the BDX Init timeout and start the BDX session
chip::DeviceLayer::SystemLayer().CancelTimer(HandleBdxInitReceivedTimeoutExpired, this);

VerifyOrReturnError(mFabricIndex.HasValue(), CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mNodeId.HasValue(), CHIP_ERROR_INCORRECT_STATE);
Expand All @@ -152,8 +92,7 @@
if (error != nil) {
CHIP_ERROR err = [MTRError errorToCHIPErrorCode:error];
LogErrorOnFailure(err);
ResetState();
AsyncResponder::NotifyEventHandledWithError(err);
AsyncResponder::NotifyEventHandled(err);
return;
}

Expand All @@ -166,9 +105,9 @@
acceptData.StartOffset = mTransfer.GetStartOffset();
acceptData.Length = mTransfer.GetTransferLength();

LogErrorOnFailure(mTransfer.AcceptTransfer(acceptData));
AsyncResponder::NotifyEventHandledWithError([MTRError errorToCHIPErrorCode:error]);
return;
CHIP_ERROR err = mTransfer.AcceptTransfer(acceptData);
LogErrorOnFailure(err);
AsyncResponder::NotifyEventHandled(err);
}
errorHandler:^(NSError *) {
// Not much we can do here
Expand All @@ -181,7 +120,7 @@
auto delagateQueue = mDelegateNotificationQueue;
if (strongDelegate == nil || delagateQueue == nil) {
LogErrorOnFailure(CHIP_ERROR_INCORRECT_STATE);
AsyncResponder::NotifyEventHandledWithError(CHIP_ERROR_INCORRECT_STATE);
AsyncResponder::NotifyEventHandled(CHIP_ERROR_INCORRECT_STATE);
return CHIP_ERROR_INCORRECT_STATE;
}

Expand All @@ -201,7 +140,6 @@
completionHandler:completionHandler];
}
});

return CHIP_NO_ERROR;
}

Expand All @@ -228,7 +166,7 @@
auto delagateQueue = mDelegateNotificationQueue;
if (strongDelegate == nil || delagateQueue == nil) {
LogErrorOnFailure(CHIP_ERROR_INCORRECT_STATE);
AsyncResponder::NotifyEventHandledWithError(CHIP_ERROR_INCORRECT_STATE);
AsyncResponder::NotifyEventHandled(CHIP_ERROR_INCORRECT_STATE);
return CHIP_ERROR_INCORRECT_STATE;
}

Expand All @@ -239,8 +177,6 @@
error:[MTRError errorForCHIPErrorCode:error]];
});
}

ResetState();
return CHIP_NO_ERROR;
}

Expand Down Expand Up @@ -268,7 +204,7 @@
assertChipStackLockedByCurrentThread();

if (data == nil) {
AsyncResponder::NotifyEventHandledWithError(CHIP_ERROR_INCORRECT_STATE);
AsyncResponder::NotifyEventHandled(CHIP_ERROR_INCORRECT_STATE);
return;
}

Expand All @@ -278,10 +214,8 @@
blockData.IsEof = isEOF;

CHIP_ERROR err = mTransfer.PrepareBlock(blockData);
if (CHIP_NO_ERROR != err) {
LogErrorOnFailure(err);
}
AsyncResponder::NotifyEventHandledWithError(err);
LogErrorOnFailure(err);
AsyncResponder::NotifyEventHandled(err);
}
errorHandler:^(NSError *) {
// Not much we can do here
Expand All @@ -296,7 +230,7 @@
auto delagateQueue = mDelegateNotificationQueue;
if (strongDelegate == nil || delagateQueue == nil) {
LogErrorOnFailure(CHIP_ERROR_INCORRECT_STATE);
AsyncResponder::NotifyEventHandledWithError(CHIP_ERROR_INCORRECT_STATE);
AsyncResponder::NotifyEventHandled(CHIP_ERROR_INCORRECT_STATE);
return CHIP_ERROR_INCORRECT_STATE;
}

Expand All @@ -318,11 +252,10 @@
completionHandler:completionHandler];
}
});

return CHIP_NO_ERROR;
}

void MTROTAImageTransferHandler::HandleAsyncTransferSessionOutput(TransferSession::OutputEvent & event)
void MTROTAImageTransferHandler::HandleTransferSessionOutput(TransferSession::OutputEvent & event)
{
VerifyOrReturn(mDelegate != nil);

Expand All @@ -332,8 +265,7 @@
err = OnTransferSessionBegin(event);
if (err != CHIP_NO_ERROR) {
LogErrorOnFailure(err);
AsyncResponder::NotifyEventHandledWithError(err);
ResetState();
AsyncResponder::NotifyEventHandled(err);
}
break;
case TransferSession::OutputEventType::kStatusReceived:
Expand All @@ -343,27 +275,13 @@
case TransferSession::OutputEventType::kInternalError:
case TransferSession::OutputEventType::kTransferTimeout:
err = OnTransferSessionEnd(event);
if (err != CHIP_NO_ERROR) {
LogErrorOnFailure(err);
AsyncResponder::NotifyEventHandledWithError(err);
ResetState();
}
break;
case TransferSession::OutputEventType::kQueryWithSkipReceived:
case TransferSession::OutputEventType::kQueryReceived:
err = OnBlockQuery(event);
if (err != CHIP_NO_ERROR) {
LogErrorOnFailure(err);
AsyncResponder::NotifyEventHandledWithError(err);
ResetState();
}
break;
case TransferSession::OutputEventType::kMsgToSend:
err = OnMessageToSend(event);
if (err != CHIP_NO_ERROR) {
LogErrorOnFailure(err);
AsyncResponder::NotifyEventHandledWithError(err);
ResetState();
AsyncResponder::NotifyEventHandled(err);
}
break;
case TransferSession::OutputEventType::kNone:
Expand All @@ -377,7 +295,13 @@
chipDie();
break;
}
LogErrorOnFailure(err);
}

void MTROTAImageTransferHandler::CleanUp()
{
assertChipStackLockedByCurrentThread();

delete this;
}

CHIP_ERROR MTROTAImageTransferHandler::ConfigureState(chip::FabricIndex fabricIndex, chip::NodeId nodeId)
Expand All @@ -394,25 +318,18 @@
VerifyOrReturnError(mDelegate != nil, CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mDelegateNotificationQueue != nil, CHIP_ERROR_INCORRECT_STATE);

// Start a timer to track whether we receive a BDX init after a successful query image in a reasonable amount of time
CHIP_ERROR err
= chip::DeviceLayer::SystemLayer().StartTimer(kBdxInitReceivedTimeout, HandleBdxInitReceivedTimeoutExpired, this);
LogErrorOnFailure(err);

ReturnErrorOnFailure(err);

mFabricIndex.SetValue(fabricIndex);
mNodeId.SetValue(nodeId);

return CHIP_NO_ERROR;
}

CHIP_ERROR MTROTAImageTransferHandler::OnMessageReceived(
chip::Messaging::ExchangeContext * ec, const chip::PayloadHeader & payloadHeader, chip::System::PacketBufferHandle && payload)
Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader, System::PacketBufferHandle && payload)
{
VerifyOrReturnError(ec != nullptr, CHIP_ERROR_INCORRECT_STATE);
CHIP_ERROR err;
ChipLogProgress(BDX, "%s: message " ChipLogFormatMessageType " protocol " ChipLogFormatProtocolId, __FUNCTION__,
ChipLogProgress(BDX, "OnMessageReceived: message " ChipLogFormatMessageType " protocol " ChipLogFormatProtocolId,
payloadHeader.GetMessageType(), ChipLogValueProtocolId(payloadHeader.GetProtocolID()));

// If we receive a ReceiveInit message, then we prepare for transfer
Expand All @@ -424,6 +341,7 @@
err = PrepareForTransfer(ec, fabricIndex, nodeId);
if (err != CHIP_NO_ERROR) {
ChipLogError(Controller, "Failed to prepare for transfer for BDX");
return err;
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
{
Clusters::OTAProvider::SetDelegate(kOtaProviderEndpoint, nullptr);

delete sOtaUnsolicitedBDXMsgHandler;
sOtaUnsolicitedBDXMsgHandler = nullptr;
}

Expand Down
14 changes: 6 additions & 8 deletions src/darwin/Framework/CHIP/MTROTAUnsolicitedBDXMessageHandler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@
NS_ASSUME_NONNULL_BEGIN

/**
* This class creates a handler for listening to all unsolicited BDX messages and when a BDX ReceiveInit
* message is received from a node, it creates a new MTROTAImageTransferHandler object as a delegate
* that handles the OTA image file transfer for that node.
* 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.
*
* Each MTROTAImageTransferHandler instance will handle one BDX transfer session.
*/
class MTROTAUnsolicitedBDXMessageHandler : public chip::Messaging::UnsolicitedMessageHandler
{
Expand All @@ -39,10 +39,10 @@ class MTROTAUnsolicitedBDXMessageHandler : public chip::Messaging::UnsolicitedMe
// Returns the number of delegates that are currently handling BDX transfers
static uint8_t GetNumberOfDelegates();

// Increase the number of delegates by 1.
// Increase the number of delegates handling BDX transfers by 1.
static void IncrementNumberOfDelegates();

// Decrease the number of delegates by 1.
// Decrease the number of delegates handling BDX transfers by 1.
static void DecrementNumberOfDelegates();

private:
Expand All @@ -52,8 +52,6 @@ class MTROTAUnsolicitedBDXMessageHandler : public chip::Messaging::UnsolicitedMe
protected:
chip::Messaging::ExchangeManager * mExchangeMgr;

MTROTAImageTransferHandler * otaImageTransferHandler;

static inline uint8_t mNumberOfDelegates = 0;
};

Expand Down
Loading

0 comments on commit bfba54f

Please sign in to comment.