Skip to content

Commit 1069138

Browse files
bzbarsky-applepull[bot]
bzbarsky-apple
authored and
pull[bot]
committed
Implement the server side of timed invoke. (#12389)
1 parent 1fd605f commit 1069138

22 files changed

+690
-30
lines changed

‎.github/workflows/examples-nrfconnect.yaml

+8
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,14 @@ jobs:
148148
run: |
149149
scripts/run_in_build_env.sh "scripts/tests/nrfconnect_native_posix_tests.sh native_posix_64"
150150
151+
- name: Uploading Failed Test Logs
152+
uses: actions/upload-artifact@v2
153+
if: ${{ failure() }} && ${{ !env.ACT }}
154+
with:
155+
name: test-log
156+
path: |
157+
src/test_driver/nrfconnect/build/Testing/Temporary/LastTest.log
158+
151159
- name: Uploading Size Reports
152160
uses: actions/upload-artifact@v2
153161
if: ${{ !env.ACT }}

‎examples/all-clusters-app/ameba/chip_main.cmake

+1
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ list(
2323
${chip_dir}/src/app/EventManagement.cpp
2424
${chip_dir}/src/app/ReadClient.cpp
2525
${chip_dir}/src/app/ReadHandler.cpp
26+
${chip_dir}/src/app/TimedHandler.cpp
2627
${chip_dir}/src/app/WriteClient.cpp
2728
${chip_dir}/src/app/WriteHandler.cpp
2829
${chip_dir}/src/app/util/CHIPDeviceCallbacksMgr.cpp

‎examples/lighting-app/ameba/chip_main.cmake

+1
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ list(
2323
${chip_dir}/src/app/EventManagement.cpp
2424
${chip_dir}/src/app/ReadClient.cpp
2525
${chip_dir}/src/app/ReadHandler.cpp
26+
${chip_dir}/src/app/TimedHandler.cpp
2627
${chip_dir}/src/app/WriteClient.cpp
2728
${chip_dir}/src/app/WriteHandler.cpp
2829
${chip_dir}/src/app/util/CHIPDeviceCallbacksMgr.cpp

‎scripts/tests/nrfconnect_native_posix_tests.sh

+1-1
Original file line numberDiff line numberDiff line change
@@ -27,4 +27,4 @@ env
2727
cd "$CHIP_ROOT/src/test_driver/nrfconnect" &&
2828
west build -b "$BOARD" &&
2929
cd build &&
30-
timeout 5m ctest -V
30+
timeout 5m ctest -V --output-on-failure

‎src/app/BUILD.gn

+2
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,8 @@ static_library("app") {
120120
"ReadHandler.cpp",
121121
"StatusResponse.cpp",
122122
"StatusResponse.h",
123+
"TimedHandler.cpp",
124+
"TimedHandler.h",
123125
"WriteClient.cpp",
124126
"WriteHandler.cpp",
125127
"decoder.cpp",

‎src/app/CommandHandler.cpp

+30-3
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ CHIP_ERROR CommandHandler::AllocateBuffer()
6363
}
6464

6565
CHIP_ERROR CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader,
66-
System::PacketBufferHandle && payload)
66+
System::PacketBufferHandle && payload, bool isTimedInvoke)
6767
{
6868
System::PacketBufferHandle response;
6969
VerifyOrReturnError(mState == CommandState::Idle, CHIP_ERROR_INCORRECT_STATE);
@@ -73,14 +73,18 @@ CHIP_ERROR CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * e
7373
mpExchangeCtx = ec;
7474

7575
// Use the RAII feature, if this is the only Handle when this function returns, DecrementHoldOff will trigger sending response.
76+
// TODO: This is broken! If something under here returns error, we will try
77+
// to SendCommandResponse(), and then our caller will try to send a status
78+
// response too. Figure out at what point it's our responsibility to
79+
// handler errors vs our caller's.
7680
Handle workHandle(this);
7781
mpExchangeCtx->WillSendMessage();
78-
ReturnErrorOnFailure(ProcessInvokeRequest(std::move(payload)));
82+
ReturnErrorOnFailure(ProcessInvokeRequest(std::move(payload), isTimedInvoke));
7983

8084
return CHIP_NO_ERROR;
8185
}
8286

83-
CHIP_ERROR CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payload)
87+
CHIP_ERROR CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payload, bool isTimedInvoke)
8488
{
8589
CHIP_ERROR err = CHIP_NO_ERROR;
8690
System::PacketBufferTLVReader reader;
@@ -97,6 +101,29 @@ CHIP_ERROR CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && pa
97101
ReturnErrorOnFailure(invokeRequestMessage.GetTimedRequest(&mTimedRequest));
98102
ReturnErrorOnFailure(invokeRequestMessage.GetInvokeRequests(&invokeRequests));
99103

104+
if (mTimedRequest != isTimedInvoke)
105+
{
106+
// The message thinks it should be part of a timed interaction but it's
107+
// not, or vice versa. Spec says to Respond with UNSUPPORTED_ACCESS.
108+
err = StatusResponse::SendStatusResponse(Protocols::InteractionModel::Status::UnsupportedAccess, mpExchangeCtx,
109+
/* aExpectResponse = */ false);
110+
111+
// Some unit tests call this function in an abnormal state when we don't
112+
// even have an exchange.
113+
if (err != CHIP_NO_ERROR && mpExchangeCtx)
114+
{
115+
// We have to manually close the exchange, because we called
116+
// WillSendMessage already.
117+
mpExchangeCtx->Close();
118+
}
119+
120+
// Null out the (now-closed) exchange, so that when we try to
121+
// SendCommandResponse() later (when our holdoff count drops to 0) it
122+
// just fails and we don't double-respond.
123+
mpExchangeCtx = nullptr;
124+
return err;
125+
}
126+
100127
invokeRequests.GetReader(&invokeRequestsReader);
101128
while (CHIP_NO_ERROR == (err = invokeRequestsReader.Next()))
102129
{

‎src/app/CommandHandler.h

+11-2
Original file line numberDiff line numberDiff line change
@@ -128,16 +128,20 @@ class CommandHandler : public Command
128128
*
129129
* This function will always call the OnDone function above on the registered callback
130130
* before returning.
131+
*
132+
* isTimedInvoke is true if and only if this is part of a Timed Invoke
133+
* transaction (i.e. was preceded by a Timed Request). If we reach here,
134+
* the timer verification has already been done.
131135
*/
132136
CHIP_ERROR OnInvokeCommandRequest(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader,
133-
System::PacketBufferHandle && payload);
137+
System::PacketBufferHandle && payload, bool isTimedInvoke);
134138
CHIP_ERROR AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus) override;
135139

136140
CHIP_ERROR AddClusterSpecificSuccess(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus) override;
137141

138142
CHIP_ERROR AddClusterSpecificFailure(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus) override;
139143

140-
CHIP_ERROR ProcessInvokeRequest(System::PacketBufferHandle && payload);
144+
CHIP_ERROR ProcessInvokeRequest(System::PacketBufferHandle && payload, bool isTimedInvoke);
141145
CHIP_ERROR PrepareCommand(const CommandPathParams & aCommandPathParams, bool aStartDataStruct = true);
142146
CHIP_ERROR FinishCommand(bool aStartDataStruct = true);
143147
CHIP_ERROR PrepareStatus(const CommandPathParams & aCommandPathParams);
@@ -166,6 +170,11 @@ class CommandHandler : public Command
166170
return FinishCommand(/* aEndDataStruct = */ false);
167171
}
168172

173+
/**
174+
* Check whether the InvokeRequest we are handling is a timed invoke.
175+
*/
176+
bool IsTimedInvoke() const { return mTimedRequest; }
177+
169178
private:
170179
friend class TestCommandInteraction;
171180
friend class CommandHandler::Handle;

‎src/app/InteractionModelEngine.cpp

+68-3
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,15 @@ void InteractionModelEngine::Shutdown()
9595
return true;
9696
});
9797

98+
mTimedHandlers.ForEachActiveObject([this](TimedHandler * obj) -> bool {
99+
// This calls back into us and deallocates |obj|. As above, this is not
100+
// really guaranteed, and we should do something better here (like
101+
// ignoring the calls to OnTimedInteractionFailed and then doing a
102+
// DeallocateAll.
103+
mpExchangeMgr->CloseAllContextsForDelegate(obj);
104+
return true;
105+
});
106+
98107
for (auto & readClient : mReadClients)
99108
{
100109
if (!readClient.IsFree())
@@ -277,7 +286,7 @@ void InteractionModelEngine::OnDone(CommandHandler & apCommandObj)
277286

278287
CHIP_ERROR InteractionModelEngine::OnInvokeCommandRequest(Messaging::ExchangeContext * apExchangeContext,
279288
const PayloadHeader & aPayloadHeader,
280-
System::PacketBufferHandle && aPayload,
289+
System::PacketBufferHandle && aPayload, bool aIsTimedInvoke,
281290
Protocols::InteractionModel::Status & aStatus)
282291
{
283292
CommandHandler * commandHandler = mCommandHandlerObjs.CreateObject(this);
@@ -287,7 +296,8 @@ CHIP_ERROR InteractionModelEngine::OnInvokeCommandRequest(Messaging::ExchangeCon
287296
aStatus = Protocols::InteractionModel::Status::Busy;
288297
return CHIP_ERROR_NO_MEMORY;
289298
}
290-
ReturnErrorOnFailure(commandHandler->OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload)));
299+
ReturnErrorOnFailure(
300+
commandHandler->OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), aIsTimedInvoke));
291301
aStatus = Protocols::InteractionModel::Status::Success;
292302
return CHIP_NO_ERROR;
293303
}
@@ -360,6 +370,25 @@ CHIP_ERROR InteractionModelEngine::OnWriteRequest(Messaging::ExchangeContext * a
360370
return CHIP_NO_ERROR;
361371
}
362372

373+
CHIP_ERROR InteractionModelEngine::OnTimedRequest(Messaging::ExchangeContext * apExchangeContext,
374+
const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload,
375+
Protocols::InteractionModel::Status & aStatus)
376+
{
377+
TimedHandler * handler = mTimedHandlers.CreateObject();
378+
if (handler == nullptr)
379+
{
380+
ChipLogProgress(InteractionModel, "no resource for Timed interaction");
381+
aStatus = Protocols::InteractionModel::Status::Busy;
382+
return CHIP_ERROR_NO_MEMORY;
383+
}
384+
385+
// The timed handler takes over handling of this exchange and will do its
386+
// own status reporting as needed.
387+
aStatus = Protocols::InteractionModel::Status::Success;
388+
apExchangeContext->SetDelegate(handler);
389+
return handler->OnMessageReceived(apExchangeContext, aPayloadHeader, std::move(aPayload));
390+
}
391+
363392
CHIP_ERROR InteractionModelEngine::OnUnsolicitedReportData(Messaging::ExchangeContext * apExchangeContext,
364393
const PayloadHeader & aPayloadHeader,
365394
System::PacketBufferHandle && aPayload)
@@ -392,12 +421,15 @@ CHIP_ERROR InteractionModelEngine::OnUnsolicitedReportData(Messaging::ExchangeCo
392421
CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext * apExchangeContext,
393422
const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload)
394423
{
424+
using namespace Protocols::InteractionModel;
425+
395426
CHIP_ERROR err = CHIP_NO_ERROR;
396427
Protocols::InteractionModel::Status status = Protocols::InteractionModel::Status::Failure;
397428

398429
if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::InvokeCommandRequest))
399430
{
400-
SuccessOrExit(OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), status));
431+
SuccessOrExit(
432+
OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), /* aIsTimedInvoke = */ false, status));
401433
}
402434
else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::ReadRequest))
403435
{
@@ -418,6 +450,10 @@ CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext
418450
ReturnErrorOnFailure(OnUnsolicitedReportData(apExchangeContext, aPayloadHeader, std::move(aPayload)));
419451
status = Protocols::InteractionModel::Status::Success;
420452
}
453+
else if (aPayloadHeader.HasMessageType(MsgType::TimedRequest))
454+
{
455+
SuccessOrExit(OnTimedRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), status));
456+
}
421457
else
422458
{
423459
ChipLogProgress(InteractionModel, "Msg type %d not supported", aPayloadHeader.GetMessageType());
@@ -550,6 +586,9 @@ void InteractionModelEngine::DispatchCommand(CommandHandler & apCommandObj, cons
550586

551587
if (handler)
552588
{
589+
// TODO: Figure out who is responsible for handling checking
590+
// apCommandObj->IsTimedInvoke() for commands that require a timed
591+
// invoke and have a CommandHandlerInterface handling them.
553592
CommandHandlerInterface::HandlerContext context(apCommandObj, aCommandPath, apPayload);
554593
handler->InvokeCommand(context);
555594

@@ -657,5 +696,31 @@ CommandHandlerInterface * InteractionModelEngine::FindCommandHandler(EndpointId
657696
return nullptr;
658697
}
659698

699+
void InteractionModelEngine::OnTimedInteractionFailed(TimedHandler * apTimedHandler)
700+
{
701+
mTimedHandlers.Deallocate(apTimedHandler);
702+
}
703+
704+
void InteractionModelEngine::OnTimedInvoke(TimedHandler * apTimedHandler, Messaging::ExchangeContext * apExchangeContext,
705+
const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload)
706+
{
707+
using namespace Protocols::InteractionModel;
708+
709+
// Reset the ourselves as the exchange delegate for now, to match what we'd
710+
// do with an initial unsolicited invoke.
711+
apExchangeContext->SetDelegate(this);
712+
mTimedHandlers.Deallocate(apTimedHandler);
713+
714+
VerifyOrDie(aPayloadHeader.HasMessageType(MsgType::InvokeCommandRequest));
715+
VerifyOrDie(!apExchangeContext->IsGroupExchangeContext());
716+
717+
Status status = Status::Failure;
718+
OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), /* aIsTimedInvoke = */ true, status);
719+
if (status != Status::Success)
720+
{
721+
StatusResponse::SendStatusResponse(status, apExchangeContext, /* aExpectResponse = */ false);
722+
}
723+
}
724+
660725
} // namespace app
661726
} // namespace chip

‎src/app/InteractionModelEngine.h

+26-1
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
#include <app/ReadClient.h>
4949
#include <app/ReadHandler.h>
5050
#include <app/StatusResponse.h>
51+
#include <app/TimedHandler.h>
5152
#include <app/WriteClient.h>
5253
#include <app/WriteHandler.h>
5354
#include <app/reporting/Engine.h>
@@ -194,6 +195,20 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman
194195
CommandHandlerInterface * FindCommandHandler(EndpointId endpointId, ClusterId clusterId);
195196
void UnregisterCommandHandlers(EndpointId endpointId);
196197

198+
/**
199+
* Called when a timed interaction has failed (i.e. the exchange it was
200+
* happening on has closed while the exchange delegate was the timed
201+
* handler).
202+
*/
203+
void OnTimedInteractionFailed(TimedHandler * apTimedHandler);
204+
205+
/**
206+
* Called when a timed invoke is received. This function takes over all
207+
* handling of the exchange, status reporting, and so forth.
208+
*/
209+
void OnTimedInvoke(TimedHandler * apTimedHandler, Messaging::ExchangeContext * apExchangeContext,
210+
const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload);
211+
197212
private:
198213
friend class reporting::Engine;
199214
friend class TestCommandInteraction;
@@ -206,7 +221,8 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman
206221
* expected to set it to success if it does not want an automatic status response message to be sent.
207222
*/
208223
CHIP_ERROR OnInvokeCommandRequest(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
209-
System::PacketBufferHandle && aPayload, Protocols::InteractionModel::Status & aStatus);
224+
System::PacketBufferHandle && aPayload, bool aIsTimedInvoke,
225+
Protocols::InteractionModel::Status & aStatus);
210226
CHIP_ERROR OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
211227
System::PacketBufferHandle && aPayload) override;
212228
void OnResponseTimeout(Messaging::ExchangeContext * ec) override;
@@ -229,6 +245,14 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman
229245
CHIP_ERROR OnWriteRequest(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
230246
System::PacketBufferHandle && aPayload, Protocols::InteractionModel::Status & aStatus);
231247

248+
/**
249+
* Called when Interaction Model receives a Timed Request message. Errors processing
250+
* the Timed Request are handled entirely within this function. The caller pre-sets status to failure and the callee is
251+
* expected to set it to success if it does not want an automatic status response message to be sent.
252+
*/
253+
CHIP_ERROR OnTimedRequest(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
254+
System::PacketBufferHandle && aPayload, Protocols::InteractionModel::Status & aStatus);
255+
232256
/**This function handles processing of un-solicited ReportData messages on the client, which can
233257
* only occur post subscription establishment
234258
*/
@@ -247,6 +271,7 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman
247271
// TODO(#8006): investgate if we can disable some IM functions on some compact accessories.
248272
// TODO(#8006): investgate if we can provide more flexible object management on devices with more resources.
249273
BitMapObjectPool<CommandHandler, CHIP_IM_MAX_NUM_COMMAND_HANDLER> mCommandHandlerObjs;
274+
BitMapObjectPool<TimedHandler, CHIP_IM_MAX_NUM_TIMED_HANDLER> mTimedHandlers;
250275
ReadClient mReadClients[CHIP_IM_MAX_NUM_READ_CLIENT];
251276
ReadHandler mReadHandlers[CHIP_IM_MAX_NUM_READ_HANDLER];
252277
WriteClient mWriteClients[CHIP_IM_MAX_NUM_WRITE_CLIENT];

‎src/app/MessageDef/InvokeRequestMessage.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ CHIP_ERROR InvokeRequestMessage::Parser::GetSuppressResponse(bool * const apSupp
119119

120120
CHIP_ERROR InvokeRequestMessage::Parser::GetTimedRequest(bool * const apTimedRequest) const
121121
{
122-
return GetSimpleValue(to_underlying(Tag::kSuppressResponse), TLV::kTLVType_Boolean, apTimedRequest);
122+
return GetSimpleValue(to_underlying(Tag::kTimedRequest), TLV::kTLVType_Boolean, apTimedRequest);
123123
}
124124

125125
CHIP_ERROR InvokeRequestMessage::Parser::GetInvokeRequests(InvokeRequests::Parser * const apInvokeRequests) const

0 commit comments

Comments
 (0)