Fix CI failures and address code review comments

- Add unit tests for new storage APIs added
- Make sure driver restores the core states if there is a problem detected at initialization

config/standalone/CHIPProjectConfig.h

@@ -74,7 +74,7 @@
 #endif
 
 #ifndef CHIP_DEVICE_CONFIG_DEVICE_SOFTWARE_VERSION_STRING
-#define CHIP_DEVICE_CONFIG_DEVICE_SOFTWARE_VERSION_STRING "v1.0"
+#define CHIP_DEVICE_CONFIG_DEVICE_SOFTWARE_VERSION_STRING "1.0"
 #endif
 
 //

examples/lighting-app/nxp/k32w/k32w0/main/AppTask.cpp

@@ -127,7 +127,7 @@ CHIP_ERROR AppTask::Init()
     gRequestorCore.Init(Server::GetInstance(), gRequestorStorage, gRequestorUser, gDownloader);
     gRequestorUser.Init(&gRequestorCore, &gImageProcessor);
 
-    gImageProcessor.SetOTAImageFile(CharSpan("test.txt"));
+    gImageProcessor.SetOTAImageFile("test.txt");
     gImageProcessor.SetOTADownloader(&gDownloader);
 
     // Connect the gDownloader and Image Processor objects

examples/ota-requestor-app/efr32/src/AppTask.cpp

@@ -542,7 +542,7 @@ void AppTask::InitOTARequestor()
 
     gRequestorUser.Init(&gRequestorCore, &gImageProcessor);
 
-    gImageProcessor.SetOTAImageFile(CharSpan("test.txt"));
+    gImageProcessor.SetOTAImageFile("test.txt");
     gImageProcessor.SetOTADownloader(&gDownloader);
 
     // Connect the Downloader and Image Processor objects

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

@@ -139,7 +139,7 @@ static void InitOTARequestor(void)
     gRequestorCore.Init(chip::Server::GetInstance(), gRequestorStorage, gRequestorUser, gDownloader);
     gRequestorUser.Init(&gRequestorCore, &gImageProcessor);
 
-    gImageProcessor.SetOTAImageFile(CharSpan::fromCharString(gOtaDownloadPath));
+    gImageProcessor.SetOTAImageFile(gOtaDownloadPath);
     gImageProcessor.SetOTADownloader(&gDownloader);
 
     // Set the image processor instance used for handling image being downloaded
@@ -223,9 +223,7 @@ int main(int argc, char * argv[])
             return -1;
         }
 
-        char execFilePathBuf[kMaxFilePathSize];
-        strncpy(execFilePathBuf, kImageExecPath, strlen(kImageExecPath));
-        argv[0] = execFilePathBuf;
+        argv[0] = kImageExecPath;
         execv(argv[0], argv);
 
         // If successfully executing the new iamge, execv should not return

examples/platform/efr32/OTAConfig.cpp

@@ -79,7 +79,7 @@ void OTAConfig::Init()
     gRequestorUser.SetPeriodicQueryTimeout(OTA_PERIODIC_TIMEOUT);
     gRequestorUser.Init(&gRequestorCore, &gImageProcessor);
 
-    gImageProcessor.SetOTAImageFile(chip::CharSpan("test.txt"));
+    gImageProcessor.SetOTAImageFile("test.txt");
     gImageProcessor.SetOTADownloader(&gDownloader);
 
     // Connect the Downloader and Image Processor objects

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

@@ -70,12 +70,23 @@ void GenericOTARequestorDriver::Init(OTARequestorInterface * requestor, OTAImage
             if (error != CHIP_NO_ERROR)
             {
                 ChipLogError(SoftwareUpdate, "Failed to confirm image: %" CHIP_ERROR_FORMAT, error.Format());
+                mRequestor->Reset();
                 return;
             }
 
             mRequestor->NotifyUpdateApplied();
         });
     }
+    else if ((mRequestor->GetCurrentUpdateState() != OTAUpdateStateEnum::kIdle))
+    {
+        // Not running a new image for the first time but also not in the idle state may indicate there is a problem
+        mRequestor->Reset();
+    }
+    else
+    {
+        // Start the first periodic query timer
+        StartDefaultProviderTimer();
+    }
 }
 
 bool GenericOTARequestorDriver::CanConsent()

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

@@ -107,17 +107,12 @@ void OTARequestor::InitState(intptr_t context)
     OTARequestor * requestorCore = reinterpret_cast<OTARequestor *>(context);
     VerifyOrDie(requestorCore != nullptr);
 
-    if (requestorCore->mCurrentUpdateState != OTAUpdateStateEnum::kApplying)
-    {
-        // This results in the initial periodic timer kicking off
-        requestorCore->RecordNewUpdateState(OTAUpdateStateEnum::kIdle, OTAChangeReasonEnum::kSuccess);
-    }
-    else
-    {
-        // This may have been a reboot from applying an image
-        OtaRequestorServerSetUpdateState(requestorCore->mCurrentUpdateState);
-    }
-
+    // This initialization may occur due to the following:
+    //   1) Regular boot up - the states should already be correct
+    //   2) Reboot from applying an image - once the image has been confirmed, the provider will be notified of the new version and
+    //   all relevant states will reset for a new OTA update. If the image cannot be confirmed, the driver will be responsible for
+    //   resetting the states appropriately, including the current update state.
+    OtaRequestorServerSetUpdateState(requestorCore->mCurrentUpdateState);
     OtaRequestorServerSetUpdateStateProgress(app::DataModel::NullNullable);
 }
 
@@ -280,6 +275,17 @@ void OTARequestor::OnNotifyUpdateAppliedFailure(void * context, CHIP_ERROR error
     requestorCore->RecordErrorUpdateState(UpdateFailureState::kNotifying, error);
 }
 
+void OTARequestor::Reset()
+{
+    mProviderLocation.ClearValue();
+    mUpdateToken.reduce_size(0);
+    RecordNewUpdateState(OTAUpdateStateEnum::kIdle, OTAChangeReasonEnum::kSuccess);
+    mTargetVersion = 0;
+
+    // Persist in case of a reboot or crash
+    StoreCurrentUpdateInfo();
+}
+
 void OTARequestor::Shutdown(void)
 {
     mServer->DispatchShutDownAndStopEventLoop();
@@ -841,7 +847,7 @@ void OTARequestor::StoreCurrentUpdateInfo()
         error = mStorage->ClearCurrentProviderLocation();
     }
 
-    if (error == CHIP_NO_ERROR)
+    if ((error == CHIP_NO_ERROR) || (error == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND))
     {
         if (mUpdateToken.size() > 0)
         {
@@ -853,12 +859,12 @@ void OTARequestor::StoreCurrentUpdateInfo()
         }
     }
 
-    if (error == CHIP_NO_ERROR)
+    if ((error == CHIP_NO_ERROR) || (error == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND))
     {
         error = mStorage->StoreCurrentUpdateState(mCurrentUpdateState);
     }
 
-    if (error == CHIP_NO_ERROR)
+    if ((error == CHIP_NO_ERROR) || (error == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND))
     {
         if (mTargetVersion > 0)
         {
@@ -870,7 +876,7 @@ void OTARequestor::StoreCurrentUpdateInfo()
         }
     }
 
-    if (error != CHIP_NO_ERROR)
+    if ((error != CHIP_NO_ERROR) && (error != CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND))
     {
         ChipLogError(SoftwareUpdate, "Failed to store current update: %" CHIP_ERROR_FORMAT, error.Format());
     }
@@ -894,7 +900,7 @@ void OTARequestor::LoadCurrentUpdateInfo()
 
     if (mStorage->LoadCurrentUpdateState(mCurrentUpdateState) != CHIP_NO_ERROR)
     {
-        mCurrentUpdateState = OTAUpdateStateEnum::kUnknown;
+        mCurrentUpdateState = OTAUpdateStateEnum::kIdle;
     }
 
     if (mStorage->LoadTargetVersion(mTargetVersion) != CHIP_NO_ERROR)
@@ -903,17 +909,6 @@ void OTARequestor::LoadCurrentUpdateInfo()
     }
 }
 
-void OTARequestor::Reset()
-{
-    mProviderLocation.ClearValue();
-    mUpdateToken.reduce_size(0);
-    RecordNewUpdateState(OTAUpdateStateEnum::kIdle, OTAChangeReasonEnum::kSuccess);
-    mTargetVersion = 0;
-
-    // Persist in case of a reboot or crash
-    StoreCurrentUpdateInfo();
-}
-
 // Invoked when the device becomes commissioned
 void OTARequestor::OnCommissioningCompleteRequestor(const DeviceLayer::ChipDeviceEvent * event, intptr_t arg)
 {

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

@@ -40,6 +40,7 @@ class OTARequestor : public OTARequestorInterface, public BDXDownloader::StateDe
     OTARequestor() : mOnConnectedCallback(OnConnected, this), mOnConnectionFailureCallback(OnConnectionFailure, this) {}
 
     //////////// OTARequestorInterface Implementation ///////////////
+    void Reset(void) override;
     void Shutdown(void) override;
 
     EmberAfStatus HandleAnnounceOTAProvider(
@@ -264,11 +265,6 @@ class OTARequestor : public OTARequestorInterface, public BDXDownloader::StateDe
      */
     void LoadCurrentUpdateInfo();
 
-    /**
-     * Reset the states for the next OTA update
-     */
-    void Reset();
-
     /**
      * Session connection callbacks
      */

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

@@ -153,6 +153,9 @@ class OTARequestorInterface
         kWrongState        = 2
     };
 
+    // Reset any relevant states
+    virtual void Reset(void) = 0;
+
     // Perform any clean up necessary
     virtual void Shutdown(void) = 0;
 

src/app/tests/TestDefaultOTARequestorStorage.cpp

@@ -147,10 +147,49 @@ void TestUpdateToken(nlTestSuite * inSuite, void * inContext)
     NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR != otaStorage.LoadUpdateToken(readUpdateToken));
 }
 
+void TestCurrentUpdateState(nlTestSuite * inSuite, void * inContext)
+{
+    TestPersistentStorageDelegate persistentStorage;
+    DefaultOTARequestorStorage otaStorage;
+    otaStorage.Init(persistentStorage);
+
+    OTARequestorStorage::OTAUpdateStateEnum updateState = OTARequestorStorage::OTAUpdateStateEnum::kApplying;
+
+    NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == otaStorage.StoreCurrentUpdateState(updateState));
+
+    updateState = OTARequestorStorage::OTAUpdateStateEnum::kUnknown;
+
+    NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == otaStorage.LoadCurrentUpdateState(updateState));
+    NL_TEST_ASSERT(inSuite, updateState == OTARequestorStorage::OTAUpdateStateEnum::kApplying);
+    NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == otaStorage.ClearCurrentUpdateState());
+    NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR != otaStorage.LoadCurrentUpdateState(updateState));
+}
+
+void TestTargetVersion(nlTestSuite * inSuite, void * inContext)
+{
+    TestPersistentStorageDelegate persistentStorage;
+    DefaultOTARequestorStorage otaStorage;
+    otaStorage.Init(persistentStorage);
+
+    uint32_t targetVersion = 2;
+
+    NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == otaStorage.StoreTargetVersion(targetVersion));
+
+    targetVersion = 0;
+
+    NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == otaStorage.LoadTargetVersion(targetVersion));
+    NL_TEST_ASSERT(inSuite, targetVersion == 2);
+    NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == otaStorage.ClearTargetVersion());
+    NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR != otaStorage.LoadTargetVersion(targetVersion));
+}
+
 const nlTest sTests[] = { NL_TEST_DEF("Test default providers", TestDefaultProviders),
                           NL_TEST_DEF("Test default providers (empty list)", TestDefaultProvidersEmpty),
                           NL_TEST_DEF("Test current provider location", TestCurrentProviderLocation),
-                          NL_TEST_DEF("Test update token", TestUpdateToken), NL_TEST_SENTINEL() };
+                          NL_TEST_DEF("Test update token", TestUpdateToken),
+                          NL_TEST_DEF("Test current update state", TestCurrentUpdateState),
+                          NL_TEST_DEF("Test target version", TestTargetVersion),
+                          NL_TEST_SENTINEL() };
 
 int TestSetup(void * inContext)
 {

src/controller/python/test/test_scripts/base.py

@@ -576,8 +576,8 @@ def TestReadBasicAttributes(self, nodeid: int, endpoint: int, group: int):
             "Location": "XX",
             "HardwareVersion": 0,
             "HardwareVersionString": "TEST_VERSION",
-            "SoftwareVersion": 0,
-            "SoftwareVersionString": "prerelease",
+            "SoftwareVersion": 1,
+            "SoftwareVersionString": "1.0",
         }
         failed_zcl = {}
         for basic_attr, expected_value in basic_cluster_attrs.items():

src/platform/CYW30739/PlatformManagerImpl.cpp

@@ -127,6 +127,11 @@ CHIP_ERROR PlatformManagerImpl::_StartEventLoopTask(void)
     return err;
 }
 
+CHIP_ERROR PlatformManagerImpl::_StopEventLoopTask()
+{
+    return CHIP_NO_ERROR;
+}
+
 void PlatformManagerImpl::_LockChipStack(void)
 {
     const wiced_result_t result = wiced_rtos_lock_mutex(mMutex);

src/platform/EFR32/OTAImageProcessorImpl.h

@@ -40,7 +40,7 @@ class OTAImageProcessorImpl : public OTAImageProcessorInterface
     CHIP_ERROR ConfirmCurrentImage() override { return CHIP_NO_ERROR; }
 
     void SetOTADownloader(OTADownloader * downloader) { mDownloader = downloader; }
-    void SetOTAImageFile(CharSpan name) { mImageFile = name; }
+    void SetOTAImageFile(const char * imageFile) { mImageFile = imageFile; }
 
 private:
     //////////// Actual handlers for the OTAImageProcessorInterface ///////////////
@@ -66,7 +66,7 @@ class OTAImageProcessorImpl : public OTAImageProcessorInterface
     MutableByteSpan mBlock;
     OTADownloader * mDownloader;
     OTAImageHeaderParser mHeaderParser;
-    CharSpan mImageFile;
+    const char * mImageFile = nullptr;
 };
 
 } // namespace chip

src/platform/Linux/OTAImageProcessorImpl.cpp

@@ -27,7 +27,7 @@ namespace chip {
 
 CHIP_ERROR OTAImageProcessorImpl::PrepareDownload()
 {
-    if (mImageFile.empty())
+    if (mImageFile == nullptr)
     {
         ChipLogError(SoftwareUpdate, "Invalid output image file supplied");
         return CHIP_ERROR_INTERNAL;
@@ -51,7 +51,7 @@ CHIP_ERROR OTAImageProcessorImpl::Apply()
 
 CHIP_ERROR OTAImageProcessorImpl::Abort()
 {
-    if (mImageFile.empty())
+    if (mImageFile == nullptr)
     {
         ChipLogError(SoftwareUpdate, "Invalid output image file supplied");
         return CHIP_ERROR_INTERNAL;
@@ -127,10 +127,10 @@ void OTAImageProcessorImpl::HandlePrepareDownload(intptr_t context)
         return;
     }
 
-    unlink(imageProcessor->mImageFile.data());
+    unlink(imageProcessor->mImageFile);
 
     imageProcessor->mHeaderParser.Init();
-    imageProcessor->mOfs.open(imageProcessor->mImageFile.data(), std::ofstream::out | std::ofstream::ate | std::ofstream::app);
+    imageProcessor->mOfs.open(imageProcessor->mImageFile, std::ofstream::out | std::ofstream::ate | std::ofstream::app);
     if (!imageProcessor->mOfs.good())
     {
         imageProcessor->mDownloader->OnPreparedForDownload(CHIP_ERROR_OPEN_FAILED);
@@ -151,7 +151,7 @@ void OTAImageProcessorImpl::HandleFinalize(intptr_t context)
     imageProcessor->mOfs.close();
     imageProcessor->ReleaseBlock();
 
-    ChipLogProgress(SoftwareUpdate, "OTA image downloaded to %s", imageProcessor->mImageFile.data());
+    ChipLogProgress(SoftwareUpdate, "OTA image downloaded to %s", imageProcessor->mImageFile);
 }
 
 void OTAImageProcessorImpl::HandleApply(intptr_t context)
@@ -162,9 +162,9 @@ void OTAImageProcessorImpl::HandleApply(intptr_t context)
     OTARequestorInterface * requestor = chip::GetRequestorInstance();
     VerifyOrReturn(requestor != nullptr);
 
-    // Move the downloaded image to th location where the new image is to be executed from
+    // Move the downloaded image to the location where the new image is to be executed from
     unlink(kImageExecPath);
-    rename(imageProcessor->mImageFile.data(), kImageExecPath);
+    rename(imageProcessor->mImageFile, kImageExecPath);
     chmod(kImageExecPath, S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH);
 
     // Shutdown the stack and expect to boot into the new image once shutdown is complete
@@ -180,7 +180,7 @@ void OTAImageProcessorImpl::HandleAbort(intptr_t context)
     }
 
     imageProcessor->mOfs.close();
-    unlink(imageProcessor->mImageFile.data());
+    unlink(imageProcessor->mImageFile);
     imageProcessor->ReleaseBlock();
 }
 

src/platform/Linux/OTAImageProcessorImpl.h

@@ -28,7 +28,7 @@
 namespace chip {
 
 // Full file path to where the new image will be executed from post-download
-static constexpr char kImageExecPath[] = "/tmp/ota.update";
+static char kImageExecPath[] = "/tmp/ota.update";
 
 class OTAImageProcessorImpl : public OTAImageProcessorInterface
 {
@@ -43,7 +43,7 @@ class OTAImageProcessorImpl : public OTAImageProcessorInterface
     CHIP_ERROR ConfirmCurrentImage() override;
 
     void SetOTADownloader(OTADownloader * downloader) { mDownloader = downloader; }
-    void SetOTAImageFile(CharSpan name) { mImageFile = name; }
+    void SetOTAImageFile(const char * imageFile) { mImageFile = imageFile; }
 
 private:
     //////////// Actual handlers for the OTAImageProcessorInterface ///////////////
@@ -69,7 +69,7 @@ class OTAImageProcessorImpl : public OTAImageProcessorInterface
     MutableByteSpan mBlock;
     OTADownloader * mDownloader;
     OTAImageHeaderParser mHeaderParser;
-    CharSpan mImageFile;
+    const char * mImageFile = nullptr;
 };
 
 } // namespace chip