Addressed review comments

shripad621git · Oct 6, 2023 · dff60d0 · dff60d0
1 parent 776bec0
commit dff60d0
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 25 deletions.
diff --git a/src/tracing/esp32_trace/BUILD.gn b/src/tracing/esp32_trace/BUILD.gn
@@ -20,6 +20,10 @@ config("tracing") {
   include_dirs = [ "include" ]
 }
 
+declare_args() {
+  matter_enable_esp_insights_system_stats = false
+}
+
 static_library("backend") {
   output_name = "libEsp32TracingBackend"
   output_dir = "${root_out_dir}/lib"
@@ -28,6 +32,17 @@ static_library("backend") {
     "esp32_tracing.cpp",
     "esp32_tracing.h",
   ]
+  if (matter_enable_esp_insights_system_stats) {
+    sources += [
+      "insights_sys_stats.cpp",
+      "insights_sys_stats.h",
+    ]
+    deps = [
+      "${chip_root}/src/lib/core",
+      "${chip_root}/src/platform",
+      "${chip_root}/src/system",
+    ]
+  }
   public_deps = [ "${chip_root}/src/tracing" ]
 }
 

diff --git a/src/tracing/esp32_trace/include/matter/tracing/insights_sys_stats.cpp b/src/tracing/esp32_trace/include/matter/tracing/insights_sys_stats.cpp
@@ -33,19 +33,19 @@ InsightsSystemMetrics & InsightsSystemMetrics::GetInstance()
     return instance;
 }
 
-void InsightsSystemMetrics::SamplingIntervalHandler(Layer * systemLayer, void * context)
+void InsightsSystemMetrics::SamplingHandler(Layer * systemLayer, void * context)
 {
     auto instance            = static_cast<InsightsSystemMetrics *>(context);
     count_t * highwatermarks = GetHighWatermarks();
     for (int i = 0; i < System::Stats::kNumEntries; i++)
     {
-        esp_err_t err = esp_diag_metrics_add_uint(instance->mLabels[i], static_cast<int>(highwatermarks[i]));
+        esp_err_t err = esp_diag_metrics_add_uint(instance->mLabels[i], static_cast<uint32_t>(highwatermarks[i]));
         if (err != ESP_OK)
         {
             ESP_LOGE(kTag, "Failed to add the metric:%s, err:%d", instance->mLabels[i], err);
         }
     }
-    DeviceLayer::SystemLayer().StartTimer(instance->GetSamplingInterval(), SamplingIntervalHandler, instance);
+    DeviceLayer::SystemLayer().StartTimer(instance->GetSamplingInterval(), SamplingHandler, instance);
 }
 
 CHIP_ERROR InsightsSystemMetrics::Unregister()
@@ -67,39 +67,38 @@ CHIP_ERROR InsightsSystemMetrics::Unregister()
             mLabels[i] = nullptr;
         }
     }
+    mRegistered = false;
     return CHIP_NO_ERROR;
 }
 
-void InsightsSystemMetrics::CancelSamplingInterval(intptr_t arg)
+void InsightsSystemMetrics::SetSamplingHandler(intptr_t arg)
 {
-    auto instance = static_cast<InsightsSystemMetrics *>(reinterpret_cast<void *>(arg));
-    DeviceLayer::SystemLayer().CancelTimer(SamplingIntervalHandler, instance);
-}
-
-CHIP_ERROR InsightsSystemMetrics::SetSamplingInterval(chip::System::Clock::Timeout aTimeout)
-{
-    if (!mRegistered)
-    {
-        return CHIP_ERROR_INCORRECT_STATE;
-    }
+    InsightsSystemMetrics * instance = reinterpret_cast<InsightsSystemMetrics *>(arg);
 
-    if (aTimeout == System::Clock::kZero)
+    if (instance->mTimeout == System::Clock::kZero)
     {
-        DeviceLayer::PlatformMgr().ScheduleWork(CancelSamplingInterval, reinterpret_cast<intptr_t>(this));
+        DeviceLayer::SystemLayer().CancelTimer(SamplingHandler, instance);
     }
-    else if (aTimeout != mTimeout)
+    else
     {
-        DeviceLayer::PlatformMgr().ScheduleWork(CancelSamplingInterval, reinterpret_cast<intptr_t>(this));
-        mTimeout       = aTimeout;
-        CHIP_ERROR err = DeviceLayer::SystemLayer().StartTimer(mTimeout, SamplingIntervalHandler, this);
+        DeviceLayer::SystemLayer().CancelTimer(SamplingHandler, instance);
+        CHIP_ERROR err = DeviceLayer::SystemLayer().StartTimer(instance->mTimeout, SamplingHandler, instance);
         if (err != CHIP_NO_ERROR)
         {
             ESP_LOGE(kTag, "Failed to start the new timer %" CHIP_ERROR_FORMAT, err.Format());
-            return err;
         }
     }
+}
 
-    return CHIP_NO_ERROR;
+CHIP_ERROR InsightsSystemMetrics::SetSamplingInterval(chip::System::Clock::Timeout aTimeout)
+{
+    if (!mRegistered)
+    {
+        return CHIP_ERROR_INCORRECT_STATE;
+    }
+    mTimeout = aTimeout;
+
+    return DeviceLayer::PlatformMgr().ScheduleWork(SetSamplingHandler, reinterpret_cast<intptr_t>(this));
 }
 
 CHIP_ERROR InsightsSystemMetrics::RegisterAndEnable(chip::System::Clock::Timeout aTimeout)
@@ -138,13 +137,14 @@ CHIP_ERROR InsightsSystemMetrics::RegisterAndEnable(chip::System::Clock::Timeout
         if (err != ESP_OK)
         {
             ESP_LOGE(kTag, "Failed to register metric:%s, err:%d", mLabels[i], err);
+            Unregister();
             return CHIP_ERROR_INCORRECT_STATE;
         }
     }
 
     mTimeout = System::Clock::Milliseconds32(aTimeout);
 
-    CHIP_ERROR err = DeviceLayer::SystemLayer().StartTimer(GetSamplingInterval(), SamplingIntervalHandler, this);
+    CHIP_ERROR err = DeviceLayer::SystemLayer().StartTimer(GetSamplingInterval(), SamplingHandler, this);
     if (err != CHIP_NO_ERROR)
     {
         ESP_LOGE(kTag, "Failed to start the timer, err:%" CHIP_ERROR_FORMAT, err.Format());

diff --git a/src/tracing/esp32_trace/include/matter/tracing/insights_sys_stats.h b/src/tracing/esp32_trace/include/matter/tracing/insights_sys_stats.h
@@ -60,8 +60,8 @@ class InsightsSystemMetrics
     System::Clock::Timeout mTimeout;
     char * mLabels[chip::System::Stats::kNumEntries];
 
-    static void SamplingIntervalHandler(System::Layer * systemLayer, void * context);
-    static void CancelSamplingInterval(intptr_t arg);
+    static void SamplingHandler(System::Layer * systemLayer, void * context);
+    static void SetSamplingHandler(intptr_t arg);
 };
 
 } // namespace Stats