Address code review comments

- Use Reader to parse BDX URI
- Pass in server instance for CASE sessions and BDX transfer usage

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

@@ -33,6 +33,7 @@ using chip::ByteSpan;
 using chip::CharSpan;
 using chip::FabricIndex;
 using chip::FabricInfo;
+using chip::MutableCharSpan;
 using chip::NodeId;
 using chip::Optional;
 using chip::Server;
@@ -62,18 +63,18 @@ void GenerateUpdateToken(uint8_t * buf, size_t bufSize)
     }
 }
 
-bool GenerateBdxUri(NodeId nodeId, const Span<char> & fileDesignator, Span<char> outUri, size_t availableSize)
+bool GenerateBdxUri(NodeId nodeId, CharSpan fileDesignator, MutableCharSpan outUri)
 {
     static constexpr char bdxPrefix[] = "bdx://";
     size_t nodeIdHexStrLen            = sizeof(nodeId) * 2;
     size_t expectedLength             = strlen(bdxPrefix) + nodeIdHexStrLen + 1 + fileDesignator.size();
 
-    if (expectedLength >= availableSize)
+    if (expectedLength >= outUri.size())
     {
         return false;
     }
 
-    size_t written = static_cast<size_t>(snprintf(outUri.data(), availableSize, "%s" ChipLogFormatX64 "/%s", bdxPrefix,
+    size_t written = static_cast<size_t>(snprintf(outUri.data(), outUri.size(), "%s" ChipLogFormatX64 "/%s", bdxPrefix,
                                                   ChipLogValueX64(nodeId), fileDesignator.data()));
 
     return expectedLength == written;
@@ -126,8 +127,9 @@ EmberAfStatus OTAProviderExample::HandleQueryImage(chip::app::CommandHandler * c
         NodeId nodeId           = fabricInfo->GetPeerId().GetNodeId();
 
         // Only doing BDX transport for now
-        GenerateBdxUri(nodeId, Span<char>(mOTAFilePath, strlen(mOTAFilePath)), Span<char>(uriBuf, kUriMaxLen), kUriMaxLen);
-        ChipLogDetail(SoftwareUpdate, "generated URI: %.*s", static_cast<int>(strlen(uriBuf)), uriBuf);
+        MutableCharSpan uri(uriBuf, kUriMaxLen);
+        GenerateBdxUri(nodeId, CharSpan(mOTAFilePath, strlen(mOTAFilePath)), uri);
+        ChipLogDetail(SoftwareUpdate, "Generated URI: %.*s", static_cast<int>(uri.size()), uri.data());
     }
 
     // Set Status for the Query Image Response

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

@@ -193,6 +193,9 @@ int main(int argc, char * argv[])
     // Initialize and interconnect the Requestor and Image Processor objects -- START
     SetRequestorInstance(&requestorCore);
 
+    // Set server instance used for session establishment
+    requestorCore.SetServerInstance(&(chip::Server::GetInstance()));
+
     // Connect the Requestor and Requestor Driver objects
     requestorCore.SetOtaRequestorDriver(&requestorUser);
 

src/app/clusters/ota-requestor/BDXDownloader.cpp

@@ -19,6 +19,8 @@
 #include "BDXDownloader.h"
 
 #include <lib/core/CHIPError.h>
+#include <lib/support/BufferReader.h>
+#include <lib/support/BytesToHex.h>
 #include <lib/support/CodeUtils.h>
 #include <protocols/bdx/BdxMessages.h>
 #include <system/SystemClock.h> /* TODO:(#12520) remove */
@@ -215,4 +217,63 @@ CHIP_ERROR BDXDownloader::HandleBdxEvent(const chip::bdx::TransferSession::Outpu
     return CHIP_NO_ERROR;
 }
 
+// TODO: Add unit tests for parsing BDX URI
+CHIP_ERROR BDXDownloader::ParseBdxUri(CharSpan uri, NodeId & nodeId, MutableCharSpan fileDesignator)
+{
+    // Check against minimum length of a valid BDX URI
+    if (uri.size() < kValidBdxUriMinLen)
+    {
+        return CHIP_ERROR_INVALID_STRING_LENGTH;
+    }
+
+    uint8_t readValue[kUriMaxLen];
+    Encoding::LittleEndian::Reader uriReader(reinterpret_cast<const uint8_t *>(uri.data()), uri.size());
+
+    // Check the scheme field matches the BDX prefix
+    memset(readValue, 0, sizeof(readValue));
+    ReturnErrorOnFailure(uriReader.ReadBytes(readValue, sizeof(bdxPrefix)).StatusCode());
+    ByteSpan expectedScheme(bdxPrefix, sizeof(bdxPrefix));
+    ByteSpan actualScheme(readValue, sizeof(bdxPrefix));
+    if (!expectedScheme.data_equal(actualScheme))
+    {
+        return CHIP_ERROR_INVALID_SCHEME_PREFIX;
+    }
+
+    // Extract the node ID from the authority field
+    memset(readValue, 0, sizeof(readValue));
+    ReturnErrorOnFailure(uriReader.ReadBytes(readValue, kNodeIdHexStringLen).StatusCode());
+    uint8_t buffer[kNodeIdHexStringLen];
+    if (Encoding::HexToBytes(reinterpret_cast<const char *>(readValue), kNodeIdHexStringLen, buffer, kNodeIdHexStringLen) == 0)
+    {
+        return CHIP_ERROR_INVALID_DESTINATION_NODE_ID;
+    }
+    nodeId = Encoding::BigEndian::Get64(buffer);
+    if (!IsOperationalNodeId(nodeId))
+    {
+        return CHIP_ERROR_INVALID_DESTINATION_NODE_ID;
+    }
+
+    // Verify the separator between authority and path fields
+    memset(readValue, 0, sizeof(readValue));
+    ReturnErrorOnFailure(uriReader.ReadBytes(readValue, sizeof(bdxSeparator)).StatusCode());
+    ByteSpan expectedSeparator(bdxSeparator, sizeof(bdxSeparator));
+    ByteSpan actualSeparator(readValue, sizeof(bdxSeparator));
+    if (!expectedSeparator.data_equal(actualSeparator))
+    {
+        return CHIP_ERROR_MISSING_URI_SEPARATOR;
+    }
+
+    // Extract file designator from the path field
+    size_t fileDesignatorLength = uriReader.Remaining();
+    memset(readValue, 0, sizeof(readValue));
+    ReturnErrorOnFailure(uriReader.ReadBytes(readValue, fileDesignatorLength).StatusCode());
+    size_t written = static_cast<size_t>(snprintf(fileDesignator.data(), fileDesignator.size(), "%s", readValue));
+    if (written != fileDesignatorLength)
+    {
+        return CHIP_ERROR_INTERNAL;
+    }
+
+    return CHIP_NO_ERROR;
+}
+
 } // namespace chip

src/app/clusters/ota-requestor/BDXDownloader.h

@@ -33,6 +33,13 @@
 
 namespace chip {
 
+// Constants for BDX URI parsing
+constexpr uint8_t bdxPrefix[]         = { 'b', 'd', 'x', ':', '/', '/' };
+constexpr uint8_t bdxSeparator[]      = { '/' };
+constexpr uint8_t kValidBdxUriMinLen  = 24;
+constexpr uint8_t kNodeIdHexStringLen = 16;
+constexpr size_t kUriMaxLen           = 256;
+
 class BDXDownloader : public chip::OTADownloader
 {
 public:
@@ -65,6 +72,11 @@ class BDXDownloader : public chip::OTADownloader
     CHIP_ERROR FetchNextData() override;
     // TODO: override SkipData
 
+    /**
+     * Validate the URI and parse the BDX URI for various fields
+     */
+    CHIP_ERROR ParseBdxUri(CharSpan uri, NodeId & nodeId, MutableCharSpan fileDesignator);
+
 private:
     void PollTransferSession();
     CHIP_ERROR HandleBdxEvent(const chip::bdx::TransferSession::OutputEvent & outEvent);

src/app/clusters/ota-requestor/OTARequestor.cpp

@@ -20,10 +20,7 @@
  * OTA Requestor logic is contained in this class.
  */
 
-#include <app/server/Server.h>
 #include <lib/core/CHIPEncoding.h>
-#include <lib/support/BytesToHex.h>
-#include <lib/support/CodeUtils.h>
 #include <platform/CHIPDeviceLayer.h>
 #include <zap-generated/CHIPClusters.h>
 
@@ -32,19 +29,14 @@
 
 namespace chip {
 
+using namespace app::Clusters::OtaSoftwareUpdateProvider::Commands;
 using bdx::TransferSession;
 
 // Global instance of the OTARequestorInterface.
 OTARequestorInterface * globalOTARequestorInstance = nullptr;
 
 constexpr uint32_t kImmediateStartDelayMs = 1; // Start the timer with this value when starting OTA "immediately"
 
-// Constants for BDX URI parsing
-constexpr char bdxPrefix[]               = "bdx://";
-constexpr char bdxSeparator[]            = "/";
-constexpr uint8_t kValidBdxUriMinLen     = 24;
-constexpr uint8_t kNodeIdHexStringMaxLen = 16;
-
 void SetRequestorInstance(OTARequestorInterface * instance)
 {
     globalOTARequestorInstance = instance;
@@ -92,55 +84,10 @@ static void LogQueryImageResponse(const QueryImageResponse::DecodableType & resp
     }
 }
 
-// TODO: Add unit tests for parsing BDX URI
-CHIP_ERROR OTARequestor::ParseBdxUri(CharSpan uri, NodeId & nodeId, CharSpan & fileDesignator)
+void StartDelayTimerHandler(chip::System::Layer * systemLayer, void * appState)
 {
-    // Check against minimum length of a valid BDX URI
-    if (uri.size() < kValidBdxUriMinLen)
-    {
-        ChipLogError(SoftwareUpdate, "Expect BDX URI to be at least %d in length, actual length is %zu", kValidBdxUriMinLen,
-                     uri.size());
-        return CHIP_ERROR_INVALID_STRING_LENGTH;
-    }
-
-    // Check the scheme field matches the BDX prefix
-    CharSpan expectedScheme(bdxPrefix, strlen(bdxPrefix));
-    if (!uri.SubSpan(0, strlen(bdxPrefix)).data_equal(expectedScheme))
-    {
-        ChipLogError(SoftwareUpdate, "Expect scheme to contain prefix %s, actual URI: %.*s", bdxPrefix,
-                     static_cast<int>(uri.size()), uri.data());
-        return CHIP_ERROR_INVALID_SCHEME_PREFIX;
-    }
-
-    // Extract the node ID from the authority field
-    CharSpan nodeIdString = uri.SubSpan(strlen(bdxPrefix), kNodeIdHexStringMaxLen);
-    uint8_t buffer[kNodeIdHexStringMaxLen];
-    if (chip::Encoding::HexToBytes(nodeIdString.data(), nodeIdString.size(), buffer, kNodeIdHexStringMaxLen) == 0)
-    {
-        ChipLogError(SoftwareUpdate, "Cannot convert image URI provider node ID: %.*s", static_cast<int>(nodeIdString.size()),
-                     nodeIdString.data());
-        return CHIP_ERROR_INVALID_DESTINATION_NODE_ID;
-    }
-
-    nodeId = chip::Encoding::BigEndian::Get64(buffer);
-    if (!IsOperationalNodeId(nodeId))
-    {
-        ChipLogError(SoftwareUpdate, "Image URI contains invalid node ID: 0x" ChipLogFormatX64, ChipLogValueX64(nodeId));
-        return CHIP_ERROR_INVALID_DESTINATION_NODE_ID;
-    }
-
-    // Verify the separator between authority and path fields
-    CharSpan separator(bdxSeparator, strlen(bdxSeparator));
-    if (!uri.SubSpan(strlen(bdxPrefix) + kNodeIdHexStringMaxLen, strlen(bdxSeparator)).data_equal(separator))
-    {
-        return CHIP_ERROR_MISSING_URI_SEPARATOR;
-    }
-
-    // Extract file designator from the path field
-    size_t fileDesignatorLength = uri.size() - (strlen(bdxPrefix) + kNodeIdHexStringMaxLen + strlen(bdxSeparator));
-    fileDesignator              = uri.SubSpan(uri.size() - fileDesignatorLength, fileDesignatorLength);
-
-    return CHIP_NO_ERROR;
+    VerifyOrReturn(appState != nullptr);
+    static_cast<OTARequestor *>(appState)->ConnectToProvider(OTARequestor::kQueryImage);
 }
 
 void OTARequestor::OnQueryImageResponse(void * context, const QueryImageResponse::DecodableType & response)
@@ -154,18 +101,22 @@ void OTARequestor::OnQueryImageResponse(void * context, const QueryImageResponse
     switch (response.status)
     {
     case EMBER_ZCL_OTA_QUERY_STATUS_UPDATE_AVAILABLE: {
+        // TODO: Add a method to OTARequestorDriver used to report error condictions
         VerifyOrReturn(response.imageURI.HasValue(), ChipLogError(SoftwareUpdate, "Update is available but no image URI present"));
 
         // Parse out the provider node ID and file designator from the image URI
-        NodeId nodeId = chip::kUndefinedNodeId;
-        CharSpan fileDesignator;
-        CHIP_ERROR err = requestorCore->ParseBdxUri(response.imageURI.Value(), nodeId, fileDesignator);
+        NodeId nodeId                         = kUndefinedNodeId;
+        char fileDesignatorBuffer[kUriMaxLen] = { 0 };
+        MutableCharSpan fileDesignator(fileDesignatorBuffer, kUriMaxLen);
+        CHIP_ERROR err = requestorCore->mBdxDownloader->ParseBdxUri(response.imageURI.Value(), nodeId, fileDesignator);
         VerifyOrReturn(err == CHIP_NO_ERROR,
-                       ChipLogError(SoftwareUpdate, "BDX image URI is invalid: %" CHIP_ERROR_FORMAT, err.Format()));
+                       ChipLogError(SoftwareUpdate, "Parse BDX image URI (%.*s) returned err=%" CHIP_ERROR_FORMAT,
+                                    static_cast<int>(response.imageURI.Value().size()), response.imageURI.Value().data(),
+                                    err.Format()));
         requestorCore->mProviderNodeId = nodeId;
 
-        // CSM should already be created for sending QueryImage command so use the same CSM based on the assumption that the
-        // provider node ID that will supply the OTA image is on the same fabric as the sender of the QueryImageResponse
+        // CSM should already be created for sending QueryImage command so use the same CSM since the
+        // provider node ID that will supply the OTA image must be on the same fabric as the sender of the QueryImageResponse
         requestorCore->ConnectToProvider(kStartBDX);
         break;
     }
@@ -174,6 +125,7 @@ void OTARequestor::OnQueryImageResponse(void * context, const QueryImageResponse
     case EMBER_ZCL_OTA_QUERY_STATUS_NOT_AVAILABLE:
         break;
     // TODO: Add download protocol not supported
+    // Issue #9524 should handle all response status appropriately
     default:
         break;
     }
@@ -227,7 +179,7 @@ EmberAfStatus OTARequestor::HandleAnnounceOTAProvider(
         return EMBER_ZCL_STATUS_FAILURE;
     }
 
-    ConnectToProvider(kQueryImage);
+    chip::DeviceLayer::SystemLayer().StartTimer(chip::System::Clock::Milliseconds32(msToStart), StartDelayTimerHandler, this);
 
     return EMBER_ZCL_STATUS_SUCCESS;
 }
@@ -253,26 +205,27 @@ CHIP_ERROR OTARequestor::SetupCASESessionManager(chip::FabricIndex fabricIndex)
     // CSM has not been setup so create a new instance of it
     if (mCASESessionManager == nullptr)
     {
-        Server * server         = &(chip::Server::GetInstance());
-        FabricInfo * fabricInfo = server->GetFabricTable().FindFabricWithIndex(fabricIndex);
+        FabricInfo * fabricInfo = mServer->GetFabricTable().FindFabricWithIndex(fabricIndex);
         if (fabricInfo == nullptr)
         {
             ChipLogError(SoftwareUpdate, "Did not find fabric for index %d", fabricIndex);
             return CHIP_ERROR_INVALID_ARGUMENT;
         }
 
         DeviceProxyInitParams initParams = {
-            .sessionManager = &(server->GetSecureSessionManager()),
-            .exchangeMgr    = &(server->GetExchangeManager()),
-            .idAllocator    = &(server->GetSessionIDAllocator()),
+            .sessionManager = &(mServer->GetSecureSessionManager()),
+            .exchangeMgr    = &(mServer->GetExchangeManager()),
+            .idAllocator    = &(mServer->GetSessionIDAllocator()),
             .fabricInfo     = fabricInfo,
+            .clientPool     = mServer->GetCASEClientPool(),
             // TODO: Determine where this should be instantiated
             .imDelegate = chip::Platform::New<chip::Controller::DeviceControllerInteractionModelDelegate>(),
         };
 
         CASESessionManagerConfig sessionManagerConfig = {
             .sessionInitParams = initParams,
             .dnsCache          = nullptr,
+            .devicePool        = mServer->GetDevicePool(),
         };
 
         mCASESessionManager = chip::Platform::New<chip::CASESessionManager>(sessionManagerConfig);
@@ -296,8 +249,8 @@ void OTARequestor::ConnectToProvider(OnConnectedAction onConnectedAction)
     // Set the action to take once connection is successfully established
     mOnConnectedAction = onConnectedAction;
 
-    ChipLogDetail(SoftwareUpdate, "Establishing session to provider node ID: 0x" ChipLogFormatX64,
-                  ChipLogValueX64(mProviderNodeId));
+    ChipLogDetail(SoftwareUpdate, "Establishing session to provider node ID 0x" ChipLogFormatX64 " on fabric index %d",
+                  ChipLogValueX64(mProviderNodeId), mProviderFabricIndex);
     err = mCASESessionManager->FindOrEstablishSession(mProviderNodeId, &mOnConnectedCallback, &mOnConnectionFailureCallback);
     VerifyOrReturn(err == CHIP_NO_ERROR,
                    ChipLogError(SoftwareUpdate, "Cannot establish connection to provider: %" CHIP_ERROR_FORMAT, err.Format()));
@@ -382,7 +335,8 @@ void OTARequestor::OnConnected(void * context, OperationalDeviceProxy * devicePr
 
             if (requestorCore->mExchangeCtx == nullptr)
             {
-                ChipLogError(BDX, "Unable to allocate ec: exchangeMgr=%p sessionExists? %u", exchangeMgr, session.HasValue());
+                ChipLogError(BDX, "Unable to allocate ec: exchangeMgr=%p sessionExists? %u, OTA progress cannot continue",
+                             exchangeMgr, session.HasValue());
                 return;
             }
         }