[ICD]Add a ShortIdleModeDuration to the ICDConfigurationData. (project-chip#42115)

* Add a ShortIdleModeDuration To the ICDConfigurationData. This is used for LIT capable device operating in SIT.

* Add new ICD unit test to validate the ShortIdleTimeDuration configuration and behaviour. fix typos, and wrong function replaces in ICDManager test

* Fix sl_short_idle_mode_duration_s default arg value. Use shortIdleInterval silabs and linux icd-lit-app. Add option to configure shortIdleInterval with Linux shell and in Linux AppMain

* Apply clang-tidy [atoi -> stroul] to the now analysed Linux/Options.cpp. Restore active and Idle duration after new ICD tests

* fixup

* correct ICD report interval negotiation to keep the current behaviour. Cleanup the code and add comments to explain the logic

* apply copilot suggestions

Author: jmartinez-silabs

examples/lit-icd-app/linux/include/CHIPProjectAppConfig.h

@@ -38,5 +38,6 @@
 
 // ICD configurations
 #define CHIP_CONFIG_ICD_IDLE_MODE_DURATION_SEC 3600
+#define CHIP_CONFIG_ICD_SHORT_IDLE_MODE_DURATION_SEC 60
 #define CHIP_CONFIG_ICD_ACTIVE_MODE_DURATION_MS 10000
 #define CHIP_CONFIG_ICD_ACTIVE_MODE_THRESHOLD_MS 5000

examples/lit-icd-app/silabs/openthread.gni

@@ -41,5 +41,6 @@ sl_transport_active_interval_ms = 1000  # 1000ms Active Polling Interval
 
 # ICD Matter Configuration flags
 sl_idle_mode_duration_s = 3600  # 60min Idle Mode Duration
+sl_short_idle_mode_duration_s = 60  # 1min Short Idle Mode Duration
 sl_active_mode_duration_ms = 0  # 0 Active Mode Duration
 sl_active_mode_threshold_ms = 5000  # 5s Active Mode Threshold

examples/platform/linux/AppMain.cpp

@@ -729,10 +729,19 @@ int ChipLinuxAppInit(int argc, char * const argv[], OptionSet * customOptions,
 
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
     if (LinuxDeviceOptions::GetInstance().icdActiveModeDurationMs.HasValue() ||
-        LinuxDeviceOptions::GetInstance().icdIdleModeDurationMs.HasValue())
+        LinuxDeviceOptions::GetInstance().icdIdleModeDurationMs.HasValue() ||
+        LinuxDeviceOptions::GetInstance().shortIdleModeDurationS.has_value())
     {
-        err = Server::GetInstance().GetICDManager().SetModeDurations(LinuxDeviceOptions::GetInstance().icdActiveModeDurationMs,
-                                                                     LinuxDeviceOptions::GetInstance().icdIdleModeDurationMs);
+        // Convert icdIdleModeDurationMs to seconds for SetModeDurations api
+        std::optional<System::Clock::Seconds32> tmpIdleModeDuration = std::nullopt;
+        if (LinuxDeviceOptions::GetInstance().icdIdleModeDurationMs.HasValue())
+            tmpIdleModeDuration = std::chrono::duration_cast<System::Clock::Seconds32>(
+                LinuxDeviceOptions::GetInstance().icdIdleModeDurationMs.Value());
+
+        err = Server::GetInstance().GetICDManager().SetModeDurations(
+            LinuxDeviceOptions::GetInstance().icdActiveModeDurationMs.std_optional(), tmpIdleModeDuration,
+            LinuxDeviceOptions::GetInstance().shortIdleModeDurationS);
+
         if (err != CHIP_NO_ERROR)
         {
             ChipLogError(NotSpecified, "Invalid arguments to set ICD mode durations");

examples/platform/linux/Options.cpp

@@ -144,6 +144,7 @@ enum
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
     kDeviceOption_icdActiveModeDurationMs,
     kDeviceOption_icdIdleModeDuration,
+    kDeviceOption_icdShortIdleModeDuration,
 #endif
 #if ENABLE_CAMERA_SERVER
     kDeviceOption_Camera_DeferredOffer,
@@ -247,6 +248,7 @@ OptionDef sDeviceOptionDefs[] = {
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
     { "icdActiveModeDurationMs", kArgumentRequired, kDeviceOption_icdActiveModeDurationMs },
     { "icdIdleModeDuration", kArgumentRequired, kDeviceOption_icdIdleModeDuration },
+    { "icdShortIdleModeDuration", kArgumentRequired, kDeviceOption_icdShortIdleModeDuration },
 #endif
 #if ENABLE_CAMERA_SERVER
     { "camera-deferred-offer", kNoArgument, kDeviceOption_Camera_DeferredOffer },
@@ -681,18 +683,18 @@ bool HandleOption(const char * aProgram, OptionSet * aOptions, int aIdentifier,
 
 #if CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE || CHIP_DEVICE_ENABLE_PORT_PARAMS
     case kDeviceOption_SecuredDevicePort:
-        LinuxDeviceOptions::GetInstance().securedDevicePort = static_cast<uint16_t>(atoi(aValue));
+        LinuxDeviceOptions::GetInstance().securedDevicePort = static_cast<uint16_t>(strtoul(aValue, nullptr, 0));
         break;
 
     case kDeviceOption_UnsecuredCommissionerPort:
-        LinuxDeviceOptions::GetInstance().unsecuredCommissionerPort = static_cast<uint16_t>(atoi(aValue));
+        LinuxDeviceOptions::GetInstance().unsecuredCommissionerPort = static_cast<uint16_t>(strtoul(aValue, nullptr, 0));
         break;
 
 #endif
 
 #if CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE
     case kDeviceOption_SecuredCommissionerPort:
-        LinuxDeviceOptions::GetInstance().securedCommissionerPort = static_cast<uint16_t>(atoi(aValue));
+        LinuxDeviceOptions::GetInstance().securedCommissionerPort = static_cast<uint16_t>(strtoul(aValue, nullptr, 0));
         break;
     case kCommissionerOption_FabricID:
         LinuxDeviceOptions::GetInstance().commissionerFabricId = static_cast<chip::FabricId>(strtoull(aValue, nullptr, 0));
@@ -717,21 +719,21 @@ bool HandleOption(const char * aProgram, OptionSet * aOptions, int aIdentifier,
 
     case kDeviceOption_InterfaceId:
         LinuxDeviceOptions::GetInstance().interfaceId =
-            Inet::InterfaceId(static_cast<chip::Inet::InterfaceId::PlatformType>(atoi(aValue)));
+            Inet::InterfaceId(static_cast<chip::Inet::InterfaceId::PlatformType>(strtoul(aValue, nullptr, 0)));
         break;
 
 #if CHIP_CONFIG_TRANSPORT_TRACE_ENABLED
     case kDeviceOption_TraceFile:
         LinuxDeviceOptions::GetInstance().traceStreamFilename.SetValue(std::string{ aValue });
         break;
     case kDeviceOption_TraceLog:
-        if (atoi(aValue) != 0)
+        if (strtoul(aValue, nullptr, 0) != 0)
         {
             LinuxDeviceOptions::GetInstance().traceStreamToLogEnabled = true;
         }
         break;
     case kDeviceOption_TraceDecode:
-        if (atoi(aValue) != 0)
+        if (strtoul(aValue, nullptr, 0) != 0)
         {
             LinuxDeviceOptions::GetInstance().traceStreamDecodeEnabled = true;
         }
@@ -807,15 +809,16 @@ bool HandleOption(const char * aProgram, OptionSet * aOptions, int aIdentifier,
         break;
 #if defined(PW_RPC_ENABLED)
     case kOptionRpcServerPort:
-        LinuxDeviceOptions::GetInstance().rpcServerPort = static_cast<uint16_t>(atoi(aValue));
+        LinuxDeviceOptions::GetInstance().rpcServerPort = static_cast<uint16_t>(strtoul(aValue, nullptr, 0));
         break;
 #endif
 #if CONFIG_BUILD_FOR_HOST_UNIT_TEST
     case kDeviceOption_SubscriptionCapacity:
-        LinuxDeviceOptions::GetInstance().subscriptionCapacity = static_cast<int32_t>(atoi(aValue));
+        LinuxDeviceOptions::GetInstance().subscriptionCapacity = static_cast<int32_t>(strtoul(aValue, nullptr, 0));
         break;
     case kDeviceOption_SubscriptionResumptionRetryIntervalSec:
-        LinuxDeviceOptions::GetInstance().subscriptionResumptionRetryIntervalSec = static_cast<int32_t>(atoi(aValue));
+        LinuxDeviceOptions::GetInstance().subscriptionResumptionRetryIntervalSec =
+            static_cast<int32_t>(strtoul(aValue, nullptr, 0));
         break;
     case kDeviceOption_IdleRetransmitTimeout: {
         auto mrpConfig            = GetLocalMRPConfig().ValueOr(GetDefaultMRPConfig());
@@ -881,12 +884,12 @@ bool HandleOption(const char * aProgram, OptionSet * aOptions, int aIdentifier,
 #endif
 #if CHIP_CONFIG_TERMS_AND_CONDITIONS_REQUIRED
     case kDeviceOption_TermsAndConditions_Version: {
-        LinuxDeviceOptions::GetInstance().tcVersion.SetValue(static_cast<uint16_t>(atoi(aValue)));
+        LinuxDeviceOptions::GetInstance().tcVersion.SetValue(static_cast<uint16_t>(strtoul(aValue, nullptr, 0)));
         break;
     }
 
     case kDeviceOption_TermsAndConditions_Required: {
-        LinuxDeviceOptions::GetInstance().tcRequired.SetValue(static_cast<uint16_t>(atoi(aValue)));
+        LinuxDeviceOptions::GetInstance().tcRequired.SetValue(static_cast<uint16_t>(strtoul(aValue, nullptr, 0)));
         break;
     }
 #endif
@@ -913,11 +916,35 @@ bool HandleOption(const char * aProgram, OptionSet * aOptions, int aIdentifier,
         }
         else
         {
-            // Covert from seconds to mini seconds
+            // Covert from seconds to milli seconds
             LinuxDeviceOptions::GetInstance().icdIdleModeDurationMs.SetValue(chip::System::Clock::Milliseconds32(value * 1000));
         }
         break;
     }
+    case kDeviceOption_icdShortIdleModeDuration: {
+        uint32_t value = static_cast<uint32_t>(strtoul(aValue, nullptr, 0));
+        if ((value < 1) || (value > 86400))
+        {
+            PrintArgError("%s: invalid value specified for icdShortIdleModeDuration: %s\n", aProgram, aValue);
+            retval = false;
+        }
+        else
+        {
+            if (LinuxDeviceOptions::GetInstance().icdIdleModeDurationMs.HasValue() &&
+                (value > std::chrono::duration_cast<System::Clock::Seconds32>(
+                             LinuxDeviceOptions::GetInstance().icdIdleModeDurationMs.Value())
+                             .count()))
+            {
+                PrintArgError("%s: icdShortIdleModeDuration value (%s) must be <= icdIdleModeDuration\n", aProgram, aValue);
+                retval = false;
+            }
+            else
+            {
+                LinuxDeviceOptions::GetInstance().shortIdleModeDurationS = chip::System::Clock::Seconds32(value);
+            }
+        }
+        break;
+    }
 #endif
 #if ENABLE_CAMERA_SERVER
     case kDeviceOption_Camera_DeferredOffer: {

examples/platform/linux/Options.h

@@ -25,6 +25,7 @@
 #pragma once
 
 #include <cstdint>
+#include <optional>
 #include <string>
 #include <vector>
 
@@ -108,6 +109,7 @@ struct LinuxDeviceOptions
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
     chip::Optional<chip::System::Clock::Milliseconds32> icdActiveModeDurationMs;
     chip::Optional<chip::System::Clock::Milliseconds32> icdIdleModeDurationMs;
+    std::optional<chip::System::Clock::Seconds32> shortIdleModeDurationS;
 #endif
     chip::Optional<std::string> vendorName;
     chip::Optional<std::string> productName;

examples/platform/silabs/MatterConfig.cpp

@@ -403,7 +403,7 @@ extern "C" void sl_matter_em4_check(uint32_t expected_idle_time_ms)
 {
     if (chip::ICDConfigurationData::GetInstance().GetICDMode() == chip::ICDConfigurationData::ICDMode::LIT)
     {
-        uint32_t idleDuration_seconds = chip::ICDConfigurationData::GetInstance().GetIdleModeDuration().count();
+        uint32_t idleDuration_seconds = chip::ICDConfigurationData::GetInstance().GetModeBasedIdleModeDuration().count();
         uint32_t threshold_ms         = idleDuration_seconds * SL_EM4_THRESHOLD_PERCENTAGE * 10;
         // Since the sleep timer will never match the actual idle time (hardware latency, etc), we set a threshold
         // Multiply by 10 to converts seconds into milliseconds (e.g. 90% of 1000sec in ms is 1000*90*10 = 900000ms)

src/app/ReadHandler.cpp

@@ -773,52 +773,40 @@ CHIP_ERROR ReadHandler::ProcessSubscribeRequest(System::PacketBufferHandle && aP
 
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
 
-    // Default behavior for ICDs where the wanted MaxInterval for a subscription is the IdleModeDuration
-    // defined in the ICD Management Cluster.
-    // Behavior can be changed with the OnSubscriptionRequested function defined in the application callbacks
+    // To optimize ICD sleep/idle cycles, it is preferred to synchronize the subscription report interval
+    // with the IdleModeDuration defined in the ICD Management Cluster.
+    // We aim to use IdleModeDuration, but must respect the Min Interval Floor and Max Interval Ceiling.
+    // Note: This behavior can be overridden using the OnSubscriptionRequested callback defined
+    // in the application.
 
-    // Default Behavior Steps :
-    // If MinInterval > IdleModeDuration, try to set the MaxInterval to the first interval of IdleModeDurations above the
-    // MinInterval.
-    // If the next interval is greater than the MaxIntervalCeiling, use the MaxIntervalCeiling.
-    // Otherwise, use IdleModeDuration as MaxInterval
+    // Per spec and static checks, GetIdleModeDuration max value fits a uint16_t.
+    // The final selected interval must be a uint16_t but we use a uint32 here during calculation to prevent overflows.
+    uint32_t preferredMaxInterval = ICDConfigurationData::GetInstance().GetIdleModeDuration().count();
+    // Determine the max interval ceiling between GetPublisherSelectedIntervalLimit
+    // and the subscriber-requested MaxInterval.
+    uint16_t maxIntervalCeiling = std::max(GetPublisherSelectedIntervalLimit(), mMaxInterval);
 
-    uint32_t decidedMaxInterval = ICDConfigurationData::GetInstance().GetIdleModeDuration().count();
-
-    // Check if the PublisherSelectedIntervalLimit is 0. If so, set decidedMaxInterval to MaxIntervalCeiling
-    if (decidedMaxInterval == 0)
+    // Spec doesn't allow IdleModeDuration of 0 but make sure here to prevent div0
+    if (preferredMaxInterval == 0)
     {
-        decidedMaxInterval = mMaxInterval;
+        preferredMaxInterval = maxIntervalCeiling;
     }
-
-    // If requestedMinInterval is greater than the IdleTimeInterval, select next active up time as max interval
-    if (mMinIntervalFloorSeconds > decidedMaxInterval)
+    else
     {
-        uint16_t ratio = mMinIntervalFloorSeconds / static_cast<uint16_t>(decidedMaxInterval);
-        if (mMinIntervalFloorSeconds % decidedMaxInterval)
+        // Must respect the minimal interval floor
+        if (preferredMaxInterval < mMinIntervalFloorSeconds)
         {
-            ratio++;
+            // Round up to the nearest multiple of IdleModeDuration that is >= mMinIntervalFloorSeconds.
+            const uint32_t remainder = mMinIntervalFloorSeconds % preferredMaxInterval;
+            preferredMaxInterval =
+                remainder == 0 ? mMinIntervalFloorSeconds : (mMinIntervalFloorSeconds + (preferredMaxInterval - remainder));
         }
 
-        decidedMaxInterval *= ratio;
-    }
-
-    // Verify that decidedMaxInterval is an acceptable value (overflow)
-    if (decidedMaxInterval > System::Clock::Seconds16::max().count())
-    {
-        decidedMaxInterval = System::Clock::Seconds16::max().count();
+        // Use the smallest interval between preferredMaxInterval and maxIntervalCeiling.
+        // This also ensure that no overflow occurs when casting to uint16_t below
+        preferredMaxInterval = std::min<uint32_t>(preferredMaxInterval, maxIntervalCeiling);
     }
-
-    // Verify that the decidedMaxInterval respects MAX(GetPublisherSelectedIntervalLimit(), MaxIntervalCeiling)
-    uint16_t maximumMaxInterval = std::max(GetPublisherSelectedIntervalLimit(), mMaxInterval);
-    if (decidedMaxInterval > maximumMaxInterval)
-    {
-        decidedMaxInterval = maximumMaxInterval;
-    }
-
-    // Set max interval of the subscription
-    mMaxInterval = static_cast<uint16_t>(decidedMaxInterval);
-
+    mMaxInterval = static_cast<uint16_t>(preferredMaxInterval);
 #endif // CHIP_CONFIG_ENABLE_ICD_SERVER
 
     //

src/app/icd/server/ICDConfigurationData.cpp

@@ -63,20 +63,62 @@ CHIP_ERROR ICDConfigurationData::SetModeDurations(Optional<System::Clock::Millis
     VerifyOrReturnError(activeModeDuration.HasValue() || idleModeDuration.HasValue(), CHIP_ERROR_INVALID_ARGUMENT);
 
     // Convert idleModeDuration to seconds for the correct precision
-    System::Clock::Seconds32 tmpIdleModeDuration = idleModeDuration.HasValue()
-        ? std::chrono::duration_cast<System::Clock::Seconds32>(idleModeDuration.Value())
-        : mIdleModeDuration;
+    std::optional<System::Clock::Seconds32> tmpIdleModeDuration = std::nullopt;
+    if (idleModeDuration.HasValue())
+        tmpIdleModeDuration = std::chrono::duration_cast<System::Clock::Seconds32>(idleModeDuration.Value());
 
-    System::Clock::Milliseconds32 tmpActiveModeDuration = activeModeDuration.ValueOr(mActiveModeDuration);
+    // Here we set shortIdleModeDuration the same as idleModeDuration to maintain the api previous behaviour
+    return SetModeDurations(activeModeDuration.std_optional(), tmpIdleModeDuration, tmpIdleModeDuration);
+}
+
+CHIP_ERROR ICDConfigurationData::SetModeDurations(std::optional<System::Clock::Milliseconds32> activeModeDuration,
+                                                  std::optional<System::Clock::Seconds32> idleModeDuration,
+                                                  std::optional<System::Clock::Seconds32> shortIdleModeDuration)
+{
+    VerifyOrReturnError(activeModeDuration.has_value() || idleModeDuration.has_value() || shortIdleModeDuration.has_value(),
+                        CHIP_ERROR_INVALID_ARGUMENT);
+
+    System::Clock::Milliseconds32 tmpActiveModeDuration = activeModeDuration.value_or(mActiveModeDuration);
+    System::Clock::Seconds32 tmpIdleModeDuration        = idleModeDuration.value_or(mIdleModeDuration);
 
     VerifyOrReturnError(tmpActiveModeDuration <= tmpIdleModeDuration, CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(tmpIdleModeDuration <= kMaxIdleModeDuration, CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(tmpIdleModeDuration >= kMinIdleModeDuration, CHIP_ERROR_INVALID_ARGUMENT);
 
-    mIdleModeDuration   = tmpIdleModeDuration;
-    mActiveModeDuration = tmpActiveModeDuration;
+    System::Clock::Seconds32 tmpShortIdleModeDuration;
+    if (shortIdleModeDuration.has_value())
+    {
+        // shortIdleModeDuration was provided, it shall be lesser than or equal to idleModeDuration.
+        tmpShortIdleModeDuration = shortIdleModeDuration.value();
+    }
+    else
+    {
+        // shortIdleModeDuration was not provided. To ensure correct mode transitions and device compliance,
+        // shortIdleModeDuration must not exceed idleModeDuration, so we use the smaller of the current shortIdleModeDuration
+        // and the resultant idleModeDuration.
+        // This approach overwrites a previous valid shortIdleModeDuration rather than erroring the call but maintains the previous
+        // api behavior.
+        tmpShortIdleModeDuration = std::min(mShortIdleModeDuration, tmpIdleModeDuration);
+    }
+
+    VerifyOrReturnError(tmpShortIdleModeDuration <= tmpIdleModeDuration, CHIP_ERROR_INVALID_ARGUMENT);
+
+    mIdleModeDuration      = tmpIdleModeDuration;
+    mActiveModeDuration    = tmpActiveModeDuration;
+    mShortIdleModeDuration = tmpShortIdleModeDuration;
 
     return CHIP_NO_ERROR;
 }
 
+bool ICDConfigurationData::ShouldUseShortIdle()
+{
+    VerifyOrReturnValue(mShortIdleModeDuration < mIdleModeDuration, false);
+    return (mFeatureMap.Has(app::Clusters::IcdManagement::Feature::kLongIdleTimeSupport) && mICDMode == ICDMode::SIT);
+}
+
+System::Clock::Seconds32 ICDConfigurationData::GetModeBasedIdleModeDuration()
+{
+    return ShouldUseShortIdle() ? mShortIdleModeDuration : mIdleModeDuration;
+}
+
 } // namespace chip