Skip to content

Commit

Permalink
Addresssed review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@nivi-apple
nivi-apple committed Oct 8, 2024
1 parent 3c0ca7c commit 91eed4b
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 53 deletions.
2 changes: 1 addition & 1 deletion src/darwin/Framework/CHIP/MTROTAImageTransferHandler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions src/darwin/Framework/CHIP/MTROTAImageTransferHandler.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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()
Expand Down Expand Up @@ -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;
Expand Down
53 changes: 29 additions & 24 deletions src/protocols/bdx/AsyncTransferFacilitator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -88,7 +101,7 @@ void AsyncTransferFacilitator::HandleNextOutputEvents()
}
mTransfer.GetNextAction(outEvent);
}
mHandlingOutputEvents = false;
mProcessingOutputEvents = false;
}

CHIP_ERROR AsyncTransferFacilitator::SendMessage(const TransferSession::MessageTypeData msgTypeData,
Expand Down Expand Up @@ -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))
Expand All @@ -146,7 +159,7 @@ CHIP_ERROR AsyncTransferFacilitator::OnMessageReceived(Messaging::ExchangeContex
ec->WillSendMessage();
}

HandleNextOutputEvents();
ProcessOutputEvents();
return err;
}

Expand All @@ -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.
Expand All @@ -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
Expand Down
58 changes: 33 additions & 25 deletions src/protocols/bdx/AsyncTransferFacilitator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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;

Expand All @@ -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.
Expand All @@ -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
Expand Down

0 comments on commit 91eed4b

Please sign in to comment.