Merge branch 'master' into bugfix/fixing_code_bugs

Author: chirag-silabs

.github/workflows/darwin.yaml

@@ -105,10 +105,9 @@ jobs:
               run: |
                   scripts/examples/gn_build_example.sh examples/ota-requestor-app/linux out/debug/ota-requestor-app chip_config_network_layer_ble=false non_spec_compliant_ota_action_delay_floor=0
             - name: Run Framework Tests
-              # For now disable unguarded-availability-new warnings because we
-              # internally use APIs that we are annotating as only available on
-              # new enough versions.  Maybe we should change out deployment
-              # target versions instead?
+              # We want to ensure that our log upload runs on timeout, so use a timeout here shorter
+              # than the 6-hour overall job timeout.  4.5 hours should be plenty.
+              timeout-minutes: 270
               working-directory: src/darwin/Framework
               run: |
                   mkdir -p /tmp/darwin/framework-tests

examples/chip-tool/templates/logging/DataModelLogger-src.zapt

@@ -1,6 +1,7 @@
 {{> header}}
 
 #include <commands/clusters/DataModelLogger.h>
+#include <zap-generated/cluster/logging/EntryToText.h>
 
 using namespace chip::app::Clusters;
 

examples/chip-tool/templates/logging/EntryToText-src.zapt

@@ -81,4 +81,14 @@ char const * GeneratedCommandIdToText(chip::ClusterId cluster, chip::CommandId i
 {{/zcl_clusters}}
 		default: return "Unknown";
 	}
+}
+
+char const * DeviceTypeIdToText(chip::DeviceTypeId id) {
+	switch(id)
+	{
+{{#zcl_device_types}}
+		case {{asHex code 8}}: return "{{caption}}";
+{{/zcl_device_types}}
+		default: return "Unknown";
+	}
 }

examples/chip-tool/templates/logging/EntryToText.zapt

@@ -10,4 +10,6 @@ char const * AttributeIdToText(chip::ClusterId cluster, chip::AttributeId id);
 
 char const * AcceptedCommandIdToText(chip::ClusterId cluster, chip::CommandId id);
 
-char const * GeneratedCommandIdToText(chip::ClusterId cluster, chip::CommandId id);
+char const * GeneratedCommandIdToText(chip::ClusterId cluster, chip::CommandId id);
+
+char const * DeviceTypeIdToText(chip::DeviceTypeId id);

examples/chip-tool/templates/partials/StructLoggerImpl.zapt

@@ -3,12 +3,34 @@ CHIP_ERROR DataModelLogger::LogValue(const char * label, size_t indent, const ch
   DataModelLogger::LogString(label, indent, "{");
 {{#zcl_struct_items}}
   {
+{{#if (isEqual type "devtype_id") }}
+{{#if isNullable }}
+      if (value.{{asLowerCamelCase label}}.IsNull())
+      {
+          CHIP_ERROR err = LogValue("{{asUpperCamelCase label}}", indent + 1, value.{{asLowerCamelCase label}});
+          if (err != CHIP_NO_ERROR)
+          {
+              DataModelLogger::LogString(indent + 1, "Struct truncated due to invalid value for '{{asUpperCamelCase label}}'");
+              return err;
+          }
+      }
+      else
+      {
+          std::string item = std::to_string(value.{{asLowerCamelCase label}}.Value()) + " (" + DeviceTypeIdToText(value.{{asLowerCamelCase label}}.Value()) + ")";
+          DataModelLogger::LogString("{{asUpperCamelCase label}}", indent + 1, item);
+      }
+{{else}}
+      std::string item = std::to_string(value.{{asLowerCamelCase label}}) + " (" + DeviceTypeIdToText(value.{{asLowerCamelCase label}}) + ")";
+      DataModelLogger::LogString("{{asUpperCamelCase label}}", indent + 1, item);
+{{/if}}
+{{else}}
       CHIP_ERROR err = LogValue("{{asUpperCamelCase label}}", indent + 1, value.{{asLowerCamelCase label}});
       if (err != CHIP_NO_ERROR)
       {
           DataModelLogger::LogString(indent + 1, "Struct truncated due to invalid value for '{{asUpperCamelCase label}}'");
           return err;
       }
+{{/if}}
   }
 {{/zcl_struct_items}}
   DataModelLogger::LogString(indent, "}");

src/app/util/attribute-storage.h

@@ -60,6 +60,12 @@ static constexpr uint16_t kEmberInvalidEndpointIndex = 0xFFFF;
     } /* cluster revision */                                                                                                       \
     }
 
+// The attrMask must contain the relevant ATTRIBUTE_MASK_* bits from
+// attribute-metadata.h.  Specifically:
+//
+// * Writable attributes must have ATTRIBUTE_MASK_WRITABLE
+// * Nullable attributes (have X in the quality column in the spec) must have ATTRIBUTE_MASK_NULLABLE
+// * Attributes that have T in the Access column in the spec must have ATTRIBUTE_MASK_MUST_USE_TIMED_WRITE
 #define DECLARE_DYNAMIC_ATTRIBUTE(attId, attType, attSizeBytes, attrMask)                                                          \
     {                                                                                                                              \
         ZAP_EMPTY_DEFAULT(), attId, attSizeBytes, ZAP_TYPE(attType), attrMask | ZAP_ATTRIBUTE_MASK(EXTERNAL_STORAGE)               \

src/darwin/Framework/CHIP/MTRDeviceController.mm

@@ -154,7 +154,11 @@ @implementation MTRDeviceController {
     MTRP256KeypairBridge _signingKeypairBridge;
     MTRP256KeypairBridge _operationalKeypairBridge;
 
-    BOOL _suspended;
+    // For now, we just ensure that access to _suspended is atomic, but don't
+    // guarantee atomicity of the the entire suspend/resume operation.  The
+    // expectation is that suspend/resume on a given controller happen on some
+    // specific queue, so can't race against each other.
+    std::atomic<bool> _suspended;
 
     // Counters to track assertion status and access controlled by the _assertionLock
     NSUInteger _keepRunningAssertionCounter;
@@ -378,9 +382,7 @@ - (BOOL)isRunning
 
 - (BOOL)isSuspended
 {
-    @synchronized(self) {
-        return _suspended;
-    }
+    return _suspended;
 }
 
 - (void)_notifyDelegatesOfSuspendState
@@ -397,48 +399,51 @@ - (void)suspend
 {
     MTR_LOG("%@ suspending", self);
 
-    @synchronized(self) {
+    NSArray * devicesToSuspend;
+    {
+        std::lock_guard lock(*self.deviceMapLock);
+        // Set _suspended under the device map lock.  This guarantees that
+        // for any given device exactly one of two things is true:
+        // * It is in the snapshot we are creating
+        // * It is created after we have changed our _suspended state.
         _suspended = YES;
+        devicesToSuspend = [self.nodeIDToDeviceMap objectEnumerator].allObjects;
+    }
 
-        NSArray * devicesToSuspend;
-        {
-            std::lock_guard lock(*self.deviceMapLock);
-            devicesToSuspend = [self.nodeIDToDeviceMap objectEnumerator].allObjects;
-        }
-
-        for (MTRDevice * device in devicesToSuspend) {
-            [device controllerSuspended];
-        }
-
-        // TODO: In the concrete class, consider what should happen with:
-        //
-        // * Active commissioning sessions (presumably close them?)
-        // * CASE sessions in general.
-        // * Possibly try to see whether we can change our fabric entry to not advertise and restart advertising.
-
-        [self _notifyDelegatesOfSuspendState];
+    MTR_LOG("%@ found %lu devices to suspend", self, static_cast<unsigned long>(devicesToSuspend.count));
+    for (MTRDevice * device in devicesToSuspend) {
+        [device controllerSuspended];
     }
+
+    // TODO: In the concrete class, consider what should happen with:
+    //
+    // * Active commissioning sessions (presumably close them?)
+    // * CASE sessions in general.
+    // * Possibly try to see whether we can change our fabric entry to not advertise and restart advertising.
+    [self _notifyDelegatesOfSuspendState];
 }
 
 - (void)resume
 {
     MTR_LOG("%@ resuming", self);
 
-    @synchronized(self) {
+    NSArray * devicesToResume;
+    {
+        std::lock_guard lock(*self.deviceMapLock);
+        // Set _suspended under the device map lock.  This guarantees that
+        // for any given device exactly one of two things is true:
+        // * It is in the snapshot we are creating
+        // * It is created after we have changed our _suspended state.
         _suspended = NO;
+        devicesToResume = [self.nodeIDToDeviceMap objectEnumerator].allObjects;
+    }
 
-        NSArray * devicesToResume;
-        {
-            std::lock_guard lock(*self.deviceMapLock);
-            devicesToResume = [self.nodeIDToDeviceMap objectEnumerator].allObjects;
-        }
-
-        for (MTRDevice * device in devicesToResume) {
-            [device controllerResumed];
-        }
-
-        [self _notifyDelegatesOfSuspendState];
+    MTR_LOG("%@ found %lu devices to resume", self, static_cast<unsigned long>(devicesToResume.count));
+    for (MTRDevice * device in devicesToResume) {
+        [device controllerResumed];
     }
+
+    [self _notifyDelegatesOfSuspendState];
 }
 
 - (BOOL)matchesPendingShutdownControllerWithOperationalCertificate:(nullable MTRCertificateDERBytes)operationalCertificate andRootCertificate:(nullable MTRCertificateDERBytes)rootCertificate
@@ -859,7 +864,7 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams
             //
             // Note that this is just an optimization to avoid throwing the information away and immediately
             // re-reading it from storage.
-            dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t) (kSecondsToWaitBeforeAPIClientRetainsMTRDevice * NSEC_PER_SEC)), self.chipWorkQueue, ^{
+            dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t) (kSecondsToWaitBeforeAPIClientRetainsMTRDevice * NSEC_PER_SEC)), dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{
                 MTR_LOG("%@ un-retain devices loaded at startup %lu", self, static_cast<unsigned long>(deviceList.count));
             });
         }];

src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm

@@ -761,7 +761,7 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams
             //
             // Note that this is just an optimization to avoid throwing the information away and immediately
             // re-reading it from storage.
-            dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t) (kSecondsToWaitBeforeAPIClientRetainsMTRDevice * NSEC_PER_SEC)), self.chipWorkQueue, ^{
+            dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t) (kSecondsToWaitBeforeAPIClientRetainsMTRDevice * NSEC_PER_SEC)), dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{
                 MTR_LOG("%@ un-retain devices loaded at startup %lu", self, static_cast<unsigned long>(deviceList.count));
             });
         }];

src/darwin/Framework/CHIP/MTRDevice_Concrete.mm

@@ -62,6 +62,7 @@ @interface MTRDevice_Concrete ()
 @property (nonatomic, readwrite) MTRDeviceState state;
 @property (nonatomic, readwrite, nullable) NSDate * estimatedStartTime;
 @property (nonatomic, readwrite, nullable, copy) NSNumber * estimatedSubscriptionLatency;
+@property (nonatomic, readwrite, assign) BOOL suspended;
 
 @end
 
@@ -395,6 +396,8 @@ - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceControlle
             }];
         }
 
+        self.suspended = controller.suspended;
+
         MTR_LOG_DEBUG("%@ init with hex nodeID 0x%016llX", self, _nodeID.unsignedLongLongValue);
     }
     return self;
@@ -719,8 +722,8 @@ - (BOOL)_subscriptionsAllowed
 {
     os_unfair_lock_assert_owner(&self->_lock);
 
-    // We should not allow a subscription for suspended controllers or device controllers over XPC.
-    return _deviceController.isSuspended == NO && ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class];
+    // We should not allow a subscription when we are suspended or for device controllers over XPC.
+    return self.suspended == NO && ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class];
 }
 
 - (void)_delegateAdded
@@ -1290,7 +1293,7 @@ - (void)_doHandleSubscriptionReset:(NSNumber * _Nullable)retryDelay
 
     os_unfair_lock_assert_owner(&_lock);
 
-    if (_deviceController.isSuspended) {
+    if (self.suspended) {
         MTR_LOG("%@ ignoring expected subscription reset on controller suspend", self);
         return;
     }
@@ -1378,7 +1381,6 @@ - (void)_reattemptSubscriptionNowIfNeededWithReason:(NSString *)reason
     }
 
     MTR_LOG("%@ reattempting subscription with reason %@", self, reason);
-    self.reattemptingSubscription = NO;
     [self _setupSubscriptionWithReason:reason];
 }
 
@@ -2302,6 +2304,10 @@ - (void)_setupSubscriptionWithReason:(NSString *)reason
 
     os_unfair_lock_assert_owner(&self->_lock);
 
+    // If we have a pending subscription reattempt, make sure it does not
+    // actually happen, since we are trying to do a subscription now.
+    self.reattemptingSubscription = NO;
+
     if (![self _subscriptionsAllowed]) {
         MTR_LOG("%@ _setupSubscription: Subscriptions not allowed. Do not set up subscription (reason: %@)", self, reason);
         return;
@@ -3991,6 +3997,7 @@ - (void)controllerSuspended
     [super controllerSuspended];
 
     std::lock_guard lock(self->_lock);
+    self.suspended = YES;
     [self _resetSubscriptionWithReasonString:@"Controller suspended"];
 
     // Ensure that any pre-existing resubscribe attempts we control don't try to
@@ -4003,6 +4010,7 @@ - (void)controllerResumed
     [super controllerResumed];
 
     std::lock_guard lock(self->_lock);
+    self.suspended = NO;
 
     if (![self _delegateExists]) {
         MTR_LOG("%@ ignoring controller resume: no delegates", self);

src/darwin/Framework/CHIPTests/MTRCommissionableBrowserTests.m

@@ -55,6 +55,7 @@ @interface DeviceScannerDelegate : NSObject <MTRCommissionableBrowserDelegate>
 @property (nonatomic, nullable) XCTestExpectation * expectation;
 @property (nonatomic) NSMutableArray<NSDictionary<NSString *, id> *> * results;
 @property (nonatomic) NSMutableArray<NSDictionary<NSString *, id> *> * removedResults;
+@property (nonatomic) BOOL expectedResultsCountReached;
 
 - (instancetype)initWithExpectation:(XCTestExpectation *)expectation;
 - (void)controller:(MTRDeviceController *)controller didFindCommissionableDevice:(MTRCommissionableBrowserResult *)device;
@@ -71,6 +72,7 @@ - (instancetype)initWithExpectation:(XCTestExpectation *)expectation
     _expectation = expectation;
     _results = [[NSMutableArray alloc] init];
     _removedResults = [[NSMutableArray alloc] init];
+    _expectedResultsCountReached = NO;
     return self;
 }
 
@@ -83,6 +85,7 @@ - (instancetype)init
     _expectation = nil;
     _results = [[NSMutableArray alloc] init];
     _removedResults = [[NSMutableArray alloc] init];
+    _expectedResultsCountReached = NO;
     return self;
 }
 
@@ -111,7 +114,14 @@ - (void)controller:(MTRDeviceController *)controller didFindCommissionableDevice
         // Ensure that we just saw the same results as our final set popping in and out if things
         // ever got removed here.
         XCTAssertEqualObjects(finalResultsSet, allResultsSet);
-        [self.expectation fulfill];
+
+        // If we have a remove and re-add after the result count reached the
+        // expected one, we can end up in this branch again.  Doing the above
+        // checks is fine, but we shouldn't double-fulfill the expectation.
+        if (self.expectedResultsCountReached == NO) {
+            self.expectedResultsCountReached = YES;
+            [self.expectation fulfill];
+        }
     }
 
     XCTAssertLessThanOrEqual(_results.count, kExpectedDiscoveredDevicesCount);

src/platform/silabs/KeyValueStoreManagerImpl.cpp

@@ -88,9 +88,9 @@ uint16_t KeyValueStoreManagerImpl::hashKvsKeyString(const char * key) const
 CHIP_ERROR KeyValueStoreManagerImpl::MapKvsKeyToNvm3(const char * key, uint16_t hash, uint32_t & nvm3Key, bool isSlotNeeded) const
 {
     CHIP_ERROR err;
-    char * strPrefix          = nullptr;
-    uint8_t firstEmptyKeySlot = kMaxEntries;
-    for (uint8_t keyIndex = 0; keyIndex < kMaxEntries; keyIndex++)
+    char * strPrefix           = nullptr;
+    uint16_t firstEmptyKeySlot = kMaxEntries;
+    for (uint16_t keyIndex = 0; keyIndex < kMaxEntries; keyIndex++)
     {
         if (mKvsKeyMap[keyIndex] == hash)
         {
@@ -165,7 +165,7 @@ void KeyValueStoreManagerImpl::ScheduleKeyMapSave(void)
         Commit the key map in nvm once it as stabilized.
     */
     SystemLayer().StartTimer(
-        std::chrono::duration_cast<System::Clock::Timeout>(System::Clock::Seconds32(SILABS_KVS_SAVE_DELAY_SECONDS)),
+        std::chrono::duration_cast<System::Clock::Timeout>(System::Clock::Seconds32(SL_KVS_SAVE_DELAY_SECONDS)),
         KeyValueStoreManagerImpl::OnScheduledKeyMapSave, NULL);
 }
 
@@ -247,7 +247,7 @@ CHIP_ERROR KeyValueStoreManagerImpl::_Delete(const char * key)
 void KeyValueStoreManagerImpl::ErasePartition(void)
 {
     // Iterate over all the Matter Kvs nvm3 records and delete each one...
-    for (uint32_t nvm3Key = SilabsConfig::kMinConfigKey_MatterKvs; nvm3Key <= SilabsConfig::kMaxConfigKey_MatterKvs; nvm3Key++)
+    for (uint32_t nvm3Key = SilabsConfig::kMinConfigKey_MatterKvs; nvm3Key <= SilabsConfig::kConfigKey_KvsLastKeySlot; nvm3Key++)
     {
         SilabsConfig::ClearConfigValue(nvm3Key);
     }