From c574fb9ed4375f3c90460a3e0e39de9e0ed3a42d Mon Sep 17 00:00:00 2001
From: Pankaj Garg <pgarg2@apple.com>
Date: Thu, 18 Nov 2021 09:00:35 -0800
Subject: [PATCH] address review comments

---
 src/controller/CHIPDeviceController.cpp       | 27 ++++++++++---------
 .../QRCodeSetupPayloadGenerator.h             |  3 +++
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
index 4697c41b25a7f0..fe36f599920314 100644
--- a/src/controller/CHIPDeviceController.cpp
+++ b/src/controller/CHIPDeviceController.cpp
@@ -382,8 +382,8 @@ void DeviceController::OnVIDReadResponse(void * context, uint16_t value)
     chip::Controller::BasicCluster cluster;
     cluster.Associate(device, kBasicClusterEndpoint);
 
-    if (cluster.ReadAttribute<chip::app::Clusters::Basic::Attributes::ProductID::TypeInfo>(
-            context, OnPIDReadResponse, OnVIDPIDReadFailureResponse) != CHIP_NO_ERROR)
+    if (cluster.ReadAttribute<app::Clusters::Basic::Attributes::ProductID::TypeInfo>(context, OnPIDReadResponse,
+                                                                                     OnVIDPIDReadFailureResponse) != CHIP_NO_ERROR)
     {
         ChipLogError(Controller, "Could not read PID for opening commissioning window");
         OnOpenPairingWindowFailureResponse(context, 0);
@@ -464,7 +464,7 @@ CHIP_ERROR DeviceController::OpenCommissioningWindowWithCallback(NodeId deviceId
         return CHIP_ERROR_INVALID_ARGUMENT;
     }
 
-    if (callback != nullptr && option != 0)
+    if (callback != nullptr && mCommissioningWindowOption != CommissioningWindowOption::kOriginalSetupCode)
     {
         OperationalDeviceProxy * device = mCASESessionManager->FindExistingSession(mDeviceWithCommissioningWindowOpen);
         VerifyOrReturnError(device != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
@@ -473,8 +473,8 @@ CHIP_ERROR DeviceController::OpenCommissioningWindowWithCallback(NodeId deviceId
         chip::Controller::BasicCluster cluster;
         cluster.Associate(device, kBasicClusterEndpoint);
 
-        return cluster.ReadAttribute<chip::app::Clusters::Basic::Attributes::VendorID::TypeInfo>(this, OnVIDReadResponse,
-                                                                                                 OnVIDPIDReadFailureResponse);
+        return cluster.ReadAttribute<app::Clusters::Basic::Attributes::VendorID::TypeInfo>(this, OnVIDReadResponse,
+                                                                                           OnVIDPIDReadFailureResponse);
     }
 
     return OpenCommissioningWindowInternal();
@@ -488,10 +488,6 @@ CHIP_ERROR DeviceController::OpenCommissioningWindowInternal()
     OperationalDeviceProxy * device = mCASESessionManager->FindExistingSession(mDeviceWithCommissioningWindowOpen);
     VerifyOrReturnError(device != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
 
-    std::string QRCode;
-    std::string manualPairingCode;
-    ByteSpan salt(Uint8::from_const_char(kSpake2pKeyExchangeSalt), strlen(kSpake2pKeyExchangeSalt));
-
     constexpr EndpointId kAdministratorCommissioningClusterEndpoint = 0;
 
     chip::Controller::AdministratorCommissioningCluster cluster;
@@ -502,6 +498,7 @@ CHIP_ERROR DeviceController::OpenCommissioningWindowInternal()
 
     if (mCommissioningWindowOption != CommissioningWindowOption::kOriginalSetupCode)
     {
+        ByteSpan salt(Uint8::from_const_char(kSpake2pKeyExchangeSalt), strlen(kSpake2pKeyExchangeSalt));
         bool randomSetupPIN = (mCommissioningWindowOption == CommissioningWindowOption::kTokenWithRandomPIN);
         PASEVerifier verifier;
 
@@ -518,11 +515,15 @@ CHIP_ERROR DeviceController::OpenCommissioningWindowInternal()
             successCallback, failureCallback, mCommissioningWindowTimeout, ByteSpan(serializedVerifier, sizeof(serializedVerifier)),
             mSetupPayload.discriminator, mCommissioningWindowIteration, salt, mPAKEVerifierID++));
 
-        ReturnErrorOnFailure(ManualSetupPayloadGenerator(mSetupPayload).payloadDecimalStringRepresentation(manualPairingCode));
-        ChipLogProgress(Controller, "Manual pairing code: [%s]", manualPairingCode.c_str());
+        char payloadBuffer[QRCodeBasicSetupPayloadGenerator::kMaxQRCodeBase38RepresentationLength];
+
+        MutableCharSpan manualCode(payloadBuffer);
+        ReturnErrorOnFailure(ManualSetupPayloadGenerator(mSetupPayload).payloadDecimalStringRepresentation(manualCode));
+        ChipLogProgress(Controller, "Manual pairing code: [%s]", payloadBuffer);
 
-        ReturnErrorOnFailure(QRCodeSetupPayloadGenerator(mSetupPayload).payloadBase38Representation(QRCode));
-        ChipLogProgress(Controller, "SetupQRCode: [%s]", QRCode.c_str());
+        MutableCharSpan QRCode(payloadBuffer);
+        ReturnErrorOnFailure(QRCodeBasicSetupPayloadGenerator(mSetupPayload).payloadBase38Representation(QRCode));
+        ChipLogProgress(Controller, "SetupQRCode: [%s]", payloadBuffer);
     }
     else
     {
diff --git a/src/setup_payload/QRCodeSetupPayloadGenerator.h b/src/setup_payload/QRCodeSetupPayloadGenerator.h
index daee1fa0aa512e..091b6df5b459d8 100644
--- a/src/setup_payload/QRCodeSetupPayloadGenerator.h
+++ b/src/setup_payload/QRCodeSetupPayloadGenerator.h
@@ -114,6 +114,9 @@ class QRCodeBasicSetupPayloadGenerator
      *               producing the requested string.
      */
     CHIP_ERROR payloadBase38Representation(MutableCharSpan & outBuffer);
+
+    // TODO - Find the optimal value for maximum length of QR Code Base38 string
+    static constexpr uint16_t kMaxQRCodeBase38RepresentationLength = 128;
 };
 
 } // namespace chip