Address review comments

project-chip · Oct 23, 2024 · ee410fc · ee410fc
1 parent 6387324
commit ee410fc
Show file tree

Hide file tree

Showing 10 changed files with 58 additions and 48 deletions.
diff --git a/examples/light-switch-app/light-switch-common/BUILD.gn b/examples/light-switch-app/light-switch-common/BUILD.gn
@@ -18,7 +18,7 @@ import("${chip_root}/src/app/icd/icd.gni")
 
 chip_data_model("light-switch-common") {
   if (chip_enable_icd_lit) {
-    zap_file = "lit-icd-light-switch-app.zap"
+    zap_file = "icd-lit-light-switch-app.zap"
   } else {
     zap_file = "light-switch-app.zap"
   }

diff --git a/...ch-common/lit-icd-light-switch-app.matter → ...ch-common/icd-lit-light-switch-app.matter b/...ch-common/lit-icd-light-switch-app.matter → ...ch-common/icd-lit-light-switch-app.matter
diff --git a/...witch-common/lit-icd-light-switch-app.zap → ...witch-common/icd-lit-light-switch-app.zap b/...witch-common/lit-icd-light-switch-app.zap → ...witch-common/icd-lit-light-switch-app.zap
diff --git a/examples/platform/silabs/BaseApplication.cpp b/examples/platform/silabs/BaseApplication.cpp
@@ -37,12 +37,12 @@
 #endif // QR_CODE_ENABLED
 #endif // DISPLAY_ENABLED
 
-#if defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER
+#if CHIP_CONFIG_ENABLE_ICD_SERVER
 #include <app/icd/server/ICDNotifier.h> // nogncheck
 #ifdef ENABLE_CHIP_SHELL
 #include <ICDShellCommands.h>
 #endif // ENABLE_CHIP_SHELL
-#endif // defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER
+#endif // CHIP_CONFIG_ENABLE_ICD_SERVER
 
 #include <ProvisionManager.h>
 #include <app/server/OnboardingCodesUtil.h>
@@ -125,7 +125,7 @@ app::Clusters::NetworkCommissioning::Instance
 bool sIsEnabled  = false;
 bool sIsAttached = false;
 
-#if !(defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER)
+#if !(CHIP_CONFIG_ENABLE_ICD_SERVER)
 bool sHaveBLEConnections = false;
 #endif // CHIP_CONFIG_ENABLE_ICD_SERVER
 
@@ -184,7 +184,7 @@ void BaseApplicationDelegate::OnCommissioningSessionStopped()
 
 void BaseApplicationDelegate::OnCommissioningWindowClosed()
 {
-#if (defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER) && SLI_SI917
+#if CHIP_CONFIG_ENABLE_ICD_SERVER && SLI_SI917
     if (!BaseApplication::GetProvisionStatus() && !isComissioningStarted)
     {
         int32_t status = wfx_power_save(RSI_SLEEP_MODE_8, STANDBY_POWER_SAVE_WITH_RAM_RETENTION);
@@ -311,9 +311,9 @@ CHIP_ERROR BaseApplication::Init()
 
 #ifdef ENABLE_CHIP_SHELL
 
-#if defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER
+#if CHIP_CONFIG_ENABLE_ICD_SERVER
     ICDCommands::RegisterCommands();
-#endif // defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER
+#endif // CHIP_CONFIG_ENABLE_ICD_SERVER
 
 #endif // ENABLE_CHIP_SHELL
 
@@ -354,7 +354,7 @@ void BaseApplication::FunctionEventHandler(AppEvent * aEvent)
     {
         // The factory reset sequence was in motion. The cancellation window expired.
         // Factory Reset the device now.
-#if defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER
+#if CHIP_CONFIG_ENABLE_ICD_SERVER
         StopStatusLEDTimer();
 #endif // CHIP_CONFIG_ENABLE_ICD_SERVER
 
@@ -412,7 +412,7 @@ bool BaseApplication::ActivateStatusLedPatterns()
     }
 #endif // MATTER_DM_PLUGIN_IDENTIFY_SERVER
 
-#if !(defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER)
+#if !(CHIP_CONFIG_ENABLE_ICD_SERVER)
     // Identify Patterns have priority over Status patterns
     if (!isPatternSet)
     {
@@ -468,7 +468,7 @@ void BaseApplication::LightEventHandler()
     // locked while these values are queried.  However we use a non-blocking
     // lock request (TryLockCHIPStack()) to avoid blocking other UI activities
     // when the CHIP task is busy (e.g. with a long crypto operation).
-#if !(defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER)
+#if !(CHIP_CONFIG_ENABLE_ICD_SERVER)
     if (PlatformMgr().TryLockChipStack())
     {
 #ifdef SL_WIFI
@@ -563,7 +563,7 @@ void BaseApplication::ButtonHandler(AppEvent * aEvent)
             else
             {
                 ChipLogProgress(AppServer, "Network is already provisioned, Ble advertisement not enabled");
-#if defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER
+#if CHIP_CONFIG_ENABLE_ICD_SERVER
                 // Temporarily claim network activity, until we implement a "user trigger" reason for ICD wakeups.
                 PlatformMgr().ScheduleWork([](intptr_t) { ICDNotifier::GetInstance().NotifyNetworkActivityNotification(); });
 #endif // CHIP_CONFIG_ENABLE_ICD_SERVER
@@ -611,7 +611,7 @@ void BaseApplication::StartFactoryResetSequence()
     StartFunctionTimer(FACTORY_RESET_CANCEL_WINDOW_TIMEOUT);
 
     sIsFactoryResetTriggered = true;
-#if defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER
+#if CHIP_CONFIG_ENABLE_ICD_SERVER
     StartStatusLEDTimer();
 #endif // CHIP_CONFIG_ENABLE_ICD_SERVER
 
@@ -627,7 +627,7 @@ void BaseApplication::CancelFactoryResetSequence()
 {
     CancelFunctionTimer();
 
-#if defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER
+#if CHIP_CONFIG_ENABLE_ICD_SERVER
     StopStatusLEDTimer();
 #endif
     if (sIsFactoryResetTriggered)
@@ -664,7 +664,7 @@ void BaseApplication::OnIdentifyStart(Identify * identify)
 {
     ChipLogProgress(Zcl, "onIdentifyStart");
 
-#if defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER
+#if CHIP_CONFIG_ENABLE_ICD_SERVER
     StartStatusLEDTimer();
 #endif
 }
@@ -673,7 +673,7 @@ void BaseApplication::OnIdentifyStop(Identify * identify)
 {
     ChipLogProgress(Zcl, "onIdentifyStop");
 
-#if defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER
+#if CHIP_CONFIG_ENABLE_ICD_SERVER
     StopStatusLEDTimer();
 #endif
 }
@@ -683,7 +683,7 @@ void BaseApplication::OnTriggerIdentifyEffectCompleted(chip::System::Layer * sys
     ChipLogProgress(Zcl, "Trigger Identify Complete");
     sIdentifyEffect = Clusters::Identify::EffectIdentifierEnum::kStopEffect;
 
-#if defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER
+#if CHIP_CONFIG_ENABLE_ICD_SERVER
     StopStatusLEDTimer();
 #endif
 }
@@ -697,7 +697,7 @@ void BaseApplication::OnTriggerIdentifyEffect(Identify * identify)
         ChipLogDetail(AppServer, "Identify Effect Variant unsupported. Using default");
     }
 
-#if defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER
+#if CHIP_CONFIG_ENABLE_ICD_SERVER
     StartStatusLEDTimer();
 #endif
 
@@ -766,7 +766,7 @@ void BaseApplication::UpdateLCDStatusScreen(bool withChipStackLock)
     status.connected   = enabled && attached;
     status.advertising = chip::Server::GetInstance().GetCommissioningWindowManager().IsCommissioningWindowOpen();
     status.nbFabric    = chip::Server::GetInstance().GetFabricTable().FabricCount();
-#if defined(CHIP_CONFIG_ENABLE_ICD_SERVER) && CHIP_CONFIG_ENABLE_ICD_SERVER
+#if CHIP_CONFIG_ENABLE_ICD_SERVER
     status.icdMode = (ICDConfigurationData::GetInstance().GetICDMode() == ICDConfigurationData::ICDMode::SIT)
         ? SilabsLCD::ICDMode_e::SIT
         : SilabsLCD::ICDMode_e::LIT;

diff --git a/...les/platform/silabs/shell/MatterShell.cpp → examples/platform/silabs/MatterShell.cpp b/...les/platform/silabs/shell/MatterShell.cpp → examples/platform/silabs/MatterShell.cpp
diff --git a/examples/platform/silabs/shell/MatterShell.h → examples/platform/silabs/MatterShell.h b/examples/platform/silabs/shell/MatterShell.h → examples/platform/silabs/MatterShell.h
diff --git a/examples/platform/silabs/SiWx917/BUILD.gn b/examples/platform/silabs/SiWx917/BUILD.gn
@@ -91,6 +91,22 @@ source_set("test-event-trigger") {
   ]
 }
 
+source_set("matter-shell") {
+  defines = [ "ENABLE_CHIP_SHELL" ]
+
+  sources = [ "${silabs_common_plat_dir}/MatterShell.cpp" ]
+  include_dirs = [
+    ".",
+    "${silabs_common_plat_dir}",
+  ]
+
+  public_deps = [
+    "${chip_root}/examples/shell/shell_common:shell_common",
+    "${chip_root}/src/lib/shell:shell",
+    "${chip_root}/src/lib/shell:shell_core",
+  ]
+}
+
 config("siwx917-common-config") {
   defines = [ "OTA_PERIODIC_TIMEOUT=${ota_periodic_query_timeout_sec}" ]
 
@@ -208,12 +224,13 @@ source_set("siwx917-common") {
   }
 
   if (chip_build_libshell) {
-    deps += [ "${silabs_common_plat_dir}/shell:matter" ]
+    deps += [ ":matter-shell" ]
 
     if (chip_enable_icd_server) {
       deps += [ "${silabs_common_plat_dir}/shell:icd" ]
     }
   }
+  }
 
   # DIC
   if (enable_dic) {

diff --git a/examples/platform/silabs/efr32/BUILD.gn b/examples/platform/silabs/efr32/BUILD.gn
@@ -123,6 +123,22 @@ source_set("openthread_core_config_efr32_chip_examples") {
   }
 }
 
+source_set("matter-shell") {
+  defines = [ "ENABLE_CHIP_SHELL" ]
+
+  sources = [ "${silabs_common_plat_dir}/MatterShell.cpp" ]
+  include_dirs = [
+    ".",
+    "${silabs_common_plat_dir}",
+  ]
+
+  public_deps = [
+    "${chip_root}/examples/shell/shell_common:shell_common",
+    "${chip_root}/src/lib/shell:shell",
+    "${chip_root}/src/lib/shell:shell_core",
+  ]
+}
+
 config("efr32-common-config") {
   defines = [ "OTA_PERIODIC_TIMEOUT=${ota_periodic_query_timeout_sec}" ]
 
@@ -276,7 +292,7 @@ source_set("efr32-common") {
   }
 
   if (chip_build_libshell) {
-    deps += [ "${silabs_common_plat_dir}/shell:matter" ]
+    deps += [ ":matter-shell" ]
 
     if (chip_enable_icd_server) {
       deps += [ "${silabs_common_plat_dir}/shell:icd" ]

diff --git a/examples/platform/silabs/shell/BUILD.gn b/examples/platform/silabs/shell/BUILD.gn
@@ -17,27 +17,10 @@ import("//build_overrides/efr32_sdk.gni")
 import("${chip_root}/src/app/icd/icd.gni")
 import("${chip_root}/third_party/silabs/silabs_board.gni")
 
-config("shell-config") {
-  defines = [ "ENABLE_CHIP_SHELL" ]
-
-  include_dirs = [ "." ]
-}
-
-# Common Matter Shell fines Silabs sample apps.
-# This source_set needs to be added as a dependency to all sources_sets that implement shell commands
-source_set("matter") {
-  sources = [
-    "MatterShell.cpp",
-    "MatterShell.h",
-  ]
-
-  public_deps = [
-    "${chip_root}/examples/shell/shell_common:shell_common",
-    "${chip_root}/src/lib/shell:shell",
-    "${chip_root}/src/lib/shell:shell_core",
-  ]
-
-  public_configs = [ ":shell-config" ]
+# This is necessary since the BUILD.gn for both platforms are still split
+shell_dependency_path = "${chip_root}/examples/platform/silabs/efr32"
+if (use_SiWx917) {
+  shell_dependency_path = "${chip_root}/examples/platform/silabs/SiWx917"
 }
 
 source_set("icd") {
@@ -46,5 +29,5 @@ source_set("icd") {
     "ICDShellCommands.h",
   ]
 
-  deps = [ ":matter" ]
+  deps = [ "${shell_dependency_path}:matter-shell" ]
 }
diff --git a/src/lib/shell/BUILD.gn b/src/lib/shell/BUILD.gn
@@ -58,15 +58,9 @@ static_library("shell") {
   } else if (chip_device_platform == "efr32" ||
              chip_device_platform == "SiWx917") {
     sources += [
-      # Temporary workaround to prevent a circular dependecy while restructuring the silabs shell files
-      # Due to some odd dependency issue, we need the source file and the include_dir for the build to pass.
-      "${chip_root}/examples/platform/silabs/shell/MatterShell.h",
       "MainLoopSilabs.cpp",
       "streamer_silabs.cpp",
     ]
-
-    # Temporary workaround to prevent a circular dependecy while restructuring the silabs shell files
-    include_dirs = [ "${chip_root}/examples/platform/silabs/shell/" ]
   } else if (chip_device_platform == "nxp") {
     sources += [
       "MainLoopDefault.cpp",