Bring OTA XML into closer alignment with the spec. (#11243)

* Bring OTA XML into closer alignment with the spec.

1) Flag optional fields of QueryImage as optional.
2) Fix ordering of arguments for QueryImage.
3) Rename ApplyUpdateResponse to match the spec.
4) Use cluster-objects to send responses in example OTA provider.

* Addresss review comments.

* Rebase to tip

examples/ota-provider-app/ota-provider-common/OTAProviderExample.cpp

@@ -18,9 +18,7 @@
 
 #include <ota-provider-common/OTAProviderExample.h>
 
-#include <app-common/zap-generated/cluster-id.h>
-#include <app-common/zap-generated/command-id.h>
-#include <app/CommandPathParams.h>
+#include <app-common/zap-generated/cluster-objects.h>
 #include <app/clusters/ota-provider/ota-provider-delegate.h>
 #include <app/util/af.h>
 #include <crypto/RandUtils.h>
@@ -31,12 +29,10 @@
 #include <string.h>
 
 using chip::ByteSpan;
+using chip::CharSpan;
+using chip::Optional;
 using chip::Span;
-using chip::app::CommandPathFlags;
-using chip::app::CommandPathParams;
 using chip::app::Clusters::OTAProviderDelegate;
-using chip::TLV::ContextTag;
-using chip::TLV::TLVWriter;
 
 constexpr uint8_t kUpdateTokenLen    = 32;                      // must be between 8 and 32
 constexpr uint8_t kUpdateTokenStrLen = kUpdateTokenLen * 2 + 1; // Hex string needs 2 hex chars for every byte
@@ -97,10 +93,12 @@ void OTAProviderExample::SetOTAFilePath(const char * path)
     }
 }
 
-EmberAfStatus OTAProviderExample::HandleQueryImage(chip::app::CommandHandler * commandObj, uint16_t vendorId, uint16_t productId,
-                                                   uint16_t hardwareVersion, uint32_t softwareVersion, uint8_t protocolsSupported,
-                                                   const chip::Span<const char> & location, bool requestorCanConsent,
-                                                   const chip::ByteSpan & metadataForProvider)
+EmberAfStatus OTAProviderExample::HandleQueryImage(chip::app::CommandHandler * commandObj,
+                                                   const chip::app::ConcreteCommandPath & commandPath, uint16_t vendorId,
+                                                   uint16_t productId, uint32_t softwareVersion, uint8_t protocolsSupported,
+                                                   const Optional<uint16_t> & hardwareVersion, const Optional<CharSpan> & location,
+                                                   const Optional<bool> & requestorCanConsent,
+                                                   const Optional<ByteSpan> & metadataForProvider)
 {
     // TODO: add confiuration for returning BUSY status
 
@@ -151,29 +149,27 @@ EmberAfStatus OTAProviderExample::HandleQueryImage(chip::app::CommandHandler * c
         queryStatus = EMBER_ZCL_OTA_QUERY_STATUS_NOT_AVAILABLE;
     }
 
-    CommandPathParams cmdParams = { emberAfCurrentEndpoint(), 0 /* mGroupId */, ZCL_OTA_PROVIDER_CLUSTER_ID,
-                                    ZCL_QUERY_IMAGE_RESPONSE_COMMAND_ID, (CommandPathFlags::kEndpointIdValid) };
-    TLVWriter * writer          = nullptr;
-    uint8_t tagNum              = 0;
-    VerifyOrReturnError((commandObj->PrepareCommand(cmdParams) == CHIP_NO_ERROR), EMBER_ZCL_STATUS_FAILURE);
-    VerifyOrReturnError((writer = commandObj->GetCommandDataIBTLVWriter()) != nullptr, EMBER_ZCL_STATUS_FAILURE);
-    VerifyOrReturnError(writer->Put(ContextTag(tagNum++), queryStatus) == CHIP_NO_ERROR, EMBER_ZCL_STATUS_FAILURE);
-    VerifyOrReturnError(writer->Put(ContextTag(tagNum++), mDelayedActionTimeSec) == CHIP_NO_ERROR, EMBER_ZCL_STATUS_FAILURE);
-    VerifyOrReturnError(writer->PutString(ContextTag(tagNum++), uriBuf) == CHIP_NO_ERROR, EMBER_ZCL_STATUS_FAILURE);
-    VerifyOrReturnError(writer->Put(ContextTag(tagNum++), newSoftwareVersion) == CHIP_NO_ERROR, EMBER_ZCL_STATUS_FAILURE);
-    VerifyOrReturnError(writer->PutString(ContextTag(tagNum++), kExampleSoftwareString) == CHIP_NO_ERROR, EMBER_ZCL_STATUS_FAILURE);
-    VerifyOrReturnError(writer->PutBytes(ContextTag(tagNum++), updateToken, kUpdateTokenLen) == CHIP_NO_ERROR,
-                        EMBER_ZCL_STATUS_FAILURE);
-    VerifyOrReturnError(writer->Put(ContextTag(tagNum++), userConsentNeeded) == CHIP_NO_ERROR, EMBER_ZCL_STATUS_FAILURE);
-    VerifyOrReturnError(writer->PutBytes(ContextTag(tagNum++), updateToken, kUpdateTokenLen) == CHIP_NO_ERROR,
-                        EMBER_ZCL_STATUS_FAILURE); // metadata
-    VerifyOrReturnError((commandObj->FinishCommand() == CHIP_NO_ERROR), EMBER_ZCL_STATUS_FAILURE);
-
+    chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::QueryImageResponse::Type response;
+    response.status                = queryStatus;
+    response.delayedActionTime     = mDelayedActionTimeSec;
+    response.imageURI              = chip::CharSpan(uriBuf, strlen(uriBuf));
+    response.softwareVersion       = newSoftwareVersion;
+    response.softwareVersionString = chip::CharSpan(kExampleSoftwareString, strlen(kExampleSoftwareString));
+    response.updateToken           = chip::ByteSpan(updateToken);
+    response.userConsentNeeded     = userConsentNeeded;
+    // TODO: Once our client is using APIs that handle optional arguments
+    // correctly, update QueryImageResponse to have the right things optional.
+    // At that point we can decide whether to send metadataForRequestor as an
+    // empty ByteSpan or whether to not send it at all.
+    response.metadataForRequestor = chip::ByteSpan();
+
+    VerifyOrReturnError(commandObj->AddResponseData(commandPath, response) == CHIP_NO_ERROR, EMBER_ZCL_STATUS_FAILURE);
     return EMBER_ZCL_STATUS_SUCCESS;
 }
 
 EmberAfStatus OTAProviderExample::HandleApplyUpdateRequest(chip::app::CommandHandler * commandObj,
-                                                           const chip::ByteSpan & updateToken, uint32_t newVersion)
+                                                           const chip::app::ConcreteCommandPath & commandPath,
+                                                           const ByteSpan & updateToken, uint32_t newVersion)
 {
     // TODO: handle multiple transfers by tracking updateTokens
 
@@ -185,15 +181,10 @@ EmberAfStatus OTAProviderExample::HandleApplyUpdateRequest(chip::app::CommandHan
 
     VerifyOrReturnError(commandObj != nullptr, EMBER_ZCL_STATUS_INVALID_VALUE);
 
-    CommandPathParams cmdParams = { emberAfCurrentEndpoint(), 0 /* mGroupId */, ZCL_OTA_PROVIDER_CLUSTER_ID,
-                                    ZCL_APPLY_UPDATE_REQUEST_RESPONSE_COMMAND_ID, (CommandPathFlags::kEndpointIdValid) };
-    TLVWriter * writer          = nullptr;
-
-    VerifyOrReturnError((commandObj->PrepareCommand(cmdParams) == CHIP_NO_ERROR), EMBER_ZCL_STATUS_FAILURE);
-    VerifyOrReturnError((writer = commandObj->GetCommandDataIBTLVWriter()) != nullptr, EMBER_ZCL_STATUS_FAILURE);
-    VerifyOrReturnError(writer->Put(ContextTag(0), updateAction) == CHIP_NO_ERROR, EMBER_ZCL_STATUS_FAILURE);
-    VerifyOrReturnError(writer->Put(ContextTag(1), mDelayedActionTimeSec) == CHIP_NO_ERROR, EMBER_ZCL_STATUS_FAILURE);
-    VerifyOrReturnError((commandObj->FinishCommand() == CHIP_NO_ERROR), EMBER_ZCL_STATUS_FAILURE);
+    chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::ApplyUpdateResponse::Type response;
+    response.action            = updateAction;
+    response.delayedActionTime = mDelayedActionTimeSec;
+    VerifyOrReturnError(commandObj->AddResponseData(commandPath, response) == CHIP_NO_ERROR, EMBER_ZCL_STATUS_FAILURE);
 
     return EMBER_ZCL_STATUS_SUCCESS;
 }

examples/ota-provider-app/ota-provider-common/OTAProviderExample.h

@@ -32,11 +32,14 @@ class OTAProviderExample : public chip::app::Clusters::OTAProviderDelegate
     void SetOTAFilePath(const char * path);
 
     // Inherited from OTAProviderDelegate
-    EmberAfStatus HandleQueryImage(chip::app::CommandHandler * commandObj, uint16_t vendorId, uint16_t productId,
-                                   uint16_t hardwareVersion, uint32_t softwareVersion, uint8_t protocolsSupported,
-                                   const chip::Span<const char> & location, bool requestorCanConsent,
-                                   const chip::ByteSpan & metadataForServer) override;
-    EmberAfStatus HandleApplyUpdateRequest(chip::app::CommandHandler * commandObj, const chip::ByteSpan & updateToken,
+    EmberAfStatus HandleQueryImage(chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
+                                   uint16_t vendorId, uint16_t productId, uint32_t softwareVersion, uint8_t protocolsSupported,
+                                   const chip::Optional<uint16_t> & hardwareVersion,
+                                   const chip::Optional<chip::CharSpan> & location,
+                                   const chip::Optional<bool> & requestorCanConsent,
+                                   const chip::Optional<chip::ByteSpan> & metadataForServer) override;
+    EmberAfStatus HandleApplyUpdateRequest(chip::app::CommandHandler * commandObj,
+                                           const chip::app::ConcreteCommandPath & commandPath, const chip::ByteSpan & updateToken,
                                            uint32_t newVersion) override;
     EmberAfStatus HandleNotifyUpdateApplied(const chip::ByteSpan & updateToken, uint32_t softwareVersion) override;
 

examples/ota-requestor-app/linux/main.cpp

@@ -175,9 +175,8 @@ void OnConnected(void * context, OperationalDeviceProxy * operationalDeviceProxy
         ChipLogError(SoftwareUpdate, "Associate() failed: %" CHIP_ERROR_FORMAT, err.Format());
         return;
     }
-    err = cluster.QueryImage(successCallback, failureCallback, kExampleVendorId, kExampleProductId, kExampleHWVersion,
-                             kExampleSoftwareVersion, kExampleProtocolsSupported, exampleLocation, kExampleClientCanConsent,
-                             metadata);
+    err = cluster.QueryImage(successCallback, failureCallback, kExampleVendorId, kExampleProductId, kExampleSoftwareVersion,
+                             kExampleProtocolsSupported, kExampleHWVersion, exampleLocation, kExampleClientCanConsent, metadata);
     if (err != CHIP_NO_ERROR)
     {
         ChipLogError(SoftwareUpdate, "QueryImage() failed: %" CHIP_ERROR_FORMAT, err.Format());

src/app/clusters/ota-provider/ota-provider-delegate.h

@@ -20,7 +20,9 @@
 
 #include <app-common/zap-generated/enums.h>
 #include <app/CommandHandler.h>
+#include <app/ConcreteCommandPath.h>
 #include <app/util/af.h>
+#include <lib/core/Optional.h>
 
 namespace chip {
 namespace app {
@@ -34,13 +36,14 @@ class OTAProviderDelegate
 {
 public:
     // TODO(#8605): protocolsSupported should be list of OTADownloadProtocol enums, not uint8_t
-    virtual EmberAfStatus HandleQueryImage(CommandHandler * commandObj, uint16_t vendorId, uint16_t productId,
-                                           uint16_t hardwareVersion, uint32_t softwareVersion, uint8_t protocolsSupported,
-                                           const chip::Span<const char> & location, bool requestorCanConsent,
-                                           const chip::ByteSpan & metadataForProvider) = 0;
-
-    virtual EmberAfStatus HandleApplyUpdateRequest(CommandHandler * commandObj, const chip::ByteSpan & updateToken,
-                                                   uint32_t newVersion) = 0;
+    virtual EmberAfStatus HandleQueryImage(CommandHandler * commandObj, const ConcreteCommandPath & commandPath, uint16_t vendorId,
+                                           uint16_t productId, uint32_t softwareVersion, uint8_t protocolsSupported,
+                                           const Optional<uint16_t> & hardwareVersion, const Optional<CharSpan> & location,
+                                           const Optional<bool> & requestorCanConsent,
+                                           const Optional<ByteSpan> & metadataForProvider) = 0;
+
+    virtual EmberAfStatus HandleApplyUpdateRequest(CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
+                                                   const chip::ByteSpan & updateToken, uint32_t newVersion) = 0;
 
     virtual EmberAfStatus HandleNotifyUpdateApplied(const chip::ByteSpan & updateToken, uint32_t softwareVersion) = 0;
 

src/app/clusters/ota-provider/ota-provider.cpp

@@ -92,7 +92,7 @@ bool emberAfOtaSoftwareUpdateProviderClusterApplyUpdateRequestCallback(
         emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_INVALID_ARGUMENT);
     }
 
-    status = delegate->HandleApplyUpdateRequest(commandObj, updateToken, newVersion);
+    status = delegate->HandleApplyUpdateRequest(commandObj, commandPath, updateToken, newVersion);
     if (status != EMBER_ZCL_STATUS_SUCCESS)
     {
         emberAfSendImmediateDefaultResponse(status);
@@ -189,27 +189,39 @@ bool emberAfOtaSoftwareUpdateProviderClusterQueryImageCallback(app::CommandHandl
     ChipLogDetail(Zcl, "  ProductID: %" PRIu16, productId);
     ChipLogDetail(Zcl, "  SoftwareVersion: %" PRIu32, softwareVersion);
     ChipLogDetail(Zcl, "  ProtocolsSupported: %" PRIu8, protocolsSupported);
-    ChipLogDetail(Zcl, "  HardwareVersion: %" PRIu16, hardwareVersion);
-    ChipLogDetail(Zcl, "  Location: %.*s", static_cast<int>(location.size()), location.data());
-    ChipLogDetail(Zcl, "  RequestorCanConsent: %" PRIu8, requestorCanConsent);
-    ChipLogDetail(Zcl, "  MetadataForProvider: %zu", metadataForProvider.size());
+    if (hardwareVersion.HasValue())
+    {
+        ChipLogDetail(Zcl, "  HardwareVersion: %" PRIu16, hardwareVersion.Value());
+    }
+    if (location.HasValue())
+    {
+        ChipLogDetail(Zcl, "  Location: %.*s", static_cast<int>(location.Value().size()), location.Value().data());
+    }
+    if (requestorCanConsent.HasValue())
+    {
+        ChipLogDetail(Zcl, "  RequestorCanConsent: %" PRIu8, requestorCanConsent.Value());
+    }
+    if (metadataForProvider.HasValue())
+    {
+        ChipLogDetail(Zcl, "  MetadataForProvider: %zu", metadataForProvider.Value().size());
+    }
 
-    if (location.size() != kLocationLen)
+    if (location.HasValue() && location.Value().size() != kLocationLen)
     {
-        ChipLogError(Zcl, "location param length %zu != expected length %zu", location.size(), kLocationLen);
+        ChipLogError(Zcl, "location param length %zu != expected length %zu", location.Value().size(), kLocationLen);
         emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_INVALID_ARGUMENT);
         return true;
     }
 
-    if (metadataForProvider.size() > kMaxMetadataLen)
+    if (metadataForProvider.HasValue() && metadataForProvider.Value().size() > kMaxMetadataLen)
     {
-        ChipLogError(Zcl, "metadata size %zu exceeds max %zu", metadataForProvider.size(), kMaxMetadataLen);
+        ChipLogError(Zcl, "metadata size %zu exceeds max %zu", metadataForProvider.Value().size(), kMaxMetadataLen);
         emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_INVALID_ARGUMENT);
         return true;
     }
 
-    status = delegate->HandleQueryImage(commandObj, vendorId, productId, hardwareVersion, softwareVersion, protocolsSupported,
-                                        location, requestorCanConsent, metadataForProvider);
+    status = delegate->HandleQueryImage(commandObj, commandPath, vendorId, productId, softwareVersion, protocolsSupported,
+                                        hardwareVersion, location, requestorCanConsent, metadataForProvider);
     if (status != EMBER_ZCL_STATUS_SUCCESS)
     {
         emberAfSendImmediateDefaultResponse(status);

src/app/zap-templates/zcl/data-model/chip/chip-ota.xml

@@ -43,18 +43,18 @@ limitations under the License.
         <define>OTA_PROVIDER_CLUSTER</define>
         <client tick="false" init="false">true</client>
         <server tick="false" init="false">true</server>
-        <command source="client" code="0x00" name="QueryImage" optional="false" cli="chip ota queryimage">
+        <command source="client" code="0x00" name="QueryImage" response="QueryImageResponse" optional="false" cli="chip ota queryimage">
             <description>Determine availability of a new Software Image</description>
-            <arg name="vendorId" type="INT16U"/>
+            <arg name="vendorId" type="vendor_id"/>
             <arg name="productId" type="INT16U"/>
-            <arg name="hardwareVersion" type="INT16U"/>
             <arg name="softwareVersion" type="INT32U"/>
             <arg name="protocolsSupported" type="OTADownloadProtocol"/> <!-- TODO(#8605): add array="true" when lists are supported -->
-            <arg name="location" type="CHAR_STRING" length="2"/>
-            <arg name="requestorCanConsent" type="BOOLEAN"/>
-            <arg name="metadataForProvider" type="OCTET_STRING" length="512"/>
+            <arg name="hardwareVersion" type="INT16U" optional="true"/>
+            <arg name="location" type="CHAR_STRING" length="2" optional="true"/>
+            <arg name="requestorCanConsent" type="BOOLEAN" optional="true"/>
+            <arg name="metadataForProvider" type="OCTET_STRING" length="512" optional="true"/>
         </command>
-        <command source="client" code="0x01" name="ApplyUpdateRequest" optional="false" cli="chip ota applyupdaterequest">
+        <command source="client" code="0x01" name="ApplyUpdateRequest" optional="false" response="ApplyUpdateResponse" cli="chip ota applyupdaterequest">
             <description>Determine next action to take for a new Software Image</description>
             <arg name="updateToken" type="OCTET_STRING" length="32"/>
             <arg name="newVersion" type="INT32U"/>
@@ -75,7 +75,7 @@ limitations under the License.
             <arg name="userConsentNeeded" type="BOOLEAN" default="false"/>
             <arg name="metadataForRequestor" type="OCTET_STRING" length="512"/>
         </command>
-        <command source="server" code="0x04" name="ApplyUpdateRequestResponse" optional="false" cli="chip ota applyupdaterequestresponse">
+        <command source="server" code="0x04" name="ApplyUpdateResponse" optional="false" cli="chip ota applyupdateresponse">
             <description>Reponse to ApplyUpdateRequest command</description>
             <arg name="action" type="OTAApplyUpdateAction"/>
             <arg name="delayedActionTime" type="INT32U"/>