Skip to content

Commit 8bc5129

Browse files
bzbarsky-applebzbarsky-apple
authored
Switch Interaction Model client error reporting to just CHIP_ERROR. (project-chip#13888)
* Switch Interaction Model client error reporting to just CHIP_ERROR. Now that we can put a StatusIB inside a CHIP_ERROR, we can stop passing both, or passing just EmberAfStatus, and pass a CHIP_ERROR that either contains a StatusIB or contains the actual client-side error we ran into (e.g. failure to decode). * Address review comments.
1 parent 68923b4 commit 8bc5129

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

51 files changed

+11499
-5656
lines changed

‎examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ static void BoundDeviceChangedHandler(const EmberBindingTableEntry * binding, ch
8282
auto onSuccess = [](const ConcreteCommandPath & commandPath, const StatusIB & status, const auto & dataResponse) {
8383
ChipLogProgress(NotSpecified, "OnOff command succeeds");
8484
};
85-
auto onFailure = [](const StatusIB & status, CHIP_ERROR error) {
85+
auto onFailure = [](CHIP_ERROR error) {
8686
ChipLogError(NotSpecified, "OnOff command failed: %" CHIP_ERROR_FORMAT, error.Format());
8787
};
8888

‎examples/chip-tool/commands/common/CommandInvoker.h

+4-7
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ class ResponseReceiver : public app::CommandSender::Callback
3333
{
3434
public:
3535
using SuccessCallback = void (*)(void * context, const ResponseType & data);
36-
using FailureCallback = void (*)(void * context, EmberAfStatus status);
36+
using FailureCallback = void (*)(void * context, CHIP_ERROR err);
3737
using DoneCallback = void (*)(void * context);
3838

3939
virtual ~ResponseReceiver() {}
@@ -46,10 +46,7 @@ class ResponseReceiver : public app::CommandSender::Callback
4646
inline void OnResponse(app::CommandSender * aCommandSender, const app::ConcreteCommandPath & aPath,
4747
const app::StatusIB & aStatus, TLV::TLVReader * aData) override;
4848

49-
void OnError(const app::CommandSender * aCommandSender, const app::StatusIB & aStatus, CHIP_ERROR aError) override
50-
{
51-
mOnError(mContext, app::ToEmberAfStatus(aStatus.mStatus));
52-
}
49+
void OnError(const app::CommandSender * aCommandSender, CHIP_ERROR aError) override { mOnError(mContext, aError); }
5350

5451
void OnDone(app::CommandSender * aCommandSender) override
5552
{
@@ -164,7 +161,7 @@ void ResponseReceiver<ResponseType>::OnResponse(app::CommandSender * aCommandSen
164161
exit:
165162
if (err != CHIP_NO_ERROR)
166163
{
167-
mOnError(mContext, app::ToEmberAfStatus(Protocols::InteractionModel::Status::Failure));
164+
mOnError(mContext, err);
168165
}
169166
}
170167

@@ -178,7 +175,7 @@ inline void ResponseReceiver<app::DataModel::NullObjectType>::OnResponse(app::Co
178175
//
179176
if (aData != nullptr)
180177
{
181-
mOnError(mContext, app::ToEmberAfStatus(Protocols::InteractionModel::Status::Failure));
178+
mOnError(mContext, CHIP_ERROR_SCHEMA_MISMATCH);
182179
return;
183180
}
184181

‎examples/chip-tool/templates/commands.zapt

+2-2
Original file line numberDiff line numberDiff line change
@@ -203,9 +203,9 @@ static void OnDefaultSuccessResponse(void * context)
203203
command->SetCommandExitStatus(CHIP_NO_ERROR);
204204
}
205205

206-
static void OnDefaultFailure(void * context, EmberAfStatus status)
206+
static void OnDefaultFailure(void * context, CHIP_ERROR err)
207207
{
208-
ChipLogProgress(chipTool, "Default Failure Response: 0x%02x", chip::to_underlying(status));
208+
ChipLogProgress(chipTool, "Default Failure Response: %" CHIP_ERROR_FORMAT, err.Format());
209209

210210
ModelCommand * command = static_cast<ModelCommand *>(context);
211211
command->SetCommandExitStatus(CHIP_ERROR_INTERNAL);

‎examples/chip-tool/templates/partials/test_cluster.zapt

+9-7
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,8 @@ class {{filename}}: public TestCommand
131131
{{#*inline "staticDoneResponse"}}OnDoneCallback_{{index}}{{/inline}}
132132

133133
{{#*inline "successArguments"}}{{#chip_tests_item_response_parameters}}{{#first}}{{#if ../leadingComma}}, {{/if}}{{/first}} {{zapTypeToDecodableClusterObjectType type ns=parent.cluster isArgument=true}} {{asLowerCamelCase name}}{{#not_last}}, {{/not_last}}{{/chip_tests_item_response_parameters}}{{/inline}}
134-
{{#*inline "failureArguments"}}{{#if leadingComma}}, {{/if}}EmberAfStatus status{{/inline}}
134+
{{! TODO: Temporary if cascade until everything is converted to the new status setup }}
135+
{{#*inline "failureArguments"}}{{#if leadingComma}}, {{/if}}CHIP_ERROR error{{/inline}}
135136
{{#*inline "staticSuccessArguments"}}void * context{{> successArguments leadingComma=true}}{{/inline}}
136137
{{#*inline "staticFailureArguments"}}void * context{{> failureArguments leadingComma=true}}{{/inline}}
137138
{{#*inline "staticDoneArguments"}}void * context{{/inline}}
@@ -155,7 +156,7 @@ class {{filename}}: public TestCommand
155156

156157
static void {{>staticFailureResponse}}({{>staticFailureArguments}})
157158
{
158-
(static_cast<{{filename}} *>(context))->{{>failureResponse}}(status);
159+
(static_cast<{{filename}} *>(context))->{{>failureResponse}}(error);
159160
}
160161

161162
static void {{>staticSuccessResponse}}({{> staticSuccessArguments}})
@@ -247,8 +248,8 @@ class {{filename}}: public TestCommand
247248
(static_cast<{{filename}} *>(context))->{{>successResponse}}({{#chip_tests_item_response_parameters}}{{#not_first}}, {{/not_first}}data.{{asLowerCamelCase name}}{{/chip_tests_item_response_parameters}});
248249
};
249250

250-
auto failure = [](void * context, EmberAfStatus status) {
251-
(static_cast<{{filename}} *>(context))->{{>failureResponse}}(status);
251+
auto failure = [](void * context, CHIP_ERROR error) {
252+
(static_cast<{{filename}} *>(context))->{{>failureResponse}}(error);
252253
};
253254

254255
{{#if isGroupCommand}}
@@ -305,14 +306,15 @@ class {{filename}}: public TestCommand
305306

306307
void {{>failureResponse}}({{>failureArguments}})
307308
{
309+
chip::app::StatusIB status(error);
308310
{{#if response.error}}
309-
VerifyOrReturn(CheckValue("status", status, {{response.error}}));
311+
VerifyOrReturn(CheckValue("status", chip::to_underlying(status.mStatus), {{response.error}}));
310312
{{#unless async}}NextTest();{{/unless}}
311313
{{else if response.errorWrongValue}}
312-
VerifyOrReturn(CheckConstraintNotValue("status", status, 0));
314+
VerifyOrReturn(CheckConstraintNotValue("status", chip::to_underlying(status.mStatus), 0));
313315
{{#unless async}}NextTest();{{/unless}}
314316
{{else}}
315-
{{#if optional}}(status == EMBER_ZCL_STATUS_UNSUPPORTED_ATTRIBUTE) ? NextTest() : {{/if}}ThrowFailureResponse();
317+
{{#if optional}}(status.mStatus == chip::Protocols::InteractionModel::Status::UnsupportedAttribute) ? NextTest() : {{/if}}ThrowFailureResponse();
316318
{{/if}}
317319
}
318320

‎examples/tv-casting-app/linux/main.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -204,9 +204,9 @@ void OnContentLauncherSuccessResponse(void * context, const LaunchResponse::Deco
204204
ChipLogProgress(AppServer, "ContentLauncher: Default Success Response");
205205
}
206206

207-
void OnContentLauncherFailureResponse(void * context, EmberAfStatus status)
207+
void OnContentLauncherFailureResponse(void * context, CHIP_ERROR error)
208208
{
209-
ChipLogError(AppServer, "ContentLauncher: Default Failure Response: %" PRIu8, status);
209+
ChipLogError(AppServer, "ContentLauncher: Default Failure Response: %" CHIP_ERROR_FORMAT, error.Format());
210210
}
211211

212212
void DeviceEventCallback(const DeviceLayer::ChipDeviceEvent * event, intptr_t arg)

‎src/app/CommandSender.cpp

+8-13
Original file line numberDiff line numberDiff line change
@@ -121,12 +121,11 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha
121121
}
122122

123123
CHIP_ERROR err = CHIP_NO_ERROR;
124-
StatusIB status(Protocols::InteractionModel::Status::Failure);
125124
VerifyOrExit(apExchangeContext == mpExchangeCtx, err = CHIP_ERROR_INCORRECT_STATE);
126125

127126
if (mState == State::AwaitingTimedStatus)
128127
{
129-
err = HandleTimedStatus(aPayloadHeader, std::move(aPayload), status);
128+
err = HandleTimedStatus(aPayloadHeader, std::move(aPayload));
130129
// Skip all other processing here (which is for the response to the
131130
// invoke request), no matter whether err is success or not.
132131
goto exit;
@@ -136,11 +135,10 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha
136135
{
137136
err = ProcessInvokeResponse(std::move(aPayload));
138137
SuccessOrExit(err);
139-
status.mStatus = Protocols::InteractionModel::Status::Success;
140138
}
141139
else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::StatusResponse))
142140
{
143-
err = StatusResponse::ProcessStatusResponse(std::move(aPayload), status);
141+
err = StatusResponse::ProcessStatusResponse(std::move(aPayload));
144142
SuccessOrExit(err);
145143
}
146144
else
@@ -153,7 +151,7 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha
153151
{
154152
if (err != CHIP_NO_ERROR)
155153
{
156-
mpCallback->OnError(this, status, err);
154+
mpCallback->OnError(this, err);
157155
}
158156
}
159157

@@ -210,9 +208,7 @@ void CommandSender::OnResponseTimeout(Messaging::ExchangeContext * apExchangeCon
210208

211209
if (mpCallback != nullptr)
212210
{
213-
StatusIB status;
214-
status.mStatus = Protocols::InteractionModel::Status::Failure;
215-
mpCallback->OnError(this, status, CHIP_ERROR_TIMEOUT);
211+
mpCallback->OnError(this, CHIP_ERROR_TIMEOUT);
216212
}
217213

218214
Close();
@@ -309,14 +305,14 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn
309305

310306
if (mpCallback != nullptr)
311307
{
312-
if (statusIB.mStatus == Protocols::InteractionModel::Status::Success)
308+
if (statusIB.IsSuccess())
313309
{
314310
mpCallback->OnResponse(this, ConcreteCommandPath(endpointId, clusterId, commandId), statusIB,
315311
hasDataResponse ? &commandDataReader : nullptr);
316312
}
317313
else
318314
{
319-
mpCallback->OnError(this, statusIB, CHIP_ERROR_IM_STATUS_CODE_RECEIVED);
315+
mpCallback->OnError(this, statusIB.ToChipError());
320316
}
321317
}
322318
}
@@ -382,10 +378,9 @@ TLV::TLVWriter * CommandSender::GetCommandDataIBTLVWriter()
382378
}
383379
}
384380

385-
CHIP_ERROR CommandSender::HandleTimedStatus(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload,
386-
StatusIB & aStatusIB)
381+
CHIP_ERROR CommandSender::HandleTimedStatus(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload)
387382
{
388-
ReturnErrorOnFailure(TimedRequest::HandleResponse(aPayloadHeader, std::move(aPayload), aStatusIB));
383+
ReturnErrorOnFailure(TimedRequest::HandleResponse(aPayloadHeader, std::move(aPayload)));
389384

390385
return SendInvokeRequest();
391386
}

‎src/app/CommandSender.h

+7-8
Original file line numberDiff line numberDiff line change
@@ -102,9 +102,9 @@ class CommandSender final : public Messaging::ExchangeDelegate
102102
*
103103
* - CHIP_ERROR_TIMEOUT: A response was not received within the expected response timeout.
104104
* - CHIP_ERROR_*TLV*: A malformed, non-compliant response was received from the server.
105-
* - CHIP_ERROR_IM_STATUS_CODE_RECEIVED: An invoke response containing a status code denoting an error was received.
106-
* When the protocol ID in the received status is IM, aInteractionModelStatus will contain the IM status
107-
* code. Otherwise, aInteractionModelStatus will always be set to IM::Status::Failure.
105+
* - CHIP_ERROR encapsulating a StatusIB: If we got a non-path-specific
106+
* status response from the server. In that case,
107+
* StatusIB::InitFromChipError can be used to extract the status.
108108
* - CHIP_ERROR*: All other cases.
109109
*
110110
* The CommandSender object MUST continue to exist after this call is completed. The application shall wait until it
@@ -114,7 +114,7 @@ class CommandSender final : public Messaging::ExchangeDelegate
114114
* @param[in] aStatusIB The status code including IM status code and optional cluster status code
115115
* @param[in] aError A system error code that conveys the overall error code.
116116
*/
117-
virtual void OnError(const CommandSender * apCommandSender, const StatusIB & aStatusIB, CHIP_ERROR aError) {}
117+
virtual void OnError(const CommandSender * apCommandSender, CHIP_ERROR aError) {}
118118

119119
/**
120120
* OnDone will be called when CommandSender has finished all work and is safe to destroy and free the
@@ -279,10 +279,9 @@ class CommandSender final : public Messaging::ExchangeDelegate
279279
// Timed Request. The caller is assumed to have already checked that our
280280
// exchange context member is the one the message came in on.
281281
//
282-
// aStatusIB will be populated with the returned status if we can parse it
283-
// successfully.
284-
CHIP_ERROR HandleTimedStatus(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload,
285-
StatusIB & aStatusIB);
282+
// If the server returned an error status, that will be returned as an error
283+
// value of CHIP_ERROR.
284+
CHIP_ERROR HandleTimedStatus(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload);
286285

287286
// Send our queued-up Invoke Request message. Assumes the exchange is ready
288287
// and mPendingInvokeData is populated.

‎src/app/DeviceControllerInteractionModelDelegate.h

+9-8
Original file line numberDiff line numberDiff line change
@@ -42,17 +42,17 @@ class DeviceControllerInteractionModelDelegate : public chip::app::CommandSender
4242
}
4343
}
4444

45-
void OnError(const app::CommandSender * apCommandSender, const chip::app::StatusIB & aStatus,
46-
CHIP_ERROR aProtocolError) override
45+
void OnError(const app::CommandSender * apCommandSender, CHIP_ERROR aError) override
4746
{
4847
// The IMDefaultResponseCallback started out life as an Ember function, so it only accepted
49-
// Ember status codes. Consequently, let's convert the IM code over to a meaningful Ember status before dispatching.
48+
// Ember status codes. Consequently, let's convert the error over to a meaningful Ember status before dispatching.
5049
//
51-
// This however, results in loss (aError is completely discarded). When full cluster-specific status codes are implemented
52-
// as well, this will be an even bigger problem.
50+
// This however, results in loss (non-IM errors and cluster-specific
51+
// status codes are completely discarded).
5352
//
5453
// For now, #10331 tracks this issue.
55-
IMDefaultResponseCallback(apCommandSender, app::ToEmberAfStatus(aStatus.mStatus));
54+
app::StatusIB status(aError);
55+
IMDefaultResponseCallback(apCommandSender, app::ToEmberAfStatus(status.mStatus));
5656
}
5757

5858
void OnDone(app::CommandSender * apCommandSender) override { return chip::Platform::Delete(apCommandSender); }
@@ -63,9 +63,10 @@ class DeviceControllerInteractionModelDelegate : public chip::app::CommandSender
6363
IMWriteResponseCallback(apWriteClient, attributeStatus.mStatus);
6464
}
6565

66-
void OnError(const app::WriteClient * apWriteClient, const app::StatusIB & aStatus, CHIP_ERROR aError) override
66+
void OnError(const app::WriteClient * apWriteClient, CHIP_ERROR aError) override
6767
{
68-
IMWriteResponseCallback(apWriteClient, aStatus.mStatus);
68+
app::StatusIB status(aError);
69+
IMWriteResponseCallback(apWriteClient, status.mStatus);
6970
}
7071

7172
void OnDone(app::WriteClient * apWriteClient) override {}

‎src/app/MessageDef/StatusIB.h

+13-2
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ struct StatusIB
4545
StatusIB(Protocols::InteractionModel::Status imStatus, ClusterStatus clusterStatus) :
4646
mStatus(imStatus), mClusterStatus(clusterStatus)
4747
{}
48+
explicit StatusIB(CHIP_ERROR error) { InitFromChipError(error); }
4849

4950
enum class Tag : uint8_t
5051
{
@@ -104,12 +105,22 @@ struct StatusIB
104105
CHIP_ERROR ToChipError() const;
105106

106107
/**
107-
* Extract a CHIP_ERROR into this StatusIB. If IsIMStatus is false for the
108-
* error, this might do a best-effort attempt to come up with a
108+
* Extract a CHIP_ERROR into this StatusIB. If IsIMStatus() is false for
109+
* the error, this might do a best-effort attempt to come up with a
109110
* corresponding StatusIB, defaulting to a generic Status::Failure.
110111
*/
111112
void InitFromChipError(CHIP_ERROR aError);
112113

114+
/**
115+
* Test whether this status is a success.
116+
*/
117+
bool IsSuccess() const { return mStatus == Protocols::InteractionModel::Status::Success; }
118+
119+
/**
120+
* Test whether this status is a failure.
121+
*/
122+
bool IsFailure() const { return !IsSuccess(); }
123+
113124
/**
114125
* Register the StatusIB error formatter.
115126
*/

‎src/app/ReadClient.cpp

+1-2
Original file line numberDiff line numberDiff line change
@@ -254,9 +254,8 @@ CHIP_ERROR ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchange
254254
}
255255
else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::StatusResponse))
256256
{
257-
StatusIB status;
258257
VerifyOrExit(apExchangeContext == mpExchangeCtx, err = CHIP_ERROR_INCORRECT_STATE);
259-
err = StatusResponse::ProcessStatusResponse(std::move(aPayload), status);
258+
err = StatusResponse::ProcessStatusResponse(std::move(aPayload));
260259
SuccessOrExit(err);
261260
}
262261
else

‎src/app/ReadClient.h

+3
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,9 @@ class ReadClient : public Messaging::ExchangeDelegate
138138
*
139139
* - CHIP_ERROR_TIMEOUT: A response was not received within the expected response timeout.
140140
* - CHIP_ERROR_*TLV*: A malformed, non-compliant response was received from the server.
141+
* - CHIP_ERROR encapsulating a StatusIB: If we got a non-path-specific
142+
* status response from the server. In that case,
143+
* StatusIB::InitFromChipError can be used to extract the status.
141144
* - CHIP_ERROR*: All other cases.
142145
*
143146
* This object MUST continue to exist after this call is completed. The application shall wait until it

‎src/app/ReadHandler.cpp

+1-2
Original file line numberDiff line numberDiff line change
@@ -143,8 +143,7 @@ CHIP_ERROR ReadHandler::OnReadInitialRequest(System::PacketBufferHandle && aPayl
143143
CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload)
144144
{
145145
CHIP_ERROR err = CHIP_NO_ERROR;
146-
StatusIB status;
147-
err = StatusResponse::ProcessStatusResponse(std::move(aPayload), status);
146+
err = StatusResponse::ProcessStatusResponse(std::move(aPayload));
148147
SuccessOrExit(err);
149148
switch (mState)
150149
{

‎src/app/StatusResponse.cpp

+8-15
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,8 @@ CHIP_ERROR StatusResponse::Send(Protocols::InteractionModel::Status aStatus, Mes
4242
return CHIP_NO_ERROR;
4343
}
4444

45-
CHIP_ERROR StatusResponse::ProcessStatusResponse(System::PacketBufferHandle && aPayload, StatusIB & aStatus)
45+
CHIP_ERROR StatusResponse::ProcessStatusResponse(System::PacketBufferHandle && aPayload)
4646
{
47-
CHIP_ERROR err = CHIP_NO_ERROR;
4847
StatusResponseMessage::Parser response;
4948
System::PacketBufferTLVReader reader;
5049
reader.Init(std::move(aPayload));
@@ -53,22 +52,16 @@ CHIP_ERROR StatusResponse::ProcessStatusResponse(System::PacketBufferHandle && a
5352
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
5453
ReturnErrorOnFailure(response.CheckSchemaValidity());
5554
#endif
56-
ReturnErrorOnFailure(response.GetStatus(aStatus.mStatus));
57-
ChipLogProgress(InteractionModel, "Received status response, status is %" PRIu8, to_underlying(aStatus.mStatus));
55+
StatusIB status;
56+
ReturnErrorOnFailure(response.GetStatus(status.mStatus));
57+
ChipLogProgress(InteractionModel, "Received status response, status is %" PRIu8, to_underlying(status.mStatus));
5858

59-
if (aStatus.mStatus == Protocols::InteractionModel::Status::Success)
59+
if (status.mStatus == Protocols::InteractionModel::Status::Success)
6060
{
61-
err = CHIP_NO_ERROR;
61+
return CHIP_NO_ERROR;
6262
}
63-
else if (aStatus.mStatus == Protocols::InteractionModel::Status::ResourceExhausted)
64-
{
65-
err = CHIP_ERROR_NO_MEMORY;
66-
}
67-
else
68-
{
69-
err = CHIP_ERROR_INCORRECT_STATE;
70-
}
71-
return err;
63+
64+
return status.ToChipError();
7265
}
7366
} // namespace app
7467
} // namespace chip

‎src/app/StatusResponse.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ class StatusResponse
3232
public:
3333
static CHIP_ERROR Send(Protocols::InteractionModel::Status aStatus, Messaging::ExchangeContext * apExchangeContext,
3434
bool aExpectResponse);
35-
static CHIP_ERROR ProcessStatusResponse(System::PacketBufferHandle && aPayload, StatusIB & aStatus);
35+
static CHIP_ERROR ProcessStatusResponse(System::PacketBufferHandle && aPayload);
3636
};
3737
} // namespace app
3838
} // namespace chip

0 commit comments

Comments
 (0)