Skip to content

Commit

Permalink
[IM] Move ownership of WriteClient to application
Browse files Browse the repository at this point in the history
  • Loading branch information
@erjiaqing
erjiaqing committed Jan 19, 2022
1 parent 150c479 commit b8e59c4
Show file tree
Hide file tree
Showing 10 changed files with 146 additions and 287 deletions.
20 changes: 0 additions & 20 deletions src/app/DeviceProxy.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,24 +68,4 @@ void DeviceProxy::CancelIMResponseHandler(void * commandObj)
mCallbacksMgr.CancelResponseCallback(transactionId, 0 /* seqNum, always 0 for IM before #6559 */);
}

CHIP_ERROR DeviceProxy::SendWriteAttributeRequest(app::WriteClientHandle aHandle, Callback::Cancelable * onSuccessCallback,
Callback::Cancelable * onFailureCallback)
{
VerifyOrReturnLogError(IsSecureConnected(), CHIP_ERROR_INCORRECT_STATE);

CHIP_ERROR err = CHIP_NO_ERROR;

app::WriteClient * writeClient = aHandle.Get();

if (onSuccessCallback != nullptr || onFailureCallback != nullptr)
{
AddIMResponseHandler(writeClient, onSuccessCallback, onFailureCallback);
}
if ((err = aHandle.SendWriteRequest(GetSecureSession().Value())) != CHIP_NO_ERROR)
{
CancelIMResponseHandler(writeClient);
}
return err;
}

} // namespace chip
3 changes: 0 additions & 3 deletions src/app/DeviceProxy.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,6 @@ class DLL_EXPORT DeviceProxy

virtual CHIP_ERROR ShutdownSubscriptions() { return CHIP_ERROR_NOT_IMPLEMENTED; }

virtual CHIP_ERROR SendWriteAttributeRequest(app::WriteClientHandle aHandle, Callback::Cancelable * onSuccessCallback,
Callback::Cancelable * onFailureCallback);

virtual CHIP_ERROR SendCommands(app::CommandSender * commandObj);

// Interaction model uses the object and callback interface instead of sequence number to mark different transactions.
Expand Down
47 changes: 0 additions & 47 deletions src/app/InteractionModelEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,14 +112,6 @@ void InteractionModelEngine::Shutdown()
//
mpActiveReadClientList = nullptr;

for (auto & writeClient : mWriteClients)
{
if (!writeClient.IsFree())
{
writeClient.Shutdown();
}
}

for (auto & writeHandler : mWriteHandlers)
{
VerifyOrDie(writeHandler.IsFree());
Expand Down Expand Up @@ -147,21 +139,6 @@ uint32_t InteractionModelEngine::GetNumActiveReadHandlers() const
return numActive;
}

uint32_t InteractionModelEngine::GetNumActiveWriteClients() const
{
uint32_t numActive = 0;

for (auto & writeClient : mWriteClients)
{
if (!writeClient.IsFree())
{
numActive++;
}
}

return numActive;
}

uint32_t InteractionModelEngine::GetNumActiveWriteHandlers() const
{
uint32_t numActive = 0;
Expand Down Expand Up @@ -205,25 +182,6 @@ CHIP_ERROR InteractionModelEngine::ShutdownSubscriptions(FabricIndex aFabricInde
return CHIP_NO_ERROR;
}

CHIP_ERROR InteractionModelEngine::NewWriteClient(WriteClientHandle & apWriteClient, WriteClient::Callback * apCallback,
const Optional<uint16_t> & aTimedWriteTimeoutMs)
{
apWriteClient.SetWriteClient(nullptr);

for (auto & writeClient : mWriteClients)
{
if (!writeClient.IsFree())
{
continue;
}
ReturnLogErrorOnFailure(writeClient.Init(mpExchangeMgr, apCallback, aTimedWriteTimeoutMs));
apWriteClient.SetWriteClient(&writeClient);
return CHIP_NO_ERROR;
}

return CHIP_ERROR_NO_MEMORY;
}

void InteractionModelEngine::OnDone(CommandHandler & apCommandObj)
{
mCommandHandlerObjs.ReleaseObject(&apCommandObj);
Expand Down Expand Up @@ -428,11 +386,6 @@ void InteractionModelEngine::OnResponseTimeout(Messaging::ExchangeContext * ec)
ChipLogValueExchange(ec));
}

uint16_t InteractionModelEngine::GetWriteClientArrayIndex(const WriteClient * const apWriteClient) const
{
return static_cast<uint16_t>(apWriteClient - mWriteClients);
}

uint16_t InteractionModelEngine::GetReadHandlerArrayIndex(const ReadHandler * const apReadHandler) const
{
return static_cast<uint16_t>(apReadHandler - mReadHandlers);
Expand Down
22 changes: 0 additions & 22 deletions src/app/InteractionModelEngine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,28 +111,9 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman
*/
CHIP_ERROR ShutdownSubscriptions(FabricIndex aFabricIndex, NodeId aPeerNodeId);

/**
* Retrieve a WriteClient that the SDK consumer can use to send a write. If the call succeeds,
* see WriteClient documentation for lifetime handling.
*
* The Write interaction is more like Invoke interaction (cluster specific commands) since it will include cluster specific
* payload, and may have the need to encode non-scalar values (like structs and arrays). Thus we use WriteClientHandle to
* prevent user's code from leaking WriteClients.
*
* @param[out] apWriteClient A pointer to the WriteClient object.
*
* @retval #CHIP_ERROR_NO_MEMORY If there is no WriteClient available
* @retval #CHIP_NO_ERROR On success.
*/
CHIP_ERROR NewWriteClient(WriteClientHandle & apWriteClient, WriteClient::Callback * callback,
const Optional<uint16_t> & aTimedWriteTimeoutMs = NullOptional);

uint32_t GetNumActiveReadHandlers() const;

uint32_t GetNumActiveWriteHandlers() const;
uint32_t GetNumActiveWriteClients() const;

uint16_t GetWriteClientArrayIndex(const WriteClient * const apWriteClient) const;

uint16_t GetReadHandlerArrayIndex(const ReadHandler * const apReadHandler) const;
/**
Expand Down Expand Up @@ -256,12 +237,9 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman

CommandHandlerInterface * mCommandHandlerList = nullptr;

// TODO(#8006): investgate if we can disable some IM functions on some compact accessories.
// TODO(#8006): investgate if we can provide more flexible object management on devices with more resources.
BitMapObjectPool<CommandHandler, CHIP_IM_MAX_NUM_COMMAND_HANDLER> mCommandHandlerObjs;
BitMapObjectPool<TimedHandler, CHIP_IM_MAX_NUM_TIMED_HANDLER> mTimedHandlers;
ReadHandler mReadHandlers[CHIP_IM_MAX_NUM_READ_HANDLER];
WriteClient mWriteClients[CHIP_IM_MAX_NUM_WRITE_CLIENT];
WriteHandler mWriteHandlers[CHIP_IM_MAX_NUM_WRITE_HANDLER];
reporting::Engine mReportingEngine;
BitMapObjectPool<ClusterInfo, CHIP_IM_SERVER_MAX_NUM_PATH_GROUPS> mClusterInfoPool;
Expand Down
81 changes: 39 additions & 42 deletions src/app/WriteClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ CHIP_ERROR WriteClient::Init(Messaging::ExchangeManager * apExchangeMgr, Callbac
ReturnErrorOnFailure(mWriteRequestBuilder.GetError());
mWriteRequestBuilder.CreateWriteRequests();
ReturnErrorOnFailure(mWriteRequestBuilder.GetError());
ClearExistingExchangeContext();

mpExchangeMgr = apExchangeMgr;
mpCallback = apCallback;
mTimedWriteTimeoutMs = aTimedWriteTimeoutMs;
Expand All @@ -57,30 +57,45 @@ CHIP_ERROR WriteClient::Init(Messaging::ExchangeManager * apExchangeMgr, Callbac
return CHIP_NO_ERROR;
}

void WriteClient::Shutdown()
void WriteClient::Close()
{
VerifyOrReturn(mState != State::Uninitialized);
ClearExistingExchangeContext();
ShutdownInternal();
}
MoveToState(State::AwaitingDestruction);

void WriteClient::ShutdownInternal()
{
mMessageWriter.Reset();
// OnDone below can destroy us before we unwind all the way back into the
// exchange code and it tries to close itself. Make sure that it doesn't
// try to notify us that it's closing, since we will be dead.
//
// For more details, see #10344.
if (mpExchangeCtx != nullptr)
{
mpExchangeCtx->SetDelegate(nullptr);
}

mpExchangeMgr = nullptr;
mpExchangeCtx = nullptr;
ClearState();

mpCallback->OnDone(this);
if (mpCallback)
{
mpCallback->OnDone(this);
}
}

void WriteClient::ClearExistingExchangeContext()
void WriteClient::Abort()
{
// Discard any existing exchange context. Effectively we can only have one IM exchange with
// a single node at any one time.
//
// If the exchange context hasn't already been gracefully closed
// (signaled by setting it to null), then we need to forcibly
// tear it down.
//
if (mpExchangeCtx != nullptr)
{
// We might be a delegate for this exchange, and we don't want the
// OnExchangeClosing notification in that case. Null out the delegate
// to avoid that.
//
// TODO: This makes all sorts of assumptions about what the delegate is
// (notice the "might" above!) that might not hold in practice. We
// really need a better solution here....
mpExchangeCtx->SetDelegate(nullptr);
mpExchangeCtx->Abort();
mpExchangeCtx = nullptr;
}
Expand Down Expand Up @@ -195,6 +210,9 @@ const char * WriteClient::GetStateStr() const

case State::ResponseReceived:
return "ResponseReceived";

case State::AwaitingDestruction:
return "AwaitingDestruction";
}
#endif // CHIP_DETAIL_LOGGING
return "N/A";
Expand All @@ -220,10 +238,6 @@ CHIP_ERROR WriteClient::SendWriteRequest(const SessionHandle & session, System::
err = FinalizeMessage(mPendingWriteData);
SuccessOrExit(err);

// Discard any existing exchange context. Effectively we can only have one exchange per WriteClient
// at any one time.
ClearExistingExchangeContext();

// Create a new exchange context.
mpExchangeCtx = mpExchangeMgr->NewContext(session, this);
VerifyOrExit(mpExchangeCtx != nullptr, err = CHIP_ERROR_NO_MEMORY);
Expand All @@ -246,7 +260,6 @@ CHIP_ERROR WriteClient::SendWriteRequest(const SessionHandle & session, System::
if (err != CHIP_NO_ERROR)
{
ChipLogError(DataManagement, "Write client failed to SendWriteRequest: %s", ErrorStr(err));
ClearExistingExchangeContext();
}
else
{
Expand All @@ -259,8 +272,10 @@ CHIP_ERROR WriteClient::SendWriteRequest(const SessionHandle & session, System::
// Always shutdown on Group communication
ChipLogDetail(DataManagement, "Closing on group Communication ");

// onDone is called
ShutdownInternal();
// Tell the application to release the object.
// TODO: The typical user would release it reference to the write client after SendWriteRequest is returned with
// success. Destruct the WriteClient before releasing that reference is weird, need to refactor the code to avoid this.
Close();
}
}

Expand Down Expand Up @@ -329,7 +344,7 @@ CHIP_ERROR WriteClient::OnMessageReceived(Messaging::ExchangeContext * apExchang

if (mState != State::AwaitingResponse)
{
ShutdownInternal();
Close();
}
// Else we got a response to a Timed Request and just sent the write.

Expand All @@ -345,7 +360,7 @@ void WriteClient::OnResponseTimeout(Messaging::ExchangeContext * apExchangeConte
{
mpCallback->OnError(this, StatusIB(Protocols::InteractionModel::Status::Failure), CHIP_ERROR_TIMEOUT);
}
ShutdownInternal();
Close();
}

CHIP_ERROR WriteClient::ProcessAttributeStatusIB(AttributeStatusIB::Parser & aAttributeStatusIB)
Expand Down Expand Up @@ -391,24 +406,6 @@ CHIP_ERROR WriteClient::ProcessAttributeStatusIB(AttributeStatusIB::Parser & aAt
return err;
}

CHIP_ERROR WriteClientHandle::SendWriteRequest(const SessionHandle & session, System::Clock::Timeout timeout)
{
CHIP_ERROR err = mpWriteClient->SendWriteRequest(session, timeout);

// Transferring ownership of the underlying WriteClient to the IM layer. IM will manage its lifetime.
// For groupcast writes, there is no transfer of ownership since the interaction is done upon transmission of the action
if (err == CHIP_NO_ERROR)
{
// Release the WriteClient without closing it.
mpWriteClient = nullptr;
}
else
{
SetWriteClient(nullptr);
}
return err;
}

CHIP_ERROR WriteClient::HandleTimedStatus(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload,
StatusIB & aStatusIB)
{
Expand Down
Loading

0 comments on commit b8e59c4

Please sign in to comment.