Implement the client side of timed write. (#12567)

Fixes that had to be made alongside the main change:

1) Fix chip-tool to use the templated WriteAttribute for its
   command-line writes so we can actually pass in the timeout optional
   parameter for testing.

2) Factor some code for dealing with timed interactions out of
   CommandSender into TimedRequest so WriteClient can share it.

3) Fix the OnError signature for WriteClient::Callback to take a
   StatusIB, so we can properly communicate errors back and actually
   test for them in YAML.

examples/chip-tool/commands/common/Command.h

@@ -190,6 +190,20 @@ class Command
         return AddArgument(name, min, max, &value->SetNonNull(), optional);
     }
 
+    size_t AddArgument(const char * name, float min, float max, chip::app::DataModel::Nullable<float> * value,
+                       bool optional = false)
+    {
+        // We always require our args to be provided for the moment.
+        return AddArgument(name, min, max, &value->SetNonNull(), optional);
+    }
+
+    size_t AddArgument(const char * name, double min, double max, chip::app::DataModel::Nullable<double> * value,
+                       bool optional = false)
+    {
+        // We always require our args to be provided for the moment.
+        return AddArgument(name, min, max, &value->SetNonNull(), optional);
+    }
+
     virtual CHIP_ERROR Run() = 0;
 
 private:

examples/chip-tool/templates/commands.zapt

@@ -517,8 +517,6 @@ public:
 
     ~Write{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}()
     {
-      delete onSuccessCallback;
-      delete onFailureCallback;
     }
 
     CHIP_ERROR SendCommand(ChipDevice * device, uint8_t endpointId) override
@@ -527,13 +525,11 @@ public:
 
         chip::Controller::{{asUpperCamelCase parent.name}}Cluster cluster;
         cluster.Associate(device, endpointId);
-        return cluster.WriteAttribute{{asUpperCamelCase name}}(onSuccessCallback->Cancel(), onFailureCallback->Cancel(), mValue);
+        return cluster.WriteAttribute<chip::app::Clusters::{{asUpperCamelCase parent.name}}::Attributes::{{asUpperCamelCase name}}::TypeInfo>(mValue, this, OnDefaultSuccessResponse, OnDefaultFailure, mTimedInteractionTimeoutMs);
     }
 
 private:
-    chip::Callback::Callback<DefaultSuccessCallback> * onSuccessCallback = new chip::Callback::Callback<DefaultSuccessCallback>(OnDefaultSuccessResponse, this);
-    chip::Callback::Callback<DefaultFailureCallback> * onFailureCallback = new chip::Callback::Callback<DefaultFailureCallback>(OnDefaultFailureResponse, this);
-    {{chipType}} mValue;
+    {{zapTypeToEncodableClusterObjectType type ns=parent.name}} mValue;
 };
 
 {{/unless}}

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

@@ -180,6 +180,29 @@ class {{filename}}: public TestCommand
         {{else}}
         const chip::EndpointId endpoint = mEndpointId.HasValue() ? mEndpointId.Value() : {{endpoint}};
         {{/if}}
+
+        {{~#*inline "maybeTimedInteractionTimeout"}}
+          {{#if timedInteractionTimeoutMs}}
+          , {{timedInteractionTimeoutMs}}
+          {{else if commandObject.mustUseTimedInvoke}}
+          , chip::NullOptional
+          {{else if attributeObject.mustUseTimedWrite}}
+          , chip::NullOptional
+          {{/if}}
+        {{/inline~}}
+
+        {{~#*inline "maybeWait"}}
+          {{#if busyWaitMs}}
+          {
+            using namespace chip::System::Clock::Literals;
+            // Busy-wait for {{busyWaitMs}} milliseconds.
+            auto & clock = chip::System::SystemClock();
+            auto start = clock.GetMonotonicTimestamp();
+            while (clock.GetMonotonicTimestamp() - start < {{busyWaitMs}}_ms);
+          }
+          {{/if}}
+        {{/inline~}}
+
         {{#if isCommand}}
         using RequestType = chip::app::Clusters::{{asUpperCamelCase cluster}}::Commands::{{asUpperCamelCase command}}::Type;
 
@@ -199,21 +222,9 @@ class {{filename}}: public TestCommand
         ReturnErrorOnFailure(chip::Controller::{{#if isGroupCommand}}InvokeGroupCommand{{else}}InvokeCommand{{/if}}({{>device}}, this, success, failure,
           {{#if isGroupCommand}}groupId{{else}}endpoint{{/if}},
           request
-          {{#if timedInteractionTimeoutMs}}
-          , {{timedInteractionTimeoutMs}}
-          {{else if commandObject.mustUseTimedInvoke}}
-          , chip::NullOptional
-          {{/if}}
+          {{> maybeTimedInteractionTimeout }}
           ));
-        {{#if busyWaitMs}}
-          {
-            using namespace chip::System::Clock::Literals;
-            // Busy-wait for {{busyWaitMs}} milliseconds.
-            auto & clock = chip::System::SystemClock();
-            auto start = clock.GetMonotonicTimestamp();
-            while (clock.GetMonotonicTimestamp() - start < {{busyWaitMs}}_ms);
-          }
-        {{/if}}
+        {{> maybeWait }}
         {{#unless async}}return CHIP_NO_ERROR;{{/unless}}
         {{else}}
         chip::Controller::{{asUpperCamelCase cluster}}ClusterTest cluster;
@@ -232,7 +243,12 @@ class {{filename}}: public TestCommand
           {{#*inline "failureResponse"}}OnFailureCallback_{{index}}{{/inline}}
           {{#*inline "successResponse"}}OnSuccessCallback_{{index}}{{/inline}}
           {{#*inline "doneResponse"}}OnDoneCallback_{{index}}{{/inline}}
-          {{#if async}}ReturnErrorOnFailure({{else}}return {{/if}}cluster.WriteAttribute<chip::app::Clusters::{{asUpperCamelCase cluster}}::Attributes::{{asUpperCamelCase attribute}}::TypeInfo>({{#chip_tests_item_parameters}}{{asLowerCamelCase name}}Argument, {{/chip_tests_item_parameters}}this, {{>successResponse}}, {{>failureResponse}}{{#if isGroupCommand}}, {{>doneResponse}}{{/if}}){{#if async}}){{/if}};
+          ReturnErrorOnFailure(cluster.WriteAttribute<chip::app::Clusters::{{asUpperCamelCase cluster}}::Attributes::{{asUpperCamelCase attribute}}::TypeInfo>({{#chip_tests_item_parameters}}{{asLowerCamelCase name}}Argument, {{/chip_tests_item_parameters}}this, {{>successResponse}}, {{>failureResponse}}
+            {{~> maybeTimedInteractionTimeout ~}}
+            {{~#if isGroupCommand}}, {{>doneResponse}}{{/if~}}
+            ));
+          {{> maybeWait }}
+          {{#unless async}}return CHIP_NO_ERROR;{{/unless}}
         {{else if isReadAttribute}}
           {{#*inline "failureResponse"}}OnFailureCallback_{{index}}{{/inline}}
           {{#*inline "successResponse"}}OnSuccessCallback_{{index}}{{/inline}}

src/app/BUILD.gn

@@ -123,6 +123,8 @@ static_library("app") {
     "StatusResponse.h",
     "TimedHandler.cpp",
     "TimedHandler.h",
+    "TimedRequest.cpp",
+    "TimedRequest.h",
     "WriteClient.cpp",
     "WriteHandler.cpp",
     "decoder.cpp",

src/app/CommandSender.cpp

@@ -27,7 +27,7 @@
 #include "CommandHandler.h"
 #include "InteractionModelEngine.h"
 #include "StatusResponse.h"
-#include <app/MessageDef/TimedRequestMessage.h>
+#include <app/TimedRequest.h>
 #include <protocols/Protocols.h>
 #include <protocols/interaction_model/Constants.h>
 
@@ -77,7 +77,9 @@ CHIP_ERROR CommandSender::SendCommandRequest(SessionHandle session, System::Cloc
 
     if (mTimedInvokeTimeoutMs.HasValue())
     {
-        return SendTimedRequest(mTimedInvokeTimeoutMs.Value());
+        ReturnErrorOnFailure(TimedRequest::Send(mpExchangeCtx, mTimedInvokeTimeoutMs.Value()));
+        MoveToState(CommandState::AwaitingTimedStatus);
+        return CHIP_NO_ERROR;
     }
 
     return SendInvokeRequest();
@@ -144,7 +146,7 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha
     {
         Close();
     }
-    // Else we got a response to a Timed Request and just send the invoke.
+    // Else we got a response to a Timed Request and just sent the invoke.
 
     return err;
 }
@@ -355,44 +357,10 @@ TLV::TLVWriter * CommandSender::GetCommandDataIBTLVWriter()
     }
 }
 
-CHIP_ERROR CommandSender::SendTimedRequest(uint16_t aTimeoutMs)
-{
-    using namespace Protocols::InteractionModel;
-    using namespace Messaging;
-
-    // The payload is an anonymous struct (2 bytes) containing a single
-    // 16-bit integer with a context tag (1 control byte, 1 byte tag, at
-    // most 2 bytes for the integer).  Use MessagePacketBuffer::New to
-    // account for other message-global overheads (MIC, etc).
-    System::PacketBufferHandle payload = MessagePacketBuffer::New(6);
-    VerifyOrReturnError(!payload.IsNull(), CHIP_ERROR_NO_MEMORY);
-
-    System::PacketBufferTLVWriter writer;
-    writer.Init(std::move(payload));
-
-    TimedRequestMessage::Builder builder;
-    ReturnErrorOnFailure(builder.Init(&writer));
-
-    builder.TimeoutMs(aTimeoutMs);
-    ReturnErrorOnFailure(builder.GetError());
-
-    ReturnErrorOnFailure(writer.Finalize(&payload));
-
-    ReturnErrorOnFailure(mpExchangeCtx->SendMessage(MsgType::TimedRequest, std::move(payload), SendMessageFlags::kExpectResponse));
-    MoveToState(CommandState::AwaitingTimedStatus);
-    return CHIP_NO_ERROR;
-}
-
 CHIP_ERROR CommandSender::HandleTimedStatus(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload,
                                             StatusIB & aStatusIB)
 {
-    using namespace Protocols::InteractionModel;
-
-    VerifyOrReturnError(aPayloadHeader.HasMessageType(MsgType::StatusResponse), CHIP_ERROR_INVALID_MESSAGE_TYPE);
-
-    ReturnErrorOnFailure(StatusResponse::ProcessStatusResponse(std::move(aPayload), aStatusIB));
-
-    VerifyOrReturnError(aStatusIB.mStatus == Status::Success, CHIP_ERROR_IM_STATUS_CODE_RECEIVED);
+    ReturnErrorOnFailure(TimedRequest::HandleResponse(aPayloadHeader, std::move(aPayload), aStatusIB));
 
     return SendInvokeRequest();
 }

src/app/CommandSender.h

@@ -26,6 +26,7 @@
 
 #include <type_traits>
 
+#include <app/MessageDef/StatusIB.h>
 #include <app/data-model/Encode.h>
 #include <lib/core/CHIPCore.h>
 #include <lib/core/CHIPTLVDebug.hpp>
@@ -223,10 +224,6 @@ class CommandSender final : public Command, public Messaging::ExchangeDelegate
     CHIP_ERROR ProcessInvokeResponse(System::PacketBufferHandle && payload);
     CHIP_ERROR ProcessInvokeResponseIB(InvokeResponseIB::Parser & aInvokeResponse);
 
-    // SendTimedRequest expects to be called after the exchange has already been
-    // created and we know for sure we need to do a timed invoke.
-    CHIP_ERROR SendTimedRequest(uint16_t aTimeoutMs);
-
     // Handle a message received when we are expecting a status response to a
     // Timed Request.  The caller is assumed to have already checked that our
     // exchange context member is the one the message came in on.

src/app/DeviceControllerInteractionModelDelegate.h

@@ -66,9 +66,9 @@ class DeviceControllerInteractionModelDelegate : public chip::app::ReadClient::C
         IMWriteResponseCallback(apWriteClient, attributeStatus.mStatus);
     }
 
-    void OnError(const app::WriteClient * apWriteClient, CHIP_ERROR aError) override
+    void OnError(const app::WriteClient * apWriteClient, const app::StatusIB & aStatus, CHIP_ERROR aError) override
     {
-        IMWriteResponseCallback(apWriteClient, Protocols::InteractionModel::Status::Failure);
+        IMWriteResponseCallback(apWriteClient, aStatus.mStatus);
     }
 
     void OnDone(app::WriteClient * apWriteClient) override {}

src/app/InteractionModelEngine.cpp

@@ -261,7 +261,8 @@ CHIP_ERROR InteractionModelEngine::ShutdownSubscriptions(FabricIndex aFabricInde
     return err;
 }
 
-CHIP_ERROR InteractionModelEngine::NewWriteClient(WriteClientHandle & apWriteClient, WriteClient::Callback * apCallback)
+CHIP_ERROR InteractionModelEngine::NewWriteClient(WriteClientHandle & apWriteClient, WriteClient::Callback * apCallback,
+                                                  const Optional<uint16_t> & aTimedWriteTimeoutMs)
 {
     apWriteClient.SetWriteClient(nullptr);
 
@@ -271,7 +272,7 @@ CHIP_ERROR InteractionModelEngine::NewWriteClient(WriteClientHandle & apWriteCli
         {
             continue;
         }
-        ReturnLogErrorOnFailure(writeClient.Init(mpExchangeMgr, apCallback));
+        ReturnLogErrorOnFailure(writeClient.Init(mpExchangeMgr, apCallback, aTimedWriteTimeoutMs));
         apWriteClient.SetWriteClient(&writeClient);
         return CHIP_NO_ERROR;
     }

src/app/InteractionModelEngine.h

@@ -141,7 +141,8 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman
      *  @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);
+    CHIP_ERROR NewWriteClient(WriteClientHandle & apWriteClient, WriteClient::Callback * callback,
+                              const Optional<uint16_t> & aTimedWriteTimeoutMs = NullOptional);
 
     /**
      *  Allocate a ReadClient that can be used to do a read interaction.  If the call succeeds, the consumer

src/app/TimedRequest.cpp

@@ -0,0 +1,69 @@
+/*
+ *
+ *    Copyright (c) 2021 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#include "TimedRequest.h"
+
+#include <app/MessageDef/TimedRequestMessage.h>
+#include <app/StatusResponse.h>
+#include <protocols/interaction_model/Constants.h>
+#include <system/TLVPacketBufferBackingStore.h>
+#include <transport/SessionManager.h>
+
+namespace chip {
+namespace app {
+
+using namespace Protocols::InteractionModel;
+using namespace Messaging;
+
+CHIP_ERROR TimedRequest::Send(ExchangeContext * aExchangeContext, uint16_t aTimeoutMs)
+{
+    // The payload is an anonymous struct (2 bytes) containing a single
+    // 16-bit integer with a context tag (1 control byte, 1 byte tag, at
+    // most 2 bytes for the integer).  Use MessagePacketBuffer::New to
+    // account for other message-global overheads (MIC, etc).
+    System::PacketBufferHandle payload = MessagePacketBuffer::New(6);
+    VerifyOrReturnError(!payload.IsNull(), CHIP_ERROR_NO_MEMORY);
+
+    System::PacketBufferTLVWriter writer;
+    writer.Init(std::move(payload));
+
+    TimedRequestMessage::Builder builder;
+    ReturnErrorOnFailure(builder.Init(&writer));
+
+    builder.TimeoutMs(aTimeoutMs);
+    ReturnErrorOnFailure(builder.GetError());
+
+    ReturnErrorOnFailure(writer.Finalize(&payload));
+
+    return aExchangeContext->SendMessage(MsgType::TimedRequest, std::move(payload), SendMessageFlags::kExpectResponse);
+}
+
+CHIP_ERROR TimedRequest::HandleResponse(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload,
+                                        StatusIB & aStatusIB)
+{
+    VerifyOrReturnError(aPayloadHeader.HasMessageType(MsgType::StatusResponse), CHIP_ERROR_INVALID_MESSAGE_TYPE);
+
+    ReturnErrorOnFailure(StatusResponse::ProcessStatusResponse(std::move(aPayload), aStatusIB));
+
+    VerifyOrReturnError(aStatusIB.mStatus == Status::Success, CHIP_ERROR_IM_STATUS_CODE_RECEIVED);
+
+    return CHIP_NO_ERROR;
+}
+
+} // namespace app
+} // namespace chip

src/app/TimedRequest.h

@@ -0,0 +1,44 @@
+/*
+ *
+ *    Copyright (c) 2021 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#pragma once
+
+#include <app/MessageDef/StatusIB.h>
+#include <lib/core/CHIPError.h>
+#include <messaging/ExchangeContext.h>
+#include <system/SystemPacketBuffer.h>
+#include <transport/raw/MessageHeader.h>
+
+#include <cstdint>
+
+namespace chip {
+namespace app {
+class TimedRequest
+{
+public:
+    // Send a timed request with the given timeout value on the given exchange.
+    static CHIP_ERROR Send(Messaging::ExchangeContext * aExchangeContext, uint16_t aTimeoutMs);
+
+    // Handle a response message (which may not actually be a StatusResponse,
+    // but came in after we sent a timed request).
+    static CHIP_ERROR HandleResponse(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload,
+                                     StatusIB & aStatusIB);
+};
+
+} // namespace app
+} // namespace chip